Skip to content

fix(api)!: return 403 for authorization failures, reserve 401 for missing session - #41658

Closed
surjeetkumar8006 wants to merge 2 commits into
RocketChat:developfrom
surjeetkumar8006:fix/api-401-403-semantics-41589
Closed

fix(api)!: return 403 for authorization failures, reserve 401 for missing session#41658
surjeetkumar8006 wants to merge 2 commits into
RocketChat:developfrom
surjeetkumar8006:fix/api-401-403-semantics-41589

Conversation

@surjeetkumar8006

@surjeetkumar8006 surjeetkumar8006 commented Aug 2, 2026

Copy link
Copy Markdown

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.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes
    • Authorization failures now consistently return HTTP 403, while HTTP 401 is reserved for missing or invalid sessions.
    • Updated room access endpoints to report forbidden access correctly.
    • Standardized invalid request body errors under the error-invalid-params error type.

…sing session (RocketChat#41589)

- Map error-unauthorized and error-not-authorized to 403 Forbidden in ApiClass
- Align Router.ts body validation errorType to 'error-invalid-params'
- Clean up permissions middleware dead branch
- Update rooms endpoints permission checks and schemas from 401 to 403

Closes RocketChat#41589
@surjeetkumar8006
surjeetkumar8006 requested a review from a team as a code owner August 2, 2026 11:15
@dionisio-bot

dionisio-bot Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label
  • This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot

changeset-bot Bot commented Aug 2, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 7b2e48c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@rocket.chat/meteor Patch
@rocket.chat/http-router Patch
@rocket.chat/core-services Patch
@rocket.chat/federation-matrix Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch

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

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@dionisio-bot dionisio-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changeset mismatch

The PR title indicates a breaking change (!), but no changeset declares a major bump — at least one is required.

Please align the PR title, milestone and changesets.

@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6d54460a-2dc1-4d66-96a4-012f7e1cd445

📥 Commits

Reviewing files that changed from the base of the PR and between 94478c6 and 7b2e48c.

📒 Files selected for processing (5)
  • .changeset/real-mails-count.md
  • apps/meteor/server/api/ApiClass.ts
  • apps/meteor/server/api/v1/middlewares/permissions.ts
  • apps/meteor/server/api/v1/rooms.ts
  • packages/http-router/src/Router.ts

Walkthrough

The API now returns 403 for authorization failures when a session exists. Missing sessions continue to use 401. Body-validation failures now use error-invalid-params, with release metadata documenting the changes.

Changes

API error contracts

Layer / File(s) Summary
Authorization response mapping
apps/meteor/server/api/ApiClass.ts, apps/meteor/server/api/v1/middlewares/permissions.ts
The route error mapper recognizes error-not-authorized. Permission failures now always return forbidden responses.
Room endpoint authorization statuses
apps/meteor/server/api/v1/rooms.ts
rooms.membersOrderedByRole, rooms.hide, and rooms.bannedUsers declare and return HTTP 403 when access is denied.
Body validation error type
packages/http-router/src/Router.ts
Body-validation failures now use error-invalid-params.
Release metadata
.changeset/real-mails-count.md
The changeset records the authorization status-code updates for both packages.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

Possibly related PRs

  • RocketChat/Rocket.Chat#41387 — Both PRs modify room authorization handling, but target different endpoints and access-control behavior.
  • RocketChat/Rocket.Chat#41590 — This PR implements the same REST authorization-status changes across the API mapper, permissions middleware, and room endpoints.

Suggested labels: type: bug

Suggested reviewers: sampaiodiego


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Warning

⚠️ This pull request shows signs of AI-generated slop (description_diff_mismatch). It has been flagged by CodeRabbit slop detection and should be reviewed carefully.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2b7abb0fc1

ℹ️ 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();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Register the new 403 response

For rooms.membersOrderedByRole, the response spec above this action still only registers 200/400/401/404. When this broadcast-room permission branch is hit under TEST_MODE, the typed router looks up a validator for status 403 and throws Missing response validator... instead of returning the intended forbidden response; it also leaves the generated route metadata without the new 403. Add 403: validateForbiddenErrorResponse to this endpoint's response map along with the behavior change.

Useful? React with 👍 / 👎.

{
success: false,
errorType: 'invalid-params',
errorType: 'error-invalid-params',

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Update body-validation expectations

When packages/http-router/src/Router.spec.ts runs, the two invalid body cases still assert errorType is invalid-params; this line now emits error-invalid-params, so the existing package test fails even though the behavior change is intentional. Please update those expectations in the same change, or preserve the old emitted value.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants