fix: enforce the profile gate on session restore - #301
Conversation
The profile-required intent was only ever applied by the interactive login path. restoreSession() returns a user without consulting deps.profiles, so an identity restored from localStorage with no profile document was never sent to /profile/create — the invariant held on login but silently not on reload, with app/user/page.tsx the only page redirecting on its own. AuthProvider now re-derives the intent after restore and applies it through the same applyIntent. The gate yields to the username gate when the restored session has no username and yappr_skip_dpns is unset, skips routes that intentionally host profile-less users (/profile/create, /dpns/register, /login, /welcome), and fails open: the profile lookups swallow query errors and return null, so a redirect additionally requires identityService.getIdentity to succeed. profiles.hasProfile is extracted to an exported hasYapprProfile so both paths ask the identical question. Vendored platform-auth is untouched.
|
Warning Review limit reachedNext included review available in 40 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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 |
|
⛔ Blockers found — Opus deferred (commit 5a0d001) |
Deploying yappr with
|
| Latest commit: |
5a0d001
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://aaba9bde.yappr.pages.dev |
| Branch Preview URL: | https://fix-restore-session-profile.yappr.pages.dev |
Deploying yappr-v2 with
|
| Latest commit: |
5a0d001
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f12cc6d1.yappr-v2.pages.dev |
| Branch Preview URL: | https://fix-restore-session-profile.yappr-v2.pages.dev |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The restore-time check addresses the normal-route gap, but two flaws prevent it from enforcing the stated invariant safely: exemptions permanently bypass the one-shot check, and profile-query failures are still treated as confirmed profile absence. These paths need correction and focused regression coverage before merge. Source: Codex reviewer (exact backend model ID was not supplied in the evidence); final verifier grok-4.5. Orchestration-only, not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `contexts/auth-context.tsx`:
- [BLOCKING] contexts/auth-context.tsx:146: Re-evaluate the gate after leaving an exempt route
This early return permanently skips the profile check for this restored session because `restoreAndGate` runs only once in the `[controller]` effect, while the root-level `AuthProvider` persists across client-side navigation. For example, a profile-less user with a username can restore on `/login`, return here, and then be pushed to `/feed` by `app/login/page.tsx`; `/welcome` also links directly to `/feed`, and `/dpns/register` redirects a user who already has a username there. No pathname change causes the gate to run again, so the user can continue browsing without a profile. Preserve or re-evaluate the restored user's profile status when leaving an exempt route, while suppressing navigation only while the user is actually on profile creation or the active username-registration flow.
- [BLOCKING] contexts/auth-context.tsx:150-158: The identity probe cannot distinguish a failed profile query
Both `unifiedProfileService.getProfile` and `profileService.getProfile` catch query errors and return `null`, while `identityService.getIdentity` checks a separate identity endpoint. A successful identity fetch therefore does not establish that either profile query completed successfully. If the unified-profile query fails but the legacy query successfully finds nothing—or both profile queries encounter a contract-specific failure—an existing unified-profile user is incorrectly redirected to profile creation as soon as the identity endpoint responds. Use strict or result-bearing profile lookups that preserve lookup failures, and redirect only when both profile document queries conclusively succeed and report absence.
- [SUGGESTION] contexts/auth-context.tsx:202-214: Add regression coverage for the restored profile gate
The existing smoke project restores no authenticated session, and the write identities already have profiles, so neither suite exercises the newly added redirect branch. Add focused coverage for a restored profile-less session on a gated route, a profiled session, a profile-query failure, and navigation from an exempt route to a gated route. The latter two cases would directly prevent regressions of the blocking issues above.
| const skipDPNS = sessionStorage.getItem(scopedKey('yappr_skip_dpns')) === 'true' | ||
| if (!user.username && !skipDPNS) return null | ||
|
|
||
| if (isProfileOptionalRoute(window.location.pathname)) return null |
There was a problem hiding this comment.
🔴 Blocking: Re-evaluate the gate after leaving an exempt route
This early return permanently skips the profile check for this restored session because restoreAndGate runs only once in the [controller] effect, while the root-level AuthProvider persists across client-side navigation. For example, a profile-less user with a username can restore on /login, return here, and then be pushed to /feed by app/login/page.tsx; /welcome also links directly to /feed, and /dpns/register redirects a user who already has a username there. No pathname change causes the gate to run again, so the user can continue browsing without a profile. Preserve or re-evaluate the restored user's profile status when leaving an exempt route, while suppressing navigation only while the user is actually on profile creation or the active username-registration flow.
source: ['codex']
| // `hasYapprProfile` cannot tell "no profile" apart from "the query failed" — | ||
| // both surface as false. `identityService.getIdentity` rethrows instead of | ||
| // swallowing, which makes it a usable liveness probe: let a failure reject so | ||
| // the caller fails open, and treat a missing identity as "not our call". | ||
| // `app/user/page.tsx` probes the same way before its own /profile/create push, | ||
| // though it goes further and logs the user out when the identity is gone — | ||
| // too destructive to do from a provider-wide effect. | ||
| const identity = await identityService.getIdentity(user.identityId) | ||
| if (!identity) return null |
There was a problem hiding this comment.
🔴 Blocking: The identity probe cannot distinguish a failed profile query
Both unifiedProfileService.getProfile and profileService.getProfile catch query errors and return null, while identityService.getIdentity checks a separate identity endpoint. A successful identity fetch therefore does not establish that either profile query completed successfully. If the unified-profile query fails but the legacy query successfully finds nothing—or both profile queries encounter a contract-specific failure—an existing unified-profile user is incorrectly redirected to profile creation as soon as the identity endpoint responds. Use strict or result-bearing profile lookups that preserve lookup failures, and redirect only when both profile document queries conclusively succeed and report absence.
source: ['codex']
| const restoreAndGate = async (): Promise<void> => { | ||
| const restoredUser = await controller.restoreSession() | ||
| if (cancelled || !restoredUser) return | ||
|
|
||
| const intent = await profileIntentForRestoredSession(restoredUser).catch((error) => { | ||
| // Fail open: a gate that cannot reach the network must never strand a | ||
| // user who does have a profile on /profile/create. | ||
| logger.error('Auth: Failed to evaluate the profile gate after session restore:', error) | ||
| return null | ||
| }) | ||
| if (cancelled || !intent) return | ||
|
|
||
| await applyIntentRef.current(intent) |
There was a problem hiding this comment.
🟡 Suggestion: Add regression coverage for the restored profile gate
The existing smoke project restores no authenticated session, and the write identities already have profiles, so neither suite exercises the newly added redirect branch. Add focused coverage for a restored profile-less session on a gated route, a profiled session, a profile-query failure, and navigation from an exempt route to a gated route. The latter two cases would directly prevent regressions of the blocking issues above.
source: ['codex']
2 participants
The inconsistency
The
profile-requiredauth intent was only ever applied by the interactive login path.PlatformAuthController.loginWithAuthKeyconsultsdeps.profiles.hasProfileand returns aprofile-requiredintent, whichapplyIntentincontexts/auth-context.tsxturns into a push to/profile/create.restoreSession()does none of that. It reads the session snapshot, checks that a private key is present, patches state, and returns the user — it never touchesdeps.profiles. So the invariant "a logged-in user without a profile document gets sent to profile creation" held on interactive login and silently not on page reload. The only page that compensated wasapp/user/page.tsx, which runs its own check.Why it matters
A restored session is the common case — it is what every page load goes through. An identity with no profile document could browse the whole app indefinitely as long as it never logged in interactively again, and every feature that assumes a profile exists had to defend itself individually. It also made the gate untrustworthy as a signal:
docs/TESTING.md§7 existed specifically to warn tests not to rely on it.The fix
AuthProvidernow re-derives the intent after a successful restore and applies it through the sameapplyIntent, so both paths share one intent → navigation mapping.profiles.hasProfilewas an inline closure in the adapters; it is extracted to an exportedhasYapprProfileso the restore path asks the byte-for-byte identical question the controller asks.vendor/platform-authis untouched — the whole fix lives in the app layer.The gate deliberately declines to fire in four cases:
withAuththat pushes to/dpns/register, so the profile gate bails when the restored session has no username andyappr_skip_dpnsis unset, rather than racing it to the router./profile/create,/dpns/register,/login,/welcomeare exempt. Matched as path suffixes so the/testingdeployment'sbasePathstill resolves, with the trailing slash stripped (trailingSlash: true). Re-checked after the lookups too, in case the user navigated during the round trip.unifiedProfileService.getProfileandprofileService.getProfileboth swallow query errors and returnnull, sohasYapprProfilereturningfalsemeans "no profile or the query failed". A redirect therefore additionally requiresidentityService.getIdentity(which rethrows rather than swallowing) to succeed — the same liveness probeapp/user/page.tsxapplies before its own/profile/createpush. A DAPI outage produces no redirect rather than a spurious one.The restore effect keeps its
[controller]dependency list — its cleanup callscontroller.dispose(), so re-running it would be destructive — and reachesapplyIntentthrough a ref instead of taking it as a dependency.What I verified
npm ci,npm run lint(no new errors or warnings on the touched files),npx tsc --noEmit(clean),npm run build(clean),npm run build:testing(clean).npx playwright test --project=smoke). Smoke is logged-out, sorestoreSessionreturnsnulland the gate never runs — this confirms no regression on the logged-out path.e2e/fixtures/auth.ts): it seedstesting:yappr_sessionas{identityId, balance: 0, publicKeys: []}— note no username — plus the private key, and setstesting:yappr_skip_dpns = "true". So the username-gate bail evaluates!username && !skipDPNS→true && false→false, meaning the gate does proceed rather than being skipped. It then callshasYapprProfile(identityId, undefined), which queries$ownerId == identityId. The provisioned bots have profiles, so it returnstrueand no redirect is issued. The write specs are unaffected. Storage scoping lines up: the fixture'sscopedKeyand the app's both resolve to thetesting:prefix.Known consequence, documented not papered over
A code review caught that this makes
e2e/write/post-lifecycle.spec.tsno longer self-bootstrapping for a brand-new pool slot. Its profile-creating test is third, but the two ahead of it navigate to/about/and/feed/, neither of which is gate-exempt — so on a slot with no profile document both get redirected and fail, and since the group ismode: 'serial', retries restart at test 1 and the profile-creating test never runs.I did not reorder the spec: the first test is what proves the bundle targets the test contracts, and creating a profile ahead of that check risks writing to production. The currently provisioned identities all have profiles, so the suite passes as-is.
docs/TESTING.md§7 is rewritten to describe the new behavior accurately, and the §4 provisioning runbook now says to create the profile by hand when adding a slot. The same applies after a chain rollback that wipes documents but leaves identities.Note
withAuthstill declares anallowWithoutProfileoption that nothing reads — it was dead before this change and remains dead. Wiring the gate throughwithAuthinstead would have covered only wrapped pages, missing/user,/about, and/explore, so I left it alone rather than widening the diff.🤖 Generated with Claude Code