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 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.

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

for x in http://localhost:8080 https://localhost:8445
do
HTTPS_PROXY=${x} ./src-cli login --insecure-skip-verify=true
SRC_PROXY=${x} ./src-cli login --insecure-skip-verify=true
done

You should get "Authenticated as [USER] on [ENDPOINT]" messages in your terminal, and in the mitmproxy and squid terminals, you should see activity.

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

Ensure that NO_PROXY is ignored when using SRC_PROXY and honored when using HTTPS_PROXY:

HTTPS_PROXY=http://localhost:8080 NO_PROXY=[HOSTNAME FROM ENDPOINT] ./src-cli login

You should get a successful login, but no entry in the mitmproxy log.

SRC_PROXY=http://localhost:8080 NO_PROXY=[HOSTNAME FROM ENDPOINT] ./src-cli login

You should get a successful login and also see an entry in the mitmproxy log.

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

keegancsmith · 2026-02-26T09:39:14Z

internal/api/proxy_test.go

+		{"https with port", "https://proxy.example.com:8443", "proxy.example.com:8443"},
+		{"https without port", "https://proxy.example.com", "proxy.example.com:443"},
+		{"http with port", "http://proxy.example.com:8080", "proxy.example.com:8080"},
+		{"http without port", "http://proxy.example.com", "proxy.example.com:80"},


get our favourite AI's to generate some example with ipv4, ipv6 and localhost.

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
- 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.

keegancsmith · 2026-03-02T06:58:19Z

cmd/src/main.go

 			}
 			if !isValidUDS {
-				return nil, errors.Newf("invalid proxy socket: %s", path)
+				return nil, errors.Newf("Invalid proxy socket: %s", path)


you changed this is some other places as well. go convention is lowercase on error messages since they get combined. eg My outer error: My inner error doesn't read as nicely as my outer error: my inner error

keegancsmith · 2026-03-02T07:00:05Z

internal/api/api.go

+	// For http:// and socks5:// proxies, the cloned
+	// transport's default Proxy handles them correctly without intervention.


if I set SRC_PROXY to a http:// or socks5:// though, won't the default transport ignore it?

I re-worked this section

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

keegancsmith · 2026-03-02T07:17:16Z

internal/api/api.go

 	}
 	s += fmt.Sprintf("   %s \\\n", shellquote.Join("-d", string(data)))
-	s += fmt.Sprintf("   %s", shellquote.Join(r.client.opts.Endpoint+"/.api/graphql"))
+	s += fmt.Sprintf("   %s", shellquote.Join(r.client.opts.EndpointURL.String()+"/.api/graphql"))


given you are doing TrimRight before you should probably do it here as well.

keegancsmith · 2026-03-02T07:22:29Z

internal/api/proxy.go

+func (c *connWithBufferedReader) Read(p []byte) (int, error) {
+	return c.r.Read(p)
+}


net.Conn documents all methods to be concurrency safe. However, bufio.Reader is not. You need to add in a mutex here.

> Multiple goroutines may invoke methods on a Conn simultaneously.

oh, thanks!

keegancsmith · 2026-03-02T07:23:39Z

internal/api/proxy.go


-				// Read and check the response from the proxy
-				resp, err := http.ReadResponse(bufio.NewReader(conn), nil)
+				br := bufio.NewReader(conn)


It isn't clear to me why you introduced the buffered reader?

Because I didn't want to lose any bytes after the headers that the reader may have buffered, which bytes could be the start of the tunneled connection data. Granted, it's probably not really a problem because the SG endpoint will in nearly every scenario also be TLS, and "TLS server will not speak until spoken to.", so there will not be any bytes beyond the header. But if the SG endpoint is plain http, we could lose bytes if we discard the buffered reader.

cmd/src/main.go

keegancsmith · 2026-03-02T07:27:49Z

cmd/src/main_test.go

+				EndpointURL: &url.URL{
+					Scheme: "https",
+					Host:   "override.com",
+				},


is it worth updating all these test cases for this? I'd consider just not comparing this field in the comparsion in the test.

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

…ables

…SRC_PROXY if present

…quest.Write to ensure correctness

…y edge cases of silently dropped/ignored pieces of the response from the proxy

…e sure it'll try HTTP/2 by setting NextProtos

…roxy

… 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

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

Open

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 from 69221c1 to 791a15c Compare March 10, 2026 02:03

peterguy added 6 commits March 9, 2026 19:46

fix formatting and error messages

3ac2124

add function to read HTTP_PROXY/HTTPS_PROXY/NO_PROXY environment vari…

b2f1e4e

…ables

use new function to read proxy settings from environment, preferring …

a7a2f59

…SRC_PROXY if present

use http.Request instead of manually building the request, and use Re…

e929425

…quest.Write to ensure correctness

commit to a buffered reader for the proxy connection so we avoid nast…

9993da2

…y edge cases of silently dropped/ignored pieces of the response from the proxy

clone the transport TLS config instead of creating a new one, and mak…

4c2de90

…e sure it'll try HTTP/2 by setting NextProtos

peterguy added 26 commits March 9, 2026 19:46

fix some comments

bae69a7

whoops, forgot to commit the io import

fb16c50

dial the proxy, ensuring http/1.1 instead of http/2 for TLS-enabled p…

b3895b3

…roxy

add proxy tests

69763d9

change name of test to match production method

6031e19

add 10ms delay to test proxy server startup to try to fix ubuntu tests

8853c80

go-lint.sh

bd3b210

wait for test proxy to startup

7016f06

go-lint.sh

8a8032a

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

b4724eb

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

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

50ea28b

…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

5dae354

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

651d4e9

…ead of Endpoint

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

b82148f

… that proxies are used

restore unintentional whitespace changes

024fba3

undo changes to search_jobs - do those changes in another PR

e29397b

clarify comments and direct all proxy usage to the custom dialer

8d78585

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

0c25a83

…of net.Conn.

close the connection if the TLS handshake errors

d7bbb21

refactor to use httptest package

210862b

rename functions and address possible leaks

75d0aee

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

f810d2a

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

a13b998

…tlsConn.Close()`) is necessary

fmt.Errorf --> errors.Newf

f6c4e55

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

681f498

…ng in the custom dialer

use JoinPath in batch_remote

dff0820

peterguy force-pushed the peterguy/support-http_proxy branch from 5b46ace to dff0820 Compare March 10, 2026 02:46

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

039c39f

…ention

		// For http:// and socks5:// proxies, the cloned
		// transport's default Proxy handles them correctly without intervention.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bahrmichael commented Mar 2, 2026

Uh oh!

Uh oh!

Uh oh!

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