fix(sonar): never send the API token to a redirect target (#1170)

* fix(sonar): never send the API token to a redirect target

kosli attest sonar authenticates through a transport that attaches the
Sonar API token to every request it sends. That includes the requests Go
issues while following redirects, so the stdlib's own cross-host header
stripping never applied: a Sonar host that 302s to another host handed
the token to that host. Go's default of ten hops made the window wide.

The client now sets a CheckRedirect policy that refuses a redirect to a
different host or from https to http, and stops after five hops. Hosts
are compared lowercased with the scheme's default port removed, so a
reverse proxy emitting an explicit :443 in Location still counts as the
same host. Same-host redirects (a path rewrite, http -> https on the same
host) still work and stay authenticated. Refusing beats following without
the token: every SonarQube endpoint needs it, so an unauthenticated hop
would only fail later with a less useful error. The refusal names the
usual causes (an SSO or proxy login redirect, a non-canonical server URL)
and the setting to change.

The client also gets a 60 second deadline covering the whole chain,
redirects and the Basic retry included. The hop cap bounds how many
redirects are followed, the deadline bounds how long they may take, so a
host that accepts the connection and never answers cannot hold the run.
SonarQube API responses are small JSON documents, so a healthy server is
nowhere near it.

With same-host redirects now a first-class path, the Bearer/Basic probe
no longer treats a 3xx as proof that Bearer worked. It used to cache
Bearer on any non-401, so a Server < 10.0 that redirected /api/ce/task
to /api/ce/task/ got a 401 on the second hop with no Basic fallback. A
redirect is now returned undecided and the final response settles the
scheme.

Fixes kosli-dev/server#6880

* test(sonar): release the parked handler before the test server closes

`t.Cleanup` runs LIFO and `httptest.Server.Close` blocks until every handler
returns, so registering the release before the server meant Close ran
first while /hang was still parked. The test only finished because the
server cancels the request context when the timed-out client drops the
connection, which is the behaviour under test. Registering the release
after the server removes that dependency.

* docs(sonar): say the 3xx guard also returns 300 and 304 undecided

The comment described only the redirects the client follows. The guard
also catches 300 and 304, which are handed straight back to the caller
with the scheme still undecided.

Author: mbevc1

internal/sonar/auth.go

@@ -5,8 +5,10 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"net/url"
 	"strings"
 	"sync"
+	"time"
 )
 
 // authScheme selects how the SonarQube API token is presented to the server.
@@ -109,6 +111,11 @@ func (a *authTransport) RoundTrip(req *http.Request) (*http.Response, error) {
 	if err != nil {
 		return nil, err
 	}
+	// A 3xx says nothing about the scheme: leave it undecided and let the next hop
+	// (or the caller, for 300/304) decide.
+	if resp.StatusCode >= 300 && resp.StatusCode < 400 {
+		return resp, nil
+	}
 	if resp.StatusCode == http.StatusUnauthorized {
 		drainAndClose(resp)
 		resp, err = a.send(req, schemeBasic)
@@ -139,12 +146,58 @@ func drainAndClose(resp *http.Response) {
 	_ = resp.Body.Close()
 }
 
+const (
+	// Go follows ten by default; no real SonarQube deployment needs more than a few.
+	maxSonarRedirects = 5
+
+	// Total deadline per request, redirects and the Basic retry included. Sonar
+	// responses are small JSON documents, so a healthy server is nowhere near it.
+	sonarClientTimeout = 60 * time.Second
+)
+
+// sonarRedirectPolicy refuses redirects that would carry the API token to another
+// host or onto plain http. authTransport re-attaches the token on every hop, so the
+// stdlib's cross-host header stripping never applies; and following without the
+// token would only fail later, since every SonarQube endpoint needs it.
+func sonarRedirectPolicy(req *http.Request, via []*http.Request) error {
+	// via includes the initial request.
+	if len(via) > maxSonarRedirects {
+		return fmt.Errorf("stopped after %d redirects", maxSonarRedirects)
+	}
+	prev := via[len(via)-1].URL
+	if canonicalHost(req.URL) != canonicalHost(prev) {
+		return fmt.Errorf("cross-host redirect from %s to %s refused: the SonarQube API token is only sent to the configured host.\n"+
+			"This usually means SonarQube redirected an unauthenticated request to a login page, or the configured server URL is not the instance's canonical URL. "+
+			"If %s is the SonarQube API, point --sonar-server-url (or the scanner's sonar.host.url, which report-task.txt records) at it directly",
+			prev.Host, req.URL.Host, req.URL.Host)
+	}
+	if prev.Scheme == "https" && req.URL.Scheme != "https" {
+		return fmt.Errorf("redirect from https to http on %s refused: the SonarQube API token would be sent in plain text", prev.Host)
+	}
+	return nil
+}
+
+// canonicalHost lowercases the hostname and drops the scheme's default port, so an
+// explicit :443 in a Location header is still the same host.
+func canonicalHost(u *url.URL) string {
+	host := strings.ToLower(u.Hostname())
+	port := u.Port()
+	if port == "" || (u.Scheme == "https" && port == "443") || (u.Scheme == "http" && port == "80") {
+		return host
+	}
+	return host + ":" + port
+}
+
 // newAuthedClient builds an HTTP client that authenticates SonarQube requests with
 // the given token, presenting it as Bearer (SonarQube Cloud and Server >= 10.0) and
 // falling back to Basic for a self-hosted Server < 10.0. The token is trimmed of
 // surrounding whitespace (e.g. a trailing newline from a secret file).
 func newAuthedClient(token string, mode authScheme) *http.Client {
-	return &http.Client{Transport: &authTransport{token: strings.TrimSpace(token), mode: mode}}
+	return &http.Client{
+		Transport:     &authTransport{token: strings.TrimSpace(token), mode: mode},
+		CheckRedirect: sonarRedirectPolicy,
+		Timeout:       sonarClientTimeout,
+	}
 }
 
 // sonarResponseError turns a SonarQube response that could not be parsed as the

internal/sonar/auth_internal_test.go

@@ -1,6 +1,15 @@
 package sonar
 
-import "testing"
+import (
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"strings"
+	"sync/atomic"
+	"testing"
+	"time"
+)
 
 func TestIsSonarCloudHost(t *testing.T) {
 	cases := []struct {
@@ -24,3 +33,249 @@ func TestIsSonarCloudHost(t *testing.T) {
 		}
 	}
 }
+
+// The client-level tests below run on 127.0.0.1 and differ only by port, so
+// hostname, case and default-port handling are pinned here.
+func TestSonarRedirectPolicy(t *testing.T) {
+	cases := []struct {
+		name    string
+		prev    string
+		next    string
+		wantErr string // "" means the redirect is followed
+	}{
+		{"different hostname", "https://sonar.example.com/x", "https://evil.example.com/x", "cross-host redirect"},
+		{"subdomain of the configured host", "https://sonar.example.com/x", "https://api.sonar.example.com/x", "cross-host redirect"},
+		{"different port", "https://sonar.example.com/x", "https://sonar.example.com:8443/x", "cross-host redirect"},
+		{"https to http downgrade", "https://sonar.example.com/x", "http://sonar.example.com/x", "https to http"},
+		{"hostname differs only by case", "https://sonar.example.com/x", "https://SONAR.Example.com/y", ""},
+		{"explicit default https port added", "https://sonar.example.com/x", "https://sonar.example.com:443/x", ""},
+		{"explicit default http port dropped", "http://sonar.example.com:80/x", "http://sonar.example.com/x", ""},
+		{"http to https upgrade", "http://sonar.example.com/x", "https://sonar.example.com/x", ""},
+		{"path rewrite", "https://sonar.example.com/old", "https://sonar.example.com/new", ""},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			err := sonarRedirectPolicy(mustRequest(t, c.next), []*http.Request{mustRequest(t, c.prev)})
+			switch {
+			case c.wantErr == "" && err != nil:
+				t.Errorf("expected the redirect to be followed, got: %v", err)
+			case c.wantErr != "" && err == nil:
+				t.Errorf("expected the redirect to be refused with %q, got nil", c.wantErr)
+			case c.wantErr != "" && !strings.Contains(err.Error(), c.wantErr):
+				t.Errorf("expected error containing %q, got: %v", c.wantErr, err)
+			}
+		})
+	}
+}
+
+func TestSonarRedirectPolicy_LimitMessageMatchesRedirectsFollowed(t *testing.T) {
+	via := make([]*http.Request, 0, maxSonarRedirects+1)
+	for range maxSonarRedirects + 1 {
+		via = append(via, mustRequest(t, "https://sonar.example.com/loop"))
+	}
+	err := sonarRedirectPolicy(mustRequest(t, "https://sonar.example.com/loop"), via)
+	if err == nil || !strings.Contains(err.Error(), "stopped after 5 redirects") {
+		t.Errorf("expected the limit error to name %d redirects, got: %v", maxSonarRedirects, err)
+	}
+	if err := sonarRedirectPolicy(mustRequest(t, "https://sonar.example.com/loop"), via[:maxSonarRedirects]); err != nil {
+		t.Errorf("expected the %dth redirect to still be followed, got: %v", maxSonarRedirects, err)
+	}
+}
+
+type authRecorder struct {
+	hits atomic.Int32
+	auth atomic.Value
+}
+
+func (r *authRecorder) handler() http.HandlerFunc {
+	return func(w http.ResponseWriter, req *http.Request) {
+		r.auth.Store(req.Header.Get("Authorization"))
+		r.hits.Add(1)
+		w.WriteHeader(http.StatusOK)
+	}
+}
+
+func newTestServer(t *testing.T, h http.Handler) *httptest.Server {
+	t.Helper()
+	srv := httptest.NewServer(h)
+	t.Cleanup(srv.Close)
+	return srv
+}
+
+// server#6880: the transport re-attaches the token on every hop, so a cross-host
+// redirect must not be followed at all.
+func TestAuthedClient_CrossHostRedirect_DoesNotLeakToken(t *testing.T) {
+	target := &authRecorder{}
+	targetSrv := newTestServer(t, target.handler())
+	sonarSrv := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, targetSrv.URL+"/api/ce/task", http.StatusFound)
+	}))
+
+	client := newAuthedClient("SUPER-SECRET-SONAR-TOKEN", schemeBearer)
+	resp, err := client.Get(sonarSrv.URL + "/api/ce/task")
+	if err == nil {
+		drainAndClose(resp)
+	}
+	if n := target.hits.Load(); n != 0 {
+		t.Fatalf("redirect target must never be contacted, got %d request(s) with Authorization %q", n, target.auth.Load())
+	}
+	if err == nil {
+		t.Fatal("expected the cross-host redirect to be refused, got a response")
+	}
+	if !strings.Contains(err.Error(), "cross-host redirect") {
+		t.Errorf("expected a cross-host redirect error, got: %v", err)
+	}
+}
+
+func TestAuthedClient_SameHostRedirect_IsFollowedWithToken(t *testing.T) {
+	target := &authRecorder{}
+	mux := http.NewServeMux()
+	mux.Handle("/new", target.handler())
+	mux.HandleFunc("/old", func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, "/new", http.StatusFound)
+	})
+	srv := newTestServer(t, mux)
+
+	client := newAuthedClient("tok", schemeBearer)
+	resp, err := client.Get(srv.URL + "/old")
+	if err != nil {
+		t.Fatalf("expected the same-host redirect to be followed, got: %v", err)
+	}
+	drainAndClose(resp)
+	if resp.StatusCode != http.StatusOK {
+		t.Fatalf("expected 200 from the redirect target, got %d", resp.StatusCode)
+	}
+	if got := target.auth.Load(); got != bearerHeaderValue("tok") {
+		t.Errorf("expected the redirected request to carry the Bearer token, got %q", got)
+	}
+}
+
+func TestAuthedClient_RedirectThenUnauthorized_StillFallsBackToBasic(t *testing.T) {
+	target := &authRecorder{}
+	mux := http.NewServeMux()
+	mux.HandleFunc("/api/ce/task", func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, "/api/ce/task/", http.StatusFound)
+	})
+	mux.HandleFunc("/api/ce/task/", func(w http.ResponseWriter, r *http.Request) {
+		// Server < 10.0 accepts only Basic.
+		if !strings.HasPrefix(r.Header.Get("Authorization"), "Basic ") {
+			w.WriteHeader(http.StatusUnauthorized)
+			return
+		}
+		target.handler()(w, r)
+	})
+	srv := newTestServer(t, mux)
+
+	client := newAuthedClient("tok", schemeAuto)
+	resp, err := client.Get(srv.URL + "/api/ce/task")
+	if err != nil {
+		t.Fatalf("expected the redirected request to succeed via Basic, got: %v", err)
+	}
+	drainAndClose(resp)
+	if resp.StatusCode != http.StatusOK {
+		t.Fatalf("expected 200 after the Basic fallback on the redirected hop, got %d", resp.StatusCode)
+	}
+	if got := target.auth.Load(); got != basicHeaderValue("tok") {
+		t.Errorf("expected the redirected request to be retried with Basic, got %q", got)
+	}
+}
+
+func TestAuthedClient_RedirectLoop_StopsAtLimit(t *testing.T) {
+	var hits atomic.Int32
+	srv := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		hits.Add(1)
+		http.Redirect(w, r, "/loop", http.StatusFound)
+	}))
+
+	client := newAuthedClient("tok", schemeBearer)
+	resp, err := client.Get(srv.URL + "/loop")
+	if err == nil {
+		drainAndClose(resp)
+		t.Fatal("expected the redirect loop to be refused, got a response")
+	}
+	if !strings.Contains(err.Error(), "redirects") {
+		t.Errorf("expected a redirect-limit error, got: %v", err)
+	}
+	if n := hits.Load(); n != maxSonarRedirects+1 {
+		t.Errorf("expected the initial request plus %d followed redirects, got %d requests", maxSonarRedirects, n)
+	}
+}
+
+func TestAuthedClient_HangingRedirectTarget_TimesOut(t *testing.T) {
+	release := make(chan struct{})
+	mux := http.NewServeMux()
+	mux.HandleFunc("/old", func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, "/hang", http.StatusFound)
+	})
+	mux.HandleFunc("/hang", func(w http.ResponseWriter, r *http.Request) {
+		select {
+		case <-release:
+		case <-r.Context().Done():
+		}
+	})
+	srv := newTestServer(t, mux)
+	// Cleanups run LIFO: release the parked handler before srv.Close, which waits for it.
+	t.Cleanup(func() { close(release) })
+
+	client := newAuthedClient("tok", schemeBearer)
+	if client.Timeout != sonarClientTimeout {
+		t.Fatalf("expected the client deadline to be %v, got %v", sonarClientTimeout, client.Timeout)
+	}
+	client.Timeout = 200 * time.Millisecond // keep the test fast; the wiring is asserted above
+
+	start := time.Now()
+	resp, err := client.Get(srv.URL + "/old")
+	if err == nil {
+		drainAndClose(resp)
+		t.Fatal("expected the hanging redirect target to time out, got a response")
+	}
+	var urlErr *url.Error
+	if !errors.As(err, &urlErr) || !urlErr.Timeout() {
+		t.Errorf("expected a timeout error, got: %v", err)
+	}
+	if elapsed := time.Since(start); elapsed > 5*time.Second {
+		t.Errorf("expected the deadline to cut the request short, waited %v", elapsed)
+	}
+}
+
+// GetCETaskData reuses one *http.Request across polls. http.Client forks the request
+// before attaching its deadline, so the caller's request stays usable; pin that for
+// authTransport, which is not a *http.Transport.
+func TestAuthedClient_ReusedRequestAcrossPolls_IsNotCancelledByTimeout(t *testing.T) {
+	var polls atomic.Int32
+	srv := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		polls.Add(1)
+		_, _ = w.Write([]byte(`{"task":{"status":"PENDING"}}`))
+	}))
+
+	client := newAuthedClient("tok", schemeAuto)
+	if client.Timeout == 0 {
+		t.Fatal("this test only means something with a client deadline set")
+	}
+	req := mustRequest(t, srv.URL+"/api/ce/task?id=AYx")
+	for i := 1; i <= 3; i++ {
+		resp, err := client.Do(req)
+		if err != nil {
+			t.Fatalf("poll %d on the reused request failed: %v", i, err)
+		}
+		drainAndClose(resp)
+		if resp.StatusCode != http.StatusOK {
+			t.Fatalf("poll %d: expected 200, got %d", i, resp.StatusCode)
+		}
+	}
+	if n := polls.Load(); n != 3 {
+		t.Errorf("expected 3 polls to reach the server, got %d", n)
+	}
+	if err := req.Context().Err(); err != nil {
+		t.Errorf("the caller's request context must stay live across polls, got: %v", err)
+	}
+}
+
+func mustRequest(t *testing.T, rawURL string) *http.Request {
+	t.Helper()
+	req, err := http.NewRequest(http.MethodGet, rawURL, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	return req
+}

internal/sonar/sonar_auth_test.go

@@ -325,3 +325,27 @@ func TestGetSonarResults_Forbidden_NonJSON_RendersActualStatus(t *testing.T) {
 		}
 	}
 }
+
+// End-to-end check for server#6880 through GetSonarResults.
+func TestGetSonarResults_CrossHostRedirect_TokenStaysOnConfiguredHost(t *testing.T) {
+	target := &fakeSonar{acceptsBearer: true, acceptsBasic: true}
+	targetSrv := httptest.NewServer(target.handler())
+	defer targetSrv.Close()
+
+	redirector := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, targetSrv.URL+r.URL.RequestURI(), http.StatusFound)
+	}))
+	defer redirector.Close()
+
+	sc := sonar.NewSonarConfig("SUPER-SECRET-SONAR-TOKEN", t.TempDir(), redirector.URL+"/api/ce/task?id=AYx", "", "", "", "", "", 5)
+	_, err := sc.GetSonarResults(discardLogger())
+	if got := target.authHeaders(); len(got) != 0 {
+		t.Fatalf("the redirect target must never receive a request, got Authorization headers %v", got)
+	}
+	if err == nil {
+		t.Fatal("expected an error when the SonarQube host redirects to another host")
+	}
+	if !strings.Contains(err.Error(), "cross-host redirect") {
+		t.Errorf("expected a cross-host redirect error, got: %v", err)
+	}
+}