feat: implement login attempt restriction and IP CIDR validation logic#40201
feat: implement login attempt restriction and IP CIDR validation logic#40201namann5 wants to merge 6 commits intoRocketChat:developfrom
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 |
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughCIDR-aware IP whitelist checks were added and login identifier resolution was extended to use email as a fallback; the CIDR utility was re-exported and IPv6 parsing was hardened; tests updated to cover malformed IP/CIDR inputs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Auth as AuthServer\n(restrictLoginAttempts)
participant Helper as ServerFetch\nisIpInCidrRange
participant DB as Database/EventLogger
Client->>Auth: Submit login (credentials, IP)
Auth->>Auth: Resolve identifier\n(login.user?.username -> methodArgs.user.username -> methodArgs.user.email)
Auth->>Helper: For each whitelist entry:\ncheck exact match or CIDR contains IP
Helper-->>Auth: true / false
alt allowed by whitelist
Auth->>DB: Record successful login event\n(populate u.username)
Auth-->>Client: Allow login
else not allowed
Auth->>DB: Record failed attempt\n(populate u.username)
Auth-->>Client: Reject / rate-limit
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
coderabbitai
bot
added
type: bug
type: feature
![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: 1e33757feb
ℹ️ 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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/authentication/server/lib/restrictLoginAttempts.ts (1)
150-175:⚠️ Potential issue | 🟠 MajorEmail stored into
u.usernameviolates the schema contract and reintroduces a rate-limit bypass.At lines 153 and 167, both
saveFailedLoginAttemptsandsaveSuccessfulLoginassign email strings to theu.usernamefield when the username fallbacks are undefined. SinceIServerEvent['u']is typed asPartial<Pick<IUser, '_id' | 'username'>>and queries index on{ 't': 1, 'u.username': 1, 'ts': -1 }, storing email violates this schema. More critically, the rate-limiting logic inisValidAttemptByUser(lines 104–134) constructsloginUsername = username || email, then queries with that value againstfindLastFailedAttemptByUsernameandcountFailedAttemptsByUsernameSince. If both email and username attempts are recorded with different identifiers inu.username, an attacker can split their failed-attempt budget between them, recreating the very bypass this fix intends to prevent.Either normalize email to the canonical username/userId before storing (e.g.,
Users.findOneByEmailAddress), or add a dedicated field (e.g.,u.emailoru.loginIdentifier) and query by both identifiers in rate-limit checks. Also update the query methods at lines 118, 126, and 134 accordingly.🤖 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 150 - 175, saveFailedLoginAttempts and saveSuccessfulLogin currently write an email into u.username which violates IServerEvent['u'] and allows a rate-limit bypass; update both functions to resolve the login identifier to the canonical username (or userId) before inserting (e.g., use Users.findOneByEmailAddress or resolve login.user._id to a username) so u.username always contains the true username, and then adjust isValidAttemptByUser and its helpers (findLastFailedAttemptByUsername, countFailedAttemptsByUsernameSince) to continue querying by that canonical username; alternatively add a dedicated field like u.email or u.loginIdentifier and update the query methods to check both identifiers—ensure consistent identifier usage across saveFailedLoginAttempts, saveSuccessfulLogin, isValidAttemptByUser, findLastFailedAttemptByUsername, and countFailedAttemptsByUsernameSince.
🧹 Nitpick comments (2)
apps/meteor/app/authentication/server/lib/restrictLoginAttempts.ts (2)
56-56: Redundant exact-match branch.Per the implementation of
isIpInCidrRangeinpackages/server-fetch/src/helpers.ts, a CIDR entry with no prefix is treated as/32(IPv4) or/128(IPv6), soisIpInCidrRange(ip, entry.trim())already handles the exact-match case. Theentry.trim() === ipdisjunct can be dropped.♻️ Proposed simplification
- whitelist.some((entry) => entry.trim() === ip || isIpInCidrRange(ip, entry.trim())) + whitelist.some((entry) => isIpInCidrRange(ip, entry.trim()))🤖 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` at line 56, Remove the redundant exact-match branch in the whitelist check: simplify the predicate passed to whitelist.some by dropping the entry.trim() === ip disjunct and rely solely on isIpInCidrRange(ip, entry.trim()) (this uses the existing isIpInCidrRange implementation which treats bare addresses as /32 or /128), so update the predicate where whitelist.some is called to only call isIpInCidrRange(ip, entry.trim()).
104-105: Prefer updatingIMethodArgumentover an inline cast.
IMethodArgument.userinapps/meteor/app/authentication/server/ILoginAttempt.tsis typed as{ username: string }and does not declare ansaveFailedLoginAttempts/saveSuccessfulLoginbelow, updating the interface once keeps typing accurate across all call sites and removes the need for a local cast.♻️ Proposed change
In
apps/meteor/app/authentication/server/ILoginAttempt.ts:interface IMethodArgument { - user?: { username: string }; + user?: { username?: string; email?: string };Then here:
- const { username, email } = (login.methodArguments[0].user || {}) as { username?: string; email?: string }; - const loginUsername = username || email; + const loginUsername = login.methodArguments[0].user?.username || login.methodArguments[0].user?.email;🤖 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 104 - 105, Update the IMethodArgument.user shape in apps/meteor/app/authentication/server/ILoginAttempt.ts to include an optional email (e.g. change { username: string } to { username?: string; email?: string }) so the type accurately reflects both username and email fields; then remove the inline cast in restrictLoginAttempts.ts (where login.methodArguments[0].user is destructured) and ensure the other call sites using saveFailedLoginAttempts and saveSuccessfulLogin accept the updated user shape without casts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/meteor/app/authentication/server/lib/restrictLoginAttempts.ts`:
- Around line 150-175: saveFailedLoginAttempts and saveSuccessfulLogin currently
write an email into u.username which violates IServerEvent['u'] and allows a
rate-limit bypass; update both functions to resolve the login identifier to the
canonical username (or userId) before inserting (e.g., use
Users.findOneByEmailAddress or resolve login.user._id to a username) so
u.username always contains the true username, and then adjust
isValidAttemptByUser and its helpers (findLastFailedAttemptByUsername,
countFailedAttemptsByUsernameSince) to continue querying by that canonical
username; alternatively add a dedicated field like u.email or u.loginIdentifier
and update the query methods to check both identifiers—ensure consistent
identifier usage across saveFailedLoginAttempts, saveSuccessfulLogin,
isValidAttemptByUser, findLastFailedAttemptByUsername, and
countFailedAttemptsByUsernameSince.
---
Nitpick comments:
In `@apps/meteor/app/authentication/server/lib/restrictLoginAttempts.ts`:
- Line 56: Remove the redundant exact-match branch in the whitelist check:
simplify the predicate passed to whitelist.some by dropping the entry.trim() ===
ip disjunct and rely solely on isIpInCidrRange(ip, entry.trim()) (this uses the
existing isIpInCidrRange implementation which treats bare addresses as /32 or
/128), so update the predicate where whitelist.some is called to only call
isIpInCidrRange(ip, entry.trim()).
- Around line 104-105: Update the IMethodArgument.user shape in
apps/meteor/app/authentication/server/ILoginAttempt.ts to include an optional
email (e.g. change { username: string } to { username?: string; email?: string
}) so the type accurately reflects both username and email fields; then remove
the inline cast in restrictLoginAttempts.ts (where login.methodArguments[0].user
is destructured) and ensure the other call sites using saveFailedLoginAttempts
and saveSuccessfulLogin accept the updated user shape without casts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32aa6fbb-30be-4140-9042-05032d5d8753
📒 Files selected for processing (2)
apps/meteor/app/authentication/server/lib/restrictLoginAttempts.tspackages/server-fetch/src/index.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 (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:
packages/server-fetch/src/index.tsapps/meteor/app/authentication/server/lib/restrictLoginAttempts.ts
🧠 Learnings (5)
📓 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: 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-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.tsapps/meteor/app/authentication/server/lib/restrictLoginAttempts.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.tsapps/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
📚 Learning: 2025-10-07T15:08:37.419Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36942
File: apps/meteor/client/lib/e2ee/pbkdf2.ts:13-45
Timestamp: 2025-10-07T15:08:37.419Z
Learning: In apps/meteor/client/lib/e2ee/pbkdf2.ts, PBKDF2 iteration count validation is not enforced because the iterations parameter is outside the user's control and is system-managed.
Applied to files:
apps/meteor/app/authentication/server/lib/restrictLoginAttempts.ts
🔇 Additional comments (1)
packages/server-fetch/src/index.ts (1)
196-196: LGTM.Clean re-export that exposes the existing
isIpInCidrRangehelper to consumers of@rocket.chat/server-fetch.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
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/app/authentication/server/lib/restrictLoginAttempts.ts">
<violation number="1" location="apps/meteor/app/authentication/server/lib/restrictLoginAttempts.ts:56">
P2: CIDR whitelist matching can throw on malformed CIDR entries, causing login-attempt validation to fail at runtime.</violation>
<violation number="2" location="apps/meteor/app/authentication/server/lib/restrictLoginAttempts.ts:105">
P1: User lockout tracking is keyed by raw login identifier (username/email), causing split buckets and missed reset when success is stored under canonical username.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
… add CIDR range validation helpers
coderabbitai
bot
removed
type: bug
type: feature
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/server-fetch/src/helpers.ts (1)
53-59: Consider aborting on malformed hex groups instead of silently shifting.Sibling error paths in this same function (line 45) bail out by returning
0nwhen an embedded IPv4 group is invalid. The new NaN branch diverges: it keeps shifting and continues, which means two different malformed inputs can both collapse to the same bigint and spuriously match underisIpInCidrRange. Today the upstreamnet.isIPv6checks onipUnwrappedandnetworkmake this branch effectively unreachable, but sinceisIpInCidrRangeis now re-exported viapackages/server-fetch/src/index.tsand consumed directly byrestrictLoginAttempts.ts, a future caller that skips pre-validation would get silently-coerced-to-zero behavior rather than a deterministicfalse.A more defensive shape that mirrors the existing convention:
♻️ Proposed alignment with the existing error convention
- return groups.reduce((value, hexGroup) => { - const parsed = parseInt(hexGroup, 16); - if (isNaN(parsed)) { - return value << 16n; - } - return (value << 16n) + BigInt(parsed); - }, 0n); + let result = 0n; + for (const hexGroup of groups) { + const parsed = parseInt(hexGroup, 16); + if (Number.isNaN(parsed)) { + return 0n; + } + result = (result << 16n) + BigInt(parsed); + } + return result;Then the outer comparison at line 62 will still compare
0n === 0nwhen both sides are malformed, so if you want strict rejection of malformed inputs, add an explicit pre-check vianet.isIPv6onipat the top oftoBigInt(or validate at the call site).🤖 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 hex-group parsing in toBigInt currently shifts on NaN which silently coerces malformed IPv6 groups; change the NaN branch inside the groups.reduce in toBigInt to bail out by returning 0n (same convention used for the embedded IPv4 error path) so malformed input yields 0n deterministically; this keeps behavior consistent with the existing embedded-IPv4 check and avoids spurious matches in isIpInCidrRange when callers skip prior net.isIPv6 validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/server-fetch/src/helpers.ts`:
- Around line 53-59: The hex-group parsing in toBigInt currently shifts on NaN
which silently coerces malformed IPv6 groups; change the NaN branch inside the
groups.reduce in toBigInt to bail out by returning 0n (same convention used for
the embedded IPv4 error path) so malformed input yields 0n deterministically;
this keeps behavior consistent with the existing embedded-IPv4 check and avoids
spurious matches in isIpInCidrRange when callers skip prior net.isIPv6
validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39e94baf-a4bb-488a-b666-846c4a661770
📒 Files selected for processing (3)
apps/meteor/app/authentication/server/lib/restrictLoginAttempts.tspackages/server-fetch/src/helpers.tspackages/server-fetch/tests/checkForSsrf.spec.ts
✅ Files skipped from review due to trivial changes (1)
- packages/server-fetch/tests/checkForSsrf.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/app/authentication/server/lib/restrictLoginAttempts.ts
📜 Review details
🧰 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:
packages/server-fetch/src/helpers.ts
🧠 Learnings (3)
📓 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/helpers.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/helpers.ts
coderabbitai
bot
added
type: feature
1 participant
Closes #40195
🔐 Login Protection Hardening
This PR addresses a security gap in the login protection mechanism where email-based logins could bypass rate-limiting. It also enhances IP whitelist handling by adding CIDR range support.
🚀 Summary of Changes
1. Email-based Login Protection Fix
2. CIDR Range Support for IP Whitelist
10.0.0.0/8) in IP whitelist checksisIpInCidrRangeutility from@rocket.chat/server-fetch3. Improved Audit Logging
🛠 Implementation Details
Re-exported
isIpInCidrRangefromserver-fetchfor reuseUpdated login restriction logic to:
Refactored logging to use canonical identifiers
🧪 Verification
Automated Validation
IP whitelist:
Login identifiers:
Manual Review
📌 Impact
Summary by CodeRabbit
New Features
Bug Fixes
Tests