Repository navigation
feat(mcpl): optional Authorization header for a WebSocket server's credential (tokenTransport) - #561
Conversation
…header A ws(s):// server's credential, its `token` or what its `accessProvider` returns for a dial, is sent as a `token` query parameter, so it lands in the url and in the proxy and server access logs that record urls. `tokenTransport: 'header'` on McplServerConfig sends it as `Authorization: Bearer <credential>` on the WebSocket upgrade instead, with no query parameter; the access provider still runs at every dial and reconnect. `'query'`, the default, dials as before. buildWebSocketUrl appends no token in header mode, the url-derived credential patterns follow the url as dialed, and serverConfigProblems refuses another value, or the setting on an http(s):// or stdio server. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…or an unknown mode at the dial From review: - tokenTransport 'header' with a url that carries its own `token` query parameter is refused (serverConfigProblems and buildWebSocketUrl), since the credential would still be in the url. - buildWebSocketUrl refuses a tokenTransport that is neither mode, so a dial through openTransport or WebSocketTransport.open, past serverConfigProblems, can't fall back to the query. - A header-mode credential holding a control character fails the dial before the client sees it, and a client's own refusal of the upgrade request is reported without its text, which can quote the header. - Tests for a provider credential's failed dial and its log lines. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nfigured From review of tokenTransport 'header': - A header-mode credential must be printable ASCII (U+0020 to U+007E) with no space at either end. Node and Bun send exactly those bytes, so the server checks the credential that was configured, and the redactor's patterns match what it echoes. Outside that set the bytes differed. Node sent a trailing space or TAB and its HTTP parser stripped it, Bun stripped it before sending, and Node sent é as the byte e9 where Bun sent c3 a9. serverConfigProblems refuses a static token outside the set, so it no longer fails every background redial, and the dial refuses an access provider's. Neither refusal quotes the credential. - 'header' with a url that carries userinfo is refused too, beside the url-token refusal in tokenTransportProblem, so the configuration check and every dial refuse it. ws sends the userinfo as the request's Basic credential, and a request has one Authorization header, so whenever header mode's Bearer header carries a credential it replaces the userinfo's. Query mode sends both, as before. - The catch around new WebSocket goes. Under the character rule no credential the runtimes refuse reaches the constructor, and neither runtime's refusal quoted the value. - The tokenTransport refusal follows config-checks.ts. null counts as not set, and the refusal names the value's kind (kindOf), never the value, which could be a credential pasted into the wrong field. - Tests cover the rule's edges at the dial, the static token's refusal, the userinfo refusal, a header-mode dial with no credential, and a header-mode server's text before any dial, its static token and its url's pairs as dialed. The failed-dial test drops a console capture that no line on its path reached. - The fragment says which refusals a dial repeats. Two comments and the pending redaction fragment now say a dial that sends its token in the query. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
We've pushed one commit to this branch, What it changes:
How we checked it, at
Where the PR description no longer holds, since we've left it as you wrote it:
Written by Gladys-2413, and read by Ramona-2416 and Ogden-2688. — Lottie-2661 |
[Medium impact] The PR appears safe to merge; no actionable issues were found. SummaryAdds optional
No actionable issues found. Reviewed the code and tests and checked the header predicate’s boundary cases with Node; did not run the full test suite. Reviews (1) · Last reviewed commit: "fix(mcpl): header mode sends only a cred..." · Reviewed by Greptile |
|
@Anarchid @slimepriestess We're asking one of you to review this pull request, since it changes how the host sends a WebSocket MCPL server's credential. It's The-Glassy's change, with one commit of ours on top, What it changes for a host. A WebSocket MCPL server's config can set What our commit adds. Our earlier comment gives the reasons and runs for each.
Checks at Written by Gladys-2413. — Lottie-2661 |
# Conflicts: # src/mcpl/transport.ts # test/mcpl-ws-transport.test.ts
|
Merged main (with #563) into the branch: 248d6db. In header mode the dial now goes through #563's |
# Conflicts: # changelog.d/redact-recorded-mcp-credentials.fixed.md
|
Main moved again (0.23.0 folded the changelog fragments), so I merged it once more: ea151f4. The only conflict was |
|
Replying to this comment. @The-Glassy Which two tests fail for you, and on what system: the OS, the Node version, and whether the suite runs as root? On Yes, please add the line you offered to — Daniel-2612 |
|
Added the line (ws-auth-header.added.md). The two failures, both on my run of
I can rerun both on Node 24 to confirm, if useful. |
|
Replying to this comment. @The-Glassy Both failures reproduce on Linux with Node 25.2.1, at — Daniel-2612 |
slimepriestess
left a comment
There was a problem hiding this comment.
Approve at 4a4a57a8.
A ws(s):// server's credential in the url is a credential in every access log and proxy log that records urls. Header mode keeps it out, and the default dials exactly as before.
What we ran, on macOS with Node 20.20.2, one of CI's legs (your run was Node 25.2.1, outside the matrix, which is where your two failures come from, as Daniel-2612 confirmed on Linux): npm install, npm run build, npm test. 5,258 tests, 5,230 pass, 0 fail, 28 skipped. The count is above your description's because the head merges a newer main.
Each piece undone alone, rebuilt, and mcpl-ws-transport re-run (34 tests, all green at the head):
- The
Authorization: Bearerheader on the upgrade removed: 3 fail. buildWebSocketUrlputting the token back in the query in header mode: 4 fail.buildWebSocketUrl'stokenTransportProblemcheck ignored (a dial pastserverConfigProblemscould fall back to the query): 1 fails.- The dial-time
headerCarriesrefusal of a provider's credential removed: 1 fails. - The refusal of a url with its own
tokenparameter removed: 1 fails. The userinfo refusal: 1 fails. The unknown-mode refusal: 2 fail. serverConfigProblems' refusal of a header-mode static token a header can't carry removed: 1 fails. Its refusal oftokenTransporton a non-WebSocket server: 1 fails.- The redactor's url patterns built with the token in the query in header mode: 1 fails.
headerCarriesloosened to allow a space at either end: 2 fail.
Every piece we took out is pinned.
A leak hunt against a live local server (the real WebSocketTransport.open from dist, ws 8.22.0, header mode, with Sonnet Weft's probes):
- A server that refuses the upgrade with 401 and echoes the Authorization header in its body and in
WWW-Authenticate: the rejection is "Unexpected server response: 401" with the endpoint. No token in the message or stack. - A 302 to a second server: rejected with the status, and the second server saw no Authorization header.
- An accepting server sees the url with the user's own query kept and no
tokenparameter, andAuthorization: Bearer …. - A peer close whose reason echoes the header carries it on the transport's raw
closeevent (reasonFromPeer: true).closeReasonTextreplaces any peer reason with "websocket closed" (server-connection.ts:117), so it goes no further. That's existing behaviour, not specific to header mode. - No console output in any run.
Read and found sound:
- Header mode drops the token from the url in two places,
buildWebSocketUrland the redactor's patterns. The static token stays in the redaction patterns in both modes, and a provider's token is still noted per dial. - The transport sets no
followRedirects, andwsdefaults to not following, so a 3xx can't carry the Bearer header to another host. - Every refusal names the value's kind, never the value, and quotes nothing of the url.
- With an access provider that returns no credential, header mode dials with neither a header nor a
tokenparameter, which is what the tests pin.
Thank you for the per-dial provider handling and the character rule: both close corners that a "just set the header" version would have left open.
anima-research#578 (no error a host's call gets carries a configured credential) and this branch both edit three regions; each is resolved to keep both: - README.md's redaction paragraph: anima-research#578's text, with this branch's clause that a WebSocket dial "with `tokenTransport: 'header'` sets none" in the query. - src/mcpl/protocol-family.ts: serverTextRedactor takes this branch's `tokenTransport` in its config and returns anima-research#578's ServerTextRedactor. - src/mcpl/transport.ts: the import from protocol-family.js takes both sides' names (headerCarries and tokenTransportProblem from this branch; maskedError, serverTextRedactor and Redaction from anima-research#578). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@The-Glassy We merged
Written by Gladys-2413. — Fiona-2757 |
slimepriestess
left a comment
There was a problem hiding this comment.
Re-affirming at a9ee1072. It's the main merge with #578, which changes the same three places.
- The resolution keeps both sides. This PR's own lines, taken against
mainbefore and after the merge, are unchanged:tokenTransportinserverTextRedactor's config, header mode's url and header handling, and the README's "withtokenTransport: 'header'it sets none". Every other difference is #578's surrounding text (the redactor's return type, the imports, the README's longer paragraph). - Run on macOS with Node 20.20.2: the full suite has 5,315 tests, 5,287 pass, 0 fail, 28 skipped.
mcpl-ws-transportpasses 34/34. - Three of our earlier removals, repeated at this head, each still turn tests red: the Authorization header (3 fail), the url keeping the token in header mode (4), and the redactor's header-mode url (1).
- Sonnet Weft's leak probe, re-run against this head's
dist, gives the same results as at4a4a57a8:- a 401 that echoes the header surfaces only the status;
- a 302 forwards no Authorization;
- the url stays clean;
- no console lines.
Main has moved since the last merge (815bfe3): anima-research#579 merges 0.23.1 back, anima-research#578 keeps credentials out of host errors, anima-research#581 projects mid-turn images, anima-research#584 turns Greptile's status comments off, anima-research#561 adds a WebSocket auth header, anima-research#586 defines dispositions for stubs, anima-research#574 reports an MCP pipe that fails while connecting, anima-research#583 holds module rows from residents, and anima-research#585 sends a stream's chunks and complete to each publish's destination, its server and its channel. Git merges both sides on its own. One test file needs a change: anima-research#585's three tests with the real channel registry start their turn from an external-message that names no channel, which this branch makes a private turn, so none of its prose is published. Their trigger now names the channel they stream to, as this branch's other outgoing-stream tests do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem
A ws(s):// MCPL server's credential, its
tokenor what itsaccessProviderreturns for a dial, is sent only as atokenquery parameter, so it ends up in the url and in the proxy and server access logs that record urls. An http(s):// server already gets it as anAuthorization: Bearerheader. Raised in review of anima-research/connectome-host#343, which handles the host-side masking.Changes
tokenTransport?: 'query' | 'header'onMcplServerConfig.'header':WebSocketTransport.opensends the per-dial credential (statictokenor the access provider's, fresh at every dial and reconnect) asAuthorization: Bearer …on the upgrade, andbuildWebSocketUrlappends notokenparameter.'query', the default: dials exactly as before.Validation, in
serverConfigProblems(new helpertokenTransportProblem) and again inbuildWebSocketUrl, so a dial throughopenTransport/WebSocketTransport.opencan't fall back to the query. It refuses:'query'/'header';'header'with a url that carries its owntokenparameter, or userinfo (a request has oneAuthorizationheader);tokenoutside printable ASCII or with a space at either end.nullcounts as not set; refusals name the value's kind, never the value.Header-mode credentials must be printable ASCII (U+0020–U+007E) with no space at either end; a provider's credential outside that is refused at the dial, without quoting it.
Redaction: the url-derived credential patterns follow the url as dialed (no token in header mode). The static token and noted provider tokens are still replaced in server text as before.
Docs: README (modern MCP table row and a paragraph, plus the redaction note), and
changelog.d/ws-auth-header.added.md.Tests
test/mcpl-ws-transport.test.ts, 11 new tests (3 added in 08d7633):Beareron the upgrade with the url's other query kept, and an access provider's credential fresh on each reconnect.'query':?token=and no header, as before.buildWebSocketUrlin both modes.serverConfigProblemsand at the rawopenTransport, and a urltokenrefused in header mode.Removing the header, the url handling or the validation fails the matching tests.
npm run build && npm test: 4736 pass / 2 fail / 28 skipped (4766 tests). Cleanmainat 9c6964c gives 4728 pass / 2 fail / 28 skipped (4758) with the same 2 failures (the mount boundary: "a root placed at each start", "a path beneath a dangling symlink is not proven absent"), which look environmental here.tsc --noEmit: clean.Not verified
wsshim sending the header was checked in a separate probe (Bun 1.3.13), not in this suite.changelog.d/ws-auth-header.added.mdDescription updated after 08d7633 (maintainers' round): character rule, userinfo refusal,
nullas unset; the catch aroundnew WebSocketis gone.