History

Trace the OAuth lifecycle: token requests, connects, and health checks (#1485)

The OAuth domain layer had no spans. The token POST also bypasses Effect's HttpClient (oauth4webapi drives the raw fetch), so not even an http.client span existed underneath — AS latency and refusal rates were unmeasurable, and health verdicts were only obtainable by querying the database. - executor.oauth.token_request on all three grants: grant type, token hostname, client auth method, and the AS's RFC 6749 error code on failure. Hostname only, never the full URL. - executor.oauth.complete around the code redemption: integration, client slug, whether the token host was rebound to a regional endpoint, and the connection's freshness facts at birth (refresh token present, expiry advertised, scope shortfall counts). - executor.connection.health.check / executor.connection.validate with the verdict, its source (cache, probe, credential-only, no-capability), and http status. mcp.plugin.check_health gets the same outcome stamp — its failures fold onto the success channel, so the span previously ended green on an expired credential. - The AS error code is now a typed oauthErrorCode field on CredentialResolutionError and a structured detail on the sandbox-facing failure, instead of surviving only inside message text.

Rhys Sullivan committed 3 months ago

Commit f78b8ca1 parent b7c23337 files · +217 −25Browse files

Unified · Split

7 files
  1. e2e/scenarios/oauth-refresh-rejected.test.ts +5 −0
  2. packages/core/execution/src/tool-invoker.ts +8 −0
  3. packages/core/sdk/src/errors.ts +7 −0
  4. packages/core/sdk/src/executor.ts +72 −16
  5. packages/core/sdk/src/oauth-helpers.ts +64 −3
  6. packages/core/sdk/src/oauth-service.ts +44 −5
  7. packages/plugins/mcp/src/sdk/plugin.ts +17 −1
e2e/scenarios/oauth-refresh-rejected.test.ts+5−0
@@ -118,6 +118,7 @@ type ToolEnvelope = { · 117 unmodified lines
118118 readonly details?: {
119119 readonly category?: string;
120120 readonly recovery?: Record<string, string>;
121+ readonly upstream?: { readonly details?: { readonly oauthErrorCode?: string } };
121122 };
122123 };
123124 };
@@ -249,6 +250,10 @@ scenario( · 125 unmodified lines
249250 failure.error?.message ?? "",
250251 "the message carries the AS's RFC 6749 error code",
251252 ).toContain("invalid_request");
253+ expect(
254+ failure.error?.details?.upstream?.details?.oauthErrorCode,
255+ "the AS's code is ALSO a structured field, so agents and telemetry never parse the message for it",
256+ ).toBe("invalid_request");
252257 expect(
253258 failure.error?.message ?? "",
254259 "the message carries the AS's error description",
packages/core/execution/src/tool-invoker.ts+8−0
@@ -177,6 +177,7 @@ const credentialResolutionToolFailure = (input: { · 176 unmodified lines
177177 readonly label: string;
178178 readonly message: string;
179179 readonly reauthRequired?: boolean;
180+ readonly oauthErrorCode?: string;
180181 }) =>
181182 authToolFailure({
182183 code: input.reauthRequired === true ? "oauth_reauth_required" : "oauth_refresh_failed",
@@ -188,6 +189,12 @@ const credentialResolutionToolFailure = (input: { · 5 unmodified lines
188189 kind: "oauth",
189190 label: input.label,
190191 },
192+ // The AS's own RFC 6749 §5.2 verdict, when there was one — structured so
193+ // the agent (and anyone reading the failure) can distinguish a dead grant
194+ // from a misconfigured app without parsing the message.
195+ ...(input.oauthErrorCode !== undefined
196+ ? { upstream: { details: { oauthErrorCode: input.oauthErrorCode } } }
197+ : {}),
191198 });
192199
193200 const bindingToolFailure = (value: unknown): ToolError | null => {
@@ -310,6 +317,7 @@ export const makeExecutorToolInvoker = ( · 116 unmodified lines
310317 label: `${err.integration}.${err.owner}.${err.name}`,
311318 message: err.message,
312319 reauthRequired: err.reauthRequired,
320+ oauthErrorCode: err.oauthErrorCode,
313321 }),
314322 ),
315323 ),
packages/core/sdk/src/errors.ts+7−0
@@ -181,6 +181,13 @@ export class CredentialResolutionError extends Schema.TaggedErrorClass<Credentia · 180 unmodified lines
181181 /** True when the stored grant is permanently invalid and the user must
182182 * sign in again (RFC 6749 §5.2 invalid_grant and friends). */
183183 reauthRequired: Schema.optional(Schema.Boolean),
184+ /** The authorization server's RFC 6749 §5.2 error code (`invalid_grant`,
185+ * `invalid_client`, …), when the failure came from a token-endpoint
186+ * refusal. A typed field rather than message text so telemetry and
187+ * classification read it structurally — the misclassification where
188+ * `invalid_client` (rotated app secret, fleet-wide) surfaced as a vague
189+ * "degraded" was only findable by grepping persisted message strings. */
190+ oauthErrorCode: Schema.optional(Schema.String),
184191 },
185192 ) {}
186193
packages/core/sdk/src/executor.ts+72−16
@@ -1804,6 +1804,7 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 1803 unmodified lines
18041804 // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: OAuth2Error carries a typed `message`
18051805 message: `OAuth token refresh was rejected (${cause.error}): ${cause.message}`,
18061806 reauthRequired: cause.error === "invalid_grant",
1807+ oauthErrorCode: cause.error,
18071808 });
18081809 }
18091810 return new StorageError({
@@ -1849,16 +1850,13 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 39 unmodified lines
18491850 // The refresh path was previously invisible to telemetry: no span, no
18501851 // log, no metric. When a customer reported "my OAuth just died", there
18511852 // was no way to answer "did a refresh even fire, and did it work?"
1852− // without a repro. Stamp the outcome and the failure KIND — enumerable
1853− // identifiers only, never user content or token material.
1854− //
1855− // The AS's own RFC 6749 §5.2 code is not stamped here: it survives only
1856− // inside the error's message, and that message embeds the token
1857− // endpoint's response URL and a body preview, so it can't go on a span
1858− // attribute. Making that code a queryable dimension needs it lifted to
1859− // a typed field on CredentialResolutionError first — worth doing, since
1860− // invalid_client (a rotated secret, fleet-wide) currently looks exactly
1861− // like a transient server_error from a trace.
1853+ // without a repro. Stamp the outcome, the failure KIND, and the AS's
1854+ // own RFC 6749 §5.2 code (from the typed `oauthErrorCode` field, NOT
1855+ // the message — the message embeds the token endpoint's response URL
1856+ // and a body preview). Enumerable identifiers only, never user content
1857+ // or token material. The code is the dimension that separates
1858+ // invalid_client (a rotated app secret — fleet-wide, page someone)
1859+ // from server_error (transient, the next invoke retries).
18621860 Effect.tap(() => Effect.annotateCurrentSpan({ "executor.oauth.refresh.outcome": "ok" })),
18631861 Effect.tapError((error: StorageFailure | CredentialResolutionError) =>
18641862 Effect.annotateCurrentSpan({
@@ -1870,7 +1868,12 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 5 unmodified lines
18701868 // the refresh token itself is dead) or a transient failure the next
18711869 // invoke can retry. The split is the actionable half of the signal.
18721870 ...(Predicate.isTagged(error, "CredentialResolutionError")
1873− ? { "executor.oauth.refresh.reauth_required": error.reauthRequired === true }
1871+ ? {
1872+ "executor.oauth.refresh.reauth_required": error.reauthRequired === true,
1873+ ...(error.oauthErrorCode !== undefined
1874+ ? { "executor.oauth.error_code": error.oauthErrorCode }
1875+ : {}),
1876+ }
18741877 : {}),
18751878 }),
18761879 ),
@@ -3092,6 +3095,25 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 1215 unmodified lines
30923095 return out;
30933096 });
30943097
3098+ /** Stamp the verdict + which path produced it onto the enclosing health
3099+ * span. Every health outcome is HTTP 200, so WITHOUT these attributes the
3100+ * request envelope cannot separate healthy from expired — the "what
3101+ * fraction of connections are dead right now" question was previously
3102+ * only answerable by querying the database. `status` and `httpStatus`
3103+ * are enumerable; `detail`/`identity` (upstream free text / an email)
3104+ * never go on a span. */
3105+ const annotateHealthVerdict = (
3106+ source: "cache" | "no_capability" | "credential_only" | "probe",
3107+ result: HealthCheckResult,
3108+ ): Effect.Effect<void> =>
3109+ Effect.annotateCurrentSpan({
3110+ "executor.health.status": result.status,
3111+ "executor.health.source": source,
3112+ ...(result.httpStatus !== undefined
3113+ ? { "executor.health.http_status": result.httpStatus }
3114+ : {}),
3115+ });
3116+
30953117 const connectionCheckHealth = (
30963118 ref: ConnectionRef,
30973119 options?: {
@@ -3116,7 +3138,10 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 18 unmodified lines
31163138 }
31173139 if (options?.ifStaleMs !== undefined) {
31183140 const cached = Option.getOrNull(decodeLastHealth(connectionRow.last_health));
3119− if (cached && Date.now() - cached.checkedAt < options.ifStaleMs) return cached;
3141+ if (cached && Date.now() - cached.checkedAt < options.ifStaleMs) {
3142+ yield* annotateHealthVerdict("cache", cached);
3143+ return cached;
3144+ }
31203145 }
31213146 const integrationRow = yield* findIntegrationRow(ref.integration);
31223147 if (!integrationRow) {
@@ -3124,10 +3149,19 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 1 unmodified line
31243149 }
31253150 const runtime = runtimes.get(integrationRow.plugin_id);
31263151 const check = runtime?.plugin.checkHealth;
3127− if (!runtime || !check) return unknownHealth();
3152+ if (!runtime || !check) {
3153+ const result = unknownHealth();
3154+ yield* annotateHealthVerdict("no_capability", result);
3155+ return result;
3156+ }
31283157 const spec = describeHealthCheckForRow(integrationRow) ?? undefined;
31293158 if (spec === undefined && connectionRow.oauth_client != null) {
3159+ // No probe operation is declared, so "healthy" here means only "the
3160+ // credential resolved (refreshing if due)" — a refresh failure is
3161+ // the one real signal this path can produce, and it must not hide
3162+ // inside a green span.
31303163 const result = yield* oauthCredentialHealthWithoutProbe(connectionRow);
3164+ yield* annotateHealthVerdict("credential_only", result);
31313165 yield* persistHealthResult(ref, result);
31323166 return result;
31333167 }
@@ -3156,12 +3190,20 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 22 unmodified lines
31563190 `Health check for connection "${ref.name}" failed.`,
31573191 );
31583192 }).pipe(Effect.catchTag("CredentialResolutionError", healthFromCredentialResolutionError));
3193+ yield* annotateHealthVerdict("probe", result);
31593194 // Persist the verdict on the connection row so the accounts list shows
31603195 // alive/expired at a glance. Best-effort: a write failure must not turn
31613196 // a successful probe into an error.
31623197 yield* persistHealthResult(ref, result);
31633198 return result;
3164− });
3199+ }).pipe(
3200+ Effect.withSpan("executor.connection.health.check", {
3201+ attributes: {
3202+ "executor.integration": String(ref.integration),
3203+ "executor.connection": String(ref.name),
3204+ },
3205+ }),
3206+ );
31653207
31663208 const connectionValidate = (
31673209 input: ValidateConnectionInput,
@@ -3196,11 +3238,25 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 28 unmodified lines
31963238 // from the integration row. Nothing persists here: validate is the
31973239 // key-first flow's dry run.
31983240 const spec = input.spec ?? describeHealthCheckForRow(integrationRow) ?? undefined;
3199− return yield* foldPluginFailure(
3241+ const result = yield* foldPluginFailure(
32003242 check({ ctx: runtime.ctx, integration: record, credential, spec }),
32013243 `Validating credential for "${input.integration}" failed.`,
32023244 );
3203− });
3245+ // Nothing persists here BY DESIGN, which makes this span the only
3246+ // possible record of "what fraction of pasted credentials are rejected
3247+ // at the door" — a signal the DB can never carry.
3248+ yield* Effect.annotateCurrentSpan({
3249+ "executor.health.status": result.status,
3250+ ...(result.httpStatus !== undefined
3251+ ? { "executor.health.http_status": result.httpStatus }
3252+ : {}),
3253+ });
3254+ return result;
3255+ }).pipe(
3256+ Effect.withSpan("executor.connection.validate", {
3257+ attributes: { "executor.integration": String(input.integration) },
3258+ }),
3259+ );
32043260
32053261 // Clear the sync stamp so the next tools read re-produces this connection's
32063262 // catalog. The deferred variant of `connectionsRefresh` for signals that
packages/core/sdk/src/oauth-helpers.ts+64−3
@@ -367,6 +367,43 @@ const toOAuth2ErrorWithHttpSummary = (cause: unknown): Effect.Effect<OAuth2Error · 366 unmodified lines
367367 const failOAuth2WithHttpSummary = (cause: unknown): Effect.Effect<never, OAuth2Error> =>
368368 toOAuth2ErrorWithHttpSummary(cause).pipe(Effect.flatMap((error) => Effect.fail(error)));
369369
370+/** Trace one token-endpoint round trip. This is the ONLY place a token request
371+ * can be observed: oauth4webapi drives the raw global `fetch`, not Effect's
372+ * HttpClient, so no `http.client` span exists underneath — without this span
373+ * the AS's latency and refusal rate are invisible.
374+ *
375+ * Attribute discipline: hostname (never the full URL — some providers carry
376+ * tenant ids in the path), the grant literal, the auth method, and on failure
377+ * the AS's RFC 6749 §5.2 code. Never the OAuth2Error message (it embeds the
378+ * response URL and a body preview), never token or code material. */
379+const withTokenRequestSpan =
380+ (input: {
381+ readonly grantType: "authorization_code" | "client_credentials" | "refresh_token";
382+ readonly tokenUrl: string;
383+ readonly clientAuth: ClientAuthMethod | undefined;
384+ readonly hasResource: boolean;
385+ }) =>
386+ <A>(effect: Effect.Effect<A, OAuth2Error>): Effect.Effect<A, OAuth2Error> =>
387+ effect.pipe(
388+ Effect.tapError((error) =>
389+ Effect.annotateCurrentSpan({
390+ ...(error.error !== undefined ? { "executor.oauth.error_code": error.error } : {}),
391+ }),
392+ ),
393+ Effect.withSpan("executor.oauth.token_request", {
394+ attributes: {
395+ "executor.oauth.grant_type": input.grantType,
396+ "executor.oauth.token_host": hostnameForTelemetry(input.tokenUrl),
397+ "executor.oauth.client_auth": input.clientAuth ?? DEFAULT_CLIENT_AUTH_METHOD,
398+ "executor.oauth.has_resource": input.hasResource,
399+ },
400+ }),
401+ );
402+
403+/** The hostname alone — a malformed URL yields "invalid" rather than leaking
404+ * whatever string failed to parse. */
405+const hostnameForTelemetry = (url: string): string => URL.parse(url)?.hostname ?? "invalid";
406+
370407 // ---------------------------------------------------------------------------
371408 // oauth4webapi adapter helpers
372409 // ---------------------------------------------------------------------------
@@ -614,7 +651,15 @@ export const exchangeAuthorizationCode = ( · 241 unmodified lines
614651 return await processTokenEndpointResponse(as, client, response);
615652 },
616653 catch: (cause) => cause,
617− }).pipe(Effect.catch(failOAuth2WithHttpSummary));
654+ }).pipe(
655+ Effect.catch(failOAuth2WithHttpSummary),
656+ withTokenRequestSpan({
657+ grantType: "authorization_code",
658+ tokenUrl: input.tokenUrl,
659+ clientAuth: input.clientAuth,
660+ hasResource: input.resource !== undefined,
661+ }),
662+ );
618663
619664 // ---------------------------------------------------------------------------
620665 // Exchange client credentials → tokens (RFC 6749 §4.4)
@@ -669,7 +714,15 @@ export const exchangeClientCredentials = ( · 48 unmodified lines
669714 return tokenResponseFrom(result);
670715 },
671716 catch: (cause) => cause,
672− }).pipe(Effect.catch(failOAuth2WithHttpSummary));
717+ }).pipe(
718+ Effect.catch(failOAuth2WithHttpSummary),
719+ withTokenRequestSpan({
720+ grantType: "client_credentials",
721+ tokenUrl: input.tokenUrl,
722+ clientAuth: input.clientAuth,
723+ hasResource: input.resource !== undefined,
724+ }),
725+ );
673726
674727 // ---------------------------------------------------------------------------
675728 // Refresh access token
@@ -740,7 +793,15 @@ export const refreshAccessToken = ( · 64 unmodified lines
740793 return tokenResponseFrom(result);
741794 },
742795 catch: (cause) => cause,
743− }).pipe(Effect.catch(failOAuth2WithHttpSummary));
796+ }).pipe(
797+ Effect.catch(failOAuth2WithHttpSummary),
798+ withTokenRequestSpan({
799+ grantType: "refresh_token",
800+ tokenUrl: input.tokenUrl,
801+ clientAuth: input.clientAuth,
802+ hasResource: input.resource !== undefined,
803+ }),
804+ );
744805
745806 // ---------------------------------------------------------------------------
746807 // Refresh-needed predicate
packages/core/sdk/src/oauth-service.ts+44−5
@@ -1283,6 +1283,15 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { · 1282 unmodified lines
12831283 clientOwnerFromPayload(sessionRow.payload) ?? (String(sessionRow.owner) as Owner),
12841284 };
12851285
1286+ // Annotate as soon as the session resolves the flow's identity, so even
1287+ // a completion that fails at the exchange still says WHOSE connect died.
1288+ yield* Effect.annotateCurrentSpan({
1289+ "executor.integration": String(session.integration),
1290+ "executor.connection": String(session.name),
1291+ "executor.template": String(session.template),
1292+ "executor.oauth.client": String(session.clientSlug),
1293+ });
1294+
12861295 // Expired sessions are not redeemable — drop + treat as not found.
12871296 if (Number.isFinite(session.expiresAt) && session.expiresAt <= Date.now()) {
12881297 yield* deleteSession(input.state);
@@ -1368,9 +1377,24 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { · 79 unmodified lines
13681377 ),
13691378 );
13701379
1380+ // Everything a "why did this connect fail / where did it go" question
1381+ // needs, none of it secret: slugs, owner scope, and whether the token
1382+ // host was rebound to a regional endpoint (the Datadog multi-site path —
1383+ // a bug there previously shipped and was only diagnosable by hand).
1384+ // Deliberately absent: the code, the PKCE verifier, the token, the
1385+ // callback domain (can embed an org's private site), and identityLabel
1386+ // (resolves to an email).
1387+ yield* Effect.annotateCurrentSpan({
1388+ "executor.oauth.token_host_rebound": tokenUrl !== client.tokenUrl,
1389+ });
1390+
13711391 yield* deleteSession(input.state);
13721392 return connection;
1373− });
1393+ }).pipe(
1394+ Effect.withSpan("executor.oauth.complete", {
1395+ attributes: { "executor.oauth.grant": "authorization_code" },
1396+ }),
1397+ );
13741398
13751399 // -----------------------------------------------------------------------
13761400 // Mint the connection from a freshly exchanged token: store the access
@@ -1416,6 +1440,24 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { · 39 unmodified lines
14161440 }
14171441
14181442 const oauthScope = recordedOAuthScope(token, requestedScopes);
1443+ const missingScopes =
1444+ client.grant === "authorization_code"
1445+ ? missingGrantedOAuthScopes(requestedScopes, oauthScope)
1446+ : [];
1447+ // The freshness facts of this connection AT BIRTH, on the enclosing
1448+ // span (executor.oauth.complete, or the reconnect path's request
1449+ // envelope). Every "why did this connection later go stale" question
1450+ // starts here: a partial grant fails later as oauth_scope_insufficient
1451+ // in an unrelated trace; no refresh token means the first expiry is
1452+ // terminal; no advertised expiry means only the reactive 401 path can
1453+ // ever refresh it. Counts and booleans only — scope VALUES can encode
1454+ // customer resource names on some providers.
1455+ yield* Effect.annotateCurrentSpan({
1456+ "executor.oauth.scope_requested_count": requestedScopes.length,
1457+ "executor.oauth.scope_missing_count": missingScopes.length,
1458+ "executor.oauth.has_refresh_token": token.refresh_token !== undefined,
1459+ "executor.oauth.has_advertised_expiry": typeof token.expires_in === "number",
1460+ });
14191461 return yield* deps.mintOAuthConnection({
14201462 owner: target.owner,
14211463 name: target.name,
@@ -1436,10 +1478,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { · 14 unmodified lines
14361478 // non-resource scope from the token `scope` string, so preserve it when
14371479 // the refresh token proves it was granted.
14381480 oauthScope,
1439− missingOAuthScopes:
1440− client.grant === "authorization_code"
1441− ? missingGrantedOAuthScopes(requestedScopes, oauthScope)
1442− : [],
1481+ missingOAuthScopes: missingScopes,
14431482 oauthTokenUrl,
14441483 });
14451484 });
packages/plugins/mcp/src/sdk/plugin.ts+17−1
@@ -1555,7 +1555,23 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { · 1554 unmodified lines
15551555 detail: error.message,
15561556 } satisfies HealthCheckResult),
15571557 ),
1558− Effect.withSpan("mcp.plugin.check_health"),
1558+ // Every failure above folds onto the SUCCESS channel, so without this
1559+ // the span ends green on an expired credential — a 401 auth wall and a
1560+ // healthy dial were indistinguishable in traces.
1561+ Effect.tap((result) =>
1562+ Effect.annotateCurrentSpan({
1563+ "mcp.health.status": result.status,
1564+ ...("httpStatus" in result && result.httpStatus !== undefined
1565+ ? { "mcp.health.http_status": result.httpStatus }
1566+ : {}),
1567+ }),
1568+ ),
1569+ Effect.withSpan("mcp.plugin.check_health", {
1570+ attributes: {
1571+ "executor.integration": String(credential.integration),
1572+ "mcp.connection.name": String(credential.connection),
1573+ },
1574+ }),
15591575 ),
15601576
15611577 describeAuthMethods: describeMcpAuthMethods,