diff --git a/CHANGELOG.md b/CHANGELOG.md index 57f1dc0..d361063 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,8 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version - **The sender check keeps two `From:` and two `Sender:` lines** (audit M5), not every one an application sends: the judgement needs no more, and a hundred long lines were megabytes held per connection. +- **A link's token stays out of the duplicate-cookie log line** (audit A4), + as it already did out of the origin check's. - **The entrypoint follows no link at the DMARC maps** (audit I2): the three `dmarc_*` maps root creates at the start were missing from the list of paths where a symbolic link is removed first. diff --git a/docs/security-audit-2026-10-17.md b/docs/security-audit-2026-10-17.md index 5c7a4e6..3b7f76f 100644 --- a/docs/security-audit-2026-10-17.md +++ b/docs/security-audit-2026-10-17.md @@ -706,7 +706,9 @@ the argument. tail); TestInboundRecipientCannotWriteTheRow. - [x] **M5.** Keep at most two `From:`/`Sender:` values; security.md:810 reworded. — **Opus**, 0.5 h. *Done 2026-10-17:* TestSenderKeepsTwoFromAndSenderLines. -- [ ] **A4.** Duplicate-cookie line through `loggedPath`. — **Sonnet**, 0.25 h. +- [x] **A4.** Duplicate-cookie line through `loggedPath`. — **Sonnet**, 0.25 h. + *Done 2026-10-17:* `loggedPath` moved to `auth.LoggedPath`, used by both + lines; TestDuplicateCookieLineHidesALinksToken. - [ ] **S6.** `smtpd_etrn_restrictions = reject`, `smtpd_discard_ehlo_keywords = etrn` in postfix-config.sh; architecture.md § Postfix one line. Done when an e2e `ETRN` on 25 gets `554`. — **Opus**, 0.5 h. diff --git a/internal/web/auth/auth_test.go b/internal/web/auth/auth_test.go index f73ef93..40bd8e3 100644 --- a/internal/web/auth/auth_test.go +++ b/internal/web/auth/auth_test.go @@ -3,9 +3,11 @@ package auth import ( "database/sql" "errors" + "log" "net/http" "net/http/httptest" "net/url" + "os" "path/filepath" "strings" "testing" @@ -88,6 +90,24 @@ func TestSessionTokenRejectsDuplicates(t *testing.T) { } } +// The duplicate-cookie line names the request's path, and a link's token is +// not written to the log with it (audit 2026-10-17, A4). +func TestDuplicateCookieLineHidesALinksToken(t *testing.T) { + m := testModule(t, false) + var buf strings.Builder + log.SetOutput(&buf) + t.Cleanup(func() { log.SetOutput(os.Stderr) }) + r := httptest.NewRequest(http.MethodGet, "http://panel.example.com/confirm/the-secret-token-of-a-link", nil) + r.AddCookie(&http.Cookie{Name: "selfpost_session", Value: "planted-by-a-neighbour"}) + r.AddCookie(&http.Cookie{Name: "selfpost_session", Value: "the-real-session"}) + if _, ok := m.sessionToken(r); ok { + t.Fatal("duplicate cookies accepted") + } + if got := buf.String(); strings.Contains(got, "the-secret-token-of-a-link") || !strings.Contains(got, "GET /confirm/… carries 2 cookies") { + t.Errorf("logged %q", got) + } +} + func TestSessionTokenReadsOneCookie(t *testing.T) { m := testModule(t, true) r := httptest.NewRequest(http.MethodGet, "http://panel.example.com/domains", nil) diff --git a/internal/web/auth/handlers.go b/internal/web/auth/handlers.go index 59a1649..27b4f41 100644 --- a/internal/web/auth/handlers.go +++ b/internal/web/auth/handlers.go @@ -55,7 +55,7 @@ func (m *Module) sessionToken(r *http.Request) (string, bool) { // name cannot be planted that way (see sessionCookiePrefixed). logf("panel: %s %s carries %d cookies named %q — treating the request as signed out; "+ "another host on this domain is overwriting the session cookie, clear the cookies for the parent domain", - r.Method, r.URL.Path, n, name) + r.Method, LoggedPath(r.URL.Path), n, name) return "", false } } diff --git a/internal/web/auth/links.go b/internal/web/auth/links.go index 2e2594e..28b31c8 100644 --- a/internal/web/auth/links.go +++ b/internal/web/auth/links.go @@ -400,3 +400,17 @@ func loginInfo(r *http.Request) string { } return "" } + +// LoggedPath is a request's path as the log may hold it: the token of a +// password-reset or confirmation link is a secret, and the log is read by +// more than the person the link went to (security review, 2026-10-17). +// Every log line that names a request's path goes through it: the origin +// check's refusal and the duplicate-cookie line (audit 2026-10-17, A4). +func LoggedPath(p string) string { + for _, prefix := range []string{"/login/reset/", "/confirm/"} { + if strings.HasPrefix(p, prefix) && len(p) > len(prefix) { + return prefix + "…" + } + } + return p +} diff --git a/internal/web/loggedpath_test.go b/internal/web/auth/loggedpath_test.go similarity index 79% rename from internal/web/loggedpath_test.go rename to internal/web/auth/loggedpath_test.go index e86192f..b02514b 100644 --- a/internal/web/loggedpath_test.go +++ b/internal/web/auth/loggedpath_test.go @@ -1,4 +1,4 @@ -package web +package auth import "testing" @@ -12,8 +12,8 @@ func TestLoggedPathHidesTokens(t *testing.T) { "/confirm/": "/confirm/", "/account/profile": "/account/profile", } { - if got := loggedPath(in); got != want { - t.Errorf("loggedPath(%q) = %q, want %q", in, got, want) + if got := LoggedPath(in); got != want { + t.Errorf("LoggedPath(%q) = %q, want %q", in, got, want) } } } diff --git a/internal/web/security.go b/internal/web/security.go index 3176ee4..d5da7fc 100644 --- a/internal/web/security.go +++ b/internal/web/security.go @@ -3,7 +3,8 @@ package web import ( "net/http" "net/url" - "strings" + + "github.com/mixeme/selfpost/internal/web/auth" ) // contentSecurityPolicy is the panel's CSP. Everything the pages load — @@ -97,7 +98,7 @@ func (s *Server) secure(next http.Handler) http.Handler { // saving anything". logf("panel: rejected %s %s: cross-origin request (Sec-Fetch-Site=%q Origin=%q Host=%q); "+ "if this was a legitimate request, check that the reverse proxy passes the original Host header through", - r.Method, loggedPath(r.URL.Path), r.Header.Get("Sec-Fetch-Site"), r.Header.Get("Origin"), r.Host) + r.Method, auth.LoggedPath(r.URL.Path), r.Header.Get("Sec-Fetch-Site"), r.Header.Get("Origin"), r.Host) http.Error(w, "cross-origin request rejected", http.StatusForbidden) return } @@ -169,15 +170,3 @@ func originAllowed(r *http.Request) bool { } return u.Host == r.Host } - -// loggedPath is a request's path as the log may hold it: the token of a -// password-reset link is a secret, and the log is read by more than the -// person the link went to (security review, 2026-10-17). -func loggedPath(p string) string { - for _, prefix := range []string{"/login/reset/", "/confirm/"} { - if strings.HasPrefix(p, prefix) && len(p) > len(prefix) { - return prefix + "…" - } - } - return p -}