fix: correct isRelativeURL classification and tests #40314
Open
namann5 wants to merge 9 commits into
Open
Conversation
… add CIDR range validation helpers
Contributor
|
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 |
|
Contributor
WalkthroughThis pull request addresses URL validation issues and enhances security handling across authentication and network utilities. It fixes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
namann5
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa5c9a13a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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: 4
🧹 Nitpick comments (3)
apps/meteor/tests/unit/lib/utils/isRelativeURL.spec.ts (1)
6-18: Consider extending the test matrix with security‑relevant edge cases.The current matrix matches the stated PR objectives, but doesn't lock down behavior for inputs that browsers normalize differently than this validator. Adding cases such as the following would prevent future regressions and document intended behavior — and will surface the concern raised on
isRelativeURL.ts:const testCases = [ ['/', false], ['', false], ['test', true], ['test/test', true], ['.', true], ['./test', true], ['../test', true], ['/test', true], ['//rocket.chat/path', false], ['https://rocket.chat', false], + ['HTTPS://rocket.chat', false], + ['JaVaScRiPt:alert(1)', false], + [' javascript:alert(1)', false], + ['\tdata:text/html,evil', false], + ['\\\\evil.com\\path', false], + ['/\\evil.com', false], ['data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==', false], ] as const;Some of these will fail against the current implementation — that's the same bypass flagged on
isRelativeURL.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/unit/lib/utils/isRelativeURL.spec.ts` around lines 6 - 18, Extend the test matrix in isRelativeURL.spec.ts to cover browser‑normalization and security edge cases so isRelativeURL behavior is locked down: add cases for inputs like protocol‑like strings and mixed slashes (e.g., "http://example", "https:\\\\example", "mailto:foo", "///evil", "//example", "////example"), whitespace/punycode/percent encodings and control chars (e.g., " /test", "%2Ftest", "\u0000", "\u0009/test"), and unicode/encoded slash tricks (e.g., "%5C%2Ftest") to the testCases const and assert the expected boolean for each using the isRelativeURL function so regressions are caught and intended behavior is documented.apps/meteor/app/authentication/server/lib/restrictLoginAttempts.ts (1)
56-64: Try/catch is now redundant — optional cleanup.Now that
isIpInCidrRangereturnsfalsefor malformed CIDR input instead of throwing, thistry/catchno longer guards a real throw site (and the helper logs nothing internally either). You can either drop it for a leaner expression, or — if you want to keep noisy diagnostics for misconfigured admin entries — log whentrimmedis non-empty but produces neither an exact match nor a CIDR match. Functionally fine as-is.♻️ Possible simplification
- whitelist.some((entry) => { - const trimmed = entry.trim(); - try { - return trimmed === ip || isIpInCidrRange(ip, trimmed); - } catch (error) { - logger.error({ msg: 'Invalid CIDR entry in whitelist', entry: trimmed, error }); - return false; - } - }) + whitelist.some((entry) => { + const trimmed = entry.trim(); + return Boolean(trimmed) && (trimmed === ip || isIpInCidrRange(ip, trimmed)); + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/authentication/server/lib/restrictLoginAttempts.ts` around lines 56 - 64, Remove the redundant try/catch around the whitelist check since isIpInCidrRange no longer throws; inside the whitelist.some callback (where you currently call trimmed === ip || isIpInCidrRange(ip, trimmed)), replace the try/catch block with a simple return trimmed === ip || isIpInCidrRange(ip, trimmed). If you want to keep diagnostics instead, add a conditional that, when trimmed is non-empty and both trimmed === ip and isIpInCidrRange(ip, trimmed) are false, logs via logger.error/warn the malformed or non-matching whitelist entry (but do not wrap isIpInCidrRange in try/catch).packages/server-fetch/src/helpers.ts (1)
53-59: NaN guard is unreachable dead code; consider removing it or failing closed instead of silently zeroing.
net.isIPv6validation at line 28 rejects malformed addresses beforetoBigIntruns, so any invalid hex group (e.g.,'g'from'::g/128') never reaches the reducer. The test atcheckForSsrf.spec.ts:303confirmingisIpInCidrRange('::1', '::g/128')returnsfalsepasses via the validation gate, not the NaN guard.If the guard is kept for future safety, consider returning
0nfromtoBigInt(matching the IPv4 short-circuit at line 45) rather than silently treating a bad group as0, which could quietly produce incorrect membership results instead of surfacing the invariant violation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server-fetch/src/helpers.ts` around lines 53 - 59, The NaN guard inside the reducer in toBigInt is dead/unsafe: because net.isIPv6 validation rejects malformed addresses before toBigInt runs, the isNaN(parsed) branch is unreachable — either remove that branch entirely from the reducer, or (preferable) make it fail-closed by returning 0n for the whole toBigInt call to match the IPv4 short-circuit behavior; locate the reducer in toBigInt (the groups.reduce block that shifts/accumulates BigInt values) and replace the per-group "return value << 16n" on isNaN with logic that aborts and returns 0n from toBigInt (or remove the check if you choose to rely on net.isIPv6 validation).
🤖 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/authentication/server/lib/restrictLoginAttempts.ts`:
- Around line 112-113: Replace the mixed username/email fallback (loginUsername)
with a single canonical identifier: resolve the inbound credential to the
canonical user record (use Users.findOneByEmailAddress when only an email was
submitted, or use login.user when present) and then use that user's _id for all
rate-limit keys (the lookups that currently use loginUsername) and for the
ServerEvents writes (instead of writing the raw email into u.username). Update
the code paths that build the rate-limit keys and the ServerEvents payload to
derive and use user._id (and only include non-PII username if required),
ensuring failed-attempt counters and audit entries always map to the same
physical user.
In `@apps/meteor/lib/utils/isRelativeURL.ts`:
- Line 2: Remove the inline explanatory comments in
apps/meteor/lib/utils/isRelativeURL.ts (the `// Scheme-based URLs are
absolute...` and the other comment at lines referenced) so the implementation
contains no `//` comments; keep the existing function/variable names (e.g.,
isRelativeURL) and logic unchanged—just delete those two comment lines to comply
with the coding guideline against inline implementation comments.
- Around line 1-17: The isRelativeURL function is too permissive: normalize and
validate the input before testing the scheme by trimming ASCII
whitespace/control chars and rejecting inputs containing or starting with
backslashes, and then run the existing scheme regex and protocol-relative check
on the normalized string; specifically, in isRelativeURL() trim the input (e.g.
str = str.trim()), immediately return false if str startsWith('\\') or contains
'\\' that could normalize to '/', and then apply the existing
/^[a-z][a-z\d+\-.]*:/i test and the str.startsWith('//') check against the
trimmed/normalized value; also add unit tests for the reported edge cases ("
javascript:alert(1)", "\tdata:...", "\\evil.com\\path", "/\\evil.com") and
coordinate with the security reviewer.
In `@packages/server-fetch/tests/checkForSsrf.spec.ts`:
- Around line 300-304: The current test doesn't cover the NaN branch in the
toBigInt reducer because isIpInCidrRange short-circuits via
net.isIPv6/unwrapBrackets; either remove the redundant test or add a direct unit
test that exercises toBigInt's NaN guard by exporting (or otherwise exposing)
toBigInt and asserting its behavior when fed malformed numeric segments (so the
reducer path at lines handling NaN is covered). Update exports to expose
toBigInt (or add a test seam) and write a focused test that calls toBigInt with
inputs that force NaN and verifies the guard's result.
---
Nitpick comments:
In `@apps/meteor/app/authentication/server/lib/restrictLoginAttempts.ts`:
- Around line 56-64: Remove the redundant try/catch around the whitelist check
since isIpInCidrRange no longer throws; inside the whitelist.some callback
(where you currently call trimmed === ip || isIpInCidrRange(ip, trimmed)),
replace the try/catch block with a simple return trimmed === ip ||
isIpInCidrRange(ip, trimmed). If you want to keep diagnostics instead, add a
conditional that, when trimmed is non-empty and both trimmed === ip and
isIpInCidrRange(ip, trimmed) are false, logs via logger.error/warn the malformed
or non-matching whitelist entry (but do not wrap isIpInCidrRange in try/catch).
In `@apps/meteor/tests/unit/lib/utils/isRelativeURL.spec.ts`:
- Around line 6-18: Extend the test matrix in isRelativeURL.spec.ts to cover
browser‑normalization and security edge cases so isRelativeURL behavior is
locked down: add cases for inputs like protocol‑like strings and mixed slashes
(e.g., "http://example", "https:\\\\example", "mailto:foo", "///evil",
"//example", "////example"), whitespace/punycode/percent encodings and control
chars (e.g., " /test", "%2Ftest", "\u0000", "\u0009/test"), and unicode/encoded
slash tricks (e.g., "%5C%2Ftest") to the testCases const and assert the expected
boolean for each using the isRelativeURL function so regressions are caught and
intended behavior is documented.
In `@packages/server-fetch/src/helpers.ts`:
- Around line 53-59: The NaN guard inside the reducer in toBigInt is
dead/unsafe: because net.isIPv6 validation rejects malformed addresses before
toBigInt runs, the isNaN(parsed) branch is unreachable — either remove that
branch entirely from the reducer, or (preferable) make it fail-closed by
returning 0n for the whole toBigInt call to match the IPv4 short-circuit
behavior; locate the reducer in toBigInt (the groups.reduce block that
shifts/accumulates BigInt values) and replace the per-group "return value <<
16n" on isNaN with logic that aborts and returns 0n from toBigInt (or remove the
check if you choose to rely on net.isIPv6 validation).
🪄 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: 1f903973-e306-478e-91e5-0c1721bfd098
📒 Files selected for processing (6)
apps/meteor/app/authentication/server/lib/restrictLoginAttempts.tsapps/meteor/lib/utils/isRelativeURL.tsapps/meteor/tests/unit/lib/utils/isRelativeURL.spec.tspackages/server-fetch/src/helpers.tspackages/server-fetch/src/index.tspackages/server-fetch/tests/checkForSsrf.spec.ts
📜 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/server-fetch/src/index.tspackages/server-fetch/tests/checkForSsrf.spec.tsapps/meteor/tests/unit/lib/utils/isRelativeURL.spec.tspackages/server-fetch/src/helpers.tsapps/meteor/app/authentication/server/lib/restrictLoginAttempts.tsapps/meteor/lib/utils/isRelativeURL.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:
packages/server-fetch/tests/checkForSsrf.spec.tsapps/meteor/tests/unit/lib/utils/isRelativeURL.spec.ts
🧠 Learnings (17)
📓 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.
📚 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/server-fetch/src/index.tspackages/server-fetch/tests/checkForSsrf.spec.tsapps/meteor/tests/unit/lib/utils/isRelativeURL.spec.tspackages/server-fetch/src/helpers.tsapps/meteor/app/authentication/server/lib/restrictLoginAttempts.tsapps/meteor/lib/utils/isRelativeURL.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/server-fetch/src/index.tspackages/server-fetch/tests/checkForSsrf.spec.tsapps/meteor/tests/unit/lib/utils/isRelativeURL.spec.tspackages/server-fetch/src/helpers.tsapps/meteor/app/authentication/server/lib/restrictLoginAttempts.tsapps/meteor/lib/utils/isRelativeURL.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/server-fetch/tests/checkForSsrf.spec.tsapps/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:
packages/server-fetch/tests/checkForSsrf.spec.tsapps/meteor/tests/unit/lib/utils/isRelativeURL.spec.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/tests/unit/lib/utils/isRelativeURL.spec.tsapps/meteor/lib/utils/isRelativeURL.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:
apps/meteor/tests/unit/lib/utils/isRelativeURL.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:
apps/meteor/tests/unit/lib/utils/isRelativeURL.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:
apps/meteor/tests/unit/lib/utils/isRelativeURL.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 : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/unit/lib/utils/isRelativeURL.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 : Group related tests in the same file
Applied to files:
apps/meteor/tests/unit/lib/utils/isRelativeURL.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:
apps/meteor/tests/unit/lib/utils/isRelativeURL.spec.ts
📚 Learning: 2026-03-06T18:09:17.867Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/elements/Timestamp/DateTimeFormats.spec.tsx:20-23
Timestamp: 2026-03-06T18:09:17.867Z
Learning: In the RocketChat/Rocket.Chat gazzodown package (`packages/gazzodown`), tests are intended to run under the UTC timezone, but as of PR `#39397` this is NOT yet explicitly enforced in `jest.config.ts` or the `package.json` test scripts (which just run `jest` without `TZ=UTC`). To make timezone-sensitive snapshot tests reliable across all environments, `TZ=UTC` should be added to the test scripts in `package.json` or to `jest.config.ts` via `testEnvironmentOptions.timezone`. Without explicit UTC enforcement, snapshot tests involving date-fns formatted output or `toLocaleString()` will fail for contributors in non-UTC timezones.
Applied to files:
apps/meteor/tests/unit/lib/utils/isRelativeURL.spec.ts
📚 Learning: 2026-03-06T18:02:20.381Z
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.
Applied to files:
apps/meteor/tests/unit/lib/utils/isRelativeURL.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/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/unit/lib/utils/isRelativeURL.spec.ts
📚 Learning: 2026-04-20T17:11:59.452Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40225
File: apps/meteor/ee/server/apps/communication/endpoints/appLogsHandler.ts:55-71
Timestamp: 2026-04-20T17:11:59.452Z
Learning: In `apps/meteor/ee/server/apps/communication/endpoints/appLogsHandler.ts`, the concern about an empty `?appId=` query param bypassing the truthy check and overriding the path `appId` in the `makeAppLogsQuery` spread is not relevant. The AJV query schema (`isAppLogsProps`) validates and rejects invalid/empty `appId` values before the action handler is reached, making the in-handler guard sufficient as-is. Do not flag this pattern as a vulnerability in future reviews of this file.
Applied to files:
apps/meteor/app/authentication/server/lib/restrictLoginAttempts.ts
📚 Learning: 2026-03-16T11:57:17.987Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39657
File: apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts:43-45
Timestamp: 2026-03-16T11:57:17.987Z
Learning: In `apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts`, `externalIds` lookups are namespaced per `app.id`. Because `cleanupApps()` is called before each test run and a fresh app is installed (giving a new unique `app.id`), fixed `userId` strings like `'nonexistent-id'` in null-path tests are safe and cannot collide with stale visitor records from previous runs. Do not flag these as needing unique/random values.
Applied to files:
apps/meteor/app/authentication/server/lib/restrictLoginAttempts.ts
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
1 participant
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.
Summary
Fixes #40313
Fixes incorrect isRelativeURL classification and updates unit tests to reflect expected relative URL behavior.
What changed
isRelativeURL.ts:1 to:
isRelativeURL.spec.ts:6:
Why
The previous regex-based implementation incorrectly:
This function is used in message URL validation paths (e.g. partial URL checks), so incorrect classification can lead to wrong accept/reject behavior.
Test plan
Run:
corepack yarn workspace "@rocket.chat/meteor" .testunit:server --grep isRelativeURL
Expected: isRelativeURL unit cases pass with corrected behavior.
Summary by CodeRabbit
New Features
Bug Fixes
Tests