feat(mcp): collect custom request headers for remote MCP servers#1503
Draft
neubig wants to merge 5 commits into
Draft
feat(mcp): collect custom request headers for remote MCP servers#1503neubig wants to merge 5 commits into
neubig wants to merge 5 commits into
Conversation
Some hosted MCP servers authenticate via custom request headers rather than a single Bearer token or OAuth (e.g. Datadog's DD-API-KEY / DD-APPLICATION-KEY, https://docs.datadoghq.com/mcp_server/setup/?tab=other#authentication). The agent-canvas plumbing already supported arbitrary headers end-to-end (MCPServerConfig.headers, MCPSSEServer/MCPSHTTPServer.headers, mcp-service.api.ts, use-update-mcp-server.ts, and the backend mcp_router + fastmcp transport + encrypted settings), but the UI could not produce a header-based config, and a header-based config was silently corrupted on reload. Changes: - utils/mcp-config.ts: - parseMcpConfig now preserves non-Authorization headers onto the parsed server object (previously they were dropped, causing data loss on the next save). - getRemoteCredentialFields merges api_key (-> Authorization via ??=) and explicit headers into one headers map instead of emitting one or the other, matching the backend's headers.setdefault semantics. - hooks/mutation/use-add-mcp-server.ts: forward server.headers on add for sse/shttp (use-update-mcp-server already did this on edit). - utils/mcp-marketplace-utils.ts: add a local McpRemoteTransport view that includes the optional headerFields field, so the install modal is typed against the not-yet-published extensions schema (extensions PR #364). - components/features/mcp-page/install-server-modal.tsx: for shttp/sse options with headerFields, seed field state, render one input per header (password -> save-as-secret toggle, same pattern as stdio envFields), validate required fields, build a headers map in the submit payload, and save marked fields as secrets. The single Bearer-style api_key input is now shown only for api_key/bearer/basic strategies. - components/features/settings/mcp-settings/mcp-server-form.tsx: add a KEY=VALUE-per-line Headers textarea for sse/shttp in the manual custom editor, so users can hand-add header-based servers (e.g. Datadog) without a catalog entry. Authorization is rejected there to avoid a confusing double header (the API Key field is the canonical Bearer source). - i18n: add SETTINGS$MCP_HEADERS_LABEL, SETTINGS$MCP_HEADERS_HELP, and SETTINGS$MCP_ERROR_HEADERS_AUTHORIZATION_RESERVED with translations for all 15 supported languages. Tests: - mcp-config.test.ts: 6 new cases covering header preservation, round-trip, api_key+headers merge, hand-authored Authorization not clobbered, and parsing Authorization+extra headers back into api_key+headers. - install-server-modal.test.tsx: 3 new cases for headerFields-based installs (renders one input per header + no api_key field, blocks on empty required header, installs with headers map and no api_key/Authorization). Closes #1501. Co-authored-by: openhands <openhands@all-hands.dev>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
Contributor
There was a problem hiding this comment.
🟡 Acceptable — Correct, well-motivated feature with solid data-layer test coverage. Two issues worth addressing before merge.
What this PR does
Fixes a silent data-loss bug (custom headers dropped on reload) and enables headerFields-based authentication in the install modal, allowing providers like Datadog that authenticate via named request headers (DD-API-KEY / DD-APPLICATION-KEY) to be installed from the marketplace catalog.
The data-flow is clean end-to-end:
parseMcpConfignow preserves non-Authorizationheaders asextraHeadersgetRemoteCredentialFieldsmergesapi_key(→Authorization: Bearer …) and explicitheadersinto a single map using??=semantics that match the backend'sheaders.setdefault("Authorization", …)behaviour inmcp_router.py- The install modal renders per-field inputs for
headerFields, validates required fields, and builds the correct payload mcp-server-form.tsxadds a manualKEY=VALUEtextarea for header-based servers without a catalog entry
Issues
[IMPROVEMENT OPPORTUNITIES]
[mcp-server-form.tsx, Line 253]Redundant alias:formatHeadersis a one-liner that delegates toformatEnvironmentVariableswith no added behavior. Remove the alias and callformatEnvironmentVariables(server?.headers)directly at the call site (line 421). The current indirection implies a behavioral difference that doesn't exist.
[TESTING GAPS]
[__tests__/.../mcp-server-form.validation.test.tsx]validateHeadersFormatis untested: The newvalidateHeadersFormatinmcp-server-form.tsxadds theAuthorization-rejection rule and theKEY=VALUEformat check, butmcp-server-form.validation.test.tsxwas not updated to cover it. TheAuthorizationrejection in particular is worth a regression test — it's the primary guardrail against a confusing double-header scenario. Add cases for: valid headers pass, invalid format rejected,authorization(case-insensitive) rejected, and that the builtMCPServerConfigcarries the parsedheadersmap.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
All changes are frontend-only. The bug fix is safe (preserves headers rather than dropping them). The??=merge semantics are correct and match the backend. The defensivedelete headers.AuthorizationinbuildConfiglimits the blast radius of any validation bypass. No backend changes required.
VERDICT:
✅ Worth merging — Core logic is sound and data-layer coverage is thorough. The two items above (alias removal and form-validation tests) are polish; they don't block the feature.
KEY INSIGHT:
The cleanest decision in this PR is the ??= merge strategy in getRemoteCredentialFields — hand-authored Authorization wins over api_key-derived, which mirrors backend semantics and makes the round-trip deterministic.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
| return null; | ||
| }; | ||
|
|
||
| const formatHeaders = (headers?: Record<string, string>): string => |
Contributor
There was a problem hiding this comment.
🟡 Suggestion: formatHeaders is a transparent alias — it adds an extra named abstraction without any distinct behavior. The call site on line 421 could just read formatEnvironmentVariables(server?.headers) directly, the same way line 515 already calls formatEnvironmentVariables(server?.env) without a wrapper. Remove the alias to avoid implying a behavioral difference that doesn't exist.
The Headers textarea placeholder and the SETTINGS$MCP_HEADERS_HELP translation both used DD-API-KEY as the example, which is Datadog-specific. Replace with the generic HEADER_NAME=value so the manual editor stays provider-neutral. Co-authored-by: openhands <openhands@all-hands.dev>
…ll modal Some hosted MCP servers use a region/account-specific URL (e.g. Datadog's https://mcp.<site>.datadoghq.com/v1/mcp), so a catalog-pinned URL can return 403 for users on a different site. When a transport declares `urlEditable: true` (extensions PR #364), render the URL as an editable input pre-filled with the catalog URL instead of read-only, track it in field state, validate it on submit, and use the user-edited value in the install payload. Read-only URLs (the default) are unchanged, so existing integrations keep their pinned-host behavior. Adds a test exercising urlEditable: the URL input is enabled and pre-filled, and the user-edited URL is what gets persisted. Co-authored-by: openhands <openhands@all-hands.dev>
getMcpMarketplaceCatalog filtered by getDefaultMcpConnectionOption (the first MCP option), which can be OAuth. An OAuth-only entry (e.g. the published Datadog catalog before its `api` option lands) has no option the local install modal can render, so clicking its card opened an empty modal. Filter by getInstallableMcpConnectionOption instead so entries with no non-OAuth MCP option don't get a card at all. Adds tests: OAuth-only entries are excluded; entries with an OAuth default but a non-OAuth fallback (Slack) are kept. Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Contributor
|
PR Artifacts Notice This PR contains a
|
Contributor
✅ Mock-LLM E2E Tests60/60 passed Commit: Details
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
✅ Mock-LLM Docker E2E Test Results60/60 passed Commit: Details
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
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
4 participants
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.
HUMAN:
Human-tested checkbox left unchecked; this is an AI-authored PR pending human QA.
AGENT:
End-to-end verification performed locally:
npx tsc --noEmit— clean (0 errors).npx eslint+prettier --checkon all changed files — clean.npx vitest runfor the mcp / i18n / hooks / mcp-service suites — 204 passed, including 6 newmcp-configcases and 3 newinstall-server-modalcases exercisingheaderFields.install-server-modaltest "installs with the headers map and no api_key in the persisted config" confirms a Datadog-style entry produces a persistedmcp_configwithheaders: { "DD-API-KEY": …, "DD-APPLICATION-KEY": … }and noapi_key/Authorization, and the pre-flight test payload carries the sameheadersmap.mcp-configround-trip test confirms a persisted header-based config survivesparseMcpConfig→toSdkMcpConfigwithout loss (previously the custom headers were dropped on reload).Backend already supports arbitrary headers (
mcp_router._RemoteMCPServerSpec.headers→fastmcptransport, encrypted at rest bysettings/model.py), so no backend change was needed.Why
Some hosted MCP servers authenticate via custom request headers rather than a single Bearer token or OAuth (e.g. Datadog's
DD-API-KEY/DD-APPLICATION-KEY, https://docs.datadoghq.com/mcp_server/setup/?tab=other#authentication). The agent-canvas plumbing already supported arbitrary headers end-to-end, but the UI could not produce a header-based config, and a header-based config was silently corrupted on reload. This closes those gaps in a generalized, provider-agnostic way.Summary
utils/mcp-config.ts:parseMcpConfignow preserves non-Authorizationheaders (previously dropped → data loss on next save);getRemoteCredentialFieldsmergesapi_key+ explicitheadersinto oneheadersmap instead of emitting one or the other, matching the backend'sheaders.setdefaultsemantics.hooks/mutation/use-add-mcp-server.ts: forwardserver.headerson add forsse/shttp(update already did this on edit).install-server-modal.tsx: forshttp/sseoptions withheaderFields, render one input per header (password → save-as-secret toggle, same pattern as stdioenvFields), validaterequiredfields, build aheadersmap in the submit payload, and save marked fields as secrets. The Bearer-styleapi_keyinput is now shown only forapi_key/bearer/basicstrategies.mcp-server-form.tsx: add aKEY=VALUE-per-line Headers textarea forsse/shttpin the manual custom editor (manual escape hatch for header-based servers without a catalog entry);Authorizationis rejected there to avoid a double header.SETTINGS$MCP_HEADERS_LABEL,SETTINGS$MCP_HEADERS_HELP,SETTINGS$MCP_ERROR_HEADERS_AUTHORIZATION_RESERVEDwith translations for all 15 supported languages.Issue Number
Closes #1501. Depends on the extensions schema change in OpenHands/extensions#364.
How to Test
npm installnpm run test(ornpx vitest run __tests__/utils/mcp-config.test.ts __tests__/components/features/mcp-page/install-server-modal.test.tsx) — all tests pass.npm run typecheck— clean.headerFieldsschema from PR fix(frontend): translate MCP marketplace keys into all supported languages #364 to be published, or a local link): open the MCP marketplace, install a Datadog-style entry withheaderFields, confirm two password inputs render, fill them, and confirm the saved server carriesDD-API-KEY/DD-APPLICATION-KEYheaders. Re-open the server in the custom editor and confirm the Headers textarea is populated and re-saving preserves them.Video/Screenshots
Screenshots captured by an AI agent (OpenHands) on behalf of @neubig using placeholder credential values.
Marketplace install modal collecting custom request headers
Custom MCP server editor extra headers field
Type
Notes
headerFieldsfield on theshttp/ssetransport is added in the companion extensions PR (feat(integrations): support custom request headers on MCP shttp/sse transports extensions#364). This PR's frontend readsheaderFieldsdefensively through a localMcpRemoteTransportview so it compiles against the currently published@openhands/extensions0.6.0 types; once extensions ships the update, the local view becomes a no-op.🐳 Docker images for this PR
• GHCR package: https://github.com/OpenHands/agent-canvas/pkgs/container/agent-canvas
ghcr.io/openhands/agent-canvasghcr.io/openhands/agent-server:1.29.0-pythonopenhands-automation==1.0.0a12d410eec2e1f4a960633621ec6c88427e6d0435fbPull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-d410eecRun
All tags pushed for this build
About Multi-Architecture Support
sha-d410eec) is a multi-arch manifest supporting both amd64 and arm64sha-d410eec-amd64) are also available if needed