Skip to content

fix: strip access tokens from exported span attributes - #232

Open
Valiunia wants to merge 4 commits into
mainfrom
fix/redact-tokens-from-span-attributes
Open

fix: strip access tokens from exported span attributes#232
Valiunia wants to merge 4 commits into
mainfrom
fix/redact-tokens-from-span-attributes

Conversation

@Valiunia

@Valiunia Valiunia commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

What changed

The 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 under u, so:

Token Appears in the span as
pk.eyJ1IjoiZXhhbXBsZS1hY2NvdW50In0.signature access_token=pk.example-account.redacted
sk.eyJ1Ijoic29tZW9uZSJ9.signature access_token=sk.someone.redacted
tk.eyJ1Ijoic29tZW9uZSJ9.signature access_token=tk.someone.redacted
anything that does not parse as the above access_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 usable u, 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.

The redaction itself lives in a new shared src/utils/redactToken.ts, which also replaces the ad-hoc .replace(accessToken, '[REDACTED]') previously inlined in SearchAndGeocodeTool's debug log. Both redactToken and RedactingSpanExporter are re-exported from @mapbox/mcp-server/utils, so consumers who import the tools directly and run their own OpenTelemetry setup can wrap their own exporter the same way.

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.full and url.query. Nothing in the codebase scrubbed those attributes, so an operator who followed the setup in docs/tracing.md and set OTEL_EXPORTER_OTLP_ENDPOINT had the caller's access token copied verbatim into their telemetry backend on every tool call that hits a Mapbox API. Eleven tools build request URLs that way — directions, isochrone, matrix, static map, geocoding, place details, optimization, map matching, category search, search-and-geocode, and ground location.

docs/tracing.md also 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 requestHook on 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 public SpanExporter interface rather than mutating span state mid-flight.

Two details worth reviewing:

  • Spans needing no redaction are forwarded by reference, so the common case adds one string scan per string attribute and no allocation.
  • Spans that do need redaction are copied property-by-property, walking the prototype chain to pick up accessors, rather than mutated in place. That keeps the SDK's own span objects untouched and avoids a hardcoded field list, which has shifted between SDK versions (parentSpanId became parentSpanContext in 2.x).

How to verify

npx vitest run test/utils/redactingSpanExporter.test.ts test/utils/redactToken.test.ts

  • 6 exporter tests: redaction of url.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 plus shutdown/forceFlush. These build real ended spans through BasicTracerProvider rather than stub objects, so the field-copy path is exercised against the SDK's actual span implementation.
  • 9 redactToken tests: the pk/sk/tk placeholder 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: 774 tests pass.

End to end: npm run tracing:jaeger:start, set OTEL_EXPORTER_OTLP_ENDPOINT=http://localhost:4318, npm run build && npm run inspect:build, invoke e.g. directions_tool, then check the HTTP child span in Jaeger at http://localhost:16686url.full and url.query show access_token=<prefix>.<account>.redacted.

Watch for

  • Redaction is keyed on the 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.
  • The account name is now visible in whatever telemetry backend an operator configures. That is a deliberate trade for a more useful trace; it is an account identifier, not a credential.
  • src/utils/index.ts gains two exports. They are additive, so no consumer breakage, but they are now public API surface.
  • Longer term, passing the token as an Authorization: Bearer header 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

image

🤖 Generated with Claude Code

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 tool
call that hits a Mapbox API.

Wrap the trace 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.

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.

The redaction itself moves into a shared src/utils/redactToken.ts, which
also replaces the ad-hoc `.replace(accessToken, '[REDACTED]')` in
SearchAndGeocodeTool's debug log. Both are re-exported from
@mapbox/mcp-server/utils for consumers who import the tools directly and
run their own OpenTelemetry setup.

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 mattpodwysocki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed the fix closes the actual credential leak — verified RedactingSpanExporter against the real problem (the token landing verbatim in url.full/url.query via undici auto-instrumentation), and the exporter-level wrap is the right call over a per-instrumentation requestHook, since it isn't tied to today's four known URL attribute names.

Two things worth tracking as follow-ups rather than blockers here:

  1. Token is redacted, but the decoded username/account isn't promoted anywhere. After redaction, url.full still reads like https://api.mapbox.com/tokens/v2/testuser?access_token=*** (see the PR's own test fixture) — the account identifier rides along in the URL path, unlabeled, instead of as a deliberate account.id/user.id span attribute (which createToolSpan already has a slot for). Not a credential leak on its own — knowing which account a trace belongs to is legitimate and often desired for support/debugging — but it'd be cleaner to set it explicitly from the already-decoded token rather than leave it as a side effect of the request URL. Worth a follow-up ticket, not a blocker here.
  2. File overlap with feat/generic-map-app. This PR touches docs/tracing.md, CHANGELOG.md, and several of the same tool files (ground_location_tool, search_and_geocode_tool, category_search_tool, optimization_tool) as the in-flight feat/generic-map-app branch, which has been reworking map-preview rendering in those same tools. Worth a rebase/merge check against whichever lands second.

Valiunia and others added 3 commits July 29, 2026 09:17
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.

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>
@Valiunia
Valiunia marked this pull request as ready for review July 29, 2026 08:50
@Valiunia
Valiunia requested a review from a team as a code owner July 29, 2026 08:50
@@ -1,48 +1,58 @@
"use strict";
'use strict';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file shouldn't be included

@@ -1,9 +1,9 @@
export interface VersionInfo {
name: string;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file shouldn't be included

Comment thread src/utils/redactToken.ts
Comment on lines +8 to +12
* Character allowlist for an account name lifted out of a token payload, matching the
* username validation in StyleComparisonTool. This bounds what a token payload can put
* into a span attribute or log line; it is not a statement of Mapbox's account naming
* rules. A name outside it is not partially disclosed — it falls back to `***`.
*/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Character allowlist for an account name lifted out of a token payload, matching the
* username validation in StyleComparisonTool. This bounds what a token payload can put
* into a span attribute or log line; it is not a statement of Mapbox's account naming
* rules. A name outside it is not partially disclosed it falls back to `***`.
*/
/**
* Character allowlist for an account name lifted out of a token payload. This
* bounds what a token payload can put into a span attribute or log line the
* value comes from decoding untrusted JWT payload data, so the allowlist is a
* safety gate against log/span injection and oversized values, not a
* statement of Mapbox's actual account naming rules. A name outside it is not
* partially disclosed it falls back to `***`.
*/

Comment thread src/utils/redactToken.ts
Comment on lines +7 to +12
/**
* Character allowlist for an account name lifted out of a token payload, matching the
* username validation in StyleComparisonTool. This bounds what a token payload can put
* into a span attribute or log line; it is not a statement of Mapbox's account naming
* rules. A name outside it is not partially disclosed — it falls back to `***`.
*/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is stale in this repo — StyleComparisonTool doesn't exist in mcp-server (only in mcp-devkit-server, where this comment originated). Suggested wording that keeps the substance without the dangling cross-repo reference:

Suggested change
/**
* Character allowlist for an account name lifted out of a token payload, matching the
* username validation in StyleComparisonTool. This bounds what a token payload can put
* into a span attribute or log line; it is not a statement of Mapbox's account naming
* rules. A name outside it is not partially disclosed it falls back to `***`.
*/
/**
* Character allowlist for an account name lifted out of a token payload. This
* bounds what a token payload can put into a span attribute or log line the
* value comes from decoding untrusted JWT payload data, so the allowlist is a
* safety gate against log/span injection and oversized values, not a
* statement of Mapbox's actual account naming rules. A name outside it is not
* partially disclosed it falls back to `***`.
*/

mattpodwysocki
mattpodwysocki previously approved these changes Jul 29, 2026

@mattpodwysocki mattpodwysocki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — the core fix (redact-at-exporter, JWT-aware placeholder that keeps the prefix/account and drops the signature) is correct and well-tested; verified the base64/base64url decoding and exception-safety behavior directly, not just from the description.

Two non-blocking items already left as inline comments worth cleaning up before merge: the unrelated versionUtils-cjs reformatting, and the stale StyleComparisonTool reference in redactToken.ts (there's a suggestion ready to accept — note there are two near-duplicate suggestion comments on that line, only one needs to be applied).

@mattpodwysocki
mattpodwysocki dismissed their stale review July 29, 2026 13:35

Dismissing — approved this prematurely based on a misread of a follow-up instruction. Holding the approval until the two outstanding fixes (drop the unrelated versionUtils-cjs reformatting, fix the stale StyleComparisonTool reference in redactToken.ts) land in a commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants