fix(api): validate inbound X-Request-Id before echo and audit persist (164ac447) · Commits · FelisMC / Felis · GitLab

internal/api/middleware.go

+37 −3

Changes for internal/api/middleware.go: 37 added lines, 3 removed lines.
Original line number	Diff line number	Diff line
		@@ -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 {

internal/api/middleware_test.go

0 → 100644

+60 −0

Changes for internal/api/middleware_test.go: 60 added lines, 0 removed lines.
Original line number	Diff line number	Diff line
		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)
		}
		})
		}
		}