Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions pkg/cli/add_package_manifest.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,13 @@ func resolveRepositoryPackage(ctx context.Context, repoSpec *RepoSpec, host stri
return nil, err
}

extensionFiles, err := resolveRepositoryPackageExtensionFiles(ctx, owner, repo, packagePath, ref, host, manifest, includeSkillDirs, includeAgentFiles)
extensionFiles, err := resolveRepositoryPackageExtensionFiles(ctx, repositoryPackageLocation{
owner: owner,
repo: repo,
packagePath: packagePath,
ref: ref,
host: host,
}, manifest, includeSkillDirs, includeAgentFiles)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -172,19 +178,27 @@ type repositoryPackageExtensionFiles struct {
warnings []string
}

func resolveRepositoryPackageExtensionFiles(ctx context.Context, owner, repo, packagePath, ref, host string, manifest *repositoryPackageManifest, includeSkillDirs, includeAgentFiles []string) (*repositoryPackageExtensionFiles, error) {
type repositoryPackageLocation struct {
owner string
repo string
packagePath string
ref string
host string
}

func resolveRepositoryPackageExtensionFiles(ctx context.Context, packageLocation repositoryPackageLocation, manifest *repositoryPackageManifest, includeSkillDirs, includeAgentFiles []string) (*repositoryPackageExtensionFiles, error) {
// Resolve skill files: explicit from manifest or auto-scanned.
explicitSkillDirs := append([]string{}, manifest.Skills...)
explicitSkillDirs = append(explicitSkillDirs, includeSkillDirs...)
skillFiles, skillWarnings, err := resolvePackageSkillFiles(ctx, owner, repo, packagePath, ref, host, explicitSkillDirs)
skillFiles, skillWarnings, err := resolvePackageSkillFiles(ctx, packageLocation.owner, packageLocation.repo, packageLocation.packagePath, packageLocation.ref, packageLocation.host, explicitSkillDirs)
if err != nil {
return nil, err
}

// Resolve agent files: explicit from manifest or auto-scanned.
explicitAgentFiles := append([]string{}, manifest.Agents...)
explicitAgentFiles = append(explicitAgentFiles, includeAgentFiles...)
agentFiles, agentWarnings, err := resolvePackageAgentFiles(ctx, owner, repo, packagePath, ref, host, explicitAgentFiles)
agentFiles, agentWarnings, err := resolvePackageAgentFiles(ctx, packageLocation.owner, packageLocation.repo, packageLocation.packagePath, packageLocation.ref, packageLocation.host, explicitAgentFiles)
if err != nil {
return nil, err
}
Expand Down
9 changes: 8 additions & 1 deletion pkg/workflow/compiler.go
Original file line number Diff line number Diff line change
Expand Up @@ -560,7 +560,14 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath
// file is written and the agent receives the actionable guidance embedded in the warning.
if safeUpdateEnabled {
currentHasPR, currentHasPRTarget := extractPullRequestEventPresenceFromOnField(workflowData.RawFrontmatter["on"])
if enforceErr := EnforceSafeUpdate(oldManifest, bodySecrets, bodyActions, workflowData.Redirect, oldHasPR, oldHasPRTarget, currentHasPR, currentHasPRTarget, collectMemoryValidationScripts(workflowData)); enforceErr != nil {
if enforceErr := EnforceSafeUpdate(oldManifest, bodySecrets, bodyActions, SafeUpdateEnforcementOptions{
CurrentRedirect: workflowData.Redirect,
OldHasPullRequest: oldHasPR,
OldHasPullRequestTarget: oldHasPRTarget,
CurrentHasPullRequest: currentHasPR,
CurrentHasPullRequestTarget: currentHasPRTarget,
CurrentMemoryValidationScripts: collectMemoryValidationScripts(workflowData),
}); enforceErr != nil {
warningMsg := buildSafeUpdateWarningPrompt(enforceErr.Error())
c.AddSafeUpdateWarning(warningMsg)
fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", enforceErr.Error()))
Expand Down
22 changes: 11 additions & 11 deletions pkg/workflow/compiler_threat_detection_formal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,67 +9,67 @@ import (
)

func TestFormal_CTR016_NilManifestSkipsEnforcement(t *testing.T) {
err := EnforceSafeUpdate(nil, []string{"MY_SECRET"}, []string{"evil-org/action@deadbeef # v1"}, "", false, false, false, false, nil)
err := EnforceSafeUpdate(nil, []string{"MY_SECRET"}, []string{"evil-org/action@deadbeef # v1"}, SafeUpdateEnforcementOptions{})
require.NoError(t, err)
}

func TestFormal_CTR016_EmptyManifestRejectsNewSecret(t *testing.T) {
err := EnforceSafeUpdate(&GHAWManifest{Version: currentGHAWManifestVersion}, []string{"MY_SECRET"}, nil, "", false, false, false, false, nil)
err := EnforceSafeUpdate(&GHAWManifest{Version: currentGHAWManifestVersion}, []string{"MY_SECRET"}, nil, SafeUpdateEnforcementOptions{})
require.Error(t, err)
require.ErrorContains(t, err, "MY_SECRET")
}

func TestFormal_CTR016_GitHubTokenExempt_BareForm(t *testing.T) {
err := EnforceSafeUpdate(&GHAWManifest{Version: currentGHAWManifestVersion}, []string{"GITHUB_TOKEN"}, nil, "", false, false, false, false, nil)
err := EnforceSafeUpdate(&GHAWManifest{Version: currentGHAWManifestVersion}, []string{"GITHUB_TOKEN"}, nil, SafeUpdateEnforcementOptions{})
require.NoError(t, err)
}

func TestFormal_CTR016_GitHubTokenExempt_PrefixedForm(t *testing.T) {
err := EnforceSafeUpdate(&GHAWManifest{Version: currentGHAWManifestVersion}, []string{"secrets.GITHUB_TOKEN"}, nil, "", false, false, false, false, nil)
err := EnforceSafeUpdate(&GHAWManifest{Version: currentGHAWManifestVersion}, []string{"secrets.GITHUB_TOKEN"}, nil, SafeUpdateEnforcementOptions{})
require.NoError(t, err)
}

func TestFormal_CTR016_GhAwInternalSecretExempt(t *testing.T) {
err := EnforceSafeUpdate(&GHAWManifest{Version: currentGHAWManifestVersion}, []string{"GH_AW_GITHUB_TOKEN"}, nil, "", false, false, false, false, nil)
err := EnforceSafeUpdate(&GHAWManifest{Version: currentGHAWManifestVersion}, []string{"GH_AW_GITHUB_TOKEN"}, nil, SafeUpdateEnforcementOptions{})
require.NoError(t, err)
}

func TestFormal_CTR016_SecretPrefixNormalization(t *testing.T) {
manifest := &GHAWManifest{Version: currentGHAWManifestVersion, Secrets: []string{"MY_SECRET"}}
err := EnforceSafeUpdate(manifest, []string{"secrets.MY_SECRET"}, nil, "", false, false, false, false, nil)
err := EnforceSafeUpdate(manifest, []string{"secrets.MY_SECRET"}, nil, SafeUpdateEnforcementOptions{})
require.NoError(t, err)
}

func TestFormal_CTR016_NewActionDriftRejected(t *testing.T) {
manifest := &GHAWManifest{Version: currentGHAWManifestVersion, Actions: []GHAWManifestAction{{Repo: "actions/checkout", SHA: "abc1234", Version: "v4"}}}
err := EnforceSafeUpdate(manifest, nil, []string{"actions/checkout@abc1234 # v4", "evil-org/steal@deadbeef # v1"}, "", false, false, false, false, nil)
err := EnforceSafeUpdate(manifest, nil, []string{"actions/checkout@abc1234 # v4", "evil-org/steal@deadbeef # v1"}, SafeUpdateEnforcementOptions{})
require.Error(t, err)
require.ErrorContains(t, err, "evil-org/steal")
}

func TestFormal_CTR016_RemovedActionDriftRejected(t *testing.T) {
manifest := &GHAWManifest{Version: currentGHAWManifestVersion, Actions: []GHAWManifestAction{{Repo: "my-org/approved-action", SHA: "abc1234", Version: "v1"}}}
err := EnforceSafeUpdate(manifest, nil, []string{}, "", false, false, false, false, nil)
err := EnforceSafeUpdate(manifest, nil, []string{}, SafeUpdateEnforcementOptions{})
require.Error(t, err)
require.ErrorContains(t, err, "Previously-approved action")
require.ErrorContains(t, err, "my-org/approved-action")
}

func TestFormal_CTR016_KnownActionPinUpdateAllowed(t *testing.T) {
manifest := &GHAWManifest{Version: currentGHAWManifestVersion, Actions: []GHAWManifestAction{{Repo: "my-org/action", SHA: "abc1234", Version: "v1"}}}
err := EnforceSafeUpdate(manifest, nil, []string{"my-org/action@def5678 # v2"}, "", false, false, false, false, nil)
err := EnforceSafeUpdate(manifest, nil, []string{"my-org/action@def5678 # v2"}, SafeUpdateEnforcementOptions{})
require.NoError(t, err)
}

func TestFormal_CTR016_RedirectWhitespaceNormalization(t *testing.T) {
manifest := &GHAWManifest{Version: currentGHAWManifestVersion, Redirect: "owner/repo/workflows/new.md@main"}
err := EnforceSafeUpdate(manifest, nil, nil, " owner/repo/workflows/new.md@main ", false, false, false, false, nil)
err := EnforceSafeUpdate(manifest, nil, nil, SafeUpdateEnforcementOptions{CurrentRedirect: " owner/repo/workflows/new.md@main "})
require.NoError(t, err)
}

func TestFormal_CTR016_RedirectChangeRejected(t *testing.T) {
manifest := &GHAWManifest{Version: currentGHAWManifestVersion, Redirect: "owner/repo/workflows/old.md@main"}
err := EnforceSafeUpdate(manifest, nil, nil, "owner/repo/workflows/new.md@main", false, false, false, false, nil)
err := EnforceSafeUpdate(manifest, nil, nil, SafeUpdateEnforcementOptions{CurrentRedirect: "owner/repo/workflows/new.md@main"})
require.Error(t, err)
require.ErrorContains(t, err, "New redirect configured")
require.ErrorContains(t, err, "Previously-approved redirect removed")
Expand Down
19 changes: 15 additions & 4 deletions pkg/workflow/safe_update_enforcement.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,17 @@ var ghAwInternalSecrets = map[string]bool{
"COPILOT_GITHUB_TOKEN": true,
}

// SafeUpdateEnforcementOptions carries current-compilation context used by
// EnforceSafeUpdate to detect unsafe changes against the existing manifest.
type SafeUpdateEnforcementOptions struct {
CurrentRedirect string
OldHasPullRequest bool
OldHasPullRequestTarget bool
CurrentHasPullRequest bool
CurrentHasPullRequestTarget bool
CurrentMemoryValidationScripts []GHAWManifestMemoryValidationScript
}

// EnforceSafeUpdate validates that no new restricted secrets or unapproved action
// changes have been introduced compared to those recorded in the existing manifest.
//
Expand All @@ -50,7 +61,7 @@ var ghAwInternalSecrets = map[string]bool{
// e.g. "actions/checkout@abc1234 # v4".
//
// Returns a structured, actionable error when violations are found.
func EnforceSafeUpdate(manifest *GHAWManifest, secretNames []string, actionRefs []string, currentRedirect string, oldHasPullRequest bool, oldHasPullRequestTarget bool, currentHasPullRequest bool, currentHasPullRequestTarget bool, currentMemoryValidationScripts []GHAWManifestMemoryValidationScript) error {
func EnforceSafeUpdate(manifest *GHAWManifest, secretNames []string, actionRefs []string, opts SafeUpdateEnforcementOptions) error {
if manifest == nil {
// Lock file exists but predates the safe-updates feature (no gh-aw-manifest
// section). Skip enforcement so legacy lock files are not flagged on upgrade.
Expand All @@ -60,9 +71,9 @@ func EnforceSafeUpdate(manifest *GHAWManifest, secretNames []string, actionRefs

secretViolations := collectSecretViolations(manifest, secretNames)
addedActions, removedActions := collectActionViolations(manifest, actionRefs)
addedRedirect, removedRedirect := collectRedirectViolations(manifest, currentRedirect)
memoryValidationScriptChanges := collectMemoryValidationScriptChanges(manifest, currentMemoryValidationScripts)
pullRequestTargetEscalation := hasPullRequestTargetEscalation(oldHasPullRequest, oldHasPullRequestTarget, currentHasPullRequest, currentHasPullRequestTarget)
addedRedirect, removedRedirect := collectRedirectViolations(manifest, opts.CurrentRedirect)
memoryValidationScriptChanges := collectMemoryValidationScriptChanges(manifest, opts.CurrentMemoryValidationScripts)
pullRequestTargetEscalation := hasPullRequestTargetEscalation(opts.OldHasPullRequest, opts.OldHasPullRequestTarget, opts.CurrentHasPullRequest, opts.CurrentHasPullRequestTarget)

if len(secretViolations) == 0 && len(addedActions) == 0 && len(removedActions) == 0 && addedRedirect == "" && removedRedirect == "" && len(memoryValidationScriptChanges) == 0 && !pullRequestTargetEscalation {
safeUpdateLog.Printf("Safe update check passed (%d secret(s), %d action(s) verified)",
Expand Down
12 changes: 10 additions & 2 deletions pkg/workflow/safe_update_enforcement_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,13 @@ func TestEnforceSafeUpdate(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := EnforceSafeUpdate(tt.manifest, tt.secretNames, tt.actionRefs, tt.redirect, tt.oldHasPR, tt.oldHasPRTarget, tt.currentHasPR, tt.currentHasPRTarget, nil)
err := EnforceSafeUpdate(tt.manifest, tt.secretNames, tt.actionRefs, SafeUpdateEnforcementOptions{
CurrentRedirect: tt.redirect,
OldHasPullRequest: tt.oldHasPR,
OldHasPullRequestTarget: tt.oldHasPRTarget,
CurrentHasPullRequest: tt.currentHasPR,
CurrentHasPullRequestTarget: tt.currentHasPRTarget,
})
if tt.wantErr {
require.Error(t, err, "expected safe update enforcement error")
for _, msg := range tt.wantErrMsgs {
Expand Down Expand Up @@ -470,7 +476,9 @@ func TestMemoryValidationScriptChangesRequireSafeUpdateReview(t *testing.T) {
"repo-memory:removed (removed)",
}, changes)

err := EnforceSafeUpdate(manifest, nil, nil, "", false, false, false, false, current)
err := EnforceSafeUpdate(manifest, nil, nil, SafeUpdateEnforcementOptions{
CurrentMemoryValidationScripts: current,
})
require.Error(t, err)
require.ErrorContains(t, err, "Memory validation script changes")
require.ErrorContains(t, err, "cache-memory:added (added)")
Expand Down
Loading