auth: the duplicate-cookie line hides a link's token (audit A4)
Security audit 2026-10-17, finding A4 (low), stage 0. The line logged when a request carries two unprefixed session cookies printed r.URL.Path raw, so GET /confirm/<token> wrote the token to the log (reachable only with PANEL_COOKIE_SECURE=false, where a sibling host can plant the bare name). loggedPath moves from internal/web to internal/web/auth as LoggedPath (web imports auth, not the other way) and both lines - the origin check's refusal and the duplicate-cookie one - go through it. Tests: TestDuplicateCookieLineHidesALinksToken; TestLoggedPathHidesTokens moved with the function. go test ./internal/web ./internal/web/auth. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -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),
|
- **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
|
not every one an application sends: the judgement needs no more, and a
|
||||||
hundred long lines were megabytes held per connection.
|
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
|
- **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
|
three `dmarc_*` maps root creates at the start were missing from the list
|
||||||
of paths where a symbolic link is removed first.
|
of paths where a symbolic link is removed first.
|
||||||
|
|||||||
@@ -706,7 +706,9 @@ the argument.
|
|||||||
tail); TestInboundRecipientCannotWriteTheRow.
|
tail); TestInboundRecipientCannotWriteTheRow.
|
||||||
- [x] **M5.** Keep at most two `From:`/`Sender:` values; security.md:810
|
- [x] **M5.** Keep at most two `From:`/`Sender:` values; security.md:810
|
||||||
reworded. — **Opus**, 0.5 h. *Done 2026-10-17:* TestSenderKeepsTwoFromAndSenderLines.
|
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
|
- [ ] **S6.** `smtpd_etrn_restrictions = reject`, `smtpd_discard_ehlo_keywords
|
||||||
= etrn` in postfix-config.sh; architecture.md § Postfix one line. Done when
|
= etrn` in postfix-config.sh; architecture.md § Postfix one line. Done when
|
||||||
an e2e `ETRN` on 25 gets `554`. — **Opus**, 0.5 h.
|
an e2e `ETRN` on 25 gets `554`. — **Opus**, 0.5 h.
|
||||||
|
|||||||
@@ -3,9 +3,11 @@ package auth
|
|||||||
import (
|
import (
|
||||||
"database/sql"
|
"database/sql"
|
||||||
"errors"
|
"errors"
|
||||||
|
"log"
|
||||||
"net/http"
|
"net/http"
|
||||||
"net/http/httptest"
|
"net/http/httptest"
|
||||||
"net/url"
|
"net/url"
|
||||||
|
"os"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
"strings"
|
"strings"
|
||||||
"testing"
|
"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) {
|
func TestSessionTokenReadsOneCookie(t *testing.T) {
|
||||||
m := testModule(t, true)
|
m := testModule(t, true)
|
||||||
r := httptest.NewRequest(http.MethodGet, "http://panel.example.com/domains", nil)
|
r := httptest.NewRequest(http.MethodGet, "http://panel.example.com/domains", nil)
|
||||||
|
|||||||
@@ -55,7 +55,7 @@ func (m *Module) sessionToken(r *http.Request) (string, bool) {
|
|||||||
// name cannot be planted that way (see sessionCookiePrefixed).
|
// name cannot be planted that way (see sessionCookiePrefixed).
|
||||||
logf("panel: %s %s carries %d cookies named %q — treating the request as signed out; "+
|
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",
|
"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
|
return "", false
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -400,3 +400,17 @@ func loginInfo(r *http.Request) string {
|
|||||||
}
|
}
|
||||||
return ""
|
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
|
||||||
|
}
|
||||||
|
|||||||
@@ -1,4 +1,4 @@
|
|||||||
package web
|
package auth
|
||||||
|
|
||||||
import "testing"
|
import "testing"
|
||||||
|
|
||||||
@@ -12,8 +12,8 @@ func TestLoggedPathHidesTokens(t *testing.T) {
|
|||||||
"/confirm/": "/confirm/",
|
"/confirm/": "/confirm/",
|
||||||
"/account/profile": "/account/profile",
|
"/account/profile": "/account/profile",
|
||||||
} {
|
} {
|
||||||
if got := loggedPath(in); got != want {
|
if got := LoggedPath(in); got != want {
|
||||||
t.Errorf("loggedPath(%q) = %q, want %q", in, got, want)
|
t.Errorf("LoggedPath(%q) = %q, want %q", in, got, want)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -3,7 +3,8 @@ package web
|
|||||||
import (
|
import (
|
||||||
"net/http"
|
"net/http"
|
||||||
"net/url"
|
"net/url"
|
||||||
"strings"
|
|
||||||
|
"github.com/mixeme/selfpost/internal/web/auth"
|
||||||
)
|
)
|
||||||
|
|
||||||
// contentSecurityPolicy is the panel's CSP. Everything the pages load —
|
// 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".
|
// saving anything".
|
||||||
logf("panel: rejected %s %s: cross-origin request (Sec-Fetch-Site=%q Origin=%q Host=%q); "+
|
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",
|
"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)
|
http.Error(w, "cross-origin request rejected", http.StatusForbidden)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
@@ -169,15 +170,3 @@ func originAllowed(r *http.Request) bool {
|
|||||||
}
|
}
|
||||||
return u.Host == r.Host
|
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
|
|
||||||
}
|
|
||||||
|
|||||||
Reference in New Issue
Block a user