feat: 2fa backup codes#1
Conversation
Co-authored-by: Peer Richelsen <peeroke@gmail.com>
|
@CodeAnt-AI: review |
|
CodeAnt AI is running the review. |
| // FIXME: this passes even when switch is not checked, compare to test | ||
| // below which checks for data-state="checked" and works as expected | ||
| await page.waitForSelector(`[data-testid=two-factor-switch]`); | ||
| await expect(page.locator(`[data-testid=two-factor-switch]`).isChecked()).toBeTruthy(); |
There was a problem hiding this comment.
Suggestion: The assertion uses expect(page.locator(...).isChecked()).toBeTruthy() without awaiting isChecked(), so expect receives a Promise (always truthy) and the test passes even when the switch is not actually checked; instead, you should assert directly on the locator being checked (or on the awaited boolean) so the test correctly fails when 2FA is not enabled. [logic error]
Severity Level: Major ⚠️
- ❌ 2FA enablement test passes even when toggle unchecked.
- ⚠️ Regressions in 2FA UI state may go undetected.
- ⚠️ Weakens reliability of 2FA end-to-end coverage.| await expect(page.locator(`[data-testid=two-factor-switch]`).isChecked()).toBeTruthy(); | |
| await expect(page.locator(`[data-testid=two-factor-switch]`)).toBeChecked(); |
Steps of Reproduction ✅
1. From the repo root `/workspace/codeant-cal_dot_com`, run the Playwright test file
`apps/web/playwright/login.2fa.e2e.ts` (e.g. via `npx playwright test
apps/web/playwright/login.2fa.e2e.ts`), which defines the "2FA Tests" suite at lines
16–162 in `apps/web/playwright/login.2fa.e2e.ts`.
2. The test `"should allow a user to enable 2FA and login using 2FA"` at lines 20–88
executes the `"Enable 2FA"` step (lines 22–55), navigating to
`/settings/security/two-factor-auth`, clicking the two-factor switch, and completing OTP
setup.
3. After submitting a valid OTP, the test reaches the assertion block at lines 50–53: it
waits for the selector `[data-testid=two-factor-switch]` and then runs `await
expect(page.locator(`[data-testid=two-factor-switch]`).isChecked()).toBeTruthy();`, where
`locator().isChecked()` returns a `Promise<boolean>`.
4. Because the Promise is not awaited, the `expect` call at line 53 receives a Promise
object (always truthy) instead of a boolean; thus, even if the two-factor switch is not
actually checked in the UI, the assertion passes and the test will not fail, which matches
the FIXME comment at lines 50–51 indicating the test incorrectly passes.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** apps/web/playwright/login.2fa.e2e.ts
**Line:** 53:53
**Comment:**
*Logic Error: The assertion uses `expect(page.locator(...).isChecked()).toBeTruthy()` without awaiting `isChecked()`, so `expect` receives a Promise (always truthy) and the test passes even when the switch is not actually checked; instead, you should assert directly on the locator being checked (or on the awaited boolean) so the test correctly fails when 2FA is not enabled.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| <button className="text-emphasis h-9" type="button" onClick={() => toggleIsPasswordVisible()}> | ||
| <button | ||
| className="text-emphasis h-9" | ||
| tabIndex={-1} |
There was a problem hiding this comment.
Suggestion: Setting the password visibility toggle button's tabIndex to -1 makes it unreachable via keyboard navigation, which breaks keyboard accessibility for this control and prevents non-mouse users from toggling password visibility. [logic error]
Severity Level: Major ⚠️
- ⚠️ Keyboard-only users cannot toggle password visibility.
- ⚠️ Affects login password field on `/auth/login`.
- ⚠️ Affects password change form at `settings/security/password`.
- ⚠️ Affects 2FA enable/disable password confirmation modals.| tabIndex={-1} |
Steps of Reproduction ✅
1. Render the shared PasswordField component from
`packages/ui/components/form/inputs/Input.tsx:21-64`, which internally renders the toggle
button defined at lines 47-51 with `tabIndex={-1}`.
2. Use any screen that consumes PasswordField, for example the login page at
`apps/web/pages/auth/login.tsx:193-209`, where the password input is rendered via
`<PasswordField id="password" ... {...register("password")} />`.
3. In a browser, navigate to `/auth/login` so that the form from `login.tsx` is rendered,
then use only the keyboard (Tab/Shift+Tab) to move focus through the form fields (email,
password, forgot-password link).
4. Observe that the password visibility toggle button rendered by PasswordField (the
`<button>` at `Input.tsx:47-51`) never receives focus via Tab because `tabIndex={-1}`
removes it from the tab order, so keyboard-only users cannot activate it to toggle
password visibility.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/ui/components/form/inputs/Input.tsx
**Line:** 49:49
**Comment:**
*Logic Error: Setting the password visibility toggle button's `tabIndex` to `-1` makes it unreachable via keyboard navigation, which breaks keyboard accessibility for this control and prevents non-mouse users from toggling password visibility.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Nitpicks 🔍
|
|
CodeAnt AI finished running the review. |
User description
Test 3
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/cal.com-coderabbit#3
CodeAnt-AI Description
Add single-use backup codes for two-factor authentication and a "lost access" recovery flow
What Changed
Impact
✅ Fewer account lockouts when users lose their authenticator✅ Clearer 2FA recovery with copy/download of backup codes✅ Shorter recovery during 2FA disable and login💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.