diff --git a/CHANGELOG.md b/CHANGELOG.md index fd094b4..774abcd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,11 @@ the repair and the household's note do not depend on it. **Catalog:** Immich's ` - **The operator:** `app_repaired_after_crash` — counts only (found / repaired / still broken / lost, the boot, the duration, a skip reason). warning when anything is lost, still broken, failed or skipped; info when all repaired; nothing when nothing was found. File names never reach a log line or an event (tests assert both). +- **Security review of `8d862e4` (fixed before the build):** the block's commands first ran through compose's shared + runner, which LOGS a failed command's stdout and puts its stderr into the error — and that error text went into the + event's `problem`. A failing lost-file check could have sent a household's file name to the log and the hub. Now the + commands run through a quiet runner (no logging; stderr discarded) and `problem` keeps the exit code only + (`TestAUB_AFailingCommandLeaksNoOutput`, red-proofed). - Tests: `internal/stacks/after_unclean_boot_test.go`, `internal/aftercrash/aftercrash_test.go`, `internal/crashboot` (`TestDecided_*`), `internal/notify/app_repaired_after_crash_test.go`, `internal/web/lost_files_note_test.go`, one new parity fixture (`app_info_lost_files`, the new card only). diff --git a/controller/internal/stacks/after_unclean_boot.go b/controller/internal/stacks/after_unclean_boot.go index 95f9b85..17ff0e9 100644 --- a/controller/internal/stacks/after_unclean_boot.go +++ b/controller/internal/stacks/after_unclean_boot.go @@ -1,17 +1,22 @@ package stacks import ( + "bytes" "encoding/json" "errors" "fmt" + "io" "io/fs" "os" + "os/exec" "path/filepath" "regexp" "sort" "strings" "syscall" "time" + + "gitea.dooplex.hu/admin/felhom-controller/internal/dockerexec" ) // ── after_unclean_boot (R-1007, `09` §3 decision 236): an app repairs the files a power cut left empty ── @@ -359,11 +364,53 @@ func (m *Manager) aubRepair(dir string, spec *AfterUncleanBootSpec, ids []string return nil } -// aubExec runs one command in the service's container (the after_load seam; tests replace afterLoadFn). The output -// may carry file names: it is returned, never logged. +// aubExec runs one command in the service's container. The output may carry the household's FILE NAMES (the lost-file +// check prints them): it is returned to the caller only — never logged, and the error carries the exit code only. +// That is why this is not afterLoadExec: compose's shared runner logs a failed command's stdout and puts its stderr in +// the error, and the error text reaches the operator event (security review of 8d862e4). Tests replace afterLoadFn. +// Pinned by TestAUB_AFailingCommandLeaksNoOutput. func (m *Manager) aubExec(dir, service string, cmd []string) (string, error) { args := append([]string{"exec", "-T", service}, cmd...) - return m.afterLoadExec(dir, args...) + var out string + var err error + if m.afterLoadFn != nil { + out, err = m.afterLoadFn(dir, args...) + } else { + out, err = m.quietComposeExec(dir, args...) + } + return out, exitOnly(err) +} + +// quietComposeExec is composeExecCustomEnv without its logging of the command, its output and its stderr. +func (m *Manager) quietComposeExec(dir string, args ...string) (string, error) { + var cmd *exec.Cmd + if m.composeCmd == "docker compose" { + cmd = dockerexec.Command("docker", append([]string{"compose"}, args...)...) + } else { + cmd = dockerexec.Command("docker-compose", args...) + } + cmd.Dir = dir + cmd.Env = m.stackEnv(dir) + var stdout bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = io.Discard + err := cmd.Run() + return stdout.String(), err +} + +// exitOnly keeps an error's exit code and drops any text it carries (stderr, output). +func exitOnly(err error) error { + if err == nil { + return nil + } + var ee *exec.ExitError + if errors.As(err, &ee) { + return fmt.Errorf("exit code %d", ee.ExitCode()) + } + if code := regexp.MustCompile(`^exit code (-?\d+)`).FindStringSubmatch(err.Error()); code != nil { + return fmt.Errorf("exit code %s", code[1]) + } + return errors.New("the command could not run") } // aubRunning: the app is up (its own containers running and healthy). Seam: aubRunningFn. diff --git a/controller/internal/stacks/after_unclean_boot_test.go b/controller/internal/stacks/after_unclean_boot_test.go index 6c9be07..cde66b8 100644 --- a/controller/internal/stacks/after_unclean_boot_test.go +++ b/controller/internal/stacks/after_unclean_boot_test.go @@ -267,3 +267,25 @@ func TestAUB_DismissHidesTheNoteKeepsTheRecord(t *testing.T) { t.Fatalf("%+v", r) } } + +// Security review of 8d862e4: a failing command's output and stderr can hold the household's file names (the lost +// check prints them). They must reach neither the log nor the result's Problem (which goes to the operator event). +// RED-PROOF: return `err` instead of `exitOnly(err)` from aubExec → the stderr name lands in Problem → fails. +func TestAUB_AFailingCommandLeaksNoOutput(t *testing.T) { + m, drive, _, logBuf := aubManager(t) + root := filepath.Join(drive, "appdata/cloudapp/thumbs") + aubWrite(t, filepath.Join(root, "u1", idA+"_thumbnail.webp"), "") + m.afterLoadFn = func(_ string, args ...string) (string, error) { + return "LOST {\"name\":\"IMG_SECRET.jpg\"}\n", errors.New("exit code 1\nstderr: ERR at IMG_SECRET.jpg") + } + res := m.RunAfterUncleanBoot("cloudapp", time.Time{}) + if res.Problem == "" { + t.Fatal("a failing run must report a problem") + } + if strings.Contains(res.Problem, "IMG_SECRET") || strings.Contains(logBuf.String(), "IMG_SECRET") { + t.Fatalf("a file name leaked: problem=%q", res.Problem) + } + if !strings.Contains(res.Problem, "exit code 1") { + t.Fatalf("the exit code must stay: %q", res.Problem) + } +}