fix(api): validate inbound X-Request-Id before echo and audit persist
withRequestID honored any inbound X-Request-Id verbatim, and that value is echoed on the response, embedded in the error envelope, and persisted into audit_logs.request_id. An unvalidated caller-supplied id is therefore an audit-integrity vector: an arbitrarily long value bloats the audit row, and a stray control byte (CR/LF) could smuggle a forged entry into a log sink. Accept an inbound id only when it is well-formed — non-empty, at most 64 bytes, and restricted to a log-safe charset ([A-Za-z0-9._-]) — otherwise mint a fresh server id. A rejected request loses its inbound trace link, which is strictly better than storing attacker-controlled text in the audit trail.
This commit is contained in:
2 files changed
+97
-3
No files matched your search
@@ -7,12 +7,20 @@ import (
|
|||||||
"net/http"
|
"net/http"
|
||||||
)
|
)
|
||||||
|
|
||||||
// withRequestID assigns a request id (honoring an inbound X-Request-Id) and
|
// withRequestID assigns a request id (honoring a WELL-FORMED inbound X-Request-Id)
|
||||||
// echoes it on the response and into the context for the error envelope.
|
// 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 {
|
func withRequestID(next http.Handler) http.Handler {
|
||||||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
id := r.Header.Get("X-Request-Id")
|
id := r.Header.Get("X-Request-Id")
|
||||||
if id == "" {
|
if !validRequestID(id) {
|
||||||
id = newRequestID()
|
id = newRequestID()
|
||||||
}
|
}
|
||||||
w.Header().Set("X-Request-Id", id)
|
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
|
// withRecover turns a panicking handler into a 500 envelope instead of a
|
||||||
// dropped connection.
|
// dropped connection.
|
||||||
func withRecover(next http.Handler) http.Handler {
|
func withRecover(next http.Handler) http.Handler {
|
||||||
|
|||||||
@@ -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)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in new issue
Block a user