fix: isRelativeURL now correctly handles scheme-based and simple rela…#40742
fix: isRelativeURL now correctly handles scheme-based and simple rela…#40742shashankk-singh wants to merge 1 commit into
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 |
|
WalkthroughThe ChangesRelative URL Classification
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/tests/unit/lib/utils/isRelativeURL.spec.ts (1)
6-14: ⚡ Quick winAdd edge-case assertions to lock the new contract.
Please extend
testCaseswith at least['/test', true](from the issue objective) and['//example.com', false]to prevent regressions around path-absolute vs network-path inputs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/tests/unit/lib/utils/isRelativeURL.spec.ts` around lines 6 - 14, Extend the testCases const in isRelativeURL.spec.ts by adding the two edge-case entries [' /test', true] (without the leading space) and ['//example.com', false] to ensure path-absolute inputs are treated as relative while network-paths are treated as absolute; update the testCases array (symbol: testCases) to include these entries alongside the existing cases so the isRelativeURL tests cover both edge conditions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/lib/utils/isRelativeURL.ts`:
- Line 1: The isRelativeURL function is too permissive: trim the input first,
explicitly reject network-path references that start with '//' and then test for
a URL scheme anchored at the beginning (e.g., /^[a-zA-Z][a-zA-Z0-9+.-]*:/) so
inputs like '//evil.com' and ' javascript:alert(1)' are treated as
non-relative; update apps/meteor/lib/utils/isRelativeURL.ts to trim the string,
return false if it begins with '//' after trim, and then run the anchored scheme
regex (or use a safe URL parse where appropriate), and adjust
apps/meteor/tests/unit/lib/utils/isRelativeURL.spec.ts to expect '/' => false
and add tests for '//evil.com' and whitespace-prefixed 'javascript:' inputs to
assert they are not relative.
---
Nitpick comments:
In `@apps/meteor/tests/unit/lib/utils/isRelativeURL.spec.ts`:
- Around line 6-14: Extend the testCases const in isRelativeURL.spec.ts by
adding the two edge-case entries [' /test', true] (without the leading space)
and ['//example.com', false] to ensure path-absolute inputs are treated as
relative while network-paths are treated as absolute; update the testCases array
(symbol: testCases) to include these entries alongside the existing cases so the
isRelativeURL tests cover both edge conditions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1a2fc6b-faed-4c05-840b-d27b7cdebf24
📒 Files selected for processing (2)
apps/meteor/lib/utils/isRelativeURL.tsapps/meteor/tests/unit/lib/utils/isRelativeURL.spec.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: cubic · AI code reviewer
🧰 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:
apps/meteor/lib/utils/isRelativeURL.tsapps/meteor/tests/unit/lib/utils/isRelativeURL.spec.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.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/tests/unit/lib/utils/isRelativeURL.spec.ts
🧠 Learnings (5)
📚 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/lib/utils/isRelativeURL.tsapps/meteor/tests/unit/lib/utils/isRelativeURL.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 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/lib/utils/isRelativeURL.tsapps/meteor/tests/unit/lib/utils/isRelativeURL.spec.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/lib/utils/isRelativeURL.tsapps/meteor/tests/unit/lib/utils/isRelativeURL.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:
apps/meteor/tests/unit/lib/utils/isRelativeURL.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:
apps/meteor/tests/unit/lib/utils/isRelativeURL.spec.ts
| @@ -1 +1 @@ | |||
| export const isRelativeURL = (str: string): boolean => /^[^\/]+\/[^\/].*$|^\/[^\/].*$/gim.test(str); | |||
| export const isRelativeURL = (str: string): boolean => !/^[a-zA-Z][a-zA-Z0-9+.-]*:/.test(str); No newline at end of file | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current behavior and test coverage gaps around risky inputs.
echo "== Current implementation =="
rg -n "export const isRelativeURL" apps/meteor/lib/utils/isRelativeURL.ts
echo
echo "== Existing isRelativeURL test cases =="
rg -n "testCases|\\['" apps/meteor/tests/unit/lib/utils/isRelativeURL.spec.ts
echo
echo "== Evaluate current regex behavior on edge cases =="
python - <<'PY'
import re
pat = re.compile(r'^[a-zA-Z][a-zA-Z0-9+.-]*:')
samples = [
"/", "/test", "//evil.com", "test", ".", "./test", "../test",
"data:image/gif;base64,AAA", "javascript:alert(1)", " javascript:alert(1)", "\tjavascript:alert(1)"
]
for s in samples:
print(f"{s!r:35} -> isRelativeURL={not bool(pat.search(s))}")
PYRepository: RocketChat/Rocket.Chat
Length of output: 1239
Harden isRelativeURL regex—current logic is too permissive for href security gates.
apps/meteor/lib/utils/isRelativeURL.ts uses !/^[a-zA-Z][a-zA-Z0-9+.-]*:/.test(str), which treats //evil.com as relative (true) and also treats whitespace-prefixed scheme payloads like ' javascript:alert(1)' / '\tjavascript:...' as relative because the scheme isn’t at the start.
Additionally, apps/meteor/tests/unit/lib/utils/isRelativeURL.spec.ts expects ['/', false], but the current regex evaluates '/' as true, so the test expectations are inconsistent with the implementation.
Add normalization/trim (and ideally a URL/URL-safe parse) before scheme checks, explicitly reject network-path refs (//...), and extend unit tests for //evil.com and whitespace-prefixed javascript: inputs (and reconcile the '/' expectation).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/meteor/lib/utils/isRelativeURL.ts` at line 1, The isRelativeURL function
is too permissive: trim the input first, explicitly reject network-path
references that start with '//' and then test for a URL scheme anchored at the
beginning (e.g., /^[a-zA-Z][a-zA-Z0-9+.-]*:/) so inputs like '//evil.com' and '
javascript:alert(1)' are treated as non-relative; update
apps/meteor/lib/utils/isRelativeURL.ts to trim the string, return false if it
begins with '//' after trim, and then run the anchored scheme regex (or use a
safe URL parse where appropriate), and adjust
apps/meteor/tests/unit/lib/utils/isRelativeURL.spec.ts to expect '/' => false
and add tests for '//evil.com' and whitespace-prefixed 'javascript:' inputs to
assert they are not relative.
There was a problem hiding this comment.
2 issues found across 2 files
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/tests/unit/lib/utils/isRelativeURL.spec.ts">
<violation number="1" location="apps/meteor/tests/unit/lib/utils/isRelativeURL.spec.ts:8">
P2: The test expects `isRelativeURL('/')` to return `false`, but the new implementation would return `true` since `/` does not start with a scheme pattern. This test will fail. Either update the expected value to `true` or adjust the implementation to treat absolute-path references (`/...`) as non-relative.</violation>
</file>
<file name="apps/meteor/lib/utils/isRelativeURL.ts">
<violation number="1" location="apps/meteor/lib/utils/isRelativeURL.ts:1">
P1: This regex is too permissive for security-sensitive URL validation. It treats protocol-relative URLs (`//evil.com`) as relative and allows whitespace-prefixed schemes (`' javascript:alert(1)'`, `'\tjavascript:...'`) to bypass detection since the `^` anchor requires the scheme at position 0. Both are well-known XSS/open-redirect vectors. Consider trimming leading whitespace before the check and explicitly rejecting network-path references (`//...`).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| @@ -1 +1 @@ | |||
| export const isRelativeURL = (str: string): boolean => /^[^\/]+\/[^\/].*$|^\/[^\/].*$/gim.test(str); | |||
| export const isRelativeURL = (str: string): boolean => !/^[a-zA-Z][a-zA-Z0-9+.-]*:/.test(str); No newline at end of file | |||
There was a problem hiding this comment.
P1: This regex is too permissive for security-sensitive URL validation. It treats protocol-relative URLs (//evil.com) as relative and allows whitespace-prefixed schemes (' javascript:alert(1)', '\tjavascript:...') to bypass detection since the ^ anchor requires the scheme at position 0. Both are well-known XSS/open-redirect vectors. Consider trimming leading whitespace before the check and explicitly rejecting network-path references (//...).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/lib/utils/isRelativeURL.ts, line 1:
<comment>This regex is too permissive for security-sensitive URL validation. It treats protocol-relative URLs (`//evil.com`) as relative and allows whitespace-prefixed schemes (`' javascript:alert(1)'`, `'\tjavascript:...'`) to bypass detection since the `^` anchor requires the scheme at position 0. Both are well-known XSS/open-redirect vectors. Consider trimming leading whitespace before the check and explicitly rejecting network-path references (`//...`).</comment>
<file context>
@@ -1 +1 @@
-export const isRelativeURL = (str: string): boolean => /^[^\/]+\/[^\/].*$|^\/[^\/].*$/gim.test(str);
+export const isRelativeURL = (str: string): boolean => !/^[a-zA-Z][a-zA-Z0-9+.-]*:/.test(str);
\ No newline at end of file
</file context>
| @@ -5,12 +5,12 @@ import { isRelativeURL } from '../../../../lib/utils/isRelativeURL'; | |||
| describe('isRelativeURL', () => { | |||
There was a problem hiding this comment.
P2: The test expects isRelativeURL('/') to return false, but the new implementation would return true since / does not start with a scheme pattern. This test will fail. Either update the expected value to true or adjust the implementation to treat absolute-path references (/...) as non-relative.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/tests/unit/lib/utils/isRelativeURL.spec.ts, line 8:
<comment>The test expects `isRelativeURL('/')` to return `false`, but the new implementation would return `true` since `/` does not start with a scheme pattern. This test will fail. Either update the expected value to `true` or adjust the implementation to treat absolute-path references (`/...`) as non-relative.</comment>
<file context>
@@ -5,12 +5,12 @@ import { isRelativeURL } from '../../../../lib/utils/isRelativeURL';
const testCases = [
['/', false],
- ['test', false], // TODO: should be true?
+ ['test', true],
['test/test', true],
- ['.', false], // TODO: should be true?
</file context>
…tive URLs
Proposed changes
The current isRelativeURL implementation incorrectly classifies some URLs:
Fixed by replacing the regex with a simpler approach: detect scheme-based URLs
(anything matching word: at the start) and return false for those, true for everything else.
Issue(s)
Closes #40313
Steps to test or reproduce
Run the unit tests for isRelativeURL — all TODOs in the test file are now resolved.
Further comments
Summary by CodeRabbit