Repository navigation
fix(observability): mask ?apikey= in the http.url span attribute - #1790
Open
rudycelekli wants to merge 1 commit into
Open
rudycelekli wants to merge 1 commit into
rudycelekli wants to merge 1 commit into
Conversation
The OTLP tracing middleware recorded r.URL.String() as http.url on every REST API span. ?apikey= is an accepted credential, and the Web UI's SSE stream (/events?apikey=...) and the tray send the admin key that way, so with tracing enabled the admin key was exported to the collector. Render the query through oauth.LogSafeQueryString, the renderer http.log already uses for the same requests (SEC-01).
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
When OTLP tracing is enabled (
observability.tracing.enabled),TracingManager.HTTPMiddlewarewraps the whole REST API router and recordsr.URL.String()as thehttp.urlattribute of every span. That string includes the query.?apikey=is an accepted credential (resolveAuth). The Web UI's SSE stream connects to/events?apikey=<admin key>(frontend/src/services/api.ts), and the tray sends the admin key the same way. So the admin API key lands in span attributes and is exported to the configured collector (Jaeger, Tempo, a shared OTel pipeline). Anyone who can read traces can then drive the REST API as admin.httpLoggingMiddlewarealready masks the same query before it writes to http.log (SEC-01). The tracing middleware sees the same requests and was missed.Trigger
Enable tracing, then open the Web UI. Every SSE (re)connect is a
GET /events?apikey=…request, and each sampled one exports the key.Fix
Build
http.urlfrom a copy of the request URL whoseRawQueryhas been passed throughoauth.LogSafeQueryString, the same renderer http.log uses. The path and the non-secret parameters stay as they were.http.targetand the span name already use only the path.internal/oauthdoes not importinternal/observability, so the new import adds no cycle.Test
TestHTTPMiddleware_MasksAPIKeyInSpanURLsends/events?apikey=<64-hex key>&limit=5through the middleware with an in-memory span recorder. The recordedhttp.urlmust not contain the key, and must keep/events?andlimit=5.Commands run
CGO_ENABLED=0 go test -count=1 -run 'Test' ./internal/observability/: okmain, and again with the fix reverted, the new test fails withhttp.url carries the API key: "/events?apikey=3f9a…b5c6&limit=5".go vet ./internal/observability/: ok