Skip to content

fix: sanitizeUrl incorrectly rewrites internal navigation links#39706

Open
vmridul wants to merge 3 commits into
RocketChat:developfrom
vmridul:fix/sanitizeURLIncorrectInternalLinks
Open

fix: sanitizeUrl incorrectly rewrites internal navigation links#39706
vmridul wants to merge 3 commits into
RocketChat:developfrom
vmridul:fix/sanitizeURLIncorrectInternalLinks

Conversation

@vmridul
Copy link
Copy Markdown

@vmridul vmridul commented Mar 18, 2026

Problem

sanitizeUrl() was forcing all protocol less URLs into protocol relative form (//...), which broke internal links like /channel/general, ./docs, #section, ?tab=files. This caused rendered markdown links in messages to navigate incorrectly or fail.

Before

When a user posted a markdown link to an internal page or anchor:

  • /channel/general was rewritten as ///channel/general (browser treats this as a network request instead of a route)
  • #section was rewritten as //#section
  • ?tab=files was rewritten as //?tab=files

Result: clicking internal links in chat messages would either navigate to wrong page or stay on current page.

After

Internal link patterns are now recognized and passed through unchanged:

  • /channel/general stays /channel/general
  • #section stays #section
  • ?tab=files stays ?tab=files

All other link types continue to work normally (external domains, XSS blocking, etc.)

Solution

Added a check to preserve URLs starting with /, ./, ../, #, or ? before the existing domain-rewriting logic.

Changes

  • packages/gazzodown/src/elements/sanitizeUrl.ts: Added check to preserve internal/local URLs
  • packages/gazzodown/src/elements/sanitizeUrl.spec.ts: Added 3 test cases covering relative paths, hash fragments, and query only links

Summary by CodeRabbit

  • Bug Fixes

    • Fixed an issue where internal navigation links were being incorrectly rewritten. Relative paths, hash fragments, and query parameters are now preserved and function correctly.
  • Tests

    • Added comprehensive test coverage to ensure relative paths, hash fragments, and query-only links are properly handled.

@dionisio-bot
Copy link
Copy Markdown
Contributor

dionisio-bot Bot commented Mar 18, 2026

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label
  • This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Mar 18, 2026

🦋 Changeset detected

Latest commit: 0da95e2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 41 packages
Name Type
@rocket.chat/gazzodown Patch
@rocket.chat/meteor Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/livechat Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/ui-voip Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/api-client Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/ddp-client Patch
@rocket.chat/http-router Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/abac Patch
@rocket.chat/federation-matrix Patch
@rocket.chat/license Patch
@rocket.chat/media-calls Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/models Patch
@rocket.chat/network-broker Patch
@rocket.chat/omni-core-ee Patch
@rocket.chat/mock-providers Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/instance-status Patch
@rocket.chat/omni-core Patch
@rocket.chat/server-fetch Patch

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
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 18, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: cc57bfba-10b5-4f6a-bd1b-f10368181523

📥 Commits

Reviewing files that changed from the base of the PR and between 6f76232 and 0da95e2.

📒 Files selected for processing (3)
  • .changeset/stale-sheep-join.md
  • packages/gazzodown/src/elements/sanitizeUrl.spec.ts
  • packages/gazzodown/src/elements/sanitizeUrl.ts
📜 Recent review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:

  • packages/gazzodown/src/elements/sanitizeUrl.spec.ts
  • packages/gazzodown/src/elements/sanitizeUrl.ts
**/*.spec.ts

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use .spec.ts extension for test files (e.g., login.spec.ts)

Files:

  • packages/gazzodown/src/elements/sanitizeUrl.spec.ts
🧠 Learnings (14)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/elements/Timestamp/RelativeTime.spec.tsx:63-70
Timestamp: 2026-03-06T18:02:20.381Z
Learning: In RocketChat/Rocket.Chat, tests in the `packages/gazzodown` package (and likely the broader test suite) are always expected to run in the UTC timezone. This makes `toLocaleString()` output deterministic, so snapshot tests that include locale/timezone-sensitive content (such as `title` attributes from `toLocaleString()`) are stable and do not need to be replaced with targeted assertions.
📚 Learning: 2026-03-16T21:50:37.589Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: For changes related to OpenAPI migrations in Rocket.Chat/OpenAPI, when removing endpoint types and validators from rocket.chat/rest-typings (e.g., UserRegisterParamsPOST, /v1/users.register) document this as a minor changeset (not breaking) per RocketChat/Rocket.Chat-Open-API#150 Rule 7. Note that the endpoint type is re-exposed via a module augmentation .d.ts in the consuming package (e.g., packages/web-ui-registration/src/users-register.d.ts). In reviews, ensure the changeset clearly states: this is a non-breaking change, the major version should not be bumped, and the changeset reflects a minor version bump. Do not treat this as a breaking change during OpenAPI migrations.

Applied to files:

  • .changeset/stale-sheep-join.md
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file

Applied to files:

  • packages/gazzodown/src/elements/sanitizeUrl.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files

Applied to files:

  • packages/gazzodown/src/elements/sanitizeUrl.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior in Playwright tests

Applied to files:

  • packages/gazzodown/src/elements/sanitizeUrl.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts

Applied to files:

  • packages/gazzodown/src/elements/sanitizeUrl.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests

Applied to files:

  • packages/gazzodown/src/elements/sanitizeUrl.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory

Applied to files:

  • packages/gazzodown/src/elements/sanitizeUrl.spec.ts
📚 Learning: 2025-12-10T21:00:43.645Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:43.645Z
Learning: Adopt the monorepo-wide Jest testMatch pattern: <rootDir>/src/**/*.spec.{ts,js,mjs} (represented here as '**/src/**/*.spec.{ts,js,mjs}') to ensure spec files under any package's src directory are picked up consistently across all packages in the Rocket.Chat monorepo. Apply this pattern in jest.config.ts for all relevant packages to maintain uniform test discovery.

Applied to files:

  • packages/gazzodown/src/elements/sanitizeUrl.spec.ts
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.

Applied to files:

  • packages/gazzodown/src/elements/sanitizeUrl.spec.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:

  • packages/gazzodown/src/elements/sanitizeUrl.spec.ts
  • packages/gazzodown/src/elements/sanitizeUrl.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:

  • packages/gazzodown/src/elements/sanitizeUrl.spec.ts
  • packages/gazzodown/src/elements/sanitizeUrl.ts
📚 Learning: 2026-03-06T18:02:11.732Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/elements/Timestamp/RelativeTime.spec.tsx:63-70
Timestamp: 2026-03-06T18:02:11.732Z
Learning: In Rocket.Chat, tests within the gazzodown package (and the broader test suite) should run in UTC to make locale/timezone-sensitive outputs from toLocaleString() deterministic. Ensure snapshot tests that capture values like title attributes based on toLocaleString() remain stable by running tests in UTC. Actions you can take: - configure the test environment to TZ=UTC (e.g., in CI or local npm/yarn test scripts), - optionally set process.env.TZ = 'UTC' in Jest/setupFiles or the test bootstrap, - avoid asserting locale-sensitive strings directly; prefer targeted assertions or mocks where necessary. Apply this guideline to all spec files under packages/gazzodown/src that involve timestamp rendering (pattern above).

Applied to files:

  • packages/gazzodown/src/elements/sanitizeUrl.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.

Applied to files:

  • packages/gazzodown/src/elements/sanitizeUrl.spec.ts
🔇 Additional comments (5)
.changeset/stale-sheep-join.md (1)

1-5: Changeset metadata and release note are correctly scoped.

The patch bump and summary line match the fix impact for @rocket.chat/gazzodown.

packages/gazzodown/src/elements/sanitizeUrl.ts (3)

6-10: Whitespace normalization and empty-input guard are solid.

Trimming first and short-circuiting blank values closes the whitespace-only edge case cleanly.


13-19: Protocol parsing now consistently uses sanitized input.

Using the trimmed value for protocol detection/parsing keeps behavior consistent and preserves the dangerous-protocol blocklist path.


21-31: Internal/local link preservation fix is correctly implemented.

The new prefix checks (/, ./, ../, #, ?) address the navigation regression while retaining fallback handling for non-protocol external-like inputs.

packages/gazzodown/src/elements/sanitizeUrl.spec.ts (1)

110-122: Targeted regression coverage looks good.

These tests precisely cover the internal-link cases that previously regressed (/, ./, ../, #, ?) and should prevent recurrence.

As per coding guidelines, “Use descriptive test names that clearly communicate expected behavior in Playwright tests,” and these test titles meet that clarity requirement.


Walkthrough

A patch release for @rocket.chat/gazzodown fixes a bug where sanitizeUrl incorrectly rewrites internal navigation links. The function is updated to preserve relative paths, hash fragments, and query-only links, with accompanying test coverage.

Changes

Cohort / File(s) Summary
Release metadata
.changeset/stale-sheep-join.md
Changeset file marking a patch version bump for @rocket.chat/gazzodown, documenting the sanitizeUrl bug fix.
URL sanitization logic and tests
packages/gazzodown/src/elements/sanitizeUrl.ts, packages/gazzodown/src/elements/sanitizeUrl.spec.ts
Updated sanitizeUrl function to preserve internal navigation patterns (relative paths, hash fragments, query strings) while maintaining security checks for dangerous protocols. New tests verify correct handling of /channel/general, ./docs/getting-started, ../help, #section, and ?tab=files.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested labels

type: bug

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and clearly describes the main bug fix: preventing sanitizeUrl from incorrectly rewriting internal navigation links, which is the core purpose of this changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

CodeRabbit can approve the review once all CodeRabbit's comments are resolved.

Enable the reviews.request_changes_workflow setting to automatically approve the review once all CodeRabbit's comments are resolved.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

No issues found across 3 files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant