fix: add validation for SAML SLO redirect URLs#38994
Merged
julio-rocketchat merged 9 commits intodevelopfrom Mar 26, 2026
Merged
fix: add validation for SAML SLO redirect URLs#38994julio-rocketchat merged 9 commits intodevelopfrom
julio-rocketchat merged 9 commits intodevelopfrom
Conversation
Contributor
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 6b923a5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 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 |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSAML SLO redirect handling now requires the SAML service, validates the configured IdP SLO URL and the user-provided Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as RocketChatServer
participant Config as SAML_Service_Config
Client->>Server: GET /_saml/sloRedirect/:provider?redirect=...
Server->>Config: load service config for provider
Server->>Server: ensure IdP SLO redirect configured
Server->>Server: ensure `redirect` param present
Server->>Server: parse configured URL and requested URL
Server->>Server: compare origins and normalized pathnames
alt validation succeeds
Server->>Client: 302 Location: validated request URL
else validation fails
Server->>Client: 4xx/5xx error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts`:
- Around line 428-432: The strict equality check between configuredURL.pathname
and requestURL.pathname fails when paths differ only by a trailing slash; update
the comparison in the SAML logout flow (the block using configuredURL.pathname
and requestURL.pathname, e.g., where idpSLORedirectURL is validated) to
normalize both pathnames before comparing (strip a single trailing slash from
each but preserve "/" for root) so that "/slo" and "/slo/" are treated as equal;
implement the normalization once (helper or inline) and use the normalized
values for the 403 check and any subsequent logic that relies on the pathname
match.
- Around line 422-435: The redirect currently forwards the full user-supplied
requestURL (including query string and fragment) to the IdP, allowing
attacker-controlled SAML parameters to be injected; change the redirect to use
the server-configured idpSLORedirectURL (or construct the redirect URL from
configuredURL plus only an allowlisted set of SAML parameters) instead of
requestURL.toString(), ensuring you strip any incoming query/fragment or only
append validated keys (e.g., only RelayState/SAMLRequest if explicitly required)
before calling res.writeHead(..., { Location: ... }) in SAML.ts where
configuredURL and requestURL are compared.
- Line 396: req.query.redirect is asserted with "as string" which gives no
runtime protection against arrays; update the logic around userRedirect (the
variable assigned from req.query.redirect in SAML.ts) to perform a runtime type
guard: check if req.query.redirect is an array (Array.isArray) and either reject
(return 400) or safely select a single element (e.g., first) only after
validating it is a string, then proceed to construct new URL(userRedirect).
Ensure any non-string or unexpected types are handled by the same 400 error path
used for invalid URLs so the redirect validation remains safe.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
📜 Review details
⏰ 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). (1)
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
🔇 Additional comments (1)
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts (1)
77-77: LGTM — service context correctly threaded toprocessSLORedirectAction.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38994 +/- ##
===========================================
- Coverage 70.54% 70.49% -0.06%
===========================================
Files 3243 3243
Lines 115243 115243
Branches 20979 20960 -19
===========================================
- Hits 81301 81236 -65
- Misses 31891 31948 +57
- Partials 2051 2059 +8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts (2)
435-436:⚠️ Potential issue | 🟠 MajorVerify whether forwarding full user query to IdP is intentional.
After origin/path checks, Line 436 still redirects with
requestURL.toString(), which preserves attacker-controlled query params if they match the configured endpoint. Please confirm this is expected for your SLO flow and threat model (especially for redirect-chain concerns).#!/bin/bash set -euo pipefail # Find all producers/consumers of SLO redirect URLs and redirect query construction. rg -nP --type=ts -C4 '_saml/sloRedirect|sloRedirect|idpSLORedirectURL|query\.redirect|redirect='Expected result: identify whether
redirectis always server-generated/trusted vs attacker-controlled input from clients.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts` around lines 435 - 436, The redirect currently uses requestURL.toString() in the res.writeHead(302, { Location: ... }) call which can forward attacker-controlled query parameters; update the SLO redirect logic (around requestURL usage in SAML.ts) to only include server-trusted components: either reconstruct the Location from a configured idpSLORedirectURL/idp endpoint plus a server-generated query (e.g., session or SAML params), or whitelist/strip incoming query keys before appending them; ensure the code path that builds the redirect (where requestURL is created) validates ownership of any redirect/redirect-like parameter and never directly reflects untrusted client query params to the IdP redirect.
396-407:⚠️ Potential issue | 🟡 MinorAdd a runtime type guard for
redirectbefore parsing.Line 396 uses
as string, which is compile-time only. In this security path, reject non-string/array forms explicitly beforenew URL(...).🛡️ Proposed fix
- const userRedirect = req.query.redirect as string; + const rawRedirect = req.query.redirect; + const userRedirect = Array.isArray(rawRedirect) ? rawRedirect[0] : rawRedirect; @@ - if (!userRedirect) { + if (typeof userRedirect !== 'string' || userRedirect.length === 0) { res.writeHead(400); res.end('Missing redirect parameter'); return; }#!/bin/bash set -euo pipefail # Verify how IIncomingMessage types query values and whether non-string query forms are possible. fd 'IIncomingMessage.ts' --type f FILE="$(fd 'IIncomingMessage.ts' --type f | head -n1)" sed -n '1,220p' "$FILE" # Inspect nearby SAML usage of req.query to confirm runtime assumptions. rg -nP --type=ts -C3 '\breq\.query\.[A-Za-z_]+' apps/meteor/app/meteor-accounts-saml/server/lib/SAML.tsExpected result: confirm
req.querycan hold non-string forms at runtime; if so, keep explicit guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts` around lines 396 - 407, The code currently assumes req.query.redirect is a string by using "as string" for userRedirect before calling new URL; add a runtime type guard that checks typeof req.query.redirect === 'string' (and optionally that it's not empty) and reject other forms (arrays/objects/undefined) by returning a 400 with 'Missing or invalid redirect parameter' before constructing new URL; update the logic around the userRedirect/local variable (userRedirect, req.query.redirect) and keep the existing idpSLORedirectURL check intact so new URL(...) only runs on a validated string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts`:
- Around line 435-436: The redirect currently uses requestURL.toString() in the
res.writeHead(302, { Location: ... }) call which can forward attacker-controlled
query parameters; update the SLO redirect logic (around requestURL usage in
SAML.ts) to only include server-trusted components: either reconstruct the
Location from a configured idpSLORedirectURL/idp endpoint plus a
server-generated query (e.g., session or SAML params), or whitelist/strip
incoming query keys before appending them; ensure the code path that builds the
redirect (where requestURL is created) validates ownership of any
redirect/redirect-like parameter and never directly reflects untrusted client
query params to the IdP redirect.
- Around line 396-407: The code currently assumes req.query.redirect is a string
by using "as string" for userRedirect before calling new URL; add a runtime type
guard that checks typeof req.query.redirect === 'string' (and optionally that
it's not empty) and reject other forms (arrays/objects/undefined) by returning a
400 with 'Missing or invalid redirect parameter' before constructing new URL;
update the logic around the userRedirect/local variable (userRedirect,
req.query.redirect) and keep the existing idpSLORedirectURL check intact so new
URL(...) only runs on a validated string.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
📜 Review details
⏰ 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). (10)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
🧠 Learnings (11)
📓 Common learnings
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 Learning: 2026-01-26T18:26:01.279Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38227
File: apps/meteor/app/api/server/router.ts:44-49
Timestamp: 2026-01-26T18:26:01.279Z
Learning: In apps/meteor/app/api/server/router.ts, when retrieving bodyParams and queryParams from the Hono context via c.get(), do not add defensive defaults (e.g., ?? {}). The code should fail fast if these parameters are missing, as endpoint handlers expect them to be present and breaking here helps surface parsing problems rather than hiding them.
Applied to files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
📚 Learning: 2026-01-16T22:56:30.299Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 38224
File: apps/meteor/client/views/admin/viewLogs/AnalyticsReports.tsx:15-20
Timestamp: 2026-01-16T22:56:30.299Z
Learning: For the logs documentation link in apps/meteor/client/lib/links.ts, the correct URL slug is `/i/logs-docs` (plural), not `/i/logs-doc` (singular).
Applied to files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
📚 Learning: 2025-09-15T13:10:30.049Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 36868
File: packages/http-router/src/Router.ts:416-425
Timestamp: 2025-09-15T13:10:30.049Z
Learning: In packages/http-router/src/Router.ts, the dispatch() method's use of replaceAll('//', '/') on the full path is acceptable because URL normalization and query string handling is performed by the caller function before dispatch() is invoked.
Applied to files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
📚 Learning: 2026-02-23T17:53:18.785Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Applied to files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
🔇 Additional comments (2)
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts (2)
77-77: Service context is correctly threaded into SLO redirect handling.Passing
serviceon Line 77 is the right wiring to enable provider-aware URL validation inprocessSLORedirectAction.
428-430: Path normalization for trailing slashes is a good hardening detail.Treating
/sloand/slo/as equivalent avoids false 403s for valid IdP endpoint variants.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts">
<violation number="1" location="apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts:396">
P2: Unsafe `as string` cast on `req.query.redirect`. Query parameters can be `string | string[] | undefined`. Validate the type explicitly instead of casting, so that array values are properly rejected before reaching URL parsing.
(Based on your team's feedback about avoiding unsafe type casts and validating values first.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
pierre-lehnen-rc
requested changes
Mar 2, 2026
Co-authored-by: Pierre Lehnen <55164754+pierre-lehnen-rc@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts`:
- Around line 413-419: The catch block currently treats failures parsing both
idpSLORedirectURL and userRedirect the same; split the parsing into two distinct
attempts so a failure parsing configuredURL (new URL(idpSLORedirectURL)) returns
a 500 (server configuration error) while a failure parsing requestURL (new
URL(userRedirect)) returns a 400 (bad user input). Specifically, replace the
single try/catch with one try/catch around new URL(idpSLORedirectURL) that calls
res.writeHead(500) and res.end('Invalid server SLO redirect URL') on error, and
a separate try/catch around new URL(userRedirect) that calls res.writeHead(400)
and res.end('Invalid URL format') on error; reference the variables
configuredURL, idpSLORedirectURL, requestURL and userRedirect to locate the code
to change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
📜 Review details
⏰ 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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
🧠 Learnings (11)
📓 Common learnings
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 Learning: 2026-01-26T18:26:01.279Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38227
File: apps/meteor/app/api/server/router.ts:44-49
Timestamp: 2026-01-26T18:26:01.279Z
Learning: In apps/meteor/app/api/server/router.ts, when retrieving bodyParams and queryParams from the Hono context via c.get(), do not add defensive defaults (e.g., ?? {}). The code should fail fast if these parameters are missing, as endpoint handlers expect them to be present and breaking here helps surface parsing problems rather than hiding them.
Applied to files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
📚 Learning: 2026-01-16T22:56:30.299Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 38224
File: apps/meteor/client/views/admin/viewLogs/AnalyticsReports.tsx:15-20
Timestamp: 2026-01-16T22:56:30.299Z
Learning: For the logs documentation link in apps/meteor/client/lib/links.ts, the correct URL slug is `/i/logs-docs` (plural), not `/i/logs-doc` (singular).
Applied to files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
📚 Learning: 2025-09-15T13:10:30.049Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 36868
File: packages/http-router/src/Router.ts:416-425
Timestamp: 2025-09-15T13:10:30.049Z
Learning: In packages/http-router/src/Router.ts, the dispatch() method's use of replaceAll('//', '/') on the full path is acceptable because URL normalization and query string handling is performed by the caller function before dispatch() is invoked.
Applied to files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
📚 Learning: 2026-02-23T17:53:18.785Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Applied to files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts
🔇 Additional comments (2)
apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts (2)
77-77: Correctly wiringserviceinto SLO redirect handling.Line 77 now passes provider config into
processSLORedirectAction, enabling the server-side IdP URL validation flow.
422-433: Origin + normalized path enforcement is solid.Line 422-Line 433 closes the open-redirect vector by requiring same origin and path (with trailing-slash normalization), while still allowing legitimate IdP redirects.
pierre-lehnen-rc
previously approved these changes
Mar 5, 2026
Contributor
pierre-lehnen-rc
left a comment
pierre-lehnen-rc
left a comment
There was a problem hiding this comment.
Not sure but maybe we should redirect to home in case of error, as the client is not expecting this redirect to ever fail and won't be able to recover from it (though there should also not be any errors on normal usage)
KevLehman
previously approved these changes
Mar 5, 2026
julio-rocketchat
dismissed stale reviews from KevLehman and pierre-lehnen-rc
via
3346067
julio-rocketchat
approved these changes
Mar 19, 2026
julio-rocketchat
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
KevLehman
approved these changes
Mar 20, 2026
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
The SAML Single Logout (SLO) redirect functionality was vulnerable to open redirect attacks. Users could be redirected to arbitrary external URLs by manipulating the redirect query parameter, potentially leading to phishing attacks or credential theft.
This happened because the processSLORedirectAction function directly used the user-supplied redirect parameter without validation, allowing attackers to craft malicious URLs like:
/_saml/sloRedirect/saml-test/?redirect=https://malicious-site.comThis PR:
Issue(s)
VLN-153, VLN-165
Steps to test or reproduce
N/A
Further comments
N/A
Summary by CodeRabbit
Bug Fixes
Chores