refactor(authorization): forward only { _id, roles } from hasPermission wrappers - #41413
Conversation
…on wrappers
The `hasPermissionAsync` / `hasAllPermissionAsync` / `hasAtLeastOnePermissionAsync`
helpers accept a full user object but only `_id` and `roles` are ever used by the
check. Passing the whole document means credential-bearing fields (`services`,
`e2e.private_key`, …) get serialized to the authorization service when it runs
out of process. Normalize object inputs to a minimal `{ _id, roles }` subject
before the call; string ids pass through unchanged.
|
Looks like this PR is ready to merge! 🎉 |
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (7)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (3)📚 Learning: 2026-02-26T19:25:44.063ZApplied to files:
📚 Learning: 2026-02-26T19:25:44.063ZApplied to files:
📚 Learning: 2026-05-06T12:21:44.083ZApplied to files:
WalkthroughPermission-check helpers now convert user inputs into reduced authorization subjects containing ChangesAuthorization subject narrowing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #41413 +/- ##
===========================================
- Coverage 68.48% 68.46% -0.03%
===========================================
Files 4092 4092
Lines 158216 158218 +2
Branches 28678 28650 -28
===========================================
- Hits 108351 108317 -34
- Misses 44827 44869 +42
+ Partials 5038 5032 -6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/jira ARCH-1464 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![hacktron-app[bot]](https://avatars.githubusercontent.com/in/1250513?s=60&v=4){kind=small}
| const toSubject = (user: IUser['_id'] | UserWithRoles): IUser['_id'] | UserWithRoles => | ||
| typeof user === 'string' ? user : { _id: user._id, roles: user.roles }; |
There was a problem hiding this comment.
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
- Invoke any Meteor method or API endpoint that performs a permission check using
hasPermissionAsync(userId, ...)orhasAllPermissionAsync(userId, ...)while unauthenticated (whereuserIdisnullorundefined). - The helper function
toSubjectwill fail to handle thenullorundefinedvalue, throwing aTypeError: Cannot read properties of null (reading '_id'). - This unhandled exception crashes the execution context of the request, returning a 500 Internal Server Error instead of gracefully returning
false.
Fix with AI
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.
There was a problem hiding this comment.
!fp there is no away to have undefined at that point
ggazzo
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
3 participants
Why
Follow-up to the permission-check migration (#41346, #41367) — the systemic part of the P2 review finding. The
hasPermissionAsyncfamily accepts a full user object so callers can skip an internalUsers.findOneById, but only_idandrolesare ever consumed by the check. Passing the wholeIUsermeans credential-bearing fields (services,e2e.private_key, custom fields, settings…) are serialized to the authorization service whenever it runs out of process (e.g.ee/apps/authorization-service).Change
server/lib/authorization/hasPermission.ts: normalize object inputs to a minimal{ _id, roles }subject before callingAuthorization.*. String ids pass through unchanged. Behavior-preserving — the check already reads only_id/roles.Effect
Direct
Authorization.hasPermissioncallers outside these wrappers (already minimal: FederationMatrix passes{ _id, roles }; room/service and canAccessRoom pass an id) are unaffected.Task: ARCH-2260
Summary by CodeRabbit