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

.changeset/rest-401-403-semantics.md

@@ -0,0 +1,14 @@
+---
+'@rocket.chat/meteor': minor
+---
+
+Stops the REST API from answering `401 Unauthorized` for authorization failures, so that a 401 only ever means "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 once the 9.0.0 breaking-change flag turned on — 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 broke live sessions in the `sendMessage` / `getReadReceipts` migration.
+
+- `error-unauthorized` / `unauthorized` now always map to 403, dropping the 401 they would have been promoted to in 9.0.0 (403 today, unchanged).
+- `error-not-authorized` / `not-authorized` (the dominant permission code, previously unmapped and falling through to 400) map to 403 from 9.0.0.
+- `rooms.membersOrderedByRole` (broadcast member list), `rooms.hide` and `rooms.bannedUsers` returned 401 for access/permission failures; they return 403 from 9.0.0, and their response schemas now declare it.
+- Removed a dead `if (applyBreakingChanges)` fork in the permissions middleware whose 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 and is tracked separately.

apps/meteor/server/api/ApiClass.ts

@@ -898,12 +898,17 @@ export class APIClass<TBasePath extends string = '', TOperations extends Record<
 								switch (e.error) {
 									case 'error-too-many-requests':
 										return api.tooManyRequests(typeof e === 'string' ? e : e.message);
+									// every `error-unauthorized`/`error-not-authorized` throw in the codebase follows an authorization check,
+									// so they map to 403; 401 is reserved for a missing or invalid session
 									case 'unauthorized':
 									case 'error-unauthorized':
+										return api.forbidden(typeof e === 'string' ? e : e.message);
+									case 'not-authorized':
+									case 'error-not-authorized':
 										if (applyBreakingChanges) {
-											return api.unauthorized(typeof e === 'string' ? e : e.message);
+											return api.forbidden(typeof e === 'string' ? e : e.message);
 										}
-										return api.forbidden(typeof e === 'string' ? e : e.message);
+										return api.failure(typeof e === 'string' ? e : e.message, e.error, process.env.TEST_MODE ? e.stack : undefined, e);
 									case 'forbidden':
 									case 'error-forbidden':
 										if (applyBreakingChanges) {

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

apps/meteor/server/api/v1/rooms.ts

@@ -77,7 +77,7 @@ import { saveNotificationSettingsMethod } from '../../meteor-methods/users/saveN
 import type { NotificationFieldType } from '../../meteor-methods/users/saveNotificationSettings';
 import { roomsGetMethod } from '../../publications/room';
 import { settings } from '../../settings';
-import type { ExtractRoutesFromAPI } from '../ApiClass';
+import { applyBreakingChanges, type ExtractRoutesFromAPI } from '../ApiClass';
 import { API } from '../api';
 import { MultipartUploadHandler } from '../lib/MultipartUploadHandler';
 import { composeRoomWithLastMessage } from '../lib/composeRoomWithLastMessage';
@@ -1134,6 +1134,7 @@ API.v1.get(
 			}),
 			400: validateBadRequestErrorResponse,
 			401: validateUnauthorizedErrorResponse,
+			403: validateForbiddenErrorResponse,
 			404: validateNotFoundErrorResponse,
 		},
 	},
@@ -1152,7 +1153,8 @@ API.v1.get(
 		}
 
 		if (findResult.broadcast && !(await hasPermissionAsync(this.user, 'view-broadcast-member-list', findResult._id))) {
-			return API.v1.unauthorized();
+			// TODO: MAJOR drop the 401 branch — an authorization failure is 403
+			return applyBreakingChanges ? API.v1.forbidden() : API.v1.unauthorized();
 		}
 
 		// Ensures that role priorities for the specified room are synchronized correctly.
@@ -1292,13 +1294,15 @@ 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();
+			// TODO: MAJOR drop the 401 branch — an authorization failure is 403
+			return applyBreakingChanges ? API.v1.forbidden() : API.v1.unauthorized();
 		}
 
 		const user = await Users.findOneById(this.userId, { projection: { _id: 1 } });
@@ -1695,13 +1699,15 @@ 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();
+				// TODO: MAJOR drop the 401 branch — an authorization failure is 403
+				return applyBreakingChanges ? API.v1.forbidden() : API.v1.unauthorized();
 			}
 
 			const { offset, count } = await getPaginationItems(this.queryParams);

apps/meteor/tests/end-to-end/api/rooms.ts

@@ -4663,6 +4663,7 @@ describe('[Rooms]', () => {
 				.set(nonMemberCredentials)
 				.send({ roomId: roomB._id })
 				.expect('Content-Type', 'application/json')
+				// TODO: MAJOR 403 — currently 401 until the 9.0.0 breaking changes are applied
 				.expect(401)
 				.expect((res) => {
 					expect(res.body).to.have.property('success', false);
@@ -5101,6 +5102,28 @@ 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')
+					// TODO: MAJOR 403 — currently 401 until the 9.0.0 breaking changes are applied
+					.expect(401)
+					.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'))