Skip to content

fix: correct engine.env secrets warning — excluded from sandbox, not leaked #43302

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 6 commits into from
Jul 4, 2026
+192 −4
Merged

fix: correct engine.env secrets warning — excluded from sandbox, not leaked #43302

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
197cacf
Initial plan
Copilot Jul 4, 2026
08ef4ac
fix: correct engine.env secrets warning to reflect auto-exclusion fro…
Copilot Jul 4, 2026
9fd3f26
test: add TestComputeAWFExcludeEnvVarNames to verify engine.env auto-…
Copilot Jul 4, 2026
6c11bbc
docs(adr): add draft ADR-43302 for section-aware secrets validation m…
github-actions[bot] Jul 4, 2026
46e1ea5
Merge branch 'main' into copilot/fix-compiler-warning-for-secrets
github-actions[bot] Jul 4, 2026
5e54cea
fix: qualify engine.env exclusion messages with AWF v0.25.3+ version …
Copilot Jul 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 98 additions & 0 deletions pkg/workflow/awf_helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1118,6 +1118,104 @@ func TestAWFSupportsExcludeEnv(t *testing.T) {
}
}

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] TestComputeAWFExcludeEnvVarNames covers engine.env vars well, but has no test for agent.env vars (the function also scans agentConfig.Env for ${{ secrets. values). A regression there would be silent.

💡 Suggestion

Add one case:

{
    name: "agent.env secret var is auto-excluded",
    workflowData: &WorkflowData{
        AgentConfig: &AgentConfig{
            Env: map[string]string{
                "AGENT_TOKEN": "${{ secrets.AGENT_SECRET }}",
            },
        },
    },
    coreSecretVarNames: []string{},
    want: []string{"AGENT_TOKEN"},
},

This ensures the exclusion logic is symmetric for both engine.env and agent.env.

@copilot please address this.

// TestComputeAWFExcludeEnvVarNames verifies that engine.env vars whose values contain
// ${{ secrets.* }} are automatically included in the --exclude-env list, and that
// non-secret engine.env vars and plain-value core secrets are handled correctly.
func TestComputeAWFExcludeEnvVarNames(t *testing.T) {
tests := []struct {
name string
workflowData *WorkflowData
coreSecretVarNames []string
want []string
notWant []string
}{
{
name: "engine.env secret var is auto-excluded",
workflowData: &WorkflowData{
EngineConfig: &EngineConfig{
Env: map[string]string{
"GOOGLE_API_KEY": "${{ secrets.SOME_KEY }}",
},
},
},
coreSecretVarNames: []string{},
want: []string{"GOOGLE_API_KEY"},
},
{
name: "engine.env non-secret var is not excluded",
workflowData: &WorkflowData{
EngineConfig: &EngineConfig{
Env: map[string]string{
"DEBUG": "true",
"LOG_LEVEL": "info",
},
},
},
coreSecretVarNames: []string{},
want: []string{},

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty want slice produces zero positive assertions: When want: []string{}, the assertion loop body never executes — nothing is verified about the returned slice being empty. If ComputeAWFExcludeEnvVarNames accidentally returned non-empty results for plain-value vars, this test would still pass.

💡 Suggested fix

Add an explicit emptiness check after the loop:

if len(tt.want) == 0 {
    assert.Empty(t, got, "expected exclude list to be empty")
}

Or fold it into the loop by asserting assert.Empty whenever the want list is nil/empty. Either way, the test should fail when the production code returns unexpected entries for non-secret env vars.

notWant: []string{"DEBUG", "LOG_LEVEL"},
},
{
name: "engine.env mixes secret and non-secret vars: only secrets excluded",
workflowData: &WorkflowData{
EngineConfig: &EngineConfig{
Env: map[string]string{
"GOOGLE_API_KEY": "${{ secrets.SOME_KEY }}",
"LOG_LEVEL": "debug",
},
},
},
coreSecretVarNames: []string{},
want: []string{"GOOGLE_API_KEY"},
notWant: []string{"LOG_LEVEL"},
},
{
name: "engine.env secret combined with core secret vars",
workflowData: &WorkflowData{
EngineConfig: &EngineConfig{
Env: map[string]string{
"CUSTOM_API_KEY": "${{ secrets.CUSTOM_KEY }}",
},
},
},
coreSecretVarNames: []string{"GEMINI_API_KEY"},
want: []string{"GEMINI_API_KEY", "CUSTOM_API_KEY"},
},
{
name: "engine.env secret embedded in a larger string is excluded",
workflowData: &WorkflowData{
EngineConfig: &EngineConfig{
Env: map[string]string{
"AUTH_HEADER": "Bearer ${{ secrets.TOKEN }}",

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The AUTH_HEADER test case uses "Bearer ${{ secrets.TOKEN }}" — but the actual literal in the file is "****** secrets.TOKEN }}" (missing the opening ${{), which still triggers the strings.Contains(envValue, "${{ secrets.") check in the implementation. The test name says "embedded in a larger string" but is actually testing detection without the full ${{ prefix — that is a different edge case than what the name implies.

💡 Suggestion

Rename and/or add a proper embedded-string variant:

{
    name: "engine.env secret bare prefix (missing opening) is NOT excluded",
    workflowData: &WorkflowData{
        EngineConfig: &EngineConfig{
            Env: map[string]string{
                "AUTH_HEADER": "Bearer secrets.TOKEN }}",  // no ${{, should not match
            },
        },
    },
    coreSecretVarNames: []string{},
    want:    []string{},
    notWant: []string{"AUTH_HEADER"},
},
{
    name: "engine.env secret embedded in larger string is excluded",
    workflowData: &WorkflowData{
        EngineConfig: &EngineConfig{
            Env: map[string]string{
                "AUTH_HEADER": "Bearer ${{ secrets.TOKEN }}",  // embedded, should match
            },
        },
    },
    coreSecretVarNames: []string{},
    want: []string{"AUTH_HEADER"},
},

@copilot please address this.

},
},
},
coreSecretVarNames: []string{},
want: []string{"AUTH_HEADER"},
},
{
name: "nil engine config produces no exclusions beyond core secrets",
workflowData: &WorkflowData{
EngineConfig: nil,
},
coreSecretVarNames: []string{"COPILOT_GITHUB_TOKEN"},
want: []string{"COPILOT_GITHUB_TOKEN"},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := ComputeAWFExcludeEnvVarNames(tt.workflowData, tt.coreSecretVarNames)
for _, name := range tt.want {
assert.Contains(t, got, name, "expected %q in exclude list", name)
}
for _, name := range tt.notWant {
assert.NotContains(t, got, name, "expected %q to be absent from exclude list", name)
}
})
}
}

// TestBuildAWFArgsCliProxy tests that BuildAWFArgs correctly injects --difc-proxy-host
// and --difc-proxy-ca-cert based on the cli-proxy feature flag.
func TestBuildAWFArgsCliProxy(t *testing.T) {
Expand Down
6 changes: 3 additions & 3 deletions pkg/workflow/env_secrets_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,7 @@ func TestValidateEngineEnvSecrets(t *testing.T) {
},
strictMode: true,
expectError: true,
errorMsg: "strict mode: secrets detected in 'engine.env' section will be leaked to the agent container. Found: ${{ secrets.API_KEY }}",
errorMsg: "strict mode: secrets detected in 'engine.env' section are auto-excluded from the agent sandbox via awf --exclude-env and are not accessible to the agent. Found: ${{ secrets.API_KEY }}",
},
{
name: "engine.env with multiple secrets in strict mode fails",
Expand All @@ -553,7 +553,7 @@ func TestValidateEngineEnvSecrets(t *testing.T) {
},
strictMode: true,
expectError: true,
errorMsg: "strict mode: secrets detected in 'engine.env' section will be leaked to the agent container",
errorMsg: "strict mode: secrets detected in 'engine.env' section are auto-excluded from the agent sandbox",
},
{
name: "engine.env with secret embedded in string in strict mode fails",
Expand All @@ -568,7 +568,7 @@ func TestValidateEngineEnvSecrets(t *testing.T) {
},
strictMode: true,
expectError: true,
errorMsg: "strict mode: secrets detected in 'engine.env' section will be leaked to the agent container. Found: ${{ secrets.TOKEN }}",
errorMsg: "strict mode: secrets detected in 'engine.env' section are auto-excluded from the agent sandbox via awf --exclude-env and are not accessible to the agent. Found: ${{ secrets.TOKEN }}",
},
{
name: "engine.env with secret in non-strict mode emits warning (no error)",

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The non-strict engine.env test only asserts expectError: false — the updated warning message text is never verified, so regressions to the new message would go undetected.

💡 Suggestion

Capture stderr in the test and assert the corrected message. The existing TestValidateEnvSecretsNonStrictMode uses the same pattern (no assertion on message) with a comment that stderr cannot be captured. Consider introducing a captureWarnings helper or redirect os.Stderr temporarily to verify the new message text:

var buf bytes.Buffer
old := os.Stderr
r, w, _ := os.Pipe()
os.Stderr = w
err := compiler.validateEnvSecrets(tt.frontmatter)
w.Close()
os.Stderr = old
io.Copy(&buf, r)
assert.NoError(t, err)
assert.Contains(t, buf.String(), "will be excluded from the agent sandbox via awf --exclude-env")
assert.NotContains(t, buf.String(), "will be leaked")

@copilot please address this.

Expand Down
14 changes: 13 additions & 1 deletion pkg/workflow/strict_mode_env_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,11 +132,23 @@ func (c *Compiler) validateEnvSecretsSection(config map[string]any, sectionName

// In strict mode, this is an error
if c.strictMode {
if sectionName == "engine.env" {

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/codebase-design] The sectionName == "engine.env" string literal is repeated in two independent branches (lines 135 and 145). If engine.env is ever renamed in the call-site (e.g. to "engine.environment"), both branches need updating but the compiler won’t catch it.

💡 Suggestion

Extract the constant or pass a boolean:

const engineEnvSection = "engine.env"

// or — simpler — let the caller pass isEngineEnv bool so the function signature
// makes the branching intent explicit and can be tested with a boolean instead
// of a magic string.

This also keeps the function focused on validation rather than section naming conventions.

@copilot please address this.

// engine.env secrets are auto-excluded from the agent sandbox via awf --exclude-env,
// so they are not leaked, but strict mode still requires engine-specific configuration.
return fmt.Errorf("strict mode: secrets detected in 'engine.env' section are auto-excluded from the agent sandbox via awf --exclude-env and are not accessible to the agent. Found: %s. Use engine-specific secret configuration instead. See: https://github.github.com/gh-aw/reference/engines/", strings.Join(secretRefs, ", "))

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] The new message guarantees exclusion via --exclude-env, but BuildAWFArgs silently skips that flag for workflows pinned to AWF < v0.25.3 — so the claim is still factually wrong for old-pinned workflows.

💡 Suggested fix

Either qualify the message with a version condition, or have validateEnvSecretsSection receive the AWF version and vary the message:

// Option A: always mention the version requirement
return fmt.Errorf("strict mode: secrets detected in \u0027engine.env\u0027 section " +
    "are auto-excluded from the agent sandbox via awf --exclude-env (AWF v0.25.3+) "+
    "and are not accessible to the agent. Found: %s. ...", ...)

Without this, a workflow pinned to v0.24.x would get the reassuring new message while --exclude-env never runs, leaving the secrets visible to the agent.

See: pkg/workflow/awf_helpers.go — awfSupportsExcludeEnv / AWFExcludeEnvMinVersion.

@copilot please address this.

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contradictory strict-mode error message: The error tells users their secret is safe ("auto-excluded... not accessible to the agent") but then returns an error blocking the build. Users will be confused about why they need to change anything if the secret is already protected.

💡 Suggested fix

The message should explain why strict mode still rejects this pattern despite runtime exclusion — e.g., the raw secret value still appears in the workflow file, is accessible to the host process before AWF is invoked, and strict mode enforces engine-specific configuration for defence in depth:

return fmt.Errorf(
    "strict mode: secrets detected in 'engine.env' section. "+
    "While these are excluded from the agent sandbox at runtime (via awf --exclude-env), "+
    "the raw secret value is still exposed in the workflow source and to the host runner process. "+
    "Strict mode requires engine-specific secret configuration to avoid any exposure. "+
    "Found: %s. See: https://github.github.com/gh-aw/reference/engines/",
    strings.Join(secretRefs, ", "))

This removes the contradiction and gives the user a clear reason to act.

}
return fmt.Errorf("strict mode: secrets detected in '%s' section will be leaked to the agent container. Found: %s. Use engine-specific secret configuration instead. See: https://github.github.com/gh-aw/reference/engines/", sectionName, strings.Join(secretRefs, ", "))
}

// In non-strict mode, emit a warning
warningMsg := fmt.Sprintf("Warning: secrets detected in '%s' section will be leaked to the agent container. Found: %s. Consider using engine-specific secret configuration instead.", sectionName, strings.Join(secretRefs, ", "))
var warningMsg string
if sectionName == "engine.env" {
// engine.env secrets are auto-excluded from the agent sandbox via awf --exclude-env,
// so the warning should reflect that they are excluded, not leaked.
warningMsg = fmt.Sprintf("Warning: secrets detected in 'engine.env' section will be excluded from the agent sandbox via awf --exclude-env; the agent process itself will not see these values directly. Found: %s. Consider using engine-specific secret configuration instead.", strings.Join(secretRefs, ", "))

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] The non-strict engine.env message is inconsistent with the strict-mode one: non-strict says "will be excluded" (future tense / conditional) while strict says "are auto-excluded" (present, definitive). Both share the same version-gate risk, but the different phrasing may confuse users who see one message in development and the other in CI.

💡 Suggestion

Aligning on the same phrasing and the same version caveat in both branches makes the UX clearer:

// strict
"are auto-excluded from the agent sandbox via awf --exclude-env (requires AWF v0.25.3+)"
// non-strict warning
"will be excluded from the agent sandbox via awf --exclude-env (requires AWF v0.25.3+)"

@copilot please address this.

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

False guarantee for workflows pinning old AWF: The warning unconditionally tells users their secret "will be excluded from the agent sandbox via awf --exclude-env", but BuildAWFArgs only emits --exclude-env when awfSupportsExcludeEnv(firewallConfig) returns true (requires AWF ≥ v0.25.3). For workflows that pin an older AWF version, --env-all still injects the secret into the container and this claim is factually wrong.

💡 Suggested fix

validateEnvSecretsSection has no access to firewallConfig, so it cannot verify the version at warning time. Two options:

Option A — hedge on version in the message:

warningMsg = fmt.Sprintf(
    "Warning: secrets detected in 'engine.env' section. On AWF >= v0.25.3 these "+
    "are auto-excluded via --exclude-env; on older pinned AWF versions the secret "+
    "still reaches the container. Found: %s. Consider using engine-specific "+
    "secret configuration instead.",
    strings.Join(secretRefs, ", "))

Option B — thread firewallConfig (or a excludeEnvSupported bool) into validateEnvSecretsSection so the message can be dynamically accurate.

Without a fix, a user on AWF v0.25.0 who sees this warning will believe their secret is sandboxed when it is not.

} else {
warningMsg = fmt.Sprintf("Warning: secrets detected in '%s' section will be leaked to the agent container. Found: %s. Consider using engine-specific secret configuration instead.", sectionName, strings.Join(secretRefs, ", "))
}
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(warningMsg))
c.IncrementWarningCount()

Expand Down
Loading