feat: enable password policy by default and increase MinLength
#38032
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 4525f83 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 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 |
WalkthroughPassword policy is enabled by default and minimum length raised to 14 in server settings; the setup wizard and UI now validate against centralized password-policy options; tests and fixtures updated to use stronger passwords and to toggle the policy where needed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Admin as Admin Browser (SetupWizard)
participant UI as ui-client (AdminInfoStep)
participant Hooks as Password Policy Hooks (usePasswordPolicyOptions / useVerifyPassword)
participant Settings as Server Settings
participant API as Server API (create user)
Note over Settings,Hooks: Default policy enabled, minLength = 14
Admin->>UI: enter password
UI->>Settings: read policy settings (via usePasswordPolicyOptions)
UI->>Hooks: validate(password, options)
Hooks-->>UI: validation result + hints/errors
UI-->>Admin: show passwordRulesHint or error
alt valid
UI->>API: submit create admin user
API-->>UI: success
UI-->>Admin: advance step
else invalid
UI-->>Admin: block submit, show errors
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/server/settings/accounts.ts (1)
817-821: Extraneouspublic: trueinenableQueryobject.The
enableQueryobject is used for conditional field enabling based on the parent setting's value. Thepublic: trueproperty appears misplaced here—it's not a valid property forenableQueryand should only appear in the setting definition itself (which the dependent settings already have).🔎 Proposed fix
const enableQuery = { _id: 'Accounts_Password_Policy_Enabled', value: true, - public: true, };
🧹 Nitpick comments (2)
packages/ui-client/src/views/setupWizard/steps/AdminInfoStep.tsx (2)
71-82: Consider providing specific validation failure details.The current implementation returns a generic error message (
Password_must_meet_the_complexity_requirements) when password validation fails. SincevalidatePasswordPolicylikely returns details about which rules failed, consider surfacing the specific validation failure to help users understand exactly what's wrong with their password.🔎 Example improvement
const validatePassword = (password: string): boolean | string => { if (!password || password.length === 0) { return t('Required_field', { field: t('Password') }); } const passwordValidation = validatePasswordPolicy(password); if (!passwordValidation.valid) { - return t('Password_must_meet_the_complexity_requirements'); + // If the hook provides failure details, surface them + const failedRule = passwordValidation.failedRules?.[0]; + if (failedRule) { + return t(`${failedRule.name}-label`, 'limit' in failedRule ? { limit: failedRule.limit } : {}); + } + return t('Password_must_meet_the_complexity_requirements'); } return true; };
22-30: Hardcoded defaults may drift from server settings.The fallback values here (e.g.,
minLength: 14,forbidRepeatingCharactersCount: 3) duplicate the server-side defaults inaccounts.ts. If server defaults change in the future, these could become inconsistent.Consider extracting shared constants or relying on the settings infrastructure to provide defaults, reducing the risk of drift.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.changeset/nervous-clouds-carry.mdapps/meteor/server/settings/accounts.tspackages/ui-client/src/views/setupWizard/steps/AdminInfoStep.tsx
🧰 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/server/settings/accounts.tspackages/ui-client/src/views/setupWizard/steps/AdminInfoStep.tsx
🧠 Learnings (2)
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.
Applied to files:
apps/meteor/server/settings/accounts.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/server/settings/accounts.ts
⏰ 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). (2)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
.changeset/nervous-clouds-carry.md (1)
1-6: LGTM!The changeset correctly documents the minor version bumps for both packages and provides a clear description of the behavioral changes (enabling password policy by default and updating SetupWizard error handling).
apps/meteor/server/settings/accounts.ts (1)
812-827: Good security defaults.Enabling the password policy by default and setting
MinLengthto 14 (exceeding the 8-character requirement from VLN-47) strengthens the default security posture. The remaining policy settings (forbid repeating characters, require mixed case, numbers, and special characters) are already enabled by default, which aligns well with the objective.packages/ui-client/src/views/setupWizard/steps/AdminInfoStep.tsx (2)
88-100: LGTM!The integration with
AdminInfoPageis clean—passingvalidatePasswordandpasswordRulesHintenables the setup wizard to enforce and display password policy requirements. This achieves the PR objective of handling password policy errors during initial setup.
45-60: The implementation is correct.useVerifyPassword('')returns all enabled password policy rules regardless of the empty input, which is the intended behavior for displaying policy requirements upfront. ThesendValidationMessage()method returns an array of all configured validation rules, and theAdminInfoStepcode correctly maps these to translatable labels for display.Likely an incorrect or invalid review comment.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38032 +/- ##
===========================================
+ Coverage 70.64% 70.66% +0.02%
===========================================
Files 3145 3145
Lines 108772 108772
Branches 19583 19563 -20
===========================================
+ Hits 76841 76863 +22
+ Misses 29916 29906 -10
+ Partials 2015 2003 -12
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 Docker Image Size Report➡️ Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "01/08 14:56 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Thu, 08 Jan 2026 14:56:24 GMT |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 10 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/ui-client/src/views/setupWizard/steps/AdminInfoStep.tsx">
<violation number="1" location="packages/ui-client/src/views/setupWizard/steps/AdminInfoStep.tsx:45">
P1: Mismatched default values: `useVerifyPassword` defaults minLength to 7, but local `validatePasswordPolicy` defaults to 14. This can cause the password rules hint to display incorrect requirements. Consider using only `useVerifyPassword` for both validation and hints, or ensure the defaults match.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/tests/data/user.ts (1)
7-7: Consider strengthening the admin password for better security practices.While
adminPassword = adminUsernametechnically meets the 14-character minimum (the username is 31 characters), using the username as the password is a weak security practice because it's predictable and lacks complexity (no numbers or special characters). Even in test environments, demonstrating secure password patterns is recommended.🔒 Suggested improvement
-export const adminPassword = adminUsername; +export const adminPassword = 'R0ck3t.Adm1n!P@ssw0rd2024';
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
apps/meteor/tests/data/user.tsapps/meteor/tests/e2e/account-security.spec.tsapps/meteor/tests/e2e/administration.spec.tsapps/meteor/tests/e2e/register.spec.tsapps/meteor/tests/e2e/reset-password.spec.tsapps/meteor/tests/e2e/user-required-password-change.spec.tsapps/meteor/tests/end-to-end/api/users.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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/tests/e2e/user-required-password-change.spec.tsapps/meteor/tests/e2e/administration.spec.tsapps/meteor/tests/e2e/account-security.spec.tsapps/meteor/tests/e2e/reset-password.spec.tsapps/meteor/tests/data/user.tsapps/meteor/tests/e2e/register.spec.tsapps/meteor/tests/end-to-end/api/users.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/e2e/user-required-password-change.spec.tsapps/meteor/tests/e2e/administration.spec.tsapps/meteor/tests/e2e/account-security.spec.tsapps/meteor/tests/e2e/reset-password.spec.tsapps/meteor/tests/e2e/register.spec.ts
apps/meteor/tests/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.spec.ts: All test files must be created inapps/meteor/tests/e2e/directory
Avoid usingpage.locator()in Playwright tests - always prefer semantic locators such aspage.getByRole(),page.getByLabel(),page.getByText(), orpage.getByTitle()
Usetest.beforeAll()andtest.afterAll()for setup/teardown in Playwright tests
Usetest.step()for complex test scenarios to improve organization in Playwright tests
Group related tests in the same file
Utilize Playwright fixtures (test,page,expect) for consistency in test files
Prefer web-first assertions (toBeVisible,toHaveText, etc.) in Playwright tests
Useexpectmatchers for assertions (toEqual,toContain,toBeTruthy,toHaveLength, etc.) instead ofassertstatements in Playwright tests
Usepage.waitFor()with specific conditions instead of hardcoded timeouts in Playwright tests
Implement proper wait strategies for dynamic content in Playwright tests
Maintain test isolation between test cases in Playwright tests
Ensure clean state for each test execution in Playwright tests
Ensure tests run reliably in parallel without shared state conflicts
Files:
apps/meteor/tests/e2e/user-required-password-change.spec.tsapps/meteor/tests/e2e/administration.spec.tsapps/meteor/tests/e2e/account-security.spec.tsapps/meteor/tests/e2e/reset-password.spec.tsapps/meteor/tests/e2e/register.spec.ts
apps/meteor/tests/e2e/**/*.{ts,spec.ts}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,spec.ts}: Store commonly used locators in variables/constants for reuse
Follow Page Object Model pattern consistently in Playwright tests
Files:
apps/meteor/tests/e2e/user-required-password-change.spec.tsapps/meteor/tests/e2e/administration.spec.tsapps/meteor/tests/e2e/account-security.spec.tsapps/meteor/tests/e2e/reset-password.spec.tsapps/meteor/tests/e2e/register.spec.ts
🧠 Learnings (14)
📚 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/e2e/user-required-password-change.spec.tsapps/meteor/tests/e2e/administration.spec.tsapps/meteor/tests/e2e/account-security.spec.tsapps/meteor/tests/e2e/reset-password.spec.tsapps/meteor/tests/e2e/register.spec.tsapps/meteor/tests/end-to-end/api/users.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/e2e/user-required-password-change.spec.tsapps/meteor/tests/e2e/administration.spec.tsapps/meteor/tests/e2e/account-security.spec.tsapps/meteor/tests/e2e/reset-password.spec.tsapps/meteor/tests/e2e/register.spec.tsapps/meteor/tests/end-to-end/api/users.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/e2e/user-required-password-change.spec.tsapps/meteor/tests/e2e/administration.spec.tsapps/meteor/tests/e2e/account-security.spec.tsapps/meteor/tests/e2e/reset-password.spec.tsapps/meteor/tests/e2e/register.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 clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/tests/e2e/user-required-password-change.spec.tsapps/meteor/tests/e2e/administration.spec.tsapps/meteor/tests/e2e/account-security.spec.tsapps/meteor/tests/e2e/reset-password.spec.tsapps/meteor/tests/e2e/register.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:
apps/meteor/tests/e2e/user-required-password-change.spec.tsapps/meteor/tests/e2e/administration.spec.tsapps/meteor/tests/e2e/account-security.spec.tsapps/meteor/tests/e2e/reset-password.spec.tsapps/meteor/tests/e2e/register.spec.tsapps/meteor/tests/end-to-end/api/users.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/e2e/user-required-password-change.spec.tsapps/meteor/tests/e2e/administration.spec.tsapps/meteor/tests/e2e/account-security.spec.tsapps/meteor/tests/e2e/reset-password.spec.tsapps/meteor/tests/e2e/register.spec.tsapps/meteor/tests/end-to-end/api/users.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 `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/tests/e2e/user-required-password-change.spec.tsapps/meteor/tests/e2e/account-security.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 `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/tests/e2e/user-required-password-change.spec.tsapps/meteor/tests/e2e/administration.spec.tsapps/meteor/tests/e2e/account-security.spec.tsapps/meteor/tests/e2e/reset-password.spec.tsapps/meteor/tests/e2e/register.spec.tsapps/meteor/tests/end-to-end/api/users.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/e2e/user-required-password-change.spec.tsapps/meteor/tests/e2e/administration.spec.tsapps/meteor/tests/e2e/account-security.spec.tsapps/meteor/tests/e2e/reset-password.spec.tsapps/meteor/tests/e2e/register.spec.tsapps/meteor/tests/end-to-end/api/users.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/e2e/user-required-password-change.spec.tsapps/meteor/tests/e2e/administration.spec.tsapps/meteor/tests/e2e/account-security.spec.tsapps/meteor/tests/e2e/reset-password.spec.tsapps/meteor/tests/e2e/register.spec.tsapps/meteor/tests/end-to-end/api/users.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/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/e2e/administration.spec.tsapps/meteor/tests/e2e/register.spec.tsapps/meteor/tests/end-to-end/api/users.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 : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/tests/e2e/reset-password.spec.tsapps/meteor/tests/end-to-end/api/users.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, the team has decided to use Latin-1 encoding (via Binary.toArrayBuffer and Binary.toString) for password encoding and decrypt output instead of UTF-8 encoding. This is a deliberate choice for E2EE password/key material handling.
Applied to files:
apps/meteor/tests/data/user.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/tests/data/user.tsapps/meteor/tests/end-to-end/api/users.ts
🧬 Code graph analysis (1)
apps/meteor/tests/e2e/account-security.spec.ts (1)
apps/meteor/tests/e2e/page-objects/account-security.ts (1)
AccountSecurity(6-103)
⏰ 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 comments (14)
apps/meteor/tests/data/user.ts (1)
4-4: LGTM: Password updated to meet new policy requirements.The new password value meets the minimum 14-character requirement and includes appropriate complexity (uppercase, lowercase, numbers, and special characters).
apps/meteor/tests/e2e/administration.spec.ts (2)
78-79: LGTM: User creation passwords updated for policy compliance.The passwords have been appropriately updated from 'any_password' to 'P@ssw0rd1234.!', which meets the new 14-character minimum requirement and includes required complexity.
98-99: LGTM: Consistent password update for user creation test.The passwords correctly use 'P@ssw0rd1234.!' to comply with the new password policy requirements.
apps/meteor/tests/e2e/reset-password.spec.ts (1)
20-21: LGTM: Password mismatch test updated with policy-compliant values.Both passwords meet the new policy requirements (14+ characters with complexity) while remaining intentionally different to properly test the password confirmation mismatch scenario.
apps/meteor/tests/e2e/user-required-password-change.spec.ts (1)
22-22: LGTM: Appropriate policy toggle for isolated password change flow testing.Temporarily disabling the password policy in this test suite is the correct approach, as these tests focus on the "required password change" flow rather than policy validation. The policy is properly re-enabled in the cleanup phase.
Also applies to: 42-42
apps/meteor/tests/e2e/register.spec.ts (1)
34-35: LGTM: Registration tests updated with policy-compliant passwords.All password values have been consistently updated to 'P@ssw0rd1234.!' and related variants, meeting the new 14-character minimum requirement with appropriate complexity. The mismatch test scenarios correctly use different passwords that both satisfy the policy requirements.
Also applies to: 42-42, 77-77, 152-152, 161-162, 206-207
apps/meteor/tests/e2e/account-security.spec.ts (3)
11-19: LGTM: Strong random password generationThe shuffled combination of character types ensures sufficient entropy and unpredictability. The resulting password should consistently meet the 14-character minimum length requirement.
24-28: LGTM: Proper test isolationDisabling the password policy before each test ensures these account security tests focus on their intended features (2FA, password changes, E2E encryption) without interference from password validation rules.
36-36: LGTM: Proper cleanupRe-enabling the password policy in
afterAllensures the default state is restored for subsequent test runs.apps/meteor/tests/end-to-end/api/users.ts (5)
820-876: LGTM: Compliant test passwordsThe updated password
P@ssw0rd1234.!(15 characters) meets all policy requirements with uppercase, lowercase, numeric, and special characters for the registration endpoint tests.
1108-1108: LGTM: Using shared password constantUsing the imported
passwordconstant ensures consistency across tests. Based on the PR context, this constant should already be updated to meet the policy requirements.
2174-2196: LGTM: Strong test password for update operationsThe password
1Tsn0tw0rkingP@ssw0rd1234.!(28 characters) exceeds all policy requirements and is appropriately used in tests verifying password change permissions.
2703-3087: LGTM: Policy-compliant passwords in self-update testsThe password
1Tsn3wP@ssw0rd1234.!(21 characters) appropriately meets policy requirements for tests validating user self-service password updates and policy enforcement.
2891-3097: Password policy is enabled by default for these testsThese tests validate password policy enforcement (min/max length, character requirements, etc.) and depend on
Accounts_Password_Policy_Enabledbeing true. The policy is enabled by default in the system configuration, so these tests will execute correctly without explicit setup calls.
|
I don't think this is a |
julio-rocketchat
changed the title
MinLengthMinLength
You're right. I've changed the PR title |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/ui-client/src/views/setupWizard/steps/AdminInfoStep.tsx (1)
25-40: Consider extracting policy rules directly without validation.Calling
useVerifyPassword('')to build the password hint validates an empty password just to extract the rule definitions. While functional, this is semantically indirect.💡 Alternative approach
Consider creating a dedicated hook that returns policy rules without performing validation, or retrieve the rules directly from
passwordPolicyOptions:const passwordRulesHint = useMemo(() => { if (!passwordPolicyOptions.enabled) { return ''; } const rules: string[] = []; if (passwordPolicyOptions.minLength > 0) { rules.push(t('get-password-policy-minLength-label', { limit: passwordPolicyOptions.minLength })); } if (passwordPolicyOptions.mustContainAtLeastOneLowercase) { rules.push(t('get-password-policy-mustContainAtLeastOneLowercase-label')); } // ... other rules return rules.join(', '); }, [passwordPolicyOptions, t]);This avoids the indirection of validating an empty password just to discover the rules.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/ui-client/src/components/PasswordVerifier/PasswordVerifiers.spec.tsxpackages/ui-client/src/views/setupWizard/steps/AdminInfoStep.tsxpackages/ui-contexts/src/hooks/usePasswordPolicyOptions.tspackages/ui-contexts/src/hooks/useVerifyPassword.tspackages/ui-contexts/src/index.ts
🧰 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/ui-contexts/src/index.tspackages/ui-client/src/components/PasswordVerifier/PasswordVerifiers.spec.tsxpackages/ui-client/src/views/setupWizard/steps/AdminInfoStep.tsxpackages/ui-contexts/src/hooks/usePasswordPolicyOptions.tspackages/ui-contexts/src/hooks/useVerifyPassword.ts
🧠 Learnings (4)
📚 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:
packages/ui-client/src/components/PasswordVerifier/PasswordVerifiers.spec.tsx
📚 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/ui-client/src/components/PasswordVerifier/PasswordVerifiers.spec.tsx
📚 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/ui-client/src/components/PasswordVerifier/PasswordVerifiers.spec.tsx
📚 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:
packages/ui-client/src/views/setupWizard/steps/AdminInfoStep.tsx
🧬 Code graph analysis (3)
packages/ui-client/src/components/PasswordVerifier/PasswordVerifiers.spec.tsx (1)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
packages/ui-client/src/views/setupWizard/steps/AdminInfoStep.tsx (3)
packages/ui-contexts/src/hooks/usePasswordPolicyOptions.ts (1)
usePasswordPolicyOptions(5-28)packages/ui-contexts/src/index.ts (3)
usePasswordPolicyOptions(91-91)usePasswordPolicy(92-92)useVerifyPassword(93-93)packages/ui-contexts/src/hooks/useVerifyPassword.ts (1)
useVerifyPassword(6-11)
packages/ui-contexts/src/hooks/usePasswordPolicyOptions.ts (1)
packages/ui-contexts/src/index.ts (2)
usePasswordPolicyOptions(91-91)useSetting(72-72)
⏰ 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 comments (6)
packages/ui-client/src/views/setupWizard/steps/AdminInfoStep.tsx (2)
51-62: LGTM!The password validation logic correctly enforces the policy: it checks for empty passwords and validates against the configured policy, returning appropriate error messages.
71-72: LGTM!Correctly passes password validation and hints to the AdminInfoPage component, integrating password policy enforcement into the setup wizard flow.
packages/ui-contexts/src/index.ts (1)
91-91: LGTM!The export of
usePasswordPolicyOptionsis correctly placed and enables centralized password policy configuration across the UI.packages/ui-client/src/components/PasswordVerifier/PasswordVerifiers.spec.tsx (1)
19-38: LGTM!The test correctly simulates an enabled-but-empty policy by explicitly disabling all enforcement rules while keeping the policy enabled. This properly validates that no policy UI is rendered when no actual rules are active.
packages/ui-contexts/src/hooks/useVerifyPassword.ts (1)
4-10: LGTM! Clean refactoring.The refactoring consolidates password policy settings reading into
usePasswordPolicyOptions, eliminating duplication and improving maintainability. The logic remains functionally equivalent while achieving better separation of concerns.packages/ui-contexts/src/hooks/usePasswordPolicyOptions.ts (1)
1-28: Default values are consistent with server-side settings.All default values in the hook match their server-side counterparts in
apps/meteor/server/settings/accounts.ts: enabled=true, minLength=14, maxLength=-1, forbidRepeatingCharacters=true, forbidRepeatingCharactersCount=3, and all character requirement defaults=true.
cardoso
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
The idea of this PR is enabling the workspace password policy by default, enforcing minimum requirements for complexity, length, and so forth. This aligns with our efforts and initiatives to make Rocket.Chat a platform taht is secure by default and by design. To align with modern practices, we've also increased the minimum length from 7 to 14.
While enabling the policy by default is straightforward, we also had to update to
SetupWizardto ensure it returns clear, appropriate errors when the admin user is being created with a password that doesn't conform with the password policy.Additionally, as we had several tests that would create users with weak passwords, such tests started failing for not being able to create them. I've changed the password for most of these tests and disabled the password policy temporarily fo others, ensuring that tests will work as expected.
Issue(s)
VLN-47
Steps to test or reproduce
N/A
Further comments
N/A
Summary by CodeRabbit
New Features
Bug Fixes
Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.