From 504dbca828ff54b997a7bb0ccb5698bb83d8bbbc Mon Sep 17 00:00:00 2001 From: surjeetkumar800 Date: Sun, 2 Aug 2026 18:37:23 +0530 Subject: [PATCH] fix(api): return 403 for authorization failures, reserve 401 for missing session --- .changeset/real-mails-count.md | 6 +++++ apps/meteor/ee/server/api/ldap.ts | 2 +- apps/meteor/server/api/ApiClass.ts | 22 +++++++++++-------- apps/meteor/server/api/v1/ldap.ts | 4 ++-- .../server/api/v1/middlewares/permissions.ts | 9 ++------ apps/meteor/server/api/v1/rooms.ts | 12 +++++++--- docs/api-endpoint-migration.md | 4 ++-- packages/http-router/src/Router.spec.ts | 4 ++-- packages/http-router/src/Router.ts | 2 +- 9 files changed, 38 insertions(+), 27 deletions(-) create mode 100644 .changeset/real-mails-count.md diff --git a/.changeset/real-mails-count.md b/.changeset/real-mails-count.md new file mode 100644 index 0000000000000..2b53d69d99c3f --- /dev/null +++ b/.changeset/real-mails-count.md @@ -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 diff --git a/apps/meteor/ee/server/api/ldap.ts b/apps/meteor/ee/server/api/ldap.ts index e478ebc96b470..4bba27ca147e9 100644 --- a/apps/meteor/ee/server/api/ldap.ts +++ b/apps/meteor/ee/server/api/ldap.ts @@ -29,7 +29,7 @@ API.v1.post( }, async function action() { if (!this.userId) { - throw new Error('error-invalid-user'); + throw new Error('unauthorized'); } if (!(await hasPermissionAsync(this.user, 'sync-auth-services-users'))) { diff --git a/apps/meteor/server/api/ApiClass.ts b/apps/meteor/server/api/ApiClass.ts index 71dfc86bc0020..7b1ee82c1f63e 100644 --- a/apps/meteor/server/api/ApiClass.ts +++ b/apps/meteor/server/api/ApiClass.ts @@ -895,23 +895,27 @@ export class APIClass { - switch (e.error) { + const errorKey = typeof e === 'string' ? e : e?.error ?? e?.message; + const errorMessage = typeof e === 'string' + ? e + : e?.message || e?.reason || (typeof e?.error === 'string' ? e.error : undefined) || String(e); + switch (errorKey) { case 'error-too-many-requests': - return api.tooManyRequests(typeof e === 'string' ? e : e.message); + return api.tooManyRequests(errorMessage); case 'unauthorized': + case 'error-invalid-user': + return api.unauthorized(errorMessage); case 'error-unauthorized': - if (applyBreakingChanges) { - return api.unauthorized(typeof e === 'string' ? e : e.message); - } - return api.forbidden(typeof e === 'string' ? e : e.message); + case 'error-not-authorized': + return api.forbidden(errorMessage); case 'forbidden': case 'error-forbidden': if (applyBreakingChanges) { - return api.forbidden(typeof e === 'string' ? e : e.message); + return api.forbidden(errorMessage); } - return api.failure(typeof e === 'string' ? e : e.message, e.error, process.env.TEST_MODE ? e.stack : undefined, e); + return api.failure(errorMessage, e?.error, process.env.TEST_MODE ? e?.stack : undefined, e); default: - return api.failure(typeof e === 'string' ? e : e.message, e.error, process.env.TEST_MODE ? e.stack : undefined, e); + return api.failure(errorMessage, e?.error, process.env.TEST_MODE ? e?.stack : undefined, e); } })(e); } finally { diff --git a/apps/meteor/server/api/v1/ldap.ts b/apps/meteor/server/api/v1/ldap.ts index 90f85afeee7bf..36cbd544b8f84 100644 --- a/apps/meteor/server/api/v1/ldap.ts +++ b/apps/meteor/server/api/v1/ldap.ts @@ -31,7 +31,7 @@ API.v1.post( }, async function action() { if (!this.userId) { - throw new Error('error-invalid-user'); + throw new Error('unauthorized'); } if (settings.get('LDAP_Enable') !== true) { @@ -65,7 +65,7 @@ API.v1.post( }, async function action() { if (!this.userId) { - throw new Error('error-invalid-user'); + throw new Error('unauthorized'); } if (settings.get('LDAP_Enable') !== true) { diff --git a/apps/meteor/server/api/v1/middlewares/permissions.ts b/apps/meteor/server/api/v1/middlewares/permissions.ts index 41412321605f8..4c4eca751afd0 100644 --- a/apps/meteor/server/api/v1/middlewares/permissions.ts +++ b/apps/meteor/server/api/v1/middlewares/permissions.ts @@ -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(); diff --git a/apps/meteor/server/api/v1/rooms.ts b/apps/meteor/server/api/v1/rooms.ts index c12a18653e1df..6edf25d4509c0 100644 --- a/apps/meteor/server/api/v1/rooms.ts +++ b/apps/meteor/server/api/v1/rooms.ts @@ -736,6 +736,7 @@ API.v1.get( }), 400: validateBadRequestErrorResponse, 401: validateUnauthorizedErrorResponse, + 403: validateForbiddenErrorResponse, }, }, async function action() { @@ -775,6 +776,7 @@ API.v1.get( }), 400: validateBadRequestErrorResponse, 401: validateUnauthorizedErrorResponse, + 403: validateForbiddenErrorResponse, }, }, async function action() { @@ -810,6 +812,7 @@ API.v1.get( }), 400: validateBadRequestErrorResponse, 401: validateUnauthorizedErrorResponse, + 403: validateForbiddenErrorResponse, }, }, async function action() { @@ -1136,6 +1139,7 @@ API.v1.get( }), 400: validateBadRequestErrorResponse, 401: validateUnauthorizedErrorResponse, + 403: validateForbiddenErrorResponse, 404: validateNotFoundErrorResponse, }, }, @@ -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. @@ -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 } }); @@ -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); diff --git a/docs/api-endpoint-migration.md b/docs/api-endpoint-migration.md index 3c1fb0f9043a1..4cc026faed118 100644 --- a/docs/api-endpoint-migration.md +++ b/docs/api-endpoint-migration.md @@ -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 @@ -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) diff --git a/packages/http-router/src/Router.spec.ts b/packages/http-router/src/Router.spec.ts index 57205a86b9898..f2b291900b977 100644 --- a/packages/http-router/src/Router.spec.ts +++ b/packages/http-router/src/Router.spec.ts @@ -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 () => { diff --git a/packages/http-router/src/Router.ts b/packages/http-router/src/Router.ts index 794874587b970..6480ade830299 100644 --- a/packages/http-router/src/Router.ts +++ b/packages/http-router/src/Router.ts @@ -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,