Skip to content

Commit 88690ac

Browse files
authored
Harden gh aw add SHA pinning, enforce .md/.lock.yml frontmatter hash parity, require pinned action refs by default, and audit pin-resolution failures (#27419)
1 parent 5574a45 commit 88690ac

24 files changed

+689
-150
lines changed

actions/setup/md/stale_lock_file_failed.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,13 @@ The workflow run logs contain a verbose debug pass that shows exactly what was h
4545

4646
This makes it easy to spot accidental whitespace changes, encoding differences, or import path drift.
4747

48+
This mismatch can also happen on a **fresh install** (even without any manual edits) if `gh aw add <path>@<ref>` could not resolve the ref to an exact commit SHA during installation. In that case, rerun the add command with an exact SHA (or retry when API/rate-limit conditions recover), then recompile:
49+
50+
```bash
51+
gh aw add <path>@<exact-sha>
52+
gh aw compile
53+
```
54+
4855
</details>
4956

5057
<details>

cmd/gh-aw/main.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -276,6 +276,7 @@ Examples:
276276
forceOverwrite, _ := cmd.Flags().GetBool("force")
277277
refreshStopTime, _ := cmd.Flags().GetBool("refresh-stop-time")
278278
forceRefreshActionPins, _ := cmd.Flags().GetBool("force-refresh-action-pins")
279+
allowActionRefs, _ := cmd.Flags().GetBool("allow-action-refs")
279280
zizmor, _ := cmd.Flags().GetBool("zizmor")
280281
poutine, _ := cmd.Flags().GetBool("poutine")
281282
actionlint, _ := cmd.Flags().GetBool("actionlint")
@@ -335,6 +336,7 @@ Examples:
335336
ForceOverwrite: forceOverwrite,
336337
RefreshStopTime: refreshStopTime,
337338
ForceRefreshActionPins: forceRefreshActionPins,
339+
AllowActionRefs: allowActionRefs,
338340
Zizmor: zizmor,
339341
Poutine: poutine,
340342
Actionlint: actionlint,
@@ -684,6 +686,7 @@ Use "` + string(constants.CLIExtensionPrefix) + ` help all" to show help for all
684686
compileCmd.Flags().Bool("force", false, "Force overwrite of existing dependency files (e.g., dependabot.yml)")
685687
compileCmd.Flags().Bool("refresh-stop-time", false, "Force regeneration of stop-after times instead of preserving existing values from lock files")
686688
compileCmd.Flags().Bool("force-refresh-action-pins", false, "Force refresh of action pins by clearing the cache and resolving all action SHAs from GitHub API")
689+
compileCmd.Flags().Bool("allow-action-refs", false, "Allow unresolved action refs and emit warnings instead of failing compilation")
687690
compileCmd.Flags().Bool("zizmor", false, "Run zizmor security scanner on generated .lock.yml files")
688691
compileCmd.Flags().Bool("poutine", false, "Run poutine security scanner on generated .lock.yml files")
689692
compileCmd.Flags().Bool("actionlint", false, "Run actionlint linter on generated .lock.yml files")

pkg/actionpins/actionpins.go

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,23 @@ type SHAResolver interface {
4949
ResolveSHA(repo, version string) (string, error)
5050
}
5151

52+
// ResolutionErrorType classifies unresolved action-ref pinning outcomes for auditing.
53+
type ResolutionErrorType string
54+
55+
const (
56+
// ResolutionErrorTypeDynamicResolutionFailed indicates dynamic tag/ref -> SHA resolution failed.
57+
ResolutionErrorTypeDynamicResolutionFailed ResolutionErrorType = "dynamic_resolution_failed"
58+
// ResolutionErrorTypePinNotFound indicates no usable hardcoded pin was found for the ref.
59+
ResolutionErrorTypePinNotFound ResolutionErrorType = "pin_not_found"
60+
)
61+
62+
// ResolutionFailure captures an unresolved action-ref pinning event.
63+
type ResolutionFailure struct {
64+
Repo string
65+
Ref string
66+
ErrorType ResolutionErrorType
67+
}
68+
5269
// PinContext provides the runtime context needed for action pin resolution.
5370
// Callers construct one from their own state (e.g. WorkflowData fields).
5471
// The Warnings map is mutated in place to deduplicate warning output.
@@ -57,9 +74,16 @@ type PinContext struct {
5774
Resolver SHAResolver
5875
// StrictMode controls how resolution failures are handled.
5976
StrictMode bool
77+
// EnforcePinned requires unresolved refs to fail unless AllowActionRefs is true.
78+
EnforcePinned bool
79+
// AllowActionRefs lowers unresolved pinning failures to warnings.
80+
// When false, unresolved action refs return an error.
81+
AllowActionRefs bool
6082
// Warnings is a shared map for deduplicating warning messages.
6183
// Keys are cache keys in the form "repo@version".
6284
Warnings map[string]bool
85+
// RecordResolutionFailure receives unresolved pinning failures for auditing.
86+
RecordResolutionFailure func(f ResolutionFailure)
6387
}
6488

6589
var (
@@ -210,6 +234,17 @@ func findCompatiblePin(pins []ActionPin, version string) (ActionPin, bool) {
210234
return ActionPin{}, false
211235
}
212236

237+
func notifyResolutionFailure(ctx *PinContext, actionRepo, version string, errorType ResolutionErrorType) {
238+
if ctx == nil || ctx.RecordResolutionFailure == nil {
239+
return
240+
}
241+
ctx.RecordResolutionFailure(ResolutionFailure{
242+
Repo: actionRepo,
243+
Ref: version,
244+
ErrorType: errorType,
245+
})
246+
}
247+
213248
// ResolveActionPin returns the pinned action reference for a given action@version.
214249
// It consults ctx.Resolver first, then falls back to embedded pins.
215250
// If ctx is nil, only embedded pins are consulted.
@@ -301,6 +336,18 @@ func ResolveActionPin(actionRepo, version string, ctx *PinContext) (string, erro
301336
ctx.Warnings = make(map[string]bool)
302337
}
303338
cacheKey := FormatCacheKey(actionRepo, version)
339+
errorType := ResolutionErrorTypePinNotFound
340+
if ctx.Resolver != nil {
341+
errorType = ResolutionErrorTypeDynamicResolutionFailed
342+
}
343+
notifyResolutionFailure(ctx, actionRepo, version, errorType)
344+
if ctx.EnforcePinned && !ctx.AllowActionRefs {
345+
if ctx.Resolver != nil {
346+
return "", fmt.Errorf("unable to pin action %s@%s: resolution failed", actionRepo, version)
347+
}
348+
return "", fmt.Errorf("unable to pin action %s@%s", actionRepo, version)
349+
}
350+
304351
if !ctx.Warnings[cacheKey] {
305352
warningMsg := fmt.Sprintf("Unable to pin action %s@%s", actionRepo, version)
306353
if ctx.Resolver != nil {

pkg/cli/add_command.go

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ Note: For guided interactive setup, use the 'add-wizard' command instead.`,
118118
StopAfter: stopAfter,
119119
DisableSecurityScanner: disableSecurityScanner,
120120
}
121-
_, err := AddWorkflows(workflows, opts)
121+
_, err := AddWorkflows(cmd.Context(), workflows, opts)
122122
return err
123123
},
124124
}
@@ -172,9 +172,9 @@ Note: For guided interactive setup, use the 'add-wizard' command instead.`,
172172
// AddWorkflows adds one or more workflows from components to .github/workflows
173173
// with optional repository installation and PR creation.
174174
// Returns AddWorkflowsResult containing PR number (if created) and other metadata.
175-
func AddWorkflows(workflows []string, opts AddOptions) (*AddWorkflowsResult, error) {
175+
func AddWorkflows(ctx context.Context, workflows []string, opts AddOptions) (*AddWorkflowsResult, error) {
176176
// Resolve workflows first - fetches content directly from GitHub
177-
resolved, err := ResolveWorkflows(workflows, opts.Verbose)
177+
resolved, err := ResolveWorkflows(ctx, workflows, opts.Verbose)
178178
if err != nil {
179179
return nil, err
180180
}
@@ -494,12 +494,18 @@ func addWorkflowWithTracking(resolved *ResolvedWorkflow, tracker *FileTracker, o
494494
if err := os.WriteFile(destFile, []byte(content), 0600); err != nil {
495495
return fmt.Errorf("failed to write destination file '%s': %w", destFile, err)
496496
}
497+
// Read back the just-written file to ensure downstream processing (including
498+
// frontmatter hash computation) uses the exact bytes on disk and avoids parity drift.
499+
writtenContent, err := os.ReadFile(destFile)
500+
if err != nil {
501+
return fmt.Errorf("failed to read back destination file '%s': %w", destFile, err)
502+
}
497503

498504
// Show output
499505
if !opts.Quiet {
500506
fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Added workflow: "+destFile))
501507

502-
if description := ExtractWorkflowDescription(content); description != "" {
508+
if description := ExtractWorkflowDescription(string(writtenContent)); description != "" {
503509
fmt.Fprintln(os.Stderr, "")
504510
fmt.Fprintln(os.Stderr, console.FormatInfoMessage(description))
505511
fmt.Fprintln(os.Stderr, "")

pkg/cli/add_command_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
package cli
44

55
import (
6+
"context"
67
"testing"
78

89
"github.com/spf13/cobra"
@@ -99,7 +100,7 @@ func TestAddWorkflows(t *testing.T) {
99100
for _, tt := range tests {
100101
t.Run(tt.name, func(t *testing.T) {
101102
opts := AddOptions{}
102-
_, err := AddWorkflows(tt.workflows, opts)
103+
_, err := AddWorkflows(context.Background(), tt.workflows, opts)
103104

104105
if tt.expectError {
105106
require.Error(t, err, "Expected error for test case: %s", tt.name)

pkg/cli/add_current_repo_test.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
package cli
44

55
import (
6+
"context"
67
"os"
78
"os/exec"
89
"strings"
@@ -69,7 +70,7 @@ func TestAddWorkflowsFromCurrentRepository(t *testing.T) {
6970
ClearCurrentRepoSlugCache()
7071

7172
opts := AddOptions{}
72-
_, err := AddWorkflows(tt.workflowSpecs, opts)
73+
_, err := AddWorkflows(context.Background(), tt.workflowSpecs, opts)
7374

7475
if tt.expectError {
7576
if err == nil {
@@ -153,7 +154,7 @@ func TestAddWorkflowsFromCurrentRepositoryMultiple(t *testing.T) {
153154
ClearCurrentRepoSlugCache()
154155

155156
opts := AddOptions{}
156-
_, err := AddWorkflows(tt.workflowSpecs, opts)
157+
_, err := AddWorkflows(context.Background(), tt.workflowSpecs, opts)
157158

158159
if tt.expectError {
159160
if err == nil {
@@ -195,7 +196,7 @@ func TestAddWorkflowsFromCurrentRepositoryNotInGitRepo(t *testing.T) {
195196
// When not in a git repo, the check should be skipped (can't determine current repo)
196197
// The function should proceed and fail for other reasons (e.g., workflow not found)
197198
opts := AddOptions{}
198-
_, err = AddWorkflows([]string{"some-owner/some-repo/workflow"}, opts)
199+
_, err = AddWorkflows(context.Background(), []string{"some-owner/some-repo/workflow"}, opts)
199200

200201
// Should NOT get the "cannot add workflows from the current repository" error
201202
if err != nil && strings.Contains(err.Error(), "cannot add workflows from the current repository") {

pkg/cli/add_integration_test.go

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ import (
1010
"testing"
1111

1212
"github.com/github/gh-aw/pkg/fileutil"
13+
"github.com/github/gh-aw/pkg/parser"
14+
"github.com/github/gh-aw/pkg/workflow"
1315
"github.com/stretchr/testify/assert"
1416
"github.com/stretchr/testify/require"
1517
)
@@ -356,6 +358,47 @@ Please analyze the repository and provide a summary.
356358
assert.Contains(t, lockContentStr, "name: \"Test Local Workflow\"", "lock file should have workflow name")
357359
assert.Contains(t, lockContentStr, "workflow_dispatch", "lock file should have trigger")
358360
assert.Contains(t, lockContentStr, "jobs:", "lock file should have jobs section")
361+
362+
// Verify frontmatter hash parity between source markdown and lock metadata.
363+
computedHash, hashErr := parser.ComputeFrontmatterHashFromFile(destWorkflowFile, parser.NewImportCache(setup.tempDir))
364+
require.NoError(t, hashErr, "should compute frontmatter hash from added markdown file")
365+
metadata, _, metadataErr := workflow.ExtractMetadataFromLockFile(lockContentStr)
366+
require.NoError(t, metadataErr, "should extract lock metadata from compiled lock file")
367+
require.NotNil(t, metadata, "lock metadata should be present")
368+
assert.Equal(t, computedHash, metadata.FrontmatterHash,
369+
"lock file frontmatter hash should match the hash recomputed from markdown file bytes")
370+
}
371+
372+
// TestAddRemoteWorkflowFailsWhenSHAResolutionFails tests that add fails loudly when ref-to-SHA
373+
// resolution fails and does not write partial workflow artifacts.
374+
func TestAddRemoteWorkflowFailsWhenSHAResolutionFails(t *testing.T) {
375+
setup := setupAddIntegrationTest(t)
376+
defer setup.cleanup()
377+
378+
nonExistentWorkflowSpec := "github/gh-aw-does-not-exist/.github/workflows/not-real.md@main"
379+
380+
cmd := exec.Command(setup.binaryPath, "add", nonExistentWorkflowSpec)
381+
cmd.Dir = setup.tempDir
382+
output, err := cmd.CombinedOutput()
383+
outputStr := string(output)
384+
385+
require.Error(t, err, "add command should fail when SHA resolution fails")
386+
assert.Contains(t, outputStr, "failed to resolve 'main' to commit SHA",
387+
"error output should clearly explain SHA resolution failure")
388+
assert.Contains(t, outputStr, "Expected the GitHub API to return a commit SHA for the ref",
389+
"error output should explain expected behavior")
390+
assert.Contains(t, outputStr, "gh aw add github/gh-aw-does-not-exist/.github/workflows/not-real.md@<40-char-sha>",
391+
"error output should provide a concrete retry example")
392+
393+
workflowsDir := filepath.Join(setup.tempDir, ".github", "workflows")
394+
workflowFile := filepath.Join(workflowsDir, "not-real.md")
395+
lockFile := filepath.Join(workflowsDir, "not-real.lock.yml")
396+
397+
_, workflowErr := os.Stat(workflowFile)
398+
assert.True(t, os.IsNotExist(workflowErr), "workflow markdown file should not be written on SHA resolution failure")
399+
400+
_, lockErr := os.Stat(lockFile)
401+
assert.True(t, os.IsNotExist(lockErr), "lock file should not be written on SHA resolution failure")
359402
}
360403

361404
// TestAddWorkflowWithCustomName tests adding a workflow with a custom name

pkg/cli/add_interactive_orchestrator.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ func RunAddInteractive(ctx context.Context, workflowSpecs []string, verbose bool
167167
func (c *AddInteractiveConfig) resolveWorkflows() error {
168168
addInteractiveLog.Print("Resolving workflows early for description display")
169169

170-
resolved, err := ResolveWorkflows(c.WorkflowSpecs, c.Verbose)
170+
resolved, err := ResolveWorkflows(c.Ctx, c.WorkflowSpecs, c.Verbose)
171171
if err != nil {
172172
return fmt.Errorf("failed to resolve workflows: %w", err)
173173
}

pkg/cli/add_workflow_resolution.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package cli
22

33
import (
4+
"context"
45
"errors"
56
"fmt"
67
"os"
@@ -46,7 +47,7 @@ type ResolvedWorkflows struct {
4647
// ResolveWorkflows resolves workflow specifications by parsing specs and fetching workflow content.
4748
// For remote workflows, content is fetched directly from GitHub without cloning.
4849
// Wildcards are only supported for local workflows (not remote repositories).
49-
func ResolveWorkflows(workflows []string, verbose bool) (*ResolvedWorkflows, error) {
50+
func ResolveWorkflows(ctx context.Context, workflows []string, verbose bool) (*ResolvedWorkflows, error) {
5051
resolutionLog.Printf("Resolving workflows: count=%d", len(workflows))
5152

5253
if len(workflows) == 0 {
@@ -117,7 +118,7 @@ func ResolveWorkflows(workflows []string, verbose bool) (*ResolvedWorkflows, err
117118

118119
for _, spec := range parsedSpecs {
119120
// Fetch workflow content - FetchWorkflowFromSource handles both local and remote
120-
fetched, err := FetchWorkflowFromSource(spec, verbose)
121+
fetched, err := FetchWorkflowFromSourceWithContext(ctx, spec, verbose)
121122
if err != nil {
122123
return nil, fmt.Errorf("workflow '%s' not found: %w", spec.String(), err)
123124
}

pkg/cli/compile_compiler_setup.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,7 @@ func configureCompilerFlags(compiler *workflow.Compiler, config CompileConfig) {
134134

135135
// Set strict mode if specified
136136
compiler.SetStrictMode(config.Strict)
137+
compiler.SetAllowActionRefs(config.AllowActionRefs)
137138

138139
// Set trial mode if specified
139140
if config.TrialMode {

0 commit comments

Comments
 (0)