fix(auth): make credential validation failures diagnosable - #385
Conversation
Every failed credential check returned the same 503 body with no server log, so an introspection outage, a missing secret, and a credential that introspected cleanly but is unusable here were indistinguishable after the fact. Tag each throw site with a reason and emit one low cardinality server log line carrying the reason, the introspection HTTP status, elapsed ms, and whether the request was aborted by its own budget. Never log the token, the resolved API key, or the upstream body. Keep the 503 and the existing sentence. Add Retry-After so the client has a concrete wait, and drop the OAuth error code, which RFC 6749 reserves for 400 and 401 responses and which clients surface as an authentication verdict. No WWW-Authenticate: this is not a verdict on the credential.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
An introspection body that failed to parse, or that parsed to null, threw past every tagged error in introspectToken. It reached the client as a 401 with WWW-Authenticate and the raw parser message in error_description, so an upstream fault pushed a working session into reauthorization and no reason was logged. Parse defensively and treat a missing boolean `active` as introspect_malformed_body, matching the guard this file already uses on other upstream JSON reads. Also scope the telemetry doc comment: it covers rejections raised while authenticating, not the outbound client tags raised during tool execution.
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
|
Closed #384 in favour of this one, we were solving the same problem and yours goes further. Nice catch on the null body, I had missed that it slips past the tagged branches and comes back as a 401 challenge. Also verified your read on the client contract change independently: the SDK pulls the error body with .text() and only re-runs the auth flow on a 401 or 403, so dropping the OAuth code and going to plain text is safe. One small thing worth considering. The approach I'd taken was to log at the point the error is created rather than where it's caught, which covers every site for free no matter where it's thrown from. Might be worth folding in, though it's also fine as a follow up if you'd rather keep this PR tight. Rest looks good to me. Ran your branch locally, 75/75 passing. |
Logging from the authenticate catch covered only one of the two call stacks that raise this error. The outbound client checks in getClient run during tool execution, so their reasons were labels that could never appear in a log, and any throw site added later would have been dropped the same way. Build the error through a factory that emits the record and returns it. Every site is covered regardless of where it is caught or whether it is caught at all. Replace the profile field with the resource being validated. Profile describes the request rather than the error, it is not available where the error is raised, and the resource URL is both more precise and already carried into introspection. Note that delegated_credential_unavailable is unreachable: the interceptor is installed only for a session holding a managed key, and signing such a session always yields a non-empty credential or throws. It stays because the signature is string | undefined, so dropping it would mean asserting rather than checking.
|
Folded the nit in as 18f9bc4. Errors are now built through a factory that emits the record and returns it, so all nine reasons log regardless of where the error is caught. Two notes: I swapped |
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/session-credential.ts">
<violation number="1" location="src/session-credential.ts:100">
P3: When the delegated signing secret is missing, the [MCP_CREDENTIAL_VALIDATION] record now logs `resource: null`, losing the profile context the removed logger previously included. `delegationSecret()` raises this during request authentication (via `requireDelegatedCredentialSigning()` in `resolveCredentialFromHeaders`) with no `resource`, unlike the `introspect_*` tags that carry it. This regresses diagnosability for that one path. Thread the profile/resource through to this throw so the emitted record is not blank.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
That path was the one record emitted with no context beyond its reason. Pass the resource the token was validated against, using expectedAudience rather than the profile URL so the legacy audience fallback is reflected. The parameter is optional because outbound signing has no resource in scope and should not carry a misleading one.
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
Constructing the class directly compiles and silently skips the record, which is the regression this PR exists to close. Say so where someone adding a throw site will read it.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Alters the public HTTP response contract of the credential-validation endpoint: body changes from JSON to plain text, adds Retry-After, and drops the OAuth error code, so a human should approve the contract change.
Re-trigger cubic
2 participants
Why
Hosted MCP validates the caller's credential on every request. When that check does not complete cleanly, the server returns one 503 with one sentence and writes nothing to its log.
Seven distinct conditions share that response: the introspection secret is unset, the request fails at the transport, the abort budget fires, the endpoint answers with a status that is not 2xx, it answers with something that is not JSON, the body has no usable boolean
active, or the body is well formed and describes a credential that cannot be used on this resource. Every one of them produced the same bytes.That has two costs. Operators cannot tell an upstream outage from a configuration fault after the fact, so every occurrence turns into an investigation from scratch. And the client is told the service is unavailable with no indication of how long to wait, in a response shaped like an OAuth error, which reads as a verdict on the credential it just presented.
One case did not even reach that path. A body that failed to parse, or that parsed to
null, threw past every tagged error and surfaced as a 401 withWWW-Authenticatecarrying the raw parser message:An upstream fault pushing a working session into reauthorization is the exact failure this response shape exists to avoid.
Summary
Server side, every throw site is built through one factory that emits a record and returns the error:
The record is emitted where the error is raised, not where it is caught. These come from two different call stacks: request authentication, and outbound client setup during tool execution. A single catch site covers only the first and silently drops any site added later.
Deliberately low cardinality.
resourceis one of a handful of server owned URLs. The token, the resolved API key, the upstream response body and its headers, request URLs, and user agents are never included, matching the policy already documented on the neighbouring auth telemetry.Introspection bodies are now parsed defensively, so an unparseable or null body is tagged
introspect_malformed_bodyand answered like every other unusable result, using the same guard this file already applies to other upstream JSON reads.Client side, the response changes shape but not status:
Retry-After5WWW-Authenticate{"error":"temporarily_unavailable","error_description":"..."}Firecrawl credential validation is temporarily unavailableRetry-Afterstates a concrete wait, which RFC 9110 15.6.4 provides for on a 503. The reference MCP client does not read that header today, so this is a contract correction rather than a behaviour change at the client. The OAutherrorcode is dropped because RFC 6749 reserves those for 400 and 401 responses, and clients surface them as authentication failures.The status stays 503 and
WWW-Authenticatestays absent for a concrete reason: in@modelcontextprotocol/sdk, a 401 with an auth provider present re-runs the OAuth flow and retries the request, while any other status is surfaced verbatim. An upstream fault must not trigger that flow. The same code path reads the error body with.text(), never.json(), so changing the body from JSON to the sentence alone is safe and shortens what the user sees to:All nine reason tags reach the log:
introspect_secret_missing,introspect_transport_error,introspect_http_status,introspect_content_type,introspect_malformed_body,introspect_unusable_credential,delegated_signing_secret_missing,outbound_client_uninstrumented,delegated_credential_unavailable.delegated_credential_unavailableis unreachable and now says so in a comment. The interceptor holding it is installed only for a session carrying a managed key, and signing such a session always produces a non-empty credential or throws while signing. The branch stays because the signing helper returnsstring | undefined, so removing it would mean asserting instead of checking.Out of scope on purpose: no caching, no change to the abort budget, and no change to which credentials are introspected.
Test plan
npm test(build, then 75 tests, all passing).npm run lintandnpx tsc --noEmitclean.Coverage in
tests/mcp-smoke.test.mjs:Retry-After: 5, noWWW-Authenticate, body equal to the sentence, and notemporarily_unavailableanywhere in it.each credential validation failure logs its own reason and statusdrives six cases through one server and asserts the emitted reason,introspect_status,profile,aborted, and a numericelapsed_msfor each: a status that is not 2xx, a non JSON content type, a body whoseactiveis not boolean, anullbody, a truncated body, and a clean body describing an unusable credential.getClienttags take.credential validation outages do not misdirect clients into OAuthalso asserts that an unreachable endpoint logsintrospect_transport_errorwithaborted: falseand a null status.Verified against a build of
mainside by side, running the same 19 scenarios through both binaries with the per request SSE event id normalized:fco_andfc-sessions, keyless, the invalid key recovery session, both 401 challenge paths, and a realtools/callthat reached the upstream API exactly once.aborted: trueandelapsed_ms: 1506.