fix: strip access tokens from exported span attributes - #127
Conversation
Mapbox APIs take the access token as a URL query parameter, and OpenTelemetry's HTTP/undici auto-instrumentation records the full request URL on every client span as `url.full` and `url.query`. Nothing scrubbed those attributes before export, so any operator who followed the tracing setup in docs/tracing.md and set OTEL_EXPORTER_OTLP_ENDPOINT had the caller's token copied verbatim into their telemetry backend on every authenticated tool call. Wrap the OTLP exporter in a RedactingSpanExporter that rewrites `access_token=<value>` to `access_token=***` across all string span attributes (and span event attributes) on the way out, reusing the existing redactToken() helper. Redacting at the exporter rather than in an instrumentation requestHook means the scrub covers every attribute on every span, including attribute names introduced by future semantic-convention or instrumentation changes, instead of only the four URL attribute names known today. Spans needing no redaction are passed through by reference; spans that do are copied field-by-field from the prototype chain rather than mutated, so the SDK's own span state is untouched and the copy survives SDK version differences in span shape. docs/tracing.md previously claimed sensitive data was protected in tracing output without qualification. It now describes what the exporter actually does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mattpodwysocki
left a comment
There was a problem hiding this comment.
Same shape as mapbox/mcp-server#232 — confirmed the exporter-level wrap covers the real issue (the live token in url.full/url.query).
One follow-up worth tracking: the decoded username/account still isn't promoted to a deliberate span attribute. Several of the affected tools (CreateStyleTool, ListTokensTool, etc.) build request paths as styles/v1/${username} / tokens/v2/${userName} from a JWT-decoded value — after this fix, that value still rides along unlabeled in url.full's path segment rather than as account.id (which createToolSpan already has a slot for, currently only populated from extra?.accountId). Not a credential leak, and knowing which account a trace belongs to is legitimate/desired for support — but it'd be more correct to set it deliberately from the token you've already decoded (getUserNameFromToken) rather than leave it as a side effect of the request URL. Worth a follow-up ticket rather than a blocker on this PR.
Masking every token to `access_token=***` threw away context that is
useful in a trace and not sensitive: which kind of token was used, and
which account the request billed to.
Mapbox tokens are JWTs (`<prefix>.<payload>.<signature>`) whose payload
carries the account name under `u`, so redaction now decodes the payload
and emits `<prefix>.<account>.redacted` — `pk.some-account.redacted`,
`sk.some-account.redacted`, `tk.some-account.redacted`. The signature,
which is the part that actually authenticates, still never leaves the
process.
Anything that does not parse cleanly as such a token — unrecognized
prefix, wrong segment count, payload that is not base64 JSON, payload
with no usable `u` field, or an opaque legacy value — falls back to
`access_token=***`. An unrecognized shape is never partially disclosed on
the assumption it was harmless. The account name is also matched against
`[A-Za-z0-9_-]{1,64}` before being emitted, so a hostile payload cannot
inject arbitrary text into a span attribute.
This changes `redactToken()` itself, so the same enriched placeholder now
applies to the debug logs and MCP client error responses that already
used it, not only to exported spans.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fixture tokens carried a real token's decoded payload (account name and 'a' claim). The signature was already fake, so they were never usable credentials, but there is no reason for a public test file to name a real account. Switch to 'example-account'. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment above ACCOUNT_NAME_PATTERN asserted that Mapbox usernames are lowercase alphanumerics with dashes and underscores. That was not verified against anything, and it contradicted the regex directly below it, which accepts uppercase. Describe what the allowlist is actually for — bounding what a token payload can write into a span attribute or log line — and point at the existing username validation in StyleComparisonTool as the precedent for the character set, rather than claiming a rule about Mapbox account naming. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mattpodwysocki
left a comment
There was a problem hiding this comment.
Approving — same design as mcp-server#232 (redact-at-exporter, JWT-aware placeholder keeping prefix/account and dropping the signature), verified correct here too. No outstanding findings on this one: it reuses the pre-existing redactToken() in MapboxApiBasedTool.ts rather than duplicating it, and doesn't carry the unrelated-file or stale-comment issues flagged on the mcp-server PR.
What changed
The OTLP trace exporter is now wrapped in a
RedactingSpanExporter(src/utils/redactingSpanExporter.ts) that strips access token signatures from all string span attributes and span event attributes before spans leave the process.Redaction keeps the parts that are safe to publish and drops the part that authenticates. Mapbox tokens are JWTs shaped
<prefix>.<payload>.<signature>, and the payload carries the account name underu, so:pk.eyJ1IjoiZXhhbXBsZS1hY2NvdW50In0.signatureaccess_token=pk.example-account.redactedsk.eyJ1Ijoic29tZW9uZSJ9.signatureaccess_token=sk.someone.redactedtk.eyJ1Ijoic29tZW9uZSJ9.signatureaccess_token=tk.someone.redactedaccess_token=***Keeping the prefix and account name means a trace still tells you whether a call used a secret or a public token and which account it billed to — useful when debugging a permissions or quota problem — while the signature never leaves the process.
The fallback to
***covers an unrecognized prefix, the wrong segment count, a payload that is not base64 JSON, a payload with no usableu, and opaque legacy values. An unrecognized shape is never partially disclosed on the assumption it was harmless. The decoded account name is also matched against[A-Za-z0-9_-]{1,64}before being emitted, so a hostile payload cannot inject arbitrary text into a span attribute.This changes the existing
redactToken()insrc/tools/MapboxApiBasedTool.ts, so the enriched placeholder also applies to the debug logs and MCP client error responses that already used it, not only to exported spans.Why
Mapbox APIs take the access token as a URL query parameter, and OpenTelemetry's HTTP/undici auto-instrumentation records the full request URL on every HTTP client span as
url.fullandurl.query. Nothing in the codebase scrubbed those attributes, so an operator who followed the setup indocs/tracing.mdand setOTEL_EXPORTER_OTLP_ENDPOINThad the caller's access token copied verbatim into their telemetry backend on every authenticated tool call — including the tools that require secret (sk.*) tokens, such ascreate_token_tool,list_tokens_tool, and the style write tools.docs/tracing.mdalso stated under "Security Considerations → Data Privacy" that sensitive data was protected in tracing output, without qualification. That was not true of HTTP client spans. The doc now describes what the exporter actually does, including the placeholder format.Why redact at the exporter
The alternative is a
requestHookon each HTTP instrumentation, scrubbing the four URL attribute names known today (url.full,url.query,http.url,http.target). Wrapping the exporter instead covers every attribute on every span, so an attribute name introduced by a future semantic-convention or instrumentation change is scrubbed too, and it uses the publicSpanExporterinterface rather than mutating span state mid-flight.Two details worth reviewing:
parentSpanIdbecameparentSpanContextin 2.x).How to verify
npx vitest run test/utils/redactingSpanExporter.test.ts test/tools/MapboxApiBasedTool.test.tsurl.full/url.query, redaction of an arbitrary unknown attribute name, preservation of span identity/timing/resource/non-sensitive attributes, pass-through by reference when nothing matches, and delegation of the export result plusshutdown/forceFlush. These build real ended spans throughBasicTracerProviderrather than stub objects, so the field-copy path is exercised against the SDK's actual span implementation.redactTokentests: thepk/sk/tkplaceholder format, multiple tokens in one string, no signature bytes in the output, each of the five fallback cases, and strings containing no token at all.Full suite: 611 tests pass.
End to end:
npm run tracing:jaeger:start, setOTEL_EXPORTER_OTLP_ENDPOINT=http://localhost:4318,npm run build && npm run inspect:build, invoke any authenticated tool, then check the HTTP child span in Jaeger athttp://localhost:16686—url.fullandurl.queryshowaccess_token=<prefix>.<account>.redacted.Watch for
access_token=query parameter form. A token appearing in some other shape (a bare token in a URL path segment, or inside a response body captured as an attribute) is not covered. No current code path does that, but it is the assumption to revisit if one is added./tokens/v2/<username>,/styles/v1/<username>) which spans have always recorded.Authorization: Bearerheader where the Mapbox API supports it would keep it out of URLs in the first place. That is a per-endpoint compatibility question and out of scope here — worth a separate ticket.Testing
🤖 Generated with Claude Code