Repository navigation
fix(extensions): resolve custom MCP auth during registration - #7024
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
🚅 Deployed to the ironclaw-pr-7024 environment in ironclaw-ci-preview
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds RFC 9728 protected-resource metadata fallbacks and hosted MCP authentication selection. The flow spans authentication probing, OAuth preparation, installation persistence, lifecycle contracts, setup responses, WebUI recovery, and integration coverage. ChangesHosted MCP authentication and OAuth discovery
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔎 Review · PR #7024
Submitted review →Reviewed the complete trusted base-to-head comparison. The hosted-MCP authentication handling correctly preserves credential setup for metadata-less 401 responses, and the manifest-only bounded CAS update preserves concurrent normalized membership and credential state. No concrete actionable defects were found. Automatic · PR opened · attempt 1 of 3 · completed in 1m 49s Run details
|
There was a problem hiding this comment.
🔍 Review complete · PR #7024
✅ No actionable findings
Reviewed the complete trusted base-to-head comparison. The hosted-MCP authentication handling correctly preserves credential setup for metadata-less 401 responses, and the manifest-only bounded CAS update preserves concurrent normalized membership and credential state. No concrete actionable defects were found.
Validation and technical details
- Inspected all 7 changed files and surrounding hosted-MCP discovery, lifecycle preparation, persistence, compatibility projection, and fixture code.
- Traced ExtensionInstallationStorePort::upsert_manifest_only through its default contract, Arc delegation, production implementation, and libSQL/PostgreSQL parity tests.
- Verified the new CAS transform checks installation identity, manifest identity/reference, timestamp freshness, removal state, and active mutation leases while retaining child membership and credential rows.
- Traced bare MCP AuthRequired classification through sanitized runtime responses into CredentialsRejected and the structured lifecycle credential blocker.
- Reviewed wrong-token, successful continuation, malformed endpoint, unquoted metadata, stale refresh, identity mismatch, concurrent membership, persistence reopen, and tool-publication assertions.
- git diff --check passed for refs/ironloop/base..refs/ironloop/head.
- Targeted cargo tests could not be executed because cargo is not installed in the review environment (
cargo: command not found). - Base:
main - Head:
mcp-registration-followupsat543d8f8 - Run:
6c072343-ddb1-44f9-8db5-1c7d6d6f4739
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.96% — 324666 / 377676 lines Per-crate breakdown (63 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (18 entry/entries excluded from the accounting above)
|
…owups # Conflicts: # crates/ironclaw_extension_host/src/hosted_mcp_preparation.rs
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Resolve custom MCP authentication during registration with safe OAuth/bearer selection, no persistence for ambiguity, and idempotent retries.
Stats: 8 findings (from 8 raw, 8 after dedup) across 4 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0. Evidence quality: degraded (CodeGraph unavailable in exact-head codeload snapshot).
Bugs
- Medium Bearer selection closes setup before token entry (
crates/ironclaw_webui/frontend/src/pages/extensions/components/configure-modal.tsx:73-78, confidence 95) — anchor: crates/ironclaw_webui/frontend/src/pages/extensions/components/configure-modal.tsx:75
Selecting Bearer invokes the backend activation path, which returns a setup-needed response containing the bearer-token credential requirement, but this success callback immediately closes the configure modal. The parent only invalidates queries, so the user cannot enter the required token in the current flow and must rediscover and reopen the extension setup.
Fix: Keep the modal open and update its setup state when authentication selection returns credential blockers, or explicitly reopen the refreshed setup modal instead of calling onClose.
Performance
- Medium Concurrent registrations duplicate remote authentication probes (
crates/ironclaw_extension_host/src/hosted_mcp_preparation.rs:88-119, confidence 88) — anchor: crates/ironclaw_extension_host/src/hosted_mcp_preparation.rs:88
The existence check occurs before any per-extension synchronization, so concurrent Auto/OAuth registrations for the same extension can all observe no definition and independently perform the MCP handshake plus OAuth metadata requests. Only afterward do they serialize at admission; this multiplies remote traffic and latency during retries or concurrent tabs.
Fix: Add keyed per-extension single-flight coordination around the existence check and authentication resolution, while retaining the global lock only for durable admission and catalog updates. - Medium Registration permits unbounded concurrent outbound probes (
crates/ironclaw_extension_host/src/hosted_mcp_preparation.rs:189-194, confidence 72) — candidate — validate claim — anchor: crates/ironclaw_extension_host/src/hosted_mcp_preparation.rs:189
Each authenticated registration can trigger an MCP initialize request plus up to three sequential OAuth metadata requests, each with a 10-second timeout, before any durable admission. These probes run outside the lifecycle operation lock and the existing per-caller request rate limit does not cap concurrent in-flight work or impose a global/tenant budget. An attacker can submit many unique endpoints concurrently to consume outbound connection and task capacity.
Fix: Enforce a bounded global or per-tenant semaphore for registration probes and reject or queue requests when the in-flight budget is exhausted.
Tests
- Medium Selected OAuth client profiles lack caller-level coverage (
crates/ironclaw_extension_host/src/hosted_mcp_preparation.rs:727-803, confidence 85) — anchor: crates/ironclaw_extension_host/src/hosted_mcp_preparation.rs:727
All integration OAuth registration tests pass client_profile_id: None. The selected-profile branch through SharedOAuthProfiles is only covered by frontend serialization, so successful profile resolution and unknown-profile rejection could regress without detection.
Fix: Add integration coverage for explicit OAuth registration with a valid selected profile and an unknown profile that leaves registration unpersisted.
Conventions
- Medium Preserve causes when mapping admission validation errors (
crates/ironclaw_auth/src/engine/admission.rs:136-167, confidence 100) — anchor: .claude/rules/error-handling.md:17-20
The new admission validation paths repeatedly use map_err(|_| AuthProductError::MalformedConfig), discarding the underlying parsing and endpoint-validation causes. This violates the repository error-handling rule requiring server-side causes to be retained or logged even when client-facing errors are sanitized.
Fix: Use cause-preserving error constructors or log the bound source before returning the sanitized AuthProductError. - Medium Do not discard hosted-MCP validation causes (
crates/ironclaw_extension_host/src/hosted_mcp_preparation.rs:786-792, confidence 100) — anchor: .claude/rules/error-handling.md:17-20
The OAuth preparation path maps endpoint construction, canonicalization, and vendor-ID failures with map_err(|_| name_unavailable()), dropping each source error without logging or preserving it. The repository requires sanitized boundary errors to retain the server-side cause.
Fix: Replace the discarded-error mappings with cause-preserving constructors or log each source error before sanitizing it.
Local-Patterns
- Low Document the new hosted-MCP auth lifecycle method (
crates/ironclaw_extension_host/src/product_lifecycle.rs:258-263, confidence 85) — anchor: crates/ironclaw_extension_host/src/product_lifecycle.rs:258
The new public select_hosted_mcp_auth method has no doc-comment explaining its explicit-selection and authorization behavior, unlike the neighboring public lifecycle method. This makes the new lifecycle surface harder to navigate and use correctly.
Fix: Add a concise doc-comment describing that the method validates the caller and persists an explicit hosted-MCP authentication selection.
Maintainability
- Medium Option hides distinct OAuth preparation outcomes (
crates/ironclaw_extension_host/src/hosted_mcp_preparation.rs:718-797, confidence 92) — anchor: crates/ironclaw_extension_host/src/hosted_mcp_preparation.rs:718
prepare_oauth_manifest now returns Option, but None represents multiple different states: derived metadata was unavailable, or automatic OAuth metadata lacked dynamic client registration. Callers then reinterpret that same None differently for registration, activation, and explicit OAuth, spreading the real state machine across nested matches and making future outcome changes easy to misroute.
Fix: Return an explicit outcome type (for example resolved, auth-selection-required, and invalid/unavailable metadata), or split registration resolution from activation preparation so each function has one unambiguous result contract.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated no new comments.
Suppressed comments (2)
crates/ironclaw_extension_host/src/hosted_mcp_preparation.rs:953
registration_request_matchestreats an incomingauth_selection: Autoas matching any existing registered definition, even if the existing definition was explicitly registered asBearer. That means a client can submitAutoand silently get back a persisted Bearer-auth definition, which contradicts the new contract whereAutoonly resolves toNoAuthor validatedOAuthand otherwise requires an explicit selection.
match request.auth_selection.as_ref() {
None | Some(HostedMcpAuthSelection::Auto) => true,
Some(selection) => &mcp.registration_auth == selection,
}
crates/ironclaw_webui/frontend/src/i18n/en.ts:1386
extensions.customMcpAuthHintis now used both for the registration modal (which offers only OAuth/Bearer in the auth-selection-required path) and for the setup ConfigureModal (which currently renders an additionalno_authradio). The current copy explicitly says “Choose OAuth or bearer token…”, which is inconsistent with the ConfigureModal choices and can mislead screen-reader users via thearia-label.
"extensions.customMcpAuthHint": "This server requires authentication. Choose OAuth or bearer token to finish registration.",
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 40 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/ironclaw_webui/frontend/src/pages/extensions/hooks/useExtensions.ts:662
useHostedMcpAuthSelectiononly invalidates theextensionsandextension-setupqueries ononSuccess, but the mutation can still change server-side state and return an error (e.g., it re-projects into the same auth-selection setup blocker with a new message). In that case the UI won't refresh and can keep showing stale setup state. Move the invalidations toonSettled(or addonError) so the setup view is refreshed even when the mutation throws.
onSuccess: (res) => {
queryClient.invalidateQueries({ queryKey: ["extensions"] });
queryClient.invalidateQueries({ queryKey: ["extension-setup", packageKey] });
if (onSuccess) onSuccess(res);
},
});
…7024) * fix(extensions): handle metadata-less MCP auth challenges * fix(tests): follow extension contract split * test(extensions): cover concurrent MCP refresh * fix(extensions): resolve hosted MCP auth setup * fix(extensions): keep rejected auth choice recoverable * fix(extensions): preserve finalized no-auth state * fix(extensions): persist unresolved auth recovery * fix(extensions): release failed preparation checkpoints * fix(extensions): fence preparation lease cleanup * docs(extensions): clarify fenced lease cleanup * fix(mcp): resolve auth during registration * fix(extensions): address MCP lifecycle review findings * refactor(mcp): share hosted client setup * fix(mcp): require explicit auth when OAuth setup is unusable * ci: map hosted MCP support to integration lane * fix(mcp): address registration review findings * test(mcp): cover auth recovery paths * test(mcp): satisfy changed coverage gate
Summary
Autonow performs only a credential-free MCP initialization handshake: success resolves toNoAuth; validated RFC 9728/RFC 8414 metadata resolves to OAuth only when a usable client path exists; metadata without dynamic client registration (and no selected client profile), or an otherwise unexplained 401, returns typedauth_selection_requiredand persists nothing.Change Type
Linked Issue
Follow-up to #6930 and its merged review comments.
Validation
cargo fmt --all -- --check-D warningsfor MCP, extension host/manager, product/contracts, and WebUI.cargo test -p ironclaw_mcp(32 unit, 36 adapter-contract, 5 dispatch tests passed).cargo test -p ironclaw_architecturepassed.origin/mainat80a433a38.$code-review-localRuntime, Structure, and Verification lanes plus strict quality review converged on the final delta.Test Strategy
User behavior: registration either completes with a proven no-auth/OAuth mode or remains in the registration wizard asking for OAuth versus bearer. An unresolved
Autoattempt creates no extension, so installation never becomes the first place users must classify server authentication.Risk areas:
Tests added or strengthened:
initialize+notifications/initialized, discards its session, never lists tools, and fails closed for unsupported transport/missing URL.NoAutheven with an empty catalog; Auto OAuth and explicit OAuth validate metadata; GitHub-shaped OAuth metadata without DCR returns the registration blocker and persists nothing; a bare 401 does the same; a wrong OAuth retry still persists nothing; explicit bearer proceeds to credential setup and wrong/correct-token recovery; exact retries perform no additional network request.validation_code=auth_selection_requiredonfield=auth_selection, not genericinvalid_value; mutation messages survive authoritative lifecycle reprojection, with caller-level setup coverage.Test tiers:
ironclaw_architecturesuite.Commands run include:
cargo test -p ironclaw_mcp -- --nocapturecargo test -p ironclaw_reborn_integration_tests --test reborn_integration_hosted_mcp_registration -- --nocapturecargo test -p ironclaw_webui --test webui_v2_handlers_contract register_hosted_mcp -- --nocapturecargo test -p ironclaw_architecture -- --nocapturecargo clippy ... --all-targets -- -D warningstsc --noEmitcargo fmt --all -- --checkandgit diff --checkPublic MCP evidence:
Security Impact
Authentication discovery changes, but the probe uses the existing host-mediated runtime egress path. Raw challenge values, response bodies, and credentials remain outside the product wire. Metadata fetches remain HTTPS-only, bounded, redirect/private-range mediated, and credential-free. No package definition is persisted from an ambiguous or invalid OAuth registration. Tenant members still cannot mutate tenant-owned extension authentication.
Reborn Trust-Boundary Checklist
Database Impact
No schema or migration change. Unresolved registration writes nothing. Existing bounded CAS behavior preserves normalized membership and credential rows for prepared installations.
Blast Radius
Hosted-MCP registration/preparation, the concrete host-mediated MCP HTTP client, product-surface error projection, and the custom-MCP registration modal. No provider-name heuristic or new generic discovery trait was added.
Rollback Plan
Revert this PR. No migration or external state rewrite is required; already registered extensions remain removable and re-registerable.
Review Follow-Through
Historical #6930 comments and all eight review threads on this PR were checked against commit history and live code. Six prompted focused fixes; two suggestions were declined after strict quality review because they added state or coordination without changing the contract. Valid code/contract issues were fixed without weakening valid tests. The one intentionally changed integration scenario asserted the obsolete install-time auth-choice flow; it now proves the stronger registration-time no-persistence contract while preserving bearer credential recovery.
HANDOFF.mdand.preserved/remain local and uncommitted.Review track: C (network authentication, authority, lifecycle, and persistence concurrency)