Keep upstream proxy credentials out of transcripts (#22, #23) - #24
Merged
yfe404 merged 16 commits intoAug 28, 2026
Conversation
proxy_set_upstream and proxy_set_host_upstream echoed proxy_url verbatim, writing the upstream password into the MCP client transcript. * add redactProxyUrl() in utils: replaces the password with ***, keeps scheme, username, host and port so the confirmation stays useful * username preserved deliberately: for some providers it is configuration rather than a secret (Apify Proxy encodes group, country and session id there) * splices the original string rather than re-serializing, so an authority-only URL does not gain a trailing slash * unparseable input returns a placeholder, never the raw value
MatousMarik
marked this pull request as ready for review
August 27, 2026 11:37
MatousMarik
marked this pull request as draft
August 27, 2026 11:52
MatousMarik
changed the title
An authenticated upstream had to carry its password in the tool call, which the MCP client persists in the transcript. mergeUpstreamPassword() fills the password slot from PROXY_MCP_UPSTREAM_PASSWORD when the URL has a username but no password. The value can only land in the password slot — the field redaction masks — so no echo-back path exists by construction, and URL serialization percent-encodes it, so a password containing "@" or "/" cannot re-point the upstream at another host. One credential for all upstreams; per-provider secrets can wait for a second provider. A URL with no username, or one that already carries a password, is untouched, so existing calls are unaffected. Redaction also masks query values now, covering a pac+http:// token, and re-serializes the URL so a password duplicated into the query cannot survive.
getStatus() returned globalUpstream and hostUpstreams as stored, and both callers serialized them raw: proxy_status (tools/lifecycle.ts) and the proxy://status resource, which clients read unprompted. Status is read far more often than a set-upstream call, so this was the larger writer of credentials into a transcript and left the redaction added earlier only half done. * redact on the way out inside getStatus(), the one place both callers route through * the stored configs keep their real credentials; resolveProxyConfig() reads proxyUrl to route, so mutating them would break proxying * test pins both halves: status carries no raw password, and the stored config still does * getGlobalUpstream()/getHostUpstreams() intentionally still return raw values; they have no callers today, and a future routing consumer needs the real thing
The env-sourced password and the redaction behaviour existed only in tool descriptions. * README upstream section: how to keep the password out of the transcript, what redaction covers, and that the username is deliberately not redacted * mobile section: cross-reference, since proxy_mobile_setup honours the same variable * README:601 needed no change; its "process environment is not exposed" claim is scoped to Camoufox launch/list/info responses, which this does not touch
The round-trip test used a secret containing ":" and asserted losslessness, but it only checked url.parse().auth. socks-proxy-agent@7 then does auth.split(":") and takes [1] (mockttp/node_modules/socks-proxy-agent/dist/index.js:78-81), so "pa:ss" authenticates as "pa" — the test passed while giving false assurance for a socks upstream.
* drop ":" from the lossless-round-trip secret; it holds for everything else
* add a test through the https-proxy-agent path, where ":" is safe
* add a test pinning the socks truncation, so the restriction is visible and the test fails if socks-proxy-agent ever honours the full password
* README: note the socks restriction next to the env var
Not a regression: a literal socks5://user:pa%3Ass@host:1080 truncates identically, with no merge involved.
* redactUpstreamConfig: plain function over UpstreamProxyConfig instead of a generic with a conditional return type and two "as never" casts; the null case falls out of && at the call site * mobile: branch on resolvedUpstream rather than upstream_proxy_url, so the non-null assertion goes away
…as used 1. mergeUpstreamPassword() assigned the env value to url.password raw. That setter escapes "@" and "/" but not "%", while mockttp reads the credential back through url.parse().auth, which decodeURIComponent()s it — so a password of "100%pass" made that decode throw URIError, and "p%20ss" silently authenticated as "p ss". encodeURIComponent() first is lossless: the setter escapes nothing encodeURIComponent leaves alone. Pinned by a round-trip test over "100%pass", "p%20ss" and "%". 2. Omitting the password when the server has no PROXY_MCP_UPSTREAM_PASSWORD was a silent no-op reported as success, surfacing later as unexplained 407s. The set-upstream responses now carry passwordSource: env | url | none. 3. README: the env var has to be in the spawned server's environment, so the 'export' example could not work for a stdio server. Replaced with 'claude mcp add -e' and an .mcp.json "env" block.
…y a token
Second review round.
The env-sourced password was merged on "username present, password absent"
alone, so a caller could name any host and have the credential delivered
there — the proxy sends it on the first request, and the transcript shows only
"***". That is the same exfiltration class that got the ${VAR} design
withdrawn, by delivery rather than echo-back, which redaction cannot see. The
merge now also requires PROXY_MCP_UPSTREAM_HOST to match the URL's hostname
(case-insensitive, exact, port-independent), and fails closed when that
variable is unset so a half-configuration cannot become an unbound credential.
redactProxyUrl() masked userinfo and query values but left the path and
fragment verbatim, so a PAC provider carrying its token in the path — the one
scheme the README singles out — had it echoed in full by proxy_set_upstream,
proxy_status and the proxy://status resource. Path segments and the fragment
are now masked for every scheme; an upstream URL's path is never the useful
part of a confirmation message.
A proxy_url the code cannot parse no longer reports success. Redaction had
removed the echo that used to make a typo self-evident, and nothing downstream
validates it either. No scheme allowlist — an unsupported-but-parseable scheme
failing per request is pre-existing and unrelated to these issues.
passwordSource is omitted for a URL with no username, where the question does
not arise, instead of reporting "none" at a correctly unauthenticated
upstream. Username-only auth is a real credential, so "none" stays a success
rather than an error; the README documents that it cannot be expressed at the
pinned host while the password variable is set.
Also: proxy_mobile_setup's field renamed password_source -> passwordSource for
parity with the other two tools, docblocks trimmed where the PR body and README
already carry the rationale, and a stale review-round label dropped from a
test.
MatousMarik
force-pushed
the
upstream-credential-handling-22-23
branch
from
August 27, 2026 15:39
68c2559 to
120cbe8
Compare
Third review round. socks-proxy-agent splits the credential on the first ":" and keeps only what follows, so an env password of "pa:ss" authenticated as "pa" — silently. The README documented it and a test pinned it, but a caveat is thin protection against a failure that looks like a provider outage. mergeUpstreamPassword() now throws for a socks* upstream when the password contains ":", naming the variable and the remedy. The literal-URL form still truncates; nothing can guard that, and the test says so. Also: isParseableUrl() replaced by URL.canParse(), available since Node 20 and the engines floor is >=20; and the README now says PROXY_MCP_UPSTREAM_HOST must be a bare hostname, since copying the host:port pair out of the proxy URL fails closed and the fail-closed path cannot explain why.
Masking query values still echoed a bare token. "?SECRETTOKEN" has no "=", so it parses as a key with an empty value and came back as "?SECRETTOKEN=***" — the token in full, wearing the mask that is supposed to mean it is gone, so nobody would spot it. It reached the set-upstream message, proxy_status and the proxy://status resource. * drop url.search instead of masking values, matching how the fragment is already handled * less code than masking keys as well, and nothing in a proxy URL query is worth echoing in a confirmation * tests cover the bare-token forms, including one mixed with a normal pair
Two gaps between what the code does and what a caller is told. * the three proxy_url/upstream_proxy_url descriptions still said the password comes from PROXY_MCP_UPSTREAM_PASSWORD alone. Since the host pinning landed, PROXY_MCP_UPSTREAM_HOST must also be set and match, or nothing is merged — an agent reading the old text would expect a merge that silently does not happen. They now state both conditions and mention passwordSource. The README already documented this correctly; the tool descriptions are what an agent actually reads. * url.hostname keeps the brackets on an IPv6 literal, so a bare "::1" — the form the README documents — never matched and the merge failed closed. Strip brackets from both sides before comparing. A different IPv6 host is still refused.
interceptor_spawn and the camoufox launcher both spread ...process.env into the child. Since this PR puts PROXY_MCP_UPSTREAM_PASSWORD in the server environment, any caller could read it straight back:
interceptor_spawn { command: "sh", args: ["-c", "env"] }
That defeats the point of the variable — the premise is that the caller cannot see the password, only have it merged into a URL on their behalf. Verified against a running server before and after.
* delete the password from the child environment in terminal.ts and camoufox.ts
* export UPSTREAM_PASSWORD_ENV rather than restating the literal in three places
* PROXY_MCP_UPSTREAM_HOST stays: it is configuration, not a secret, and a spawned tool may legitimately need to know where traffic goes
Two ways a credential still slipped through, plus a docblock fix. * a ":" in the username was guarded only for socks. Basic auth splits the decoded pair at the first colon too (RFC 7617), so "gro:ups" + "s3cret" reaches an http proxy as user "gro", password "ups:s3cret" — the merged password discarded while the response still says passwordSource: env. The guard is now scheme-agnostic. Checking url.parse().auth alone made http look harmless, which is one layer short of what the proxy reads. * redactProxyUrl left an opaque-path URL untouched: "pac+http:host/TOKEN.pac" has no authority, so assigning pathname is a silent no-op and the token was echoed verbatim. Such a URL is not a usable upstream, so it is reduced to its scheme. * stripBrackets had been inserted between mergeUpstreamPassword and its docblock, so the 15-line comment documented the helper instead. Moved above.
Review round 4. * mergeUpstreamPassword() called decodeURIComponent(url.username), which throws URIError on a bare "%" — "http://100%pass@pinned/" failed with "URI malformed" and no mention of the username. The WHATWG parser always encodes a literal ":" in userinfo as "%3A", so /%3a/i is total and cannot throw. Same failure the password path fixed earlier, reintroduced one field over. * README overstated the spawn fix: interceptor_spawn runs arbitrary commands as the server user and can read the client config the password lives in, so removing it from the child environment is defence-in-depth, not a boundary. Says so now. * README claimed exporting in your own shell does not reach the server. CLI clients pass their environment through, so it can; the advice to use the client config stands, the absolute did not. * mobile: password_source to match its snake_case siblings, and the last non-null assertion is gone. * dropped a socks paragraph that had been pasted above the username guard, where it does not apply. * unit coverage for the spawn scrub, per AGENTS.md. Mutation-checked: removing the delete fails it.
…source Review round 5. * the camoufox scrub was the only behaviour in the diff no test could kill — deleting it left the suite green. terminal.ts and camoufox.ts were building the same child env and repeating the same delete, so both now go through spawnEnv() in utils. One pure function, unit-tested, and removing the scrub now fails three tests instead of none. * proxy_mobile_setup emits password_source (snake_case, matching its siblings) while the README said passwordSource. Both are now stated, and the mobile tool description mentions the field at all — it was the only one of the three that did not, so an agent had no in-band way to learn the key. * dropped the CHANGELOG entry and the 3.4.0 bump. src/index.ts hardcodes the server version separately and I had left it at 3.3.2, so the bump would have shipped a server advertising the old version. Choosing minor-vs-patch for someone else s release cadence is the maintainer s call anyway; the precedent commit cited for it was their own.
MatousMarik
force-pushed
the
upstream-credential-handling-22-23
branch
from
August 28, 2026 10:52
217f45a to
c700849
Compare
MatousMarik
marked this pull request as ready for review
August 28, 2026 11:46
yfe404#22's acceptance criteria list this case for redactProxyUrl; the behavior was already correct — the WHATWG parser erases the empty password — but untested.
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
2 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.
Addresses #22 and #23.
Upstream proxy URLs carry credentials, and both the tool call and the tool
response are persisted in the MCP client transcript. Today an authenticated
upstream writes its password there on every call, with no way to avoid it.
What changed
Credentials are redacted on every echo path.
redactProxyUrl()masks theuserinfo password, masks path segments, and drops the query and fragment. This
covers the set-upstream messages,
proxy_status, and theproxy://statusresource — status is read far more often than a set call, and clients read the
resource unprompted, so it was the larger writer of credentials into a
transcript. The stored config keeps its real credentials;
resolveProxyConfig()needs them to route.
The username is deliberately not redacted. For several providers it is
configuration rather than a secret — Apify Proxy encodes proxy group, country
and sticky-session id there — and it is what makes the confirmation message
worth printing. Documented in the README so a reader can check it against their
own provider.
The password can come from the environment. When
proxy_urlhas a usernamebut no password, it is filled in from
PROXY_MCP_UPSTREAM_PASSWORD, so acredential need not appear in the tool call. Two things make that safe rather
than merely quiet:
masks — so there is no echo-back path.
PROXY_MCP_UPSTREAM_HOSTis set and matches theURL's hostname. Without that, a caller who cannot read the password could
still name any host and have it delivered there, which redaction cannot see.
A half-configuration fails closed.
Responses report
passwordSource:env,urlornone.noneon a URL thatnames a user means no password was applied — otherwise that misconfiguration
reads as success and surfaces later as unexplained 407s.
URL.toString()percent-encodes the merged value, and mockttp decodes it vialegacy
url.parse().auth, so the round-trip is lossless and a passwordcontaining
@or/cannot re-point the upstream at another host.Known limits
socks*://upstreams: no:in the password. socks-proxy-agent keepsonly what follows the first
:, sopa:sswould authenticate aspa. Themerge refuses rather than deliver half a credential. A literal
socks5://u:pa%3Ass@hosttruncates the same way and cannot be guarded.:in the username is refused on every scheme — on the merge path.Basic auth splits the decoded pair at the first colon (RFC 7617) and
socks-proxy-agent does the same, so no format in this stack can carry it. As
with the socks password, a credential supplied entirely in
proxy_urlispassed through untouched and still mis-splits; only what this PR merges in is
validated.
username with no password is exactly the syntax that requests the merge, and
user:@hostcannot signal otherwise, since the parser erases the emptypassword. Unset
PROXY_MCP_UPSTREAM_PASSWORDfor such a server.of reach.
interceptor_spawnruns an arbitrary command as the server user,so a caller can read the client config the password is configured in. Removing
it from the spawned child's environment is defence-in-depth, not a boundary.
Behaviour changes to previously-accepted input
are set and the host matches. With either unset, behaviour is unchanged.
proxy_statusandproxy://statusreport upstream URLs redacted andURL-normalized — an
http://upstream gains a trailing/, and any queryor fragment is dropped. Anything parsing that output sees a different string.
Testing
npm run build && npm test— 158 passed, 0 failed. Unit only:127 on this branch, 89 on
main— the 38 added are all unit tests; nointegration test is touched.
No CI run URL: Actions are not enabled on my fork (
actions/workflowsreturnszero registered workflows, which needs a UI opt-in), and a workflow run on this
PR needs maintainer approval for a first-time contributor. As the closest
substitute, CI's exact steps on CI's Node version:
Approving the workflow run on this PR will produce the real thing.
Exercised end to end against a running server over stdio, with a real upstream
provider, on the head commit:
passwordSource: env; traffic routed through the upstreampasswordSource: nonepasswordSource: url; response shows:***@pac+http://token in query, path, or a bare?TOKENpac+http:host/TOKEN.pacpac+http:***proxy_statusandproxy://status%,%20, space, unicode, every URL-special charurl.parse().auth:in the password on socks / in the username on any schemeinterceptor_spawnreading the server environment::1or[::1]Notes
No
CHANGELOG.mdentry and no version bump — the minor-vs-patch call is yours,and
src/index.tscarries the server version separately frompackage.json, sothe two want bumping together at release time. Happy to add both if you'd rather
they land here.
Commit subjects on this branch are mixed — most are bare imperatives, the last
two are Conventional Commits. Happy to reword them all either way before merge.