diff --git a/docs/adr/47907-post-update-sha-integrity-validation-actions-lock.md b/docs/adr/47907-post-update-sha-integrity-validation-actions-lock.md new file mode 100644 index 00000000000..13eafa48990 --- /dev/null +++ b/docs/adr/47907-post-update-sha-integrity-validation-actions-lock.md @@ -0,0 +1,44 @@ +# ADR-47907: Post-Update SHA Integrity Validation for actions-lock.json + +**Date**: 2026-07-25 +**Status**: Draft +**Deciders**: pelikhan, copilot-swe-agent + +--- + +### Context + +The `gh aw update` command resolves and pins GitHub Action commit SHAs and container image digests into `.github/aw/actions-lock.json`. Prior to this change, the file was written without any final structural check, meaning malformed entries (e.g., a truncated SHA, a mismatched map key, or an incorrectly formatted container digest) could be silently persisted and later used in production workflows. The only time a bad entry would be caught was at workflow execution time — far downstream from where the corruption was introduced. + +### Decision + +We will add a terminal validation phase at the end of `RunUpdateWorkflows` that re-reads and structurally verifies all entries in `actions-lock.json` after the update, recompile, and pin-refresh steps complete. Validation failures surface as a hard error (`update validation failed: ...`) rather than being silently ignored. This validation only runs when no earlier error occurred (`firstErr == nil`), so it does not mask upstream failures. + +### Alternatives Considered + +#### Alternative 1: Inline validation during update (validate-as-you-write) + +Each action or container entry could be validated at the point it is written into the cache, as part of the existing resolve-and-pin logic. This would catch errors closer to their origin and avoid re-reading the file. It was not chosen because it would require threading validation logic into multiple existing write paths (action resolution, container pinning, recompile step), significantly increasing the coupling and risk of regressions. A single terminal pass is simpler and keeps validation isolated from write logic. + +#### Alternative 2: No post-update validation (status quo) + +The existing approach relied on correctness guarantees from the upstream resolution APIs and the write path. Malformed entries would only surface at workflow-run time. This was rejected because supply-chain integrity requires catching bad pins before they are persisted; silent failures at the lock-write layer are a security risk in an action-pinning tool. + +### Consequences + +#### Positive +- Malformed or inconsistent action commit SHAs and container digests are caught immediately at update time, before being committed to the repository. +- The validator produces aggregated, human-readable diagnostics covering all invalid entries at once, rather than stopping at the first error. +- Validation logic is isolated in a dedicated file (`update_validation.go`) with injected resolver functions, making it straightforward to unit-test without real network calls. + +#### Negative +- The terminal validation phase makes additional GitHub API calls (one per action entry, one per container entry) to re-resolve SHAs. For repositories with many pinned actions this adds latency to every `gh aw update` invocation. +- Transient API failures during validation will cause the update command to return an error even when the lock file is structurally correct, potentially creating spurious failures in CI. + +#### Neutral +- The validation step is gated on `firstErr == nil`, so it does not run when the main update phase already encountered an error. This is consistent with the existing error-propagation strategy but means validation is skipped on partial updates. +- Verbose mode logs a confirmation message to stderr when validation succeeds, matching the existing log pattern used elsewhere in the update flow. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/cli/compile_orchestrator.go b/pkg/cli/compile_orchestrator.go index 411afcae16e..b6abecf87a9 100644 --- a/pkg/cli/compile_orchestrator.go +++ b/pkg/cli/compile_orchestrator.go @@ -46,6 +46,18 @@ func CompileWorkflows(ctx context.Context, config CompileConfig) ([]*workflow.Wo return nil, err } + // When --validate is set, run a pre-gate integrity check on actions-lock.json + // before compiling. This catches malformed or inconsistent pins early so the + // user sees the problem before any lock files are rewritten. + // Structural-only validation is used (no live API calls) to keep compile fast + // and avoid false positives from floating tags. + if config.Validate { + compileOrchestratorLog.Print("Running actions-lock.json SHA integrity pre-gate check") + if err := validateUpdateSHAEntriesStructural(ctx, "."); err != nil { + return nil, fmt.Errorf("actions-lock.json integrity check failed (pre-compile): %w", err) + } + } + // Initialize actionlint statistics if actionlint is enabled if config.Actionlint && !config.NoEmit { initActionlintStats() diff --git a/pkg/cli/update_command.go b/pkg/cli/update_command.go index ed2c9f3662d..a7922bbdc02 100644 --- a/pkg/cli/update_command.go +++ b/pkg/cli/update_command.go @@ -249,6 +249,16 @@ func RunUpdateWorkflows(ctx context.Context, opts UpdateWorkflowsOptions) error } } + if firstErr == nil { + updateLog.Print("Validating action and container SHAs in actions-lock.json") + if err := validateUpdateSHAEntries(ctx, "."); err != nil { + return fmt.Errorf("update validation failed: %w", err) + } + if opts.Verbose { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Validated action and container SHAs in actions-lock.json")) + } + } + updateLog.Printf("Update process complete: had_error=%v", firstErr != nil) return firstErr } diff --git a/pkg/cli/update_validation.go b/pkg/cli/update_validation.go new file mode 100644 index 00000000000..bc5f7d60830 --- /dev/null +++ b/pkg/cli/update_validation.go @@ -0,0 +1,180 @@ +package cli + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "regexp" + "sort" + "strings" + + "github.com/github/gh-aw/pkg/gitutil" + "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/parser" + "github.com/github/gh-aw/pkg/workflow" +) + +var updateValidationLog = logger.New("cli:update_validation") + +var sha256DigestPattern = regexp.MustCompile(`^sha256:[a-f0-9]{64}$`) + +// validationResolvers holds injectable resolver functions for testability. +// Using a struct instead of package-level vars allows callers to supply their own +// resolvers inline without touching globals, enabling t.Parallel() in tests. +type validationResolvers struct { + // verifyActionCommitExists checks that a commit SHA actually exists in the + // action repository. It must return parser.ErrVerificationSkipped (wrapped) + // for auth/network failures so callers can treat those as non-fatal. + verifyActionCommitExists func(ctx context.Context, repo, sha string) error + // resolveActionVersionToSHA resolves a version tag/ref to its commit SHA. + // Used to verify that the stored SHA matches the pinned version. + resolveActionVersionToSHA func(ctx context.Context, repo, ref string) (string, error) +} + +func defaultValidationResolvers() validationResolvers { + return validationResolvers{ + verifyActionCommitExists: func(ctx context.Context, repo, sha string) error { + baseRepo := gitutil.ExtractBaseRepo(repo) + owner, name, ok := strings.Cut(baseRepo, "/") + if !ok || owner == "" || name == "" { + return fmt.Errorf("invalid action repository %q", repo) + } + return parser.VerifyCommitExists(ctx, owner, name, sha, "") + }, + resolveActionVersionToSHA: func(ctx context.Context, repo, ref string) (string, error) { + baseRepo := gitutil.ExtractBaseRepo(repo) + owner, name, ok := strings.Cut(baseRepo, "/") + if !ok || owner == "" || name == "" { + return "", fmt.Errorf("invalid action repository %q", repo) + } + return parser.ResolveRefToSHAForHost(ctx, owner, name, ref, "") + }, + } +} + +// validateUpdateSHAEntries validates the structural and liveness integrity of +// .github/aw/actions-lock.json after a workflow update. Container pins are +// validated structurally only (format, key/image/pinned_image consistency); +// live digest re-resolution is not performed because refreshing a mutable tag +// is an explicit update operation. +func validateUpdateSHAEntries(ctx context.Context, repoRoot string) error { + return validateUpdateSHAEntriesWithResolvers(ctx, repoRoot, defaultValidationResolvers()) +} + +// validateUpdateSHAEntriesStructural validates only the structural integrity of +// .github/aw/actions-lock.json (format, non-empty fields, key consistency) without +// performing any live network calls to resolve or verify SHAs. This is suitable for +// a fast pre-gate check during compilation. +func validateUpdateSHAEntriesStructural(ctx context.Context, repoRoot string) error { + return validateUpdateSHAEntriesWithResolvers(ctx, repoRoot, structuralOnlyResolvers()) +} + +// structuralOnlyResolvers returns resolvers that skip all live network checks. +// Used for structural-only validation (format, key consistency) without API calls. +func structuralOnlyResolvers() validationResolvers { + return validationResolvers{ + verifyActionCommitExists: func(_ context.Context, _, _ string) error { + return nil + }, + resolveActionVersionToSHA: func(_ context.Context, _, _ string) (string, error) { + // Return ErrVerificationSkipped so the caller treats this as non-fatal and + // skips the version→SHA round-trip check entirely. Structural-only mode + // validates format and key consistency without live API calls. + return "", fmt.Errorf("%w: structural-only mode", parser.ErrVerificationSkipped) + }, + } +} + +func validateUpdateSHAEntriesWithResolvers(ctx context.Context, repoRoot string, r validationResolvers) error { + actionsLockPath := filepath.Join(repoRoot, ".github", "aw", "actions-lock.json") + if _, err := os.Stat(actionsLockPath); err != nil { + if os.IsNotExist(err) { + return nil + } + return fmt.Errorf("failed to read actions-lock.json metadata: %w", err) + } + + cache := workflow.NewActionCache(repoRoot) + if err := cache.Load(); err != nil { + return fmt.Errorf("failed to load actions-lock.json: %w", err) + } + + var issues []string + + entryKeys := make([]string, 0, len(cache.Entries)) + for key := range cache.Entries { + entryKeys = append(entryKeys, key) + } + sort.Strings(entryKeys) + for _, key := range entryKeys { + entry := cache.Entries[key] + if entry.Repo == "" { + issues = append(issues, fmt.Sprintf("action entry %q has empty repo", key)) + } + if entry.Version == "" { + issues = append(issues, fmt.Sprintf("action entry %q has empty version", key)) + } + validSHA := false + if entry.SHA == "" { + issues = append(issues, fmt.Sprintf("action entry %q has empty SHA", key)) + } else if !IsCommitSHA(entry.SHA) { + issues = append(issues, fmt.Sprintf("action entry %q has invalid SHA %q (expected 40-character commit SHA)", key, entry.SHA)) + } else { + validSHA = true + // Verify the commit SHA actually exists in the repository via the + // GitHub commits API. Auth/network failures are non-fatal and logged; + // only a definitive not-found (e.g. HTTP 422/404) is an error. + if entry.Repo != "" { + if err := r.verifyActionCommitExists(ctx, entry.Repo, entry.SHA); err != nil { + if errors.Is(err, parser.ErrVerificationSkipped) { + updateValidationLog.Printf("action entry %q: skipping commit existence check (auth/network error): %v", key, err) + } else { + issues = append(issues, fmt.Sprintf("action entry %q: commit SHA %q not found in %q: %v", key, entry.SHA, entry.Repo, err)) + } + } + } + } + if entry.Repo != "" && entry.Version != "" { + expectedKey := entry.Repo + "@" + entry.Version + if key != expectedKey { + issues = append(issues, fmt.Sprintf("action entry key/version mismatch: key %q should be %q", key, expectedKey)) + } + if validSHA { + // Verify the stored version tag resolves to the stored SHA. + // Auth/network failures are non-fatal; only a confirmed mismatch is an error. + resolvedVersionSHA, err := r.resolveActionVersionToSHA(ctx, entry.Repo, entry.Version) + if err != nil { + updateValidationLog.Printf("action entry %q: skipping version/SHA check (resolution failed): %v", key, err) + } else if !strings.EqualFold(resolvedVersionSHA, entry.SHA) { + issues = append(issues, fmt.Sprintf("action entry %q SHA/version mismatch: version %q resolves to %q but stored SHA is %q", key, entry.Version, resolvedVersionSHA, entry.SHA)) + } + } + } + } + + containerKeys := make([]string, 0, len(cache.ContainerPins)) + for image := range cache.ContainerPins { + containerKeys = append(containerKeys, image) + } + sort.Strings(containerKeys) + for _, image := range containerKeys { + pin := cache.ContainerPins[image] + if pin.Image != image { + issues = append(issues, fmt.Sprintf("container pin key/image mismatch: key %q has image %q", image, pin.Image)) + } + if !sha256DigestPattern.MatchString(pin.Digest) { + issues = append(issues, fmt.Sprintf("container pin %q has invalid digest %q (expected sha256:<64 lowercase hex chars>)", image, pin.Digest)) + } + expectedPinnedImage := image + "@" + pin.Digest + if pin.PinnedImage != expectedPinnedImage { + issues = append(issues, fmt.Sprintf("container pin %q has inconsistent pinned_image %q (expected %q)", image, pin.PinnedImage, expectedPinnedImage)) + } + } + + if len(issues) > 0 { + return fmt.Errorf("actions-lock.json validation failed:\n - %s", strings.Join(issues, "\n - ")) + } + return nil +} diff --git a/pkg/cli/update_validation_test.go b/pkg/cli/update_validation_test.go new file mode 100644 index 00000000000..22cb1673e99 --- /dev/null +++ b/pkg/cli/update_validation_test.go @@ -0,0 +1,161 @@ +//go:build !integration + +package cli + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/github/gh-aw/pkg/parser" + "github.com/github/gh-aw/pkg/testutil" + "github.com/github/gh-aw/pkg/workflow" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// noopResolvers returns resolvers that perform no network calls. +// Tests that want specific resolver behaviour should override individual fields. +func noopResolvers() validationResolvers { + return validationResolvers{ + verifyActionCommitExists: func(_ context.Context, _, _ string) error { + return nil + }, + resolveActionVersionToSHA: func(_ context.Context, _, ref string) (string, error) { + return ref, nil + }, + } +} + +func TestValidateUpdateSHAEntries_NoActionsLock(t *testing.T) { + t.Parallel() + tmpDir := testutil.TempDir(t, "validate-update-sha-*") + require.NoError(t, validateUpdateSHAEntriesWithResolvers(context.Background(), tmpDir, noopResolvers())) +} + +func TestValidateUpdateSHAEntries_ValidEntries(t *testing.T) { + t.Parallel() + tmpDir := testutil.TempDir(t, "validate-update-sha-*") + cache := workflow.NewActionCache(tmpDir) + cache.Set("actions/checkout", "v5", "93cb6efe18208431cddfb8368fd83d5badbf9bfd") + digest := "sha256:" + strings.Repeat("a", 64) + image := "ghcr.io/github/gh-aw-firewall/agent:0.27.9" + cache.SetContainerPin(image, digest, image+"@"+digest) + require.NoError(t, cache.Save()) + + r := validationResolvers{ + verifyActionCommitExists: func(_ context.Context, _, _ string) error { + return nil + }, + resolveActionVersionToSHA: func(_ context.Context, _, ref string) (string, error) { + if ref == "v5" { + return "93cb6efe18208431cddfb8368fd83d5badbf9bfd", nil + } + return ref, nil + }, + } + + require.NoError(t, validateUpdateSHAEntriesWithResolvers(context.Background(), tmpDir, r)) +} + +func TestValidateUpdateSHAEntries_InvalidEntries(t *testing.T) { + t.Parallel() + tmpDir := testutil.TempDir(t, "validate-update-sha-*") + awDir := filepath.Join(tmpDir, ".github", "aw") + require.NoError(t, os.MkdirAll(awDir, 0o755)) + + const invalidActionsLock = `{ + "entries": { + "actions/checkout@v5": { + "repo": "actions/checkout", + "version": "v5", + "sha": "short" + }, + "actions/setup-node@v6": { + "repo": "actions/setup-node", + "version": "v7", + "sha": "395ad3262231945c25e8478fd5baf05154b1d79f" + } + }, + "containers": { + "ghcr.io/test/image:v1": { + "image": "ghcr.io/test/other:v1", + "digest": "sha256:XYZ", + "pinned_image": "ghcr.io/test/image:v1@sha256:bad" + } + } +} +` + require.NoError(t, os.WriteFile(filepath.Join(awDir, "actions-lock.json"), []byte(invalidActionsLock), 0o644)) + + r := validationResolvers{ + // Commit existence check: setup-node SHA is not found; checkout SHA passes. + verifyActionCommitExists: func(_ context.Context, repo, sha string) error { + if repo == "actions/setup-node" && sha == "395ad3262231945c25e8478fd5baf05154b1d79f" { + return errors.New("commit not found") + } + return nil + }, + // Version resolution: setup-node v7 resolves to a different SHA (mismatch). + resolveActionVersionToSHA: func(_ context.Context, repo, ref string) (string, error) { + if repo == "actions/setup-node" && ref == "v7" { + return "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", nil + } + return ref, nil + }, + } + + err := validateUpdateSHAEntriesWithResolvers(context.Background(), tmpDir, r) + require.Error(t, err) + assert.Contains(t, err.Error(), `action entry "actions/checkout@v5" has invalid SHA`) + assert.Contains(t, err.Error(), `action entry key/version mismatch: key "actions/setup-node@v6" should be "actions/setup-node@v7"`) + assert.Contains(t, err.Error(), `action entry "actions/setup-node@v6": commit SHA`) + assert.Contains(t, err.Error(), `action entry "actions/setup-node@v6" SHA/version mismatch`) + assert.Contains(t, err.Error(), `container pin key/image mismatch`) + assert.Contains(t, err.Error(), `container pin "ghcr.io/test/image:v1" has invalid digest`) + assert.Contains(t, err.Error(), `container pin "ghcr.io/test/image:v1" has inconsistent pinned_image`) +} + +func TestValidateUpdateSHAEntries_NonFatalErrors(t *testing.T) { + t.Parallel() + tmpDir := testutil.TempDir(t, "validate-update-sha-*") + cache := workflow.NewActionCache(tmpDir) + cache.Set("actions/checkout", "v5", "93cb6efe18208431cddfb8368fd83d5badbf9bfd") + digest := "sha256:" + strings.Repeat("a", 64) + image := "ghcr.io/github/gh-aw-firewall/agent:0.27.9" + cache.SetContainerPin(image, digest, image+"@"+digest) + require.NoError(t, cache.Save()) + + r := validationResolvers{ + // Auth error on commit existence check — should be skipped (non-fatal). + verifyActionCommitExists: func(_ context.Context, _, _ string) error { + return fmt.Errorf("%w: auth error", parser.ErrVerificationSkipped) + }, + // Network error on version resolution — should be skipped (non-fatal). + resolveActionVersionToSHA: func(_ context.Context, _, _ string) (string, error) { + return "", errors.New("network timeout") + }, + } + + // All network failures should be non-fatal; validation should still pass. + require.NoError(t, validateUpdateSHAEntriesWithResolvers(context.Background(), tmpDir, r)) +} + +func TestValidateUpdateSHAEntries_ContainerStructuralOnly(t *testing.T) { + t.Parallel() + tmpDir := testutil.TempDir(t, "validate-update-sha-*") + digest := "sha256:" + strings.Repeat("a", 64) + image := "ghcr.io/github/gh-aw-firewall/agent:latest" + cache := workflow.NewActionCache(tmpDir) + cache.SetContainerPin(image, digest, image+"@"+digest) + require.NoError(t, cache.Save()) + + // No container resolver in the struct — container pins are structural-only. + // Even if the tag has moved to a different digest, the stored pin is still valid + // and validation passes without performing any live lookup. + require.NoError(t, validateUpdateSHAEntriesWithResolvers(context.Background(), tmpDir, noopResolvers())) +} diff --git a/pkg/parser/remote_resolve_sha.go b/pkg/parser/remote_resolve_sha.go index 608ff1d0fe9..373be8c47d5 100644 --- a/pkg/parser/remote_resolve_sha.go +++ b/pkg/parser/remote_resolve_sha.go @@ -212,3 +212,29 @@ func resolveRefToSHAViaPublicAPI(ctx context.Context, owner, repo, ref string) ( func ResolveRefToSHAForHost(ctx context.Context, owner, repo, ref, host string) (string, error) { return resolveRefToSHA(ctx, owner, repo, ref, host) } + +// ErrVerificationSkipped is returned by VerifyCommitExists when the check cannot be +// performed due to an auth or network failure. Callers should treat it as non-fatal. +var ErrVerificationSkipped = errors.New("commit verification skipped") + +// VerifyCommitExists confirms that a full commit SHA exists in the given repository by +// querying the GitHub commits API. Unlike ResolveRefToSHAForHost, it always performs +// an API call so that a missing commit is detected (not silently passed through). +// +// Returns nil if the commit is found, ErrVerificationSkipped (wrapped) for auth or +// network errors where existence cannot be determined, and an unwrapped error when the +// API definitively reports the commit does not exist (e.g. HTTP 404). +func VerifyCommitExists(ctx context.Context, owner, repo, sha, host string) error { + client, err := createRESTClientForHostFunc(host) + if err != nil { + return fmt.Errorf("%w: failed to create REST client: %w", ErrVerificationSkipped, err) + } + var result commitLookupResponse + if apiErr := client.DoWithContext(ctx, http.MethodGet, buildCommitLookupAPIPath(owner, repo, sha), nil, &result); apiErr != nil { + if isGitHubAPIAuthError(apiErr) { + return fmt.Errorf("%w: %w", ErrVerificationSkipped, apiErr) + } + return apiErr + } + return nil +}