Repository navigation
fix(memory-plugin): make MCP proxy extra-header precedence case-insensitive - #6164
Open
akushonkamen wants to merge 2 commits into
Open
akushonkamen wants to merge 2 commits into
akushonkamen wants to merge 2 commits into
Conversation
…sitive headersForRequest merged operator extras (OPENVIKING_EXTRA_HEADERS) under the proxy's own headers with an exact-case object spread, but HTTP header names are case-insensitive: an extra spelled x-openviking-account survived next to the proxy's X-OpenViking-Account and Node fetch sent both, which the upstream joins into 'obsolete, account-a' instead of the configured identity value. Drop extras whose lowercase name collides with a header the proxy already set before merging, so the existing 'proxy headers win' precedence holds regardless of casing. Unrelated extras still pass through, the exact-case spelling keeps the proxy's value as before, and the reserved-name parser and credential/identity configuration are unchanged. Regression tests cover the four case-collision identity headers (account, user, actor-peer, user-agent) plus the unrelated-extras and exact-case controls, asserting the wire values on both initialize and tools/list against a loopback upstream. Fixes volcengine#6158
…hared-lib change The shared runtime in examples/memory-plugin-shared/lib reaches all six distributed plugins, and hosts install by the version string in each manifest, so check-plugin-version-bumps.sh requires each of them to move (required by the plugin-versions check; bash .github/scripts/ check-plugin-version-bumps.sh origin/main exits 1 without this). Bump the manifests, the claude-code package.json that mirrors its plugin.json, the agent-hook host manifests that pair with plugin.json, the dsh wire version reported by config.mjs, and the two lockfiles that npm ci validates.
This branch has not been deployed
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.
Description
The MCP proxy's "proxy headers win" precedence over operator-supplied
OPENVIKING_EXTRA_HEADERSwas exact-case only. Because HTTP header names are case-insensitive, an extra spelledx-openviking-accountsurvived next to the proxy's ownX-OpenViking-Account, and Node fetch sent both. The upstream joined the duplicate into one value, so the wire identity becameobsolete, account-ainstead of the configured account (issue reproduction).Root cause.
headersForRequest(examples/memory-plugin-shared/lib/mcp-proxy-core.mjs:268) merged operator extras with the proxy's self-set headers via an exact-case object spread ({ ...opExtras, ...built }), whilebuildOvHeaders(examples/memory-plugin-shared/lib/ov-http.mjs:31-34) emits canonical-case keys (X-OpenViking-Account,X-OpenViking-User,X-OpenViking-Actor-Peer). An extra key spelled in a different case therefore survived as a second key and reached the wire as a duplicate header, which the upstream joins with a comma.Fix. Before merging, drop extras whose lowercase name collides with a header the proxy already set, so the existing "proxy headers win" precedence holds regardless of casing. Unrelated extras still pass through, the exact-case spelling keeps the proxy's value exactly as before, and the reserved-name parser and credential/identity configuration are unchanged.
Reproduction status. Reproduced through the real production entrypoint: the pre-fix run failed the new regression tests with the actual wire values
stale, zeusandstale, openviking-memory-test/9.9.9, matching the issue's comma-joined values. Not inferred from code alone.Human Involvement
Related Issue
Fixes #6158
Type of Change
Changes Made
examples/memory-plugin-shared/lib/mcp-proxy-core.mjs:headersForRequestnow drops operator extras whose lowercase name matches a proxy-set header before the merge, making the existing precedence case-insensitive (17 lines changed).agent-plugins/servers/shared/mcp-proxy-core.mjs,examples/dsh-memory-plugin/scripts/shared/mcp-proxy-core.mjs,examples/pi-coding-agent-extension/scripts/shared/mcp-proxy-core.mjs).examples/memory-plugin-shared/mcp-proxy-core.test.mjs: 6 regression tests covering the case-collision identity headers (account, user, actor-peer, user-agent) plus unrelated-extras and exact-case controls, asserting wire values on bothinitializeandtools/listagainst a loopback upstream.chore(memory-plugin)) so the shared-runtime change ships with fresh versions, per the release check.Testing
Validation commands and results (macOS, arm64):
node --test examples/memory-plugin-shared/mcp-proxy-core.test.mjsfailed 5 of the 6 new regression tests with the actual wire valuesstale, zeus/stale, openviking-memory-test/9.9.9(the issue's comma-join reproduced). After the fix: 23 tests / 23 pass / 0 fail. Re-run green after rebasing onto the latestmain(base advanced 5 commits; none touched plugin code paths — onlyweb-studioand 4 markdown files underexamples/).node examples/memory-plugin-shared/sync.mjsfirst, thennpm ciin the dsh and pi lanes): 1318 tests / 1307 pass / 3 fail / 8 skipped. The 3 failures are allinstall-tui.test.mjsTRAE cases and reproduce byte-identically on an unmodifiedmainworktree (git worktree add+sync.mjs), so they are a pre-existing environment issue, not introduced by this change.--test-concurrency=1(node --test --test-concurrency=1 install-agent-hooks+release-marketplace): 22/22 pass.node examples/memory-plugin-shared/sync.mjsre-run after the rebase — working tree stays clean (all synced copies are committed).Skipped checks and why: web-studio eslint was not run — this PR does not touch
web-studio/.Checklist
(Documentation checkbox left unchecked intentionally: no user-facing documented behavior changes — the fix restores the precedence the docs already promise.)
Screenshots (if applicable)
Not applicable.
Additional Notes
chorecommit + the three committed sync copies), and the regression tests cover the User-Agent and mixed-case user/actor-peer collisions with unrelated gateway headers kept intact.