Commit 6042a14
* redact credentials from upstream success messages
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
* take the upstream password from the environment
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.
* redact upstream credentials in status output
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
* document the upstream password env var
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
* pin the real limits of upstream password delivery
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.
* drop a needless generic and a non-null assertion
* 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
* respond to review: encode the env password, report which credential was 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.
* bind the env password to one host, and mask every field that can carry 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.
* refuse a socks upstream that would truncate the password
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.
* drop the query when redacting a proxy URL
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
* correct the tool descriptions and match IPv6 upstream hosts
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.
* stop leaking the upstream password into spawned processes
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
* refuse a ':' username on every scheme, redact opaque-path URLs
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.
* fix(upstream): accept '%' usernames, tighten claims, add spawn coverage
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.
* refactor(interceptors): share one spawn-env scrub, document password_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.
* test(utils): pin empty-password (user:@host) redaction
#22's acceptance criteria list this case for redactProxyUrl; the behavior
was already correct — the WHATWG parser erases the empty password — but
untested.
---------
Co-authored-by: yfe404 <yfe.github@protonmail.com>
1 parent 215cc9f commit 6042a14
10 files changed
Lines changed: 798 additions & 19 deletions
File tree
- src
- interceptors
- tools
- test/unit
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
71 | 71 | | |
72 | 72 | | |
73 | 73 | | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
74 | 78 | | |
75 | 79 | | |
76 | 80 | | |
| |||
185 | 189 | | |
186 | 190 | | |
187 | 191 | | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
| 270 | + | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
| 291 | + | |
| 292 | + | |
| 293 | + | |
| 294 | + | |
| 295 | + | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
| 299 | + | |
| 300 | + | |
| 301 | + | |
| 302 | + | |
| 303 | + | |
| 304 | + | |
| 305 | + | |
| 306 | + | |
| 307 | + | |
188 | 308 | | |
189 | 309 | | |
190 | 310 | | |
| |||
368 | 488 | | |
369 | 489 | | |
370 | 490 | | |
| 491 | + | |
| 492 | + | |
| 493 | + | |
| 494 | + | |
371 | 495 | | |
372 | 496 | | |
373 | 497 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
30 | 30 | | |
31 | 31 | | |
32 | 32 | | |
| 33 | + | |
33 | 34 | | |
34 | 35 | | |
35 | 36 | | |
| |||
189 | 190 | | |
190 | 191 | | |
191 | 192 | | |
| 193 | + | |
| 194 | + | |
192 | 195 | | |
193 | 196 | | |
194 | 197 | | |
195 | | - | |
196 | | - | |
197 | | - | |
198 | | - | |
199 | | - | |
| 198 | + | |
200 | 199 | | |
201 | 200 | | |
202 | 201 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
9 | 9 | | |
10 | 10 | | |
11 | 11 | | |
| 12 | + | |
12 | 13 | | |
13 | 14 | | |
14 | 15 | | |
| |||
84 | 85 | | |
85 | 86 | | |
86 | 87 | | |
87 | | - | |
| 88 | + | |
88 | 89 | | |
89 | 90 | | |
90 | 91 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
13 | 13 | | |
14 | 14 | | |
15 | 15 | | |
16 | | - | |
| 16 | + | |
17 | 17 | | |
18 | 18 | | |
19 | 19 | | |
| |||
342 | 342 | | |
343 | 343 | | |
344 | 344 | | |
| 345 | + | |
| 346 | + | |
| 347 | + | |
| 348 | + | |
| 349 | + | |
345 | 350 | | |
346 | 351 | | |
347 | 352 | | |
| |||
502 | 507 | | |
503 | 508 | | |
504 | 509 | | |
505 | | - | |
506 | | - | |
| 510 | + | |
| 511 | + | |
| 512 | + | |
| 513 | + | |
| 514 | + | |
| 515 | + | |
507 | 516 | | |
508 | 517 | | |
509 | 518 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
24 | 24 | | |
25 | 25 | | |
26 | 26 | | |
| 27 | + | |
27 | 28 | | |
28 | 29 | | |
29 | 30 | | |
| |||
198 | 199 | | |
199 | 200 | | |
200 | 201 | | |
201 | | - | |
| 202 | + | |
202 | 203 | | |
203 | 204 | | |
204 | 205 | | |
| |||
215 | 216 | | |
216 | 217 | | |
217 | 218 | | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
218 | 231 | | |
219 | 232 | | |
220 | 233 | | |
| |||
254 | 267 | | |
255 | 268 | | |
256 | 269 | | |
257 | | - | |
258 | | - | |
| 270 | + | |
| 271 | + | |
259 | 272 | | |
260 | 273 | | |
261 | 274 | | |
| |||
306 | 319 | | |
307 | 320 | | |
308 | 321 | | |
| 322 | + | |
309 | 323 | | |
310 | 324 | | |
311 | 325 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
5 | 5 | | |
6 | 6 | | |
7 | 7 | | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
8 | 26 | | |
9 | 27 | | |
10 | 28 | | |
11 | 29 | | |
12 | 30 | | |
13 | 31 | | |
14 | | - | |
| 32 | + | |
15 | 33 | | |
16 | 34 | | |
17 | 35 | | |
18 | 36 | | |
19 | | - | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
20 | 41 | | |
21 | 42 | | |
22 | 43 | | |
23 | 44 | | |
24 | 45 | | |
25 | | - | |
| 46 | + | |
| 47 | + | |
26 | 48 | | |
27 | 49 | | |
28 | 50 | | |
| |||
57 | 79 | | |
58 | 80 | | |
59 | 81 | | |
60 | | - | |
| 82 | + | |
61 | 83 | | |
62 | 84 | | |
63 | 85 | | |
64 | 86 | | |
65 | | - | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
66 | 91 | | |
67 | 92 | | |
68 | 93 | | |
69 | 94 | | |
70 | 95 | | |
71 | | - | |
| 96 | + | |
| 97 | + | |
72 | 98 | | |
73 | 99 | | |
74 | 100 | | |
| |||
0 commit comments