feat(oauth): add OAuth 2.1 authorization flow for MCP clients - #318
feat(oauth): add OAuth 2.1 authorization flow for MCP clients#318dash0-dev[bot] wants to merge 24 commits into
Conversation
MCP hosts (Claude Code, Cursor, ChatGPT, VS Code) expose an "Authorize" action that drives RFC 9728 discovery -> RFC 8414 metadata -> RFC 7591 dynamic client registration -> consent -> authorization-code exchange with PKCE. LoreKit had none of it, so the only way to attach a client was to mint an lk_* token in the dashboard and paste it into a config file. The authorization server is the Next.js dashboard: the consent screen needs the Supabase-Auth session, the caller's org list and the api_tokens write path, all of which already live in packages/web and none of which the self-contained Deno edge tree can reach. The MCP Edge Function stays the resource server. An OAuth access token IS an api_tokens row (same lk_ format, same SHA-256 lookup), so revocation, the per-user cap, the dashboard list and the audit trail keep working unchanged; what is new is kind='oauth', a 30-day expires_at enforced at auth time, client_id, and the org allow-list the user picked on the consent screen. The "no 401 from the MCP endpoint" invariant is narrowed rather than repealed: a request with NO credential now gets 401 + WWW-Authenticate (the only discovery trigger an Authorize button can follow), while a request with an invalid or expired token still gets an in-band JSON-RPC error at HTTP 200 -- the hang that rule prevents needs a configured client with a pending tools/call, which a credential-less request does not have.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
The RFC 9728 protected-resource document was served by the MCP Edge Function and the RFC 8414 document by the Next.js app, which meant a pure module mirrored verbatim into the self-contained Deno tree (plus an edge-parity entry) to carry two string constants. RFC 9728 3.1 lets the WWW-Authenticate challenge carry an absolute resource_metadata URL, and the MCP spec requires clients to follow it, so the document does not have to be same-origin with the resource. Both now live in packages/web with one pair of constants and one owner. The edge function keeps exactly two pieces of OAuth knowledge: the challenge string, and a 308 on the path-constructed metadata URL so a client that derives it from the resource identifier instead of reading the header does not hit a 404. oauth-discovery.spec.ts source-scans both sides to keep the issuer, metadata URL and challenge shape agreeing.
|
@dash0 review |
|
⚡ Running PR Review & Implement… |
![dash0-dev[bot]](https://avatars.githubusercontent.com/in/2731631?s=60&v=4){kind=small}
There was a problem hiding this comment.
Found 2 gate(s) that need attention before human review.
| Gate | Status | Details |
|---|---|---|
| Description vs. code | ❌ | "the 20-token cap keeps working" — issueAccessToken never checks MAX_TOKENS_PER_USER |
| Prior bot feedback | ✅ | |
| Documentation | ✅ | |
| Self-review signals | ✅ | |
| Code review | ❌ | See inline comments |
Reviewed for commit 58d08799b2a154b3c313d06afa541e807fe2e73e. CI status is shown in the checks section above.
Review diagnostics
Run mode: full — 3284 lines changed across 37 files
Integrations checked: RFC 6749 §4.1.3/§5.2, RFC 7591 §3.2.1, RFC 7636 §4.1, RFC 7009 §2.2, RFC 8252 §7.1/§7.3, RFC 8414, RFC 9728 §3.1; PostgREST/supabase-js filter + RPC surface; Fetch/CORS Access-Control-Expose-Headers semantics. No dependency manifest or lock file changed.
Quality Gate: produced 14, carried forward 0, relevance-memory drops 0, dedupe drops 2,
grounding drops 0, confidence drops 4, shape drops 0, cleared 8, deferred over inline cap 0, posted inline 8.
Optimality review (2.4c): ran — 6 unit(s) judged, 6 optimal, 0 proposal(s), 0 withheld.
Skipped files: none
| permissions: grant.permissions, | ||
| kind: 'oauth', | ||
| client_id: grant.clientId, | ||
| org_ids: grant.orgIds, |
There was a problem hiding this comment.
issue: MAX_TOKENS_PER_USER (tokens.ts:44) is never checked on this path, so authorizing enough clients pushes a user past the 20-token cap the PR says keeps working. (non-blocking)
There was a problem hiding this comment.
Half fixed. The PR body and CLAUDE.md:532 no longer claim the cap holds here (2885cc1) — the code change is not applied: consumeAuthorizationCode has already burned the code by the time issueAccessToken runs, so rejecting there hands the client a dead code and there is no fitting RFC 6749 §5.2 error, leaving every retry to fail identically. Consent time is the seam that has a browser session to point at "revoke a token"; leaving this thread open for that call.
There was a problem hiding this comment.
Still not applied and the thread stays open, as agreed: a consent-time check needs MAX_TOKENS_PER_USER out of a 'use server' file plus a new consent-screen failure path that ConsentResult has no channel for. The prose half is already correct in the body, CLAUDE.md and now the migration header (481c7b6).
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'WWW-Authenticate': wwwAuthenticateChallenge(), | ||
| 'Access-Control-Expose-Headers': 'WWW-Authenticate', |
There was a problem hiding this comment.
suggestion: Access-Control-Expose-Headers is inert without Access-Control-Allow-Origin, so a browser-based MCP client still cannot read this challenge. This function sets no CORS headers on any other response. (non-blocking)
There was a problem hiding this comment.
Confirmed inert — line 132 is the only Access-Control-* header in the whole function and there is no OPTIONS handler, so a browser client never completes preflight either. Not applied: the only remedy that makes the challenge readable is Access-Control-Allow-Origin plus a preflight on the MCP resource endpoint, which opens it to every browser origin — an authz-surface decision beyond this suggestion. Leaving the thread open.
There was a problem hiding this comment.
Confirmed inert and left as-is: Access-Control-Allow-Origin plus an OPTIONS preflight on the MCP resource endpoint is an authz-surface decision, not a suggestion this run can land. Thread stays open for that call.
Both discovery values were pinned to production constants, which had two consequences. A staging Supabase project would have challenged clients toward production lorekit.io and had a token minted for the wrong resource; and the flow could not be exercised on a local stack at all, which is how that bug would have survived to production. - The edge challenge and the protected-resource redirect now go through authorizationServerIssuer(), honouring the LOREKIT_APP_URL secret that already existed for cap/rate-limit message links. - The protected-resource document resolves its resource through resolveMcpUrl() -- the one derivation from NEXT_PUBLIC_SUPABASE_URL -- rather than holding a second copy of the production URL. - resolveMcpUrls() no longer mangles a non-supabase.co origin. It split the host on the ref, so a local http://127.0.0.1:54321 became https://http://127.0.0.1:54321.supabase.co/functions/v1/mcp. Harmless while it only fed a copy-paste onboarding snippet; wrong now that the same URL is the OAuth resource identifier clients compare against. oauth-discovery.spec.ts gains assertions that neither side is re-pinned to a production literal, and deployment.md documents LOREKIT_APP_URL as load-bearing for OAuth on non-production projects.
`auth.role()` is NULL whenever there is no PostgREST request context — pg_cron, psql, the dashboard SQL editor — so `is distinct from 'service_role'` was true for exactly the callers the sweep is meant for and the function raised LK002 instead of purging. The guard now only checks a caller that actually presented a JWT; every other caller stays gated by the REVOKE/GRANT below, which is what 00004's and 00034's reapers rely on. Addresses @dash0-dev[bot] #318 (comment)
The comment on `oauth_authorization_codes.scope` described `state` — the parameter echoed back on the authorize redirect for CSRF protection, which this table never stores. It now describes the requested scope string and says plainly that the granted access is `permissions` + `org_ids`. Addresses @dash0-dev[bot] #318 (comment)
`randomToken(24)` base64url-encodes to exactly 32 characters, so stripping `-`
and `_` before `slice(0, 32)` produced a suffix of 26-32 characters (measured
over 20k samples) instead of the `lk_{rw|ro|wo}_<32>` the dashboard mints. Use
the same alphanumeric generator `lib/tokens.ts` uses; it is copied rather than
imported because that module is `'use server'` and exporting the helper there
would make it a callable action endpoint.
Addresses @dash0-dev[bot]
#318 (comment)
`MAX_TOKENS_PER_USER` is checked only in `generateToken`; `issueAccessToken` inserts without counting, so the cap does not hold on the OAuth path. Say so, name the bound (re-authorizing a client is net-zero, so drift is the number of distinct clients) and record why enforcing it at the token endpoint is the wrong seam. Addresses @dash0-dev[bot] #318 (comment)
`toolArchive`, `toolRestore`, `toolPurge` and `toolPurgeExpired` never read `tokenOrgIds` — they scope by `user_id` alone, which is already narrower than any org allow-list. The parameter is there because mcp-handler.ts dispatches every tool through one uniform signature; the docblocks now say that instead of implying an intersection that does not happen. The six handlers that do consult it are unchanged. Addresses @dash0-dev[bot] #318 (comment)
The `0 = the registration does not expire` note sat above `client_id_issued_at`, which is a timestamp; no expiry field is emitted at all. Describe the field that is actually there and state the omission explicitly. Addresses @dash0-dev[bot] #318 (comment)
`permissions` initialises to `['read', 'write']`, the widest of the three tiers, so the "least access that is still useful" claim was wrong. State the real default and the reason for it, and keep the part of the claim that is true: no org is granted unless the user ticks it. Addresses @dash0-dev[bot] #318 (comment)
There was a problem hiding this comment.
Found 2 gate(s) that need attention before human review.
| Gate | Status | Details |
|---|---|---|
| Description vs. code | ❌ | Omits the per-deployment issuer/resource resolution and the now load-bearing LOREKIT_APP_URL |
| Prior bot feedback | ✅ | |
| Documentation | ✅ | |
| Self-review signals | ✅ | |
| Code review | ❌ | See inline comments |
Reviewed for commit 1a77f85d4eec24226dbc0122f20eec337aeeb8d5. CI status is shown in the checks section above.
Review diagnostics
Run mode: full — 284 lines in delta since 58d0879 (upgraded from incremental: delta > 100 lines and a migrations/ path)
Integrations checked: RFC 6749 §4.1.1/§4.1.3/§5.1, RFC 7009 §2.2, RFC 7591 §3.2.1, RFC 7636 §4.1, RFC 8252 §7.1/§7.3, RFC 8414, RFC 9728 §3.1; Fetch/CORS simple-response semantics (Access-Control-Allow-Origin required on the actual response, not only the preflight); PostgREST filter + RPC surface. No dependency manifest or lock file changed.
Quality Gate: produced 14, carried forward 0, relevance-memory drops 0, dedupe drops 4,
grounding drops 0, confidence drops 6, shape drops 0, cleared 3, deferred over inline cap 0, posted inline 3.
Memories applied: (2 LoreKit memories influenced this review)
gate1-description-vs-code-test-count-drift— promoted, seen 3×dash0dev-gate1-description-vs-code-row— promoted, seen 3×
Optimality review (2.4c): ran — 6 unit(s) judged, 6 optimal, 0 proposal(s), 0 withheld.
Skipped files: none
The 201 was the only response from POST /api/oauth/register without an Access-Control-Allow-Origin header, so a browser-based MCP client -- the very caller the OPTIONS preflight below it exists for -- could complete the registration but never read the client_id it was issued. Declare the header once as CORS_HEADERS and spread it into all three paths (201, error, preflight) so they cannot drift apart again. The response carries no secret (public clients only, no client_secret) and registration is already open, so '*' grants nothing new. Addresses #318 (comment)
The protectedResourceMetadata docblock said `resource` must be the concrete production URL, which contradicts both the MCP_RESOURCE_URL note above it (which calls that value a default) and the route, which passes resolveMcpUrl() so a preview stack advertises its own endpoint. Reworded to say "this deployment's endpoint" and to name the derivation. The no-<ref>-placeholder rule is kept -- resolveMcpUrl() yields a concrete origin -- so the CLAUDE.md key decision it encodes is not lost. Addresses #318 (comment)
edgeBuilt and METADATA_URL were both interpolated from the same local ISSUER const, so the assertion was equal by construction -- no edit to either source could ever break it, and the belt-and-braces guard the comment promised never fired. Each side is now rebuilt from literals extracted out of that side's own source (edge AUTHORIZATION_SERVER_ISSUER + its path string; web DEFAULT_ISSUER + its path template) and the two are compared with each other and with METADATA_URL. Explicit not-toBeNull assertions on all four matches close the undefined === undefined hole a stale regex would otherwise open. Mutation-verified: repointing the edge issuer at staging.lorekit.io makes the comparison fail; the previous assertion stayed green. Addresses #318 (comment)
There was a problem hiding this comment.
Reviewed your changes and found no issues ready for human review.
| Gate | Status |
|---|---|
| Description vs. code | ✅ |
| Prior bot feedback | ✅ |
| Documentation | ✅ |
| Self-review signals | ✅ |
| Code review | ✅ |
Incremental review for commit df3170ab2ec0f39c67e34047f717ae75b55dc1bb (delta since 1a77f85). CI status is shown in the checks section above.
Review diagnostics
Run mode: incremental — 55 lines in delta across 3 files (oauth-discovery.spec.ts, api/oauth/register/route.ts, lib/oauth/metadata.ts); 0 new files, 0 high-stakes paths
Integrations checked: RFC 7591 §3.2.1 (registration response — client_id_issued_at optional, no client_id_expires_at field exists; emitted shape conforms), RFC 8414 / RFC 9728 §3.1 (issuer + metadata URL agreement across the web/edge split), Fetch CORS preflight semantics (Content-Type: application/json is non-safelisted, so the OPTIONS handler and its Access-Control-Allow-Headers are required and correct). No dependency manifest or lock file changed.
Quality Gate: produced 5, carried forward 0, relevance-memory drops 0, dedupe drops 1,
grounding drops 1, confidence drops 3, shape drops 0, cleared 0, deferred over inline cap 0, posted inline 0.
Optimality review (2.4c): ran — 3 unit(s) judged, 3 optimal, 0 proposal(s), 0 withheld.
Skipped files: none
There was a problem hiding this comment.
Reviewed your changes and found no issues ready for human review.
| Gate | Status |
|---|---|
| Description vs. code | ✅ |
| Prior bot feedback | ✅ |
| Documentation | ✅ |
| Self-review signals | ✅ |
| Code review | ✅ |
No code changes since df3170a — gate checks only for commit df3170ab2ec0f39c67e34047f717ae75b55dc1bb. CI status is shown in the checks section above.
Review diagnostics
Run mode: incremental-quick — no code changes (compare df3170a...df3170a is identical; the prior PR_REVIEWER_REPORT review was already submitted at this commit). Inline review skipped per the zero-delta short-circuit; all gates re-evaluated against the full PR state.
Integrations checked: skipped (incremental-quick)
Quality Gate: produced 0, carried forward 0, relevance-memory drops 0, dedupe drops 0,
grounding drops 0, confidence drops 0, shape drops 0, cleared 0, deferred over inline cap 0, posted inline 0.
Optimality review (2.4c): skipped (incremental-quick) — 0 unit(s) judged, 0 optimal, 0 proposal(s), 0 withheld.
Skipped files: none
main shipped its own 00049 (memory_scopes_last_activity) plus 00050-00052 while this branch was open, so merging main in left two files claiming 00049. supabase db push applies in numeric order, so a duplicate that sorts before migrations already live cannot be applied without --include-all. Renames the file and updates every reference to it in the code comments, the drift guards, docs/architecture.md and CLAUDE.md.
There was a problem hiding this comment.
Found 2 gate(s) that need attention before human review.
| Gate | Status | Details |
|---|---|---|
| Description vs. code | ❌ | Body still names 00049_oauth.sql; HEAD ships 00053_oauth.sql (renumbered in b8ff173) |
| Prior bot feedback | ✅ | |
| Documentation | ✅ | |
| Self-review signals | ✅ | |
| Code review | ❌ | See inline comments |
Reviewed for commit b8ff173dadd4a0cc8f84a7a8e33a50bb89351bfc. CI status is shown in the checks section above.
Review diagnostics
Run mode: full — upgraded from incremental (delta since df3170a is 17143 lines / 63 new files, dominated by the origin/main merge in d6524d7)
Integrations checked: RFC 6749 §4.1.1/§4.1.2.1/§4.1.3/§5.1/§5.2, RFC 7009 §2.2, RFC 7591 §3.2.1, RFC 7636 §4.1 + Appendix B, RFC 8252 §7.1/§7.3, RFC 8414, RFC 9728 §3.1; PostgREST filter/RPC surface; Fetch CORS preflight semantics. No dependency manifest or lock file changed.
Quality Gate: produced 14, carried forward 0, relevance-memory drops 0, dedupe drops 3,
grounding drops 1, confidence drops 5, shape drops 0, cleared 5, deferred over inline cap 0, posted inline 5.
Optimality review (2.4c): ran — 6 unit(s) judged, 6 optimal, 0 proposal(s), 0 withheld.
Skipped files: none
|
|
||
| ### What the consent choices mean | ||
|
|
||
| - **Organizations are narrowing, never widening.** Ticking an org lets the |
There was a problem hiding this comment.
issue: resolveRestAuth (supabase/functions/_shared/api/auth.ts:51) selects only user_id,permissions, so the same token reaches every org over /memories and outlives expires_at. This narrowing holds on MCP only. (blocking)
There was a problem hiding this comment.
Half applied: resolveRestAuth now selects expires_at and rejects an expired token, mirroring mcp/auth.ts (ce131ac). The org_ids intersection is not applied and this thread stays open — memories/handlers/{facets,scopes,tags}.ts resolve visibility inside SECURITY DEFINER RPCs that compose lorekit_member_org_ids server-side, so a client-side intersection cannot narrow them and a partial fix would leave two surfaces wide.
|
@dash0 implement |
|
⚡ Running PR Loop · Implement Only… |
|
/preview |
❌ Preview deploy failedBranch: 🔗 Open preview → https://lorekit-d5fhmroqv-mads-thines-projects.vercel.app
Endpoints: |
…w deploy The discovery documents took their issuer from NEXT_PUBLIC_APP_URL, which Vercel pulls into preview builds still holding the PRODUCTION value. A preview deployment therefore advertised production's /authorize and /token endpoints, so authorizing against a preview would have minted a token for the wrong deployment. The /preview workflow is worse still: it deploys prebuilt via the Vercel CLI, where neither VERCEL_BRANCH_URL nor VERCEL_URL is populated at build time, so nothing build-time knows the origin at all. resolveIssuer() is a ladder: the stable branch alias on a preview, the configured canonical origin elsewhere, and the request origin as a last resort -- which is also the semantically right answer for a document that describes the server you just fetched it from. It never honours NEXT_PUBLIC_APP_URL on a preview. Caching is coupled to the decision. A request-derived origin comes from a caller-controlled header, so a shared CDN cache would be a poisoning primitive; those responses are no-store, configured ones stay cacheable.
The /preview run failed at `supabase db push` with 'Remote migration versions not found in local migrations directory' naming 00054. That is not a workflow fault: main shipped 00053_read_activity and 00054_usage_event_client while this branch was open, both are already applied to the shared preview project, and this branch's highest was 00053 -- so the remote had a version the branch did not know about, and its own 00053 collided besides. Merges main and renumbers to 00055, updating every reference (code comments, the drift guards, docs/architecture.md, CLAUDE.md).
`resolveRestAuth` selected only `user_id,permissions`, so an OAuth-issued token kept working over /memories and /orgs after its `expires_at` had passed — `mcp/auth.ts` rejects the same token at that instant. Mirror the check here. Personal dashboard tokens carry a NULL `expires_at` and are unaffected. The org-allow-list half of the same finding is not addressed here and the thread stays open: three REST read paths resolve visibility inside SECURITY DEFINER RPCs, so `api_tokens.org_ids` cannot be intersected client-side. Addresses review comment by @dash0-dev[bot] #318 (comment)
The OAuth section added directly below issues tokens that expire in 30 days and that `MAX_TOKENS_PER_USER` never counts, so "Maximum 20 tokens" and "No expiry" both read as false for half the `lk_*` population. Say which population each bullet describes, and name the asymmetry: oauth rows do not get counted at issue time but do block dashboard minting. Addresses review comment by @dash0-dev[bot] #318 (comment)
`packages/web/src/lib/tokens.ts` has no `resolveAuth` — it exports only `generateToken`, `listTokens` and `revokeToken`, and `packages/mcp-server/src/auth.ts` does no `api_tokens` lookup at all. There are two SHA-256 verification sites, not three. Addresses review comment by @dash0-dev[bot] #318 (comment)
CLAUDE.md and the PR body already record that the cap is the one thing that does not survive the OAuth path; this header comment was the last place still saying it keeps working unchanged. Addresses review comment by @dash0-dev[bot] #318 (comment)
`./org-permissions.ts` was imported twice — once for `translateOrgPermissionError` and again six lines later for `OrgPermissionError`. One specifier list. Addresses review comment by @dash0-dev[bot] #318 (comment)
|
/preview |
❌ Preview deploy failedBranch: 🔗 Open preview → https://lorekit-jp64dlhah-mads-thines-projects.vercel.app
Endpoints: |
|
@dash0 > this PR is fairly old. Please rebase it and update any conflicts and update so it's ready to be merged |
|
⚡ Running Agent0 | Execute… |
|
Looked into rebasing/updating this branch against
Reconciling OAuth 2.1 bearer-token authorization with the newly-enforced API-token scoping model requires deciding how the two authorization mechanisms compose (e.g. does an OAuth-issued token need to pass through the same scope-narrowing/refusal logic as API tokens, or is it a separate code path?) — that's a design call, not something I can resolve confidently by auto-merging without risking an auth bypass or scope-check regression. Could you clarify: should this be resolved by layering OAuth token validation on top of |
…odel main landed feat(auth): enforce api token scoping across MCP and REST (05f112d) after this branch forked, restructuring tenant-scope enforcement into a generic KeyRestriction (scopes/org_access/org_ids) that superseded this PR's own narrower org_ids-only scoping. Reconciliation, following main's security model rather than layering a second mechanism on top: - Deleted this branch's packages/mcp-core/src/tenant-scope.ts (+specs) and supabase/functions/mcp/tenant-scope.ts: both superseded by main's packages/mcp-core/src/auth/tenant-scope.ts / supabase/functions/_shared/auth/tenant-scope.ts, which already cover everything intersectTokenOrgIds/tokenAllowsOrgId did, generically. - Accepted main's supabase/functions/mcp/tools.ts and mcp-handler.ts unchanged: every memory.* tool already threads keyScoping through applyTenantScope and the memory_write/memory_delete RPCs (p_key_org_access/p_key_org_ids), so OAuth-issued tokens are narrowed with zero OAuth-specific code once they populate the same columns a scoped dashboard token does. - Kept this PR's own additions (mcp/auth.ts and _shared/api/auth.ts expiry checks; mcp/index.ts's 401 + WWW-Authenticate discovery challenge; oauth-metadata.ts; the web app's OAuth consent/token routes), rebased onto main's keyScoping-based AuthContext. - Renumbered the OAuth migration 00055 -> 00095 (main independently used 00055 for an unrelated migration) and dropped its own api_tokens.org_ids column add: that column already exists from 00068_api_token_scoping.sql with different (tri-state) semantics. issueAccessToken now maps the consent screen's ticked orgs onto org_access='selected'/org_ids=<ticked>, or 'personal' when none are ticked -- the consent screen never offers 'all', so an OAuth token is always the least-privileged of the three states. - Hand-updated the generated database.types.ts (both copies) to add api_tokens.kind/expires_at/client_id and the two new oauth_* tables, since supabase gen types needs a live project this sandbox has no access to. - Resolved CLAUDE.md/docs/api-tokens.md doc conflicts by keeping main's restructured content and re-adding the OAuth sections in its style. Verified: deno check (all edge functions, 0 regressions), nx affected typecheck (mcp-core/web/feature-flags/schemas/smoke-tests), nx affected test (mcp-core 1492/1493 + web 1346/1346 passing; the one mcp-core failure is a 5s test-timeout flake in a sandbox-slow codegen spec, reproduced green in isolation), nx affected lint (0 errors, pre-existing warning baseline only).
|
Rebased and resolved conflicts, following main's newer security model rather than layering a second scoping mechanism on top, per your direction. What changedMain had landed
Verification
GitHub now reports the PR as |
|
The dashboard preview for this PR — redeployed on each push that changes the web app.
Preview always points at the PR's latest commit; Deployment is this exact commit. · Comment |
|
Reviewed your changes — 1 error, 2 warnings need attention before human review. Blocking: 1 unanswered blocking review thread.
Review details — 3 open review threads (1 blocking)Reviewed for commit
Open review threads (3)
CI — Verified — Findings grounded via direct file reads (Tier 1); no local execution/typecheck run this pass — no new behavioral issue: finding required a receipt. Run mode — full · 3711 lines in delta · PR-state record unavailable on this dispatch (no MCP tool access) — sticky footer scan found no prior report; treated as first full review. Memories — 304 indexed · 2 used
Quality — produced 0 → posted inline 0 · cleared 0 · carried forward 0 · deferred 0 · below-bar 0 Integrations — OAuth 2.1 (RFC 6749/7636/7591/7009/8252/8414/9728) — checked against cited spec sections; no version-drift found. Optimality (2.4c) — skipped (skill not installed) Standards (2.4d) — skipped (skill not installed) Skipped files — none Reviewed by the |
1 participant
Summary
Adds the OAuth 2.1 authorization-code + PKCE flow that an MCP host's Authorize button drives, so Claude Code / Cursor / ChatGPT / VS Code can attach to LoreKit without generating and pasting an
lk_*token. The consent screen lets the user choose the access level and which organizations the connection may reach.The authorization server is the Next.js dashboard, not an Edge Function: the consent screen needs the Supabase-Auth session,
listMyOrgs()and theapi_tokenswrite path, all of which already live inpackages/weband none of which the self-contained Deno edge tree can reach. Both discovery documents are served from there too — RFC 9728 §3.1 lets theWWW-Authenticatechallenge carry an absoluteresource_metadataURL, so the protected-resource document does not have to be same-origin with the resource, and one owner beats a pure module mirrored verbatim into the Deno tree for two string constants. The MCP Edge Function stays the resource server.Changes
Migration
00055_oauth.sqloauth_clients(RFC 7591 dynamic registration, public clients only —token_endpoint_auth_method = 'none'is CHECK-enforced).oauth_authorization_codes(hashed code, S256-only challenge,consumed_atfor replay detection).api_tokensgainskind/expires_at/org_ids/client_id, all nullable-or-defaulted so every existing row keeps its exact meaning with no backfill.lorekit_purge_expired_oauth(), service-role only.Authorization server (
packages/web)/.well-known/oauth-protected-resource(RFC 9728),/.well-known/oauth-authorization-server(RFC 8414),/api/oauth/register,/api/oauth/token,/api/oauth/revoke(RFC 7009)./oauth/authorizeconsent screen — outside the(dashboard)group (no sidebar competing with the decision) but reusing itsgetUser()→/login?next=…gate verbatim.pkce.ts(RFC 7636 Appendix B vector, S256-only, timing-safe compare),redirect-uri.ts,client-registration.ts,metadata.ts,errors.ts. Impure shell instore.ts.middleware.tsmatcher excludesapi/oauthand.well-known— those are reached by a client process with no cookie to refresh.Resource server (
supabase/functions/mcp)/.well-known/oauth-protected-resourcewith a308to the dashboard copy, so a client that derives the URL from the resource identifier (RFC 9728 §3.1 path insertion) instead of reading the header still resolves.resolveAuthrejects an expired token and carries the org allow-list.memory.*reads intersect the allow-list withlorekit_member_org_ids; explicit-orgwrites/deletes are pre-checked against it.Notable decisions (all argued in
CLAUDE.md)401+WWW-Authenticate— the only discovery trigger an Authorize button can follow. A request with an invalid/expired token still gets the in-band JSON-RPC error at HTTP 200. The hang the original rule prevents (mcp-remote reading a 401 as a session failure and retrying silently) requires a configured client with a pending, correlatedtools/call, which a credential-less request does not have.mcp-authz-status.spec.tsnow pins both halves.api_tokensrow, not a second credential type — revocation, the dashboard list and the audit trail all keep working. Re-authorizing replaces the client's previous token. The 20-token cap is the one thing that does not carry over:MAX_TOKENS_PER_USERis checked only ingenerateToken, andissueAccessTokennever counts rows, so authorizing enough distinct clients takes a user past 20 (re-authorizing an existing client is net-zero, which bounds the drift to the client count). Those rows still count against the dashboard's own cap, so a user over the line can no longer mint a dashboard token. Enforcing it belongs at consent time, where a browser session exists to tell the user to revoke something — rejecting at the token endpoint would burn the authorization code with no recoverable RFC 6749 §5.2 error code. Tracked as an open item on this PR, not claimed as done.org_idsis narrowing only. It is intersected withlorekit_member_org_idsvia one shared pair (intersectTokenOrgIds/tokenAllowsOrgId, mirrored + parity-guarded), never substituted for it, so leaving an org revokes access immediately. Role authorization stays insidelorekit_org_can. The intersection is applied after the per-request membership cache so one credential's restriction cannot leak into another call.invalid_grant— distinguishing "expired" from "wrong verifier" for an unauthenticated caller is an oracle. The real reason goes to telemetry only; the route spec asserts the collapse.redirect_urivalidation is its own rule, deliberately notsafeNextPath(an MCP redirect target is legitimately an off-origin loopback URL or a dotted private-use scheme). The single exception to exact matching is the loopback port, per RFC 8252 §7.3.api_key.createwithmetadata.via = 'oauth', reusing the existing vocabulary rather than widening theaudit_logCHECK.packages/mcp-server(Fly.io Node variant) is out of scope, as it already is for API-token auth.WWW-Authenticatechallenge and its308metadata redirect throughauthorizationServerIssuer()(supabase/functions/mcp/oauth-metadata.ts), which honours the existingLOREKIT_APP_URLsecret; the dashboard's protected-resource document resolves itsresourcethroughresolveMcpUrl()(packages/web/src/lib/mcp-url.ts, the one derivation fromNEXT_PUBLIC_SUPABASE_URL). Pinned constants would have had a staging Supabase project challenge clients toward productionlorekit.ioand mint a token for the wrong resource, and would have made the flow unexercisable outside production — which is how that bug would have survived.resolveMcpUrl()was also fixed to stop mangling a non-supabase.coorigin (a localhttp://127.0.0.1:54321becamehttps://http://127.0.0.1:54321.supabase.co/...), harmless while it only fed copy-paste snippets and not harmless once clients compare the value. This is the deploy-tooling carve-out to the static-URL rule, not a<ref>placeholder in user-facing copy;oauth-discovery.spec.tspins both the overridability and the concrete production literal.LOREKIT_APP_URLis now load-bearing for OAuth on every non-production project. It previously only shaped cap / rate-limit message links, so leaving it unset was cosmetic. On a preview or self-hosted Supabase project it must now be set to that deployments dashboard origin, or the edge will challenge clients towardlorekit.ioand the Authorize button will complete against the wrong authorization server. Production is unaffected — the default already ishttps://lorekit.io`.Verification
nx run-many -t typecheck --projects=web,mcp-core,mcp-server— cleannx run-many -t test --projects=mcp-core,web— 836 + 542 passing, including the updatedmcp-authz-statusguard and the new cross-packageoauth-discoveryguard (issuer, metadata URL and challenge shape agree acrosspackages/weband the Deno edge tree)nx lint web/nx lint mcp-core— cleanscripts/check-migration-order.mjs main— ok (00055 sorts after the base max, 00054)Follow-ups (deliberately not in this PR)
supabase/tests/migrations.test.sql.kind='oauth'rows by client and shows the granted orgs.lorekit_purge_expired_oauth()._shared/api/auth.ts) has its ownresolveAuth; it does not yet honourexpires_at/org_ids. Worth landing in lockstep before OAuth tokens are advertised for REST.