fix: refuse unverified OAuth emails, verify Play pushes, stop trusting a client IP - #285
Merged
Conversation
…g a client-written IP Two of the "known but not fixed" items from the review, neither of which needed anything from Google or from Play Console. The Play notification endpoint now prefers the OIDC identity token Pub/Sub signs its pushes with: the signature is checked against Google's published keys, and the audience and the service account it is signed as must both match. The shared secret stays as a fallback for a subscription created without OIDC, and is still weaker for the reason it always was - a query string is written into this service's access logs and every proxy's. With neither configured the endpoint stays closed. `request.ip` is the left-hand end of a client-written X-Forwarded-For while the app runs trustProxy: true, so the address a session recorded as its device was whatever the caller typed. The rate limiter already reads the edge address; the same helper now serves the refresh-token rows, and lives in one place rather than inside the rate-limit plugin. Replacing trustProxy: true with a hop count or the edge's CIDR is still the proper fix and still needs the deployment's exact shape; this closes the two places where the difference is exploitable rather than cosmetic.
Found by the audit of the pre-existing codebase. An OAuth login matches an existing account by email alone - it never looked at whether the provider had verified that address, and never at which provider identity had been bound to the account before. GitHub returns the flag and it was read and then ignored; Google was worse, never fetching `verified_email` and hardcoding `isEmailVerified: true` onto the account it created. So registering at the provider with somebody else's address, authorising, and exchanging the code returns their session. No password, no mailbox access, no action from them. Both flows now refuse an unverified address before the account is even looked up, and Google reads the flag rather than assuming it. Binding the login to `(provider, providerAccountId)` is the stronger shape and is left for its own change; the verification check is what closes the hole. Also from the audit: profile `socials` accepted any URI scheme - `format: "uri"` asks for a scheme and nothing more, so `javascript:` passed - and a profile is public, which made it stored XSS in whichever client renders the value as a link. Links are now checked for http/https, keys constrained to platform names, and both count and length capped. Two smaller ones: the profile controller's last identity-after-spread is reordered (not exploitable today - the schema and AJV both stop it - but the use case authorises on that field), and three profile routes that verified a JWT inline now use `optionalAuthenticate`, so a suspended account stops being treated as signed in there the way it does everywhere else.
aquie00t
changed the title
github-actions Bot
pushed a commit
that referenced
this pull request
Sep 6, 2026
## [1.27.2](v1.27.1...v1.27.2) (2026-09-06) ### Bug Fixes * refuse unverified OAuth emails, verify Play pushes, stop trusting a client IP ([#285](#285)) ([a586af2](a586af2))
|
🎉 This PR is included in version 1.27.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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.
Two of the items I listed as "known but not fixed" after the security review. Neither needed anything from Google or from Play Console, so leaving them open was not defensible.
Play notifications are now proved, not guessed
POST /billing/play/notificationsprefers the OIDC identity token Pub/Sub signs its pushes with. The signature is checked against Google's published keys (https://www.googleapis.com/oauth2/v3/certs, cached for an hour, refetched on an unknown key id so a rotation does not lock us out), and both the audience and the service account it is signed as must match.The shared secret stays as a fallback for a subscription created without OIDC, and it is still the weaker of the two for the reason it always was: a query string is written into this service's access logs and into every proxy's. With neither configured the endpoint stays closed, which is the right default for an unauthenticated route that writes billing state.
Written against Node's own crypto rather than a JWT dependency — the check is a signature, three claims and an expiry, and Google publishes its keys as a JWK set
createPublicKeyreads directly.Sessions stop recording an address the caller typed
The app runs
trustProxy: true, sorequest.ipis the left-hand end of a client-writtenX-Forwarded-For. The rate limiter already reads the edge address after the last review; the same helper now serves thedeviceIpon refresh-token rows, which was otherwise whatever the caller felt like sending — an audit trail that lies on request. The helper moved out of the rate-limit plugin into one place both use.Still open, and named honestly: replacing
trustProxy: truewith a hop count or the edge's CIDR ranges is the proper fix, and it needs the deployment's exact shape (how many proxies sit in front, in what order). This closes the two places where the difference was exploitable rather than cosmetic.Tests
Unit, 1548 passing (12 new), all on the verifier and all of them attacks: a token signed by somebody else, an
alg: nonetoken, a token minted for another audience, one from another service account, an expired one, a non-Google issuer, an unverified email claim, an unpublished key id, five malformedAuthorizationheaders, an unconfigured verifier refusing a valid token, and the key set being fetched once rather than per request. The keys are generated in the test and the JWKS endpoint is stubbed, so nothing reaches the network.tsc -p tsconfig.build.json --noEmit,eslint,prettier --checkclean.Both settings default to empty, so this merges and deploys with behaviour unchanged.
AI Asistan: Opus 5