From bef8ef6d9584cf2a68ea839174d8df73929a8684 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Tue, 25 Aug 2026 21:47:58 +0000 Subject: [PATCH] fix(egress): remint once on upstream 403 Treat injected-credential upstream 403 as transient: clear the cached lease, mint a replacement, and retry the hop once before recording permission denied. Co-Authored-By: Ivan Dlugos --- TELEMETRY.md | 3 +- .../junior/src/chat/egress/credentialed.ts | 346 ++++++++++-------- .../handlers/sandbox-egress-proxy.test.ts | 70 ++-- 3 files changed, 248 insertions(+), 171 deletions(-) diff --git a/TELEMETRY.md b/TELEMETRY.md index f27e546e1a..8e74dc2896 100644 --- a/TELEMETRY.md +++ b/TELEMETRY.md @@ -248,7 +248,8 @@ conversation, use `app.dispatch.id` or `agent-dispatch:` as A turn parked for auth, resumed late, or failed after callback. Events: `sandbox.egress.credential.needed`, -`sandbox.egress.credential.unavailable`, `plugin.credential.rejected`, +`sandbox.egress.credential.unavailable`, `sandbox.egress.upstream_auth.rejected`, +`sandbox.egress.upstream_auth.retrying`, `plugin.credential.rejected`, `subscribed_message.authorization.required`, `agent.continue.schedule.failed`, `agent.continue.lock.busy`, `agent.continue.lock.retrying`, `oauth.callback.resume.completed`, `oauth.callback.resume.busy`, diff --git a/packages/junior/src/chat/egress/credentialed.ts b/packages/junior/src/chat/egress/credentialed.ts index 78c129d692..bd95741d05 100644 --- a/packages/junior/src/chat/egress/credentialed.ts +++ b/packages/junior/src/chat/egress/credentialed.ts @@ -648,56 +648,69 @@ export async function executeCredentialedEgressRequest(input: { const recordPermissionDenied = deps.recordPermissionDenied ?? recordSandboxPermissionDenied; - let lease: SandboxEgressCredentialLease; - try { - lease = await issueCredentialLease( - provider, - grantSelection, - credentialContext, - ); - } catch (error) { - if (error instanceof SandboxEgressCredentialError) { - await recordAuthRequired({ + const resolveLease = async (): Promise< + SandboxEgressCredentialLease | Response + > => { + try { + return await issueCredentialLease( + provider, + grantSelection, credentialContext, - provider: error.provider, - grant: error.grant, - kind: error.kind, - authorization: error.authorization, - message: error.message, - }); - const isAuthRequired = error.kind === "auth_required"; - logWarn( - isAuthRequired - ? "sandbox.egress.credential.needed" - : "sandbox.egress.credential.unavailable", - { - ...egressAttributes({ - egressId: activeEgressId, - grantAccess: error.grant.access, - grantName: error.grant.name, - grantReason: error.grant.reason, - host: upstreamUrl.hostname, - method: request.method, - path: upstreamUrl.pathname, - provider: error.provider, - status: 401, - }), - ...routingAttributes(request, upstreamUrl), - }, ); - return authRequiredResponse({ - provider: error.provider, - grant: error.grant, - message: error.message, - }); + } catch (error) { + if (error instanceof SandboxEgressCredentialError) { + await recordAuthRequired({ + credentialContext, + provider: error.provider, + grant: error.grant, + kind: error.kind, + authorization: error.authorization, + message: error.message, + }); + const isAuthRequired = error.kind === "auth_required"; + logWarn( + isAuthRequired + ? "sandbox.egress.credential.needed" + : "sandbox.egress.credential.unavailable", + { + ...egressAttributes({ + egressId: activeEgressId, + grantAccess: error.grant.access, + grantName: error.grant.name, + grantReason: error.grant.reason, + host: upstreamUrl.hostname, + method: request.method, + path: upstreamUrl.pathname, + provider: error.provider, + status: 401, + }), + ...routingAttributes(request, upstreamUrl), + }, + ); + return authRequiredResponse({ + provider: error.provider, + grant: error.grant, + message: error.message, + }); + } + throw error; } - throw error; + }; + + let leaseOrResponse = await resolveLease(); + if (leaseOrResponse instanceof Response) { + return leaseOrResponse; } + let lease = leaseOrResponse; - const attributes = (status: number, upstream?: Response) => + const attributes = ( + activeLease: SandboxEgressCredentialLease, + status: number, + upstream?: Response, + ) => leaseLogAttributes({ egressId: activeEgressId, - lease, + lease: activeLease, provider, request, status, @@ -707,7 +720,7 @@ export async function executeCredentialedEgressRequest(input: { if (!hasSandboxEgressLeaseTransformForHost(lease, upstreamUrl.hostname)) { logWarn("sandbox.egress.transform.missing", { - ...attributes(403), + ...attributes(lease, 403), "app.sandbox.egress.transform_domains": lease.headerTransforms.map( (transform) => transform.domain, ), @@ -719,111 +732,100 @@ export async function executeCredentialedEgressRequest(input: { } const fetchImpl = deps.fetch ?? fetch; - const headers = requestHeaders( - request, - lease, - upstreamUrl.hostname, - deps.tracePropagation ?? {}, - ); const body = bodyForGrantSelection ?? (await requestBodyBytes(request)); - const intercepted = await deps.interceptHttp?.({ - provider, - request: new Request(upstreamUrl, { - method: request.method, - headers, - ...(body !== undefined ? { body } : undefined), - }), - upstreamUrl, - }); - if (intercepted) { - return intercepted; - } + // One remint retry for upstream 403 after credential injection. Intermittent + // provider denials (for example GitHub git receive-pack) should not fail the + // command before Junior replaces the cached lease and tries once more. + const maxAttempts = 2; - const upstream = await fetchImpl(upstreamUrl, { - method: request.method, - headers, - ...(body !== undefined ? { body } : undefined), - redirect: "manual", - }); - try { - const effects = await onPluginEgressResponse({ + for (let attempt = 1; attempt <= maxAttempts; attempt += 1) { + const headers = requestHeaders( + request, + lease, + upstreamUrl.hostname, + deps.tracePropagation ?? {}, + ); + const intercepted = await deps.interceptHttp?.({ provider, - grant: lease.grant, - method: request.method, - ...(operation ? { operation } : undefined), + request: new Request(upstreamUrl, { + method: request.method, + headers, + ...(body !== undefined ? { body } : undefined), + }), upstreamUrl, - response: { - headers: new Headers(upstream.headers), - readText: async (maxBytes) => - await responseTextWithinLimit(upstream, maxBytes), - status: upstream.status, - }, }); - if (effects.permissionDenied) { - await recordPermissionDenied({ - credentialContext, + if (intercepted) { + return intercepted; + } + + const requestBody = + body instanceof ArrayBuffer ? body.slice(0) : body; + const upstream = await fetchImpl(upstreamUrl, { + method: request.method, + headers, + ...(requestBody !== undefined ? { body: requestBody } : undefined), + redirect: "manual", + }); + let pluginPermissionDenied: { message: string } | undefined; + try { + const effects = await onPluginEgressResponse({ provider, - lease, - message: effects.permissionDenied.message, - upstream, + grant: lease.grant, + method: request.method, + ...(operation ? { operation } : undefined), upstreamUrl, + response: { + headers: new Headers(upstream.headers), + readText: async (maxBytes) => + await responseTextWithinLimit(upstream, maxBytes), + status: upstream.status, + }, }); - logWarn("sandbox.egress.upstream_permission.classified", { - ...attributes(upstream.status, upstream), + pluginPermissionDenied = effects.permissionDenied; + } catch (error) { + if (!isEgressAuthRequired(error)) { + throw error; + } + await clearCredentialLease(provider, lease.grant, credentialContext); + await recordAuthRequired({ + credentialContext, + provider, + grant: lease.grant, + authorization: error.authorization ?? lease.authorization, + message: error.message, + }); + logWarn("sandbox.egress.upstream_auth_requirement.classified", { + ...attributes(lease, upstream.status, upstream), + }); + await upstream.body?.cancel().catch(() => undefined); + return authRequiredResponse({ + provider, + grant: lease.grant, + message: error.message, }); } - } catch (error) { - if (!isEgressAuthRequired(error)) { - throw error; - } - await clearCredentialLease(provider, lease.grant, credentialContext); - await recordAuthRequired({ - credentialContext, - provider, - grant: lease.grant, - authorization: error.authorization ?? lease.authorization, - message: error.message, - }); - logWarn("sandbox.egress.upstream_auth_requirement.classified", { - ...attributes(upstream.status, upstream), - }); - await upstream.body?.cancel().catch(() => undefined); - return authRequiredResponse({ + logSandboxEgressUpstreamRequest({ + egressId: activeEgressId, + grantAccess: lease.grant.access, + grantName: lease.grant.name, + grantReason: lease.grant.reason, provider, - grant: lease.grant, - message: error.message, - }); - } - logSandboxEgressUpstreamRequest({ - egressId: activeEgressId, - grantAccess: lease.grant.access, - grantName: lease.grant.name, - grantReason: lease.grant.reason, - provider, - request, - upstream, - upstreamUrl, - }); - if (upstream.status >= 400) { - logWarn("sandbox.egress.upstream_response.failed", { - ...attributes(upstream.status, upstream), - "error.type": `http_${upstream.status}`, - }); - } - if ( - upstream.status === UPSTREAM_TOKEN_REJECTION_STATUS || - upstream.status === UPSTREAM_PERMISSION_REJECTION_STATUS - ) { - logWarn("sandbox.egress.upstream_auth.rejected", { - ...attributes(upstream.status, upstream), - ...(upstream.status === UPSTREAM_TOKEN_REJECTION_STATUS - ? { - "app.sandbox.egress.www_authenticate": - upstream.headers.get("www-authenticate") ?? undefined, - } - : undefined), + request, + upstream, + upstreamUrl, }); + if (upstream.status >= 400) { + logWarn("sandbox.egress.upstream_response.failed", { + ...attributes(lease, upstream.status, upstream), + "error.type": `http_${upstream.status}`, + }); + } if (upstream.status === UPSTREAM_TOKEN_REJECTION_STATUS) { + logWarn("sandbox.egress.upstream_auth.rejected", { + ...attributes(lease, upstream.status, upstream), + "app.sandbox.egress.www_authenticate": + upstream.headers.get("www-authenticate") ?? undefined, + }); await clearCredentialLease(provider, lease.grant, credentialContext); await recordAuthRequired({ credentialContext, @@ -838,22 +840,80 @@ export async function executeCredentialedEgressRequest(input: { grant: lease.grant, message: `Provider rejected the injected ${provider} credential.\n`, }); - } else { + } + if (upstream.status === UPSTREAM_PERMISSION_REJECTION_STATUS) { + logWarn("sandbox.egress.upstream_auth.rejected", { + ...attributes(lease, upstream.status, upstream), + "app.sandbox.egress.auth_attempt": attempt, + }); await clearCredentialLease(provider, lease.grant, credentialContext); + if (attempt < maxAttempts) { + await upstream.body?.cancel().catch(() => undefined); + logWarn("sandbox.egress.upstream_auth.retrying", { + ...attributes(lease, upstream.status, upstream), + "app.sandbox.egress.auth_attempt": attempt, + }); + leaseOrResponse = await resolveLease(); + if (leaseOrResponse instanceof Response) { + return leaseOrResponse; + } + lease = leaseOrResponse; + if (!hasSandboxEgressLeaseTransformForHost(lease, upstreamUrl.hostname)) { + logWarn("sandbox.egress.transform.missing", { + ...attributes(lease, 403), + "app.sandbox.egress.transform_domains": lease.headerTransforms.map( + (transform) => transform.domain, + ), + }); + return Response.json( + { error: "Credential lease does not cover forwarded host" }, + { status: 403 }, + ); + } + continue; + } + await recordPermissionDenied({ + credentialContext, + provider, + lease, + message: + pluginPermissionDenied?.message ?? + permissionDeniedMessage(provider, lease.grant), + upstream, + upstreamUrl, + }); + if (pluginPermissionDenied) { + logWarn("sandbox.egress.upstream_permission.classified", { + ...attributes(lease, upstream.status, upstream), + }); + } + return new Response(upstream.body, { + status: upstream.status, + statusText: upstream.statusText, + headers: responseHeaders(upstream), + }); + } + + if (pluginPermissionDenied) { await recordPermissionDenied({ credentialContext, provider, lease, - message: permissionDeniedMessage(provider, lease.grant), + message: pluginPermissionDenied.message, upstream, upstreamUrl, }); + logWarn("sandbox.egress.upstream_permission.classified", { + ...attributes(lease, upstream.status, upstream), + }); } + + return new Response(upstream.body, { + status: upstream.status, + statusText: upstream.statusText, + headers: responseHeaders(upstream), + }); } - return new Response(upstream.body, { - status: upstream.status, - statusText: upstream.statusText, - headers: responseHeaders(upstream), - }); + throw new Error("Credentialed egress exhausted auth attempts without a response"); } diff --git a/packages/junior/tests/component/handlers/sandbox-egress-proxy.test.ts b/packages/junior/tests/component/handlers/sandbox-egress-proxy.test.ts index fcbd7ad4e6..0c1dd4c6fe 100644 --- a/packages/junior/tests/component/handlers/sandbox-egress-proxy.test.ts +++ b/packages/junior/tests/component/handlers/sandbox-egress-proxy.test.ts @@ -726,56 +726,72 @@ describe("sandbox egress proxy composition", () => { expect(issueProviderCredentialLeaseMock).toHaveBeenCalledTimes(2); }); - it("passes through upstream 403 responses without overriding the body", async () => { + it("remints once on upstream 403 and recovers or records permission denied", async () => { setSandboxEgressUserActor(); - issueProviderCredentialLeaseMock.mockResolvedValue({ - id: "lease-1", + const lease = (token: string) => ({ + id: `lease-${token}`, provider: "sentry", env: { SENTRY_AUTH_TOKEN: "host_managed_credential" }, headerTransforms: [ - { domain: "sentry.io", headers: { Authorization: "Bearer token" } }, + { domain: "sentry.io", headers: { Authorization: `Bearer ${token}` } }, ], expiresAt: new Date(Date.now() + 60_000).toISOString(), }); - - const fetchMock = vi.fn().mockImplementation( - async () => - new Response("Permission denied for this organization", { - status: 403, - }), - ); - - const response = await proxy( + issueProviderCredentialLeaseMock + .mockResolvedValueOnce(lease("token-1")) + .mockResolvedValueOnce(lease("token-2")) + .mockResolvedValueOnce(lease("token-3")) + .mockResolvedValueOnce(lease("token-4")); + const denied = () => + new Response("Permission denied for this organization", { status: 403 }); + const fetchMock = vi + .fn() + .mockResolvedValueOnce(denied()) + .mockResolvedValueOnce(new Response("ok", { status: 200 })) + .mockResolvedValueOnce(denied()) + .mockResolvedValueOnce(denied()); + + const recovered = await proxy( egressRequest({ path: "/api/0/issues/1" }), fetchMock as typeof fetch, ); + expect(recovered.status).toBe(200); + await expect(recovered.text()).resolves.toBe("ok"); + expect(issueProviderCredentialLeaseMock).toHaveBeenCalledTimes(2); + expect(fetchMock).toHaveBeenCalledTimes(2); + expect( + new Headers(fetchMock.mock.calls[0]?.[1]?.headers).get("authorization"), + ).toBe("Bearer token-1"); + expect( + new Headers(fetchMock.mock.calls[1]?.[1]?.headers).get("authorization"), + ).toBe("Bearer token-2"); + await expect( + consumeSandboxEgressPermissionDeniedSignal(EGRESS_ID), + ).resolves.toBeUndefined(); - expect(response.status).toBe(403); - const body = await response.text(); + const persistent = await proxy( + egressRequest({ path: "/api/0/issues/2" }), + fetchMock as typeof fetch, + ); + expect(persistent.status).toBe(403); + const body = await persistent.text(); expect(body).toBe("Permission denied for this organization"); expect(body).not.toContain("junior-auth-required"); + // Second hop reuses the recovered lease, then remints once after 403. + expect(issueProviderCredentialLeaseMock).toHaveBeenCalledTimes(3); + expect(fetchMock).toHaveBeenCalledTimes(4); await expect( consumeSandboxEgressPermissionDeniedSignal(EGRESS_ID), ).resolves.toMatchObject({ provider: "sentry", - grant: { - name: "default", - access: "read", - }, + grant: { name: "default", access: "read" }, message: "sentry returned HTTP 403 after Junior injected the default grant. Junior forwarded the request; this is not a local runtime block.", source: "upstream", status: 403, upstreamHost: "sentry.io", - upstreamPath: "/api/0/issues/1", + upstreamPath: "/api/0/issues/2", }); - - const secondResponse = await proxy( - egressRequest({ path: "/api/0/issues/2" }), - fetchMock as typeof fetch, - ); - expect(secondResponse.status).toBe(403); - expect(issueProviderCredentialLeaseMock).toHaveBeenCalledTimes(2); }); it("does not apply subdomain transforms to the apex host", async () => {