fix(api)!: return 403 for authorization failures, reserve 401 for missing session - #41590
Draft
ggazzo wants to merge 2 commits into
Draft
fix(api)!: return 403 for authorization failures, reserve 401 for missing session#41590ggazzo wants to merge 2 commits into
ggazzo wants to merge 2 commits into
Conversation
Every `error-unauthorized` / `error-not-authorized` throw in the codebase follows an authorization check, never a missing session, yet the error switch in ApiClass mapped `error-unauthorized` to 401 once the 9.0.0 breaking-change flag turns on — while the permission middleware answered 403 for the same condition. Clients could not infer session state from the status code, which is what logged out live sessions in the sendMessage/getReadReceipts migration. - `error-unauthorized` now always maps to 403, dropping the 401 it would have been promoted to in 9.0.0 - `error-not-authorized`, previously unmapped and falling through to 400, maps to 403 from 9.0.0 - rooms.membersOrderedByRole, rooms.hide and rooms.bannedUsers returned 401 for access failures; they return 403 from 9.0.0 and declare it in their response schemas - drop the dead `if (applyBreakingChanges)` fork in the permissions middleware, both branches were identical The unauthenticated side still throws `error-invalid-user` and answers 400; splitting it out needs a dedicated error code and is tracked separately. Refs #41589, ARCH-2297
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: f5280a2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 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
WalkthroughREST authorization error mapping and permission handling are updated for 401/403 semantics. Three room endpoints declare 403 responses while retaining breaking-change compatibility, and end-to-end tests and release notes document the behavior. ChangesREST authorization semantics
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
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 (2)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/server/api/ApiClass.ts (1)
901-902: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the new inline implementation comments.
The same guideline violation appears at all affected sites; retain the migration rationale in the changeset or PR description instead.
apps/meteor/server/api/ApiClass.ts#L901-L902: remove the authorization-mapping comment.apps/meteor/server/api/v1/rooms.ts#L1156-L1156: remove the 401/403 migration comment.apps/meteor/server/api/v1/rooms.ts#L1304-L1304: remove the 401/403 migration comment.apps/meteor/server/api/v1/rooms.ts#L1709-L1709: remove the 401/403 migration comment.apps/meteor/tests/end-to-end/api/rooms.ts#L4666-L4666: remove the test comment.apps/meteor/tests/end-to-end/api/rooms.ts#L5116-L5116: remove the test comment.As per coding guidelines, avoid code comments in the implementation.
🤖 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/server/api/ApiClass.ts` around lines 901 - 902, Remove the new inline migration and authorization-mapping comments without changing surrounding behavior: delete the comments at apps/meteor/server/api/ApiClass.ts lines 901-902; apps/meteor/server/api/v1/rooms.ts lines 1156, 1304, and 1709; and apps/meteor/tests/end-to-end/api/rooms.ts lines 4666 and 5116. Retain the underlying implementation and tests unchanged.Source: Coding guidelines
🤖 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 `@apps/meteor/tests/end-to-end/api/rooms.ts`:
- Line 5117: Update the status assertion in the affected end-to-end room API
test to use the same 9.0.0 breaking-change version gate as the rooms API
implementation: expect 403 when the gate is enabled and 401 otherwise, so both
behaviors are validated.
---
Nitpick comments:
In `@apps/meteor/server/api/ApiClass.ts`:
- Around line 901-902: Remove the new inline migration and authorization-mapping
comments without changing surrounding behavior: delete the comments at
apps/meteor/server/api/ApiClass.ts lines 901-902;
apps/meteor/server/api/v1/rooms.ts lines 1156, 1304, and 1709; and
apps/meteor/tests/end-to-end/api/rooms.ts lines 4666 and 5116. Retain the
underlying implementation and tests unchanged.
🪄 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: 038e183a-9f9c-422d-b306-e00671207c95
📒 Files selected for processing (5)
.changeset/rest-401-403-semantics.mdapps/meteor/server/api/ApiClass.tsapps/meteor/server/api/v1/middlewares/permissions.tsapps/meteor/server/api/v1/rooms.tsapps/meteor/tests/end-to-end/api/rooms.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: CodeQL-Build
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: Hacktron Security Check
🧰 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:
apps/meteor/server/api/v1/middlewares/permissions.tsapps/meteor/tests/end-to-end/api/rooms.tsapps/meteor/server/api/ApiClass.tsapps/meteor/server/api/v1/rooms.ts
🧠 Learnings (4)
📚 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/rest-401-403-semantics.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:
apps/meteor/server/api/v1/middlewares/permissions.tsapps/meteor/tests/end-to-end/api/rooms.tsapps/meteor/server/api/ApiClass.tsapps/meteor/server/api/v1/rooms.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/server/api/v1/middlewares/permissions.tsapps/meteor/tests/end-to-end/api/rooms.tsapps/meteor/server/api/ApiClass.tsapps/meteor/server/api/v1/rooms.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/server/api/v1/middlewares/permissions.tsapps/meteor/tests/end-to-end/api/rooms.tsapps/meteor/server/api/ApiClass.tsapps/meteor/server/api/v1/rooms.ts
🔇 Additional comments (5)
apps/meteor/server/api/ApiClass.ts (1)
903-911: LGTM!apps/meteor/server/api/v1/middlewares/permissions.ts (1)
46-47: LGTM!apps/meteor/server/api/v1/rooms.ts (1)
80-80: LGTM!Also applies to: 1137-1137, 1157-1157, 1297-1299, 1305-1305, 1702-1704, 1710-1710
apps/meteor/tests/end-to-end/api/rooms.ts (1)
5105-5115: LGTM!Also applies to: 5118-5125
.changeset/rest-401-403-semantics.md (1)
1-15: LGTM!
| .query({ roomId: privateRoom._id }) | ||
| .expect('Content-Type', 'application/json') | ||
| // TODO: MAJOR 403 — currently 401 until the 9.0.0 breaking changes are applied | ||
| .expect(401) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the expected status version-aware.
apps/meteor/server/api/v1/rooms.ts returns 403 when the 9.0.0 breaking-change gate is enabled and 401 otherwise. Hardcoding 401 here will fail on 9.0.0+ and does not validate the new behavior. Assert the status using the same version gate.
🤖 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/rooms.ts` at line 5117, Update the status
assertion in the affected end-to-end room API test to use the same 9.0.0
breaking-change version gate as the rooms API implementation: expect 403 when
the gate is enabled and 401 otherwise, so both behaviors are validated.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #41590 +/- ##
===========================================
- Coverage 68.69% 68.67% -0.02%
===========================================
Files 4139 4140 +1
Lines 159268 159306 +38
Branches 27964 27983 +19
===========================================
- Hits 109403 109402 -1
- Misses 44705 44739 +34
- Partials 5160 5165 +5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 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="apps/meteor/server/api/ApiClass.ts">
<violation number="1" location="apps/meteor/server/api/ApiClass.ts:905">
P2: `rooms.changeArchivationState` can now emit 403 for its archive/unarchive permission check, but its declared responses omit 403. Add the forbidden response schema for this and other routes that propagate `error-not-authorized`, so test validation and generated API contracts match runtime behavior.</violation>
<violation number="2" location="apps/meteor/server/api/ApiClass.ts:907">
P1: Authorization failures thrown as native `Error` still return 400 in 9.0.0 because `switch (e.error)` cannot match their message. Normalize the error code from `e.error` or `e.message` before this switch so these existing REST authorization paths reach 403.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| case 'error-unauthorized': | ||
| return api.forbidden(typeof e === 'string' ? e : e.message); | ||
| case 'not-authorized': | ||
| case 'error-not-authorized': |
Contributor
There was a problem hiding this comment.
P1: Authorization failures thrown as native Error still return 400 in 9.0.0 because switch (e.error) cannot match their message. Normalize the error code from e.error or e.message before this switch so these existing REST authorization paths reach 403.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/api/ApiClass.ts, line 907:
<comment>Authorization failures thrown as native `Error` still return 400 in 9.0.0 because `switch (e.error)` cannot match their message. Normalize the error code from `e.error` or `e.message` before this switch so these existing REST authorization paths reach 403.</comment>
<file context>
@@ -898,12 +898,17 @@ export class APIClass<TBasePath extends string = '', TOperations extends Record<
case 'error-unauthorized':
+ return api.forbidden(typeof e === 'string' ? e : e.message);
+ case 'not-authorized':
+ case 'error-not-authorized':
if (applyBreakingChanges) {
- return api.unauthorized(typeof e === 'string' ? e : e.message);
</file context>
| // so they map to 403; 401 is reserved for a missing or invalid session | ||
| case 'unauthorized': | ||
| case 'error-unauthorized': | ||
| return api.forbidden(typeof e === 'string' ? e : e.message); |
Contributor
There was a problem hiding this comment.
P2: rooms.changeArchivationState can now emit 403 for its archive/unarchive permission check, but its declared responses omit 403. Add the forbidden response schema for this and other routes that propagate error-not-authorized, so test validation and generated API contracts match runtime behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/api/ApiClass.ts, line 905:
<comment>`rooms.changeArchivationState` can now emit 403 for its archive/unarchive permission check, but its declared responses omit 403. Add the forbidden response schema for this and other routes that propagate `error-not-authorized`, so test validation and generated API contracts match runtime behavior.</comment>
<file context>
@@ -898,12 +898,17 @@ export class APIClass<TBasePath extends string = '', TOperations extends Record<
+ // so they map to 403; 401 is reserved for a missing or invalid session
case 'unauthorized':
case 'error-unauthorized':
+ return api.forbidden(typeof e === 'string' ? e : e.message);
+ case 'not-authorized':
+ case 'error-not-authorized':
</file context>
ggazzo
marked this pull request as draft
July 27, 2026 17:58
…sion Drops the `applyBreakingChanges` gating and applies the change directly instead of deferring it to the 9.0.0 flag. - `error-not-authorized` / `not-authorized` now map to 403 instead of falling through to `API.v1.failure()` (400) - rooms.membersOrderedByRole, rooms.hide and rooms.bannedUsers return 403 unconditionally - the permissions middleware answers 401 for a missing session instead of 403 with `[error-unauthorized]` in the message Changeset bumped to major. BREAKING CHANGE: REST endpoints that denied a request on a permission or room-access check answered 400 or 401 and now answer 403; routes with `permissionsRequired` and no `authRequired` answer 401 instead of 403 when called without a session.
![dionisio-bot[bot]](https://avatars.githubusercontent.com/in/257776?s=60&v=4){kind=small}
ggazzo
changed the title
dionisio-bot
Bot
dismissed
their stale review
July 27, 2026 17:59
Changesets now match the title and milestone
dionisio-bot
Bot
dismissed
their stale review
July 27, 2026 18:06
Changesets now match the title and milestone
This was referenced Aug 2, 2026
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.
Proposed changes (including videos or screenshots)
In the REST API the status codes for authentication and authorization failures are inverted relative to their meaning:
error-invalid-user("you are not logged in") falls through toAPI.v1.failure()→ 400, whileerror-unauthorized("you are logged in but not allowed") was mapped to 401 under the9.0.0flag. So the only code that reached 401 never meant unauthenticated, and the condition that genuinely is 401 answered 400.I classified all 31
error-unauthorized/error-not-authorizedthrows inapps/meteor/server,apps/meteor/appandapps/meteor/eeby the guard preceding them — every single one is an authorization check (hasPermissionAsync,canAccessRoom,canDeleteFile, agent department scope). None means unauthenticated. Full per-site census in the issue.This PR makes 401 mean exactly one thing — no valid session — so a client can act on it again. The change is applied directly, not deferred behind
applyBreakingChanges, hence themajorchangeset.server/api/ApiClass.ts—error-unauthorized/unauthorizedanderror-not-authorized/not-authorizedall map toforbidden()(403).error-unauthorizedwas already 403 and simply no longer flips to 401;error-not-authorizedwas not handled by the switch at all and fell through to 400.server/api/v1/rooms.ts—rooms.membersOrderedByRole(view-broadcast-member-list),rooms.hideandrooms.bannedUsersreturned 401 for access/permission failures and now return 403. Response schemas declare403.server/api/v1/middlewares/permissions.ts— a missing session now answers 401 instead of 403-with-[error-unauthorized]-in-the-message. Also removed the deadif (applyBreakingChanges)fork on the!hasPermissionpath, whose two branches were byte-identical (the legacy branch named its variablefailure, soAPI.v1.failurewas probably intended and the if/else was left half-finished).Worth calling out: before this, the same permission denial answered 403 from
permissionsMiddlewareand 401 from a handler once the flag turned on. That inconsistency is gone.Why this matters beyond tidiness
The ~134 unauthenticated checks live almost entirely in
apps/meteor/server/meteor-methods/. Over DDP the error code reaches the client verbatim and no status code is involved. As each method migrates to a typed route, that same check starts answering 400 — indistinguishable from a validation error.It already cost us once: in #40675 (
sendMessage+getReadReceiptsmigration) a client handler that treated any REST 401 as an expired session logged out live sessions and failed the omnichannel e2e shards. It was reverted, and it cannot be reimplemented correctly until 401 has a single meaning. This PR is the prerequisite.Issue(s)
Refs #41589
Refs ARCH-2297
Steps to test or reproduce
rooms.hideas a user without access to the room → was 401, now 403.rooms.bannedUserson a private room the caller is not in → was 401, now 403.rooms.membersOrderedByRoleon a broadcast room withoutview-broadcast-member-list→ was 401, now 403. (Untested branch:findRoomByIdOrNameanswersnotFoundfirst for non-members, so it is only reachable for a member of a broadcast room.)error-not-authorized(e.g.rooms.changeArchivationStatewithoutarchive-room) → was 400, now 403.permissionsRequiredand noauthRequired, called with no session → was 403, now 401.Tests:
apps/meteor/tests/end-to-end/api/rooms.ts. Updated therooms.hideno-access assertion to 403, and added the missing no-access test forrooms.bannedUsers, which had none.Further comments
Status: draft while CI tells us the real blast radius. Two things I expect the suite to surface, and I'd rather have the numbers than guess:
error-not-authorizedthrow now get 403.403:in their response schema — response validation runs against the declared schema inTEST_MODE, so those may fail rather than pass through.One subtlety found while auditing: several
error-not-authorizedsites throw a plainError, notMeteor.Error/MeteorError—server/api/lib/rooms.ts:27,68,113,server/api/v1/omnichannel/lib/inquiries.ts:28,server/lib/omnichannel/closeLivechatRoom.ts:39,ee/server/lib/omnichannel/business-hour/lib/business-hour.ts:14,ee/server/api/ldap.ts:36. The switch keys offe.error, which isundefinedon a plainError, so those still fall through to 400 and are not affected by this PR. They carry the code inmessageinstead and want a separate cleanup.Deliberately not in this PR — step 2 of the issue: introducing a dedicated
error-unauthenticatedcode mapped to 401 and moving the ~134 missing-session guards onto it. A blanket remap oferror-invalid-useris not viable, since the same code also means "that user does not exist" (244 occurrences in total, only 134 of them guarded by a missing-session check). Separate, much larger change, and it depends on this one landing first.I also left
rooms.ts:1147alone — in the same handler as the broadcast check, it answersnotFoundfor an access failure, a third treatment of the same class of condition in one file. Changing it to 403 would leak room existence, so it deserves its own discussion rather than a drive-by fix.