diff --git a/.changeset/restore-sandbox-agent-false.md b/.changeset/restore-sandbox-agent-false.md new file mode 100644 index 00000000000..c39c3ae6b51 --- /dev/null +++ b/.changeset/restore-sandbox-agent-false.md @@ -0,0 +1,15 @@ +--- +"gh-aw": major +--- + +Restore `sandbox.agent: false` as a supported non-strict mode and require `features.dangerously-disable-sandbox-agent: true` to enable it. + +Rename the feature flag from `features.dangerously-disable-sandbox` to `features.dangerously-disable-sandbox-agent`. The restored opt-out requires the renamed flag to be set to `true`: + +```yaml +features: + dangerously-disable-sandbox-agent: true +sandbox: + agent: false +strict: false +``` diff --git a/.github/aw/syntax-agentic.md b/.github/aw/syntax-agentic.md index 40b5fc88ec8..12ef4102d9b 100644 --- a/.github/aw/syntax-agentic.md +++ b/.github/aw/syntax-agentic.md @@ -61,7 +61,7 @@ description: Agentic workflow specific frontmatter fields for GitHub Agentic Wor - `difc-proxy: true` - Enable DIFC (Data Integrity and Flow Control) proxy injection. When set alongside `tools.github.min-integrity`, injects proxy steps around the agent for full network-boundary integrity enforcement. - `cli-proxy: true` - Enable AWF CLI proxy sidecar for secure read-only `gh` CLI access without exposing `GITHUB_TOKEN` (requires AWF v0.26.0+). Prerequisite for `integrity-reactions`; the compiler enables it automatically when `integrity-reactions: true` is set. - `integrity-reactions: true` - Enable reaction-based integrity promotion/demotion. Maintainers can use 👍/❤️ reactions to promote content to `approved` and 👎/😕 to demote it to `none`. Compiler automatically enables `cli-proxy`. Requires `tools.github.min-integrity` to be set and MCPG >= v0.2.18. Defaults: endorsement reactions THUMBS_UP/HEART, disapproval reactions THUMBS_DOWN/CONFUSED, endorser-min-integrity: approved, disapproval-integrity: none. - - `dangerously-disable-sandbox-agent: ""` - Required when `sandbox.agent: false` is set. Must be a plain string justification (minimum 20 characters; expressions are not allowed) that explains why disabling the sandbox is safe for this workflow. + - `dangerously-disable-sandbox-agent: true` - Required when `sandbox.agent: false` is set. This opt-out is rejected in strict mode. - **`experiments:`** - A/B testing experiments for balanced variant selection (object) - Maps experiment names to variant lists (bare array) or full config objects @@ -329,13 +329,14 @@ description: Agentic workflow specific frontmatter fields for GitHub Agentic Wor - When `engine.env` sets `OPENAI_BASE_URL` or `ANTHROPIC_BASE_URL` (custom provider endpoints, e.g. OpenRouter), `model-fallback` is disabled automatically so provider-specific model slugs pass through verbatim; set it explicitly to override. - - To disable the agent firewall while keeping MCP gateway enabled, you must provide the dangerous-disable justification feature: + - To disable the agent firewall while keeping MCP gateway enabled, set `strict: false` and enable the dangerous sandbox opt-out: ```yaml features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false + strict: false ``` - **`sandbox.agent.runtime`** (string) selects the sandbox security and topology profile: `docker` (default: rootless AWF with network isolation), `docker-sudo-iptables` (privileged AWF with legacy iptables networking and host/service access), `gvisor` (gVisor `runsc` kernel-level isolation), `docker-sbx` (KVM microVM), or `cloud-hypervisor` (preview KVM runtime). Omitting the field is equivalent to `docker`. gVisor and Docker sbx are incompatible with `runner.topology: arc-dind`; the compiler derives the privileged setup each runtime needs. Docker sbx also requires `DOCKER_PAT`/`DOCKER_USERNAME` secrets and a KVM-capable runner when runtime installation is enabled. diff --git a/.github/aw/upgrade-agentic-workflows.md b/.github/aw/upgrade-agentic-workflows.md index 3ecfa940f94..f0b7580d717 100644 --- a/.github/aw/upgrade-agentic-workflows.md +++ b/.github/aw/upgrade-agentic-workflows.md @@ -102,12 +102,13 @@ Before attempting to compile, apply automatic codemods: This will automatically update workflow files with changes like: - Replacing 'timeout_minutes' with 'timeout-minutes' - - Replacing `network.firewall: false` with: + - Replacing `network.firewall: false` with `sandbox.agent: false`. To keep the sandbox disabled, explicitly add: ```yaml features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false + strict: false ``` - Removing deprecated 'mcp-scripts.mode' field diff --git a/docs/adr/29483-automate-removal-of-deprecated-sandbox-keys-via-codemods.md b/docs/adr/29483-automate-removal-of-deprecated-sandbox-keys-via-codemods.md index 587196073e3..185abb83994 100644 --- a/docs/adr/29483-automate-removal-of-deprecated-sandbox-keys-via-codemods.md +++ b/docs/adr/29483-automate-removal-of-deprecated-sandbox-keys-via-codemods.md @@ -1,9 +1,13 @@ # ADR-29483: Automate Removal of Deprecated Sandbox Keys via Codemods **Date**: 2026-05-01 -**Status**: Draft +**Status**: Superseded **Deciders**: Unknown +> Superseded by the restored non-strict opt-out policy: `sandbox.agent: false` is +> supported when `features.dangerously-disable-sandbox-agent: true` is set, and +> the removal codemod is no longer registered. + --- ## Part 1 — Narrative (Human-Friendly) diff --git a/docs/adr/38325-require-operator-justification-for-disabling-agent-sandbox.md b/docs/adr/38325-require-operator-justification-for-disabling-agent-sandbox.md index 1066e9f5865..7df29754c90 100644 --- a/docs/adr/38325-require-operator-justification-for-disabling-agent-sandbox.md +++ b/docs/adr/38325-require-operator-justification-for-disabling-agent-sandbox.md @@ -1,7 +1,11 @@ # ADR-38325: Require Operator-Authored Justification to Disable the Agent Sandbox **Date**: 2026-06-10 -**Status**: Draft +**Status**: Superseded + +> Superseded by the restored non-strict opt-out policy: `sandbox.agent: false` now +> requires `features.dangerously-disable-sandbox-agent: true`, while strict mode +> continues to reject the configuration. ## Context diff --git a/docs/src/content/docs/reference/gh-aw-as-mcp-server.md b/docs/src/content/docs/reference/gh-aw-as-mcp-server.md index 29c4b2d3a72..4819aefbe53 100644 --- a/docs/src/content/docs/reference/gh-aw-as-mcp-server.md +++ b/docs/src/content/docs/reference/gh-aw-as-mcp-server.md @@ -129,7 +129,7 @@ For `audit`, each run identifier may be a numeric run ID, a run URL, a job URL, For `checks`, normalized states are `success`, `failed`, `pending`, `no_checks`, and `policy_blocked`. Use `required_state` as the authoritative CI verdict when optional third-party deployments are present. -Available `fix` codemods: `timeout-minutes-migration`, `network-firewall-migration`, `sandbox-agent-false-removal`, `mcp-scripts-mode-removal`, `steps-run-secrets-to-env`. +Available `fix` codemods include `timeout-minutes-migration`, `network-firewall-migration`, `mcp-scripts-mode-removal`, and `steps-run-secrets-to-env`. ## Using GH-AW as an MCP from an Agentic Workflow diff --git a/docs/src/content/docs/reference/sandbox.md b/docs/src/content/docs/reference/sandbox.md index 1f5595e1025..a07a0aec6c6 100644 --- a/docs/src/content/docs/reference/sandbox.md +++ b/docs/src/content/docs/reference/sandbox.md @@ -22,9 +22,9 @@ Configure the coding agent sandbox type to control how the AI engine is isolated sandbox: agent: awf -# Disable coding agent sandbox - requires an operator-authored justification +# Disable coding agent sandbox - requires an explicit feature flag features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false @@ -37,23 +37,20 @@ If `sandbox` is not specified in your workflow, it defaults to `sandbox.agent: a **Disabling Coding Agent Sandbox** -Setting `sandbox.agent: false` is deprecated and will be removed in a future release. It disables the agent firewall while keeping the MCP gateway enabled, removing a trust boundary, and should only be used when strictly necessary. +Setting `sandbox.agent: false` disables the agent firewall while keeping the MCP gateway enabled. This removes a trust boundary and is only supported when `strict: false`. -To disable the agent sandbox, you **must** add `features.dangerously-disable-sandbox-agent` with a literal justification string of at least 20 characters. The justification must explain why the trust boundary is being removed and is stored for diagnostics and audit. The following values are rejected by the compiler: - -- Boolean `true` — no longer accepted as a legacy shorthand -- Expressions such as `${{ inputs.reason }}` — must be a static literal -- Strings shorter than 20 characters after trimming whitespace +To disable the agent sandbox, you **must** set `features.dangerously-disable-sandbox-agent: true`. Missing, false, and non-boolean values are rejected by the compiler. ```yaml wrap features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false +strict: false ``` > [!WARNING] -> Disabling the agent sandbox removes a security trust boundary. The `dangerously-disable-sandbox-agent` value is a permanent, reviewable record of why this workflow runs without the agent firewall. Write a reason that will be meaningful to future reviewers. +> Disabling the agent sandbox removes a security trust boundary and is always rejected in strict mode. Only use this opt-out in controlled environments where the agent can be trusted with direct network access. ### Runtime Profiles diff --git a/pkg/cli/codemod_network_firewall_test.go b/pkg/cli/codemod_network_firewall_test.go index b03699e95dd..067c509bfd4 100644 --- a/pkg/cli/codemod_network_firewall_test.go +++ b/pkg/cli/codemod_network_firewall_test.go @@ -85,7 +85,7 @@ permissions: assert.NotContains(t, result, "firewall:", "Should remove firewall field") assert.Contains(t, result, "sandbox:", "Should add sandbox block") assert.Contains(t, result, "agent: false", "Should convert firewall false to sandbox.agent: false") - assert.NotContains(t, result, "dangerously-disable-sandbox-agent", "Codemod must not silently invent a justification; operator must provide one") + assert.NotContains(t, result, "dangerously-disable-sandbox-agent", "Codemod must not silently enable the sandbox opt-out; operator must provide it") } func TestNetworkFirewallCodemod_NoNetworkField(t *testing.T) { @@ -216,10 +216,10 @@ sandbox: assert.Contains(t, result, "sandbox:", "Should preserve existing sandbox block") assert.Contains(t, result, "mcp: true", "Should preserve existing sandbox settings") assert.Contains(t, result, "agent: false", "Should migrate firewall false to sandbox.agent: false") - assert.NotContains(t, result, "dangerously-disable-sandbox-agent", "Codemod must not silently invent a justification; operator must provide one") + assert.NotContains(t, result, "dangerously-disable-sandbox-agent", "Codemod must not silently enable the sandbox opt-out; operator must provide it") } -func TestNetworkFirewallCodemod_PreservesExistingSandboxDisableJustification(t *testing.T) { +func TestNetworkFirewallCodemod_PreservesExistingSandboxDisableFeature(t *testing.T) { t.Parallel() codemod := getNetworkFirewallCodemod() @@ -228,7 +228,7 @@ on: workflow_dispatch network: firewall: false features: - dangerously-disable-sandbox-agent: "already documented justification string with enough detail" + dangerously-disable-sandbox-agent: true sandbox: mcp: true --- @@ -241,7 +241,7 @@ sandbox: "firewall": false, }, "features": map[string]any{ - "dangerously-disable-sandbox-agent": "already documented justification string with enough detail", + "dangerously-disable-sandbox-agent": true, }, "sandbox": map[string]any{ "mcp": true, @@ -252,8 +252,7 @@ sandbox: require.NoError(t, err) assert.True(t, applied) - assert.Contains(t, result, `dangerously-disable-sandbox-agent: "already documented justification string with enough detail"`) - assert.NotContains(t, result, "migrated from deprecated", "Should not overwrite existing justification with a generic one") + assert.Contains(t, result, "dangerously-disable-sandbox-agent: true") } func TestNetworkFirewallCodemod_MigratesFirewallVersionIntoExistingSandbox(t *testing.T) { diff --git a/pkg/cli/codemod_sandbox_agent_false_removal.go b/pkg/cli/codemod_sandbox_agent_false_removal.go deleted file mode 100644 index b731eba0b61..00000000000 --- a/pkg/cli/codemod_sandbox_agent_false_removal.go +++ /dev/null @@ -1,52 +0,0 @@ -package cli - -import "github.com/github/gh-aw/pkg/logger" - -var sandboxAgentFalseRemovalCodemodLog = logger.New("cli:codemod_sandbox_agent_false_removal") - -// getSandboxAgentFalseRemovalCodemod creates a codemod that removes the deprecated -// sandbox.agent: false key. Setting sandbox.agent to false was previously supported as -// an escape hatch for testing but is now rejected in strict mode because it disables -// important agent sandbox security protections. Remove the key to restore default -// (sandboxed) behavior, or set 'strict: false' to opt out of strict mode. -func getSandboxAgentFalseRemovalCodemod() Codemod { - return Codemod{ - ID: "sandbox-agent-false-removal", - Name: "Remove deprecated sandbox.agent: false field", - Description: "Removes 'sandbox.agent: false' which is no longer allowed in strict mode. The agent sandbox firewall is now always enabled by default. Remove this key to restore sandboxed behavior, or set 'strict: false' to opt out of strict mode.", - IntroducedIn: "0.26.0", - Apply: func(content string, frontmatter map[string]any) (string, bool, error) { - if isFrontmatterStrictFalse(frontmatter) { - return content, false, nil - } - if !isSandboxAgentFalse(frontmatter) { - return content, false, nil - } - newContent, applied, err := applyFrontmatterLineTransform(content, func(lines []string) ([]string, bool) { - return removeFieldFromBlock(lines, "agent", "sandbox") - }) - if applied { - sandboxAgentFalseRemovalCodemodLog.Print("Removed deprecated sandbox.agent: false") - } - return newContent, applied, err - }, - } -} - -// isSandboxAgentFalse returns true when frontmatter["sandbox"]["agent"] is boolean false. -func isSandboxAgentFalse(frontmatter map[string]any) bool { - sandboxVal, ok := frontmatter["sandbox"] - if !ok { - return false - } - sandboxMap, ok := sandboxVal.(map[string]any) - if !ok { - return false - } - agentVal, ok := sandboxMap["agent"] - if !ok { - return false - } - agentBool, ok := agentVal.(bool) - return ok && !agentBool -} diff --git a/pkg/cli/codemod_sandbox_agent_false_removal_test.go b/pkg/cli/codemod_sandbox_agent_false_removal_test.go deleted file mode 100644 index 8ac414cd686..00000000000 --- a/pkg/cli/codemod_sandbox_agent_false_removal_test.go +++ /dev/null @@ -1,280 +0,0 @@ -//go:build !integration - -package cli - -import ( - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestGetSandboxAgentFalseRemovalCodemod(t *testing.T) { - t.Parallel() - codemod := getSandboxAgentFalseRemovalCodemod() - - assert.Equal(t, "sandbox-agent-false-removal", codemod.ID) - assert.Equal(t, "Remove deprecated sandbox.agent: false field", codemod.Name) - assert.NotEmpty(t, codemod.Description) - assert.Equal(t, "0.26.0", codemod.IntroducedIn) - require.NotNil(t, codemod.Apply) -} - -func TestSandboxAgentFalseRemoval_RemovesAgentFalse(t *testing.T) { - t.Parallel() - codemod := getSandboxAgentFalseRemovalCodemod() - - content := `--- -on: workflow_dispatch -sandbox: - agent: false -permissions: - contents: read ---- - -# Test` - - frontmatter := map[string]any{ - "on": "workflow_dispatch", - "sandbox": map[string]any{ - "agent": false, - }, - "permissions": map[string]any{ - "contents": "read", - }, - } - - result, applied, err := codemod.Apply(content, frontmatter) - - require.NoError(t, err) - assert.True(t, applied) - assert.NotContains(t, result, "agent: false") - // Empty sandbox block must be removed to avoid "got null, want object" compile error - assert.NotContains(t, result, "sandbox:") -} - -func TestSandboxAgentFalseRemoval_PreservesOtherSandboxKeys(t *testing.T) { - t.Parallel() - codemod := getSandboxAgentFalseRemovalCodemod() - - content := `--- -on: workflow_dispatch -sandbox: - agent: false - mcp: - port: 8080 ---- - -# Test` - - frontmatter := map[string]any{ - "on": "workflow_dispatch", - "sandbox": map[string]any{ - "agent": false, - "mcp": map[string]any{ - "port": 8080, - }, - }, - } - - result, applied, err := codemod.Apply(content, frontmatter) - - require.NoError(t, err) - assert.True(t, applied) - assert.NotContains(t, result, "agent: false") - assert.Contains(t, result, "mcp:") - assert.Contains(t, result, "port: 8080") -} - -func TestSandboxAgentFalseRemoval_NoSandboxKey(t *testing.T) { - t.Parallel() - codemod := getSandboxAgentFalseRemovalCodemod() - - content := `--- -on: workflow_dispatch -permissions: - contents: read ---- - -# Test` - - frontmatter := map[string]any{ - "on": "workflow_dispatch", - "permissions": map[string]any{ - "contents": "read", - }, - } - - result, applied, err := codemod.Apply(content, frontmatter) - - require.NoError(t, err) - assert.False(t, applied) - assert.Equal(t, content, result) -} - -func TestSandboxAgentFalseRemoval_AgentNotFalse(t *testing.T) { - t.Parallel() - codemod := getSandboxAgentFalseRemovalCodemod() - - content := `--- -on: workflow_dispatch -sandbox: - agent: awf ---- - -# Test` - - frontmatter := map[string]any{ - "on": "workflow_dispatch", - "sandbox": map[string]any{ - "agent": "awf", - }, - } - - result, applied, err := codemod.Apply(content, frontmatter) - - require.NoError(t, err) - assert.False(t, applied) - assert.Equal(t, content, result) -} - -func TestSandboxAgentFalseRemoval_AgentObject(t *testing.T) { - t.Parallel() - codemod := getSandboxAgentFalseRemovalCodemod() - - content := `--- -on: workflow_dispatch -sandbox: - agent: - id: awf ---- - -# Test` - - frontmatter := map[string]any{ - "on": "workflow_dispatch", - "sandbox": map[string]any{ - "agent": map[string]any{ - "id": "awf", - }, - }, - } - - result, applied, err := codemod.Apply(content, frontmatter) - - require.NoError(t, err) - assert.False(t, applied) - assert.Equal(t, content, result) -} - -func TestSandboxAgentFalseRemoval_AgentTrue(t *testing.T) { - t.Parallel() - codemod := getSandboxAgentFalseRemovalCodemod() - - content := `--- -on: workflow_dispatch -sandbox: - agent: true ---- - -# Test` - - frontmatter := map[string]any{ - "on": "workflow_dispatch", - "sandbox": map[string]any{ - "agent": true, - }, - } - - result, applied, err := codemod.Apply(content, frontmatter) - - require.NoError(t, err) - assert.False(t, applied) - assert.Equal(t, content, result) -} - -func TestSandboxAgentFalseRemoval_SkipsWhenStrictFalse(t *testing.T) { - t.Parallel() - codemod := getSandboxAgentFalseRemovalCodemod() - - content := `--- -on: workflow_dispatch -strict: false -sandbox: - agent: false ---- - -# Test` - - frontmatter := map[string]any{ - "on": "workflow_dispatch", - "strict": false, - "sandbox": map[string]any{ - "agent": false, - }, - } - - result, applied, err := codemod.Apply(content, frontmatter) - - require.NoError(t, err) - assert.False(t, applied, "should not apply when strict: false is set") - assert.Equal(t, content, result) -} - -func TestSandboxAgentFalseRemoval_PreservesMarkdown(t *testing.T) { - t.Parallel() - codemod := getSandboxAgentFalseRemovalCodemod() - - content := `--- -on: workflow_dispatch -sandbox: - agent: false ---- - -# Workflow Title - -This workflow was using the nosandbox escape hatch.` - - frontmatter := map[string]any{ - "on": "workflow_dispatch", - "sandbox": map[string]any{ - "agent": false, - }, - } - - result, applied, err := codemod.Apply(content, frontmatter) - - require.NoError(t, err) - assert.True(t, applied) - assert.Contains(t, result, "# Workflow Title") - assert.NotContains(t, result, "agent: false") -} - -func TestSandboxAgentFalseRemoval_NoAgentKey(t *testing.T) { - t.Parallel() - codemod := getSandboxAgentFalseRemovalCodemod() - - content := `--- -on: workflow_dispatch -sandbox: - mcp: - port: 8080 ---- - -# Test` - - frontmatter := map[string]any{ - "on": "workflow_dispatch", - "sandbox": map[string]any{ - "mcp": map[string]any{ - "port": 8080, - }, - }, - } - - result, applied, err := codemod.Apply(content, frontmatter) - - require.NoError(t, err) - assert.False(t, applied) - assert.Equal(t, content, result) -} diff --git a/pkg/cli/fix_codemods.go b/pkg/cli/fix_codemods.go index 730b2c61345..8cbaa756828 100644 --- a/pkg/cli/fix_codemods.go +++ b/pkg/cli/fix_codemods.go @@ -129,7 +129,6 @@ func getLaterCodemods() []Codemod { getCLIProxyBashDisabledCodemod(), // Set tools.cli-proxy: false when tools.bash is disabled getSandboxMCPContainerRemovalCodemod(), // Remove deprecated sandbox.mcp.container (now managed internally) getSandboxMCPVersionRemovalCodemod(), // Remove deprecated sandbox.mcp.version (now managed internally) - getSandboxAgentFalseRemovalCodemod(), // Remove deprecated sandbox.agent: false (rejected in strict mode) getSandboxRuntimeProfileCodemod(), // Migrate sandbox.agent.sudo / legacy-security to sandbox.agent.runtime profiles getInferToDisableModelInvocationCodemod(), // Migrate deprecated 'infer' to 'disable-model-invocation' getRunInstallScriptsToRuntimesNodeCodemod(), // Move top-level run-install-scripts under runtimes.node diff --git a/pkg/cli/fix_codemods_test.go b/pkg/cli/fix_codemods_test.go index f9101496bc4..b32878df78b 100644 --- a/pkg/cli/fix_codemods_test.go +++ b/pkg/cli/fix_codemods_test.go @@ -132,7 +132,6 @@ func TestGetAllCodemods_ContainsExpectedCodemods(t *testing.T) { "cli-proxy-false-when-bash-disabled", "sandbox-mcp-container-removal", "sandbox-mcp-version-removal", - "sandbox-agent-false-removal", "bash-single-quoted-args-rewrite", "bash-allowlist-unsupported-engine-guided-error", "infer-to-disable-model-invocation", @@ -262,7 +261,6 @@ func expectedCodemodOrder() []string { "cli-proxy-false-when-bash-disabled", "sandbox-mcp-container-removal", "sandbox-mcp-version-removal", - "sandbox-agent-false-removal", "sandbox-runtime-profiles", "infer-to-disable-model-invocation", "run-install-scripts-to-runtimes-node", diff --git a/pkg/cli/mcp_tools_management.go b/pkg/cli/mcp_tools_management.go index 0b627d35983..7dbfca5928a 100644 --- a/pkg/cli/mcp_tools_management.go +++ b/pkg/cli/mcp_tools_management.go @@ -169,7 +169,7 @@ type fixArgs struct { } // registerFixTool registers the fix tool with the MCP server. -func registerFixTool(server *mcp.Server, execCmd execCmdFunc) { +func registerFixTool(server *mcp.Server, execCmd execCmdFunc) { //nolint:largefunc // Existing MCP tool registration remains centralized. mcp.AddTool(server, &mcp.Tool{ Name: "fix", Annotations: &mcp.ToolAnnotations{ @@ -185,7 +185,6 @@ and migrate to new syntax. Codemods preserve formatting and comments as much as Available codemods: • timeout-minutes-migration: Replaces 'timeout_minutes' with 'timeout-minutes' • network-firewall-migration: Removes deprecated 'network.firewall' field -• sandbox-agent-false-removal: Removes 'sandbox.agent: false' (firewall now mandatory) • mcp-scripts-mode-removal: Removes deprecated 'mcp-scripts.mode' field If no workflows are specified, all Markdown files in .github/workflows will be processed. diff --git a/pkg/constants/feature_constants.go b/pkg/constants/feature_constants.go index 861d6385327..f41b88b467f 100644 --- a/pkg/constants/feature_constants.go +++ b/pkg/constants/feature_constants.go @@ -84,7 +84,7 @@ const ( // Workflow frontmatter usage: // // features: - // dangerously-disable-sandbox-agent: "controlled environment with no internet access" + // dangerously-disable-sandbox-agent: true DangerouslyDisableSandboxAgentFeatureFlag FeatureFlag = "dangerously-disable-sandbox-agent" // GHAWDetectionFeatureFlag controls the external threat-detect binary detection path. // The external detector is enabled by default. Set this flag to false to use diff --git a/pkg/workflow/aw_info_steps_test.go b/pkg/workflow/aw_info_steps_test.go index 32bdeb7608e..9c39b291266 100644 --- a/pkg/workflow/aw_info_steps_test.go +++ b/pkg/workflow/aw_info_steps_test.go @@ -48,7 +48,7 @@ permissions: pull-requests: read engine: copilot features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false strict: false diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index a252ddcdb9b..be53c207d54 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -144,7 +144,7 @@ func shouldDowngradeDefaultToolsetPermissionError(githubTool *GitHubToolConfig) // generateAndValidateYAML generates GitHub Actions YAML and validates // the output size and format. -func (c *Compiler) generateAndValidateYAML(workflowData *WorkflowData, markdownPath string, lockFile string) (string, []string, []string, error) { +func (c *Compiler) generateAndValidateYAML(workflowData *WorkflowData, markdownPath string, lockFile string) (string, []string, []string, error) { //nolint:largefunc // Existing YAML generation and validation remains centralized. // Generate the YAML content along with the collected body secrets and action refs // (returned to avoid a second scan of the full YAML in the caller for safe update enforcement). yamlContent, bodySecrets, bodyActions, err := c.generateYAML(workflowData, markdownPath) @@ -435,7 +435,7 @@ func (c *Compiler) readLockFileFromHEAD(lockFile string) (string, error) { // This function avoids re-parsing when workflow data has already been extracted, // making it efficient for scenarios where the same workflow is compiled multiple times // or when workflow data comes from a non-file source. -func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath string) error { +func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath string) error { //nolint:largefunc // Existing compilation lifecycle remains centralized. // Store markdownPath for use in dynamic tool generation and prompt generation c.markdownPath = markdownPath @@ -473,6 +473,15 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath workflowLog.Printf("Starting compilation: %s -> %s", markdownPath, lockFile) + // CompileWorkflowData is also a public entry point, so enforce the + // strict-mode sandbox restriction here rather than relying on the + // orchestrator path to validate it. + if err := c.withEffectiveStrictMode(workflowData.RawFrontmatter, func() error { + return c.validateStrictFirewall("", nil, workflowData.SandboxConfig) + }); err != nil { + return formatCompilerError(markdownPath, "error", err.Error(), err) + } + // Resolve and cache the baseline manifest only when safe update mode is active. // This avoids unnecessary git/filesystem reads on compile paths that skip safe update // enforcement (e.g., --approve or strict: false). diff --git a/pkg/workflow/compiler_test.go b/pkg/workflow/compiler_test.go index 6ab62cdc20f..eb8f0d80341 100644 --- a/pkg/workflow/compiler_test.go +++ b/pkg/workflow/compiler_test.go @@ -311,6 +311,29 @@ Test content require.NoError(t, err, "Lock file should be created") } +func TestCompileWorkflowData_RejectsDisabledSandboxInStrictMode(t *testing.T) { + tmpDir := testutil.TempDir(t, "compiler-data-strict-sandbox-test") + markdownPath := filepath.Join(tmpDir, "test.md") + + compiler := NewCompiler(WithNoEmit(true)) + workflowData := &WorkflowData{ + RawFrontmatter: map[string]any{ + "strict": true, + }, + Features: map[string]any{ + "dangerously-disable-sandbox-agent": true, + }, + SandboxConfig: &SandboxConfig{ + Agent: &AgentSandboxConfig{Disabled: true}, + }, + } + + err := compiler.CompileWorkflowData(workflowData, markdownPath) + require.Error(t, err) + require.ErrorContains(t, err, "strict mode") + require.ErrorContains(t, err, "sandbox.agent: false") +} + func TestCompileWorkflow_CachesResolvedManifestBaseline(t *testing.T) { tmpDir := testutil.TempDir(t, "compiler-manifest-cache") testFile := filepath.Join(tmpDir, "test-workflow.md") diff --git a/pkg/workflow/compiler_validators.go b/pkg/workflow/compiler_validators.go index 16cbda96984..70a117b4d2d 100644 --- a/pkg/workflow/compiler_validators.go +++ b/pkg/workflow/compiler_validators.go @@ -432,14 +432,6 @@ func (c *Compiler) emitGeneralToolWarnings(workflowData *WorkflowData, markdownP "See: https://gh.io/gh-aw/reference/concurrency for details.")) c.IncrementWarningCount() } - if isAgentSandboxDisabled(workflowData) { - fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", - "sandbox.agent: false is deprecated and will be removed in a future release. "+ - "It disables the firewall, giving the AI agent direct network access without filtering. "+ - "The MCP gateway remains enabled. Only use this for testing or in controlled "+ - "environments where you trust the AI agent completely.")) - c.IncrementWarningCount() - } c.emitSandboxRuntimeWarnings(workflowData, markdownPath) c.emitPiThreatDetectionAuthWarning(workflowData, markdownPath) if workflowData.SafeOutputs != nil && workflowData.SafeOutputs.AssignToAgent != nil && diff --git a/pkg/workflow/compiler_validators_test.go b/pkg/workflow/compiler_validators_test.go index ffbf709d69c..6ee843713f5 100644 --- a/pkg/workflow/compiler_validators_test.go +++ b/pkg/workflow/compiler_validators_test.go @@ -275,7 +275,7 @@ func TestEmitGeneralToolWarningsCloudHypervisorReviewTrigger(t *testing.T) { assert.Equal(t, 1, compiler.GetWarningCount()) } -func TestEmitGeneralToolWarningsDeprecatedSandboxOptions(t *testing.T) { +func TestEmitGeneralToolWarningsDeprecatedSandboxRuntimes(t *testing.T) { tests := []struct { name string agent *AgentSandboxConfig @@ -291,11 +291,6 @@ func TestEmitGeneralToolWarningsDeprecatedSandboxOptions(t *testing.T) { agent: &AgentSandboxConfig{Runtime: AgentRuntimeDockerSbx}, expectedMessage: "sandbox.agent.runtime: docker-sbx is deprecated and will be removed in a future release", }, - { - name: "disabled AWF", - agent: &AgentSandboxConfig{Disabled: true}, - expectedMessage: "sandbox.agent: false is deprecated and will be removed in a future release", - }, } for _, tt := range tests { @@ -938,7 +933,7 @@ func TestShouldEmitCopilotRequestsEnableTip(t *testing.T) { } } -func TestValidateToolConfiguration_EmitsSandboxWarningBeforeThreatDetectionError(t *testing.T) { +func TestValidateToolConfiguration_DoesNotWarnForDisabledSandbox(t *testing.T) { tmpDir := testutil.TempDir(t, "tool-warning-test") markdownPath := filepath.Join(tmpDir, "test.md") @@ -948,7 +943,7 @@ func TestValidateToolConfiguration_EmitsSandboxWarningBeforeThreatDetectionError workflowData := &WorkflowData{ Name: "Test", Features: map[string]any{ - "dangerously-disable-sandbox-agent": "controlled environment with no internet access", + "dangerously-disable-sandbox-agent": true, }, SandboxConfig: &SandboxConfig{ Agent: &AgentSandboxConfig{Disabled: true}, @@ -966,8 +961,8 @@ func TestValidateToolConfiguration_EmitsSandboxWarningBeforeThreatDetectionError require.Error(t, validateErr) require.ErrorContains(t, validateErr, "threat detection requires sandbox.agent") - assert.Contains(t, stderr, "sandbox.agent: false is deprecated and will be removed in a future release") - assert.Equal(t, initialWarnings+1, compiler.GetWarningCount()) + assert.NotContains(t, stderr, "sandbox.agent: false") + assert.Equal(t, initialWarnings, compiler.GetWarningCount()) } // TestWarnPromptTmpPaths tests the /tmp path heuristic used by the compiler. diff --git a/pkg/workflow/importable_tools_test.go b/pkg/workflow/importable_tools_test.go index 1fee613a965..617ae999e6b 100644 --- a/pkg/workflow/importable_tools_test.go +++ b/pkg/workflow/importable_tools_test.go @@ -727,7 +727,7 @@ permissions: tools: bash: true features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false imports: @@ -818,7 +818,7 @@ permissions: contents: read issues: read features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false imports: @@ -892,7 +892,7 @@ permissions: tools: github: false features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false imports: diff --git a/pkg/workflow/prompts_test.go b/pkg/workflow/prompts_test.go index 50d9ff1dac9..ea22f9e36cb 100644 --- a/pkg/workflow/prompts_test.go +++ b/pkg/workflow/prompts_test.go @@ -860,7 +860,7 @@ tools: playwright: mode: cli features: - dangerously-disable-sandbox-agent: "unit test verifying prompt gating logic" + dangerously-disable-sandbox-agent: true sandbox: agent: false strict: false diff --git a/pkg/workflow/pull_request_target_validation_test.go b/pkg/workflow/pull_request_target_validation_test.go index cf9260f314b..b37b5232146 100644 --- a/pkg/workflow/pull_request_target_validation_test.go +++ b/pkg/workflow/pull_request_target_validation_test.go @@ -28,7 +28,7 @@ func TestPullRequestTargetValidation(t *testing.T) { // ---- non-strict mode ---- { - name: "pull_request_target with checkout disabled - non-strict - sandbox warning only", + name: "pull_request_target with checkout disabled - non-strict - no warning", frontmatter: `--- strict: false on: @@ -37,7 +37,7 @@ on: tools: github: false features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false checkout: false @@ -48,8 +48,8 @@ Test workflow content.`, filename: "prt-checkout-false-non-strict.md", strictMode: false, expectError: false, - expectWarning: true, - warningCount: 1, // sandbox.agent: false + expectWarning: false, + warningCount: 0, }, { name: "pull_request_target with no checkout key - non-strict - insecure checkout warning", @@ -61,7 +61,7 @@ on: tools: github: false features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false --- @@ -72,7 +72,7 @@ Test workflow content.`, strictMode: false, expectError: false, expectWarning: true, - warningCount: 2, // sandbox.agent: false warning + insecure-checkout warning (non-strict mode) + warningCount: 1, // insecure-checkout warning (non-strict mode) }, { name: "pull_request_target with trusted checkout - non-strict - no warnings no error", @@ -108,7 +108,7 @@ on: tools: github: false features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false --- @@ -118,8 +118,8 @@ Test workflow content.`, filename: "pr-non-strict.md", strictMode: false, expectError: false, - expectWarning: true, - warningCount: 1, // sandbox.agent: false only + expectWarning: false, + warningCount: 0, }, { name: "push trigger - non-strict - no diagnostic", @@ -131,7 +131,7 @@ on: tools: github: false features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false --- @@ -141,8 +141,8 @@ Test workflow content.`, filename: "push-non-strict.md", strictMode: false, expectError: false, - expectWarning: true, - warningCount: 1, // sandbox.agent: false only + expectWarning: false, + warningCount: 0, }, // ---- strict mode ---- diff --git a/pkg/workflow/sandbox.go b/pkg/workflow/sandbox.go index 7ea9a245112..01b806972ef 100644 --- a/pkg/workflow/sandbox.go +++ b/pkg/workflow/sandbox.go @@ -88,7 +88,6 @@ type AgentSandboxConfig struct { Runtime AgentRuntime `yaml:"runtime,omitempty"` // Sandbox runtime profile for the agent container (see sandbox_runtime_profile.go) AllowHostPorts []int `yaml:"-"` // Additional host TCP ports the agent may connect to (docker-sudo-iptables only). Disabled bool `yaml:"-"` // True when agent is explicitly set to false (disables firewall). This is a runtime flag, not serialized to YAML. - DisableReason string `yaml:"-"` // Operator-authored justification from dangerously-disable-sandbox-agent feature; available for diagnostics and audit logging. Config *SandboxRuntimeConfig `yaml:"config,omitempty"` // Custom SRT config (optional) Command string `yaml:"command,omitempty"` // Custom command to replace AWF or SRT installation Args []string `yaml:"args,omitempty"` // Additional arguments to append to the command diff --git a/pkg/workflow/sandbox_agent_disabled_test.go b/pkg/workflow/sandbox_agent_disabled_test.go index f26ebce00f2..fc8b042fd75 100644 --- a/pkg/workflow/sandbox_agent_disabled_test.go +++ b/pkg/workflow/sandbox_agent_disabled_test.go @@ -75,7 +75,7 @@ func TestSandboxAgentFalse(t *testing.T) { markdown := `--- engine: copilot features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false strict: false @@ -131,6 +131,8 @@ Test workflow with agent sandbox disabled. markdown := `--- engine: copilot +features: + dangerously-disable-sandbox-agent: true sandbox: agent: false strict: true @@ -154,13 +156,13 @@ Test workflow with agent sandbox disabled in strict mode. require.ErrorContains(t, err, "sandbox.agent: false") }) - t.Run("sandbox.agent: false shows warning at compile time", func(t *testing.T) { + t.Run("sandbox.agent: false does not show a deprecation warning", func(t *testing.T) { workflowsDir := t.TempDir() markdown := `--- engine: copilot features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false strict: false @@ -184,9 +186,9 @@ Test workflow. err = compiler.CompileWorkflow(workflowPath) require.NoError(t, err) - // Should have incremented warning count + // Supported non-strict usage should not increment the warning count finalWarnings := compiler.GetWarningCount() - assert.Greater(t, finalWarnings, initialWarnings, "Expected warning to be emitted for sandbox.agent: false") + assert.Equal(t, initialWarnings, finalWarnings, "Expected no warning for supported sandbox.agent: false") }) } @@ -197,7 +199,7 @@ func TestSandboxAgentFalseWithTools(t *testing.T) { markdown := `--- engine: copilot features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false strict: false diff --git a/pkg/workflow/sandbox_agent_false_test.go b/pkg/workflow/sandbox_agent_false_test.go index e525925934f..8dfac340b17 100644 --- a/pkg/workflow/sandbox_agent_false_test.go +++ b/pkg/workflow/sandbox_agent_false_test.go @@ -21,7 +21,7 @@ network: - defaults - github.com features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false strict: false @@ -195,7 +195,7 @@ Test workflow to verify sandbox.agent: false is rejected without the feature fla } }) - t.Run("sandbox.agent: false with short justification is rejected", func(t *testing.T) { + t.Run("sandbox.agent: false with feature false is rejected", func(t *testing.T) { workflowsDir := t.TempDir() markdown := `--- @@ -205,17 +205,17 @@ network: - defaults - github.com features: - dangerously-disable-sandbox-agent: "too short" + dangerously-disable-sandbox-agent: false sandbox: agent: false strict: false on: workflow_dispatch --- -Test workflow to verify sandbox.agent: false is rejected when feature justification is too short. +Test workflow to verify sandbox.agent: false is rejected when the feature is false. ` - workflowPath := filepath.Join(workflowsDir, "test-agent-false-short-flag.md") + workflowPath := filepath.Join(workflowsDir, "test-agent-false-disabled-flag.md") err := os.WriteFile(workflowPath, []byte(markdown), 0644) if err != nil { t.Fatalf("Failed to write workflow file: %v", err) @@ -224,14 +224,14 @@ Test workflow to verify sandbox.agent: false is rejected when feature justificat compiler := NewCompiler() err = compiler.CompileWorkflow(workflowPath) if err == nil { - t.Fatal("Expected compilation to fail when justification is too short, but got nil error") + t.Fatal("Expected compilation to fail when the feature is false, but got nil error") } - if !strings.Contains(err.Error(), "at least 20 characters") { - t.Fatalf("Expected error to mention minimum length, got: %v", err) + if !strings.Contains(err.Error(), "dangerously-disable-sandbox-agent") { + t.Fatalf("Expected error to reference 'dangerously-disable-sandbox-agent', got: %v", err) } }) - t.Run("sandbox.agent: false with expression justification is rejected", func(t *testing.T) { + t.Run("sandbox.agent: false with string feature is rejected", func(t *testing.T) { workflowsDir := t.TempDir() markdown := `--- @@ -241,17 +241,17 @@ network: - defaults - github.com features: - dangerously-disable-sandbox-agent: "${{ inputs.reason }}" + dangerously-disable-sandbox-agent: "true" sandbox: agent: false strict: false on: workflow_dispatch --- -Test workflow to verify sandbox.agent: false is rejected when feature uses an expression. +Test workflow to verify sandbox.agent: false is rejected when the feature is not a boolean. ` - workflowPath := filepath.Join(workflowsDir, "test-agent-false-expression-flag.md") + workflowPath := filepath.Join(workflowsDir, "test-agent-false-string-flag.md") err := os.WriteFile(workflowPath, []byte(markdown), 0644) if err != nil { t.Fatalf("Failed to write workflow file: %v", err) @@ -260,10 +260,10 @@ Test workflow to verify sandbox.agent: false is rejected when feature uses an ex compiler := NewCompiler() err = compiler.CompileWorkflow(workflowPath) if err == nil { - t.Fatal("Expected compilation to fail when justification uses an expression, but got nil error") + t.Fatal("Expected compilation to fail when the feature is a string, but got nil error") } - if !strings.Contains(err.Error(), "expressions") { - t.Fatalf("Expected error to mention expressions are not allowed, got: %v", err) + if !strings.Contains(err.Error(), "dangerously-disable-sandbox-agent") { + t.Fatalf("Expected error to reference 'dangerously-disable-sandbox-agent', got: %v", err) } }) } diff --git a/pkg/workflow/sandbox_test.go b/pkg/workflow/sandbox_test.go index fa20c0fcdd2..701739bb2c6 100644 --- a/pkg/workflow/sandbox_test.go +++ b/pkg/workflow/sandbox_test.go @@ -64,10 +64,10 @@ func TestValidateSandboxConfig(t *testing.T) { }, }, { - name: "sandbox.agent false with valid justification", + name: "sandbox.agent false with feature enabled", data: &WorkflowData{ Features: map[string]any{ - "dangerously-disable-sandbox-agent": "controlled environment with no internet access", + "dangerously-disable-sandbox-agent": true, }, SandboxConfig: &SandboxConfig{ Agent: &AgentSandboxConfig{ @@ -77,7 +77,7 @@ func TestValidateSandboxConfig(t *testing.T) { }, }, { - name: "sandbox.agent false without justification", + name: "sandbox.agent false without feature", data: &WorkflowData{ SandboxConfig: &SandboxConfig{ Agent: &AgentSandboxConfig{ @@ -89,10 +89,10 @@ func TestValidateSandboxConfig(t *testing.T) { errorMsg: "dangerously-disable-sandbox-agent", }, { - name: "sandbox.agent false with short justification", + name: "sandbox.agent false with feature disabled", data: &WorkflowData{ Features: map[string]any{ - "dangerously-disable-sandbox-agent": "too short", + "dangerously-disable-sandbox-agent": false, }, SandboxConfig: &SandboxConfig{ Agent: &AgentSandboxConfig{ @@ -101,13 +101,13 @@ func TestValidateSandboxConfig(t *testing.T) { }, }, expectError: true, - errorMsg: "at least 20 characters", + errorMsg: "dangerously-disable-sandbox-agent", }, { - name: "sandbox.agent false with expression justification", + name: "sandbox.agent false with legacy feature", data: &WorkflowData{ Features: map[string]any{ - "dangerously-disable-sandbox-agent": "${{ inputs.reason }}", + "dangerously-disable-sandbox": true, }, SandboxConfig: &SandboxConfig{ Agent: &AgentSandboxConfig{ @@ -116,7 +116,22 @@ func TestValidateSandboxConfig(t *testing.T) { }, }, expectError: true, - errorMsg: "expressions", + errorMsg: "dangerously-disable-sandbox-agent", + }, + { + name: "sandbox.agent false with non-boolean feature", + data: &WorkflowData{ + Features: map[string]any{ + "dangerously-disable-sandbox-agent": "true", + }, + SandboxConfig: &SandboxConfig{ + Agent: &AgentSandboxConfig{ + Disabled: true, + }, + }, + }, + expectError: true, + errorMsg: "dangerously-disable-sandbox-agent", }, } diff --git a/pkg/workflow/sandbox_validation.go b/pkg/workflow/sandbox_validation.go index 2703d4119c0..a2b4def3ba1 100644 --- a/pkg/workflow/sandbox_validation.go +++ b/pkg/workflow/sandbox_validation.go @@ -11,7 +11,6 @@ package workflow import ( - "errors" "fmt" "regexp" "strconv" @@ -24,8 +23,6 @@ import ( var sandboxValidationLog = logger.New("workflow:sandbox_validation") -const minSandboxDisableJustificationLength = 20 - var githubActionsExpressionPattern = regexp.MustCompile(`\$\{\{[\s\S]*\}\}`) var mcpGatewayEnvNamePattern = regexp.MustCompile(`^[A-Z_][A-Z0-9_]*$`) @@ -77,7 +74,7 @@ func validateMountsSyntax(mounts []string) error { // validateSandboxConfig validates the sandbox configuration // Returns an error if the configuration is invalid -func validateSandboxConfig(workflowData *WorkflowData) error { +func validateSandboxConfig(workflowData *WorkflowData) error { //nolint:largefunc // Existing sandbox validation remains centralized. if workflowData == nil { return nil } @@ -89,22 +86,20 @@ func validateSandboxConfig(workflowData *WorkflowData) error { sandboxConfig := workflowData.SandboxConfig // Check if sandbox.agent: false was specified - // This requires the "dangerously-disable-sandbox-agent" feature to include a - // justification string. Without a valid justification, disabling the sandbox - // is a validation error. + // This requires the "dangerously-disable-sandbox-agent" feature to be explicitly enabled. if sandboxConfig.Agent != nil && sandboxConfig.Agent.Disabled { - justification, err := getSandboxDisableJustification(workflowData) - if err != nil { - flag := string(constants.DangerouslyDisableSandboxAgentFeatureFlag) + flag := string(constants.DangerouslyDisableSandboxAgentFeatureFlag) + value, found := getFeatureValueCaseInsensitive(workflowData.Features, flag) + enabled, isBoolean := value.(bool) + if !found || !isBoolean || !enabled { return NewValidationError( "sandbox.agent", "false", - fmt.Sprintf("disabling the agent sandbox removes a trust boundary: '%s' must be a literal justification string (%d+ chars, no expressions): %v", flag, minSandboxDisableJustificationLength, err), - fmt.Sprintf("Add the feature value to your workflow frontmatter:\n\nfeatures:\n %s: \"controlled environment with no internet access\"\nsandbox:\n agent: false\n\nSee: %s", flag, constants.DocsSandboxURL), + fmt.Sprintf("disabling the agent sandbox removes a trust boundary and requires 'features.%s: true'", flag), + fmt.Sprintf("Explicitly enable the dangerous sandbox opt-out:\n\nfeatures:\n %s: true\nsandbox:\n agent: false\n\nSee: %s", flag, constants.DocsSandboxURL), ) } - sandboxConfig.Agent.DisableReason = justification - sandboxValidationLog.Printf("sandbox.agent: false permitted by %s justification: %q", constants.DangerouslyDisableSandboxAgentFeatureFlag, justification) + sandboxValidationLog.Printf("sandbox.agent: false permitted by features.%s: true", flag) if workflowData.EngineConfig != nil && workflowData.EngineConfig.ID == string(constants.CodexEngine) && @@ -418,44 +413,6 @@ func validateSandboxRuntimeProfile(workflowData *WorkflowData, agentConfig *Agen return nil } -func getSandboxDisableJustification(workflowData *WorkflowData) (string, error) { - if workflowData == nil || workflowData.Features == nil { - return "", errors.New("features block is missing dangerously-disable-sandbox-agent configuration. Expected a non-empty string justification under features when sandbox.agent is false. Example:\nfeatures:\n dangerously-disable-sandbox-agent: \"Temporary migration while hardening container profile\"") - } - - flagName := string(constants.DangerouslyDisableSandboxAgentFeatureFlag) - value, found := getFeatureValueCaseInsensitive(workflowData.Features, flagName) - if !found { - return "", errors.New("dangerously-disable-sandbox-agent key is missing from features. Expected a non-empty string justification under features when sandbox.agent is false. Example:\nfeatures:\n dangerously-disable-sandbox-agent: \"Temporary migration while hardening container profile\"") - } - - justification, ok := value.(string) - if !ok { - return "", NewValidationError( - "features.dangerously-disable-sandbox-agent", - fmt.Sprintf("%T", value), - "dangerously-disable-sandbox-agent feature value must be a string justification", - "Provide a string justification.\n\nExample:\nfeatures:\n dangerously-disable-sandbox-agent: \"Temporary migration while hardening container profile\"", - ) - } - - trimmed := strings.TrimSpace(justification) - if len(trimmed) < minSandboxDisableJustificationLength { - return "", NewValidationError( - "features.dangerously-disable-sandbox-agent", - trimmed, - fmt.Sprintf("dangerously-disable-sandbox-agent justification is shorter than %d characters", minSandboxDisableJustificationLength), - fmt.Sprintf("Provide a descriptive justification string with at least %d characters.\n\nExample:\nfeatures:\n dangerously-disable-sandbox-agent: \"Temporary migration while hardening container profile\"", minSandboxDisableJustificationLength), - ) - } - - if githubActionsExpressionPattern.MatchString(trimmed) { - return "", errors.New("dangerously-disable-sandbox-agent justification uses a GitHub Actions expression. Expected a literal explanatory string, not an expression. Example:\nfeatures:\n dangerously-disable-sandbox-agent: \"Temporary migration while hardening container profile\"") - } - - return trimmed, nil -} - func getFeatureValueCaseInsensitive(features map[string]any, flagName string) (any, bool) { if value, exists := features[flagName]; exists { return value, true diff --git a/pkg/workflow/sandbox_validation_test.go b/pkg/workflow/sandbox_validation_test.go index dea5f891a3c..f69c0eb2cf8 100644 --- a/pkg/workflow/sandbox_validation_test.go +++ b/pkg/workflow/sandbox_validation_test.go @@ -79,101 +79,6 @@ func TestSandboxTypeCaseSensitivity(t *testing.T) { } } -// TestGetSandboxDisableJustification tests the full justification validation logic, -// including all the rejection cases required by the acceptance criteria: -// - boolean true fails (no longer a legacy shorthand) -// - expressions fail -// - too-short strings fail -// - whitespace-padded strings fail -// - a 20+ character literal reason passes -func TestGetSandboxDisableJustification(t *testing.T) { - makeData := func(value any) *WorkflowData { - return &WorkflowData{ - Features: map[string]any{ - "dangerously-disable-sandbox-agent": value, - }, - } - } - - t.Run("boolean true is rejected", func(t *testing.T) { - _, err := getSandboxDisableJustification(makeData(true)) - require.Error(t, err) - require.ErrorContains(t, err, "string", "should explain that a string is required") - }) - - t.Run("boolean false is rejected", func(t *testing.T) { - _, err := getSandboxDisableJustification(makeData(false)) - require.Error(t, err) - require.ErrorContains(t, err, "string", "should explain that a string is required") - }) - - t.Run("empty string is rejected", func(t *testing.T) { - _, err := getSandboxDisableJustification(makeData("")) - require.Error(t, err) - require.ErrorContains(t, err, "20", "should mention minimum length") - }) - - t.Run("short string is rejected", func(t *testing.T) { - _, err := getSandboxDisableJustification(makeData("too short")) - require.Error(t, err) - require.ErrorContains(t, err, "20", "should mention minimum length") - }) - - t.Run("whitespace-padded short string is rejected", func(t *testing.T) { - // 22 spaces - long enough on paper but collapses to empty after TrimSpace - _, err := getSandboxDisableJustification(makeData(" ")) - require.Error(t, err) - require.ErrorContains(t, err, "20", "should mention minimum length") - }) - - t.Run("whitespace-padded string where trimmed is below minimum is rejected", func(t *testing.T) { - // "short" padded with whitespace to 25 total chars still fails (trimmed is 5) - _, err := getSandboxDisableJustification(makeData(" short ")) - require.Error(t, err) - require.ErrorContains(t, err, "20", "should mention minimum length") - }) - - t.Run("GitHub Actions expression is rejected", func(t *testing.T) { - _, err := getSandboxDisableJustification(makeData("${{ inputs.reason }}")) - require.Error(t, err) - require.ErrorContains(t, err, "expression") - }) - - t.Run("longer expression with surrounding text is rejected", func(t *testing.T) { - _, err := getSandboxDisableJustification(makeData("reason: ${{ inputs.reason }} end")) - require.Error(t, err) - require.ErrorContains(t, err, "expression") - }) - - t.Run("20+ character literal reason passes", func(t *testing.T) { - justification, err := getSandboxDisableJustification(makeData("controlled environment with no internet access")) - require.NoError(t, err) - assert.Equal(t, "controlled environment with no internet access", justification) - }) - - t.Run("justification is trimmed before return", func(t *testing.T) { - justification, err := getSandboxDisableJustification(makeData(" controlled environment with no internet access ")) - require.NoError(t, err) - assert.Equal(t, "controlled environment with no internet access", justification) - }) - - t.Run("feature missing returns error", func(t *testing.T) { - _, err := getSandboxDisableJustification(&WorkflowData{Features: map[string]any{}}) - require.Error(t, err) - require.ErrorContains(t, err, "missing") - }) - - t.Run("nil features returns error", func(t *testing.T) { - _, err := getSandboxDisableJustification(&WorkflowData{}) - require.Error(t, err) - }) - - t.Run("nil workflow data returns error", func(t *testing.T) { - _, err := getSandboxDisableJustification(nil) - require.Error(t, err) - }) -} - // TestValidateSandboxConfigTrustBoundaryMessage tests that the compiler diagnostic // says the sandbox removal is a trust boundary change, not just a validator check. func TestValidateSandboxConfigTrustBoundaryMessage(t *testing.T) { @@ -245,31 +150,12 @@ func TestValidateSandboxConfigMCPEnvironmentVariableNames(t *testing.T) { }) } -// TestValidateSandboxConfigStoresJustification tests that a valid justification is -// stored in AgentSandboxConfig.DisableReason for downstream diagnostics and audit. -func TestValidateSandboxConfigStoresJustification(t *testing.T) { - const reason = "controlled environment with no internet access" - workflowData := &WorkflowData{ - Features: map[string]any{ - "dangerously-disable-sandbox-agent": reason, - }, - SandboxConfig: &SandboxConfig{ - Agent: &AgentSandboxConfig{Disabled: true}, - }, - } - - err := validateSandboxConfig(workflowData) - require.NoError(t, err, "valid justification should pass validation") - assert.Equal(t, reason, workflowData.SandboxConfig.Agent.DisableReason, - "justification must be stored on AgentSandboxConfig for audit/logging") -} - func TestValidateSandboxConfigRejectsCodexCopilotWithoutAgentSandbox(t *testing.T) { workflowData := &WorkflowData{ Model: "copilot/auto", EngineConfig: &EngineConfig{ID: "codex"}, Features: map[string]any{ - "dangerously-disable-sandbox-agent": "controlled environment with no internet access", + "dangerously-disable-sandbox-agent": true, }, SandboxConfig: &SandboxConfig{ Agent: &AgentSandboxConfig{Disabled: true}, diff --git a/pkg/workflow/workflow_run_validation_test.go b/pkg/workflow/workflow_run_validation_test.go index aa001374483..ed067ca8f60 100644 --- a/pkg/workflow/workflow_run_validation_test.go +++ b/pkg/workflow/workflow_run_validation_test.go @@ -37,7 +37,7 @@ on: tools: github: false features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false --- @@ -48,7 +48,7 @@ Test workflow content.`, strictMode: false, expectError: false, expectWarning: true, - warningCount: 2, // 1 for workflow_run without branches + 1 for sandbox.agent: false + warningCount: 1, // workflow_run without branches }, { name: "workflow_run without branches - strict mode - should error", @@ -84,7 +84,7 @@ on: tools: github: false features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false --- @@ -95,7 +95,7 @@ Test workflow content.`, strictMode: false, expectError: false, expectWarning: false, - warningCount: 1, // 1 for sandbox.agent: false + warningCount: 0, }, { name: "workflow_run with branches - strict mode - should pass", @@ -242,7 +242,7 @@ on: tools: github: false features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false --- @@ -253,7 +253,7 @@ Test workflow content.`, strictMode: false, expectError: false, expectWarning: false, - warningCount: 1, // 1 for sandbox.agent: false + warningCount: 0, }, { name: "mixed triggers with workflow_run without branches - should warn/error", @@ -268,7 +268,7 @@ on: tools: github: false features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false --- @@ -279,7 +279,7 @@ Test workflow content.`, strictMode: false, expectError: false, expectWarning: true, - warningCount: 2, // 1 for workflow_run without branches + 1 for sandbox.agent: false + warningCount: 1, // workflow_run without branches }, { name: "workflow_run with empty branches array - should warn/error", @@ -293,7 +293,7 @@ on: tools: github: false features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false --- @@ -304,7 +304,7 @@ Test workflow content.`, strictMode: false, expectError: false, expectWarning: false, - warningCount: 1, // 1 for sandbox.agent: false + warningCount: 0, }, } @@ -365,7 +365,7 @@ on: push tools: github: false features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false --- @@ -374,7 +374,7 @@ sandbox: Test workflow content.`, filename: "no-on-field.md", expectError: false, - warningCount: 1, // 1 for sandbox.agent: false + warningCount: 0, }, { name: "multiple workflow_run configs - first without branches - should warn", @@ -387,7 +387,7 @@ on: tools: github: false features: - dangerously-disable-sandbox-agent: "controlled environment with no internet access" + dangerously-disable-sandbox-agent: true sandbox: agent: false --- @@ -396,7 +396,7 @@ sandbox: Test workflow content.`, filename: "multiple-workflows.md", expectError: false, - warningCount: 2, // 1 for workflow_run without branches + 1 for sandbox.agent: false + warningCount: 1, // workflow_run without branches }, }