Skip to content

feat: route outbound fetches through HTTP_PROXY, HTTPS_PROXY, and NO_PROXY - #17

Open
RonCodes88 wants to merge 2 commits into
only-cli:mainfrom
RonCodes88:feat/http-proxy-env-vars
Open

feat: route outbound fetches through HTTP_PROXY, HTTPS_PROXY, and NO_PROXY#17
RonCodes88 wants to merge 2 commits into
only-cli:mainfrom
RonCodes88:feat/http-proxy-env-vars

Conversation

@RonCodes88

Copy link
Copy Markdown

Summary

  • Honor HTTP_PROXY, HTTPS_PROXY, and NO_PROXY for both impers and native fetch paths
  • Keep SSRF checks on page targets; proxy hosts are not blocked
  • Add offline proxy tests and README note

Closes #9

Test plan

  • npm test

@only-cli

Copy link
Copy Markdown
Owner

Thanks for taking this on, and for the care that went into it. The CONNECT tunnel is done properly: proxy auth stays on the CONNECT hop and never reaches the origin, SNI is omitted for IP literals, and the tunnel test uses ca: instead of switching off certificate verification, which is the right way to write that test. Extracting followRedirects so both transports share one hop check is a real improvement over the two duplicated loops on main.

A few things to sort out before this goes in. Everything below was reproduced against this branch.

Critical

The private address guard stops working for hostnames once a proxy is set. assertSafeTarget decides using a local DNS lookup that swallows failure (src/fetch.js:145, .catch(() => [])), but with a proxy the name is resolved at the proxy. Internal names usually do not resolve on the client, which is exactly the corporate case this feature is for.

HTTP_PROXY set, intranet.invalid is NXDOMAIN locally
fetchPage('http://intranet.invalid/admin') -> 200, internal body returned
proxy log: GET http://intranet.invalid/admin

On main the same call fails ENOTFOUND. IP literals and localhost are still blocked, so the gap is names only. This matters because page URLs are attacker influenced (oc do <n> follows links out of untrusted page content), so it turns "refused" into "fetched" for the class of URL the guard exists to stop. Simplest fix is to fail closed when a proxy is selected and the name does not resolve locally. If you would rather keep it reachable by design, that needs saying plainly in the README, since the guard becomes advisory once a proxy is in play.

High

1. NO_PROXY is not authoritative on the impers path. When resolveProxy returns null the code omits the proxy option, and libcurl then applies its own env parsing. A host the user deliberately excluded still goes through the proxy. Note this only shows up with the lowercase form, since libcurl ignores uppercase HTTP_PROXY on purpose:

http_proxy=http://127.0.0.1:PORT  no_proxy=example.com
resolveProxy -> null (oc intends direct)
impers still went through the proxy

Omitting the option is not the same as disabling it. Passing curl's empty-proxy disable form, or setting its no-proxy option, would make oc's decision stick.

2. Credentials in the page URL are handed to the proxy in the request line. path: target.href at src/fetch.js:270 keeps the userinfo:

proxyGet('http://alice:s3cr3t@example.test/private', proxy)
proxy log: GET http://alice:s3cr3t@example.test/private

Native fetch never puts userinfo in the request line, and your CONNECT path already strips it correctly via authority(), so this is only the absolute-URI branch. It writes the secret into proxy access logs on a box the user may not control. Stripping username and password before building the absolute URI should be all it needs.

Medium

3. The tests cover the two halves but not the wiring. resolveProxy and proxyGet are each tested in isolation, and the one fetchPage test asserts blocking rather than routing. Reverting the proxy branch in viaFetch back to the plain fetch() call from main, that is, deleting the feature, leaves all 16 tests passing. The impers branch has no coverage at all, which is why item 1 above slipped through. A test that points http_proxy at a local server and asserts fetchPage actually reaches it would close both.

4. A bare % in a proxy password crashes every fetch. decodeURIComponent throws on http://user:pa%ss@host, and it surfaces as oc: URI malformed with nothing pointing at the env var. Correctly encoded passwords work, so the decode is right, it just needs to tolerate the unencoded case.

Feedback

  • splitHostPort mis-parses unbracketed IPv6 in NO_PROXY: 2606:4700::1 splits into host 2606:4700: and port 1, so the entry matches nothing. Bracketed form works. CIDR entries (10.0.0.0/8) and wildcard subdomains (*.example.com) never match either, and all three fail by proxying rather than bypassing.
  • socks proxies work on a normal install because libcurl accepts them, but hard fail after npm install --omit=optional with a message that reads like oc never supports socks. Worth rejecting in resolveProxy for both transports, or naming the transport in the error.
  • No connect or response timeout on the new transport. It replaces undici's 300s defaults, so a proxy that accepts the connection then goes quiet leaves oc hanging with no output.
  • The exported tlsOpts on proxyGet spreads last, so a caller can pass rejectUnauthorized: false. Nothing in the codebase does, but it is worth narrowing to the options the tests actually need.

Address the critical and high items and I am happy to merge. The medium ones would be good to fold in at the same time since 3 is what would have caught 1. Thanks again for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support HTTP(S) proxy for fetching pages

2 participants