feat(oauth): enable profile-bound remote authorization - #180
Conversation
📝 WalkthroughWalkthroughThe change adds a standards-compatible remote OAuth runtime for exact v3 HTTPS Streamable HTTP connections, including strict configuration validation, browser PKCE authorization, profile-bound vault credentials, refresh/reconnect handling, transport integration, redacted errors, and comprehensive tests and documentation. ChangesRemote OAuth runtime
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config/schema.ts`:
- Around line 70-89: Unify client-id-metadata URL validation between
oauthClientRegistrationSchema and isSafeHttpsUrl by reusing a shared validator
or adding the missing query-string restriction to isSafeHttpsUrl. Ensure both
paths accept and reject identical URLs, including rejecting URLs with non-empty
search components while preserving the existing HTTPS and credential
restrictions.
In `@src/oauth/oauth-metadata-fetch-guard.ts`:
- Around line 19-50: The OAuthMetadataFetchGuard cache currently stores issuer
support from any authorization metadata response, allowing unrelated or poisoned
responses to affect later requests. In the constructor’s fetch wrapper, validate
that the metadata response issuer matches the requested URL origin/issuer before
adding it to issuerResponseSupport, or scope the cache per request; preserve
issuerResponseSupported’s lookup behavior for valid matching discovery
responses.
In `@src/oauth/remote-oauth-credential-refresher.ts`:
- Around line 26-28: Distinguish cancellation from genuine reauthentication in
refresh(): add or reuse a local cancellation throw path such as
refreshCancelledLocally() that produces the OAUTH_REFRESH_CANCELLED outcome, and
replace the signal.aborted branches in refresh() (including the checks around
lines 89–95 and 109–110) while keeping reauthenticationRequired() for missing or
malformed refresh-token cases.
In `@src/oauth/secure-credential-store.ts`:
- Around line 6-7: Update maximumSerializedCredentialBytes and the save flow
around validateCredential so envelopes containing all four bounded fields cannot
be written successfully and later rejected by parseEnvelope; either widen the
serialized limit to cover clientId and clientSecret or enforce the existing
limit on the serialized payload before keyring.setPassword, preserving
fail-closed behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 78c1b234-3b22-4382-ab81-ff34d1700df7
📒 Files selected for processing (37)
CHANGELOG.mdREADME.mddocs/architecture.mddocs/config.mddocs/library-api.mddocs/oauth-console-threat-model.mddocs/oauth-support.mddocs/security.mdsrc/cli/exit-codes.tssrc/config/generate-json-schema.tssrc/config/schema.tssrc/mcp/server/miftah-server.tssrc/oauth/connection-lifecycle.tssrc/oauth/loopback-authorization-handoff.tssrc/oauth/oauth-metadata-fetch-guard.tssrc/oauth/remote-oauth-client-provider.tssrc/oauth/remote-oauth-credential-refresher.tssrc/oauth/remote-oauth-runtime.tssrc/oauth/secure-credential-store.tssrc/runtime/create-miftah-runtime.tssrc/runtime/create-runtime.tssrc/runtime/resolve-runtime-config.tssrc/upstream/upstream-process-manager.tssrc/utils/errors.tstests/cli-exit-codes.test.tstests/config-schema-contract.test.tstests/helpers/fake-remote-upstream.tstests/oauth-config.test.tstests/oauth-console-threat-model-docs-contract.test.tstests/oauth-loopback-handoff.test.tstests/oauth-secure-credential-store.test.tstests/oauth-support-docs-contract.test.tstests/readme-product-contract.test.tstests/remote-oauth-client-provider.test.tstests/remote-oauth-credential-refresher.test.tstests/remote-oauth-runtime.test.tstests/remote-oauth-transport.test.ts
💤 Files with no reviewable changes (1)
- src/runtime/resolve-runtime-config.ts
|
Independent local Claude Opus review completed on the original PR head. No correctness or security blocker was found. The actionable low-severity findings were resolved on
One efficiency advisory was intentionally not changed: the MCP OAuth provider must expose an exact loopback redirect URI, including its dynamically allocated port, before the SDK builds the authorization request. Deferring listener allocation until after the challenge would make that redirect URI unavailable or introduce a port-rebinding race. The listener is single-use and is closed immediately after successful connection or on every failure/manager cleanup path. Post-review validation:
|
All four actionable findings were addressed or verified with regression evidence on 96dfd95, every review thread is resolved, and the incremental CodeRabbit review is explicitly rate-limited. An independent local Claude Opus review also found no blocker.
Closes #82
Summary
Enable standards-compatible remote MCP OAuth for exact profile-bound Streamable HTTP upstreams. The implementation covers Protected Resource Metadata and authorization-server discovery, PKCE S256 browser authorization, RFC 8707 resource binding, RFC 9207 issuer-response validation, three explicit client registration modes, encrypted credential persistence, refresh, and authenticated reconnect.
The runtime and published schema now reject unsupported or ambiguous registration values, preserve the actual granted scope set, and keep every credential tied to its config, profile, upstream, resource, and issuer identity.
Security impact
Validation
npm run lintnpm run typechecknpm test— exact-head Linux quality passed.npm run buildnode dist/cli/main.js schemanpm run check:packCHANGELOG.mdare updated when applicable.Additional evidence: