fix(api): return 403 for authorization failures, reserve 401 for missing session - #41660
fix(api): return 403 for authorization failures, reserve 401 for missing session#41660surjeetkumar8006 wants to merge 1 commit 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: c7cefd3 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 (7)
🚧 Files skipped from review as they are similar to previous changes (7)
WalkthroughThe API now returns 403 for authenticated authorization failures and reserves 401 for missing sessions. HTTP router body validation uses ChangesAPI error response standardization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 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 |
![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: c7223ecd12
ℹ️ 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 (applyBreakingChanges) { | ||
| return api.unauthorized(typeof e === 'string' ? e : e.message); | ||
| } | ||
| case 'error-not-authorized': |
There was a problem hiding this comment.
This new mapping also catches plain Error('error-not-authorized'), but several typed routes that can throw that value still only declare 400/401 responses. For example, rooms.adminRooms calls findAdminRooms, which throws error-not-authorized when the caller lacks view-room-administration, while its response block has no 403; because API.v1.get() registers typed routes with typed: true, packages/http-router/src/Router.ts throws a missing-response-validator error under TEST_MODE instead of returning the intended 403. Please add 403: validateForbiddenErrorResponse to all affected routes or keep this remap limited to endpoints whose specs were updated.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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: Update the changeset metadata for `@rocket.chat/meteor` and
`@rocket.chat/http-router` to use the breaking-release level that matches the
documented 401-to-403 API behavior, or revise the summary to remove the breaking
marker only if the change is confirmed backward-compatible.
🪄 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: 9f0de4d8-b038-4ec8-b351-2e0f83d15e7a
📒 Files selected for processing (7)
.changeset/real-mails-count.mdapps/meteor/server/api/ApiClass.tsapps/meteor/server/api/v1/middlewares/permissions.tsapps/meteor/server/api/v1/rooms.tsdocs/api-endpoint-migration.mdpackages/http-router/src/Router.spec.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 (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:
packages/http-router/src/Router.spec.tspackages/http-router/src/Router.tsapps/meteor/server/api/v1/middlewares/permissions.tsapps/meteor/server/api/v1/rooms.tsapps/meteor/server/api/ApiClass.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:
packages/http-router/src/Router.spec.ts
🧠 Learnings (9)
📚 Learning: 2025-12-10T21:00:43.645Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:43.645Z
Learning: Adopt the monorepo-wide Jest testMatch pattern: <rootDir>/src/**/*.spec.{ts,js,mjs} (represented here as '**/src/**/*.spec.{ts,js,mjs}') to ensure spec files under any package's src directory are picked up consistently across all packages in the Rocket.Chat monorepo. Apply this pattern in jest.config.ts for all relevant packages to maintain uniform test discovery.
Applied to files:
packages/http-router/src/Router.spec.ts
📚 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:
packages/http-router/src/Router.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:
packages/http-router/src/Router.spec.tspackages/http-router/src/Router.tsapps/meteor/server/api/v1/middlewares/permissions.tsapps/meteor/server/api/v1/rooms.tsapps/meteor/server/api/ApiClass.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.spec.tspackages/http-router/src/Router.tsapps/meteor/server/api/v1/middlewares/permissions.tsapps/meteor/server/api/v1/rooms.tsapps/meteor/server/api/ApiClass.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:
packages/http-router/src/Router.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:
packages/http-router/src/Router.spec.tspackages/http-router/src/Router.tsapps/meteor/server/api/v1/middlewares/permissions.tsapps/meteor/server/api/v1/rooms.tsapps/meteor/server/api/ApiClass.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/real-mails-count.md
📚 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/middlewares/permissions.tsapps/meteor/server/api/v1/rooms.tsapps/meteor/server/api/ApiClass.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
🔇 Additional comments (7)
packages/http-router/src/Router.ts (1)
248-248: LGTM!packages/http-router/src/Router.spec.ts (1)
210-215: LGTM!docs/api-endpoint-migration.md (2)
600-600: LGTM!
618-618: LGTM!apps/meteor/server/api/v1/middlewares/permissions.ts (1)
46-47: LGTM!apps/meteor/server/api/v1/rooms.ts (1)
1139-1139: 🗄️ Data Integrity & IntegrationNo change needed.
ForbiddenErrorResponseSchemaallows optionalerror,status, andmessage, so it accepts bothAPI.v1.forbidden()bodies (error: 'unauthorized'anderror: 'forbidden').apps/meteor/server/api/ApiClass.ts (1)
898-904: 🗄️ Data Integrity & IntegrationNo change needed. The 401/403 mapping matches the current API error-code contract: missing-session/authentication paths return separate
unauthorized()responses, while these codes are used for authenticated permission/access failures.
| --- | ||
| "@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.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== changeset files =="
git ls-files .changeset | sed -n '1,80p'
echo
echo "== target changeset =="
cat -n .changeset/real-mails-count.md
echo
echo "== package identifiers / monorepo =="
git ls-files package.json "**/package.json" | sed -n '1,120p'
rg -n '"(`@rocket.chat/meteor`|`@rocket.chat/http-router`)"|name:"(`@rocket.chat/meteor`|`@rocket.chat/http-router`)"' package.json $(git ls-files '*package.json') 2>/dev/null | sed -n '1,120p'
echo
echo "== repo policy hints for breaking changesets =="
git ls-files | rg -i '(changeset|change.*set|release|changelog|contribut|README|license|package\.json' | sed -n '1,120p'
rg -n 'breaking|!!!|!|changeset|patch|minor|major|release|change' .changeset README.md package.json 2>/dev/null | sed -n '1,160p'
echo
echo "== related status code descriptions in target package files, if present =="
rg -n '401|403|fix\(api\)|authorization failures|missing session' . 2>/dev/null | sed -n '1,200p'Repository: RocketChat/Rocket.Chat
Length of output: 6610
🌐 Web query:
Changesets convention breaking change patch release ! exclamation mark semantic version breaking change
💡 Result:
In the Changesets workflow, the exclamation mark (!) convention typically used in Conventional Commits to denote breaking changes is not a native feature of the Changesets tool itself [1][2][3]. Changesets operates on the principle of "intentional releases" rather than parsing commit history [3]. When you create a changeset, you manually declare the intended semantic version bump type (major, minor, or patch) directly within the changeset file’s YAML front matter [4][5][6]. Key distinctions to keep in mind: 1. Intentional vs. Automated: Unlike tools such as semantic-release, which may scan commit messages for the ! convention to automate versioning, Changesets requires the developer to explicitly choose the bump type when running the changeset command [3][5]. 2. Defining Breaking Changes: If a change is breaking, you must manually set the package bump type to major in the changeset file [6][7]. The exclamation mark has no functional impact on the Changesets CLI logic [1][8]. 3. Changelog Documentation: While the ! does not trigger versioning behavior, it is common practice to document breaking changes clearly within the summary section of the changeset file [5][9]. Because Changesets allows you to include markdown in this summary, you should describe the breaking change and provide migration instructions directly there [5][6][9]. If you are seeing the exclamation mark used in conjunction with Changesets in some repositories, it is likely part of the project's internal contribution guidelines (e.g., to help human reviewers identify breaking changes in PRs) rather than a feature of the Changesets tool itself [10].
Citations:
- 1: https://github.com/changesets/changesets/
- 2: https://github.com/changesets/changesets/blob/main/docs/decisions.md
- 3: https://levelup.gitconnected.com/intentional-releases-why-chose-changesets-over-semantic-release-9d16d693540b
- 4: https://github.com/changesets/changesets/blob/main/docs/dictionary.md
- 5: https://github.com/changesets/changesets/blob/master/docs/adding-a-changeset.md
- 6: https://github.com/changesets/changesets/blob/main/docs/detailed-explanation.md
- 7: https://github.com/changesets/changesets/blob/ccce7433d5183fbccb3e3ad9005c66c3b3bc6bd9/packages/cli/README.md
- 8: https://www.npmjs.com/package/@changesets/cli
- 9: https://opensource.adobe.com/spectrum-web-components/guides/writing-changesets/
- 10: https://github.com/microsoft/FluidFramework/blob/main/.changeset/README.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== .changeset configuration =="
cat -n .changeset/config.json
echo
echo "== .changeset README snippets =="
wc -l .changeset/README.md
sed -n '1,220p' .changeset/README.md
echo
echo "== package metadata =="
python3 - <<'PY'
import json
from pathlib import Path
for p in [Path('package.json'), Path('apps/meteor/package.json'), Path('packages/http-router/package.json')]:
if p.exists():
print(f'--- {p} ---')
data=json.load(open(p))
print(f"publishConfig={data.get('publishConfig')}")
print(f"name={data.get('name')}")
print(f"version={data.get('version')}")
print()
PY
echo
echo "== search for changeset breaking/major/minor policy or conventional commit usage =="
rg -n 'Breaking|breaking|major|minor|patch|!!!|Conventional|changeset|Version|Release|version' .github .changeset README.md CONTRIBUTING.md package.json scripts scripts/** 2>/dev/null | sed -n '1,220p'Repository: RocketChat/Rocket.Chat
Length of output: 22882
🌐 Web query:
Google API error 401 auth required 403 forbidden status code REST API authorization breaking change
💡 Result:
When interacting with Google APIs, 401 Unauthorized and 403 Forbidden errors signify distinct issues in the request lifecycle: 401 errors indicate that Google cannot verify who is making the request (authentication), while 403 errors indicate that the authenticated identity lacks permission to access the requested resource (authorization) [1][2][3]. 401 Unauthorized (Authentication Required) A 401 status code means the request is missing a valid access token or the provided credentials are invalid, expired, or malformed [1][4][2]. Common causes include: - Expired or revoked access tokens [1][2][5]. - An incorrect or missing Authorization header [2]. - Mismatched credentials (e.g., the token project does not match the resource project) [2]. - Improperly registered or disabled OAuth 2.0 clients [6]. Resolution: Ensure your application provides a valid OAuth 2.0 access token [1][3]. If using client libraries, they often handle token refreshes automatically; otherwise, you must implement logic to refresh the token using a long-lived refresh token or re-run the OAuth flow [1][4]. 403 Forbidden (Permission Denied) A 403 status code means the request was successfully authenticated, but the identity lacks the necessary privileges to perform the requested action [1][2][7]. Common causes include: - Insufficient Identity and Access Management (IAM) permissions [2][8]. - Exceeding API rate limits or usage quotas [4][2]. - The requested resource does not exist or is protected by a policy that explicitly denies access [2][8]. - API usage restrictions, such as the API not being enabled for the project or the user not being added to an application's test user list [6][9]. Resolution: Evaluate the reason field in the API's JSON error response to determine the specific cause [1][2]. Use the Policy Troubleshooter in the Google Cloud Console to identify which IAM policies are blocking access for a specific principal, resource, and permission [8][10]. Breaking Changes and REST API Authorization Google maintains strict compatibility standards [11]. While minor and patch updates must not break existing client code, major version changes (encoded in the URI path) may introduce incompatible changes [12][11]. Google recommends consulting AIP-180 and AIP-181 for guidance when breaking changes are necessary for an API [12]. Always prefer official client libraries, as they are designed to handle authentication, retries, and versioning complexities behind the scenes, reducing the impact of underlying API surface updates [1][4]. For further troubleshooting, consult the specific documentation for the Google API you are using, as error details vary by service [1][4][13]. Log API errors on the client side, as Google does not provide full API request logs to developers for privacy and security reasons [13].
Citations:
- 1: https://developers.google.com/workspace/drive/api/guides/handle-errors
- 2: https://docs.cloud.google.com/storage/docs/json_api/v1/status-codes
- 3: https://cloudwebschool.com/docs/gcp/troubleshooting/troubleshooting-authentication-errors/
- 4: https://developers.google.com/workspace/gmail/api/guides/handle-errors
- 5: https://developers.google.com/identity/protocols/oauth2
- 6: https://support.google.com/accounts/answer/16668185?hl=en
- 7: https://cloud.google.com/api-gateway/docs/troubleshoot
- 8: https://docs.cloud.google.com/iam/docs/resolve-permission-errors
- 9: https://support.google.com/code/answer/61523?hl=en
- 10: https://cloud.google.com/policy-intelligence/docs/troubleshoot-access
- 11: https://cloud.google.com/apis/design/compatibility
- 12: https://cloud.google.com/apis/design/versioning
- 13: https://cloud.google.com/apis/docs/troubleshooting
Align the patch-level changeset with the visible API behavior.
The entries for @rocket.chat/meteor and @rocket.chat/http-router are patch, while the summary documents a change in observable status codes from 401 to 403. Use the breaking-release level for these packages, or replace fix(api)! with a non-breaking summary if this change can be demonstrated as compatible at patch time.
🤖 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, Update the changeset
metadata for `@rocket.chat/meteor` and `@rocket.chat/http-router` to use the
breaking-release level that matches the documented 401-to-403 API behavior, or
revise the summary to remove the breaking marker only if the change is confirmed
backward-compatible.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 7 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=".changeset/real-mails-count.md">
<violation number="1" location=".changeset/real-mails-count.md:6">
P2: The summary marks this as a breaking change with `fix(api)!:`, yet both packages are bumped at `patch` in the frontmatter. Since the PR rewrites REST error-status semantics (401→403 for authorization failures), clients depending on the old codes are impacted; either bump to `major` to match the `!` marker, or drop the `!` if the intent is a non-breaking patch.</violation>
</file>
<file name="apps/meteor/server/api/ApiClass.ts">
<violation number="1" location="apps/meteor/server/api/ApiClass.ts:898">
P2: This remap causes any `Error('error-not-authorized')` thrown from typed routes to resolve to a 403 response, but several routes registered with `typed: true` (e.g. `rooms.adminRooms`) don't declare a `403` schema in their response map — only `400`/`401`. Since the typed Router validates responses against declared schemas under `TEST_MODE`, those routes will throw a missing-response-validator error instead of returning the intended 403. Add `403: validateForbiddenErrorResponse` to all typed routes that can throw `error-not-authorized`, or scope this remap to endpoints whose specs were already updated.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| "@rocket.chat/http-router": patch | ||
| --- | ||
|
|
||
| fix(api)!: return 403 for authorization failures, reserve 401 for missing session |
There was a problem hiding this comment.
P2: The summary marks this as a breaking change with fix(api)!:, yet both packages are bumped at patch in the frontmatter. Since the PR rewrites REST error-status semantics (401→403 for authorization failures), clients depending on the old codes are impacted; either bump to major to match the ! marker, or drop the ! if the intent is a non-breaking patch.
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 summary marks this as a breaking change with `fix(api)!:`, yet both packages are bumped at `patch` in the frontmatter. Since the PR rewrites REST error-status semantics (401→403 for authorization failures), clients depending on the old codes are impacted; either bump to `major` to match the `!` marker, or drop the `!` if the intent is a non-breaking patch.</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>
| } catch (e: any) { | ||
| result = ((e: any) => { | ||
| switch (e.error) { | ||
| const errorKey = typeof e === 'string' ? e : e.error || e.message; |
There was a problem hiding this comment.
P2: This remap causes any Error('error-not-authorized') thrown from typed routes to resolve to a 403 response, but several routes registered with typed: true (e.g. rooms.adminRooms) don't declare a 403 schema in their response map — only 400/401. Since the typed Router validates responses against declared schemas under TEST_MODE, those routes will throw a missing-response-validator error instead of returning the intended 403. Add 403: validateForbiddenErrorResponse to all typed routes that can throw error-not-authorized, or scope this remap to endpoints whose specs were already updated.
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 898:
<comment>This remap causes any `Error('error-not-authorized')` thrown from typed routes to resolve to a 403 response, but several routes registered with `typed: true` (e.g. `rooms.adminRooms`) don't declare a `403` schema in their response map — only `400`/`401`. Since the typed Router validates responses against declared schemas under `TEST_MODE`, those routes will throw a missing-response-validator error instead of returning the intended 403. Add `403: validateForbiddenErrorResponse` to all typed routes that can throw `error-not-authorized`, or scope this remap to endpoints whose specs were already updated.</comment>
<file context>
@@ -895,14 +895,13 @@ export class APIClass<TBasePath extends string = '', TOperations extends Record<
} catch (e: any) {
result = ((e: any) => {
- switch (e.error) {
+ const errorKey = typeof e === 'string' ? e : e.error || e.message;
+ switch (errorKey) {
case 'error-too-many-requests':
</file context>
c7223ec to
c7cefd3
Compare
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
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-params.Documentation