feat(auth): keyless OAuth (loopback PKCE) — stop requiring an API key [BLOCKED on AS client registration]#41
feat(auth): keyless OAuth (loopback PKCE) — stop requiring an API key [BLOCKED on AS client registration]#41izgorodin wants to merge 2 commits into
Conversation
The stdio server required a pasted MNEMOVERSE_API_KEY. Now, with no key, it runs a one-time browser sign-in (OAuth 2.1 authorization-code + PKCE, loopback redirect per RFC 8252), caches + silently refreshes the token in ~/.mnemoverse/tokens.json, and authorizes with Bearer. The API key stays supported as a fallback (CI / headless / self-host) and still wins when set. Server still starts auth-free (tools/list works); auth resolves lazily on the first tool call. Verified against the live AS: device_authorization grant is NOT offered (well-known = authorization_code + refresh_token only), so loopback is the correct browser flow. 🔴 BLOCKED on an AS-side change: GET /authorize with client_id=mnemoverse-cli returns 302 error=invalid_client — the public CLI client is not registered. Needs better-auth to register a public client 'mnemoverse-cli' (PKCE, no secret) with loopback redirect http://127.0.0.1:*/callback allowed. Do NOT npm-publish until that lands and e2e sign-in is tested. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 7 minutes and 7 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
… 8 blockers)
A two-wave hostile review of the keyless loopback-PKCE flow surfaced one
CRITICAL and several HIGH issues; all are now closed and unit-tested.
- CRITICAL (Windows local RCE + functional break): the old browser launcher
`cmd /c start "" <url>` ran the OAuth URL through cmd's parser — it truncated
the URL at the first '&' (keyless OAuth never worked on Windows) AND executed
`&segment` as a shell command (reproduced: `&ver`). Replace with a shell-free
`rundll32 url.dll,FileProtocolHandler <url>` passing the URL as one argv
element; add assertSafeEndpoint() so discovery endpoints (and MNEMOVERSE_AUTH_URL)
must be https (http only for loopback dev) — defense in depth against a
poisoned/MITM'd discovery doc.
- HIGH: redirect_uri was rebuilt from server.address() AFTER server.close()
(null-deref + broke OAuth exact-match) → capture redirectUri once in listen()
and reuse it verbatim.
- HIGH: no server.on('error') → unhandled error hung the Promise → add handler
that settles it (and timeout settles too).
- HIGH: callback tore down the listener on the FIRST request before validating
code/state → DoS + masked CSRF + double-exchange. Rewrite: single-shot
`settled` guard, validate code+state+Host BEFORE any teardown, invalid → 400
and IGNORE so the legitimate redirect can still win.
- HIGH: no in-flight dedupe → concurrent first calls spawned N servers+browsers
and raced token writes → module-level `inflight` memoizes the sign-in.
- HIGH: keyless-by-default hung CI/headless for the full timeout → fast-fail via
browserSignInAvailable() gated on env (CI / MNEMOVERSE_NO_BROWSER), NOT isTTY
(a stdio MCP server is non-TTY even under Claude Desktop).
- MEDIUM: secrets dir created world-traversable → mkdir 0o700 + POSIX chmod
fallback (file stays 0o600).
- MEDIUM: raw token-endpoint error body spliced into the thrown Error →
bounded + stripped; invalid_client special-cased to a self-explaining message.
Also: in-memory token cache (no readFileSync per tool call), AUTH_TIMEOUT_MS
env-configurable, fallback-first stderr wording. Unit tests (node --test, 8/8)
lock the single-argv browser launch (the RCE fix) + https endpoint validation.
Still draft + BLOCKED on AS: the public client `mnemoverse-cli` must be
registered on auth.mnemoverse.com before e2e. Deferred to the post-AS e2e pass:
401-driven local token invalidation (recoverability, not security).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🔒 Two-round adversarial security review — done; hardened, still gatedWhy this update: before merge I ran two hostile review waves over the keyless OAuth flow (auth = critical/dangerous). The first wave found a CRITICAL and 6×HIGH; this commit closes all of them; the second wave verified each is closed with no new critical/high regression. Round 1 — closed (8 blockers)
Tests: Round 2 — verdict: round-1 complete, no new blockersNon-blocking follow-ups, folded into the post-AS e2e pass: 401-driven local token invalidation (recoverability, not a hole). The core-API error-body passthrough in Remaining gate (unchanged — still 🔴 blocked)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR makes @mnemoverse/mcp-memory-server usable without a pre-provisioned MNEMOVERSE_API_KEY by adding a lazy, loopback OAuth 2.1 authorization-code + PKCE flow that runs on first authenticated tool call, caches tokens in ~/.mnemoverse/tokens.json, and silently refreshes them thereafter (while keeping the API key path as the preferred fallback when set).
Changes:
- Add a new
src/oauth.tsmodule implementing discovery, browser launch (no-shell), loopback callback handling, token exchange/refresh, and token caching. - Wire OAuth fallback into
apiFetchinsrc/index.ts(API key still wins when present). - Add unit tests for security-critical pure helpers and a
npm testscript.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/oauth.test.mjs | Adds Node test-runner unit tests for browserCommand, b64url, and assertSafeEndpoint against compiled dist/ output. |
| src/oauth.ts | Implements the keyless OAuth loopback PKCE flow, token persistence, refresh, and endpoint validation helpers. |
| src/index.ts | Switches request auth from “API key required” to “API key else OAuth Bearer token” with lazy resolution in apiFetch. |
| package.json | Adds a test script to build and run Node tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| mkdirSync(TOKEN_DIR, { recursive: true, mode: 0o700 }); | ||
| try { | ||
| if (platform() !== "win32") chmodSync(TOKEN_DIR, 0o700); | ||
| } catch { | ||
| /* best effort — tighten a pre-existing loose dir; ignore if not permitted */ | ||
| } | ||
| writeFileSync(TOKEN_FILE, JSON.stringify(t, null, 2), { mode: 0o600 }); | ||
| memTokens = t; |
| ], | ||
| "scripts": { | ||
| "build": "tsc", | ||
| "test": "tsc && node --test test/*.test.mjs", |
Summary
Make
@mnemoverse/mcp-memory-serverkeyless. With noMNEMOVERSE_API_KEY, the stdio server now runs a one-time browser sign-in (OAuth 2.1 authorization-code + PKCE, loopback redirect RFC 8252), caches + silently refreshes the token in~/.mnemoverse/tokens.json, and calls core withAuthorization: Bearer. API key stays a fallback (CI / headless / self-host) and wins when set. Server still starts auth-free (tools/listworks); auth resolves lazily on the first tool call.New:
src/oauth.ts. Wired insrc/index.ts(apiFetchauth resolution + comment).tscbuilds clean.Why loopback (not device-code)
Verified against the live AS —
auth.mnemoverse.com/.well-known/oauth-authorization-serveradvertises onlyauthorization_code+refresh_token(nodevice_authorization_endpoint). So the loopback browser flow is the correct headless path.🔴 BLOCKED — AS-side change required (verified, not assumed)
GET …/oauth2/authorize?client_id=mnemoverse-cli&redirect_uri=http://127.0.0.1:PORT/callback→ 302 →error=invalid_client"client_id is required". The public CLI client is not registered.Needed in better-auth (auth service): register a public OAuth client
mnemoverse-cli(PKCE, no secret) with loopback redirecthttp://127.0.0.1:*/callback(andhttp://localhost:*/callback) allowed. The hosted AS host-allowlists DCR toclaude.ai/claude.com, so this must be pre-registered server-side. This is a sensitive auth change — founder-gated, separate repo.Test plan (e2e gated on the AS change)
tscbuilds; server starts auth-free;tools/listworks keylessmnemoverse-cli, full sign-in → token cached → write/read succeed (blocked)🎯 Reviewer-voice (security — please scrutinize before publish)
stategenerated + verified on callback.~/.mnemoverse/tokens.json, written0o600.127.0.0.1, ephemeral port; callback validatesstate+ closes server; 5-min timeout.Self-review
npm publishuntil the AS client lands + e2e passes (this PR is draft)🤖 Generated with Claude Code