fix(password-policies): sanitize and validate forbidRepeatingCharactersCount before constructing RegExp - #41760
Conversation
…rsCount before constructing RegExp
|
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 Plus Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used📓 Path-based instructions (3)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
**/*.spec.ts📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
packages/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (9)📓 Common learnings📚 Learning: 2025-10-07T15:08:37.419ZApplied to files:
📚 Learning: 2025-12-10T21:00:43.645ZApplied to files:
📚 Learning: 2026-02-24T19:22:48.358ZApplied to files:
📚 Learning: 2026-02-26T19:25:44.063ZApplied to files:
📚 Learning: 2026-02-26T19:25:44.063ZApplied to files:
📚 Learning: 2026-03-06T18:10:15.268ZApplied to files:
📚 Learning: 2026-05-06T12:21:44.083ZApplied to files:
📚 Learning: 2026-03-16T21:50:42.118ZApplied to files:
🪛 ast-grep (0.45.1)packages/password-policies/src/PasswordPolicy.ts[warning] 99-99: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns. (regexp-from-variable) 🔇 Additional comments (3)
WalkthroughThe password policy now accepts only safe integer repetition counts of at least 1. Invalid values use the default count of 3 before regex construction. Parameterized tests verify validation, messages, and policy metadata. ChangesPassword Policy Sanitization
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![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 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="packages/password-policies/src/PasswordPolicy.ts">
<violation number="1" location="packages/password-policies/src/PasswordPolicy.ts:85">
P2: This change silently alters the semantics of `forbidRepeatingCharactersCount: 0`. Previously `0` was a functional configuration producing the valid regex `(.)\1{0,}` (rejecting any repeated character) and, being falsy, suppressed the corresponding `get-password-policy` entry in `getPasswordPolicy()`. Now `0` is silently coerced to the default `3`, which relaxes the policy and, because `this.forbidRepeatingCharactersCount` becomes truthy, newly emits the policy entry where it was previously omitted. Since the corresponding setting is an `int` with no stated minimum, an admin who set `0` would see their policy quietly weakened without warning. Consider handling `0` explicitly (coerce only clearly-invalid values or clamp/log) rather than silently remapping to `3`, and document the behavior change if intentional.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const safeForbidRepeatingCharactersCount = | ||
| typeof forbidRepeatingCharactersCount === 'number' && | ||
| Number.isSafeInteger(forbidRepeatingCharactersCount) && | ||
| forbidRepeatingCharactersCount >= 1 ? forbidRepeatingCharactersCount : 3; |
There was a problem hiding this comment.
P2: This change silently alters the semantics of forbidRepeatingCharactersCount: 0. Previously 0 was a functional configuration producing the valid regex (.)\1{0,} (rejecting any repeated character) and, being falsy, suppressed the corresponding get-password-policy entry in getPasswordPolicy(). Now 0 is silently coerced to the default 3, which relaxes the policy and, because this.forbidRepeatingCharactersCount becomes truthy, newly emits the policy entry where it was previously omitted. Since the corresponding setting is an int with no stated minimum, an admin who set 0 would see their policy quietly weakened without warning. Consider handling 0 explicitly (coerce only clearly-invalid values or clamp/log) rather than silently remapping to 3, and document the behavior change if intentional.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/password-policies/src/PasswordPolicy.ts, line 85:
<comment>This change silently alters the semantics of `forbidRepeatingCharactersCount: 0`. Previously `0` was a functional configuration producing the valid regex `(.)\1{0,}` (rejecting any repeated character) and, being falsy, suppressed the corresponding `get-password-policy` entry in `getPasswordPolicy()`. Now `0` is silently coerced to the default `3`, which relaxes the policy and, because `this.forbidRepeatingCharactersCount` becomes truthy, newly emits the policy entry where it was previously omitted. Since the corresponding setting is an `int` with no stated minimum, an admin who set `0` would see their policy quietly weakened without warning. Consider handling `0` explicitly (coerce only clearly-invalid values or clamp/log) rather than silently remapping to `3`, and document the behavior change if intentional.</comment>
<file context>
@@ -79,19 +79,25 @@ export class PasswordPolicy {
+ const safeForbidRepeatingCharactersCount =
+ typeof forbidRepeatingCharactersCount === 'number' &&
+ Number.isSafeInteger(forbidRepeatingCharactersCount) &&
+ forbidRepeatingCharactersCount >= 1 ? forbidRepeatingCharactersCount : 3;
+
+
</file context>
2 participants
summary
Sanitize
forbidRepeatingCharactersCountbefore interpolating it into the RegExp constructor.Invalid values (negative numbers,
0, floats,NaN, unsafe large numbers, non-numbers) now fall back to the default of3, preventing aSyntaxErrorthat could crash the workspace.Why
Fixes #41620
Changes
>= 1Testing
All 20 tests passed: