fix(config): keep sandbox.enabled out of the provider-command path (#687 review)

mergeConfig now copies Sandbox.Enabled, and it is shared by two callers: the
global-config merge (trusted) and the provider-command merge (not). A provider
command is an arbitrary executable named in config, and LoadProviderCommand
parses its stdout into a full FileConfig -- so one returning a valid provider
plus {"sandbox":{"enabled":false}} could disable the sandbox meant to constrain
it, despite the setting being documented as global-config/CLI only. Confirmed
reproducible in both directions before the fix.

Clear Sandbox.Enabled from the provider-command config before merging, mirroring
the carve-out mergeProjectConfig already applies for the same reason. Done at the
call site rather than inside mergeConfig because that helper must keep honouring
the setting for the trusted global-config merge.

Adds TestResolveSandboxEnabledIgnoredFromProviderCommand covering both
directions (cannot disable, cannot enable) and asserting the provider itself is
still applied, so the guard cannot be satisfied by dropping the command wholesale.

internal/cli/exec.go

@@ -939,6 +939,12 @@ func providerSensitiveEnvKeys(resolved config.ResolvedConfig) []string {
 // applyConfiguredSandboxPolicy overlays every config-sourced sandbox knob onto
 // the default policy.
 func applyConfiguredSandboxPolicy(policy sandbox.Policy, cfg config.SandboxConfig) sandbox.Policy {
+	// An explicit `"sandbox": {"enabled": false}` disables the sandbox: every
+	// request is allowed and commands run unwrapped. Only reachable from global
+	// config / CLI — project config never sets Enabled (see mergeProjectConfig).
+	if cfg.Enabled != nil && !*cfg.Enabled {
+		policy.Mode = sandbox.ModeDisabled
+	}
 	if network := strings.TrimSpace(cfg.Network); network != "" {
 		switch sandbox.NetworkMode(network) {
 		case sandbox.NetworkAllow, sandbox.NetworkDeny:

internal/cli/sandbox_test.go

@@ -1001,3 +1001,18 @@ func sandboxPolicyCapabilityStatus(capabilities []sandbox.BackendCapability, key
 	}
 	return ""
 }
+
+func TestApplyConfiguredSandboxPolicyEnabledFalseDisables(t *testing.T) {
+	disabled := false
+	if got := applyConfiguredSandboxPolicy(sandbox.DefaultPolicy(), config.SandboxConfig{Enabled: &disabled}); got.Mode != sandbox.ModeDisabled {
+		t.Fatalf("enabled:false must yield ModeDisabled, got %q", got.Mode)
+	}
+	// Omitted (nil) and explicit true keep the default enforcing mode.
+	if got := applyConfiguredSandboxPolicy(sandbox.DefaultPolicy(), config.SandboxConfig{}); got.Mode == sandbox.ModeDisabled {
+		t.Fatal("omitted enabled must not disable the sandbox")
+	}
+	enabled := true
+	if got := applyConfiguredSandboxPolicy(sandbox.DefaultPolicy(), config.SandboxConfig{Enabled: &enabled}); got.Mode == sandbox.ModeDisabled {
+		t.Fatal("enabled:true must not disable the sandbox")
+	}
+}

internal/config/resolver.go

@@ -93,6 +93,16 @@ func Resolve(options ResolveOptions) (ResolvedConfig, error) {
 		if err != nil {
 			return ResolvedConfig{}, err
 		}
+		// Sandbox.Enabled is NOT accepted from a provider command, for the same
+		// reason project config cannot set it (see mergeProjectConfig): only
+		// global config and the CLI may turn the sandbox off. A provider command
+		// is an arbitrary executable named in config, and LoadProviderCommand
+		// parses its stdout into a full FileConfig — so without this, a command
+		// returning a valid provider plus {"sandbox":{"enabled":false}} would
+		// disable the very sandbox meant to constrain what it can do. Cleared
+		// here rather than in mergeConfig because that helper is shared with the
+		// trusted global-config merge, which must keep honouring the setting.
+		commandConfig.Sandbox.Enabled = nil
 		mergeConfig(&cfg, commandConfig)
 	}
 
@@ -216,6 +226,9 @@ func mergeConfig(dst *FileConfig, src FileConfig) {
 		mergeProvider(dst, provider)
 	}
 	mergeMCPConfig(&dst.MCP, src.MCP, true)
+	if src.Sandbox.Enabled != nil {
+		dst.Sandbox.Enabled = src.Sandbox.Enabled
+	}
 	if network := strings.TrimSpace(src.Sandbox.Network); network != "" {
 		dst.Sandbox.Network = network
 	}
@@ -273,6 +286,10 @@ func mergeProjectConfig(dst *FileConfig, src FileConfig) error {
 	if err := mergeProjectMCPConfig(&dst.MCP, src.MCP); err != nil {
 		return err
 	}
+	// Sandbox.Enabled is intentionally NOT merged from project config: a cloned
+	// repo's .zero/config.json must not be able to disable the sandbox that
+	// constrains it. Only global config and CLI can turn the sandbox off.
+	//
 	// Sandbox.AdditionalWriteRoots is intentionally NOT merged from project
 	// config: a cloned repo's .zero/config.json must not be able to grant
 	// itself write access outside the workspace. Global config and CLI flags
@@ -688,6 +705,9 @@ func applyOverrides(cfg *FileConfig, overrides Overrides) {
 	if overrides.MaxTurns > 0 {
 		cfg.MaxTurns = overrides.MaxTurns
 	}
+	if overrides.Sandbox.Enabled != nil {
+		cfg.Sandbox.Enabled = overrides.Sandbox.Enabled
+	}
 	if overrides.Sandbox.BlockUnixSockets {
 		cfg.Sandbox.BlockUnixSockets = true
 	}

internal/config/resolver_test.go

@@ -1744,6 +1744,85 @@ func TestResolveSandboxNetworkProjectConfigCannotWeaken(t *testing.T) {
 	}
 }
 
+func TestResolveSandboxEnabledUserConfigDisables(t *testing.T) {
+	userPath := writeConfig(t, `{"sandbox": {"enabled": false}}`)
+	resolved, err := Resolve(ResolveOptions{UserConfigPath: userPath, Env: map[string]string{}})
+	if err != nil {
+		t.Fatalf("Resolve: %v", err)
+	}
+	if resolved.Sandbox.Enabled == nil || *resolved.Sandbox.Enabled {
+		t.Fatalf("user config enabled:false must resolve to a false pointer, got %v", resolved.Sandbox.Enabled)
+	}
+}
+
+func TestResolveSandboxDisableIgnoredFromProjectConfig(t *testing.T) {
+	// Security: a cloned repo's project config must NOT be able to disable the
+	// sandbox that constrains it. Only global config / CLI can turn it off.
+	userPath := writeConfig(t, `{}`)
+	projectPath := writeConfig(t, `{"sandbox": {"enabled": false}}`)
+	resolved, err := Resolve(ResolveOptions{UserConfigPath: userPath, ProjectConfigPath: projectPath, Env: map[string]string{}})
+	if err != nil {
+		t.Fatalf("Resolve: %v", err)
+	}
+	if resolved.Sandbox.Enabled != nil {
+		t.Fatalf("project config enabled:false must be ignored, got %v — a repo could disable the sandbox", *resolved.Sandbox.Enabled)
+	}
+}
+
+func TestResolveSandboxEnabledIgnoredFromProviderCommand(t *testing.T) {
+	// Security: a provider command is an arbitrary executable named in config, and
+	// its stdout is parsed into a full FileConfig — so it must NOT be able to reach
+	// Sandbox.Enabled, in either direction. Same rule as project config: only global
+	// config / CLI may turn the sandbox off.
+	for _, tc := range []struct {
+		name  string
+		value string
+	}{
+		{name: "cannot disable", value: "false"},
+		{name: "cannot enable", value: "true"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			command := writeCommand(t, commandScript{
+				Stdout: `{"activeProvider":"cmd","providers":[{"name":"cmd","provider":"openai","apiKey":"sk-command","model":"gpt-command"}],"sandbox":{"enabled":` + tc.value + `}}`,
+			})
+			resolved, err := Resolve(ResolveOptions{ProviderCommand: command, Env: map[string]string{}})
+			if err != nil {
+				t.Fatalf("Resolve: %v", err)
+			}
+			// The provider itself must still be applied — only the sandbox toggle is dropped.
+			if resolved.ActiveProvider != "cmd" {
+				t.Fatalf("ActiveProvider = %q, want cmd (provider command must still apply)", resolved.ActiveProvider)
+			}
+			if resolved.Sandbox.Enabled != nil {
+				t.Fatalf("provider command set sandbox.enabled=%s, got %v — a provider command could toggle the sandbox", tc.value, *resolved.Sandbox.Enabled)
+			}
+		})
+	}
+}
+
+func TestResolveSandboxEnabledCLIOverride(t *testing.T) {
+	// A CLI/programmatic override (applied after every config source) must be able
+	// to disable the sandbox, even with no config file.
+	disabled := false
+	resolved, err := Resolve(ResolveOptions{Env: map[string]string{}, Overrides: Overrides{Sandbox: SandboxConfig{Enabled: &disabled}}})
+	if err != nil {
+		t.Fatalf("Resolve: %v", err)
+	}
+	if resolved.Sandbox.Enabled == nil || *resolved.Sandbox.Enabled {
+		t.Fatalf("CLI override enabled:false must disable the sandbox, got %v", resolved.Sandbox.Enabled)
+	}
+	// An explicit true override must win over a lower-precedence global-config false.
+	userPath := writeConfig(t, `{"sandbox": {"enabled": false}}`)
+	enabled := true
+	resolved2, err := Resolve(ResolveOptions{UserConfigPath: userPath, Env: map[string]string{}, Overrides: Overrides{Sandbox: SandboxConfig{Enabled: &enabled}}})
+	if err != nil {
+		t.Fatalf("Resolve: %v", err)
+	}
+	if resolved2.Sandbox.Enabled == nil || !*resolved2.Sandbox.Enabled {
+		t.Fatalf("CLI override enabled:true must win over config false, got %v", resolved2.Sandbox.Enabled)
+	}
+}
+
 func TestResolveNotifyValid(t *testing.T) {
 	path := writeConfig(t, `{"notify":{"mode":"both","focusMode":"always"}}`)
 	resolved, err := Resolve(ResolveOptions{UserConfigPath: path, Env: map[string]string{}})

internal/config/types.go

@@ -62,6 +62,12 @@ func HasProviderProfile(profile ProviderProfile) bool {
 }
 
 type SandboxConfig struct {
+	// Enabled turns the sandbox off when set to false (`"sandbox": {"enabled":
+	// false}`). A pointer distinguishes an explicit false (disable) from an
+	// omitted key (keep the default: enabled). Honored from the GLOBAL user
+	// config and CLI only — deliberately NOT project config, so a cloned repo
+	// cannot disable the sandbox that constrains it. nil / true keep enforcement.
+	Enabled *bool `json:"enabled,omitempty"`
 	// Network controls whether shell commands classified as network-touching
 	// (curl, git push, package installs, …) are allowed: "allow" or "deny".
 	// Empty keeps the built-in default (deny). Without this knob the engine's

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{