From 601bdba25351720569356842fb4bf557c4dae4c2 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Mon, 19 Oct 2026 05:03:57 +0200 Subject: [PATCH] v0.332.0 (live rc.2 + security review): the pass redirect is absolute; no pass for a cross-site request; no dot-segments in to (R-1100) --- CHANGELOG.md | 9 +++++ controller/internal/web/files_sso.go | 26 ++++++++++++- controller/internal/web/files_sso_test.go | 45 ++++++++++++++++++++++- 3 files changed, 76 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e8f6d2..037c695 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,15 @@ brief of 2026-10-19 (felhom.eu `REPORT-day-2026-10-19.md`); design: felhom.eu `a `TestFileBrowserConfig_R1100_TrustsOnlyTheBoxSecret`, `_HeaderUserGetsTheHouseholdsRights`, `TestFileBrowserRoute_R1100_AsksTheControllerAndStripsTheHeader` (each red-proved). Link tests now read the folder from the pass link's `to`. i18n fixtures: none changed (no fixture page shows a folder link or the note). + **Hardening after the live rc.2 walk and a security review (same release):** the pass answer's redirect to the folder + is ABSOLUTE (`https://files./…`) — traefik makes a relative Location from a forwardAuth answer absolute against + the controller's own address (rc.2 on 9201 sent the browser to `http://felhom-controller:8080/files/`); `/__files/open` + mints no pass for a request another site started (`Sec-Fetch-Site: cross-site` → the launcher, a login-CSRF into the + household's own files at a path the other site picked); `to` refuses dot-segments (`..`, `.`, also percent-encoded), + so it cannot resolve out of `/files/`. Tests `TestFilesSSO_R1100_CrossSiteNoPass` and the absolute-Location check, + red-proved („a cross-site request got a pass"; `safeFilesPath("/files/../api/x")`; `302 → "/files/HDD%202/media"`). + **Residual, stated:** FileBrowser's own token for the header user (2 h, its `tokenExpirationHours`) is cleared with + the files cookie, but a browser that lost only the files cookie keeps that token until it expires. - **The wizard's typed-back words now go to the box, not only to the browser.** The claim asks the agent to keep the new package (`{"defer_upload": true}`); an agent ≥ 0.160.0 answers with `confirm_required` and the two word diff --git a/controller/internal/web/files_sso.go b/controller/internal/web/files_sso.go index 0f75de6..1b8eb16 100644 --- a/controller/internal/web/files_sso.go +++ b/controller/internal/web/files_sso.go @@ -184,7 +184,8 @@ func (s *Server) filesCookieKey(r *http.Request) (present, ok bool) { return true, s.filesSessionAlive(parts[0]) } -// safeFilesPath accepts a FileBrowser path to go to after the pass: "/" or under /files/ — never another host. +// safeFilesPath accepts a FileBrowser path to go to after the pass: "/" or under /files/ — never another host, and never +// a dot-segment that a browser would resolve out of /files/ (onto FileBrowser's API, say). func safeFilesPath(p string) string { if p == "" || p == "/" { return "/" @@ -192,6 +193,16 @@ func safeFilesPath(p string) string { if !strings.HasPrefix(p, "/files/") || strings.HasPrefix(p, "//") || strings.ContainsAny(p, "\\\r\n") { return "/" } + path, _, _ := strings.Cut(p, "?") + dec, err := url.PathUnescape(path) + if err != nil { + return "/" + } + for _, seg := range strings.Split(dec, "/") { + if seg == ".." || seg == "." { + return "/" + } + } return p } @@ -229,6 +240,14 @@ func (s *Server) ServeFilesOpen(w http.ResponseWriter, r *http.Request) { http.Redirect(w, r, "/", http.StatusFound) return } + // A pass is minted only for the household's own press (the dashboard page: same-origin) or a typed or bookmarked + // address (none) — never for a request another site started. A browser that sends no Sec-Fetch-Site (an old one) is + // let through: the pass opens only the household's own files on this box. Pinned by TestFilesSSO_R1100_CrossSiteNoPass. + if sfs := r.Header.Get("Sec-Fetch-Site"); sfs == "cross-site" { + s.logger.Printf("[WARN] [web] files sign-in: a pass asked for from another site — refused, visitor %s", clientIP(r)) + http.Redirect(w, r, "/", http.StatusFound) + return + } c, err := r.Cookie(sessionCookieName) if err != nil || !s.isValidSession(c.Value) { http.Redirect(w, r, "/login?"+url.Values{"next": {filesOpenPath + "?" + url.Values{"to": {to}}.Encode()}}.Encode(), http.StatusFound) @@ -291,7 +310,10 @@ func (s *Server) ServeFilesAuth(w http.ResponseWriter, r *http.Request) { http.SetCookie(w, &http.Cookie{Name: fileBrowserJWTCookie, Value: "", Path: "/", Domain: host, MaxAge: -1, HttpOnly: true, Secure: true}) s.logger.Printf("[INFO] [web] files sign-in: a signed-in household browser passed — visitor %s", clientIP(r)) w.Header().Set("Cache-Control", "no-store") - http.Redirect(w, r, safeFilesPath(rd), http.StatusFound) + // ABSOLUTE, like the setup gate's: traefik makes a relative Location in a forwardAuth answer absolute against + // the AUTH server's address — measured on 9201, "/files/" became http://felhom-controller:8080/files/. + // Pinned by TestFilesSSO_R1100_HouseholdOpensTheFolderWithoutALogin. + http.Redirect(w, r, "https://"+host+safeFilesPath(rd), http.StatusFound) return } diff --git a/controller/internal/web/files_sso_test.go b/controller/internal/web/files_sso_test.go index 534b283..30bfb53 100644 --- a/controller/internal/web/files_sso_test.go +++ b/controller/internal/web/files_sso_test.go @@ -122,8 +122,13 @@ func TestFilesSSO_R1100_HouseholdOpensTheFolderWithoutALogin(t *testing.T) { } w := filesAsk(s, "files.example.hu", pass, htmlAccept) fc := filesCookieIn(w, filesCookieName) - if w.Code != http.StatusFound || w.Header().Get("Location") != folder { - t.Fatalf("the pass answer: %d → %q, want 302 → %s", w.Code, w.Header().Get("Location"), folder) + // Absolute: traefik makes a relative Location in a forwardAuth answer absolute against the controller's own + // address (measured on 9201 with rc.2: "/files/" → http://felhom-controller:8080/files/). + // + // COMPANION RED-PROOF (observed 2026-10-19, the live rc.2 shape): redirect to safeFilesPath(rd) alone → this fails + // with `the pass answer: 302 → "/files/HDD%202/media"`. Restored. + if w.Code != http.StatusFound || w.Header().Get("Location") != "https://files.example.hu"+folder { + t.Fatalf("the pass answer: %d → %q, want 302 → https://files.example.hu%s", w.Code, w.Header().Get("Location"), folder) } if fc == nil || fc.Domain != "" || !fc.HttpOnly || !fc.Secure || fc.SameSite != http.SameSiteLaxMode || fc.Path != "/" { t.Fatalf("the files cookie must be host-only, HttpOnly, Secure, Lax: %+v", fc) @@ -340,3 +345,39 @@ func TestFilesSSO_R1100_DataTabLoginNote(t *testing.T) { t.Fatal("without the one sign-in the note must stay") } } + +// A pass is never minted for a request another site started (a CSRF that would sign the household into its own files at +// a path the other site picked), and a dot-segment never leaves /files/. +// +// COMPANION RED-PROOF (observed 2026-10-19): drop the Sec-Fetch-Site check in ServeFilesOpen → this fails with "a +// cross-site request got a pass"; drop the dot-segment loop in safeFilesPath → "safeFilesPath(\"/files/../api/x\")". Restored. +func TestFilesSSO_R1100_CrossSiteNoPass(t *testing.T) { + s := newFilesServer(t) + sess := sessionCookie(s) + for _, sfs := range []string{"same-origin", "none", ""} { + r := httptest.NewRequest(http.MethodGet, "https://felhom.example.hu"+filesOpenPath+"?to=%2F", nil) + r.AddCookie(sess) + if sfs != "" { + r.Header.Set("Sec-Fetch-Site", sfs) + } + if passFrom(t, s.filesEntry(r), "files.example.hu") == "" { + t.Fatalf("Sec-Fetch-Site %q: the household's own press got no pass", sfs) + } + } + for _, sfs := range []string{"cross-site"} { + r := httptest.NewRequest(http.MethodGet, "https://felhom.example.hu"+filesOpenPath+"?to=%2F", nil) + r.AddCookie(sess) + r.Header.Set("Sec-Fetch-Site", sfs) + if passFrom(t, s.filesEntry(r), "files.example.hu") != "" { + t.Fatal("a cross-site request got a pass") + } + } + for _, bad := range []string{"/files/../api/x", "/files/%2e%2e/api/x", "/files/a/./b", "/files/%2E%2E"} { + if got := safeFilesPath(bad); got != "/" { + t.Fatalf("safeFilesPath(%q) = %q", bad, got) + } + } + if got := safeFilesPath("/files/M%C3%A9dia%20&%20k%C3%B6nyvt%C3%A1r/a..b"); got == "/" { + t.Fatal("a name with two dots inside it is a real folder name") + } +}