security: reject disable-xpia-prompt in strict mode at compile time#28057
security: reject disable-xpia-prompt in strict mode at compile time#28057
Conversation
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/efedb545-e8e5-4f6a-8028-a53bfe718ef5 Co-authored-by: szabta89 <1330202+szabta89@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/efedb545-e8e5-4f6a-8028-a53bfe718ef5 Co-authored-by: szabta89 <1330202+szabta89@users.noreply.github.com>
|
@szabta89 wouldn't this break testing? Proper fix would be to make it an error in strict mode. |
You are right. Either only add this in strict mode, or, even better, just close this as a non-issue with a comment. I have updated the pentester workflows now to consider closed security findings when making a decision about potentially new findings, so, hopefully, we will not produce repeat findings. |
|
@copilot mark this feature as error in strict mode and ignore bash state |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/d4a467c6-862f-4a0c-86a3-73556f15e25b Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🧪 Test Quality Sentinel ReportTest Quality Score: 80/100✅ Excellent test quality
Test Classification Details
Analysis
No mock libraries, no testify mocks, no missing build tags. Both files correctly carry
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds strict-mode compile-time validation to prevent workflows from disabling the XPIA (Cross-Prompt Injection Attack) defense via features.disable-xpia-prompt: true.
Changes:
- Added
validateStrictDisableXPIA()strict-mode validator to rejectdisable-xpia-promptwhen enabled. - Wired the new validator into the strict mode validation orchestrator.
- Added/updated unit tests to cover strict-mode behavior and to confirm non-strict mode remains permissive.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/strict_mode_permissions_validation.go | Adds the new strict-mode validation function that rejects disabling XPIA. |
| pkg/workflow/strict_mode_validation.go | Calls the new strict-mode validation as part of the strict mode validation sequence. |
| pkg/workflow/strict_mode_validation_test.go | Adds unit tests for the new strict-mode validation behavior. |
| pkg/workflow/features_validation_test.go | Adds a non-strict-mode test asserting the feature flag remains allowed. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/workflow/strict_mode_validation_test.go:775
- The new tests cover validateStrictDisableXPIA() directly, but they don’t assert that validateStrictMode() (the orchestrator) rejects workflows with
disable-xpia-promptenabled. Adding at least one case to TestValidateStrictMode for this flag (and a mixed-case key variant, given case-insensitive feature handling) would prevent regressions in the wiring and bypasses.
// TestValidateStrictDisableXPIA tests the validateStrictDisableXPIA function
func TestValidateStrictDisableXPIA(t *testing.T) {
tests := []struct {
name string
frontmatter map[string]any
expectError bool
errorMsg string
}{
{
name: "no features field - allowed",
frontmatter: map[string]any{"on": "push"},
expectError: false,
},
{
name: "features without disable-xpia-prompt - allowed",
frontmatter: map[string]any{
"on": "push",
"features": map[string]any{
"action-tag": "v0",
},
},
expectError: false,
},
{
name: "disable-xpia-prompt: false - allowed",
frontmatter: map[string]any{
"on": "push",
"features": map[string]any{
"disable-xpia-prompt": false,
},
},
expectError: false,
},
{
name: "disable-xpia-prompt: true - rejected",
frontmatter: map[string]any{
"on": "push",
"features": map[string]any{
"disable-xpia-prompt": true,
},
},
expectError: true,
errorMsg: "strict mode: 'disable-xpia-prompt: true' is not allowed",
},
{
name: "disable-xpia-prompt: true with bash tool - rejected (bash state irrelevant)",
frontmatter: map[string]any{
"on": "push",
"features": map[string]any{
"disable-xpia-prompt": true,
},
"tools": map[string]any{
"bash": true,
},
},
expectError: true,
errorMsg: "strict mode: 'disable-xpia-prompt: true' is not allowed",
},
{
name: "disable-xpia-prompt: true without bash tool - still rejected",
frontmatter: map[string]any{
"on": "push",
"features": map[string]any{
"disable-xpia-prompt": true,
},
},
expectError: true,
errorMsg: "strict mode: 'disable-xpia-prompt: true' is not allowed",
},
{
name: "disable-xpia-prompt as non-empty string - rejected",
frontmatter: map[string]any{
"on": "push",
"features": map[string]any{
"disable-xpia-prompt": "yes",
},
},
expectError: true,
errorMsg: "strict mode: 'disable-xpia-prompt: true' is not allowed",
},
{
name: "disable-xpia-prompt as empty string - allowed",
frontmatter: map[string]any{
"on": "push",
"features": map[string]any{
"disable-xpia-prompt": "",
},
},
expectError: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
compiler := NewCompiler()
err := compiler.validateStrictDisableXPIA(tt.frontmatter)
if tt.expectError && err == nil {
t.Error("Expected validation to fail but it succeeded")
} else if !tt.expectError && err != nil {
t.Errorf("Expected validation to succeed but it failed: %v", err)
} else if tt.expectError && err != nil && tt.errorMsg != "" {
if !strings.Contains(err.Error(), tt.errorMsg) {
t.Errorf("Expected error containing '%s', got '%s'", tt.errorMsg, err.Error())
}
}
})
}
}
- Files reviewed: 4/4 changed files
- Comments generated: 3
| featuresMap, ok := featuresValue.(map[string]any) | ||
| if !ok { | ||
| return nil | ||
| } | ||
| flagVal, exists := featuresMap["disable-xpia-prompt"] | ||
| if !exists { | ||
| return nil | ||
| } | ||
| // Only reject when the flag is explicitly enabled (true / non-empty string) | ||
| enabled := false | ||
| switch v := flagVal.(type) { | ||
| case bool: | ||
| enabled = v | ||
| case string: | ||
| enabled = v != "" | ||
| } |
There was a problem hiding this comment.
validateStrictDisableXPIA() only checks for the exact key disable-xpia-prompt. Feature lookup elsewhere is case-insensitive (see pkg/workflow/features.go:50-56), and the schema allows arbitrary feature keys, so features: { Disable-XPIA-Prompt: true } would still disable the XPIA prompt but bypass this strict-mode check. Consider normalizing keys (e.g., strings.ToLower on feature names) or reusing the existing case-insensitive feature parsing logic so strict mode can’t be bypassed via casing.
| // 6. Refuse disable-xpia-prompt feature flag | ||
| if err := c.validateStrictDisableXPIA(frontmatter); err != nil { | ||
| if returnErr := collector.Add(err); returnErr != nil { | ||
| return returnErr // Fail-fast mode | ||
| } | ||
| } |
There was a problem hiding this comment.
This strict-mode check is wired into validateStrictMode(), but validateStrictMode() runs before imports are processed/merged (compiler_orchestrator_engine.go calls validateStrictMode before ProcessImportsFromFrontmatterWithSource, and features are merged later in compiler_orchestrator_workflow.go:178-185). As a result, disable-xpia-prompt coming from an import won’t be rejected in strict mode. To enforce the stated security guarantee, run this validation after imports/features are merged (or validate against the merged feature set).
| { | ||
| name: "disable-xpia-prompt with bash tool - allowed in non-strict mode", | ||
| data: &WorkflowData{ | ||
| Features: map[string]any{ | ||
| "disable-xpia-prompt": true, | ||
| }, | ||
| ParsedTools: NewTools(map[string]any{ | ||
| "bash": true, | ||
| }), | ||
| }, | ||
| expectError: false, |
There was a problem hiding this comment.
This test name implies bash tool configuration affects validation, but validateFeatures() currently only validates features.action-tag and does not inspect ParsedTools. Consider renaming the test (or adding a comment) to avoid implying a dependency that doesn’t exist.
disable-xpia-prompt: truein a strict-mode workflow removes the primary XPIA (Cross-Prompt Injection Attack) defense. In strict mode this combination is now rejected at compile time regardless of bash tool state.Changes
strict_mode_permissions_validation.go):validateStrictDisableXPIA()rejects any workflow that setsdisable-xpia-prompt: truewhen compiled in strict mode. Bash tool configuration is not considered — the flag alone is sufficient to fail validation.strict_mode_validation.go): Added as step 6 invalidateStrictMode().disable-xpia-prompt: trueis still permitted outside strict mode, avoiding breakage for test and development workflows.strict_mode_validation_test.go): covers all relevant cases — flag enabled/disabled, with/without bash tool, string values.→ strict mode compile error: