fix(api): return 403 for authorization failures, reserve 401 for missing session - #41659
fix(api): return 403 for authorization failures, reserve 401 for missing session#41659surjeetkumar8006 wants to merge 2 commits into
Conversation
|
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: 14591cd The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
|
|
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
**/*.spec.ts📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (6)📚 Learning: 2025-12-10T21:00:43.645ZApplied to files:
📚 Learning: 2026-02-24T19:22:48.358ZApplied to files:
📚 Learning: 2026-02-26T19:25:44.063ZApplied to files:
📚 Learning: 2026-02-26T19:25:44.063ZApplied to files:
📚 Learning: 2026-03-06T18:10:15.268ZApplied to files:
📚 Learning: 2026-05-06T12:21:44.083ZApplied to files:
🔇 Additional comments (2)
WalkthroughAPI authorization failures now return HTTP 403, while HTTP 401 remains for missing sessions. Affected room endpoint schemas declare 403 responses. Invalid request-body validation now uses ChangesAuthorization status normalization
Request validation error type
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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.
Actionable comments posted: 3
🤖 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.
Inline comments:
In @.changeset/real-mails-count.md:
- Around line 1-6: Delete the changeset file `.changeset/real-mails-count.md` so
this PR does not schedule patch releases for either package.
In `@apps/meteor/server/api/v1/rooms.ts`:
- Around line 1697-1701: Add the 401 response mapping to the `rooms.bannedUsers`
response schema alongside the existing 200 and 403 entries, using the
established `validateUnauthorizedErrorResponse` symbol. Keep the endpoint’s
existing authentication and other response declarations unchanged.
In `@packages/http-router/src/Router.ts`:
- Line 248: Update the body-validation assertions in Router.spec.ts to expect
error-invalid-params instead of invalid-params, matching the errorType returned
by the handler in Router.ts. Change the assertions at the affected test cases
while preserving the existing validation behavior and coverage.
🪄 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 Plus
Run ID: db45b3cd-7df0-4cb5-999b-84bc13cabe83
📒 Files selected for processing (5)
.changeset/real-mails-count.mdapps/meteor/server/api/ApiClass.tsapps/meteor/server/api/v1/middlewares/permissions.tsapps/meteor/server/api/v1/rooms.tspackages/http-router/src/Router.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (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/http-router/src/Router.tsapps/meteor/server/api/v1/rooms.tsapps/meteor/server/api/ApiClass.tsapps/meteor/server/api/v1/middlewares/permissions.ts
🧠 Learnings (6)
📚 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/real-mails-count.md
📚 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/http-router/src/Router.tsapps/meteor/server/api/v1/rooms.tsapps/meteor/server/api/ApiClass.tsapps/meteor/server/api/v1/middlewares/permissions.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/http-router/src/Router.tsapps/meteor/server/api/v1/rooms.tsapps/meteor/server/api/ApiClass.tsapps/meteor/server/api/v1/middlewares/permissions.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:
packages/http-router/src/Router.tsapps/meteor/server/api/v1/rooms.tsapps/meteor/server/api/ApiClass.tsapps/meteor/server/api/v1/middlewares/permissions.ts
📚 Learning: 2026-07-29T23:45:21.859Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 41632
File: apps/meteor/server/api/v1/groups.ts:948-959
Timestamp: 2026-07-29T23:45:21.859Z
Learning: For API v1 routes under apps/meteor/server/api/v1, keep item-level response schemas strict by using `$ref`-based schemas for list and messages (and ensure they intentionally mirror the corresponding route contracts, as done in channels.ts). Only use “loose”/non-`$ref` item schemas when the underlying data source is inherently partial (e.g., uploads where `content` can be `null`, or queries like `findUsersOfRoom` with a fixed projection). Do not relax item schemas merely because the route supports an optional client `fields` projection—optional field selection alone is not a reason to change schema strictness.
Applied to files:
apps/meteor/server/api/v1/rooms.ts
📚 Learning: 2026-07-31T02:44:35.111Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 41635
File: apps/meteor/ee/server/api/sessions.ts:114-138
Timestamp: 2026-07-31T02:44:35.111Z
Learning: In Rocket.Chat typed REST response schemas, accept the composition of a Typia-generated entity schema with an `allOf` branch requiring `success: true`: `allOf: [{ $ref: <entity schema> }, { properties: { success: { type: 'boolean', enum: [true] } }, required: ['success'] }]`. Do not flag this pattern when used for REST endpoints, provided TEST_MODE response validation passes, as demonstrated by the `IOAuthApps` and `IEmailInbox` endpoints.
Applied to files:
apps/meteor/server/api/v1/rooms.tsapps/meteor/server/api/ApiClass.tsapps/meteor/server/api/v1/middlewares/permissions.ts
🔇 Additional comments (2)
apps/meteor/server/api/v1/rooms.ts (1)
1157-1157: LGTM!Also applies to: 1296-1303
apps/meteor/server/api/ApiClass.ts (1)
903-904: 🗄️ Data Integrity & IntegrationNo change needed for authorization response schemas.
authRequiredroutes reject missing auth withunauthorized/error-unauthorized, which the middleware and ApiClass route as 401. The missing 401 inrooms.bannedUsersis not required for the current auth path.> Likely an incorrect or invalid review comment.
| --- | ||
| "@rocket.chat/meteor": patch | ||
| "@rocket.chat/http-router": patch | ||
| --- | ||
|
|
||
| fix(api)!: return 403 for authorization failures, reserve 401 for missing session |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove this changeset from the PR.
The PR objective states that this change includes no changeset. This file schedules patch releases for two packages. Delete it before merge.
🤖 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 @.changeset/real-mails-count.md around lines 1 - 6, Delete the changeset file
`.changeset/real-mails-count.md` so this PR does not schedule patch releases for
either package.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21ac7e9e58
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| if (findResult.broadcast && !(await hasPermissionAsync(this.user, 'view-broadcast-member-list', findResult._id))) { | ||
| return API.v1.unauthorized(); | ||
| return API.v1.forbidden(); |
There was a problem hiding this comment.
When a requester can access a broadcast room but lacks view-broadcast-member-list, this new API.v1.forbidden() returns status 403 while the endpoint's response map above still only declares 400/401/404. Because API.v1.get registers typed routes, the router's test-mode response validation will hit the options.typed missing-validator path for this scenario and fail the request instead of returning the intended 403; add 403: validateForbiddenErrorResponse to this endpoint's response spec.
Useful? React with 👍 / 👎.
| { | ||
| success: false, | ||
| errorType: 'invalid-params', | ||
| errorType: 'error-invalid-params', |
There was a problem hiding this comment.
For invalid request bodies on routes using this router, this now emits error-invalid-params, but the package's own body-validation contract still expects invalid-params (packages/http-router/src/Router.spec.ts checks this for missing and wrong-typed body fields). This will break clients/tests that distinguish body validation failures from query parsing failures; leave the body path as invalid-params unless all consumers and tests are migrated together.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 5 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="packages/http-router/src/Router.ts">
<violation number="1" location="packages/http-router/src/Router.ts:248">
P2: This change alters the `errorType` returned for body-validation failures on typed-router POST/PUT endpoints from `invalid-params` to `error-invalid-params`. That behavior now contradicts this repository's own migration documentation (docs/api-endpoint-migration.md), which explicitly states "This only affects query parameter validation (GET/DELETE). Body parameter validation (POST/PUT) keeps 'invalid-params'." It also breaks any consumer/tests that still assert `'invalid-params'` for body validation errors. Please update the migration doc to reflect that body validation now also returns `error-invalid-params`, and confirm related e2e assertions are updated.</violation>
<violation number="2" location="packages/http-router/src/Router.ts:248">
P1: This change makes body validation return `errorType: 'error-invalid-params'`, but the existing unit tests in `Router.spec.ts` (the 'should validate request body' case, assertions on `errorType`) still expect the old `'invalid-params'` value. Since those tests exercise the exact code path being modified, they will now fail. Please update the corresponding assertions to `'error-invalid-params'` (or otherwise keep the spec consistent with the new behavior) as part of this PR so the test suite stays green.</violation>
</file>
<file name=".changeset/real-mails-count.md">
<violation number="1" location=".changeset/real-mails-count.md:6">
P3: The PR description states this change has no changeset, yet this file schedules patch releases for @rocket.chat/meteor and @rocket.chat/http-router. Remove this changeset before merge to match the stated intent, or update the PR description if a release is actually intended.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| { | ||
| success: false, | ||
| errorType: 'invalid-params', | ||
| errorType: 'error-invalid-params', |
There was a problem hiding this comment.
P1: This change makes body validation return errorType: 'error-invalid-params', but the existing unit tests in Router.spec.ts (the 'should validate request body' case, assertions on errorType) still expect the old 'invalid-params' value. Since those tests exercise the exact code path being modified, they will now fail. Please update the corresponding assertions to 'error-invalid-params' (or otherwise keep the spec consistent with the new behavior) as part of this PR so the test suite stays green.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/http-router/src/Router.ts, line 248:
<comment>This change makes body validation return `errorType: 'error-invalid-params'`, but the existing unit tests in `Router.spec.ts` (the 'should validate request body' case, assertions on `errorType`) still expect the old `'invalid-params'` value. Since those tests exercise the exact code path being modified, they will now fail. Please update the corresponding assertions to `'error-invalid-params'` (or otherwise keep the spec consistent with the new behavior) as part of this PR so the test suite stays green.</comment>
<file context>
@@ -245,7 +245,7 @@ export class Router<
{
success: false,
- errorType: 'invalid-params',
+ errorType: 'error-invalid-params',
error: validatorFn.errors?.map((error: any) => error.message).join('\n '),
},
</file context>
| { | ||
| success: false, | ||
| errorType: 'invalid-params', | ||
| errorType: 'error-invalid-params', |
There was a problem hiding this comment.
P2: This change alters the errorType returned for body-validation failures on typed-router POST/PUT endpoints from invalid-params to error-invalid-params. That behavior now contradicts this repository's own migration documentation (docs/api-endpoint-migration.md), which explicitly states "This only affects query parameter validation (GET/DELETE). Body parameter validation (POST/PUT) keeps 'invalid-params'." It also breaks any consumer/tests that still assert 'invalid-params' for body validation errors. Please update the migration doc to reflect that body validation now also returns error-invalid-params, and confirm related e2e assertions are updated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/http-router/src/Router.ts, line 248:
<comment>This change alters the `errorType` returned for body-validation failures on typed-router POST/PUT endpoints from `invalid-params` to `error-invalid-params`. That behavior now contradicts this repository's own migration documentation (docs/api-endpoint-migration.md), which explicitly states "This only affects query parameter validation (GET/DELETE). Body parameter validation (POST/PUT) keeps 'invalid-params'." It also breaks any consumer/tests that still assert `'invalid-params'` for body validation errors. Please update the migration doc to reflect that body validation now also returns `error-invalid-params`, and confirm related e2e assertions are updated.</comment>
<file context>
@@ -245,7 +245,7 @@ export class Router<
{
success: false,
- errorType: 'invalid-params',
+ errorType: 'error-invalid-params',
error: validatorFn.errors?.map((error: any) => error.message).join('\n '),
},
</file context>
| @@ -0,0 +1,6 @@ | |||
| --- | |||
There was a problem hiding this comment.
P3: The PR description states this change has no changeset, yet this file schedules patch releases for @rocket.chat/meteor and @rocket.chat/http-router. Remove this changeset before merge to match the stated intent, or update the PR description if a release is actually intended.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .changeset/real-mails-count.md, line 6:
<comment>The PR description states this change has no changeset, yet this file schedules patch releases for @rocket.chat/meteor and @rocket.chat/http-router. Remove this changeset before merge to match the stated intent, or update the PR description if a release is actually intended.</comment>
<file context>
@@ -0,0 +1,6 @@
+"@rocket.chat/http-router": patch
+---
+
+fix(api)!: return 403 for authorization failures, reserve 401 for missing session
</file context>
…ectations, and error messages
3 participants
Proposal
Fixes REST API HTTP status code semantics for authentication vs authorization failures and aligns validation errorType naming consistency across the typed router, resolving #41589.
Endpoints
ApiClass, Router, permissionsMiddleware, rooms.getMembers, rooms.hide, rooms.bannedUsers
Notes
Reserve 401 status strictly for unauthenticated requests (!user / missing session).
Remap permission denials (error-unauthorized / error-not-authorized) to 403 (forbidden).
Align Router.ts body validation error type from invalid-params to error-invalid-params matching ajvQuery failure responses.
Clean up dead applyBreakingChanges branch in permissions.ts middleware and update rooms.ts response schemas to validateForbiddenErrorResponse.
No changeset (API semantics alignment).
Task: #41589
Summary by CodeRabbit
Refactor: Standardized REST API error handling to return 403 Forbidden for authorization failures and reserved 401 Unauthorized strictly for unauthenticated missing-session requests.
Bug Fixes: Aligned body validation errorType in Hono typed router to 'error-invalid-params' for consistency with query validation errors across all REST endpoints.
Summary by CodeRabbit
Bug Fixes
error-invalid-paramstype.Documentation