diff --git a/internal/api/middleware.go b/internal/api/middleware.go index 0512597..710c82f 100644 --- a/internal/api/middleware.go +++ b/internal/api/middleware.go @@ -7,12 +7,20 @@ import ( "net/http" ) -// withRequestID assigns a request id (honoring an inbound X-Request-Id) and -// echoes it on the response and into the context for the error envelope. +// withRequestID assigns a request id (honoring a WELL-FORMED inbound X-Request-Id) +// and echoes it on the response and into the context for the error envelope. +// +// A caller-supplied id is honored for cross-service tracing, but only after +// validation: the id is echoed to the client, embedded in the error envelope, AND +// persisted verbatim into audit_logs.request_id, so an unvalidated one is an +// audit-integrity vector — an arbitrarily long value bloats the audit row and a +// stray control byte could smuggle a forged line into a log sink. A rejected id is +// replaced with a fresh server-minted one: that one request loses its inbound trace +// link, which is strictly better than storing attacker-controlled text. func withRequestID(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { id := r.Header.Get("X-Request-Id") - if id == "" { + if !validRequestID(id) { id = newRequestID() } w.Header().Set("X-Request-Id", id) @@ -21,6 +29,32 @@ func withRequestID(next http.Handler) http.Handler { }) } +// maxRequestIDLen caps an inbound X-Request-Id we are willing to echo and persist. +// 64 characters comfortably fits a UUID or a typical distributed-trace id while +// bounding what reaches audit_logs.request_id. +const maxRequestIDLen = 64 + +// validRequestID reports whether an inbound X-Request-Id is safe to echo and store: +// non-empty, within maxRequestIDLen, and restricted to an unambiguous, log-safe +// charset (ASCII alphanumerics plus '-', '_', '.'). The byte-length check bounds it +// regardless of encoding, and the charset excludes whitespace, CR/LF, and every +// other control or multibyte rune, so nothing that survives can pollute a log line +// or the audit row. +func validRequestID(id string) bool { + if id == "" || len(id) > maxRequestIDLen { + return false + } + for _, c := range id { + switch { + case c >= 'a' && c <= 'z', c >= 'A' && c <= 'Z', c >= '0' && c <= '9': + case c == '-', c == '_', c == '.': + default: + return false + } + } + return true +} + // withRecover turns a panicking handler into a 500 envelope instead of a // dropped connection. func withRecover(next http.Handler) http.Handler { diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go new file mode 100644 index 0000000..184619e --- /dev/null +++ b/internal/api/middleware_test.go @@ -0,0 +1,60 @@ +package api + +import ( + "regexp" + "strings" + "testing" +) + +// genRequestIDRE matches a server-minted request id: newRequestID emits 8 random +// bytes as lowercase hex, i.e. exactly 16 hex characters. +var genRequestIDRE = regexp.MustCompile(`^[0-9a-f]{16}$`) + +// TestRequestIDValidation pins the audit-integrity guard on withRequestID. A +// caller-supplied X-Request-Id lands in the response header, the error envelope, +// AND audit_logs.request_id, so a well-formed id is honored verbatim (the trace +// link is worth keeping) but anything oversized or carrying a stray byte is dropped +// and replaced with a fresh server-minted id — never echoed or persisted. /healthz +// is Public, so withRequestID (part of the shared baseChain) runs without auth. +func TestRequestIDValidation(t *testing.T) { + h := newTestAPI(newFakeRepo(), newFakeCluster()).ExternalHandler() + + assignedID := func(reqID string) string { + var hdr map[string]string + if reqID != "" { + hdr = map[string]string{"X-Request-Id": reqID} + } + return do(h, "GET", "/healthz", "", hdr).Header().Get("X-Request-Id") + } + + // A well-formed inbound id is honored verbatim (cross-service trace link kept). + const good = "trace-abc_123.DEF" + if got := assignedID(good); got != good { + t.Fatalf("valid X-Request-Id: assigned %q, want it echoed as %q", got, good) + } + + // Absent → the server mints one. + if got := assignedID(""); !genRequestIDRE.MatchString(got) { + t.Fatalf("absent X-Request-Id: assigned %q, want a generated 16-hex id", got) + } + + // Every malformed id is rejected and replaced with a fresh generated id, never + // reflected back — the load-bearing property for audit-log integrity. + for _, bad := range []struct{ name, id string }{ + {"too long", strings.Repeat("a", maxRequestIDLen+1)}, + {"space", "has space"}, + {"crlf injection", "abc\r\ndef"}, + {"path separator", "a/b"}, + {"multibyte", "trace-ünïcödé"}, + } { + t.Run(bad.name, func(t *testing.T) { + got := assignedID(bad.id) + if got == bad.id { + t.Fatalf("rejected id was echoed back verbatim: %q", got) + } + if !genRequestIDRE.MatchString(got) { + t.Fatalf("replacement %q is not a generated 16-hex id", got) + } + }) + } +}