diff --git a/.changeset/rest-401-403-semantics.md b/.changeset/rest-401-403-semantics.md new file mode 100644 index 0000000000000..682e1abae59d4 --- /dev/null +++ b/.changeset/rest-401-403-semantics.md @@ -0,0 +1,18 @@ +--- +'@rocket.chat/meteor': major +--- + +**Breaking:** the REST API no longer answers `401 Unauthorized` for authorization failures. A 401 now means one thing only — the request carries no valid session. + +Every `error-unauthorized` / `error-not-authorized` throw in the codebase follows an authorization check (`hasPermissionAsync`, `canAccessRoom`, `canDeleteFile`, agent department scope), never a missing session, but the error switch in `server/api/ApiClass.ts` mapped `error-unauthorized` to 401 while the permission middleware answered 403 for the very same condition. A client could not infer session state from the status code, which is what logged out live sessions in the `sendMessage` / `getReadReceipts` migration. + +Status code changes: + +- `error-not-authorized` / `not-authorized` — the dominant permission code, previously unmapped and falling through to `API.v1.failure()`: **400 → 403**. +- `error-unauthorized` / `unauthorized`: stays **403**, and no longer flips to 401. +- `rooms.membersOrderedByRole` (broadcast member list), `rooms.hide`, `rooms.bannedUsers` — returned 401 for access and permission failures: **401 → 403**. Their response schemas now declare it. +- Routes declaring `permissionsRequired` without `authRequired`, called with no session: **403 → 401**. + +Also removed a dead `if (applyBreakingChanges)` fork in the permissions middleware whose two branches were identical. + +The unauthenticated side is unchanged here: those checks still throw `error-invalid-user` and answer 400. Splitting them out needs a dedicated error code, since `error-invalid-user` also means "that user does not exist", and is tracked separately. diff --git a/apps/meteor/server/api/ApiClass.ts b/apps/meteor/server/api/ApiClass.ts index 71dfc86bc0020..e1c54b24d1be8 100644 --- a/apps/meteor/server/api/ApiClass.ts +++ b/apps/meteor/server/api/ApiClass.ts @@ -898,11 +898,12 @@ export class APIClass { .set(nonMemberCredentials) .send({ roomId: roomB._id }) .expect('Content-Type', 'application/json') - .expect(401) + .expect(403) .expect((res) => { expect(res.body).to.have.property('success', false); }); @@ -5101,6 +5101,27 @@ describe('[Rooms]', () => { await Promise.all(bannedUsers.map((user) => deleteUser(user))); }); + it('should return forbidden if user does not have access to the room', async () => { + const privateRoom = (await createRoom({ type: 'p', name: `banned-users-private-${Date.now()}-${Math.random()}` })).body.group; + const nonMember = await createUser({ joinDefaultChannels: false }); + const nonMemberCredentials = await login(nonMember.username, password); + + try { + await request + .get(api('rooms.bannedUsers')) + .set(nonMemberCredentials) + .query({ roomId: privateRoom._id }) + .expect('Content-Type', 'application/json') + .expect(403) + .expect((res) => { + expect(res.body).to.have.property('success', false); + }); + } finally { + await deleteRoom({ type: 'p', roomId: privateRoom._id }); + await deleteUser(nonMember); + } + }); + it('should list every banned user of the room with only the projected fields', async () => { const res = await request .get(api('rooms.bannedUsers'))