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)
gates / gates (push) Successful in 47s
gates / gates (push) Successful in 47s
This commit is contained in:
@@ -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
|
||||
}
|
||||
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user