Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
surjeetkumar8006 wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
21ac7e9
fix(api): return 403 for authorization failures, reserve 401 for miss…
surjeetkumar800 Aug 2, 2026
14591cd
fix: address PR review feedback on 401/403 response schemas, test exp…
surjeetkumar800 Aug 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/real-mails-count.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---

@cubic-dev-ai cubic-dev-ai Bot Aug 2, 2026

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.

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>

"@rocket.chat/meteor": patch
"@rocket.chat/http-router": patch
---

fix(api)!: return 403 for authorization failures, reserve 401 for missing session
Comment on lines +1 to +6

@coderabbitai coderabbitai Bot Aug 2, 2026

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.

📐 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.

7 changes: 3 additions & 4 deletions apps/meteor/server/api/ApiClass.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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':
return api.tooManyRequests(typeof e === 'string' ? e : e.message);
case 'unauthorized':
case 'error-unauthorized':
if (applyBreakingChanges) {
return api.unauthorized(typeof e === 'string' ? e : e.message);
}
case 'error-not-authorized':
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
return api.forbidden(typeof e === 'string' ? e : e.message);
case 'forbidden':
case 'error-forbidden':
Expand Down
9 changes: 2 additions & 7 deletions apps/meteor/server/api/v1/middlewares/permissions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,8 @@ export const permissionsMiddleware =
}

if (!hasPermission) {
if (applyBreakingChanges) {
const forbidden = API.v1.forbidden('User does not have the permissions required for this action [error-unauthorized]');
return c.json(forbidden.body, forbidden.statusCode);
}

const failure = API.v1.forbidden('User does not have the permissions required for this action [error-unauthorized]');
return c.json(failure.body, failure.statusCode);
const forbidden = API.v1.forbidden('User does not have the permissions required for this action [error-unauthorized]');
return c.json(forbidden.body, forbidden.statusCode);
}

return next();
Expand Down
9 changes: 6 additions & 3 deletions apps/meteor/server/api/v1/rooms.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1136,6 +1136,7 @@ API.v1.get(
}),
400: validateBadRequestErrorResponse,
401: validateUnauthorizedErrorResponse,
403: validateForbiddenErrorResponse,
404: validateNotFoundErrorResponse,
},
},
Expand All @@ -1154,7 +1155,7 @@ API.v1.get(
}

if (findResult.broadcast && !(await hasPermissionAsync(this.user, 'view-broadcast-member-list', findResult._id))) {
return API.v1.unauthorized();
return API.v1.forbidden();

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 2, 2026

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 Add the 403 response schema for this new forbidden path

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 👍 / 👎.

Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
}

// Ensures that role priorities for the specified room are synchronized correctly.
Expand Down Expand Up @@ -1294,13 +1295,14 @@ API.v1.post(
200: successResponseSchema,
400: validateBadRequestErrorResponse,
401: validateUnauthorizedErrorResponse,
403: validateForbiddenErrorResponse,
},
},
async function action() {
const { roomId } = this.bodyParams;

if (!(await canAccessRoomIdAsync(roomId, this.userId))) {
return API.v1.unauthorized();
return API.v1.forbidden();
}

const user = await Users.findOneById(this.userId, { projection: { _id: 1 } });
Expand Down Expand Up @@ -1697,13 +1699,14 @@ export const roomEndpoints = API.v1
response: {
200: roomsBannedUsersResponseSchema,
401: validateUnauthorizedErrorResponse,
403: validateForbiddenErrorResponse,
},
},
Comment thread
coderabbitai[bot] marked this conversation as resolved.
async function action() {
const { roomId } = this.queryParams;

if (!(await canAccessRoomIdAsync(roomId, this.userId))) {
return API.v1.unauthorized();
return API.v1.forbidden();
}

const { offset, count } = await getPaginationItems(this.queryParams);
Expand Down
4 changes: 2 additions & 2 deletions docs/api-endpoint-migration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -597,7 +597,7 @@ expect(res.body).to.have.property('errorType', 'invalid-params');
expect(res.body).to.have.property('errorType', 'error-invalid-params');
```

This only affects **query** parameter validation (GET/DELETE). Body parameter validation (POST/PUT) keeps `'invalid-params'`.
This affects both **query** parameter validation (GET/DELETE) and **body** parameter validation (POST/PUT).

### Error message format changes

Expand All @@ -615,7 +615,7 @@ expect(res.body).to.have.property('error', "must have required property 'platfor

When migrating an endpoint, search for its tests and update:

1. `errorType` from `'invalid-params'` to `'error-invalid-params'` (for query params only)
1. `errorType` from `'invalid-params'` to `'error-invalid-params'` for query and body params
2. Remove `' [invalid-params]'` suffix from `error` message assertions
3. Verify that status codes remain the same (400 for validation errors)

Expand Down
4 changes: 2 additions & 2 deletions packages/http-router/src/Router.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,12 +207,12 @@ describe('Router', () => {
const invalidResponse = await request(app).post('/api/validate-body').send({ name: 'John' });

expect(invalidResponse.status).toBe(400);
expect(invalidResponse.body).toHaveProperty('errorType', 'invalid-params');
expect(invalidResponse.body).toHaveProperty('errorType', 'error-invalid-params');

const invalidTypeResponse = await request(app).post('/api/validate-body').send({ name: 'John', age: 'thirty' });

expect(invalidTypeResponse.status).toBe(400);
expect(invalidTypeResponse.body).toHaveProperty('errorType', 'invalid-params');
expect(invalidTypeResponse.body).toHaveProperty('errorType', 'error-invalid-params');
});

it('should validate response body in test mode', async () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/http-router/src/Router.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ export class Router<
return c.json(
{
success: false,
errorType: 'invalid-params',
errorType: 'error-invalid-params',
Comment thread
coderabbitai[bot] marked this conversation as resolved.

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 2, 2026

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 Keep body validation errorType compatible

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 cubic-dev-ai Bot Aug 2, 2026

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.

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>

@cubic-dev-ai cubic-dev-ai Bot Aug 2, 2026

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.

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>

error: validatorFn.errors?.map((error: any) => error.message).join('\n '),
},
400,
Expand Down