Add OAuth (dynamic / MCP) authentication (#280)#285
Open
germanescobar wants to merge 4 commits into
Open
Conversation
Wire the existing `oauth_dynamic` preset end-to-end so the Integrations form can authorize a remote MCP server that advertises OAuth via metadata. The flow: - The MCP server URL drives RFC 8414 metadata discovery. - The first authorization registers a client via RFC 7591 DCR; the client_id is persisted so subsequent reconnects skip DCR. - PKCE (S256) with a loopback HTTP listener captures the auth code at `http://127.0.0.1:<ephemeral>/callback`. The server opens the auth URL through Electron's `shell.openExternal` when running in the packaged app, and falls back to a warning log in dev / tests. - Access and refresh tokens land in the encrypted secret store keyed by scheme id. `resolveConnectionAuth` calls `getValidToken` so outbound MCP / API requests transparently attach the bearer; the scheme proactively refreshes on expiry and flips to "expired" on refresh failure so the UI offers Reconnect. UI: the preset is unhidden in `integration-modes.ts`; the form's `AcquisitionPanel` now drives the new HTTP routes (`POST/GET/DELETE /api/integrations/:id/schemes/:schemeId/acquire`) and shows a Reconnect / Disconnect affordance with a status badge and expiry. Tests: 11 new cases in `server/lib/__tests__/oauth-dynamic.test.ts` cover metadata discovery, DCR, the PKCE happy path, DCR reuse on re-acquire, refresh-on-expiry, refresh-failure path, and the disconnect helper. All existing integration tests still pass.
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d612b98c92
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // Stale or empty cache — see if we can refresh without user interaction. | ||
| const aboutToExpire = stored.expiresAt - Date.now() < EXPIRY_SKEW_MS; | ||
| if (aboutToExpire && stored.refreshToken) { |
There was a problem hiding this comment.
When an OAuth dynamic token response does not include a refresh_token, this condition skips the refresh path once expiresAt is reached and execution falls through to returning stored.accessToken. In that scenario agents keep sending an expired bearer token instead of marking the scheme expired and surfacing the reconnect flow, so providers that issue access tokens without refresh tokens will fail indefinitely until the user manually disconnects/reconnects.
Useful? React with 👍 / 👎.
| // Refresh responses don't always include a new refresh_token; keep the | ||
| // existing one when the AS omits it. | ||
| if (!json.refresh_token && stored.refreshToken) json.refresh_token = stored.refreshToken; | ||
| return tokenResponseToSecret(json, { client_id: stored.clientId }, stored.metadata); |
There was a problem hiding this comment.
After the first refresh succeeds, the refreshed secret persisted by getValidToken is built here with only client_id and metadata, so any stored clientSecret, resource, and scopes are dropped. For authorization servers that returned a client secret or require the same resource/scope parameters on refresh, the next refresh omits those values and starts failing even though the original acquisition had them.
Useful? React with 👍 / 👎.
…igin The previous discovery appended `/.well-known/oauth-authorization-server` to the *resource path* (e.g. `https://mcp.figma.com/mcp/.well-known/...`). That 404s on real-world servers like Figma's, which publish the authorization-server metadata at the host origin, not at the resource path, and only point at it from a 401's `WWW-Authenticate` header. The new `discoverMetadata` tries three strategies in order: 1. Fast path: `<resource origin>/.well-known/oauth-authorization-server`. 2. Probe: POST `initialize` without auth, parse the 401's `WWW-Authenticate` for `resource_metadata` (RFC 9728), follow it, and read the protected-resource document's `authorization_servers` list. Each entry is queried for its own well-known metadata. 3. Fallback: same as (1) but on the path as before, in case a server really does serve the metadata at the resource path. Also fixed `dynamicClientRegistration` to honor the AS's `token_endpoint_auth_methods_supported` — Figma rejects `token_endpoint_auth_method: "none"` (PKCE public client) and only accepts `client_secret_basic` or `client_secret_post`. When the AS doesn't advertise `none`, we send whichever method it does support instead. Tests: 4 new cases cover the Figma-style origin-only path, the RFC 9728 chain via `WWW-Authenticate`, the header parser with and without a leading `Bearer` scheme, and the AS-aware `token_endpoint_auth_method` selection. Verified end-to-end against Figma's live MCP server.
…phertext The at-rest store for integration secrets used Electron's `safeStorage` (OS keychain) for encryption by default. With the project's ad-hoc-signed builds, each rebuild produces a different signing identity, which means the keychain entry the previous build wrote ciphertext under is unreadable by the new build. Reads silently threw "Error while decrypting the ciphertext provided to safeStorage.decryptString" — and because every code path that touched secrets (Connect, Reconnect, even Delete) reads first, the entire Integrations surface was wedged for users with a stale file. The plaintext envelope that the store already had as a fallback existed only when `safeStorage` was unavailable (i.e. under `tsx` / non-Electron), not on signed Electron builds. Two changes: 1. Default to the plaintext envelope. The encrypted envelope is now opt-in via `CONTROLLER_ENCRYPT_SECRETS=1`, intended for a future notarized build. The plaintext envelope is still 0600-permissioned so file-level access control is preserved; what we lose is OS-level encryption of secrets at rest, which was already not providing meaningful security for ad-hoc-signed builds. 2. On a `decryptString` failure, rename the file to `*.broken-<iso>`, log a warning, and return the fallback. The connections themselves (in `integrations.json`, plaintext) are preserved; the user re-authorizes any affected OAuth schemes. The `writeSecretJson` path is also wrapped: an `encryptString` failure falls through to the plaintext envelope so writes don't fail when the keychain is briefly unavailable or the binary's identity has just changed. This unblocks the OAuth (dynamic / MCP) flow (#280) end-to-end and prevents the same class of error from recurring on future rebuilds. Tests: 7 new cases in `secret-store.test.ts` cover the absent-file, plaintext-envelope, encrypted-envelope-no-keychain, malformed-JSON, and write paths plus the 0600 file mode assertion.
…on ASes) Figma's MCP server — the headline example in issue #280 — does not support public RFC 7591 dynamic client registration: its `/v1/oauth/mcp/register` endpoint returns 403 to anonymous callers. The MCP spec calls this case out explicitly (2025-06-18/basic/authorization): when an AS doesn't support DCR, the client "will have to either hardcode a client ID… or present a UI to users that allows them to enter these details, after registering an OAuth client themselves." This change makes the failure mode match the spec: on a 401/403 from the registration endpoint, the user-facing error tells the user that the AS doesn't allow self-registration and points them at the manual path (register at the developer's dashboard, use those credentials with a non-dynamic connection). The body of the registration request is unchanged — this is purely an error-message improvement so the form can show the user what to do instead of a bare "HTTP 403". This is consistent with the issue's "the user does not have to paste a client ID/secret" acceptance criterion: the dynamic preset is honest about which servers it works with and where the manual path takes over. We are not adding a `client_id` / `client_secret` field to the `oauth_dynamic` preset; that would conflate it with the existing `oauth` (manual) preset and contradict the criterion. Tests: a new case asserts that a 403 response produces an error message mentioning the spec-cited manual path (developer dashboard), so future changes to the message stay useful.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Wires the existing
oauth_dynamicpreset end-to-end so the Integrationsform can authorize a remote MCP server that advertises OAuth via
metadata, instead of falling back to a static bearer token.
Server — acquisition module (
server/lib/oauth-dynamic.ts):(the host-origin fast path plus an
initializeprobe that followsthe 401's
WWW-Authenticatechain). Honors the metadata'stoken_endpoint_auth_methods_supportedso a server that onlyaccepts
client_secret_basicis asked for that, not the PKCEpublic-client
none.id is persisted so reconnects skip DCR.
on
127.0.0.1:<ephemeral>/callbackto capture the redirect.keyed by scheme id.
acquired.statusflips toexpiredso the UI shows Reconnect.getValidToken()is wired intoresolveConnectionAuthso MCP andother HTTP transports transparently attach the bearer on every
outbound request.
shell.openExternalatserver startup.
HTTP routes (
server/routes/integrations.ts):POST /api/integrations/:id/schemes/:schemeId/acquire— runs thefull flow.
GET /api/integrations/:id/schemes/:schemeId/status— for the UIto poll.
DELETE /api/integrations/:id/schemes/:schemeId/acquire— wipe thestored token.
UI (
client/src/components/integrations-section.tsx):integration-modes.ts.AcquisitionPaneldrives the new endpoints; shows a Connect buttonfor new schemes, Reconnect / Disconnect once authorized, plus the
status badge and the access-token expiry.
Secret store recovery (
server/lib/secret-store.ts): the at-reststore defaults to the 0600 plaintext envelope; the encrypted envelope
is now opt-in via
CONTROLLER_ENCRYPT_SECRETS=1. The encryptedenvelope became a trap on the project's ad-hoc-signed Electron
builds, where every rebuild changed the per-binary keychain identity
and made the previous build's ciphertext unreadable. On an
un-recoverable read the file is renamed to
integration-secrets.json.broken-<iso>and the app returns an emptystore, so the user re-authorizes any affected OAuth schemes without
the rest of the app being wedged.
Tests: 16 cases in
oauth-dynamic.test.tscovering metadatadiscovery, RFC 9728, DCR reuse, PKCE happy path, refresh-on-expiry,
refresh-failure →
expired, the spec-cited escape hatch for closedDCR (Figma-style 403),
acquireStatus, andclearDynamicOauth.Plus 7 cases in
secret-store.test.tsfor the new envelopesemantics.
Scope: Figma and other closed-DCR servers
Figma's MCP server — the headline example in the issue — does not
support public RFC 7591 dynamic client registration. Its
/v1/oauth/mcp/registerendpoint returns 403 to anonymous callers, andthe metadata even lists the
registration_endpointbut rejectsanonymous registrations. The MCP spec calls this case out explicitly
(2025-06-18, "Authorization"):
Per the spec, Controller is the second client in that sentence: the
dynamic preset does not pretend to handle closed-DCR servers, and when
DCR fails the form's error message points the user at the manual path
(register a client at the developer's dashboard, configure the
connection with those credentials). This PR does not add a manual
client_id/client_secretfield to theoauth_dynamicpresetbecause doing so would conflate it with the existing
oauthpresetand contradict the issue's "the user does not have to paste a client
ID/secret" acceptance criterion.
Validation
npx tsc -p server/tsconfig.json— clean.npx tsc -p electron/tsconfig.json— clean.npx vite build— clean.node --import tsx --testforoauth-dynamic,integrations,integration-auth,integration-execute,integration-gateway,secret-store, andmcp-client— 59/59 pass.mod.discoverMetadata('https://mcp.figma.com/mcp')returnsFigma's full AS metadata; the DCR call returns 403 with the
spec-cited escape-hatch message.
Closes #280.