RocketChat / RocketChat/Rocket.Chat
REST API: 401/403 semantics inverted — `error-unauthorized` means permission denied, `error-invalid-user` means unauthenticated
Nobody has claimed this yet.
- Dominant language
- TypeScript
- Stars
- 46.1k
- Forks
- 13.9k
- Avg merge
- 3d 3h
- Merged PRs (30d)
- 130
Description
Summary
In the REST API, the HTTP status codes for authentication and authorization failures are inverted relative to their meaning:
- "You are not logged in" is reported with
error-invalid-user, which falls through toAPI.v1.failure()→ HTTP 400. - "You are logged in but not allowed" is reported with
error-unauthorized/error-not-authorized;error-unauthorizedis mapped to HTTP 401 once the9.0.0breaking-change flag turns on.
So the only error code promoted to 401 is one that never means "unauthenticated", and the condition that genuinely is 401 answers 400. Three endpoints already return 401 for authorization failures today, independent of the flag.
This matters beyond tidiness: a client cannot infer session state from the status code. Any client-side handling of "session expired" (redirect to login, clear stored credentials) built on a 401 will fire on permission denials instead, and will miss the real unauthenticated case.
Census
Counted over apps/meteor/server, apps/meteor/app, apps/meteor/ee (specs excluded), classifying each throw by the guard that precedes it.
| Actual intent | Error code | Sites | Status today | Status in 9.0.0 |
|---|---|---|---|---|
| Not authenticated (no session) | error-invalid-user |
134 | 400 | 400 |
| Not authenticated (no session) | error-action-not-allowed, error-not-allowed |
3 | 400 | 400 |
| Authenticated, no permission | error-not-authorized |
27 | 400 | 400 |
| Authenticated, no permission | error-unauthorized |
4 | 403 | 401 |
error-invalid-user has 244 occurrences in total; the 134 above are the subset guarded by a missing-session check (if (!this.userId), if (!Meteor.userId())). The rest use the same code for "that user does not exist", so the code carries two unrelated meanings.
Every error-unauthorized / error-not-authorized throw is an authorization check
No site among the 31 means "unauthenticated":
server/lib/users/setUserAvatar.ts:47 !hasPermissionAsync('edit-other-user-avatar')
server/meteor-methods/users/resetAvatar.ts:32 !canEditOtherUserAvatar
server/meteor-methods/platform/cloud.ts 10 permission checks
server/meteor-methods/rooms/archiveRoom.ts:38 permission check
server/meteor-methods/rooms/unarchiveRoom.ts:33 permission check
server/meteor-methods/rooms/getRoomJoinCode.ts:27 permission check
server/meteor-methods/messages/deleteFileMessage.ts:51 !Upload.canDeleteFile()
server/meteor-methods/auth/createToken.ts:16,20 secret mismatch, 'user-generate-access-token'
server/meteor-methods/auth/crowd.ts:28,60 permission check
server/api/lib/rooms.ts:27,68,113 permission scope
server/api/v1/omnichannel/lib/inquiries.ts:28 department not allowed for agent
server/slashcommands/archiveroom/server.ts:57 permission check
server/slashcommands/unarchiveroom/server.ts:56 permission check
server/bridges/slack/removeChannelLinks.ts:25 permission check
server/lib/omnichannel/closeLivechatRoom.ts:39 permission check
ee/server/api/ldap.ts:36 permission check
ee/server/lib/omnichannel/business-hour/lib/business-hour.ts:14 permission check
One partial exception: server/api/v1/middlewares/authenticationHono.ts:42 throws error-unauthorized for 'Users must have a username', which is closer to an unusable account than to a permission denial.
Where it goes wrong
1. The 9.0.0 mapping promotes permission denials to 401
apps/meteor/server/api/ApiClass.ts:900-906
case 'unauthorized':
case 'error-unauthorized':
if (applyBreakingChanges) {
return api.unauthorized(typeof e === 'string' ? e : e.message); // 401
}
return api.forbidden(typeof e === 'string' ? e : e.message); // 403
applyBreakingChanges = shouldBreakInVersion('9.0.0') (ApiClass.ts:63), so this is inert on 8.x and active from 9.0.0.
The case pair assumes error-unauthorized = unauthenticated and error-forbidden = no permission. The census above shows the codebase does not use it that way. The permission middleware disagrees with the switch as well — for the very same condition it answers 403 while embedding the code in the message (server/api/v1/middlewares/permissions.ts:28,47,51):
API.v1.forbidden('User does not have the permissions required for this action [error-unauthorized]')
Result in 9.0.0: the same permission denial returns 403 when the check lives in the middleware and 401 when it lives in the handler.
2. error-not-authorized is never mapped
The dominant permission code (27 sites) is not handled by the switch and falls to default: → API.v1.failure() → HTTP 400, both today and in 9.0.0.
3. Three endpoints return 401 for authorization failures today
| Location | Condition | Returns | Should be |
|---|---|---|---|
server/api/v1/rooms.ts:1155 |
!hasPermissionAsync(user, 'view-broadcast-member-list', rid) |
401 | 403 |
server/api/v1/rooms.ts:1301 (rooms.hide) |
!canAccessRoomIdAsync(roomId, userId) |
401 | 403 |
server/api/v1/rooms.ts:1704 (rooms.bannedUsers) |
!canAccessRoomIdAsync(roomId, userId) |
401 | 403 |
Their response schemas declare 401: validateUnauthorizedErrorResponse, so the published contract carries the mistake. Note also that rooms.ts:1147, in the same handler as line 1155, answers notFound for an access failure — three different treatments of the same class of condition in one file.
The only legitimate 401 producers are authenticationHono.ts:29 and permissions.ts:24 (both !user), and ApiClass.ts:1079,1095 (failed login).
4. Dead branch in the permission middleware
server/api/v1/middlewares/permissions.ts:45-52 — both sides of the if (applyBreakingChanges) are identical:
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);
}
The variable in the legacy branch is named failure, suggesting API.v1.failure was intended there and the if/else was left half-finished.
Why the DDP → REST migration makes this urgent
The 134 unauthenticated checks live almost entirely in apps/meteor/server/meteor-methods/. Called over DDP, the error code reaches the client verbatim and no status code is involved. As each method is migrated to a typed route, that same check starts answering HTTP 400 — indistinguishable from a validation error.
This already surfaced in the sendMessage / getReadReceipts migration (#40675), where a client-side handler that treated any REST 401 as an expired session logged out live sessions. That handler was reverted, and it cannot be reimplemented correctly until 401 has a single meaning.
Proposed fix, in order
ApiClass.ts:900-906— maperror-unauthorizedanderror-not-authorizedtoforbidden()(403). This removes 401 from permission denials before 9.0.0 activates it.- Introduce a dedicated code for the unauthenticated case (e.g.
error-unauthenticated) mapped to 401, and switch the ~134 missing-session guards to it. A blanket remap oferror-invalid-useris not viable because the code also means "user not found". rooms.ts:1155,1301,1704— return 403 and update the response schemas.permissions.ts:45-52— resolve the dead branch.
Steps 1 and 2 are breaking for API consumers (403 stays 403; the error-not-authorized cases move 400 → 403; missing-session cases move 400 → 401), so they belong in the 9.0.0 batch.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the status mapping in apps/meteor/server/api/ApiClass.ts, then read server/api/v1/middlewares/permissions.ts and the affected branches in server/api/v1/rooms.ts. Trace the missing-session guards in the meteor methods before changing their error codes. Done means unauthenticated requests consistently return 401, permission denials return 403, and the rooms response schemas match those statuses.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- api, authentication, authorization, backend-api-design
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 35/100