fix: save and return PKCE code_challenge in OAuth2 authorization code#39465
Open
subhaushsingh wants to merge 1 commit intoRocketChat:developfrom
Open
fix: save and return PKCE code_challenge in OAuth2 authorization code#39465subhaushsingh wants to merge 1 commit intoRocketChat:developfrom
subhaushsingh wants to merge 1 commit intoRocketChat:developfrom
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 3f38217 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (4)📓 Common learnings📚 Learning: 2026-01-17T01:51:47.764ZApplied to files:
📚 Learning: 2026-02-26T19:25:44.063ZApplied to files:
📚 Learning: 2026-02-26T19:25:44.063ZApplied to files:
🔇 Additional comments (2)
WalkthroughPersisted PKCE parameters by adding Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/tests/end-to-end/api/oauth-server.ts">
<violation number="1" location="apps/meteor/tests/end-to-end/api/oauth-server.ts:206">
P2: PKCE cleanup hook unconditionally deletes using `pkceAppId` and expects 200 even when `pkceAppId` may be unset, which can mask the real failing test with a secondary teardown failure.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/meteor/tests/end-to-end/api/oauth-server.ts (2)
179-262: Inconsistent indentation style.The PKCE test suite uses 4-space indentation while the rest of the file uses tabs. This should be consistent with the existing file style.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/end-to-end/api/oauth-server.ts` around lines 179 - 262, The new PKCE test block (describe('[PKCE flow]') including functions generateCodeVerifier and generateCodeChallenge and tests that reference pkceAppId/pkceClientId/pkceCode/etc.) uses 4-space indentation which is inconsistent with the rest of the file's tab-based style; update the entire describe block and its nested functions/its/it tests to use tabs for indentation to match existing formatting in the file so linting/format checks pass.
247-257: Consider adding a negative test for invalid code_verifier.To fully validate the PKCE implementation, consider adding a test that verifies token exchange fails when an incorrect
code_verifieris provided.📝 Suggested additional test
it('should fail token exchange with invalid code_verifier', async () => { const invalidVerifier = 'invalid_verifier_that_does_not_match'; await request .post('/oauth/token') .type('form') .send({ grant_type: 'authorization_code', code: pkceCode, client_id: pkceClientId, client_secret: pkceClientSecret, redirect_uri: pkceRedirectUri, code_verifier: invalidVerifier, }) .expect(400); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/end-to-end/api/oauth-server.ts` around lines 247 - 257, Add a negative PKCE test that attempts the token exchange with an incorrect code_verifier to ensure the server rejects it: create a new it block (e.g., "should fail token exchange with invalid code_verifier") that posts to '/oauth/token' using the same parameters (grant_type: 'authorization_code', code: pkceCode, client_id: pkceClientId, client_secret: pkceClientSecret, redirect_uri: pkceRedirectUri) but sets code_verifier to an invalid string and asserts the response status is 400; reuse the request test helper and existing pkce* variables (pkceCode, pkceClientId, pkceClientSecret, pkceRedirectUri) used in the successful test.apps/meteor/server/oauth2-server/model.ts (1)
155-156: Inconsistent indentation on line 156.Line 156 appears to have extra leading whitespace compared to adjacent lines.
🔧 Suggested fix
codeChallenge: code.codeChallenge, - codeChallengeMethod: code.codeChallengeMethod, + codeChallengeMethod: code.codeChallengeMethod,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/oauth2-server/model.ts` around lines 155 - 156, The line assigning codeChallengeMethod has extra leading whitespace causing inconsistent indentation; open the block where codeChallenge: code.codeChallenge and codeChallengeMethod: code.codeChallengeMethod are set (look for those property assignments in model.ts) and remove the extra spaces so codeChallengeMethod: code.codeChallengeMethod aligns exactly with the codeChallenge line, preserving the surrounding indentation style in that object literal or return statement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/server/oauth2-server/model.ts`:
- Around line 155-156: The line assigning codeChallengeMethod has extra leading
whitespace causing inconsistent indentation; open the block where codeChallenge:
code.codeChallenge and codeChallengeMethod: code.codeChallengeMethod are set
(look for those property assignments in model.ts) and remove the extra spaces so
codeChallengeMethod: code.codeChallengeMethod aligns exactly with the
codeChallenge line, preserving the surrounding indentation style in that object
literal or return statement.
In `@apps/meteor/tests/end-to-end/api/oauth-server.ts`:
- Around line 179-262: The new PKCE test block (describe('[PKCE flow]')
including functions generateCodeVerifier and generateCodeChallenge and tests
that reference pkceAppId/pkceClientId/pkceCode/etc.) uses 4-space indentation
which is inconsistent with the rest of the file's tab-based style; update the
entire describe block and its nested functions/its/it tests to use tabs for
indentation to match existing formatting in the file so linting/format checks
pass.
- Around line 247-257: Add a negative PKCE test that attempts the token exchange
with an incorrect code_verifier to ensure the server rejects it: create a new it
block (e.g., "should fail token exchange with invalid code_verifier") that posts
to '/oauth/token' using the same parameters (grant_type: 'authorization_code',
code: pkceCode, client_id: pkceClientId, client_secret: pkceClientSecret,
redirect_uri: pkceRedirectUri) but sets code_verifier to an invalid string and
asserts the response status is 400; reuse the request test helper and existing
pkce* variables (pkceCode, pkceClientId, pkceClientSecret, pkceRedirectUri) used
in the successful test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f61cea3-b08d-48d1-b5f8-87bc4bf13094
📒 Files selected for processing (4)
.changeset/fix-pkce-code-verifier.mdapps/meteor/server/oauth2-server/model.tsapps/meteor/tests/end-to-end/api/oauth-server.tspackages/core-typings/src/IOAuthAuthCode.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/core-typings/src/IOAuthAuthCode.tsapps/meteor/tests/end-to-end/api/oauth-server.tsapps/meteor/server/oauth2-server/model.ts
🧠 Learnings (13)
📓 Common learnings
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/core-typings/src/IOAuthAuthCode.tsapps/meteor/tests/end-to-end/api/oauth-server.tsapps/meteor/server/oauth2-server/model.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/core-typings/src/IOAuthAuthCode.tsapps/meteor/tests/end-to-end/api/oauth-server.tsapps/meteor/server/oauth2-server/model.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/end-to-end/api/oauth-server.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/end-to-end/api/oauth-server.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/oauth-server.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/end-to-end/api/oauth-server.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/oauth-server.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/oauth-server.ts
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
.changeset/fix-pkce-code-verifier.md
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
.changeset/fix-pkce-code-verifier.md
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
.changeset/fix-pkce-code-verifier.md
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Applied to files:
.changeset/fix-pkce-code-verifier.md
🔇 Additional comments (4)
.changeset/fix-pkce-code-verifier.md (1)
1-6: LGTM!The changeset correctly identifies the affected packages and provides an accurate description of the fix.
packages/core-typings/src/IOAuthAuthCode.ts (1)
8-9: LGTM!The optional PKCE fields are correctly typed and appropriately added to support both PKCE and non-PKCE flows.
apps/meteor/server/oauth2-server/model.ts (2)
119-120: LGTM!The PKCE fields are correctly retrieved from the database and included in the returned
AuthorizationCodeobject, enabling the OAuth2 library to verify thecode_verifierduring token exchange.
126-127: LGTM. ThePicktype correctly includescodeChallengeandcodeChallengeMethodfor PKCE support, aligning with@node-oauth/oauth2-serverv5's AuthorizationCode interface.
… flow Signed-off-by: Subh Aush Singh <subhaushsingh@gmail.com>
subhaushsingh
force-pushed
the
fix-39459-pkce-login
branch
from
b84e119 to
3f38217
Compare
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.
Proposed changes (including videos or screenshots)
The PKCE flow in Rocket.Chat's third-party OAuth2 login was broken. When a client
initiated authorization with a
code_challenge, thesaveAuthorizationCodemethodwas silently dropping the
codeChallengeandcodeChallengeMethodfields due to arestrictive
Pick<>type — so they were never saved to MongoDB.When the client then sent the
code_verifierat the token endpoint,getAuthorizationCodehad nothing to return for those fields, leaving the@node-oauth/oauth2-serverlibrary with nothing to verify against, resulting ininvalid_grant: code verifier is invalid.Fix:
codeChallenge?andcodeChallengeMethod?toIOAuthAuthCodeinterfacePick<>type insaveAuthorizationCodeto include both PKCE fields$setblockgetAuthorizationCodeso the library can verify themThe implicit flow (no PKCE) is completely unaffected.
Issue(s)
Fixes #39459
Related to #35419
Steps to test or reproduce
invalid_grant: code verifier is invalidFurther comments
This is a follow-up to #35419 which was partially fixed in #37707.
That PR fixed the implicit flow but the PKCE flow remained broken.
A new e2e test covering the full PKCE flow has been added to
oauth-server.ts.Summary by CodeRabbit
Bug Fixes
Tests
Chores