fix(auth): stop signup auth-watcher from racing past email verification - #4528
Conversation
watch.auth called pushAfterAuth() as soon as signup() flipped
authStore.auth to true. That pre-flush watcher job lands on an earlier
microtask than validate()'s own `await authStore.signup()` continuation,
so on the organizations-disabled path (including serverConfig === null
after a failed config fetch) it could navigate into the app before
validate() reached the emailVerificationRequired branch and set
signupStep to 'emailVerification' — silently skipping the verification
gate.
validate() already calls pushAfterAuth() in every terminal branch, so
the watcher's fast-path push was redundant as well as unsafe; removed
it (and the now-unused `auth` computed) entirely. OAuth's full-page
href redirects (handled in created()) are unaffected.
Adds a regression suite that runs the real Pinia auth store (the
existing suite mocks a static { auth: false } store, so the watcher
never fired and the race was untested) and confirms the fix against
both the organizations-disabled and serverConfig-null paths.
Closes #4437
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe signup view no longer redirects from authentication watcher updates. A real-store Vitest suite verifies that email verification remains visible and navigation does not occur when server configuration succeeds or fails. ChangesSignup email verification flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SignupView
participant AuthStore
participant Router
SignupView->>AuthStore: submit signup
AuthStore-->>SignupView: emailVerificationRequired
SignupView->>SignupView: show emailVerification step
SignupView-->>Router: no navigation
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/auth/tests/auth.signup.view.race.unit.tests.js`:
- Around line 22-46: Add JSDoc headers to each new mock factory in the vi.mock
calls and to the ability.can callback, including a one-line description and an
`@returns` annotation with the appropriate type; include `@param` annotations if any
callback arguments are present. Leave test and lifecycle callbacks unchanged, as
the exemption applies only to those APIs.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f63165ad-f85c-4be7-931c-4de235eb1e86
📒 Files selected for processing (2)
src/modules/auth/tests/auth.signup.view.race.unit.tests.jssrc/modules/auth/views/signup.view.vue
💤 Files with no reviewable changes (1)
- src/modules/auth/views/signup.view.vue
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4528 +/- ##
=======================================
Coverage 99.57% 99.57%
=======================================
Files 36 36
Lines 1411 1411
Branches 439 439
=======================================
Hits 1405 1405
Misses 6 6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ate() (#4534) * fix(auth): stop signin auth-watcher from racing navigation past validate() watch.auth was the sole navigator on signin: it pushed the redirect route as soon as signin() flipped authStore.auth to true. That pre-flush watcher job lands on an earlier microtask than validate()'s own `await authStore.signin()` continuation, decoupling navigation from the flow that triggered it. Any future step added after the awaited signin() (MFA challenge, forced password rotation, consent interstitial) would be silently skipped before it could run - reproducing #4437's bug class (fixed for signup in #4528) on this view. Moves navigation into validate()'s success path, explicitly after authStore.signin() resolves and authStore.auth is true, preserving the redirect-query behavior byte-for-byte (including the startsWith('/') open-redirect guard). Removes the now-unused watch.auth handler and auth computed. OAuth's full-page href redirects (handled in created()) are unaffected. Adds a regression suite that runs the real Pinia auth store (the existing suite mocks a static { auth: false } store, so the watcher never fired and the race was untested) covering the default-route branch, the redirect-query branch, the non-"/"-prefixed redirect guard, and the failed-signin (no navigation) branch. Closes #4533 * refactor(simplify): extract submitSignin helper in the #4533 race suite Cuts the fill-email/fill-password/validate/flushPromises sequence that repeated verbatim across all 4 tests into one submitSignin() helper. Keeps each scenario as its own named it() block (each documents a distinct, security-relevant redirect case) rather than collapsing into a test.each table.
1 participant
Bug
On a signup that requires email verification, the user could be dropped straight into the app WITHOUT ever seeing the "check your inbox" step — when organizations are disabled, or when the server-config fetch returned null.
signup.view.vue'swatch.authcalledpushAfterAuth()as soon as the store flippedauth = true(set synchronously insidesignup()). That pre-flush watcher job lands on an earlier microtask thanvalidate()'s ownawait authStore.signup()continuation, so on the!serverConfig?.organizations?.enabledpath (also true whenserverConfig === nullafter a failed config fetch) the watcher could navigate into the app beforevalidate()reached theemailVerificationRequiredbranch and setsignupStep = 'emailVerification'— silently skipping the verification gate. No router guard enforces email-verified downstream, so nothing caught it.The existing unit suite mocked
useAuthStorewith a static{ auth: false }object — the component'sauthcomputed never changed, so the watcher never fired in tests, masking the race entirely.Fix
validate()already callspushAfterAuth()in every terminal branch where navigation should happen, so the watcher's fast-path push was redundant as well as unsafe. Removed thewatch.authhandler (and the now-unusedauthcomputed) entirely. OAuth's full-page href redirects (handled increated()) are unaffected.Test
Added
auth.signup.view.race.unit.tests.js, which runs the real Pinia auth store (only network/analytics/ability are mocked) soauthflips via genuine reactivity, reproducing the actual microtask race. Both tests:router.push('/tasks')fires before the verification step is shown)Covers: organizations disabled +
emailVerificationRequired, and theserverConfig === null(failed config fetch) path.Closes #4437
Summary by CodeRabbit
Bug Fixes
Tests