fix(oauth): refuse redirects on credential POSTs (#729)

Token exchange/refresh and the device authorization/poll POSTs validated only
the initial endpoint, then let the http.Client follow a 307/308 that replays the
form body (code, PKCE verifier, refresh_token, client_secret) to an unvalidated
origin.

Add withoutRedirects(): a shallow client copy whose CheckRedirect refuses every
redirect (ErrUnsafeRedirect). Applied at PostToken (covers ExchangeCode, Refresh,
and MCP which delegate to it), RequestDeviceCode, and pollDeviceOnce. The caller's
client is never mutated; legitimate token endpoints that return 200 are unaffected.

internal/oauth/device.go

@@ -49,9 +49,9 @@ func RequestDeviceCode(ctx context.Context, client *http.Client, cfg Config, now
 	if err := validateTokenEndpoint(endpoint); err != nil {
 		return DeviceAuth{}, err
 	}
-	if client == nil {
-		client = http.DefaultClient
-	}
+	// Refuse redirects so a device-authorization redirect can't replay the
+	// client_id/client_secret POST body to an unvalidated origin.
+	client = withoutRedirects(client)
 	if now == nil {
 		now = time.Now
 	}
@@ -157,9 +157,9 @@ func pollDeviceOnce(ctx context.Context, client *http.Client, cfg Config, device
 	if err := validateTokenEndpoint(cfg.TokenEndpoint); err != nil {
 		return Token{}, err
 	}
-	if client == nil {
-		client = http.DefaultClient
-	}
+	// Refuse redirects so a poll redirect can't replay the device_code/secret
+	// POST body to an unvalidated origin.
+	client = withoutRedirects(client)
 	form := url.Values{}
 	form.Set("grant_type", deviceGrantType)
 	form.Set("device_code", deviceCode)

internal/oauth/device_test.go

@@ -2,6 +2,7 @@ package oauth
 
 import (
 	"context"
+	"errors"
 	"net/http"
 	"net/http/httptest"
 	"strings"
@@ -28,6 +29,30 @@ func TestRequestDeviceCodeSendsClientSecret(t *testing.T) {
 	}
 }
 
+func TestRequestDeviceCodeRefusesRedirect(t *testing.T) {
+	endpoint, hits := redirectTrap(t, http.StatusTemporaryRedirect) // 307
+	_, err := RequestDeviceCode(context.Background(), http.DefaultClient,
+		Config{ClientID: "c", ClientSecret: "shh", DeviceAuthorizationEndpoint: endpoint}, nil)
+	if !errors.Is(err, ErrUnsafeRedirect) {
+		t.Fatalf("RequestDeviceCode err = %v, want ErrUnsafeRedirect", err)
+	}
+	if n := hits(); n != 0 {
+		t.Fatalf("attacker received %d request(s) — client_secret replayed", n)
+	}
+}
+
+func TestPollDeviceOnceRefusesRedirect(t *testing.T) {
+	endpoint, hits := redirectTrap(t, http.StatusPermanentRedirect) // 308
+	_, err := pollDeviceOnce(context.Background(), http.DefaultClient,
+		Config{ClientID: "c", ClientSecret: "shh", TokenEndpoint: endpoint}, "device-code", nil)
+	if !errors.Is(err, ErrUnsafeRedirect) {
+		t.Fatalf("pollDeviceOnce err = %v, want ErrUnsafeRedirect", err)
+	}
+	if n := hits(); n != 0 {
+		t.Fatalf("attacker received %d request(s) — device_code/secret replayed", n)
+	}
+}
+
 func TestRequestDeviceCode(t *testing.T) {
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
 		_, _ = w.Write([]byte(`{"device_code":"dc","user_code":"WXYZ-1234","verification_uri":"https://x/dev","verification_uri_complete":"https://x/dev?c=WXYZ","expires_in":600,"interval":3}`))

internal/oauth/flow.go

@@ -116,6 +116,23 @@ func validateTokenEndpoint(endpoint string) error {
 	return ValidateEndpointURL(endpoint)
 }
 
+// withoutRedirects returns a shallow copy of client whose redirect policy
+// refuses to follow ANY redirect. A credential-bearing OAuth POST (token
+// exchange/refresh, device authorization, device-token poll) must never let a
+// 307/308 replay its form body — codes, PKCE verifiers, refresh tokens, client
+// secrets — to a target that was never endpoint-validated. The caller's client
+// is left unmodified.
+func withoutRedirects(client *http.Client) *http.Client {
+	if client == nil {
+		client = http.DefaultClient
+	}
+	copied := *client
+	copied.CheckRedirect = func(*http.Request, []*http.Request) error {
+		return ErrUnsafeRedirect
+	}
+	return &copied
+}
+
 func isLoopbackHost(host string) bool {
 	if host == "localhost" {
 		return true
@@ -176,9 +193,9 @@ func PostToken(ctx context.Context, client *http.Client, tokenEndpoint string, f
 	if err := validateTokenEndpoint(tokenEndpoint); err != nil {
 		return Token{}, err
 	}
-	if client == nil {
-		client = http.DefaultClient
-	}
+	// Refuse redirects so a 307/308 from the token endpoint can't replay this
+	// credential-bearing POST body to another (unvalidated) origin.
+	client = withoutRedirects(client)
 	if now == nil {
 		now = time.Now
 	}

internal/oauth/flow_test.go

@@ -5,10 +5,12 @@ import (
 	"crypto/sha256"
 	"encoding/base64"
 	"errors"
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"strings"
+	"sync/atomic"
 	"testing"
 	"time"
 )
@@ -126,6 +128,64 @@ func TestBuildAuthorizationURLRejectsInsecureEndpoint(t *testing.T) {
 	}
 }
 
+// redirectTrap starts an "attacker" server that counts any hit, and a credential
+// endpoint that redirects to it with the given status. It returns the endpoint
+// URL and a func reporting how many times the attacker was contacted — a
+// non-zero count means the credential-bearing POST body was replayed (#729).
+func redirectTrap(t *testing.T, status int) (endpoint string, attackerHits func() int32) {
+	t.Helper()
+	var hits int32
+	attacker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		atomic.AddInt32(&hits, 1)
+		_, _ = io.WriteString(w, `{"access_token":"leaked","refresh_token":"leaked"}`)
+	}))
+	t.Cleanup(attacker.Close)
+	endp := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, attacker.URL, status)
+	}))
+	t.Cleanup(endp.Close)
+	return endp.URL, func() int32 { return atomic.LoadInt32(&hits) }
+}
+
+func TestPostTokenRefusesRedirect(t *testing.T) {
+	endpoint, hits := redirectTrap(t, http.StatusTemporaryRedirect) // 307
+	form := url.Values{}
+	form.Set("grant_type", "authorization_code")
+	form.Set("code", "the-code")
+	form.Set("client_secret", "shh")
+	_, err := PostToken(context.Background(), http.DefaultClient, endpoint, form, Token{}, nil)
+	if !errors.Is(err, ErrUnsafeRedirect) {
+		t.Fatalf("PostToken err = %v, want ErrUnsafeRedirect", err)
+	}
+	if n := hits(); n != 0 {
+		t.Fatalf("attacker received %d request(s) — credential body was replayed", n)
+	}
+}
+
+func TestExchangeCodeRefusesRedirect(t *testing.T) {
+	endpoint, hits := redirectTrap(t, http.StatusPermanentRedirect) // 308
+	_, err := ExchangeCode(context.Background(), http.DefaultClient,
+		Config{ClientID: "c", TokenEndpoint: endpoint}, "the-code", "verifier", "http://127.0.0.1/cb", nil)
+	if !errors.Is(err, ErrUnsafeRedirect) {
+		t.Fatalf("ExchangeCode err = %v, want ErrUnsafeRedirect", err)
+	}
+	if n := hits(); n != 0 {
+		t.Fatalf("attacker received %d request(s) — code/verifier replayed", n)
+	}
+}
+
+func TestRefreshRefusesRedirect(t *testing.T) {
+	endpoint, hits := redirectTrap(t, http.StatusTemporaryRedirect) // 307
+	_, err := Refresh(context.Background(), http.DefaultClient,
+		Config{ClientID: "c", TokenEndpoint: endpoint}, Token{RefreshToken: "rt"}, nil)
+	if !errors.Is(err, ErrUnsafeRedirect) {
+		t.Fatalf("Refresh err = %v, want ErrUnsafeRedirect", err)
+	}
+	if n := hits(); n != 0 {
+		t.Fatalf("attacker received %d request(s) — refresh_token replayed", n)
+	}
+}
+
 func TestValidateTokenEndpoint(t *testing.T) {
 	cases := map[string]bool{
 		"https://auth.example.com/token": true,

internal/oauth/oauth.go

@@ -90,6 +90,11 @@ var (
 	// ErrInsecureTokenEndpoint is returned when a credential would be sent over a
 	// non-https, non-loopback endpoint.
 	ErrInsecureTokenEndpoint = errors.New("oauth: refusing to send credential to a non-https token endpoint")
+	// ErrUnsafeRedirect is returned when a credential-bearing OAuth POST (token
+	// exchange/refresh, device authorization, or device-token poll) is redirected.
+	// Following it would replay the form body — codes, PKCE verifiers, refresh
+	// tokens, client secrets — to a target that never passed endpoint validation.
+	ErrUnsafeRedirect = errors.New("oauth: refusing to follow a redirect from a credential endpoint")
 	// ErrNoRefreshToken is returned when a refresh is attempted without one.
 	ErrNoRefreshToken = errors.New("oauth: no refresh token available")
 	// ErrAuthorizationPending is the RFC 8628 "keep polling" signal.

internal/sandbox/analyzer.go

@@ -65,76 +65,6 @@ func AnalyzeCommand(script string) AnalysisResult {
 	return result
 }
 
-// astCommandFields parses command with the shell parser and returns each simple
-// command as its literal field slice (program + args as text), resolving the
-// real command positions across quoting, command substitution, subshells, and
-// newline separators — the constructs the hand-written splitter in
-// safe_command.go mis-handles (issue #473). It returns nil when the command
-// cannot be parsed (e.g. a Windows cmd.exe string), so callers fall through to
-// the regex path rather than hard-blocking.
-func astCommandFields(command string) [][]string {
-	file, err := syntax.NewParser().Parse(strings.NewReader(command), "")
-	if err != nil {
-		return nil
-	}
-	var commands [][]string
-	syntax.Walk(file, func(node syntax.Node) bool {
-		call, ok := node.(*syntax.CallExpr)
-		if !ok || len(call.Args) == 0 {
-			return true
-		}
-		// EVERY word must be a static literal, not just the program name.
-		// wordText keeps only the literal/quoted parts of a word and silently
-		// drops expansions, so a dynamic word anywhere in the call reconstructs
-		// to something the shell will never run: `$(printf foo)vim` (runs as
-		// `foovim`) collapses to "vim", and `git $(printf foo)rebase -i` (runs
-		// as `git foorebase -i`, non-interactive) collapses to `git rebase -i`
-		// and fabricates an interactive match. Since the runtime value of an
-		// expansion is unknowable here, skip the whole call rather than classify
-		// a lossy reconstruction. Skipping is the safe direction: the
-		// hand-written passes above already ran, and a missed detection falls
-		// through to the normal permission prompt instead of hard-blocking a
-		// command the user never wrote.
-		for _, word := range call.Args {
-			if !isLiteralWord(word) {
-				return true
-			}
-		}
-		fields := make([]string, 0, len(call.Args))
-		for _, word := range call.Args {
-			fields = append(fields, wordText(word))
-		}
-		commands = append(commands, fields)
-		return true
-	})
-	return commands
-}
-
-// isLiteralWord reports whether every part of word is a static literal (bare or
-// quoted). A word containing a command substitution, parameter/arithmetic
-// expansion, process substitution, etc. is dynamic — its runtime value is
-// unknown, so its wordText (a partial literal) must not be trusted as a program
-// name.
-func isLiteralWord(word *syntax.Word) bool {
-	if word == nil {
-		return false
-	}
-	for _, part := range word.Parts {
-		switch typed := part.(type) {
-		case *syntax.Lit, *syntax.SglQuoted:
-		case *syntax.DblQuoted:
-			for _, inner := range typed.Parts {
-				if _, ok := inner.(*syntax.Lit); !ok {
-					return false
-				}
-			}
-		default:
-			return false
-		}
-	}
-	return true
-}
-
 // analyzeInto parses script and folds its interactive/destructive/network usage
 // into result, sharing seen so program names are de-duplicated across recursion.
 func analyzeInto(script string, result *AnalysisResult, seen map[string]bool, depth int) {

internal/sandbox/safe_command.go

@@ -219,65 +219,9 @@ func DetectInteractiveCommand(command string, goos string) InteractiveCommandRes
 		}
 	}
 
-	// AST second opinion (issue #473): the hand-written segment split above
-	// mis-handles interactive programs hidden by unusual quoting, command
-	// substitution, subshells, or newline separators. Re-extract the real simple
-	// commands from the parsed shell tree and apply the SAME full pipeline
-	// (interactive segments, sh -c payload recursion, per-program lookup), so a
-	// bypass is caught while every program stays classified exactly as before
-	// (ssh with a trailing command, python -c, etc. remain allowed). A command
-	// the parser cannot handle (Windows cmd.exe, obfuscation) yields no commands
-	// and falls through unchanged — the guard never hard-blocks on a parse error.
-	for _, fields := range astCommandFields(command) {
-		if result, ok := inspectCommandFields(fields, goos); ok {
-			return result
-		}
-	}
-
 	return InteractiveCommandResult{}
 }
 
-// inspectCommandFields applies the full interactive-detection pipeline to one
-// already-split command's fields: the multi-word interactive segments, then the
-// `sh -c <payload>` recursion, then the per-program lookup with its
-// non-interactive suppressions (hasNonInteractiveFlag covers REPL flags and
-// trailing-command clients like ssh). It mirrors the hand-written passes above
-// so an AST-extracted command is classified identically to a plainly-split one.
-func inspectCommandFields(fields []string, goos string) (InteractiveCommandResult, bool) {
-	body := strings.ToLower(commandBody(fields))
-	for _, seg := range interactiveSegments {
-		if body == seg.match || strings.HasPrefix(body, seg.match+" ") {
-			suggestion := seg.suggestion
-			if goos == "windows" && seg.windowsSuggestion != "" {
-				suggestion = seg.windowsSuggestion
-			}
-			return InteractiveCommandResult{Interactive: true, Command: seg.command, Reason: seg.reason, Suggestion: suggestion}, true
-		}
-	}
-	first := firstProgram(fields)
-	if first == "" {
-		return InteractiveCommandResult{}, false
-	}
-	if payload := shellDashCPayload(first, fields); payload != "" {
-		if inner := DetectInteractiveCommand(payload, goos); inner.Interactive {
-			return inner, true
-		}
-		return InteractiveCommandResult{}, false
-	}
-	program, ok := interactivePrograms[first]
-	if !ok || (program.windowsOnly && goos != "windows") {
-		return InteractiveCommandResult{}, false
-	}
-	if hasNonInteractiveFlag(first, fields) {
-		return InteractiveCommandResult{}, false
-	}
-	suggestion := program.suggestion
-	if goos == "windows" && program.windowsSuggestion != "" {
-		suggestion = program.windowsSuggestion
-	}
-	return InteractiveCommandResult{Interactive: true, Command: first, Reason: program.reason, Suggestion: suggestion}, true
-}
-
 // wrapperPrograms are launcher prefixes that precede the real program. After
 // one of these we keep scanning for the actual executable.
 var wrapperPrograms = map[string]bool{

internal/sandbox/safe_command_test.go

@@ -406,87 +406,3 @@ func TestDetectInteractiveMongoEvalAndFullPaths(t *testing.T) {
 		}
 	}
 }
-
-// The hand-written segment splitter misses interactive programs hidden by
-// constructs only a real shell parser resolves (a newline separator collapsed
-// to a space; a brace group that shifts the real command position). The AST
-// second opinion must catch them (issue #473).
-func TestDetectInteractiveCommandCatchesParserBypasses(t *testing.T) {
-	for _, cmd := range []string{
-		"echo hi\nvim file.txt", // newline separator collapsed to a space by the regex path
-		"{ vim file.txt; }",     // brace group hides the real program position
-	} {
-		got := DetectInteractiveCommand(cmd, "linux")
-		if !got.Interactive {
-			t.Errorf("DetectInteractiveCommand(%q) = not interactive, want caught (parser bypass)", cmd)
-			continue
-		}
-		if got.Command != "vim" {
-			t.Errorf("DetectInteractiveCommand(%q).Command = %q, want vim", cmd, got.Command)
-		}
-	}
-}
-
-// The AST second opinion must not flag an interactive program NAME that appears
-// only inside a quoted argument (not a real command position) — the parser
-// distinguishes program from argument, so this must stay non-interactive.
-func TestDetectInteractiveCommandNoFalsePositiveOnQuotedArgument(t *testing.T) {
-	got := DetectInteractiveCommand(`echo "please run vim later"`, "linux")
-	if got.Interactive {
-		t.Fatalf("interactive name inside a quoted argument must not be flagged, got %#v", got)
-	}
-}
-
-// The AST second opinion must run the FULL pipeline — multi-word interactive
-// segments and sh -c payload recursion, not just the per-program lookup — so a
-// grouped/nested bypass is caught; and it must not fabricate a program name from
-// a command substitution concatenated with a literal (issue #473, review).
-func TestDetectInteractiveCommandASTPipelineBoundaries(t *testing.T) {
-	interactive := []struct{ cmd, wantCmd string }{
-		{"{ git rebase -i HEAD~1; }", "git rebase -i"}, // multi-word segment in a brace group
-		{"{ sh -c 'vim file.txt'; }", "vim"},           // sh -c payload in a brace group
-	}
-	for _, tc := range interactive {
-		got := DetectInteractiveCommand(tc.cmd, "linux")
-		if !got.Interactive || got.Command != tc.wantCmd {
-			t.Errorf("DetectInteractiveCommand(%q) = %+v, want interactive Command=%q", tc.cmd, got, tc.wantCmd)
-		}
-	}
-}
-
-// The AST pass must not fabricate a program name from a dynamic (non-literal)
-// program word: `$(printf '%s' foo)vim` runs as `foovim`, not vim. Tested at the
-// extractor so the hand-written pass (which pre-empts DetectInteractiveCommand
-// with its own, separate handling of that construct) does not mask the guard.
-func TestAstCommandFieldsSkipsDynamicProgram(t *testing.T) {
-	for _, fields := range astCommandFields("$(printf '%s' foo)vim file.txt") {
-		if firstProgram(fields) == "vim" {
-			t.Fatalf("astCommandFields fabricated a vim command from a substitution: %v", fields)
-		}
-	}
-}
-
-// The literalness guard covers every word, not just the program name. A dynamic
-// ARGUMENT is dropped by wordText the same way a dynamic program word is, so
-// `git $(printf foo)rebase -i HEAD~1` (which runs as `git foorebase -i`, a
-// non-existent and non-interactive subcommand) would otherwise be reconstructed
-// as `git rebase -i` and blocked — a false positive on a command the user never
-// wrote. Asserted through DetectInteractiveCommand as well as the extractor,
-// because the whole point is what the caller ends up blocking.
-func TestAstCommandFieldsSkipsDynamicArgument(t *testing.T) {
-	const dynamic = "{ git $(printf foo)rebase -i HEAD~1 ; }"
-	for _, fields := range astCommandFields(dynamic) {
-		if firstProgram(fields) == "git" {
-			t.Fatalf("astCommandFields reconstructed a git command from a dynamic argument: %v", fields)
-		}
-	}
-	if got := DetectInteractiveCommand(dynamic, "linux"); got.Interactive {
-		t.Errorf("DetectInteractiveCommand(%q) = %+v, want not interactive", dynamic, got)
-	}
-	// The genuine form must still be detected — the guard skips lossy
-	// reconstructions, it does not weaken real detection.
-	const genuine = "{ git rebase -i HEAD~1 ; }"
-	if got := DetectInteractiveCommand(genuine, "linux"); !got.Interactive || got.Command != "git rebase -i" {
-		t.Errorf("DetectInteractiveCommand(%q) = %+v, want interactive Command=%q", genuine, got, "git rebase -i")
-	}
-}