fix: JWT Algorithm Confusion in Google Auth Adapter (GHSA-4q3h-vp4r-prv2)#10072
Merged
mtrezza merged 3 commits intoparse-community:alphafrom Feb 23, 2026
Merged
Conversation
|
I will reformat the title to use the proper commit message syntax. |
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughJWT verification hardened across Apple, Facebook, and Google adapters: header-provided alg is no longer trusted (RS256 enforced). Google adapter switched to JWKS-based signing-key retrieval with caching and explicit error when key not found. Tests updated/added to cover forgery and key-handling scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Adapter as Auth Adapter
participant JWKS as JWKS Cache/Client
participant jwtLib as jwt.verify
Client->>Adapter: send id_token
Adapter->>Adapter: extract kid from token header
Adapter->>JWKS: getSigningKey(kid, cache options)
JWKS-->>Adapter: signingKey (or error if not found)
Adapter->>jwtLib: verify(token, signingKey, algorithms:['RS256'], audience: clientId)
jwtLib-->>Adapter: verified claims (or verification error)
Adapter-->>Client: auth result / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
spec/AuthenticationAdapters.spec.js (1)
503-519: Minor: consider also asserting the error message, not just the code.The test correctly validates that a forged
alg:nonetoken is rejected withParse.Error.OBJECT_NOT_FOUND. For robustness, you could also assert the error message contains a jwt-related reason (e.g.,expect(e.message).toMatch(/invalid/)or similar) to ensure the rejection comes from jwt.verify's algorithm check rather than some other code path. This is optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/AuthenticationAdapters.spec.js` around lines 503 - 519, Add an assertion that the thrown error's message indicates a JWT/algorithm issue in the existing test that calls google.validateAuthData (the 'should reject forged alg:none JWT...' test): after catching the error (variable e) and asserting e.code === Parse.Error.OBJECT_NOT_FOUND, also assert e.message matches a jwt-related pattern (e.g. /invalid|algorithm|jwt/i) to ensure the rejection is due to jwt verification and not another code path; keep the existing spyOn(authUtils, 'getSigningKey') and forgedToken setup unchanged.src/Adapters/Auth/google.js (1)
112-117: Redundant audience check —jwt.verifyalready validates this.The
audience: clientIdoption on line 94 causesjwt.verifyto throw if the audience doesn't match. This manual check at lines 112-117 is unreachable whenclientIdis truthy (jwt.verify throws first), and skipped whenclientIdis falsy. Consider removing it to reduce dead code.Note: this predates this PR, so feel free to defer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Adapters/Auth/google.js` around lines 112 - 117, Remove the redundant manual audience validation that compares jwtClaims.aud to clientId and throws a Parse.Error; jwt.verify already enforces the audience when called with the audience: clientId option. Specifically, delete the if (clientId && jwtClaims.aud !== clientId) { throw new Parse.Error(...); } block (which references jwtClaims, clientId and Parse.Error) so the code relies on jwt.verify for audience checks and avoid unreachable/dead code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@spec/AuthenticationAdapters.spec.js`:
- Around line 503-519: Add an assertion that the thrown error's message
indicates a JWT/algorithm issue in the existing test that calls
google.validateAuthData (the 'should reject forged alg:none JWT...' test): after
catching the error (variable e) and asserting e.code ===
Parse.Error.OBJECT_NOT_FOUND, also assert e.message matches a jwt-related
pattern (e.g. /invalid|algorithm|jwt/i) to ensure the rejection is due to jwt
verification and not another code path; keep the existing spyOn(authUtils,
'getSigningKey') and forgedToken setup unchanged.
In `@src/Adapters/Auth/google.js`:
- Around line 112-117: Remove the redundant manual audience validation that
compares jwtClaims.aud to clientId and throws a Parse.Error; jwt.verify already
enforces the audience when called with the audience: clientId option.
Specifically, delete the if (clientId && jwtClaims.aud !== clientId) { throw new
Parse.Error(...); } block (which references jwtClaims, clientId and Parse.Error)
so the code relies on jwt.verify for audience checks and avoid unreachable/dead
code.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
spec/AuthenticationAdapters.spec.jssrc/Adapters/Auth/apple.jssrc/Adapters/Auth/facebook.jssrc/Adapters/Auth/google.js
mtrezza
changed the title
Member
Author
|
Commit message:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
spec/index.spec.js (2)
676-676:alg: 'RS256'infakeDecodedTokenis dead test data — consider removing or inverting itThe hardened adapters no longer read
algfrom the token header (that's the exact vulnerability being fixed). Keepingalg: 'RS256'in the mock silently implies the adapter may still use it. Removing it keeps the mock honest; alternatively, setting it toalg: 'none'would make this test simultaneously confirm that the header'salgis ignored even when adversarially set.♻️ Suggested change
- const fakeDecodedToken = { kid: '123', alg: 'RS256' }; + const fakeDecodedToken = { kid: '123', alg: 'none' }; // alg is intentionally ignored by the hardened adapter🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/index.spec.js` at line 676, Test mock includes a misleading alg header; update the fakeDecodedToken used in the spec (variable fakeDecodedToken) to either remove the alg property entirely or set alg: 'none' so the test no longer implies the adapter reads header alg — change the fakeDecodedToken declaration accordingly (remove alg or set to 'none') and re-run the tests to ensure they still validate that header.alg is ignored.
662-689: Test still uses thedone-callback pattern — prefer async/await♻️ Suggested refactor
- it('should not fail when Google signin is introduced without the optional clientId', done => { + it('should not fail when Google signin is introduced without the optional clientId', async () => { const jwt = require('jsonwebtoken'); const authUtils = require('../lib/Adapters/Auth/utils'); - reconfigureServer({ + await reconfigureServer({ auth: { google: {} }, - }) - .then(() => { - const fakeClaim = { - iss: 'https://accounts.google.com', - aud: 'secret', - exp: Date.now(), - sub: 'the_user_id', - }; - const fakeDecodedToken = { kid: '123', alg: 'RS256' }; - const fakeSigningKey = { kid: '123', rsaPublicKey: 'the_rsa_public_key' }; - spyOn(authUtils, 'getHeaderFromToken').and.callFake(() => fakeDecodedToken); - spyOn(authUtils, 'getSigningKey').and.resolveTo(fakeSigningKey); - spyOn(jwt, 'verify').and.callFake(() => fakeClaim); - const user = new Parse.User(); - user - .linkWith('google', { - authData: { id: 'the_user_id', id_token: 'the_token' }, - }) - .then(done); - }) - .catch(done.fail); + }); + const fakeClaim = { + iss: 'https://accounts.google.com', + aud: 'secret', + exp: Date.now(), + sub: 'the_user_id', + }; + const fakeDecodedToken = { kid: '123', alg: 'RS256' }; + const fakeSigningKey = { kid: '123', rsaPublicKey: 'the_rsa_public_key' }; + spyOn(authUtils, 'getHeaderFromToken').and.callFake(() => fakeDecodedToken); + spyOn(authUtils, 'getSigningKey').and.resolveTo(fakeSigningKey); + spyOn(jwt, 'verify').and.callFake(() => fakeClaim); + const user = new Parse.User(); + await user.linkWith('google', { + authData: { id: 'the_user_id', id_token: 'the_token' }, + }); });Based on learnings: new and modified tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with
done().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/index.spec.js` around lines 662 - 689, The test "should not fail when Google signin is introduced without the optional clientId" still uses the done callback pattern; convert it to an async function and use await instead: make the spec async, await reconfigureServer(...), set up the spies (authUtils.getHeaderFromToken, authUtils.getSigningKey, jwt.verify) as before, then await user.linkWith('google', { authData: {...} }) and let the test complete by returning from the async function (remove done and done.fail). Ensure the test name and spy references (authUtils.getHeaderFromToken, authUtils.getSigningKey, jwt.verify, user.linkWith) remain the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spec/index.spec.js`:
- Around line 677-679: The test mock for authUtils.getSigningKey (the
fakeSigningKey used in the spyOn(...).and.resolveTo(...) call) supplies only
rsaPublicKey but the adapter checks googleKey.publicKey first (in google.js), so
update the fakeSigningKey to include a publicKey property (e.g., { kid: '123',
publicKey: 'the_rsa_public_key' }) so the test exercises the normal code path;
leave the getHeaderFromToken spy as-is.
---
Nitpick comments:
In `@spec/index.spec.js`:
- Line 676: Test mock includes a misleading alg header; update the
fakeDecodedToken used in the spec (variable fakeDecodedToken) to either remove
the alg property entirely or set alg: 'none' so the test no longer implies the
adapter reads header alg — change the fakeDecodedToken declaration accordingly
(remove alg or set to 'none') and re-run the tests to ensure they still validate
that header.alg is ignored.
- Around line 662-689: The test "should not fail when Google signin is
introduced without the optional clientId" still uses the done callback pattern;
convert it to an async function and use await instead: make the spec async,
await reconfigureServer(...), set up the spies (authUtils.getHeaderFromToken,
authUtils.getSigningKey, jwt.verify) as before, then await
user.linkWith('google', { authData: {...} }) and let the test complete by
returning from the async function (remove done and done.fail). Ensure the test
name and spy references (authUtils.getHeaderFromToken, authUtils.getSigningKey,
jwt.verify, user.linkWith) remain the same.
parseplatformorg
pushed a commit
that referenced
this pull request
Feb 23, 2026
## [9.3.1-alpha.4](9.3.1-alpha.3...9.3.1-alpha.4) (2026-02-23) ### Bug Fixes * JWT Algorithm Confusion in Google Auth Adapter ([GHSA-4q3h-vp4r-prv2](https://github.com/parse-community/parse-server/security/advisories/GHSA-4q3h-vp4r-prv2)) ([#10072](#10072)) ([9d5942d](9d5942d))
Contributor
|
🎉 This change has been released in version 9.3.1-alpha.4 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #10072 +/- ##
=======================================
Coverage 92.60% 92.61%
=======================================
Files 191 191
Lines 15745 15711 -34
Branches 176 176
=======================================
- Hits 14581 14551 -30
+ Misses 1152 1148 -4
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
2 participants
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.
Pull Request
Issue
JWT Algorithm Confusion in Google Auth Adapter (GHSA-4q3h-vp4r-prv2).
Tasks
Summary by CodeRabbit
Bug Fixes
Enhancements
Tests