From 8f029aa70bca574497c84216423295336ce197cb Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Sat, 17 Oct 2026 12:59:43 +0200 Subject: [PATCH] R-1126: the gate publishes the container it checked, by id (TOCTOU from the commit's security review) web.SyncFileBrowserMounts can re-create FileBrowser between the check and the connect; a connect by name would publish the new, unchecked container. The run is now " " and the connect targets that id. TestFileBrowserGate_ConnectsTheCheckedContainerById, red-proved. Claude-Session: https://claude.ai/code/session_0159rPz1ZhFKsS53msqPYxtS --- .../internal/stacks/filebrowser_gate.go | 13 ++++++++++-- .../internal/stacks/filebrowser_password.go | 8 ++++--- .../stacks/filebrowser_password_test.go | 21 +++++++++++++++++++ 3 files changed, 37 insertions(+), 5 deletions(-) diff --git a/controller/internal/stacks/filebrowser_gate.go b/controller/internal/stacks/filebrowser_gate.go index ed4a900..8f98f14 100644 --- a/controller/internal/stacks/filebrowser_gate.go +++ b/controller/internal/stacks/filebrowser_gate.go @@ -70,15 +70,24 @@ func (m *Manager) joinFileBrowserNetwork() { // ensureFileBrowserPublic joins FileBrowser to traefik-public only when THIS run was checked and its default login is // refused. Anything less — the run unknown, not checked yet, the check failed — leaves it unreachable from outside. +// +// TOCTOU (found by the commit's security review, 2026-10-17): the connect targets the CONTAINER ID read with the run, +// never the name — web.SyncFileBrowserMounts can re-create FileBrowser between the check and the connect, and a +// connect by name would publish the new, unchecked container. By id, a replaced container's connect simply fails. +// Pinned by TestFileBrowserGate_ConnectsTheCheckedContainerById. func (m *Manager) ensureFileBrowserPublic() { run := m.fbStartedAt() if run == "" || run != m.fbProbedRun { return } - if m.fbNetOn(fileBrowserContainer, traefikNetwork) { + target := fileBrowserContainer // tests' runs carry no id + if f := strings.Fields(run); len(f) == 2 { + target = f[0] + } + if m.fbNetOn(target, traefikNetwork) { return } - if err := m.fbNetConnect(fileBrowserContainer, traefikNetwork); err != nil { + if err := m.fbNetConnect(target, traefikNetwork); err != nil { m.logger.Printf("[WARN] [infra] filebrowser: could not join %s (files. stays unreachable): %v", traefikNetwork, err) return } diff --git a/controller/internal/stacks/filebrowser_password.go b/controller/internal/stacks/filebrowser_password.go index b91904d..68cd15f 100644 --- a/controller/internal/stacks/filebrowser_password.go +++ b/controller/internal/stacks/filebrowser_password.go @@ -76,8 +76,10 @@ func fbLogin(do fbHTTPDo, base, password string) (string, int, error) { return strings.Trim(strings.TrimSpace(string(b)), `"`), resp.StatusCode, nil } -// fbStartedAt identifies FileBrowser's current run (its container's .State.StartedAt); "" when unknown. Tests set -// fbStarted; with a test FileBrowser (fbHTTP / fbBaseURL) and no fbStarted it is "" — a test never reaches docker. +// fbStartedAt identifies FileBrowser's current run: " <.State.StartedAt>" (R-1126: the gate connects that +// exact container id, so a container re-created between the check and the connect is never published). "" when +// unknown. Tests set fbStarted; with a test FileBrowser (fbHTTP / fbBaseURL) and no fbStarted it is "" — a test never +// reaches docker. func (m *Manager) fbStartedAt() string { if m.fbStarted != nil { return m.fbStarted() @@ -85,7 +87,7 @@ func (m *Manager) fbStartedAt() string { if m.fbHTTP != nil || m.fbBaseURL != "" { return "" } - out, err := dockerexec.Command("docker", "inspect", "--format", "{{.State.StartedAt}}", fileBrowserContainer).Output() + out, err := dockerexec.Command("docker", "inspect", "--format", "{{.Id}} {{.State.StartedAt}}", fileBrowserContainer).Output() if err != nil { return "" } diff --git a/controller/internal/stacks/filebrowser_password_test.go b/controller/internal/stacks/filebrowser_password_test.go index 6016291..a411a9e 100644 --- a/controller/internal/stacks/filebrowser_password_test.go +++ b/controller/internal/stacks/filebrowser_password_test.go @@ -296,3 +296,24 @@ func TestFileBrowserGate_PublishedOnlyAfterTheCheck(t *testing.T) { t.Fatalf("a checked restart did more: %v", events) } } + +// R-1126 TOCTOU: the gate connects the container whose run it checked, by ID — so a FileBrowser re-created between the +// check and the connect (web.SyncFileBrowserMounts runs outside infraMu) is never the one published. +// RED-PROOF (2026-10-17): connecting by the name „filebrowser" fails this test. +func TestFileBrowserGate_ConnectsTheCheckedContainerById(t *testing.T) { + fb := &fakeFB{password: "admin"} + srv := httptest.NewServer(fb.handler()) + defer srv.Close() + m, _, _ := fbManager(t, srv.URL) + m.fbStarted = func() string { return "3f2a9c 2026-10-17T11:20:00Z" } + var connected []string + m.fbNetOnFn = func(c, n string) bool { return false } + m.fbNetConnectFn = func(c, n string) error { + connected = append(connected, c+"@"+n) + return nil + } + m.fileBrowserGateLocked() + if len(connected) != 2 || connected[1] != "3f2a9c@traefik-public" { + t.Fatalf("the checked container must be published by its id, got %v", connected) + } +}