fix: imported fixes 09-18-2025 #36982
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 12e25c3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.changeset/grumpy-berries-arrive.md (1)
5-5: Fix MD034 (bare URL) and tighten the release note.Wrap the URL to appease markdownlint and make the note a bit clearer for release consumers.
-Security Hotfix (https://docs.rocket.chat/docs/security-fixes-and-updates) +Security hotfix — see [security fixes and updates](https://docs.rocket.chat/docs/security-fixes-and-updates).apps/meteor/app/api/server/ApiClass.ts (1)
968-972: Avoid treating a missing X-Auth-Token as the literal string "undefined".
String(undefined)is truthy and triggers an unnecessary DB lookup with a bogus hashed token. Use a guarded read.- const userToken = String(headers['x-auth-token']); - - if (userId && userToken) { + const rawToken = headers['x-auth-token']; + const userToken = rawToken ? String(rawToken) : undefined; + + if (userId && userToken) {apps/meteor/tests/end-to-end/api/users.ts (1)
684-729: Tests correctly assert 401 for unauthenticated /users.info (with and without Anonymous Read).Good coverage for “no creds” and “X-User-Id only” cases.
Consider adding one more case: “X-Auth-Token only” (no X-User-Id) should also 401, to fully bracket header permutations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.changeset/grumpy-berries-arrive.md(1 hunks)apps/meteor/app/api/server/ApiClass.ts(1 hunks)apps/meteor/tests/end-to-end/api/users.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
apps/meteor/tests/end-to-end/api/users.ts
🧬 Code graph analysis (1)
apps/meteor/tests/end-to-end/api/users.ts (1)
apps/meteor/tests/data/api-data.ts (1)
credentials(39-42)
🪛 markdownlint-cli2 (0.17.2)
.changeset/grumpy-berries-arrive.md
5-5: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
apps/meteor/app/api/server/ApiClass.ts (2)
816-816: Deriving userId from the authenticated user is correct.This removes header spoofing risk and aligns downstream permission checks with a verified identity. LGTM.
821-825: Early 401 gate reads clean; behavior matches intent.Short‑circuiting before rate‑limit/validation is correct — use authOrAnonRequired: true for anon‑OK endpoints and authRequired: true for auth‑only endpoints. Ripgrep results show X-User-Id only in tests/fixtures (apps/meteor/tests/**); no production routes read the x-user-id header directly.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36982 +/- ##
===========================================
- Coverage 66.21% 66.18% -0.03%
===========================================
Files 3384 3384
Lines 115027 115027
Branches 21064 21066 +2
===========================================
- Hits 76161 76136 -25
- Misses 36260 36283 +23
- Partials 2606 2608 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
julio-rocketchat
changed the title
WalkthroughSwitches API identity source from the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router as API Router
participant Api as ApiClass
participant Session as Auth/Session
participant Settings as Workspace Settings
Client->>Router: HTTP Request (headers, params)
Router->>Api: Dispatch handler
Api->>Session: check this.user?
Session-->>Api: user or null
Api->>Settings: read Accounts_AllowAnonymousRead
Settings-->>Api: true/false
alt No user AND authRequired
Api-->>Client: 401 Unauthorized
else No user AND authOrAnonRequired AND !Accounts_AllowAnonymousRead
Api-->>Client: 401 Unauthorized
else Proceed
Api-->>Client: continue to endpoint logic / success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.changeset/grumpy-berries-arrive.md (1)
5-5: Fix markdownlint MD034 and clarify the security note.Wrap the bare URL and briefly summarize the behavioral change (session-based auth, 401 vs 400), so release notes are actionable.
-Security Hotfix (https://docs.rocket.chat/docs/security-fixes-and-updates) +### Security + +Harden API authorization by deriving identity from the authenticated session instead of the X-User-Id header. Anonymous-read now returns 401 (Unauthorized) when disabled. This closes a broken access control path affecting admin/sensitive endpoints. + +See: [Security fixes and updates](https://docs.rocket.chat/docs/security-fixes-and-updates)apps/meteor/app/api/server/ApiClass.ts (3)
821-825: Auth gating logic is correct; minor hardening suggestedThe two guards correctly block anonymous access when Anonymous Read is off, and require auth when
authRequiredis set.To avoid any truthiness surprises and to clarify intent, coerce the setting to boolean once and reuse it:
- const shouldPreventAnonymousRead = !this.user && options.authOrAnonRequired && !settings.get('Accounts_AllowAnonymousRead'); + const allowAnonymous = !!settings.get<boolean>('Accounts_AllowAnonymousRead'); + const shouldPreventAnonymousRead = !this.user && options.authOrAnonRequired && !allowAnonymous;
821-825: Defense-in-depth: ignore spoofed X-User-Id and derive identity from tokenToday
authenticatedRoutestill accepts a caller-providedx-user-id(paired with a valid token). To fully eliminate header spoofing as an attack surface, derive the user solely from the token, and if a mismatchingx-user-idis supplied, treat it as unauthorized (and optionally log).Apply this change to
authenticatedRoute:@@ - protected async authenticatedRoute(req: Request): Promise<IUser | null> { - const headers = Object.fromEntries(req.headers.entries()); - - const { 'x-user-id': userId } = headers; - - const userToken = String(headers['x-auth-token']); - - if (userId && userToken) { - return Users.findOne( - { - 'services.resume.loginTokens.hashedToken': Accounts._hashLoginToken(userToken), - '_id': userId, - }, - { - projection: getDefaultUserFields(), - }, - ); - } - return null; - } + protected async authenticatedRoute(req: Request): Promise<IUser | null> { + const rawToken = req.headers.get('x-auth-token') || ''; + if (!rawToken) { + return null; + } + const hashed = Accounts._hashLoginToken(String(rawToken)); + const user = await Users.findOne( + { 'services.resume.loginTokens.hashedToken': hashed }, + { projection: getDefaultUserFields() }, + ); + // Optional: reject mismatched X-User-Id if the client sent one + const hintedId = req.headers.get('x-user-id') || undefined; + if (hintedId && user && user._id !== hintedId) { + // logger.warn(`Mismatched X-User-Id for provided token (hinted=${hintedId}, tokenUser=${user._id})`); + return null; + } + return user ?? null; + }
821-825: Anonymous path can hit non-null assertion on this.tokenWhen Anonymous Read is enabled and the route uses
authOrAnonRequired,this.useris null andauthTokenmay be absent. Later we callAccounts._setAccountData(..., this.token!), which will passundefinedat runtime. Guard it.Here’s a targeted fix (outside the changed hunk):
@@ - Accounts._setAccountData(connection.id, 'loginToken', this.token!); + if (this.token) { + Accounts._setAccountData(connection.id, 'loginToken', this.token); + }Please run an anonymous GET on a route with
authOrAnonRequired: trueand Anonymous Read enabled to confirm no runtime error occurs today.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.changeset/grumpy-berries-arrive.md(1 hunks)apps/meteor/app/api/server/ApiClass.ts(1 hunks)apps/meteor/tests/end-to-end/api/channels.ts(1 hunks)apps/meteor/tests/end-to-end/api/users.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
apps/meteor/tests/end-to-end/api/users.ts
🧬 Code graph analysis (1)
apps/meteor/tests/end-to-end/api/users.ts (1)
apps/meteor/tests/data/api-data.ts (1)
credentials(39-42)
🪛 markdownlint-cli2 (0.17.2)
.changeset/grumpy-berries-arrive.md
5-5: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (4)
apps/meteor/tests/end-to-end/api/users.ts (2)
684-696: Good negative coverage for missing credentials (expects 401).This validates the new auth guard for /users.info. LGTM.
698-729: Add spoof‑resistance e2e and audit server-side X-User-Id usageAdd an e2e that proves a low‑privilege token + spoofed X-User-Id cannot access admin-only endpoints (e.g., GET /api/v1/audit.settings). Audit and patch any server handlers that read x-user-id so they verify the token→user mapping (do not authorize based solely on the header).
Files to audit (found usages): apps/meteor/server/routes/userDataDownload.ts:36, apps/meteor/app/file-upload/server/lib/FileUpload.ts:470, apps/meteor/app/api/server/middlewares/authentication.ts (header handling), apps/meteor/app/api/server/helpers/getLoggedInUser.ts:7, apps/meteor/app/api/server/ApiClass.ts:966.
Example test (add under a new e2e Security describe):
it('should ignore X-User-Id header and deny admin-only endpoint with low-privilege token', async () => { const regular = await createUser(); const regularCreds = await login(regular.username, password); await request .get(api('audit.settings')) .set({ 'X-Auth-Token': regularCreds['X-Auth-Token'], // Spoof header to admin; server must ignore this for authZ 'X-User-Id': credentials['X-User-Id'], }) .expect('Content-Type', 'application/json') .expect(403) .expect((res) => { expect(res.body).to.have.property('success', false); }); await deleteUser(regular); });apps/meteor/tests/end-to-end/api/channels.ts (1)
3587-3591: Approve — 401 for anonymous read is correct. Message assertion unchanged; LGTM. Verification script to find stale tests expecting 400 returned no output; unable to confirm absence of such tests — re-run rg -nP 'channels.anonymousread.expect(\s400\s*)' -- apps/meteor/tests/end-to-end or inspect tests manually.apps/meteor/app/api/server/ApiClass.ts (1)
816-816: Good: userId now derived from authenticated user objectSetting
this.userId = this.user?._idremoves reliance on theX-User-Idheader at call sites. This is the right direction for fixing VLN-151.Run to spot any routes that read
this.userIdwithout settingauthRequiredorauthOrAnonRequired(those handlers would now seeundefinedfor anonymous calls):
|
/patch |
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com> Co-authored-by: Abhinav Kumar <abhinav@avitechlab.com>
|
Pull request #37000 added to Project: "Patch 7.10.1" |
|
/backport 7.9.4 |
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com> Co-authored-by: Abhinav Kumar <abhinav@avitechlab.com>
|
Pull request #37001 added to Project: "Patch 7.9.4" |
|
/backport 7.8.5 |
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com> Co-authored-by: Abhinav Kumar <abhinav@avitechlab.com>
|
Pull request #37002 added to Project: "Patch 7.8.5" |
|
/backport 7.7.9 |
|
Sorry, I couldn't do that backport because of conflicts. Could you please solve them? you can do so by running the following commands: after that just run |
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com> Co-authored-by: Abhinav Kumar <abhinav@avitechlab.com>
|
/backport 7.7.9 |
|
Pull request #37003 added to Project: "Patch 7.7.9" |
|
/backport 7.6.6 |
|
Sorry, I couldn't do that backport because of conflicts. Could you please solve them? you can do so by running the following commands: after that just run |
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com> Co-authored-by: Abhinav Kumar <abhinav@avitechlab.com>
|
/backport 7.6.6 |
|
Pull request #37004 added to Project: "Patch 7.6.6" |
|
/backport 7.5.5 |
|
Sorry, I couldn't do that backport because of conflicts. Could you please solve them? you can do so by running the following commands: after that just run |
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com> Co-authored-by: Abhinav Kumar <abhinav@avitechlab.com>
|
/backport 7.5.5 |
|
Pull request #37005 added to Project: "Patch 7.5.5" |
|
/backport 7.4.6 |
|
Sorry, I couldn't do that backport because of conflicts. Could you please solve them? you can do so by running the following commands: after that just run |
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com> Co-authored-by: Abhinav Kumar <abhinav@avitechlab.com>
|
/backport 7.4.6 |
|
Pull request #37006 added to Project: "Patch 7.4.6" |
3 participants
Proposed changes (including videos or screenshots)
Issue(s)
VLN-151
Steps to test or reproduce
Further comments
Summary by CodeRabbit