feature: Add support for HTTP_PROXY, HTTPS_PROXY, and NO_PROXY by peterguy · Pull Request #1260 · sourcegraph/src-cli

peterguy · 2026-02-25T22:22:12Z

Adds support for the standard environment variables HTTP_PROXY, HTTPS_PROXY, and NO_PROXY.

SRC_PROXY still takes precedence, including over NO_PROXY.

Also added fixes for connecting to TLS-enabled proxies, where we want to connect to the SG API using HTTP/2, but many TLS-enabled proxies support only HTTP/1.1, so we need to connect to the proxy via HTTP/1.1, then connect through the proxy to the SG API using HTTP/2.

Moved endpoint and proxy parsing to the config building so that typos and malformed urls are caught early and errors returned immediately.

Test plan

Added CI tests validating proxy behavior.

Manual testing

Install mitmproxy and squid.

Create an SSL cert for squid:

openssl req -x509 -newkey rsa:2048 \
  -keyout squid-key.pem -out squid-cert.pem \
  -days 365 -nodes -subj "/CN=localhost"
cat squid-key.pem squid-cert.pem > squid-proxy.pem

Create a config for squid:

cat << EOF >squid.conf
https_port 8445 cert=./squid-proxy.pem
sslproxy_session_cache_size 0
acl localnet src 127.0.0.1/32
http_access allow localhost
acl SSL_ports port 443
acl Safe_ports port 80
acl Safe_ports port 443
acl Safe_ports port 1025-65535
http_access deny !Safe_ports
http_access deny CONNECT !SSL_ports
http_access deny all
http_port 3128
EOF

Run mitmproxy - mitmproxy -v - in one terminal.

Run squid - squid -f ./squid.conf -N -d 1 - in another terminal.

In a third terminal, in the src-cli repo, build the binary: go build -o ./src-cli ./cmd/src

Ensure you have SRC_ENDPOINT and SRC_ACCESS_TOKEN set in your environment, and unset SRC_PROXY. SRC_ENDPOINT needs to be https://.

Try connecting with each proxy (mitmproxy is 8080, squid is 8445) and with no proxy:

for x in http://localhost:8080 https://localhost:8445
do
HTTPS_PROXY=${x} ./src-cli login --insecure-skip-verify=true
HTTP_PROXY=${x} ./src-cli login --insecure-skip-verify=true
SRC_PROXY=${x} ./src-cli login --insecure-skip-verify=true
HTTPS_PROXY=${x} NO_PROXY=${${SRC_ENDPOINT#*://}%%/*} ./src-cli login --insecure-skip-verify=true
SRC_PROXY=${x} NO_PROXY=${${SRC_ENDPOINT#*://}%%/*} ./src-cli login --insecure-skip-verify=true
doneHTTPS_PROXY="" SRC_PROXY="" ./src-cli login --insecure-skip-verify=true

You should get eleven "Authenticated as [USER] on [ENDPOINT]" messages in your terminal. You should see three connections in each of the mitmproxy and squid terminals. This ensures the program will read all of the proxy environment variables, ignore HTTP_PROXY, will honor NO_PROXY when HTTPS_PROXY is defined, and ignore NO_PROXY when SRC_PROXY is defined.

This tests connecting through a proxy that connects with http (mitmproxy), and one that is TLS-enabled (squid).

Closes https://linear.app/sourcegraph/issue/CPL-236/src-cli-support-standard-http-proxy

internal/api/api.go

keegancsmith

Nice! Inline comments, but otherwise LGTM

keegancsmith · 2026-02-26T09:30:02Z

internal/api/api.go

 	if opts.ProxyURL != nil || opts.ProxyPath != "" {
+		// Explicit SRC_PROXY configuration takes precedence.
 		transport = withProxyTransport(transport, opts.ProxyURL, opts.ProxyPath)
+	} else if proxyURL := envProxyURL(opts.Endpoint); proxyURL != nil && proxyURL.Scheme == "https" {


why does https:// not work without intervention? (as documented below for http and socks5)

Since we're connecting to the SG API using HTTP/2, the standard transport tries to connect to the TLS-enabled proxy using HTTP/2 also, which fails if the proxy doesn't support HTTP/2. I haven't found a proxy for local testing that does support HTTP/2; I suspect that most/all proxies that are TLS-enabled don't support HTTP/2.

internal/api/proxy.go

keegancsmith · 2026-02-26T09:38:07Z

internal/api/proxy_test.go

+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("proxy listen: %v", err)
+	}
+
+	srv := &http.Server{Handler: handler}
+
+	if useTLS {
+		cert := generateTestCert(t, "127.0.0.1")
+		srv.TLSConfig = &tls.Config{Certificates: []tls.Certificate{cert}}
+		go srv.ServeTLS(ln, "", "")
+	} else {
+		go srv.Serve(ln)
+	}
+	t.Cleanup(func() { srv.Close() })
+
+	// Wait for the server to be ready
+	waitForServerReady(t, ln.Addr().String(), useTLS, 5*time.Second)
+
+	scheme := "http"
+	if useTLS {
+		scheme = "https"
+	}
+	pURL, _ := url.Parse(fmt.Sprintf("%s://%s", scheme, ln.Addr().String()))
+	return pURL, ch


I think you can greatly simplify code here (and remove helpers like waitForServerReady and generateTestCert) by using the httptest package. It already provides functions to listen on TLS/etc and does things like wait for the server to be listening.

This advice still stands

I think I finally fixed up these tests?

internal/api/proxy_test.go

keegancsmith · 2026-02-26T09:40:18Z

You might wanna rebase, I think william just did some CI updates which include things like go-lint/etc.

peterguy · 2026-02-26T16:23:43Z

Thanks for all of your commentary; I forgot to open this PR in draft mode! Been trying to address what looks like flakey tests in the Ubuntu CI runner; I'll go through your very helpful comments soon.

peterguy · 2026-02-27T00:48:01Z

This change is part of the following stack:

refactor: validate and parse the endpoint and proxy at program load #1267
- feature: Add support for HTTP_PROXY, HTTPS_PROXY, and NO_PROXY #1260 ◀

_{Change managed by git-spice.}

keegancsmith · 2026-02-27T09:18:35Z

@peterguy alright just re-request review when ready.

keegancsmith

Requesting changes since I just wanna re-review once you have addressed the comments. FYI I pushed some commits to resolve the CI issues.

cmd/src/main.go

internal/api/api.go

keegancsmith · 2026-03-02T07:14:34Z

internal/api/proxy_test.go

+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("proxy listen: %v", err)
+	}
+
+	srv := &http.Server{Handler: handler}
+
+	if useTLS {
+		cert := generateTestCert(t, "127.0.0.1")
+		srv.TLSConfig = &tls.Config{Certificates: []tls.Certificate{cert}}
+		go srv.ServeTLS(ln, "", "")
+	} else {
+		go srv.Serve(ln)
+	}
+	t.Cleanup(func() { srv.Close() })
+
+	// Wait for the server to be ready
+	waitForServerReady(t, ln.Addr().String(), useTLS, 5*time.Second)
+
+	scheme := "http"
+	if useTLS {
+		scheme = "https"
+	}
+	pURL, _ := url.Parse(fmt.Sprintf("%s://%s", scheme, ln.Addr().String()))
+	return pURL, ch


This advice still stands

internal/api/api.go

internal/api/proxy.go

cmd/src/main.go

cmd/src/main_test.go

cmd/src/main.go

internal/api/proxy.go

bahrmichael · 2026-03-02T08:30:09Z

I would wait for an update for Keegan's requested changes before doing my review pass if that's okay.

cmd/src/main.go

… env up front so that typos and malformed urls will surface immediately

…e proxy in the config is now the one gathered from either SRC_PROXY or the standard env variables

…ead of Endpoint

… that proxies are used

…of net.Conn.

…tlsConn.Close()`) is necessary

…ng in the custom dialer

…ention

peterguy · 2026-03-27T00:04:43Z

Alright; I think this is cleaned up and ready for a re-review.

bahrmichael · 2026-03-27T08:18:10Z

Amp found two things, please take them with a grain of salt:

There appear to be two approaches, where one bakes in the resolved url initially, and another (http.ProxyFromEnvironment) checks it on each request(?):

cmd/src/main.go#L327-L332 resolves HTTP_PROXY / HTTPS_PROXY / NO_PROXY once against the initial endpoint and stores the result in cfg.proxyURL, and internal/api/api.go#L111-L112 then bakes that fixed proxy URL into the transport. That diverges from http.ProxyFromEnvironment, which is evaluated per request. Because http.Client follows redirects automatically, a redirect to a different host or scheme will never re-check NO_PROXY and will never switch between HTTP_PROXY and HTTPS_PROXY. A request that starts on http://... and redirects to https://..., or redirects to a host excluded by NO_PROXY, will keep using the wrong proxy decision for the rest of the chain.

Apparently the current tests don't cover the new code paths:

The new TLS-proxy tests don’t actually cover the regression the patch is fixing. internal/api/proxy_test.go#L52-L54 starts the proxy with StartTLS(), but never enables HTTP/2 on that proxy server, while the production change in internal/api/proxy.go#L98-L103 is specifically about preventing ALPN from negotiating h2 to a TLS-enabled proxy. So TestWithProxyTransport_HTTPSProxy proves “HTTPS proxy works,” but it does not reproduce the h2 CONNECT failure mode this code is meant to guard against. I’d want one regression test that advertises h2 on the proxy side and fails unless the client pins that hop to HTTP/1.1.

keegancsmith requested a review from a team February 26, 2026 07:12

bahrmichael reviewed Feb 26, 2026

View reviewed changes

internal/api/api.go Outdated Show resolved Hide resolved

bahrmichael reviewed Feb 26, 2026

View reviewed changes

internal/api/api.go Outdated Show resolved Hide resolved

keegancsmith approved these changes Feb 26, 2026

View reviewed changes

peterguy force-pushed the peterguy/support-http_proxy branch from b1d6176 to 05f0fbb Compare February 27, 2026 00:48

peterguy requested review from bahrmichael and keegancsmith February 27, 2026 19:32

peterguy force-pushed the peterguy/support-http_proxy branch from 622bd60 to 3536377 Compare February 27, 2026 19:40

keegancsmith requested changes Mar 2, 2026

View reviewed changes

keegancsmith reviewed Mar 2, 2026

View reviewed changes

internal/api/proxy.go Show resolved Hide resolved

keegancsmith force-pushed the peterguy/support-http_proxy branch from 6ab67e5 to 78eadd2 Compare March 2, 2026 10:00

bahrmichael reviewed Mar 4, 2026

View reviewed changes

cmd/src/main.go Outdated Show resolved Hide resolved

bahrmichael reviewed Mar 4, 2026

View reviewed changes

cmd/src/main.go Outdated Show resolved Hide resolved

bahrmichael reviewed Mar 4, 2026

View reviewed changes

cmd/src/main.go Outdated Show resolved Hide resolved

peterguy changed the base branch from main to peterguy/clean-up-config March 8, 2026 03:32

peterguy force-pushed the peterguy/support-http_proxy branch from 78eadd2 to 54aada1 Compare March 8, 2026 03:32

peterguy mentioned this pull request Mar 8, 2026

refactor: validate and parse the endpoint and proxy at program load #1267

Merged

peterguy force-pushed the peterguy/clean-up-config branch from cab95e8 to 97c1b0a Compare March 10, 2026 01:53

peterguy force-pushed the peterguy/support-http_proxy branch 4 times, most recently from 448305b to a0d8f91 Compare March 11, 2026 04:40

peterguy force-pushed the peterguy/clean-up-config branch from 5ccd030 to 9ad25e5 Compare March 11, 2026 06:01

peterguy force-pushed the peterguy/support-http_proxy branch from a0d8f91 to ed682d7 Compare March 11, 2026 06:11

peterguy changed the title ~~Add support for HTTP_PROXY, HTTPS_PROXY, and NO_PROXY~~ feature: Add support for HTTP_PROXY, HTTPS_PROXY, and NO_PROXY Mar 12, 2026

peterguy added 19 commits March 16, 2026 14:47

go-lint.sh

7dc0f3c

parse the endpoint into a URL up front, and gather the proxy from the…

0eccb9e

… env up front so that typos and malformed urls will surface immediately

use the parsed endpoint url and consolidate proxy handling because th…

5c25857

…e proxy in the config is now the one gathered from either SRC_PROXY or the standard env variables

fix proxyDialAddr and add more tests for it

87bfdc5

add EndpointURL to the tests and other places that should use it inst…

fc75fbe

…ead of Endpoint

use the client to connect to the API instead of http.DefaultClient so…

14482ff

… that proxies are used

restore unintentional whitespace changes

e93546c

undo changes to search_jobs - do those changes in another PR

c822ace

clarify comments and direct all proxy usage to the custom dialer

6195a34

add a mutex for the buffered reader to match the concurency behavior …

f229e0e

…of net.Conn.

close the connection if the TLS handshake errors

4a0f274

refactor to use httptest package

02e81ce

rename functions and address possible leaks

96c4fec

check I/O errors; use resp.ProtoMajor instead of strings

5092a82

add test to confirm that closing the connection on handshake error (`…

493fe9f

…tlsConn.Close()`) is necessary

fmt.Errorf --> errors.Newf

15ad45c

add test for https connection rejection - ensure correct error handli…

d486976

…ng in the custom dialer

add retry for test that is flaky on CI, probably due to resource cont…

af1263b

…ention

another try at addressing CI test errors

45a8725

peterguy force-pushed the peterguy/support-http_proxy branch from ed682d7 to 45a8725 Compare March 16, 2026 21:47

peterguy added 6 commits March 25, 2026 18:51

clean up proxy and remove buffered reader - don't actually need it

29d9b97

clean up and refactor tests

7495578

refactor buildTransport to match incoming changes

6520ac4

Merge branch 'main' into peterguy/support-http_proxy

8f016c5

fix error capitalization

7119ea9

fix typos

96980e4

peterguy requested review from bahrmichael and keegancsmith March 27, 2026 00:04

Conversation

peterguy commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Manual testing

Uh oh!

Uh oh!

Uh oh!

keegancsmith left a comment

Choose a reason for hiding this comment

Uh oh!

keegancsmith Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

peterguy Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keegancsmith Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

keegancsmith Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

peterguy Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keegancsmith commented Feb 26, 2026

Uh oh!

peterguy commented Feb 26, 2026

Uh oh!

peterguy commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keegancsmith commented Feb 27, 2026

Uh oh!

keegancsmith left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

keegancsmith Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bahrmichael commented Mar 2, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

peterguy commented Mar 27, 2026

Uh oh!

bahrmichael commented Mar 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

peterguy commented Feb 25, 2026 •

edited

Loading

peterguy commented Feb 27, 2026 •

edited

Loading