mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-07 13:41:30 +00:00
shankar0123
af5c39252f
CodeQL alert #34 (severity: HIGH, rule: go/reflected-xss) fired on commit8191b1e(Phase 6 SCALE-L2 ETag middleware): internal/api/middleware/etag.go:220 return r.ResponseWriter.Write(b) "Cross-site scripting vulnerability due to user-provided value." Root cause (analysis): The etagRecorder type buffers response bytes from the wrapped handler so the ETag middleware can hash the body before deciding 304-vs-200. On the over-sized-response truncation path (body > 64 KiB), bytes are forwarded directly to the underlying ResponseWriter at line 220. CodeQL's data-flow query traces: *http.Request (source: user input) → handler reads query/path/body → handler echoes data into the JSON response payload (a cert's common_name, an audit row's actor display name, etc.) → json.NewEncoder(w).Encode(...) calls w.Write([]byte) → etagRecorder.Write forwards to r.ResponseWriter.Write(b) ^^^^^^^^^^^^^^^^^^^^^^^^^^^ sink — CodeQL flags reflected-XSS CodeQL can't see that the wrapped handler set Content-Type: application/json via handler.JSON() before any byte was written; it sees a generic byte forwarder writing to an http.ResponseWriter with no proximate Content-Type guarantee. Browsers don't interpret application/json as HTML — so this is technically a false positive — but the data-flow path is real and a future handler that forgets to set Content-Type would convert it into a real vuln (browsers can content-sniff a JSON body as text/html when Content-Type is absent). Fix (defense-in-depth, not just suppression): Add an explicit Content-Type guard at writeHeadersToWire() — the centralized chokepoint that ALL wire-write paths funnel through (line 213 in Write's truncation branch, line 258 in flush's main branch). If Content-Type is unset at this point, default to "application/json; charset=utf-8". This: 1. Makes the Content-Type invariant the middleware relies on explicit at the sink, which is the standard pattern CodeQL's go/reflected-xss recognizes as "validated before write". 2. Adds REAL defense-in-depth: a hypothetical future handler wired through ETag that forgot Content-Type can no longer expose a content-sniff vuln. The middleware enforces the safe shape at the boundary. 3. Is behavior-preserving for the 5 current consumers — every wrapped list endpoint (/api/v1/{certificates,agents,jobs, audit,discovered-certificates}) routes JSON responses through handler.JSON() at internal/api/handler/response.go:60, which already sets Content-Type: application/json. Path is no-op for them. Why not a simpler approach: • Removing line 220 (refactor to avoid the data-flow): the truncation path is required behavior — once buffer > 64 KiB the middleware degrades to no-caching pass-through, which requires writing the body bytes to the wire. The data flow is structural. • html.EscapeString(b) before write: would corrupt JSON. Wrong encoder for the content type. • Bare CodeQL suppression comment: closes the alert without actually addressing the latent bug a future handler could create. Defense-in-depth is the operator's stated preference per the CLAUDE.md "always take the complete path" principle. Verification (sandbox constraints disclosed honestly): • Manual syntax inspection — diff is 21-line additive, all inside writeHeadersToWire(). Brace count balanced (27/27), paren count balanced (53/53). No imports changed (http.Header API was already in use). • CI guards: all 48 pass locally. • Existing etag_test.go has 10 contract tests covering: ETag emit on GET, 304-on-If-None-Match, 200-on-mutation, POST bypass, 5xx/4xx pass-through, OversizedResponse degradation, wildcard match, HEAD parity, PassThrough body preservation. Behavior analysis (see commit body): every test either (a) has the handler set Content-Type explicitly (no-op for the new guard) or (b) goes through the 304-direct-write path in ETag() which bypasses the recorder entirely. All 10 tests should remain green when `make verify` runs on workstation. • Go toolchain NOT available in sandbox (no `go vet` / `go test` / `golangci-lint` / `staticcheck`). Disk pressure on the shared /sessions partition (166 MB free of 9.8 GB) prevented installing Go for this run. The CLAUDE.md operating rule allows this fallback path provided the verification gap is disclosed and the operator runs `make verify` on workstation BEFORE pushing. Operator: please run `make verify` from the repo root on your workstation before pushing. The change is minimal + additive, but the Go test suite should be the final green-light. Falsifiable proof for the next CodeQL scan: alert #34 should auto-close on the next push to master once the post-fix run sees the Content-Type setter precede every Write to the wire. Ground-truth: origin/master tip6c00f7bverified via GitHub API BEFORE commit per the operating rule.