feat: Add LINE id_token validation#10436
Open
swittk wants to merge 3 commits intoparse-community:alphafrom
Open
feat: Add LINE id_token validation#10436swittk wants to merge 3 commits intoparse-community:alphafrom
swittk wants to merge 3 commits intoparse-community:alphafrom
Conversation
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
📝 WalkthroughWalkthroughAdded JWT-based LINE id_token verification, secure-options validation, and lifecycle integration (beforeFind) to enforce id/nonce/iss checks; updated code-exchange to require clientSecret, normalized user id mapping, and expanded test coverage and E2E tests for secure and legacy flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ParseAdapter as Adapter
participant LINE_JWKS as "LINE JWKS (external)"
participant JWTLib as "jwt.verify"
note over Client,Adapter: Flow: login with id_token or code
Client->>Adapter: send authData { id_token } / { code }
alt id_token path
Adapter->>JWTLib: parse header (kid, alg)
alt alg == ES256
Adapter->>LINE_JWKS: fetch JWKS (cached)
LINE_JWKS-->>Adapter: jwk (public key)
Adapter->>JWTLib: verify token with public key (alg: ES256)
else alg == HS256
Adapter->>JWTLib: verify token with clientSecret (alg: HS256)
end
JWTLib-->>Adapter: verified claims
Adapter->>Adapter: validate iss, sub/id, nonce
Adapter-->>Client: success (authData.id set, id_token removed)
else code exchange path
Adapter->>LINE_JWKS: (not used)
Adapter->>Adapter: require clientSecret else error
Adapter->>LINE_API: exchange code for access_token (server-side)
LINE_API-->>Adapter: access_token
Adapter->>LINE_API: fetch profile with access_token
LINE_API-->>Adapter: profile
Adapter-->>Client: normalized user { id, ... }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ 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 |
swittk
changed the title
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
![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/Adapters/Auth/line.spec.js (1)
15-34: Minor: JWTexpclaim should be in seconds, not milliseconds.The mock uses
Date.now() + 1000for theexpclaim, but JWT expiration is specified in seconds since epoch, not milliseconds. While this doesn't affect test execution (sincejwt.verifyis mocked), using accurate mock data improves test clarity.🔧 Proposed fix
function mockEs256IdTokenVerification(claims = {}) { const jwtClaims = { iss: 'https://access.line.me', aud: 'validClientId', - exp: Date.now() + 1000, + exp: Math.floor(Date.now() / 1000) + 3600, sub: 'mockUserId', ...claims, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/Adapters/Auth/line.spec.js` around lines 15 - 34, In mockEs256IdTokenVerification update the jwtClaims.exp to be in seconds instead of milliseconds by replacing Date.now() + 1000 with a seconds-based value (e.g., Math.floor(Date.now() / 1000) + 1) so the mocked exp claim correctly represents seconds-since-epoch; modify the exp assignment inside the jwtClaims object in mockEs256IdTokenVerification accordingly.src/Adapters/Auth/line.js (1)
106-130: JWKS client is recreated on every call, defeating the cache.The
jwksClientinstance is created fresh each timegetLineKeyByKeyIdis invoked. Sincejwks-rsamaintains its cache per-client instance, everyverifyIdTokencall will fetch keys from LINE's JWKS endpoint anew rather than reusing cached keys.Consider caching the client at the adapter instance level (similar to how other auth adapters handle JWKS clients).
♻️ Proposed refactor to cache the JWKS client
-const getLineKeyByKeyId = async ( - /** `@type` {string} The JWT header `kid`. */ - keyId, - /** Maximum number of cached JWKS entries */ - cacheMaxEntries, - /** Maximum JWKS cache age in milliseconds */ - cacheMaxAge) => { - const client = jwksClient({ - jwksUri: 'https://api.line.me/oauth2/v2.1/certs', - cache: true, - cacheMaxEntries, - cacheMaxAge, - }); - - let key; - try { - key = await authUtils.getSigningKey(client, keyId); - } catch { - throw new Parse.Error( - Parse.Error.OBJECT_NOT_FOUND, - `Unable to find matching key for Key ID: ${keyId}` - ); - } - return key; -}; +let cachedJwksClient = null; +let cachedJwksConfig = null; + +const getJwksClient = (cacheMaxEntries, cacheMaxAge) => { + const configKey = `${cacheMaxEntries}:${cacheMaxAge}`; + if (!cachedJwksClient || cachedJwksConfig !== configKey) { + cachedJwksClient = jwksClient({ + jwksUri: 'https://api.line.me/oauth2/v2.1/certs', + cache: true, + cacheMaxEntries, + cacheMaxAge, + }); + cachedJwksConfig = configKey; + } + return cachedJwksClient; +}; + +const getLineKeyByKeyId = async (keyId, cacheMaxEntries, cacheMaxAge) => { + const client = getJwksClient(cacheMaxEntries, cacheMaxAge); + let key; + try { + key = await authUtils.getSigningKey(client, keyId); + } catch { + throw new Parse.Error( + Parse.Error.OBJECT_NOT_FOUND, + `Unable to find matching key for Key ID: ${keyId}` + ); + } + return key; +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Adapters/Auth/line.js` around lines 106 - 130, getLineKeyByKeyId creates a new jwksClient on every call which defeats jwks-rsa caching; move creation of the jwksClient to the adapter instance (e.g., this.jwksClient or a private field) and reuse it from getLineKeyByKeyId, only constructing it once (lazily if needed) with cache, cacheMaxEntries and cacheMaxAge applied; update getLineKeyByKeyId to call authUtils.getSigningKey(this.jwksClient, keyId) (or initialize this.jwksClient first) and keep the same Parse.Error handling.
🤖 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/Adapters/Auth/line.spec.js`:
- Around line 15-34: In mockEs256IdTokenVerification update the jwtClaims.exp to
be in seconds instead of milliseconds by replacing Date.now() + 1000 with a
seconds-based value (e.g., Math.floor(Date.now() / 1000) + 1) so the mocked exp
claim correctly represents seconds-since-epoch; modify the exp assignment inside
the jwtClaims object in mockEs256IdTokenVerification accordingly.
In `@src/Adapters/Auth/line.js`:
- Around line 106-130: getLineKeyByKeyId creates a new jwksClient on every call
which defeats jwks-rsa caching; move creation of the jwksClient to the adapter
instance (e.g., this.jwksClient or a private field) and reuse it from
getLineKeyByKeyId, only constructing it once (lazily if needed) with cache,
cacheMaxEntries and cacheMaxAge applied; update getLineKeyByKeyId to call
authUtils.getSigningKey(this.jwksClient, keyId) (or initialize this.jwksClient
first) and keep the same Parse.Error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1415cab2-f414-4834-b557-e23e50d5e794
📒 Files selected for processing (2)
spec/Adapters/Auth/line.spec.jssrc/Adapters/Auth/line.js
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Adapters/Auth/line.spec.js`:
- Around line 67-69: The test's expected error message does not match what the
adapter actually throws when id_token is missing; update the assertion in
spec/Adapters/Auth/line.spec.js that calls adapter.verifyIdToken({}) to expect
the adapter's actual message ("Line auth is invalid for this user.") instead of
"id token is invalid for this user.", so the call to adapter.verifyIdToken({})
and its rejection check match the thrown error string.
- Around line 78-84: The test currently stubs authUtils.getHeaderFromToken but
still allows verifyIdToken to perform a real signing-key lookup, making it
flaky; update the test to also stub the signing-key lookup (e.g.,
authUtils.getSigningKey or the method your adapter calls to fetch the public
key) to synchronously return null or throw a deterministic error so no network
request is made, then assert adapter.verifyIdToken({ id_token: 'the_token' }) is
rejected with 'Unable to find matching key for Key ID: 789'—this ensures the
missing-public-key path is fully offline and deterministic while still
referencing getHeaderFromToken and verifyIdToken.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d5aa1868-7af1-4b3f-8924-596de6f312a8
📒 Files selected for processing (1)
spec/Adapters/Auth/line.spec.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
spec/Adapters/Auth/line.spec.js (1)
87-97: Add a regression test for unsupportedalgvalues.You cover the positive ES256 guard well, but the explicit rejection branch for unsupported algorithms (e.g.,
alg: 'none'orRS256) should be locked with a dedicated test to prevent security regressions.Suggested test addition
+ it('should reject unsupported id_token signing algorithms', async function () { + spyOn(authUtils, 'getHeaderFromToken').and.returnValue({ kid: '123', alg: 'none' }); + + await expectAsync(adapter.verifyIdToken({ id_token: 'the_token' })).toBeRejectedWithError( + 'Unsupported Line id_token signing algorithm: none' + ); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/Adapters/Auth/line.spec.js` around lines 87 - 97, Add a regression test that sends an id_token whose JWT header contains an unsupported alg (e.g., "none" or "RS256") and asserts adapter.verifyIdToken rejects/throws rather than accepting it; locate the test suite that calls adapter.verifyIdToken (the existing spec using mockEs256IdTokenVerification) and add a new it() that stubs/mock the token header to have alg 'none' (or 'RS256'), then assert the promise rejects (or throws) and that jwt.verify is not invoked with an unsafe algorithm (or is not called at all), referencing adapter.verifyIdToken and jwt.verify in your assertions.
🤖 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/Adapters/Auth/line.spec.js`:
- Around line 87-97: Add a regression test that sends an id_token whose JWT
header contains an unsupported alg (e.g., "none" or "RS256") and asserts
adapter.verifyIdToken rejects/throws rather than accepting it; locate the test
suite that calls adapter.verifyIdToken (the existing spec using
mockEs256IdTokenVerification) and add a new it() that stubs/mock the token
header to have alg 'none' (or 'RS256'), then assert the promise rejects (or
throws) and that jwt.verify is not invoked with an unsafe algorithm (or is not
called at all), referencing adapter.verifyIdToken and jwt.verify in your
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e0940f60-e5a1-4be1-abc8-e32bb744c316
📒 Files selected for processing (1)
spec/Adapters/Auth/line.spec.js
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.
Pull Request
Issue
Parse Server's LINE auth adapter only supported secure server-side
code + redirect_uriexchange, or the deprecatedinsecure id + access_tokenflow. Unlike the Google / Apple / Facebook adapters, it did not support validating a secure client-provided token directly.This blocks architecture where LINE login is handled by a separate auth service and the Parse Server should only validate the resulting LINE identity without holding the LINE client secret.
Approach
Added secure LINE OpenID Connect
id_tokenvalidation to the LINE auth adapter.code + redirect_uriflow continues workingid_tokenvalidation using LINE signing keys / supported algorithmsallowsInsecureAuthflag path for legacy fallback (insecureid + access_token) when enableduserIdtoidso the LINE adapter matches the BaseCodeAuthAdapter contractTasks
Summary by CodeRabbit
New Features
Improvements
Tests