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

bahrmichael · 2026-02-26T07:40:01Z

internal/api/api.go

+func envProxyURL(endpoint string) *url.URL {
+	u, err := url.Parse(endpoint)
+	if err != nil || u.Scheme == "" || u.Host == "" {
+		return nil


What do you think about logging the error here?

Great idea. I re-did the URL parsing for both the endpoint and the proxy - pushing that out to main.go when the program loads, so that users get notified immediately if there are parsing problems for either of them.

bahrmichael · 2026-02-26T07:40:10Z

internal/api/api.go

+	}
+	proxyURL, err := http.ProxyFromEnvironment(&http.Request{URL: u})
+	if err != nil || proxyURL == nil {
+		return nil


What do you think about logging the error here?

Great idea. I re-did the URL parsing for both the endpoint and the proxy - pushing that out to main.go when the program loads, so that users get notified immediately if there are parsing problems for either of them.

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.

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

internal/api/proxy.go

+// not already present (443 for https, 80 for http).
+func proxyDialAddr(proxyURL *url.URL) string {
+	addr := proxyURL.Host
+	if _, _, err := net.SplitHostPort(addr); err != nil {


why are you ignoring the host here and instead assuming addr is just the host? I think this could be a bug right? IE if your proxy url has a port specified won't this break?

Yeah, that's confusing. I wonder if I goofed and deleted a line or something. All the tests were passing; guess I'd better revisit those also! 😊

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

internal/api/proxy.go

+// proxyDialAddr returns proxyURL.Host with a default port appended if one is
+// not already present (443 for https, 80 for http).
+func proxyDialAddr(proxyURL *url.URL) string {
+	addr := proxyURL.Host


I think you should be using the Hostname and Port methods instead of net.SplitHostPort. For example, I think ipv6 addresses would break since the way they are represented in a URL are different to net.SplitHostPort expects.

Hmmmm, the docs for net.SplitHostPort mention IPV6; I'll add some test cases to see if it does work.

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:

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.

…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

…trim a trailing slash

… 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

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?

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.

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?

keegancsmith · 2026-03-02T07:26:21Z

cmd/src/main.go

 	ProxyURL          *url.URL
 	ProxyPath         string
 	ConfigFilePath    string
+	EndpointURL       *url.URL


minor: but these fields should probably all not be exported. Looks like we don't want to marshal them / they are internally read/set.

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.

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

cmd/src/main.go

+
 var cfg *config

 // config represents the config format.


[Re: line +130]

I think you should marshal into a struct with just the json fields here. Then from your readConfig function return something which just contains the fields you want to read. Then you don't have both Endpoint and EndpointURL in this struct which is a bit weird.

See this comment inline on Graphite.

keegancsmith · 2026-03-02T07:33:07Z

internal/api/proxy.go

+		}
+		tlsConn := tls.Client(conn, cfg)
 		if err := tlsConn.HandshakeContext(ctx); err != nil {
 			return nil, err


I think you need to call tlsConn.Close here to cleanup.

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

bahrmichael reviewed Feb 26, 2026

View reviewed changes

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 added 19 commits February 27, 2026 11:39

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

385a259

…ables

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

7651284

…SRC_PROXY if present

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

d2ca262

…quest.Write to ensure correctness

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

d6b88e1

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

d477d71

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

fix some comments

662ff63

whoops, forgot to commit the io import

569629b

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

badd02e

…roxy

add proxy tests

f3e4f82

change name of test to match production method

115b34b

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

af15d4b

go-lint.sh

b83906c

wait for test proxy to startup

f305cb6

go-lint.sh

7977998

remove cleanEndpoint because it just obscures the one thing it does: …

380249f

…trim a trailing slash

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

0deaf78

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

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

ac284e8

…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

1bf5b79

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

43aa0c4

…ead of Endpoint

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

3536377

… that proxies are used

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

keegancsmith added 2 commits March 2, 2026 08:54

go fmt ./...

419f6f7

fix golint issue

76bfc55

keegancsmith requested changes Mar 2, 2026

View reviewed changes

keegancsmith reviewed Mar 2, 2026

View reviewed changes

		// 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!

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!

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!

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!

Choose a reason for hiding this comment

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