-
Notifications
You must be signed in to change notification settings - Fork 295
fix: propagate environment: frontmatter field to all safe-output jobs
#20384
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -501,6 +501,7 @@ type SafeOutputsConfig struct { | |||||
| Steps []any `yaml:"steps,omitempty"` // User-provided steps injected after setup/checkout and before safe-output code | ||||||
| IDToken *string `yaml:"id-token,omitempty"` // Override id-token permission: "write" to force-add, "none" to disable auto-detection | ||||||
| ConcurrencyGroup string `yaml:"concurrency-group,omitempty"` // Concurrency group for the safe-outputs job (cancel-in-progress is always false) | ||||||
| Environment string `yaml:"environment,omitempty"` // Override the GitHub deployment environment for the safe-outputs job (defaults to the top-level environment: field) | ||||||
|
||||||
| Environment string `yaml:"environment,omitempty"` // Override the GitHub deployment environment for the safe-outputs job (defaults to the top-level environment: field) | |
| Environment string `yaml:"environment,omitempty"` // Override the GitHub deployment environment for all safe-output jobs (defaults to the top-level environment: field) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,10 +4,13 @@ package workflow | |
|
|
||
| import ( | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/github/gh-aw/pkg/constants" | ||
| "github.com/github/gh-aw/pkg/stringutil" | ||
| "github.com/github/gh-aw/pkg/testutil" | ||
| ) | ||
|
|
||
| func TestEnvironmentSupport(t *testing.T) { | ||
|
|
@@ -220,3 +223,135 @@ This is a test.` | |
| t.Errorf("Expected properly indented environment section, but got:\n%s", yamlContent) | ||
| } | ||
| } | ||
|
|
||
| // TestSafeOutputsEnvironmentPropagation verifies that the top-level environment: field is | ||
| // propagated to the safe_outputs job so that environment-scoped secrets are accessible. | ||
| func TestSafeOutputsEnvironmentPropagation(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| frontmatter string | ||
| expectEnvInSafe bool | ||
|
Comment on lines
+227
to
+233
|
||
| expectedEnvValue string | ||
| }{ | ||
| { | ||
| name: "top-level environment propagated to safe_outputs job", | ||
| frontmatter: `--- | ||
| on: | ||
| issues: | ||
| types: [opened] | ||
| environment: production | ||
| safe-outputs: | ||
| add-comment: {} | ||
| --- | ||
|
|
||
| # Test Workflow | ||
|
|
||
| This is a test.`, | ||
| expectEnvInSafe: true, | ||
| expectedEnvValue: "environment: production", | ||
| }, | ||
| { | ||
| name: "safe-outputs environment overrides top-level environment", | ||
| frontmatter: `--- | ||
| on: | ||
| issues: | ||
| types: [opened] | ||
| environment: production | ||
| safe-outputs: | ||
| environment: staging | ||
| add-comment: {} | ||
| --- | ||
|
|
||
| # Test Workflow | ||
|
|
||
| This is a test.`, | ||
| expectEnvInSafe: true, | ||
| expectedEnvValue: "environment: staging", | ||
| }, | ||
| { | ||
| name: "no environment means safe_outputs has no environment", | ||
| frontmatter: `--- | ||
| on: | ||
| issues: | ||
| types: [opened] | ||
| safe-outputs: | ||
| add-comment: {} | ||
| --- | ||
|
|
||
| # Test Workflow | ||
|
|
||
| This is a test.`, | ||
| expectEnvInSafe: false, | ||
| expectedEnvValue: "", | ||
| }, | ||
| { | ||
| name: "safe-outputs-only environment when no top-level environment", | ||
| frontmatter: `--- | ||
| on: | ||
| issues: | ||
| types: [opened] | ||
| safe-outputs: | ||
| environment: dev | ||
| add-comment: {} | ||
| --- | ||
|
|
||
| # Test Workflow | ||
|
|
||
| This is a test.`, | ||
| expectEnvInSafe: true, | ||
| expectedEnvValue: "environment: dev", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| tmpDir := testutil.TempDir(t, "safe-outputs-env-test") | ||
| workflowFile := filepath.Join(tmpDir, "test.md") | ||
| if err := os.WriteFile(workflowFile, []byte(tt.frontmatter), 0644); err != nil { | ||
| t.Fatalf("Failed to write workflow file: %v", err) | ||
| } | ||
|
|
||
| compiler := NewCompiler() | ||
| if err := compiler.CompileWorkflow(workflowFile); err != nil { | ||
| t.Fatalf("CompileWorkflow() error: %v", err) | ||
| } | ||
|
|
||
| lockFile := stringutil.MarkdownToLockFile(workflowFile) | ||
| lockContent, err := os.ReadFile(lockFile) | ||
| if err != nil { | ||
| t.Fatalf("Failed to read lock file: %v", err) | ||
| } | ||
| yamlStr := string(lockContent) | ||
|
|
||
| // Find the safe_outputs job section | ||
| safeOutputsIdx := strings.Index(yamlStr, " safe_outputs:\n") | ||
| if safeOutputsIdx == -1 { | ||
| t.Fatal("safe_outputs job not found in generated YAML") | ||
| } | ||
|
|
||
| // Find the next top-level job after safe_outputs (indented by 2 spaces) | ||
| nextJobIdx := len(yamlStr) | ||
| lines := strings.Split(yamlStr[safeOutputsIdx+len(" safe_outputs:\n"):], "\n") | ||
| offset := safeOutputsIdx + len(" safe_outputs:\n") | ||
| for _, line := range lines { | ||
| if line != "" && !strings.HasPrefix(line, " ") && !strings.HasPrefix(line, " #") { | ||
| nextJobIdx = offset | ||
| break | ||
| } | ||
| offset += len(line) + 1 | ||
| } | ||
|
|
||
| safeOutputsSection := yamlStr[safeOutputsIdx:nextJobIdx] | ||
|
|
||
| if tt.expectEnvInSafe { | ||
| if !strings.Contains(safeOutputsSection, tt.expectedEnvValue) { | ||
| t.Errorf("Expected safe_outputs job to contain %q, but got:\n%s", tt.expectedEnvValue, safeOutputsSection) | ||
| } | ||
| } else { | ||
| if strings.Contains(safeOutputsSection, "environment:") { | ||
| t.Errorf("Expected safe_outputs job to have no environment field, but found one in:\n%s", safeOutputsSection) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -506,6 +506,12 @@ func (c *Compiler) extractSafeOutputsConfig(frontmatter map[string]any) *SafeOut | |||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // Handle environment configuration (override for safe-outputs job; falls back to top-level environment) | ||||||||||||||||||
| config.Environment = c.extractTopLevelYAMLSection(outputMap, "environment") | ||||||||||||||||||
| if config.Environment != "" { | ||||||||||||||||||
| safeOutputsConfigLog.Printf("Configured environment override for safe-outputs job: %s", config.Environment) | ||||||||||||||||||
|
Comment on lines
+509
to
+512
|
||||||||||||||||||
| // Handle environment configuration (override for safe-outputs job; falls back to top-level environment) | |
| config.Environment = c.extractTopLevelYAMLSection(outputMap, "environment") | |
| if config.Environment != "" { | |
| safeOutputsConfigLog.Printf("Configured environment override for safe-outputs job: %s", config.Environment) | |
| // Handle environment configuration (override for all safe-outputs jobs; falls back to top-level environment) | |
| config.Environment = c.extractTopLevelYAMLSection(outputMap, "environment") | |
| if config.Environment != "" { | |
| safeOutputsConfigLog.Printf("Configured environment override for all safe-outputs jobs: %s", config.Environment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The schema description says the safe-outputs.environment override (and top-level propagation) applies to the “safe-outputs job”, but the implementation applies it to all safe-output jobs (including conclusion, pre_activation, upload_assets, and custom safe-jobs). Please update the description to match the implemented behavior so users aren’t surprised by environment protections/secret scoping across those jobs.