fix(auth): send the ID token on AppSync requests - #162
Merged
Merged
Conversation
finding c7beb960 (high): Amplify's default userPool auth mode sends the access token on every AppSync request, but the pre-token-generation trigger's claimsOverrideDetails only decorates the ID token with custom:organization. Every access-token-authenticated request therefore reached extractOrgFromEvent with no org claim, failing org gates closed for non-admins across all 59 call sites. - server.ts: generateClient now takes a headers override (withIdTokenHeader) that fetches the current session and sets Authorization to the ID token. Applies uniformly to queries, mutations, and subscriptions since it's set once at client construction. - integrationServiceBackend.ts: same override applied to its independent generateClient() instance. - Backend verified claim-compatible: extractOrgFromEvent/isAdminFromEvent/ deriveRoles read only custom:organization, custom:role, cognito:groups, sub/username via AppSync's decoded identity — none of these differ between ID and access tokens for AppSync's purposes, and cognito:groups is present on both (confirmed by new tests), so admin detection is unaffected. No token_use/scope/client_id claim reads exist on the AppSync path. - Added regression tests: frontend asserts generateClient is constructed with the ID-token header function (not the access token); backend asserts extractOrgFromEvent resolves org from ID-token-shaped claims and returns null for access-token-shaped claims lacking custom:organization. Does not wire the AdminGetUser fallback (no USER_POOL_ID/IAM grant) — out of scope per explicit direction; recommend a follow-up to remove/annotate that dead code path separately.
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
1 participant
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.
Summary
The organisation claim never reached any resolver. The frontend sent Cognito's AppSync, but
custom:organizationaccess token to exists only in the ID token - the pre-token-generation trigger runs atV1_0, which decorates the ID token alone. Live Cloudwatch evidence: the AppSync event carriedtoken_use: "access"and no custom claim.So
extractOrgFromEventreturned null for every user at all 59 call sites across 22 files. Every organisation gate failed closed for non-admins, while admins passed via bypass. TheAdminGetUserfallback that appears to cover this is dead twice over on the deployed resolver: noUSER_POOL_IDenvironment variable, so it returns null at the guard, and nocognito-idppermission on the role.This is fail-closed, so it is a functional defect rather than cross-tenant exposure - but it means the organisation isolation shipped recently has never actually executed. It is likely why governance findings still showed no stamped organisation after the stamping change deployed, and why datastore rows carry the UI selector label instead of a tenant.
Changes
Both
generateClient()sites now send the ID token via sharedwithIdTokenHeaderoverride:services/server.ts- backs every query, mutation and subscription, since the subscription call sites all route throughserverServiceservices/integrationServiceBackend.ts- constructs its own independent clientA fix covering queries but not subscriptions would have looked correct while leaving live updates unauthorised, so both agents enumerated the sites independently.
Claim compatibility, verified not assumed
custom:organizationextractorgFromEvent- the bugcognito:groupsisAdminFromEvent,deriveRolessub/usernameAdmin detection is unaffected -
cognito:groupsis in both, and that is asserted by test across both token shapes rather than reasoned about, since breaking admin detection would be worse than the original bug. No code on the AppSync path readstoken_use,scopeorclient_id;auth.ts'sGetUserCommandflow is a separate REST path, untouched.Deliberately not done
No
USER_POOL_IDenvironment variable and nocognito-idpgrant - wiring the fallback was rejected because it would add a per-request Cognito call to every gated operation and widen IAM across 22 files to carry data the token should hold natively. No backend authorization logic changed.Testing
Regression tests on both sides, proven to bite: reverting the client to the access token produces five failures, restored byte-identically.
Frontend tsc 0, jest 2073, build 0; backend tsc 0, lint 0, jest 7872.
Honest limit: local tests cannot confirm the deployed AppSync authorizer accepts the ID token, or that the claim arrives end to end. That requires a deployed test.
Follow-ups
AdminGetUserfallback is dead code that reads like safety net - remove or annotate it