fix(auth): require authenticated access across browser and HTTP - #241
fix(auth): require authenticated access across browser and HTTP#241brianorwhatever wants to merge 4 commits into
Conversation
Resolve current and migrated identities at a shared authenticated boundary, protect private reads and scoped writes, and remove acting DIDs from clients. Track session revocation and expiry so reactive subscriptions lose access. Preserve compatibility names pending deployed-client confirmation. Add forged identity, scope, session lifecycle, shared editing, migration and upload tests, and document the coordinated rollout and remaining live verification gates.
|
🚅 Deployed to the boop-pr-241 environment in Friends
|
|
@pullfrog review this |
There was a problem hiding this comment.
Caution
Two changes create states a user cannot recover from on their own: unbookmarking a list stops working the moment its owner unpublishes, and a login restore that never settles leaves the app permanently stuck in "Authentication is already in progress".
Reviewed changes
Every private read and protected write moves behind a server-resolved actor, so caller-supplied DIDs no longer confer authority anywhere in the backend.
- Shared operation wrappers —
actorQuery/actorMutation/actorActioninconvex/lib/authenticated.tsresolve the actor, enforce a scope, verify identity assertions, and authorize declared resources before the business handler runs, exporting apublic/internalpair per operation. - Database-backed sessions — a new
accessSessionstable plusconvex/actorSession.tsvalidate JWTs against stored rows so logout, revocation, and expiry invalidate live reactive subscriptions rather than just clearing client state. - Identity arguments removed —
ownerDid,userDid,checkedByDidand friends are stripped from operation args across ~40 backend modules; ownership now derives fromctx.actor. - Resource authorization —
authorizeResourcesinconvex/lib/permissions.tsresolves items and anchors back to their list and requires ownership or an active publication. - Client adapters —
src/lib/authenticatedConvex.tswrapsuseQuery/useMutation/useActionto inject the session token and strip legacy identity fields, gated by a generated 132-entry registry that CI checks for staleness. - Public auth entry points stubbed —
convex/authSessions.tsandconvex/auth.tsreplace their public session/user functions with unconditional throws directing callers to the HTTP login endpoints. - Storage path hardening —
isDirectChildKeyconstrains site upload and asset keys to a direct child of the actor's own prefix. - Boundary test suite —
scripts/auth-boundary.test.mjsbundles and invokes the real handlers with real signed JWTs against an in-memory database with working index predicates; these tests can genuinely fail.
⚠️ The compatibility layer does not keep un-upgraded clients working
The PR body and docs/authentication-rollout.md frame the retained public names as compatibility, but they are not backward compatible in practice. convex/authSessions.ts re-adds createSession, getSession, markSessionVerified and deleteSession as stubs that throw unconditionally, and convex/auth.ts does the same for upsertUser. Any deployed client still calling those names hard-fails at the first login attempt rather than degrading.
The same holds for every converted operation: Convex arg validators reject undeclared args, so an old client sending userDid gets a validation error, and a client sending no authToken gets an auth error. The retained names preserve the API surface, not the behavior.
The rollout doc is explicit that the deployed browser/iOS/Android/integration inventory is not yet confirmed, and the PR is correctly marked draft. The gap worth closing before cutover is that nothing in the code enforces the gate — there is no version check, no feature flag, and no staged path. Whoever merges this can deploy it without the inventory ever being confirmed.
# Rollout sequencing has no enforcement mechanism
## Affected sites
- `convex/authSessions.ts` — public session functions replaced with unconditional throws
- `convex/auth.ts` — `upsertUser` replaced with an unconditional throw
- `docs/authentication-rollout.md` — states the client inventory is unconfirmed
## Required outcome
The cutover cannot reach production before the deployed-client inventory is confirmed, and the confirmation is recorded somewhere durable rather than in a doc.
## Open questions for the human
- Is there a mechanism to hold this at staging, or is the draft status the only gate?
- iOS and Android ship a webview over the same bundle, but the native shell version still gates which bundle loads — has the minimum shipped version been checked?
- Are there third-party or scripted HTTP integrations calling the Convex function endpoints directly that would not appear in an app-version inventory?
⚠️ Removing the legacy-DID migration branch closes account-migration onboarding
upsertUserInternal previously had two branches this PR deletes: migrating a legacy DID onto a Turnkey account, and linking Turnkey to an existing user found by DID. In their place is if (args.legacyDid) throw new Error("Identity migration requires verified account linking").
Existing legacyDid links on user rows keep working — authorize and isResourceOwner both consult them — so already-migrated users are unaffected. What is gone is the ability to create a new one. startOtp in src/hooks/useAuth.tsx still accepts a legacyDid parameter and the OTP flow still threads it through, so the client-side entry point survives while the server-side path now throws.
This reads as intentional (the old path let a caller claim any DID), but there is no replacement flow in this PR, and a user mid-migration hits a bare error string.
# Legacy-DID migration is closed with no replacement path
## Affected sites
- `convex/auth.ts` — `upsertUserInternal`, legacy-DID migration and DID-linking branches removed
- `src/hooks/useAuth.tsx` — `startOtp(email, legacyDid?)` still accepts and forwards `legacyDid`
## Required outcome
Either a verified account-linking flow exists, or the client stops offering migration and the dead `legacyDid` parameter is removed so users cannot reach the throw.
## Open questions for the human
- Is closing new migrations intended for this PR, or is a verified-linking flow expected to land alongside it?
- How many users are still un-migrated? If the answer is zero, removing the client-side `legacyDid` plumbing is the cleaner fix.
ℹ️ No owner or retirement date for the compatibility surface
convex/lib/clientAuth.ts lists ten identityAssertionFields accepted-and-ignored on every operation, commented "retained until deployed client versions are confirmed". checkAssertions validates them against the resolved actor, so they are not a security hole — but they are ten optional string args on the public signature of ~130 operations, and the client adapter strips them on the way out, so nothing this PR ships actually sends them.
The stubbed public names in convex/authSessions.ts and convex/auth.ts are in the same position. docs/authentication-rollout.md says to retire them "only after supported clients have migrated" without naming who checks or when.
# Compatibility surface has no removal trigger
## Affected sites
- `convex/lib/clientAuth.ts` — `identityAssertionFields`
- `convex/authSessions.ts`, `convex/auth.ts` — throwing stubs
- `docs/authentication-rollout.md` — retirement described but unscheduled
## Required outcome
A tracked follow-up with an owner and a concrete trigger, so this does not become permanent surface area by default.
ℹ️ Nitpicks
src/pages/PriorityFocus.tsx—userDidandlegacyDidare removed fromPriorityItem's destructure butuserDid: stringremains in its prop type, so the type still requires a prop the component ignores.convex/bitcoinAnchors.ts— the spread...ctx.credentials,around new line 247 is indented one level deeper than its siblings.- The PR reports
npm run lintat 49 errors (down from 51). Not introduced here, and the count moved the right way, but a boundary change of this size lands on a codebase where lint is not a signal.
Claude Opus | 𝕏
There was a problem hiding this comment.
Reviewed changes
- Re-read the two new commits (7c904d1 → afb3e6f) against the full 128-file diff, then verified each claimed fix against the files at HEAD rather than trusting the range-diff rendering.
- Confirmed all five prior review threads are genuinely addressed and retired them:
SharedListResourcenow gates ontokenwith a sign-in nudge and arole="alert"rollback path;unbookmarkList/isBookmarked/getPublicationStatusdropped their caller-supplieduserDidand checkctx.actorinstead;accessSessionsgained aby_expires_atindex plus a boundedcleanupExpiredSessionscron;withAuthTimeoutbounds theestablishcall on both the restore and OTP paths. - Traced the new
AuthError extends ConvexErrorboundary throughconvex/lib/authError.ts,actor.ts,session.ts,permissions.ts,httpResponses.tsandsrc/lib/sync.ts, including theestablishInternallazy-registration path for pre-rollout HTTP JWTs and the revoked-tombstone guard that stops a logged-out token re-establishing itself. - Audited the release gate (
scripts/check-authentication-cutover.mjs,release/authentication-cutover.json,railway.json,deploy-convex.yaml) for fail-open paths, and confirmedRAILWAY_ENVIRONMENT_NAMEis the variable Railway actually injects at build time. - Checked that the tests added in this delta can fail:
scripts/sync-auth.test.mjs,scripts/shared-list-auth.test.mjs,scripts/authentication-cutover.test.mjs, and the new bookmark/cleanup/ConvexError-round-trip cases inscripts/auth-boundary.test.mjs.
Important
The gate, the session lifecycle and the resource-declaration model all hold up. One issue introduced by this commit is worth resolving before merge: a permanently-denied offline mutation can wedge the entire sync queue with no recovery path for the user.
🚦 Rollout sequencing
The gate fails closed on every path I could enumerate — GitHub Actions never passes --railway, so needsApproval is unconditionally true there; process.exitCode = 1 does short-circuit the && in railway.json; the check runs before npm ci and before deployment secrets are used; and no other workflow deploys Convex (mobile-build.yml only runs convex codegen). docs/authentication-rollout.md states the production block is intentional, which resolves the main question. Two residual points:
- The doc says merging "will intentionally fail production deployment", but the gate is scoped to every push touching
convex/**, not to the cutover. An unrelated Convex hotfix after this merges will fail with an error about authenticated-client rollout evidence. Worth a sentence in the doc so the next person isn't confused, or scoping the trigger. checkApprovalonly requires non-empty strings andapprovedBy === record.owner— both fields live in the same file, and whoever opens a PR can set them. There is no CODEOWNERS entry onrelease/, so "only the named release owner may flip this" rests entirely on branch protection configured outside this repo. Worth confirming that protection exists, since this gate is the only thing between a breaking auth change and production.- Minor: the
^(boop-pr-\d+|staging|development)$allowlist encodes a Railway environment-naming convention that lives in the Railway project, not here. If preview environments are named differently, previews get needlessly blocked — fail-closed, so not a security issue, but a surprising one.
ℹ️ Nitpicks
convex/attachments.ts:60,92,126and several"Not authorized to …"throws inconvex/auth.tsare still plainError, so they surface as 500 throughhandlerErrorResponseand are invisible toaccessFailurein the sync queue. Either convert them or note why they're deliberately out of scope.- Leftovers from the mechanical argument removal:
src/pages/NoteEditor.tsx:28has a stray trailing comma and padding ({ itemId: itemId as Id<"items">, }), and blank lines were left inside object literals insrc/components/Attachments.tsx(three call sites),convex/agentReadHttp.ts, andconvex/referrals.ts(a doubled blank line where the_generated/serverimport was). - Removing the
AuthErrorbranch from every*Http.tshandler changes authorization denials from a uniform 401 to 403 (handlerErrorResponsenow splits on message, andscripts/sync-auth.test.mjs:47-48pins that). That reads like the intended REST semantics, but it is a behavior change for any API-key consumer branching on 401 — worth a line inAPI.mdif it isn't there already.
Claude Opus | 𝕏
There was a problem hiding this comment.
ℹ️ The head-of-line block is genuinely fixed. Two smaller things worth a look, both inline.
Reviewed changes
- Read the range-diff for 99e995f against the full 129-file diff, then verified each claimed fix against the files at HEAD rather than trusting the range-diff rendering.
- Confirmed all four prior threads are addressed and retired them:
authorizeResourcesnow throws a uniformresourceUnavailable()on every branch (lines 57/63/69/73/79/82),getUserBookmarkIdsunionsdid+legacyDidwith dedupe and has a real test inscripts/bookmark-identities.test.mjs,docs/authentication-rollout.mdnow states outright that the gate blocks every subsequent production Convex deploy including unrelated hotfixes, and the sync queue no longerreturns out of the loop on a resource denial. - Traced the new
FORBIDDENcode end to end:convex/lib/authError.ts→resourceUnavailable()→authorizeResources/getItemForSync/ theconvex/auth.tscompat shims →authErrorDataon the client. Checked that no credential failure was reclassified intoFORBIDDEN—requireSession,authenticate,establishOperationandrequireScopeall still emitUNAUTHORIZED/INVALID_TOKEN, so the "sign in again" toast still means what it says. - Verified the retry accounting the fix depends on:
getQueuedMutations()re-reads IndexedDB each pass andupdateMutationRetrypersists, so aFORBIDDENmutation's budget survives restarts and only that mutation is discarded. The newscripts/sync-auth.test.mjscases assert on applied-mutation contents, exactretryCountand queue length, so they can fail. - Re-checked the existence-oracle surface across
permissions.ts,lists.ts,items.ts,attachments.ts,comments.ts,assignees.tsand the*Http.tshandlers.getListandlistWithItemsOperationreturnnulluniformly,handlerErrorResponsemapsFORBIDDENto a flat 403 before any message matching, and the leftover plain-Error"not found" throws inlists.ts/assignees.tsare unreachable becauseauthorizeResourcesalready covers those IDs. One handler is not covered — see thedemoteItemcomment.
🔁 Sync queue
The wedge is fixed, but closing it made the deletion path unreachable. getItemForSync now throws resourceUnavailable() where it used to return null, and authorizeResources throws the same for a missing item or list — so a remotely-deleted item no longer produces "Item not found", it produces FORBIDDEN. Details inline at src/lib/sync.ts:243. This is a UX regression, not data loss: the edit is still discarded, just five sync cycles later and behind a misleading message.
ℹ️ Nitpicks
- The
FORBIDDENbranch (src/lib/sync.ts:229-240)continues with nothis.delay(...), unlike the generic retry path below it which backs off 1s/2s/4s/8s/16s.sync()is called fromuseOffline.tsx:79on everynetworkStatusChangeand frommanualSyncbehind the "Sync now" button, which re-enables as soon asisSyncingclears — and aFORBIDDEN-only pass now completes near-instantly. Five taps in a few seconds, or Wi-Fi/cellular flapping, burns the whole budget against a denial that might have resolved a minute later. ReusingRETRY_DELAYS[mutation.retryCount]here would make the two paths symmetric. checkAssertionsinconvex/lib/authenticated.tsthrowsAuthError("Identity assertion does not match authenticated account", "UNAUTHORIZED"), buthttpResponses.ts:80matchesIdentity assertionand returns 403 — the structured code and the HTTP status disagree. Pre-existing in the regex, but this commit is the one that writes the 401/403 split down as a contract inAPI.md, so it is now a documented inconsistency rather than an unnoticed one.convex/comments.ts:102deleteCommentdeclaresresources: () => ({})and authorizes by hand, throwing plainError("Comment not found")vsError("Not authorized to delete this comment")for a guessedcommentId. Production redaction collapses both to "Server Error", so this is not a prod oracle — but it is the one file in the conversion that still relies on redaction rather than onresourceUnavailable().
Claude Opus | 𝕏
|
Reply to the initial review:
Addressed in afb3e6f: the configured Convex and Railway production paths check
Removed the unused
The rollout document now tracks The unused PriorityItem prop and credential-spread indentation were also fixed in afb3e6f. Production approval and live validation remain outstanding; none of these code fixes claim deployed-client confirmation. |
|
Reply to the follow-up review:
Addressed in 99e995f: the rollout document explicitly says the pending gate blocks every subsequent production Convex deployment and Railway build, including unrelated hotfixes. A later full deployment contains this authentication change, so limiting approval to the latest changed files would bypass the gate. PR/staging deployments can collect evidence while production remains unapproved.
It does not currently provide enforced owner-only review. The 2026-09-09 inspection found The documented non-production Railway names are
The attachment and account authorization throws identified here were converted to structured resource denials in 99e995f. That commit also removed the listed argument-removal formatting leftovers and documented the HTTP 401/403 split in API.md. The offline queue now processes later edits after a resource denial and exhausts only the denied edit's retry budget; credential failures preserve the queue. Commit 0425318 adds retry backoff and makes the missing-or-inaccessible message accurate. |
|
Reply to the latest review:
Addressed in 0425318. Retryable resource denials use the existing 1/2/4/8/16-second backoff; exhausted edits are discarded without another delay. Deterministic tests check the delay sequence, preserved progress for later edits, and that rapid sync attempts cannot spend additional retries while backoff is pending. The obsolete deletion branches and impossible missing-item test fixture are removed; the message now covers resources that may have been removed or access that is unavailable.
HTTP responses now honor structured error codes before inspecting legacy error messages. An identity assertion that does not match the authenticated account remains
Comment deletion now returns the same structured resource denial for missing comments, orphaned comments, and comments the caller cannot delete. Author, migrated-author, list-owner, and shared-editor deletion behavior remains intact. The inline demotion issue is fixed by declaring the new parent as a resource before the handler runs. Validation for 0425318: Node 232/232 and Bun 256/256 tests pass; TypeScript, the production build, and lint on the changed source files pass. The build retains the existing large-chunk warnings. Production cutover remains unapproved. |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Re-read the one new commit (0425318, 6 files, +98/−31) against the full 129-file diff, verified each claimed fix against the files at HEAD rather than trusting the range-diff rendering, and dispatched three independent specialist passes — an HTTP-status-contract lens, an offline-sync-queue lens, and an authorization lens. All three returned clean negatives.
- Closed the
demoteItemparent oracle —convex/items.ts:942now declaresresources: args => ({ items: [args.itemId, args.newParentId] }), soauthorizeResourcesresolves the new parent to its owning list before the handler runs. The distinguishable"Parent item not found"/"Items must be in the same list"probe is gone, and no authorization outcome changed: legitimate demotion always targets a parent in the same already-owned list. - Unified
deleteComment's three error paths —convex/comments.ts:111,116,136replace"Comment not found"/"Item not found"/"Not authorized to delete this comment"with a singleresourceUnavailable(). The gating condition (isAuthor || canUserEditList) is untouched, so this closes the enumeration oracle without changing who can delete what. - Corrected the 401/403 split —
convex/lib/httpResponses.ts:76-79hoists the structuredAuthErrorbranch above the message regexes. Exactly one status flipped:checkAssertions(codeUNAUTHORIZED) went 403 → 401. That resolves the nit in the right direction —API.md:38-42defines 401 as a credential problem and 403 as scope-or-resource, so the old regex-driven 403 was the latent bug. Every otherAuthErrorcall site acrossactorSession.ts,lib/auth.ts,lib/actor.tsandlib/session.tsproduces the same status as before. - Removed the dead deletion paths in the sync queue —
src/lib/sync.tsdrops both the"Item not found"catch arm andcheckForConflict'sif (!serverItem)guard, and softens the two toasts to "may have been removed or access is unavailable". ConfirmedgetItemForSync(convex/items.ts:588-597) genuinely cannot returnnullany more, so the guard removal is safe rather than merely type-clean. - Backed off the
FORBIDDENretry path —src/lib/sync.ts:237-238reusesRETRY_DELAYS[mutation.retryCount], making the two retry paths symmetric. Checked that this does not reopen the head-of-line block: it is still acontinue, so healthy edits queued behind a denial apply in the same pass, and the per-mutation budget is unchanged. - Confirmed the new tests can fail — the
demoteItemanddeleteCommentcases inscripts/auth-boundary.test.mjsexercise genuinely distinct inputs against the fixture (I1→L1owned by the actor,I2→L2owned bydid:other), not two spellings of the same case. Inscripts/sync-auth.test.mjs,assert.deepEqual(delays, [1000,2000,4000,8000,16000])is exact and driven by an override of the realthis.delaycall, and theofflinefixture now returnsstructuredClone(s.queue)so a mutated in-loop copy can no longer mask a persistence bug.
ℹ️ Nothing left that another review pass can settle
Both threads from the prior review are genuinely addressed and have been retired. The remaining gate on this PR is operational, not code: release/authentication-cutover.json still ships all-null, checkApproval accepts any non-empty approvedBy === owner pair editable in this same PR, and docs/authentication-rollout.md records that main had no protection or CODEOWNERS as of 2026-09-09. So the deployed browser/iOS/Android/integration inventory and the reviewer enforcement behind that record are the merge blockers, and they are yours to confirm rather than mine to verify. This approval covers the code.
Claude Opus | 𝕏
The code review is complete, but these confirmations are still missing. The release record has no client inventory, staging evidence, or approval. The documented repository inspection found no enforced owner-only review, and this task has not changed live repository policy. This needs a release-owner decision: hold merge until deployed-client/staging evidence is recorded and required owner review is enforced, or explicitly accept merging the code while production stays blocked and record acceptance of the approval record's current enforcement limits. The first option satisfies the stated merge gate; the second allows code integration but does not authorize production cutover or retirement of compatibility entry points. I recommend holding merge until those confirmations are complete, consistent with the rollout document. No production approval, client-version evidence, or owner-review enforcement is being claimed here. |

Summary
Private reads and protected writes now require a verified human session or a scoped agent key across browser, mobile, and HTTP callers. Asserting an owner's current or legacy DID no longer grants access to their data or permission to change it.
Authorization and compatibility
Canonical login-account selection and duplicate-email signup protections remain intact.
Login and offline recovery
Session establishment during restore and OTP verification has a 15-second timeout, and failed verification clears local credentials. Login and logout are serialized. Shared-list controls require sign-in and display write failures.
Offline session failures pause synchronization without spending retry counts or discarding remaining edits. Resource denials use the affected edit's existing five-retry budget and backoff, allow later edits to run, and discard the denied edit with a warning when retries are exhausted. Successful edits do not erase the failure status. Missing and inaccessible resources have identical responses, so these errors do not reveal private resource existence.
Migrated bookmark IDs include both account identities without duplicates. Users can inspect and remove their own bookmarks after unpublishing without gaining access to the private list.
Related: #235
Validation
The security reproduction and regression tests exercise handlers, signed JWTs, HTTP dispatch, and browser components locally. Live OTP delivery, subscription invalidation timing, object storage, supported native releases, and coordinated staging behavior remain unverified.
Rollout gate
This PR is open for review, and Railway has deployed a PR preview. Production cutover remains unapproved; this work has not deployed the production authentication change.
Confirm deployed browser, iOS, Android, and integration versions and record staging evidence before merging. The pending release attestation blocks every configured production Convex deployment and Railway build after merge, including unrelated hotfixes, until the evidence is recorded. Complete the gate first or ship urgent unrelated fixes before this PR. Manual production deployments must apply the same check.
The record is an auditable attestation, not authenticated owner approval.
maincurrently has no branch protection, repository rulesets, orCODEOWNERS; required owner review is not enforced. No live repository policy was changed.The rollout checklist covers the coordinated backend/client release and later compatibility retirement. Retire entry points only after supported versions are confirmed and the required observation window shows no compatibility use.
Post-Deploy Monitoring & Validation
Release owner: Brian (
brianorwhatever). Monitor login success, private-query failures, offline retries, HTTP 401/403 rates, and session-expiry scheduler failures for the first 30 minutes and again at 24 hours. Pause rollout if supported clients cannot authenticate or any private-access bypass succeeds; retain the authenticated boundary while repairing the release.Note
Require authenticated access across browser and HTTP operations
accessSessionstable andactorSessioninternal functions to validate JWT tokens against stored session records and resolve the authenticated actor.actorQuery,actorMutation, andactorActionwrappers that authenticate the actor, enforce scopes, and authorize resources before executing business logic.ownerDid,userDid) from backend operations and derives ownership from the server-resolved actor.Macroscope summarized 7c904d1.