Skip to content

Enforce authenticated identity on every public Convex entry point - #288

Merged
brianorwhatever merged 7 commits into
mainfrom
fix/236-authenticated-identity
Oct 8, 2026
Merged

brianorwhatever merged 7 commits into
mainfrom
fix/236-authenticated-identity

Conversation

@brianorwhatever

@brianorwhatever brianorwhatever commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Closes #236.

Summary

#241 already put an authenticated session/API-key boundary in front of most browser operations, so the issue's cited locations (items.checkItem with checkedByDid, lists.getUserLists, unauthenticated list reads) were already fixed on main. This PR finishes the job:

  • It inventories and classifies every public function, and adds a test that enforces the classification.
  • It closes the identity gaps the audit found. The most serious was an account-takeover path through DID re-mint.

Inventory

docs/authenticated-function-inventory.md lists all 172 public registrations:

Class Count Identity comes from
Actor-wrapped 156 Session record or API-key row, resolved inside the transaction
Self-authenticating 5 actorSession.*, auth.* (compatibility names)
Rejecting stubs 7 Throw for every caller
Intentionally public 4 No identity: published lists, public templates, display names, waitlist

scripts/public-function-boundary.test.mjs loads the real registrations and fails CI when:

  • a public function is unclassified;
  • a protected function accepts an anonymous, unknown-key, or asserted current/legacy-DID call. Each call uses validator-shaped arguments that name real fixture rows;
  • such a call reads beyond the credential tables, or writes, before rejecting;
  • a non-public HTTP route answers anonymous, forged-key or forged-bearer requests with anything other than 401/403, writes, or reads beyond the credential tables. /d/* may also read its public resolution tables.

I checked it against deliberately reintroduced gaps and it fails on them. Its HTTP pass found category and billing routes returning 500 for credential failures; those now return 401/403.

Gaps closed

  • DID re-mint takeover (critical). POST /api/user/remintDid accepted any did:webvh, including another user's, and applyRemint never checked uniqueness. Ownership is DID-based, so the caller became owner of the victim's lists and keys.
    • Re-mint and updateDID now require a did:webvh minted at the caller's own path, user-<sub-org>. That is what every client already sends.
    • Re-mint refuses a DID held by another account.
    • The publication prefix rewrite now matches only {oldDid}/….
    • updateDID no longer accepts did:key; only server-side login derives one.
  • Publication DID binding.
    • publishList requires webvhDid to be the publisher's own {did}/resources/list-{id}.
    • The /d/* resolver fallback now requires the list owner to control the publication DID. Before, one account could serve its list under another account's path.
  • Internal operations that were public. None were called by any client.
    • Removed or made internal: anchor record writes (owners could mark a forged anchor confirmed), list-wide push, and activity writes.
    • Now internal-only: the Sites and DID resolver lookups. These returned whole list, site and hostname rows.
  • Session revocation bypass (adversarial review; dates from fix(auth): require authenticated access across browser and HTTP #241). jose accepts a JWT whose signature's last base64url character has its unused low bits flipped. Sessions and revocation tombstones are keyed by the token string's hash, so a re-encoded copy of a logged-out or revoked token could establish a fresh session, making logout and revocation ineffective. verifyAuthToken now accepts only canonical encodings. Every token the signer issues is canonical, so existing sessions are unaffected.
  • Clients adopting refused DIDs. The client used to adopt a freshly minted did:webvh even when updateDID failed. It now adopts it only on success, so it never publishes or acts under a DID the server refused.
  • Cross-account references. createList and updateListCategory accept only the actor's own categories; on create, a category deleted meanwhile leaves the list uncategorized. deleteUserData declares its account at the boundary.

Auth integration choice

This keeps #241's session-record boundary rather than Convex ctx.auth (auth.config.ts):

  • Signing keys. Convex custom JWT needs asymmetric tokens and a JWKS endpoint. The Turnkey/OTP flow issues HS256 tokens, so switching means new keys, a published JWKS, and a re-login for every session. That is an infrastructure change this fix doesn't need.
  • Revocation. accessSessions is what makes logout, expiry, and persistent-mobile-session revocation invalidate reactive queries; ctx.auth identities are stateless.
  • API keys. Agents would still need the argument path for their keys.

authenticate() in convex/lib/actor.ts is the single place to add a ctx.auth branch later.

Agent/API-key attribution and #277

  • Every call resolves ctx.actor.credential: the session row or the specific API-key row, with scope and revocation checked in the same transaction.

  • It is now persisted as an optional credential on:

    • activity rows: assignments, unassignments, inherited assignments, presence;
    • comments;
    • a new comment_deleted activity row, which records only the comment ID.

    Two keys on one account leave distinguishable history, and a test confirms a credential named in arguments is never the one recorded. Credentials are stored for audit but stripped from every read response, since published lists are readable by any signed-in account.

  • The assignment helpers read the credential from the actor context the wrapper already provides, so Sign action records with owner Turnkey keys through one shared path #277's items.ts call sites are untouched.

  • Sign action records with owner Turnkey keys through one shared path #277 adds signed action records binding the same credential for item and list actions.

  • Merging this branch with Sign action records with owner Turnkey keys through one shared path #277 is conflict-free, and the combined tree passes 716/716 tests plus the registry check.

Rollout (coordinated with #262)

  1. Deploy Convex first. Every change here tightens the server. The only schema changes are additive: optional credential on activities and comments, and the comment_deleted activity type, which agents reading /api/activity/list may now see. There is no new public name, so no deploy order can widen access.
  2. Then ship clients. The only client change drops four unused names from the session registry.
  3. Old and stale clients keep working, because none call the removed names. Non-conforming inputs (a foreign category, a non-self publication DID, did:key) get an error and nothing is written.
  4. Rollback would reopen the gaps; repair forward.

#262's pending client-inventory and staging evidence is unchanged and still gates recipient grants. docs/authentication-rollout.md lists read-only data checks for prior misuse; I have not run them.

Independent review

Cross-provider review (Codex GPT-6-Sol) round 1 raised three points, all addressed in a5830c6:

  • the template bypass (rule reverted; now a follow-up);
  • key attribution not persisted (now on activity rows);
  • the boundary test not checking reads or HTTP routes (both added).

Round 2 confirmed those responses and found two remaining gaps, both now fixed: comment writes and deletions didn't record the acting key, and the HTTP pass didn't assert on reads.

Round 3 found three problems with that fix, all now resolved:

  • stored credential IDs were returned to published-list readers (now stripped from reads);
  • the deletion row could expose a formerly private comment's author (it now keeps only the comment ID);
  • that author DID would not be rewritten on re-mint (no longer stored).

Adversarial review

Two independent attack passes ran after the review rounds: one hunting exploits with proof-of-concept tests, one hunting breaks in legitimate flows.

  • Exploit pass: confirmed the JWT re-encoding revocation bypass (fixed above). It found re-mint, publication binding, resource authorization, assertions, attachment keys and the credential projection sound.
  • Compatibility pass: found no breaks in current publish, re-mint, updateDID, offline or agent flows. It flagged two edge cases, both fixed above: client DID drift and stale categories.
  • Production data risk: active /d/* publications whose list owner matches no account would now 404. I added a read-only check for these to the pre-deploy data checks; the fix is to repair the rows, not loosen the check.

Verification

  • bun test: 702 pass, 0 fail (683 on main + 19 new: 6 boundary tests, 9 identity-binding regressions including token revival, 4 client DID-adoption tests). The boundary scan now includes convex/ subdirectories such as migrations/. Against main's code, the binding and attribution regressions fail; the credential-resolution test documents existing Clarify unsigned provenance and preserve authenticated credential identity #273 behaviour.
  • tsc -p convex/tsconfig.json, tsc -b, generate-auth-client --check and vite build pass. Lint on changed files: no new errors (4 pre-existing Function-type errors in didResourcesHttp.ts, versus 5 on main).
  • Not verified:
    • live Convex deploy and codegen;
    • OTP login, publishing and DID re-mint on deployed web/iOS/Android;
    • the production data checks.

Follow-ups (outside #236)

  • Public templates from shared lists. An editor can publish a list's contents as a public template, via createFromList (isPublic), or by saving privately and then updateTemplate. Any reader can also retype the items into createTemplate, so no server rule alone prevents it. This is a product decision; I tried an owner-only rule in this PR, but review showed it was bypassable, so I removed it.
  • Push-token re-binding, and registerPushToken accepting any web URL, which the server then POSTs to.
  • getUsersByDids has no input cap.
  • Anonymous public-list reads ignore the owner-deletion barrier.
  • items:write keys can delete lists they own.
  • The attachment download path doesn't register cookie JWTs.

🤖 Generated with Claude Code

Inventory every public query, mutation and action and enforce the
classification with an exhaustive boundary test over the real
registrations: protected functions must reject anonymous, unknown-key and
asserted current/legacy identities before any read or write.

Close the remaining identity gaps found by the audit:
- DID re-mint/updateDID require a did:webvh at the caller's own path, and
  re-mint refuses a DID held by another account (account takeover).
- publishList binds webvhDid to the publisher's own resource DID; the /d/*
  fallback requires the list owner to control the publication DID.
- Anchor record writes, list-wide push, activity writes and the Sites/DID
  resolver lookups are no longer publicly callable.
- Lists can only be filed in the actor's own categories; only owners can
  publish a list as a public template; deleteUserData declares its account.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@railway-app

railway-app Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

🚅 Deployed to the boop-pr-288 environment in Friends

Service Status Web Updated
boop ✅ Success (View Logs) Web Oct 8, 2026 at 8:23 am UTC

@railway-app
railway-app Bot temporarily deployed to Friends / boop-pr-288 October 8, 2026 07:40 Destroyed

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues. One of the new identity rules can be bypassed in two calls; the other two suggestions are small.

Reviewed changes

I reviewed the full diff at 8ec21e7. The touched suites pass locally: identity-binding, public-function-boundary, auth-boundary, private-sharing and remint-did, 122/122. I grepped src/ and convex/ and found no remaining callers of any name this PR removes or makes internal.

  • DID re-mint and updateDID binding: both endpoints now require a did:webvh at the caller's own user-<sub-org> path. applyRemint refuses a DID that another account already holds, and only rewrites whole {oldDid}/… prefixes.
  • Publication DID binding: publishList only accepts the publisher's own {did}/resources/list-{id}. Clients already send this, built from the server-canonical useCurrentUser().did.
  • /d/* resolver fallback: the new internal getPublishedListForPath requires the publication's controller DID to be the list owner's current or legacy DID.
  • Public surface reduced: anchor record writes, recordActivity, sendListNotification, the Sites lookups and the DID-log/resource lookups are now removed or internal. The generated client registry is updated to match.
  • Cross-account references: createList and updateListCategory now only accept the actor's own categories. createFromList blocks public templates for non-owners. deleteUserData now declares its accounts resource.
  • Enforcement: a new exhaustive registry test classifies every public registration. New identity-binding regressions cover the closed gaps. The inventory and runbook docs are updated.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

Comment thread convex/templates.ts Outdated
Comment thread scripts/public-function-boundary.test.mjs Outdated
const value = row[field];
if (typeof value === "string" && value.startsWith(args.oldDid)) {
// Whole-DID prefix only: `{oldDid}/...`, never a longer DID sharing its characters.
if (typeof value === "string" && value.startsWith(`${args.oldDid}/`)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small consistency gap: previewRemint still uses bare startsWith(args.oldDid) for prefix fields. The operator dry run will therefore count rows that applyRemint no longer rewrites, such as a longer DID that shares the prefix. Use the same `${args.oldDid}/` check there.

…P routes and private reads

- Activity rows record the session or specific API key that acted
  (optional activities.credential), read from the actor context so
  PR #277's items.ts call sites are untouched.
- Boundary test now synthesizes validator-shaped arguments, fails on any
  non-credential read before rejection, and drives every non-public HTTP
  route anonymously and with forged credentials.
- Category and billing routes return 401/403 for credential failures
  instead of 500.
- Revert the createFromList public-template restriction: it was bypassable
  (private save then updateTemplate, or createTemplate) and is a product
  decision; recorded as a follow-up.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@railway-app
railway-app Bot temporarily deployed to Friends / boop-pr-288 October 8, 2026 07:49 Destroyed
…edes the auth check

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@railway-app
railway-app Bot temporarily deployed to Friends / boop-pr-288 October 8, 2026 07:50 Destroyed

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues. One small exposure is noted inline: the new credential IDs are readable by anyone who can read the activity feed.

Reviewed changes

I reviewed the changes since 8ec21e7. At a5830c6 the public-function-boundary, identity-binding, auth-boundary and private-sharing suites pass, 115/115.

  • Activity credential attribution: added an optional activities.credential field. Assignment, unassignment, inherited-assignment and presence rows now record the acting session or specific API key, through actingCredential(ctx) or ctx.actor.credential.
  • HTTP auth status: the billingHttp and categoriesHttp catch blocks now send AuthErrors through handlerErrorResponse, so credential failures return 401/403 instead of 500.
  • Boundary test hardened: protected registrations are now called with business arguments sampled from their validators and naming real fixture rows. Any read outside the credential tables before rejection fails the test. A new pass checks every non-public HTTP route anonymously, with a forged key and with a forged bearer token.
  • Template rule withdrawn: dropped the partial owner-only isPublic check in createFromList and its "closed gap" claim. Template publication is now listed as a product-decision follow-up, which resolves the earlier thread.

The earlier previewRemint and recursive-discovery threads are still open. The new commit doesn't touch them.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

Comment thread convex/schema.ts Outdated
…k HTTP reads

Round 2 review: comments now persist the session/API-key credential, and
deleting one leaves a comment_deleted activity row (ID and author, no
text) with the deleting credential. The HTTP boundary pass also fails on
non-credential reads before rejection.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@railway-app
railway-app Bot temporarily deployed to Friends / boop-pr-288 October 8, 2026 07:56 Destroyed

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues. The new comment attribution also exposes credential IDs to readers; see the inline note.

Reviewed changes

I reviewed the changes since 7ab6552. At da78255, the identity-binding, public-function-boundary, auth-boundary and private-sharing suites pass 115/115, and tsc -p convex/tsconfig.json is clean.

  • Comment credential attribution: addComment now stores ctx.actor.credential on the comment row. The schema validator is now a shared actingCredential, used by both activities and comments.
  • Comment deletion audit: deleteComment now writes a comment_deleted activity row. The row records the acting credential, plus the comment's ID, author and creation time. It does not keep the comment text.
  • HTTP boundary pass: non-public routes now fail the test if they read anything beyond the credential tables before rejecting. /d/* may also read didLogs, lists and publications.
  • Docs: updated the inventory and runbook to describe the comment attribution and the new activity type.

The earlier previewRemint, recursive-discovery and getListActivity credential-exposure threads are still open. These commits don't touch them.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

Comment thread convex/comments.ts
… the comment ID

Round 3 review: published lists are readable by any signed-in account,
so getListActivity/getItemComments (and their HTTP routes) now strip the
stored credential, and comment_deleted rows no longer retain the deleted
comment's author or timestamp (which also avoids stale DIDs after re-mint).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@railway-app
railway-app Bot temporarily deployed to Friends / boop-pr-288 October 8, 2026 08:00 Destroyed

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues. This commit fixes both credential-exposure threads, and I've resolved them. The earlier previewRemint and recursive-discovery suggestions are still open.

Reviewed changes

I reviewed the changes since da78255. At 99b3e2a, the identity-binding, public-function-boundary, auth-boundary and private-sharing suites pass 115/115, and tsc -p convex/tsconfig.json is clean.

  • Credential read projection: added withoutCredential in convex/lib/actor.ts. getListActivity and getItemComments now return rows without credential. /api/activity/list calls the same handler internally, so it gets the projection too. The other reads of these tables are getCommentCount and the Explorer createdAt lookup in originals.ts. Neither returns the field.
  • Smaller deletion audit row: a comment_deleted row now records only { commentId }. It no longer stores the deleted comment's author or creation time.
  • Regression coverage: identity-binding.test.mjs now checks that an editor reading activity and comments never receives credential, and that the deletion note has exactly the comment ID.
  • Docs: the inventory and runbook now say that credentials are stored for audit and stripped from reads. They list an owner-facing audit view as a possible follow-up.

Pullfrog  | Fix it ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@railway-app
railway-app Bot temporarily deployed to Friends / boop-pr-288 October 8, 2026 08:03 Destroyed
…t DID drift

- verifyAuthToken accepts only canonical base64url segments. jose decodes
  the signature's unused trailing bits leniently, so a re-encoded copy of
  a logged-out/revoked token hashed differently from its tombstone and
  could establish a fresh session (pre-existing since #241).
- Clients adopt a newly minted did:webvh only when updateDID succeeds, so
  they never act under a DID the server refused.
- createList leaves a list uncategorized when its category was deleted
  meanwhile; foreign categories are still rejected.
- Boundary test scans convex subdirectories (migrations/) too.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@railway-app
railway-app Bot temporarily deployed to Friends / boop-pr-288 October 8, 2026 08:21 Destroyed

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues in the fixes themselves. There is one small doc-placement nit inline. The earlier previewRemint suggestion is still open.

Reviewed changes

I reviewed the changes since b35d90b. At d78e17d, the identity-binding, public-function-boundary, auth-provider, auth-boundary and private-sharing suites pass 135/135, and tsc -p convex/tsconfig.json is clean.

  • JWT canonical encoding: verifyAuthToken now rejects any segment whose base64url decode→encode round-trip differs from the input. This closes the case where flipping a signature's trailing bits revived a revoked token. Every JWT check in convex/ goes through verifyAuthToken: actorSession.establish/revoke and requireSession. No path skips the new check.
  • Stale categories on create: createList now stores no category when the category no longer exists, so queued offline creates don't fail. A foreign category still gets resourceUnavailable, and updateListCategory still rejects a missing one.
  • Client DID adoption: on both session restore and OTP login, the client adopts a minted did:webvh only when /api/user/updateDID returns ok. On restore it no longer stores a DID log for a refused DID.
  • Recursive boundary discovery: the registry scan now includes convex/ subdirectories such as migrations/, with outbase: 'convex'. I resolved the earlier thread about this.
  • Docs: the runbook now covers both fixes, plus a pre-deploy check for /d/* publications whose owner matches no account.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

Comment thread convex/lib/jwt.ts
Comment on lines +42 to +48
/**
* jose decodes base64url leniently: the unused low bits of a segment's final
* character may vary, so one signature has several valid token strings. Sessions and
* revocation tombstones are keyed by the token string's hash, so a re-encoded copy of a
* logged-out token would otherwise establish a fresh session. Accept only the single
* canonical encoding, which is what jose's signer produces.
*/

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this helper was inserted between verifyAuthToken's JSDoc (L35–41) and the function itself. The @param/@returns/@throws block now documents isCanonicalJwt, and verifyAuthToken has none. Move isCanonicalJwt and its comment above the original JSDoc, so that block sits directly on verifyAuthToken again.

@brianorwhatever
brianorwhatever merged commit 79f7c73 into main Oct 8, 2026
10 checks passed

This branch was successfully deployed

No deployments
Friends / boop-pr-288 — d78e17d2 Deployed Oct 8, 2026 by railway-app[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[P0] Enforce authenticated identity across browser and agent operations

1 participant