Repository navigation
fix(egress): Add sandbox credentials only for placeholder requests - #2191
Draft
sentry-junior[bot] wants to merge 1 commit into
Draft
sentry-junior[bot] wants to merge 1 commit into
sentry-junior[bot] wants to merge 1 commit into
Conversation
The sandbox egress proxy added plugin credentials to every request to a plugin domain. A browser in the sandbox could not keep its own login on provider sites such as sentry.io, because the injected token won over the session cookie. The proxy now adds credentials only when an auth header carries a `host_managed_credential` placeholder, in any scheme (Bearer, token, API key header, or Basic password). Other requests are forwarded without credentials and keep their cookies. Plugin tool egress on the host is unchanged. Git and the tts script sent no auth header and relied on injection, so they now send the placeholder. Refs #2190 Co-Authored-By: David Cramer <david@sentry.io>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
1 screenshot change — 1 changed · 0 added · 0 removed Review screenshots in Frameshift
|
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PoC for #2190.
The sandbox egress proxy added plugin credentials to every request to a plugin domain. A browser in the sandbox could not log in to a provider site as another account. For example,
qa-sentry-frontendonsentry.iocould not keep its own login. Sentry'sDEFAULT_AUTHENTICATIONruns token auth beforeSessionAuthentication, so the injected Bearer token wins over the session cookie.Now the sandbox proxy adds credentials only when an auth header carries the
host_managed_credentialplaceholder. All other requests go upstream without credentials and keep their own headers and cookies. These requests skip grant selection and lease issuance, so a browser request no longer causes ajunior-auth-required401.What each client sends (I checked each one against a local capture server or
GH_DEBUG):Authorization: Bearer host_managed_credentialAuthorization: Bearer host_managed_credentialAuthorization: token ghp_host_managed_credentialDD-API-KEY/DD-APPLICATION-KEY= placeholdercredential.helper=+http.emptyAuth)x-access-token:ghp_host_managed_credentialthroughhttp.https://github.com/.extraHeader(insandboxPrepareand in workspace clone/fetch)speech.mjsAuthorization: Bearer host_managed_credentialctx.egress.fetch)Google already rejects sandbox egress. MCP-only plugins declare no domains.
Changes
auth-token-placeholder.ts: adds theHOST_MANAGED_CREDENTIAL_MARKERconstant andhasHostManagedCredentialPlaceholder(). The function checks every header exceptCookieand decodesBasicauth.sandbox/egress/proxy.ts: requests with the placeholder go toexecuteCredentialedEgressRequest. All other requests go to the newforwardEgressRequestWithoutCredentials.egress/headers.ts: request and response header building moved out ofcredentialed.ts, so that file stays under 1,000 lines.policies/security.md: documents the rule. It also says the check is not a security edge, because the placeholder is public.Known gaps (PoC)
api.github.com. That is still stricter than before. A follow-up could use a random placeholder per sandbox.auth-token-placeholdervalues contain the marker. All placeholders in this repo do. App-local plugins (junior-prod) are not checked.skills/tts/scripts/speech.mjs) needs the same one-line header change after a release.gh,sentry,pup, and a browser login on sentry.io) is needed before this leaves draft.Checks run
vitest run tests/integration/sandbox-egress-proxy.test.ts tests/component/handlers/sandbox-egress-proxy.test.ts tests/unit/misc/noninteractive-command.test.ts: 61 passed. The new integration case fails when the placeholder check is removed.packages/junior-githubvitest: all passed.packages/junior-ttsnode --test: 5 passed.pnpm typecheck,pnpm lint(includes file-length): passed.event-automations/slack-schedule-toolstimed out under a 35-file parallel run. They pass when run alone, and this PR does not change them.via David Cramer.
--
View Junior Session [Sentry]