Skip to content

refactor(authorization): forward only { _id, roles } from hasPermission wrappers #41413

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ggazzo merged 1 commit into from
Jul 16, 2026
Merged

refactor(authorization): forward only { _id, roles } from hasPermission wrappers #41413

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions apps/meteor/server/lib/authorization/hasPermission.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,23 @@ import { Authorization } from '@rocket.chat/core-services';
import type { UserWithRoles } from '@rocket.chat/core-services';
import type { IUser, IPermission, IRoom } from '@rocket.chat/core-typings';

// Forward only the fields the permission check needs, so a full user document
// (with services, e2e keys, etc.) isn't serialized to the authorization service.
const toSubject = (user: IUser['_id'] | UserWithRoles): IUser['_id'] | UserWithRoles =>
typeof user === 'string' ? user : { _id: user._id, roles: user.roles };
Comment on lines +7 to +8

@hacktron-app hacktron-app Bot Jul 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MEDIUM Unhandled TypeError / Denial of Service via null/undefined user in permission checks

The helper function toSubject in apps/meteor/server/lib/authorization/hasPermission.ts converts a user object or user ID into a subject representation for permission checks. However, it does not check if the user argument is null or undefined. If user is null or undefined (e.g., when a user is unauthenticated or when a database lookup fails), typeof user === 'string' evaluates to false. The function then attempts to evaluate { _id: user._id, roles: user.roles }, which throws a TypeError: Cannot read properties of null (reading '_id') or TypeError: Cannot read properties of undefined (reading '_id'). Previously, passing null or undefined to permission check functions like hasPermissionAsync was safe because the downstream Authorization service explicitly checked if (!userId) { return false; }. Introducing toSubject without a null/undefined check causes unhandled exceptions (500 errors) in calling contexts, leading to Denial of Service or broken application flows for anonymous/guest access.

Steps to Reproduce
  1. Invoke any Meteor method or API endpoint that performs a permission check using hasPermissionAsync(userId, ...) or hasAllPermissionAsync(userId, ...) while unauthenticated (where userId is null or undefined).
  2. The helper function toSubject will fail to handle the null or undefined value, throwing a TypeError: Cannot read properties of null (reading '_id').
  3. This unhandled exception crashes the execution context of the request, returning a 500 Internal Server Error instead of gracefully returning false.
Fix with AI

Open in Cursor Open in Claude

A security vulnerability was found by Hacktron.

File: apps/meteor/server/lib/authorization/hasPermission.ts
Lines: 7-8
Severity: medium

Vulnerability: Unhandled TypeError / Denial of Service via null/undefined user in permission checks

Description:
The helper function `toSubject` in `apps/meteor/server/lib/authorization/hasPermission.ts` converts a user object or user ID into a subject representation for permission checks. However, it does not check if the `user` argument is null or undefined. If `user` is null or undefined (e.g., when a user is unauthenticated or when a database lookup fails), `typeof user === 'string'` evaluates to `false`. The function then attempts to evaluate `{ _id: user._id, roles: user.roles }`, which throws a `TypeError: Cannot read properties of null (reading '_id')` or `TypeError: Cannot read properties of undefined (reading '_id')`. Previously, passing `null` or `undefined` to permission check functions like `hasPermissionAsync` was safe because the downstream `Authorization` service explicitly checked `if (!userId) { return false; }`. Introducing `toSubject` without a null/undefined check causes unhandled exceptions (500 errors) in calling contexts, leading to Denial of Service or broken application flows for anonymous/guest access.

Proof of Concept:
**Steps to Reproduce**

1. Invoke any Meteor method or API endpoint that performs a permission check using `hasPermissionAsync(userId, ...)` or `hasAllPermissionAsync(userId, ...)` while unauthenticated (where `userId` is `null` or `undefined`).
2. The helper function `toSubject` will fail to handle the `null` or `undefined` value, throwing a `TypeError: Cannot read properties of null (reading '_id')`.
3. This unhandled exception crashes the execution context of the request, returning a 500 Internal Server Error instead of gracefully returning `false`.

Affected Code:
const toSubject = (user: IUser['_id'] | UserWithRoles): IUser['_id'] | UserWithRoles =>
	typeof user === 'string' ? user : { _id: user._id, roles: user.roles };

Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.

Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.

Triage: Reply !fp <reason> (false positive), !valid (confirmed), !accepted_risk <reason>, or !fixed (resolved). Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.

View finding in Hacktron

@ggazzo ggazzo Jul 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!fp there is no away to have undefined at that point


export const hasAllPermissionAsync = async (
user: IUser['_id'] | UserWithRoles,
permissions: IPermission['_id'][],
scope?: IRoom['_id'],
): Promise<boolean> => Authorization.hasAllPermission(user, permissions, scope);
): Promise<boolean> => Authorization.hasAllPermission(toSubject(user), permissions, scope);
export const hasPermissionAsync = async (
user: IUser['_id'] | UserWithRoles,
permissionId: IPermission['_id'],
scope?: IRoom['_id'],
): Promise<boolean> => Authorization.hasPermission(user, permissionId, scope);
): Promise<boolean> => Authorization.hasPermission(toSubject(user), permissionId, scope);
export const hasAtLeastOnePermissionAsync = async (
user: IUser['_id'] | UserWithRoles,
permissions: IPermission['_id'][],
scope?: IRoom['_id'],
): Promise<boolean> => Authorization.hasAtLeastOnePermission(user, permissions, scope);
): Promise<boolean> => Authorization.hasAtLeastOnePermission(toSubject(user), permissions, scope);
Loading