fix(sandbox): require literal words for the whole AST call (#473 review)

astCommandFields guarded only the program word, then ran every argument
through wordText, which keeps literal/quoted parts and silently drops
CmdSubst/ParamExp. A dynamic ARGUMENT was therefore reconstructed lossily:
`git $(printf foo)rebase -i HEAD~1` runs as `git foorebase -i` (a
non-existent, non-interactive subcommand) but collapsed to `git rebase -i`
and was blocked -- a false positive on a command the user never wrote, and
new on this branch (the base correctly reports not-interactive).

Extend the literalness guard to every word in the call: if any word carries
an expansion, skip that AST command rather than classify a reconstruction
whose runtime value is unknowable. Skipping is the safe direction -- the
hand-written passes still run first, and a miss falls through to the normal
permission prompt instead of hard-blocking.

Genuine detection is unchanged: `git rebase -i HEAD~1` and the #473 bypass
cases still classify exactly as before.

internal/mcp/oauth.go

@@ -133,6 +133,26 @@ func discoverAuthorizationServer(ctx context.Context, client *http.Client, baseU
 	}, nil
 }
 
+// validateResolvedEndpoints applies the shared https/loopback endpoint rule to
+// every non-empty endpoint in the resolved metadata — configured or discovered
+// alike — so discovery metadata can never downgrade an MCP login to an insecure
+// or attacker-controlled authorization, token, or registration endpoint.
+func validateResolvedEndpoints(metadata authServerMetadata) error {
+	for _, ep := range []struct{ kind, url string }{
+		{"authorization", metadata.AuthorizationEndpoint},
+		{"token", metadata.TokenEndpoint},
+		{"registration", metadata.RegistrationEndpoint},
+	} {
+		if strings.TrimSpace(ep.url) == "" {
+			continue
+		}
+		if err := oauth.ValidateEndpointURL(ep.url); err != nil {
+			return fmt.Errorf("mcp oauth: %s endpoint: %w", ep.kind, err)
+		}
+	}
+	return nil
+}
+
 // resolveAuthorizationServer discovers metadata and applies explicit config
 // overrides. Configured endpoints take precedence over discovered ones, and act
 // as a fallback when discovery fails or omits a value.
@@ -148,6 +168,9 @@ func resolveAuthorizationServer(ctx context.Context, client *http.Client, baseUR
 			TokenEndpoint:         strings.TrimSpace(cfg.TokenEndpoint),
 			RegistrationEndpoint:  strings.TrimSpace(cfg.RegistrationEndpoint),
 		}
+		if err := validateResolvedEndpoints(metadata); err != nil {
+			return authServerMetadata{}, err
+		}
 		return metadata, nil
 	}
 
@@ -179,6 +202,9 @@ func resolveAuthorizationServer(ctx context.Context, client *http.Client, baseUR
 	if strings.TrimSpace(metadata.TokenEndpoint) == "" {
 		return authServerMetadata{}, errors.New("no token endpoint discovered or configured")
 	}
+	if err := validateResolvedEndpoints(metadata); err != nil {
+		return authServerMetadata{}, err
+	}
 	return metadata, nil
 }
 

internal/mcp/oauth_test.go

@@ -73,6 +73,22 @@ func TestResolveEndpointsFallsBackToConfig(t *testing.T) {
 	}
 }
 
+// Discovered MCP metadata that serves an insecure endpoint must be rejected
+// before use, so discovery cannot downgrade the MCP login to an
+// attacker-controlled origin (issue #511).
+func TestResolveAuthorizationServerRejectsInsecureDiscovered(t *testing.T) {
+	mux := http.NewServeMux()
+	server := httptest.NewServer(mux)
+	defer server.Close()
+	mux.HandleFunc("/.well-known/oauth-authorization-server", func(w http.ResponseWriter, _ *http.Request) {
+		_, _ = w.Write([]byte(`{"issuer":"` + server.URL + `","token_endpoint":"` + server.URL + `/token","authorization_endpoint":"http://evil.example/authorize"}`))
+	})
+	_, err := resolveAuthorizationServer(context.Background(), server.Client(), server.URL, OAuthConfig{})
+	if err == nil || !strings.Contains(err.Error(), "authorization endpoint") {
+		t.Fatalf("err = %v, want insecure authorization endpoint rejection", err)
+	}
+}
+
 // roundTripperFunc adapts a function to http.RoundTripper so a test can
 // observe every outbound request.
 type roundTripperFunc func(*http.Request) (*http.Response, error)

internal/oauth/flow.go

@@ -39,6 +39,13 @@ func BuildAuthorizationURL(cfg Config, pkce PKCE, state, redirectURI string, ext
 	if endpoint == "" {
 		return "", errors.New("oauth: no authorization endpoint configured")
 	}
+	// Backstop: validate the authorization endpoint at the shared choke point
+	// both the provider and MCP flows build their browser URL through, so a
+	// discovery-downgraded endpoint can never open in the browser even if a
+	// merge site missed it.
+	if err := ValidateEndpointURL(endpoint); err != nil {
+		return "", err
+	}
 	parsed, err := url.Parse(endpoint)
 	if err != nil {
 		return "", fmt.Errorf("oauth: parse authorization endpoint: %w", err)
@@ -83,13 +90,15 @@ func isReservedAuthParam(key string) bool {
 	}
 }
 
-// validateTokenEndpoint refuses to send a credential to a token endpoint that is
-// not https, unless it is a loopback host (mirrors oauth2-client.js
-// validateTokenEndpointURL). This prevents leaking codes/tokens over cleartext.
-func validateTokenEndpoint(endpoint string) error {
+// ValidateEndpointURL refuses an OAuth endpoint that is not https, unless it is
+// a loopback host (mirrors oauth2-client.js validateTokenEndpointURL). Every
+// credential-bearing endpoint — configured OR learned from discovery — must
+// pass this single rule, so discovery metadata can never downgrade a login to
+// cleartext or redirect it to an attacker-controlled http origin.
+func ValidateEndpointURL(endpoint string) error {
 	parsed, err := url.Parse(trimmed(endpoint))
 	if err != nil || parsed.Host == "" {
-		return fmt.Errorf("oauth: invalid token endpoint %q", endpoint)
+		return fmt.Errorf("oauth: invalid endpoint %q", endpoint)
 	}
 	if parsed.Scheme == "https" {
 		return nil
@@ -100,6 +109,13 @@ func validateTokenEndpoint(endpoint string) error {
 	return fmt.Errorf("%w: %s", ErrInsecureTokenEndpoint, parsed.Scheme+"://"+parsed.Host)
 }
 
+// validateTokenEndpoint checks a token endpoint against the shared endpoint rule
+// (kept as a named helper for the token-exchange and device call sites). This
+// prevents leaking codes/tokens over cleartext.
+func validateTokenEndpoint(endpoint string) error {
+	return ValidateEndpointURL(endpoint)
+}
+
 func isLoopbackHost(host string) bool {
 	if host == "localhost" {
 		return true

internal/oauth/flow_test.go

@@ -112,6 +112,20 @@ func TestBuildAuthorizationURLRejectsPlain(t *testing.T) {
 	}
 }
 
+// The shared choke point both flows build their browser URL through must refuse
+// an insecure authorization endpoint, so a discovery-downgraded endpoint can
+// never open in the browser even if a merge site missed it (issue #511).
+func TestBuildAuthorizationURLRejectsInsecureEndpoint(t *testing.T) {
+	_, err := BuildAuthorizationURL(
+		Config{AuthorizationEndpoint: "http://evil.example/authorize", ClientID: "c"},
+		PKCE{Method: MethodS256, Challenge: "chal"},
+		"state", "http://127.0.0.1/cb", nil,
+	)
+	if !errors.Is(err, ErrInsecureTokenEndpoint) {
+		t.Fatalf("err = %v, want ErrInsecureTokenEndpoint", err)
+	}
+}
+
 func TestValidateTokenEndpoint(t *testing.T) {
 	cases := map[string]bool{
 		"https://auth.example.com/token": true,

internal/oauth/manager.go

@@ -218,13 +218,26 @@ func (m *Manager) resolveEndpoints(ctx context.Context, cfg Config) (Config, err
 	// may already be sufficient for the chosen flow. Only merge on success (and
 	// never return from the error branch, which would trip the nilerr linter).
 	if meta, err := m.discover(ctx, cfg.IssuerURL); err == nil {
-		if cfg.AuthorizationEndpoint == "" {
+		// A discovered endpoint must pass the same https/loopback rule as a
+		// configured one before it is merged, so discovery metadata can never
+		// silently downgrade the login to an insecure or attacker-controlled
+		// endpoint. Fail closed on a bad endpoint rather than merging it.
+		if cfg.AuthorizationEndpoint == "" && meta.AuthorizationEndpoint != "" {
+			if err := ValidateEndpointURL(meta.AuthorizationEndpoint); err != nil {
+				return cfg, fmt.Errorf("oauth: discovered authorization endpoint: %w", err)
+			}
 			cfg.AuthorizationEndpoint = meta.AuthorizationEndpoint
 		}
-		if cfg.TokenEndpoint == "" {
+		if cfg.TokenEndpoint == "" && meta.TokenEndpoint != "" {
+			if err := ValidateEndpointURL(meta.TokenEndpoint); err != nil {
+				return cfg, fmt.Errorf("oauth: discovered token endpoint: %w", err)
+			}
 			cfg.TokenEndpoint = meta.TokenEndpoint
 		}
-		if cfg.DeviceAuthorizationEndpoint == "" {
+		if cfg.DeviceAuthorizationEndpoint == "" && meta.DeviceAuthorizationEndpoint != "" {
+			if err := ValidateEndpointURL(meta.DeviceAuthorizationEndpoint); err != nil {
+				return cfg, fmt.Errorf("oauth: discovered device authorization endpoint: %w", err)
+			}
 			cfg.DeviceAuthorizationEndpoint = meta.DeviceAuthorizationEndpoint
 		}
 	}

internal/oauth/manager_test.go

@@ -2,6 +2,7 @@ package oauth
 
 import (
 	"context"
+	"errors"
 	"io"
 	"net/http"
 	"net/http/httptest"
@@ -200,6 +201,24 @@ func TestManagerGetFreshDiscoversIssuerEndpoints(t *testing.T) {
 	}
 }
 
+// A discovery document that serves an insecure (non-https, non-loopback)
+// endpoint must be rejected before it is merged, so discovery metadata cannot
+// silently downgrade the login to an attacker-controlled origin (issue #511).
+func TestResolveEndpointsRejectsInsecureDiscoveredEndpoint(t *testing.T) {
+	mux := http.NewServeMux()
+	server := httptest.NewServer(mux)
+	defer server.Close()
+	mux.HandleFunc("/.well-known/oauth-authorization-server", func(w http.ResponseWriter, _ *http.Request) {
+		// Loopback issuer, but an attacker-controlled http authorization endpoint.
+		_, _ = io.WriteString(w, `{"issuer":"`+server.URL+`","token_endpoint":"`+server.URL+`/token","authorization_endpoint":"http://evil.example/authorize"}`)
+	})
+	m := managerFor(t, map[string]string{}, nil)
+	_, err := m.resolveEndpoints(context.Background(), Config{IssuerURL: server.URL})
+	if !errors.Is(err, ErrInsecureTokenEndpoint) {
+		t.Fatalf("resolveEndpoints err = %v, want ErrInsecureTokenEndpoint", err)
+	}
+}
+
 func TestManagerGetFreshRefreshesExpired(t *testing.T) {
 	fp := newFakeProvider(t, `{"access_token":"fresh-at","expires_in":3600}`)
 	env := map[string]string{

internal/sandbox/analyzer.go

@@ -65,6 +65,76 @@ 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,9 +219,65 @@ 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,3 +406,87 @@ 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")
+	}
+}