Harden OAuth discovery and token endpoints

- Scheme-validate the discovery authorization_endpoint (https, or http on
  loopback) before it's dispatched to the OS browser launcher, so a hostile
  discovery doc can't hand the OS a file:// (or other) URL.
- Enforce RequireSecureURL on the DCR registration_endpoint.
- Set CheckRedirect on the OAuth HTTP client so token exchange/refresh POST
  bodies (auth code, refresh_token) aren't replayed to a redirect target.
  Idempotent GET/HEAD (OAuth discovery) may still follow redirects, so we
  don't break discovery or force an unintended Launchpad fallback.

Author: jeremy

internal/appctx/context.go

@@ -83,8 +83,24 @@ func (a *authAdapter) AccessToken(ctx context.Context) (string, error) {
 
 // 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).
+	// 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.
+	httpClient := &http.Client{
+		Timeout: 30 * time.Second,
+		CheckRedirect: func(_ *http.Request, via []*http.Request) error {
+			if len(via) > 0 && via[0].Method != http.MethodGet && via[0].Method != http.MethodHead {
+				return http.ErrUseLastResponse
+			}
+			return nil
+		},
+	}
 	authMgr := auth.NewManager(cfg, httpClient)
 
 	// Create observability components

internal/auth/auth.go

@@ -621,7 +621,28 @@ 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. Centralizing the rule keeps the registration,
+// authorization, and redirect-following checks consistent.
+func isSecureEndpointURL(u *url.URL) bool {
+	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: scheme must be https (or http on loopback)", registrationEndpoint))
+	}
+
 	customRedirect := opts.RedirectURI != defaultRedirectURI
 	regReq := map[string]any{
 		"client_name":                "basecamp-cli",
@@ -643,7 +664,29 @@ 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, _ []*http.Request) error {
+		if !isSecureEndpointURL(req.URL) {
+			return output.ErrAuth(fmt.Sprintf("refusing DCR redirect to %q: scheme must be https (or http on loopback)", 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 {
 		return nil, output.ErrNetwork(err)
 	}
@@ -704,6 +747,14 @@ func (m *Manager) buildAuthURL(cfg *oauth.Config, oauthType, scope, state, codeC
 		return "", 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: scheme must be https (or http on loopback)", cfg.AuthorizationEndpoint))
+	}
+
 	q := u.Query()
 	q.Set("response_type", "code")
 	q.Set("client_id", clientID)

internal/auth/auth_test.go

@@ -592,6 +592,42 @@ func TestBuildAuthURL_UsesResolvedRedirectURI(t *testing.T) {
 	assert.Contains(t, authURL, "redirect_uri=http%3A%2F%2Flocalhost%3A9999%2Fmy-callback")
 }
 
+// TestBuildAuthURL_RejectsUnsafeScheme guards against a hostile discovery
+// document handing the OS browser launcher a non-https authorization endpoint
+// (e.g. file://). https and http-on-loopback are accepted; everything else
+// must error before reaching OpenBrowser.
+func TestBuildAuthURL_RejectsUnsafeScheme(t *testing.T) {
+	m := &Manager{cfg: config.Default(), httpClient: http.DefaultClient}
+	opts := &LoginOptions{RedirectURI: "http://localhost:9999/callback"}
+
+	accepted := []string{
+		"https://auth.example.com/authorize",
+		"http://localhost:3000/authorize",
+		"http://127.0.0.1:3000/authorize",
+	}
+	for _, endpoint := range accepted {
+		t.Run("accepts "+endpoint, func(t *testing.T) {
+			oauthCfg := &oauth.Config{AuthorizationEndpoint: endpoint}
+			_, err := m.buildAuthURL(oauthCfg, "bc3", "read", "state", "challenge", "cid", opts)
+			require.NoError(t, err)
+		})
+	}
+
+	rejected := []string{
+		"file:///etc/passwd",
+		"http://evil.example.com/authorize",
+		"javascript:alert(1)",
+		"-flag",
+	}
+	for _, endpoint := range rejected {
+		t.Run("rejects "+endpoint, func(t *testing.T) {
+			oauthCfg := &oauth.Config{AuthorizationEndpoint: endpoint}
+			_, err := m.buildAuthURL(oauthCfg, "bc3", "read", "state", "challenge", "cid", opts)
+			require.Error(t, err)
+		})
+	}
+}
+
 func TestExchangeCode_UsesResolvedRedirectURI(t *testing.T) {
 	// Capture the request body sent to the token endpoint
 	var receivedBody string
@@ -697,6 +733,138 @@ func TestRegisterBC3Client_DefaultRedirectPersisted(t *testing.T) {
 	assert.NoError(t, statErr, "client.json should be written for default redirect URI")
 }
 
+// TestRegisterBC3Client_RejectsUnsafeScheme guards against a hostile discovery
+// document handing the DCR POST a non-https registration endpoint (e.g.
+// file://). https and http-on-loopback are accepted; everything else must error
+// before any request is made. Mirrors buildAuthURL's scheme whitelist.
+func TestRegisterBC3Client_RejectsUnsafeScheme(t *testing.T) {
+	m := &Manager{cfg: config.Default(), httpClient: http.DefaultClient}
+	opts := &LoginOptions{RedirectURI: defaultRedirectURI}
+
+	rejected := []string{
+		"file:///etc/passwd",
+		"http://evil.example.com/register",
+		"ftp://evil.example.com/register",
+		"javascript:alert(1)",
+		"data:text/html,foo",
+	}
+	for _, endpoint := range rejected {
+		t.Run("rejects "+endpoint, func(t *testing.T) {
+			_, err := m.registerBC3Client(context.Background(), endpoint, opts)
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), "registration endpoint")
+		})
+	}
+}
+
+// TestRegisterBC3Client_FollowsRedirect verifies the DCR POST follows a
+// proxy-canonicalized 3xx on the registration endpoint (rather than silently
+// failing under the manager's GET-only guard) when the redirect target stays
+// within the secure-endpoint whitelist — here a loopback http:// hop. The DCR
+// body carries only client metadata, so following such a redirect is safe.
+func TestRegisterBC3Client_FollowsRedirect(t *testing.T) {
+	mux := http.NewServeMux()
+	mux.HandleFunc("/register", func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, "/register-canonical", http.StatusTemporaryRedirect)
+	})
+	mux.HandleFunc("/register-canonical", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		fmt.Fprint(w, `{"client_id":"dcr-id","client_secret":"dcr-secret"}`)
+	})
+	srv := httptest.NewServer(mux)
+	defer srv.Close()
+
+	tmpDir := t.TempDir()
+	t.Setenv("XDG_CONFIG_HOME", tmpDir)
+
+	// Manager carries a guarded client (as appctx wires it) to prove the DCR
+	// path uses its own unguarded client rather than m.httpClient.
+	guarded := srv.Client()
+	guarded.CheckRedirect = func(_ *http.Request, via []*http.Request) error {
+		if len(via) > 0 && via[0].Method != http.MethodGet && via[0].Method != http.MethodHead {
+			return http.ErrUseLastResponse
+		}
+		return nil
+	}
+	m := &Manager{
+		cfg:        config.Default(),
+		httpClient: guarded,
+		store:      newTestStore(t, tmpDir),
+	}
+	opts := &LoginOptions{RedirectURI: "http://localhost:7777/cb"}
+
+	creds, err := m.registerBC3Client(context.Background(), srv.URL+"/register", opts)
+	require.NoError(t, err)
+	assert.Equal(t, "dcr-id", creds.ClientID)
+}
+
+// TestRegisterBC3Client_FollowsHTTPSRedirect verifies an https redirect hop is
+// followed: the scheme stays within the secure-endpoint whitelist, so the
+// re-validation in CheckRedirect must not reject it.
+func TestRegisterBC3Client_FollowsHTTPSRedirect(t *testing.T) {
+	mux := http.NewServeMux()
+	mux.HandleFunc("/register", func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, "/register-canonical", http.StatusTemporaryRedirect)
+	})
+	mux.HandleFunc("/register-canonical", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		fmt.Fprint(w, `{"client_id":"dcr-id","client_secret":"dcr-secret"}`)
+	})
+	srv := httptest.NewTLSServer(mux)
+	defer srv.Close()
+
+	tmpDir := t.TempDir()
+	t.Setenv("XDG_CONFIG_HOME", tmpDir)
+
+	m := &Manager{
+		cfg:        config.Default(),
+		httpClient: srv.Client(), // trusts the test server's TLS cert
+		store:      newTestStore(t, tmpDir),
+	}
+	opts := &LoginOptions{RedirectURI: "http://localhost:7777/cb"}
+
+	creds, err := m.registerBC3Client(context.Background(), srv.URL+"/register", opts)
+	require.NoError(t, err)
+	assert.Equal(t, "dcr-id", creds.ClientID)
+}
+
+// TestRegisterBC3Client_RejectsUnsafeRedirect guards against a hostile server
+// 307/308-redirecting the DCR POST (body and all) to a scheme/host outside the
+// secure-endpoint whitelist that was only enforced on the original endpoint.
+// Each redirect hop must be re-validated, so file:// and non-loopback http://
+// targets are rejected before the body is replayed.
+func TestRegisterBC3Client_RejectsUnsafeRedirect(t *testing.T) {
+	targets := map[string]string{
+		"file scheme":       "file:///etc/passwd",
+		"non-loopback http": "http://evil.example.com/register",
+		"other scheme":      "ftp://evil.example.com/register",
+	}
+	for name, target := range targets {
+		t.Run(name, func(t *testing.T) {
+			mux := http.NewServeMux()
+			mux.HandleFunc("/register", func(w http.ResponseWriter, r *http.Request) {
+				http.Redirect(w, r, target, http.StatusTemporaryRedirect)
+			})
+			srv := httptest.NewServer(mux)
+			defer srv.Close()
+
+			tmpDir := t.TempDir()
+			t.Setenv("XDG_CONFIG_HOME", tmpDir)
+
+			m := &Manager{
+				cfg:        config.Default(),
+				httpClient: srv.Client(),
+				store:      newTestStore(t, tmpDir),
+			}
+			opts := &LoginOptions{RedirectURI: "http://localhost:7777/cb"}
+
+			_, err := m.registerBC3Client(context.Background(), srv.URL+"/register", opts)
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), "redirect")
+		})
+	}
+}
+
 func TestLoadClientCredentials_BC3_CustomRedirect_SkipsStoredClient(t *testing.T) {
 	// DCR server
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {