Repository navigation
feat: avoid secrets:inherit as default, infer minimal set automatically #29455
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 2 commits
a742ae2
10ed5d3
21c9e93
e090373
d7ab9f0
785413f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -910,3 +910,154 @@ safe-outputs: | |
| assert.NotContains(t, yaml, "fromJSON(needs.safe_outputs.outputs.call_workflow_payload).payload", | ||
| "payload itself must not be duplicated as a fromJSON entry") | ||
| } | ||
|
|
||
| // workerLockYMLWithSecrets creates a worker lock file that declares on.workflow_call.secrets, | ||
| // simulating a worker compiled with the new secrets-inference feature. | ||
| func workerLockYMLWithSecrets(secrets []string) string { | ||
| var secretsYAML strings.Builder | ||
| for _, s := range secrets { | ||
| secretsYAML.WriteString(" " + s + ":\n required: false\n") | ||
| } | ||
| return `name: Worker | ||
| "on": | ||
| workflow_call: | ||
| inputs: | ||
| payload: | ||
| type: string | ||
| required: false | ||
| secrets: | ||
| ` + secretsYAML.String() + `jobs: | ||
| work: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - run: echo "Working" | ||
| ` | ||
| } | ||
|
|
||
| // createWorkerWithSecrets writes a worker lock file that includes on.workflow_call.secrets. | ||
| func createWorkerWithSecrets(t *testing.T, workflowsDir, name string, secrets []string) { | ||
| t.Helper() | ||
| content := workerLockYMLWithSecrets(secrets) | ||
| err := os.WriteFile(filepath.Join(workflowsDir, name+".lock.yml"), []byte(content), 0644) | ||
| require.NoError(t, err, "Failed to write worker %s with secrets", name) | ||
| } | ||
|
|
||
| // TestCallWorkflowCompile_ExplicitSecretsWhenWorkerDeclaresThem verifies that when the worker | ||
| // declares on.workflow_call.secrets, the orchestrator emits explicit secret mappings instead | ||
| // of secrets: inherit. | ||
| func TestCallWorkflowCompile_ExplicitSecretsWhenWorkerDeclaresThem(t *testing.T) { | ||
| tmpDir := testutil.TempDir(t, "call-workflow-explicit-secrets") | ||
| workflowsDir := filepath.Join(tmpDir, ".github", "workflows") | ||
| require.NoError(t, os.MkdirAll(workflowsDir, 0755)) | ||
|
|
||
| createWorkerWithSecrets(t, workflowsDir, "worker-a", []string{"MY_TOKEN", "ANOTHER_SECRET"}) | ||
|
|
||
| gatewayMD := `--- | ||
| on: workflow_dispatch | ||
| engine: copilot | ||
| permissions: | ||
| contents: read | ||
| safe-outputs: | ||
| call-workflow: | ||
| - worker-a | ||
| --- | ||
|
|
||
| # Gateway with explicit secrets | ||
| ` | ||
| yaml := compileAndReadLock(t, filepath.Join(workflowsDir, "gateway.md"), gatewayMD) | ||
|
|
||
| assert.Contains(t, yaml, "call-worker-a:", "Should contain call-worker-a job") | ||
| assert.NotContains(t, yaml, "secrets: inherit", "Should NOT use secrets: inherit when worker declares secrets") | ||
| assert.Contains(t, yaml, "MY_TOKEN: ${{ secrets.MY_TOKEN }}", "Should map MY_TOKEN explicitly") | ||
| assert.Contains(t, yaml, "ANOTHER_SECRET: ${{ secrets.ANOTHER_SECRET }}", "Should map ANOTHER_SECRET explicitly") | ||
| } | ||
|
|
||
| // TestCallWorkflowCompile_FallsBackToInheritWhenNoSecretsInWorker verifies that when the worker | ||
| // does NOT declare on.workflow_call.secrets, the orchestrator falls back to secrets: inherit. | ||
| func TestCallWorkflowCompile_FallsBackToInheritWhenNoSecretsInWorker(t *testing.T) { | ||
| tmpDir := testutil.TempDir(t, "call-workflow-inherit-fallback") | ||
| workflowsDir := filepath.Join(tmpDir, ".github", "workflows") | ||
| require.NoError(t, os.MkdirAll(workflowsDir, 0755)) | ||
|
|
||
| // Worker with NO secrets declarations (legacy format) | ||
| createWorker(t, workflowsDir, "legacy-worker", "") | ||
|
|
||
| gatewayMD := `--- | ||
| on: workflow_dispatch | ||
| engine: copilot | ||
| permissions: | ||
| contents: read | ||
| safe-outputs: | ||
| call-workflow: | ||
| - legacy-worker | ||
| --- | ||
|
|
||
| # Gateway with legacy worker | ||
| ` | ||
| yaml := compileAndReadLock(t, filepath.Join(workflowsDir, "gateway.md"), gatewayMD) | ||
|
|
||
| assert.Contains(t, yaml, "call-legacy-worker:", "Should contain call-legacy-worker job") | ||
| assert.Contains(t, yaml, "secrets: inherit", "Should fall back to secrets: inherit for legacy worker") | ||
| } | ||
|
|
||
| // TestCallWorkflowCompile_WorkerGetsSecretsDeclarationsOnCompilation verifies that compiling | ||
| // a reusable workflow with workflow_call trigger injects on.workflow_call.secrets declarations | ||
| // for the secrets it uses. | ||
| func TestCallWorkflowCompile_WorkerGetsSecretsDeclarationsOnCompilation(t *testing.T) { | ||
| tmpDir := testutil.TempDir(t, "call-workflow-worker-secrets-injection") | ||
| workflowsDir := filepath.Join(tmpDir, ".github", "workflows") | ||
| require.NoError(t, os.MkdirAll(workflowsDir, 0755)) | ||
|
|
||
| // Worker markdown with workflow_call trigger that uses a custom secret | ||
| workerMD := `--- | ||
| name: My Worker | ||
| on: | ||
| workflow_call: | ||
| inputs: | ||
| payload: | ||
| type: string | ||
| required: false | ||
| engine: copilot | ||
| permissions: | ||
| contents: read | ||
| safe-outputs: | ||
| add-comment: | ||
| max: 1 | ||
| --- | ||
|
|
||
| # Worker that uses a custom token | ||
|
|
||
| Use MY_CUSTOM_TOKEN from secrets.MY_CUSTOM_TOKEN when making API calls. | ||
| ` | ||
| workerFile := filepath.Join(workflowsDir, "my-worker.md") | ||
| err := os.WriteFile(workerFile, []byte(workerMD), 0644) | ||
| require.NoError(t, err) | ||
|
|
||
| compiler := NewCompiler(WithVersion("1.0.0")) | ||
| err = compiler.CompileWorkflow(workerFile) | ||
| require.NoError(t, err, "Worker compilation should succeed") | ||
|
|
||
| lockFile := workerFile[:len(workerFile)-3] + ".lock.yml" | ||
| content, err := os.ReadFile(lockFile) | ||
| require.NoError(t, err, "Lock file should exist after compilation") | ||
| lockContent := string(content) | ||
|
|
||
| // The compiled worker should declare its secrets in on.workflow_call.secrets | ||
| assert.Contains(t, lockContent, "workflow_call:", "Should have workflow_call trigger") | ||
| assert.Contains(t, lockContent, "COPILOT_GITHUB_TOKEN", "Should declare COPILOT_GITHUB_TOKEN") | ||
| assert.Contains(t, lockContent, "GH_AW_GITHUB_TOKEN", "Should declare GH_AW_GITHUB_TOKEN") | ||
|
|
||
| // GITHUB_TOKEN should NOT be in the on.workflow_call.secrets section. | ||
| // It may appear in job steps, so we check the on: section specifically. | ||
| // Verify the secrets block exists and GITHUB_TOKEN is absent from it by | ||
| // finding the secrets section between "workflow_call:" and "workflow_dispatch:". | ||
| workflowCallStart := strings.Index(lockContent, "workflow_call:") | ||
| workflowDispatchStart := strings.Index(lockContent, "workflow_dispatch:") | ||
| if workflowCallStart >= 0 && workflowDispatchStart > workflowCallStart { | ||
| onWorkflowCallSection := lockContent[workflowCallStart:workflowDispatchStart] | ||
| assert.NotContains(t, onWorkflowCallSection, "GITHUB_TOKEN", | ||
| "GITHUB_TOKEN should not be declared in on.workflow_call.secrets (auto-provided by GitHub Actions)") | ||
| assert.Contains(t, onWorkflowCallSection, "secrets:", | ||
| "Should have secrets section under workflow_call") | ||
| } | ||
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| package workflow | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
| "sort" | ||
|
|
||
| "github.com/github/gh-aw/pkg/logger" | ||
| "github.com/goccy/go-yaml" | ||
| ) | ||
|
|
||
| var callWorkflowSecretsLog = logger.New("workflow:call_workflow_secrets") | ||
|
|
||
| // extractCallWorkflowSecrets returns the list of secret names declared in the worker | ||
| // workflow's on.workflow_call.secrets section. This is used by orchestrator workflows | ||
| // to map secrets explicitly instead of using secrets: inherit. | ||
| // | ||
| // Priority: .lock.yml > .yml (same as extractCallWorkflowPermissions). | ||
| // .md sources are not consulted since they have not yet been compiled and do not | ||
| // have an on.workflow_call.secrets section. | ||
| // | ||
| // Returns nil when no compiled workflow file is found or when no secrets are declared, | ||
| // which signals the caller to fall back to secrets: inherit for backward compatibility. | ||
| func extractCallWorkflowSecrets(workflowName, markdownPath string) ([]string, error) { | ||
| fileResult, err := findWorkflowFile(workflowName, markdownPath) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to find workflow file for '%s': %w", workflowName, err) | ||
| } | ||
|
|
||
| // Priority: .lock.yml > .yml | ||
| if fileResult.lockExists { | ||
| return extractSecretsFromWorkflowFile(fileResult.lockPath) | ||
| } | ||
|
|
||
| if fileResult.ymlExists { | ||
| return extractSecretsFromWorkflowFile(fileResult.ymlPath) | ||
| } | ||
|
|
||
| // No compiled file found — return nil so the caller falls back to secrets: inherit. | ||
| callWorkflowSecretsLog.Printf("No compiled workflow file found for '%s', falling back to secrets: inherit", workflowName) | ||
| return nil, nil | ||
| } | ||
|
|
||
| // extractSecretsFromWorkflowFile parses a .lock.yml or .yml workflow file and returns | ||
| // the secret names declared in its on.workflow_call.secrets section. | ||
| func extractSecretsFromWorkflowFile(filePath string) ([]string, error) { | ||
| cleanPath := filepath.Clean(filePath) | ||
| // filePath originates from findWorkflowFile(), which validates all paths via | ||
| // isPathWithinDir() to prevent directory traversal before returning them. | ||
| content, err := os.ReadFile(cleanPath) // #nosec G304 -- path pre-validated by findWorkflowFile() via isPathWithinDir() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read workflow file %s: %w", filePath, err) | ||
| } | ||
|
|
||
| var workflow map[string]any | ||
| if err := yaml.Unmarshal(content, &workflow); err != nil { | ||
| return nil, fmt.Errorf("failed to parse workflow file %s: %w", filePath, err) | ||
| } | ||
|
|
||
| secrets := extractWorkflowCallSecretsFromParsed(workflow) | ||
| callWorkflowSecretsLog.Printf("Extracted %d workflow_call secrets from %s", len(secrets), filePath) | ||
| return secrets, nil | ||
| } | ||
|
|
||
| // extractWorkflowCallSecretsFromParsed extracts the secret names declared in | ||
| // on.workflow_call.secrets from an already-parsed workflow map. | ||
| func extractWorkflowCallSecretsFromParsed(workflow map[string]any) []string { | ||
| onSection, ok := workflow["on"] | ||
| if !ok { | ||
| return nil | ||
| } | ||
| onMap, ok := onSection.(map[string]any) | ||
| if !ok { | ||
| return nil | ||
| } | ||
| workflowCallVal, ok := onMap["workflow_call"] | ||
| if !ok { | ||
| return nil | ||
| } | ||
| workflowCallMap, ok := workflowCallVal.(map[string]any) | ||
| if !ok { | ||
| return nil | ||
| } | ||
| secretsSection, ok := workflowCallMap["secrets"] | ||
| if !ok { | ||
| return nil | ||
| } | ||
| secretsMap, ok := secretsSection.(map[string]any) | ||
| if !ok { | ||
| return nil | ||
| } | ||
|
|
||
| names := make([]string, 0, len(secretsMap)) | ||
| for name := range secretsMap { | ||
| names = append(names, name) | ||
| } | ||
| sort.Strings(names) | ||
| return names | ||
| } |
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.
require.NoErrordoesn't format the message, so the%splaceholder will be printed literally. Userequire.NoErrorf(...)(or drop the formatting verb) so the worker name is included in the failure output.