v0.311.0 (before the build): the repair's commands run quietly — no household file name can reach a log or the operator event
gates / gates (push) Successful in 50s
gates / gates (push) Successful in 50s
Security review of 8d862e4: compose's shared runner logs a failed command's stdout and puts its
stderr in the error, and that error became the event's `problem`. The after_unclean_boot commands now
run through a quiet runner (nothing logged, stderr discarded) and `problem` keeps the exit code only.
TestAUB_AFailingCommandLeaksNoOutput, red-proofed.
Claude-Session: https://claude.ai/code/session_012qRErfCoiTkvDK9N5XHbzb
This commit is contained in:
@@ -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).
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user