RocketChat / RocketChat/Rocket.Chat

REST API: 401/403 semantics inverted — `error-unauthorized` means permission denied, `error-invalid-user` means unauthenticated

Open
#41,589 3 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

type: bug
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 to API.v1.failure()HTTP 400.
  • "You are logged in but not allowed" is reported with error-unauthorized / error-not-authorized; error-unauthorized is mapped to HTTP 401 once the 9.0.0 breaking-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

  1. ApiClass.ts:900-906 — map error-unauthorized and error-not-authorized to forbidden() (403). This removes 401 from permission denials before 9.0.0 activates it.
  2. 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 of error-invalid-user is not viable because the code also means "user not found".
  3. rooms.ts:1155,1301,1704 — return 403 and update the response schemas.
  4. 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.