Skip to content

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

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 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 @@
---
"@rocket.chat/meteor": patch
"@rocket.chat/http-router": patch
---

fix(api): return 403 for authorization failures, reserve 401 for missing session
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;

@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 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>

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':

@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 403 schemas for remapped authorization errors

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

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
12 changes: 9 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 @@ -736,6 +736,7 @@ API.v1.get(
}),
400: validateBadRequestErrorResponse,
401: validateUnauthorizedErrorResponse,
403: validateForbiddenErrorResponse,
},
},
async function action() {
Expand Down Expand Up @@ -775,6 +776,7 @@ API.v1.get(
}),
400: validateBadRequestErrorResponse,
401: validateUnauthorizedErrorResponse,
403: validateForbiddenErrorResponse,
},
},
async function action() {
Expand Down Expand Up @@ -810,6 +812,7 @@ API.v1.get(
}),
400: validateBadRequestErrorResponse,
401: validateUnauthorizedErrorResponse,
403: validateForbiddenErrorResponse,
},
},
async function action() {
Expand Down Expand Up @@ -1136,6 +1139,7 @@ API.v1.get(
}),
400: validateBadRequestErrorResponse,
401: validateUnauthorizedErrorResponse,
403: validateForbiddenErrorResponse,
404: validateNotFoundErrorResponse,
},
},
Expand All @@ -1154,7 +1158,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();
}

// Ensures that role priorities for the specified room are synchronized correctly.
Expand Down Expand Up @@ -1294,13 +1298,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 +1702,14 @@ export const roomEndpoints = API.v1
response: {
200: roomsBannedUsersResponseSchema,
401: validateUnauthorizedErrorResponse,
403: validateForbiddenErrorResponse,
},
},
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',
error: validatorFn.errors?.map((error: any) => error.message).join('\n '),
},
400,
Expand Down