fix: normalize OAuth app redirect URI list parsing#41562
Conversation
`parseUriList` short-circuited and returned the raw input whenever it contained no `,` or `\n`, so a single redirect URI kept any surrounding whitespace. That value is persisted verbatim and later compared with strict equality against the `redirect_uri` sent by the client, so an OAuth app configured with a stray leading/trailing space always failed authorization with `invalid_redirect_uri`. Give the helper a single consistent contract: always return the trimmed, non-empty entries as `string[]`. Callers keep persisting the existing comma-joined string, and now validate on entry count rather than on the character length of the joined value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 99e2dbc The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
|
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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (3)📚 Learning: 2026-02-26T19:25:44.063ZApplied to files:
📚 Learning: 2026-02-26T19:25:44.063ZApplied to files:
📚 Learning: 2026-05-06T12:21:44.083ZApplied to files:
🔇 Additional comments (3)
WalkthroughOAuth redirect URI parsing now consistently returns trimmed URI arrays. OAuth app creation and updates validate parsed values and persist normalized comma-separated strings, with unit and end-to-end tests covering whitespace, separators, and empty entries. ChangesOAuth redirect URI handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/tests/end-to-end/api/oauthapps.ts (1)
149-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover normalization on OAuth app updates too.
These tests cover creation only, while
apps/meteor/server/meteor-methods/auth/updateOAuthApp.tsLines 45-56 also changed the normalization and persistence contract. Add an update test for trimmed and/or multiple redirect URIs to prevent the two paths from diverging.Also applies to: 167-183
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/tests/end-to-end/api/oauthapps.ts` around lines 149 - 165, Extend the OAuth app end-to-end tests near the existing creation normalization cases with an update scenario that changes an app’s redirect URI using surrounding whitespace and/or multiple URIs. Call the OAuth app update flow, assert success, and verify the persisted application redirectUri is normalized consistently with creation, covering the contract implemented by updateOAuthApp.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/meteor/tests/end-to-end/api/oauthapps.ts`:
- Around line 149-165: Extend the OAuth app end-to-end tests near the existing
creation normalization cases with an update scenario that changes an app’s
redirect URI using surrounding whitespace and/or multiple URIs. Call the OAuth
app update flow, assert success, and verify the persisted application
redirectUri is normalized consistently with creation, covering the contract
implemented by updateOAuthApp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 43d84acc-5912-406d-8c9e-84d1adb8772d
📒 Files selected for processing (6)
.changeset/quiet-pandas-listen.mdapps/meteor/server/lib/auth/oauth2-server/addOAuthApp.tsapps/meteor/server/lib/auth/oauth2-server/parseUriList.tsapps/meteor/server/meteor-methods/auth/updateOAuthApp.tsapps/meteor/tests/end-to-end/api/oauthapps.tsapps/meteor/tests/unit/server/lib/auth/oauth2-server/parseUriList.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
apps/meteor/tests/unit/server/lib/auth/oauth2-server/parseUriList.spec.tsapps/meteor/tests/end-to-end/api/oauthapps.tsapps/meteor/server/meteor-methods/auth/updateOAuthApp.tsapps/meteor/server/lib/auth/oauth2-server/parseUriList.tsapps/meteor/server/lib/auth/oauth2-server/addOAuthApp.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/tests/unit/server/lib/auth/oauth2-server/parseUriList.spec.ts
🧠 Learnings (6)
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
apps/meteor/tests/unit/server/lib/auth/oauth2-server/parseUriList.spec.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 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:
apps/meteor/tests/unit/server/lib/auth/oauth2-server/parseUriList.spec.tsapps/meteor/tests/end-to-end/api/oauthapps.tsapps/meteor/server/meteor-methods/auth/updateOAuthApp.tsapps/meteor/server/lib/auth/oauth2-server/parseUriList.tsapps/meteor/server/lib/auth/oauth2-server/addOAuthApp.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:
apps/meteor/tests/unit/server/lib/auth/oauth2-server/parseUriList.spec.tsapps/meteor/tests/end-to-end/api/oauthapps.tsapps/meteor/server/meteor-methods/auth/updateOAuthApp.tsapps/meteor/server/lib/auth/oauth2-server/parseUriList.tsapps/meteor/server/lib/auth/oauth2-server/addOAuthApp.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/tests/unit/server/lib/auth/oauth2-server/parseUriList.spec.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/tests/unit/server/lib/auth/oauth2-server/parseUriList.spec.tsapps/meteor/tests/end-to-end/api/oauthapps.tsapps/meteor/server/meteor-methods/auth/updateOAuthApp.tsapps/meteor/server/lib/auth/oauth2-server/parseUriList.tsapps/meteor/server/lib/auth/oauth2-server/addOAuthApp.ts
📚 Learning: 2026-03-16T21:50:37.589Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: For changes related to OpenAPI migrations in Rocket.Chat/OpenAPI, when removing endpoint types and validators from rocket.chat/rest-typings (e.g., UserRegisterParamsPOST, /v1/users.register) document this as a minor changeset (not breaking) per RocketChat/Rocket.Chat-Open-API#150 Rule 7. Note that the endpoint type is re-exposed via a module augmentation .d.ts in the consuming package (e.g., packages/web-ui-registration/src/users-register.d.ts). In reviews, ensure the changeset clearly states: this is a non-breaking change, the major version should not be bumped, and the changeset reflects a minor version bump. Do not treat this as a breaking change during OpenAPI migrations.
Applied to files:
.changeset/quiet-pandas-listen.md
🔇 Additional comments (5)
apps/meteor/server/lib/auth/oauth2-server/parseUriList.ts (1)
1-5: LGTM!apps/meteor/tests/unit/server/lib/auth/oauth2-server/parseUriList.spec.ts (1)
1-38: LGTM!apps/meteor/server/lib/auth/oauth2-server/addOAuthApp.ts (1)
46-56: LGTM!apps/meteor/server/meteor-methods/auth/updateOAuthApp.ts (1)
45-56: LGTM!.changeset/quiet-pandas-listen.md (1)
1-5: LGTM!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
The update path shares `parseUriList` and the comma-joining persistence step with the create path, but only create was covered. Addresses review feedback on RocketChat#41562. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks for the reviews. Addressed — @coderabbitai's nitpick about the update path was a fair catch: Not addressed — the Note the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- PR changed files and current head ---'
git status --short
git log -1 --oneline
git diff --name-only HEAD^ HEAD
echo '--- oauthapps test structure and added update test context ---'
ast-grep outline apps/meteor/tests/end-to-end/api/oauthapps.ts --items all --type function --view expanded || true
rg -n -A35 -B12 'should trim and normalize the redirectUri when updating an app|/oauth-apps\.update' apps/meteor/tests/end-to-end/api/oauthapps.ts
echo '--- update implementation context ---'
ast-grep outline apps/meteor/server/meteor-methods/auth/updateOAuthApp.ts --items all --type function --view expanded || true
rg -n -A35 -B10 'parseUriList|redirectUris|redirectUri' apps/meteor/server/meteor-methods/auth/updateOAuthApp.ts
echo '--- JSDoc presence in named helper files ---'
for f in \
apps/meteor/server/lib/auth/oauth2-server/addOAuthApp.ts \
apps/meteor/server/lib/auth/oauth2-server/oauth2-server.ts \
apps/meteor/server/lib/auth/oauth2-server/index.ts \
apps/meteor/server/lib/auth/oauth2-server/parseUriList.ts; do
echo "FILE: $f"
rg -n '^\s*/\*\*' "$f" || true
done
echo '--- coverage/config references (bounded search) ---'
rg -n -i -C2 'docstring.?coverage|jsdoc|typedoc' \
.coderabbit.yaml .coderabbit.yml package.json turbo.json 2>/dev/null || trueLength of output: 8289
The new I also confirmed that none of the four neighboring OAuth server files contains JSDoc blocks, and found no relevant documentation-coverage configuration in the checked root config files. I agree that adding documentation solely for this small helper would be inconsistent, so I’m withdrawing that warning. CI execution remains pending maintainer approval. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
1 participant
Proposed changes (including videos or screenshots)
parseUriListhad an inconsistent contract. It short-circuited and returned the raw, untrimmed input whenever the value contained no,or\n, and otherwise returned a trimmed, comma-joined string:That short-circuit is a real user-facing bug, not just a typing wart. The returned value is persisted as
IOAuthApps.redirectUriand later compared with strict equality against theredirect_urithe client sends:So an OAuth app whose Redirect URI was saved with a stray leading/trailing space (trivially easy to introduce by copy/paste into the admin textarea) stored
"http://localhost:3000 ", never matched"http://localhost:3000", and failed every authorization withinvalid_redirect_uri. Configuring two or more URIs happened to work, because only the multi-entry path trimmed.Changes
parseUriListnow has one consistent contract: it always returns the trimmed, non-empty entries asstring[], for empty, single, and multiple inputs alike. TheTODOhack is gone.addOAuthApp/updateOAuthAppjoin the parsed entries when persisting, and now validate on entry count (redirectUris.length === 0) instead of on the character length of the joined string.addOAuthAppvalidates before building the document rather than after, so no partially-built application object is constructed on the invalid path.Deliberate deviation from the issue
The issue also suggests migrating the stored value to an array. I kept the persisted shape unchanged (comma-joined
string), becauseIOAuthApps.redirectUriis typedstringand drives the generated#/components/schemas/IOAuthAppsresponse schema used byoauth-apps.list/.get/.create/.update, and both OAuth2 consumers (server/oauth2-server/oauth.ts,server/oauth2-server/model.ts) doclient.redirectUri.split(','). Changing the stored shape would be an API/schema break plus a data migration — out of scope for this bug fix, and a separate call for maintainers. The helper's own contract is nowstring[]as the issue asks, with the string-joining confined to the persistence boundary.Issue(s)
Closes #39762
Steps to test or reproduce
Before this change:
http://localhost:3000(note the single trailing space) and save.GET /oauth/authorize?response_type=code&client_id=<clientId>&redirect_uri=http://localhost:3000./oauth/error/invalid_redirect_uri.After this change the URI is stored as
http://localhost:3000and the authorization proceeds. Entering two URIs was unaffected before and after.Automated coverage added:
apps/meteor/tests/unit/server/lib/auth/oauth2-server/parseUriList.spec.ts— 8 cases covering empty, blank, single, single-with-whitespace, trailing separator, comma-separated, newline-separated, and mixed separators with blank entries. All 8 fail ondevelopand pass with this change.apps/meteor/tests/end-to-end/api/oauthapps.ts— twooauth-apps.createcases asserting the storedredirectUriis trimmed for a single URI and trimmed/normalized for a list.Further comments
Verified locally (Node 22.22.3):
mocha --config ./.mocharc.js— 2129 passing, 0 failing, 12 pending (withTZ=UTC, matching CI; without it, 3 pre-existing timezone-dependentparseMessageSearchQueryspecs fail ondeveloptoo, unrelated to this change).mocha --config ./.mocharc.definition.js— 149 passing.tsc --noEmit --skipLibCheck— clean.eslinton all changed files — clean.prettier --checkon all changed files — clean.While tracing this I noticed a separate latent issue that I did not touch:
EditOauthApp.tsxguards withArray.isArray(data.redirectUri), which suggests some workspaces still hold array-valuedredirectUrifrom the pre-join(',')era. Those documents would throw onclient.redirectUri.split(',')inoauth.ts/model.ts. Happy to open a separate issue/PR if maintainers confirm that shape exists in the wild.Summary by CodeRabbit
Bug Fixes
Tests