security: harden OAuth discovery and token endpoints

internal/appctx/context.go

@@ -81,10 +81,33 @@ func (a *authAdapter) AccessToken(ctx context.Context) (string, error) {
 	return a.mgr.AccessToken(ctx)
 }
 
+// checkAuthClientRedirect is the CheckRedirect guard for the auth manager's HTTP
+// client (OAuth discovery, token refresh). Refuse to follow redirects for
+// non-idempotent requests: RFC 6749 token endpoints don't legitimately
+// 3xx-redirect POSTs, and because the exchange/refresh requests set GetBody, Go
+// would replay the auth code / refresh_token to the redirect target (only the
+// initial endpoint is origin-validated). Idempotent GET/HEAD requests (e.g.
+// OAuth discovery) carry no credential body, so they may follow redirects
+// normally — blocking those would needlessly fail discovery and force the
+// Launchpad fallback. Still cap the hop count so a looping endpoint fails fast
+// instead of spinning until the client timeout.
+func checkAuthClientRedirect(_ *http.Request, via []*http.Request) error {
+	if len(via) > 0 && via[0].Method != http.MethodGet && via[0].Method != http.MethodHead {
+		return http.ErrUseLastResponse
+	}
+	if len(via) >= 10 {
+		return fmt.Errorf("stopped after 10 redirects")
+	}
+	return nil
+}
+
 // NewApp creates a new App with the given configuration.
 func NewApp(cfg *config.Config) *App {
-	// Create HTTP client for auth manager (OAuth discovery, token refresh)
-	httpClient := &http.Client{Timeout: 30 * time.Second}
+	// Create HTTP client for auth manager (OAuth discovery, token refresh).
+	httpClient := &http.Client{
+		Timeout:       30 * time.Second,
+		CheckRedirect: checkAuthClientRedirect,
+	}
 	authMgr := auth.NewManager(cfg, httpClient)
 
 	// Create observability components

internal/appctx/context_test.go

@@ -7,7 +7,9 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"sync/atomic"
 	"testing"
+	"time"
 
 	"github.com/basecamp/basecamp-sdk/go/pkg/basecamp"
 	"github.com/stretchr/testify/assert"
@@ -48,6 +50,39 @@ func TestNewAppSetsCombinedUserAgent(t *testing.T) {
 	require.NoError(t, err)
 }
 
+// TestCheckAuthClientRedirect_StopsLoop verifies the auth client's redirect
+// guard caps idempotent (GET) follows at Go's default 10-hop limit. A looping
+// endpoint would otherwise spin until the 30s client timeout instead of failing
+// fast, since the guard only blocks non-GET/HEAD redirects.
+func TestCheckAuthClientRedirect_StopsLoop(t *testing.T) {
+	var hops atomic.Int32
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		hops.Add(1)
+		http.Redirect(w, r, "/", http.StatusFound)
+	}))
+	defer srv.Close()
+
+	client := &http.Client{CheckRedirect: checkAuthClientRedirect, Timeout: 5 * time.Second}
+	req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, srv.URL, nil)
+	require.NoError(t, err)
+	resp, err := client.Do(req)
+	if resp != nil {
+		_ = resp.Body.Close()
+	}
+	require.Error(t, err, "redirect loop must fail rather than hang")
+	assert.Contains(t, err.Error(), "stopped after 10 redirects")
+	assert.LessOrEqual(t, hops.Load(), int32(11), "client must give up around the 10-redirect cap")
+}
+
+// TestCheckAuthClientRedirect_BlocksCredentialPOST verifies a non-GET/HEAD
+// initial request never follows a redirect: the guard returns ErrUseLastResponse
+// so a credential-bearing POST body is not replayed to the redirect target.
+func TestCheckAuthClientRedirect_BlocksCredentialPOST(t *testing.T) {
+	post := &http.Request{Method: http.MethodPost}
+	err := checkAuthClientRedirect(nil, []*http.Request{post})
+	assert.ErrorIs(t, err, http.ErrUseLastResponse)
+}
+
 func TestWithAppAndFromContext(t *testing.T) {
 	cfg := &config.Config{}
 	app := NewApp(cfg)

internal/auth/auth.go

@@ -5,6 +5,7 @@ import (
 	"bufio"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"net"
@@ -194,6 +195,19 @@ func (m *Manager) refreshLocked(ctx context.Context, origin string, creds *Crede
 
 	tokenEndpoint := creds.TokenEndpoint
 
+	// The token endpoint here is a persisted (possibly migrated) value from
+	// the credential store and receives the refresh token plus client
+	// credentials. The SDK's RequireSecureEndpoint only checks scheme==https,
+	// so a poisoned store could still carry userinfo (https://user@evil/),
+	// empty-host, or opaque/malformed https forms that it would let through.
+	// Apply the same strict check used for the other OAuth endpoints before
+	// any POST.
+	if u, err := url.Parse(tokenEndpoint); err != nil {
+		return output.ErrAuth(fmt.Sprintf("invalid token endpoint %q: %v", tokenEndpoint, err))
+	} else if !isSecureEndpointURL(u) {
+		return output.ErrAuth(fmt.Sprintf("invalid token endpoint %q: must be an absolute https URL (or http on loopback)", tokenEndpoint))
+	}
+
 	// Resolve client credentials for the refresh request
 	var clientID, clientSecret string
 	switch creds.OAuthType {
@@ -621,7 +635,43 @@ func (m *Manager) loadBC3Client() (*ClientCredentials, error) {
 	return &creds, nil
 }
 
+// isSecureEndpointURL reports whether u uses a scheme safe for OAuth endpoints
+// derived from the server-controlled discovery document: https, or http only on
+// loopback for local development. The URL must also be absolute with a hostname —
+// url.Parse accepts opaque forms like "https:foo" and port-only authorities like
+// "https://:3000/" that carry the right scheme but no hostname, which would
+// otherwise slip through to the transport or browser launcher. URLs carrying
+// userinfo (user:pass@host) are rejected outright: they enable phishing
+// displays in browsers and net/http synthesizes a Basic Authorization header
+// from them. Centralizing the rule keeps the registration, authorization, and
+// redirect-following checks consistent.
+func isSecureEndpointURL(u *url.URL) bool {
+	if u.Hostname() == "" {
+		return false
+	}
+	// Userinfo enables phishing display in browsers ("evil.example@real.host")
+	// and Basic-auth synthesis in net/http requests.
+	if u.User != nil {
+		return false
+	}
+	// IsLocalhost takes the host:port form and strips the port itself.
+	return u.Scheme == "https" || (u.Scheme == "http" && hostutil.IsLocalhost(u.Host))
+}
+
 func (m *Manager) registerBC3Client(ctx context.Context, registrationEndpoint string, opts *LoginOptions) (*ClientCredentials, error) {
+	// The registration endpoint comes from the server-controlled discovery
+	// document. Restrict it to https (or http on loopback for local
+	// development) so a hostile discovery doc can't hand the DCR POST a
+	// file:// (or other) scheme that RequireSecureURL would let through.
+	// Mirrors buildAuthURL's scheme whitelist.
+	u, err := url.Parse(registrationEndpoint)
+	if err != nil {
+		return nil, output.ErrAuth(fmt.Sprintf("invalid registration endpoint %q: %v", registrationEndpoint, err))
+	}
+	if !isSecureEndpointURL(u) {
+		return nil, output.ErrAuth(fmt.Sprintf("invalid registration endpoint %q: must be an absolute https URL (or http on loopback)", registrationEndpoint))
+	}
+
 	customRedirect := opts.RedirectURI != defaultRedirectURI
 	regReq := map[string]any{
 		"client_name":                "basecamp-cli",
@@ -643,8 +693,51 @@ func (m *Manager) registerBC3Client(ctx context.Context, registrationEndpoint st
 	}
 	req.Header.Set("Content-Type", "application/json")
 
-	resp, err := m.httpClient.Do(req)
+	// Use a dedicated client with its own CheckRedirect guard. The DCR POST body
+	// carries only client metadata (no auth code or refresh token), so following a
+	// proxy-canonicalized 3xx redirect is safe — and necessary, since the manager's
+	// guarded client would silently fail first-time login on such redirects. But Go
+	// replays the POST body on 307/308, so re-validate EACH hop's target with the
+	// same scheme rule applied to the registration endpoint; otherwise a hostile
+	// server could 307 the body to a file:// or non-loopback http:// URL, escaping
+	// the whitelist that only covered the original endpoint.
+	checkRedirect := func(req *http.Request, via []*http.Request) error {
+		if !isSecureEndpointURL(req.URL) {
+			return output.ErrAuth(fmt.Sprintf("refusing DCR redirect to %q: must be an absolute https URL (or http on loopback)", req.URL.String()))
+		}
+		// Only 307/308 preserve the POST method and body. On 301/302/303 Go
+		// downgrades the upcoming request (req here) to a body-less GET, so the
+		// registration would silently arrive empty and first-time login would
+		// fail confusingly. Refuse instead of resending as GET.
+		if req.Method != http.MethodPost {
+			return output.ErrAuth(fmt.Sprintf("refusing DCR redirect to %q: redirect downgraded the registration POST to %s, dropping the request body; the endpoint must redirect with 307/308", req.URL.String(), req.Method))
+		}
+		// A redirect loop is a deterministic endpoint misconfiguration, not a
+		// transient network failure: return an auth-class error so the
+		// errors.As unwrap below surfaces it directly instead of masking it
+		// as a retryable output.ErrNetwork.
+		if len(via) >= 10 {
+			return output.ErrAuth(fmt.Sprintf("registration endpoint redirect loop: stopped after 10 redirects at %q", req.URL.String()))
+		}
+		return nil
+	}
+	var dcrClient *http.Client
+	if m.httpClient != nil {
+		c := *m.httpClient // http.Client has no locks; value copy is safe
+		c.CheckRedirect = checkRedirect
+		dcrClient = &c
+	} else {
+		dcrClient = &http.Client{Timeout: 30 * time.Second, CheckRedirect: checkRedirect}
+	}
+	resp, err := dcrClient.Do(req)
 	if err != nil {
+		// A CheckRedirect rejection surfaces as a *url.Error wrapping our
+		// output.ErrAuth. Surface it directly rather than masking the security
+		// failure as a retryable network error.
+		var outErr *output.Error
+		if errors.As(err, &outErr) {
+			return nil, outErr
+		}
 		return nil, output.ErrNetwork(err)
 	}
 	defer resp.Body.Close()
@@ -701,7 +794,15 @@ func (m *Manager) saveBC3Client(creds *ClientCredentials) error {
 func (m *Manager) buildAuthURL(cfg *oauth.Config, oauthType, scope, state, codeChallenge, clientID string, opts *LoginOptions) (string, error) {
 	u, err := url.Parse(cfg.AuthorizationEndpoint)
 	if err != nil {
-		return "", err
+		return "", output.ErrAuth(fmt.Sprintf("invalid authorization endpoint %q: %v", cfg.AuthorizationEndpoint, err))
+	}
+
+	// The authorization endpoint comes from the server-controlled discovery
+	// document and is later dispatched to the OS browser handler (xdg-open /
+	// open). Restrict it to https (or http on loopback for local development)
+	// so a hostile discovery doc can't hand the OS a file:// (or other) URL.
+	if !isSecureEndpointURL(u) {
+		return "", output.ErrAuth(fmt.Sprintf("invalid authorization endpoint %q: must be an absolute https URL (or http on loopback)", cfg.AuthorizationEndpoint))
 	}
 
 	q := u.Query()
@@ -725,6 +826,19 @@ func (m *Manager) buildAuthURL(cfg *oauth.Config, oauthType, scope, state, codeC
 }
 
 func (m *Manager) exchangeCode(ctx context.Context, cfg *oauth.Config, oauthType, code, codeVerifier string, clientCreds *ClientCredentials, opts *LoginOptions) (*Credentials, error) {
+	// The token endpoint comes from the server-controlled discovery document
+	// and receives the authorization code plus client credentials. The SDK's
+	// RequireSecureEndpoint only checks scheme==https, which lets userinfo
+	// (https://legit@evil.com/token) and empty-host forms through. Apply the
+	// same strict check used for the registration and authorization endpoints.
+	u, err := url.Parse(cfg.TokenEndpoint)
+	if err != nil {
+		return nil, output.ErrAuth(fmt.Sprintf("invalid token endpoint %q: %v", cfg.TokenEndpoint, err))
+	}
+	if !isSecureEndpointURL(u) {
+		return nil, output.ErrAuth(fmt.Sprintf("invalid token endpoint %q: must be an absolute https URL (or http on loopback)", cfg.TokenEndpoint))
+	}
+
 	exchanger := oauth.NewExchanger(m.httpClient)
 
 	req := oauth.ExchangeRequest{