From 4507d82b4c712d4cc75920650426ef5666ea356e Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sun, 17 May 2026 15:01:37 +0200 Subject: [PATCH] fix(security): address CodeQL findings on Stockholm + SiriusXM stubs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of the eight CodeQL alerts on PR #313 had clean, low-cost fixes: - go/clear-text-logging (#141, #142): the SiriusXM stub logged the raw Authorization header value at INFO. The header carries a long-lived bearer token (margeAuthToken) — capturing service logs would yield replayable credentials. Switch to logging only the boolean presence (`authPresent=%t`). - go/bad-redirect-check (#138): the Stockholm handler's bare-path redirect uses cfg.BasePath verbatim. basePath is operator-provided (CLI flag / STOCKHOLM_BASE_PATH env), not request input — but a value like "//evil.com" would still produce a scheme-relative redirect to an external host. Reject any leading-double-slash or embedded backslash at construction time so the redirect target can only ever be an absolute local path. The remaining CodeQL alerts are out of scope here: - go/request-forgery on proxy.go (#139, #140): the /api/http-proxy endpoint takes a user-provided url= parameter and fetches it by design — that's the whole point of the proxy. Mitigations already in place: isProxyLoop rejects self-references; the proxy is only reachable under a LAN trust model. - go/path-injection on static.go (#143, #144, #145): the path-traversal guard in resolveStaticFile (string-prefix check on absolute paths) is sound, but CodeQL doesn't trace it across the function boundary. A clearer refactor to filepath.Rel might silence the alert; deferred. Co-Authored-By: Claude Opus 4.7 (1M context) --- pkg/service/handlers/handlers_bmx_siriusxm.go | 13 +++++++++---- pkg/service/stockholm/handler.go | 13 +++++++++++++ 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/pkg/service/handlers/handlers_bmx_siriusxm.go b/pkg/service/handlers/handlers_bmx_siriusxm.go index a4c25fe..b9e2d5a 100644 --- a/pkg/service/handlers/handlers_bmx_siriusxm.go +++ b/pkg/service/handlers/handlers_bmx_siriusxm.go @@ -21,10 +21,15 @@ import ( // HandleSiriusXMLiveAdapter returns the SIRIUSXM_EVEREST service descriptor // from bmx_services.json for the bare live-adapter base URL. +// +// NB: we log the *presence* of the Authorization header, not its value — +// the header carries a long-lived bearer token (margeAuthToken) that +// would be replayable if a logfile got captured. CodeQL +// go/clear-text-logging caught the original `auth=%q` shape. func (s *Server) HandleSiriusXMLiveAdapter(w http.ResponseWriter, r *http.Request) { - log.Printf("[BMX SiriusXM] %s %s ua=%q auth=%q query=%q", + log.Printf("[BMX SiriusXM] %s %s ua=%q authPresent=%t query=%q", r.Method, r.URL.Path, r.UserAgent(), - r.Header.Get("Authorization"), r.URL.RawQuery) + r.Header.Get("Authorization") != "", r.URL.RawQuery) svc, err := extractBMXService(bmxServicesJSON, "SIRIUSXM_EVEREST") if err != nil { @@ -45,9 +50,9 @@ func (s *Server) HandleSiriusXMLiveAdapter(w http.ResponseWriter, r *http.Reques // pass — the _links in the descriptor publish /availability, /token, // /navigate, /logout; playback URLs come dynamically from navigate. func (s *Server) HandleSiriusXMLiveAdapterSubpath(w http.ResponseWriter, r *http.Request) { - log.Printf("[BMX SiriusXM] UNIMPLEMENTED %s %s ua=%q auth=%q query=%q", + log.Printf("[BMX SiriusXM] UNIMPLEMENTED %s %s ua=%q authPresent=%t query=%q", r.Method, r.URL.Path, r.UserAgent(), - r.Header.Get("Authorization"), r.URL.RawQuery) + r.Header.Get("Authorization") != "", r.URL.RawQuery) http.Error(w, "not implemented", http.StatusNotFound) } diff --git a/pkg/service/stockholm/handler.go b/pkg/service/stockholm/handler.go index 0e00da3..75b1d0b 100644 --- a/pkg/service/stockholm/handler.go +++ b/pkg/service/stockholm/handler.go @@ -64,6 +64,19 @@ func New(stockholmDir, workspaceRoot, backendURL, basePath string) (*Handler, er } basePath = strings.TrimRight(basePath, "/") + + // Defence in depth: basePath is operator-provided (CLI flag / + // STOCKHOLM_BASE_PATH env var), not request input — but if it + // were ever set to "//evil.com" (typo or hostile env injection) + // the bare-path redirect below would go scheme-relative to + // evil.com. Reject any leading-double-slash and any backslash + // so the redirect target can only ever be an absolute local + // path. CodeQL go/bad-redirect-check raised the original + // concern. + if strings.HasPrefix(basePath, "//") || strings.HasPrefix(basePath, "/\\") || strings.ContainsAny(basePath, "\\") { + return nil, fmt.Errorf("invalid stockholm base path %q: must be an absolute path starting with a single '/'", basePath) + } + cfg.BasePath = basePath state.SeedFromEnv(cfg)