golem15/summercms
4
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(14.2): WR-01 access log never records the URL path

Browse Source 
An unmatched request logged its path. A token route reached with another
method (405) or with a trailing slash (404) is answered by ServeMux before
any route handler records the pattern, so the share or invitation token in
the path reached the log. The record now carries method, pattern, status
and duration only.
This commit is contained in:
Jakub Zych
2026-10-12 11:17:51 +02:00
parent 18a4ceabd2
commit fbcfaaf05f
4 changed files with 43 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files

51
modules/surf/accesslog_test.go
View File

@@ -109,11 +109,14 @@ func TestAccessLogSmoke(t *testing.T) {
t.Fatalf("records = %d, want 1:\n%s", len(recs), buf.String())
}
r = recs[0]
if r["pattern"] != "" || r["status"] != float64(404) || r["path"] != "/acme/missing" {
if r["pattern"] != "" || r["status"] != float64(404) {
t.Fatalf("unmatched record = %v", r)
}
if strings.Contains(buf.String(), "secret") {
t.Fatalf("unmatched record leaks the query:\n%s", buf.String())
if _, ok := r["path"]; ok {
t.Fatalf("unmatched record carries path: %v", r)
}
if strings.Contains(buf.String(), "secret") || strings.Contains(buf.String(), "/acme/missing") {
t.Fatalf("unmatched record leaks the path or query:\n%s", buf.String())
}
off, offBuf := accessLogRouter(t, false)
@@ -235,8 +238,9 @@ func accessRawRecords(t *testing.T, buf *bytes.Buffer) []map[string]any {
// TestAccessLog: off, nothing is logged. On, one record per request with
// the matched pattern and status for a 200, a handler 404 and a recovered
// panic (500); an unmatched request gets status 404 and its path without
// the query; a locale-prefixed request still records the route pattern;
// panic (500); an unmatched request (a 404, a 405 for a route reached with
// another method, a trailing-slash 404) gets an empty pattern and no path;
// a locale-prefixed request still records the route pattern;
// duration_ms is a non-negative integer; no record carries the query
// string, a header name or value or the Authorization value; and a
// handler's http.ResponseController flush keeps working.
@@ -244,21 +248,29 @@ func TestAccessLog(t *testing.T) {
type want struct {
pattern string
status int64
path string // "" means the record must have no path
}
cases := []struct {
target string
code int
want want
}{
{"/blog/posts/7?token=query-secret", http.StatusOK, want{"GET /blog/posts/{id}", 200, ""}},
{"/blog/drafts/9?token=query-secret", http.StatusNotFound, want{"GET /blog/drafts/{id}", 404, ""}},
{"/blog/broken?token=query-secret", http.StatusInternalServerError, want{"GET /blog/broken", 500, ""}},
{"/acme/missing?token=query-secret", http.StatusNotFound, want{"", 404, "/acme/missing"}},
{"/pl/blog/posts/8?token=query-secret", http.StatusOK, want{"GET /blog/posts/{id}", 200, ""}},
{"/blog/posts/7?token=query-secret", http.StatusOK, want{"GET /blog/posts/{id}", 200}},
{"/blog/drafts/9?token=query-secret", http.StatusNotFound, want{"GET /blog/drafts/{id}", 404}},
{"/blog/broken?token=query-secret", http.StatusInternalServerError, want{"GET /blog/broken", 500}},
{"/acme/missing?token=query-secret", http.StatusNotFound, want{"", 404}},
{"/pl/blog/posts/8?token=query-secret", http.StatusOK, want{"GET /blog/posts/{id}", 200}},
// A token route reached with another method (405) or a trailing
// slash (404) matches no route: ServeMux answers before any route
// handler runs, so the path segment must not reach the log.
{"POST /blog/posts/path-secret-405", http.StatusMethodNotAllowed, want{"", 405}},
{"/blog/posts/path-secret-404/", http.StatusNotFound, want{"", 404}},
}
request := func(target string) *http.Request {
req := httptest.NewRequest(http.MethodGet, target, nil)
method := http.MethodGet
if m, rest, ok := strings.Cut(target, " "); ok {
method, target = m, rest
}
req := httptest.NewRequest(method, target, nil)
req.Header.Set("Authorization", "Bearer authz-secret-value")
req.Header.Set("X-Blog-Probe", "probe-header-value")
req.Header.Set("Cookie", "session=cookie-secret-value")
@@ -279,7 +291,8 @@ func TestAccessLog(t *testing.T) {
buf.Reset()
flushErr = context.Canceled
rec := httptest.NewRecorder()
h.ServeHTTP(rec, request(tc.target))
req := request(tc.target)
h.ServeHTTP(rec, req)
if rec.Code != tc.code {
t.Fatalf("GET %s = %d, want %d", tc.target, rec.Code, tc.code)
}
@@ -292,7 +305,7 @@ func TestAccessLog(t *testing.T) {
}
r := recs[0]
status, err := r["status"].(json.Number).Int64()
if err != nil || r["method"] != "GET" || r["pattern"] != tc.want.pattern || status != tc.want.status || r["level"] != "INFO" {
if err != nil || r["method"] != req.Method || r["pattern"] != tc.want.pattern || status != tc.want.status || r["level"] != "INFO" {
t.Fatalf("GET %s: record %v, want pattern %q status %d", tc.target, r, tc.want.pattern, tc.want.status)
}
d, ok := r["duration_ms"].(json.Number)
@@ -302,17 +315,13 @@ func TestAccessLog(t *testing.T) {
if n, err := strconv.ParseInt(d.String(), 10, 64); err != nil || n < 0 {
t.Fatalf("GET %s: duration_ms %q is not a non-negative integer", tc.target, d)
}
path, hasPath := r["path"]
if tc.want.path == "" && hasPath {
t.Fatalf("GET %s: a matched record carries path: %v", tc.target, r)
}
if tc.want.path != "" && path != tc.want.path {
t.Fatalf("GET %s: path = %v, want %q", tc.target, path, tc.want.path)
if _, hasPath := r["path"]; hasPath {
t.Fatalf("GET %s: the record carries path: %v", tc.target, r)
}
for _, leak := range []string{
"query-secret", "token=", "authz-secret-value", "Authorization", "Bearer",
"X-Blog-Probe", "probe-header-value", "cookie-secret-value", "Cookie",
"/blog/posts/7", "/blog/posts/8", "/pl/",
"/blog/posts/7", "/blog/posts/8", "/pl/", "path-secret", "/acme/missing",
} {
if strings.Contains(buf.String(), leak) {
t.Fatalf("GET %s: the log carries %q:\n%s", tc.target, leak, buf.String())