diff --git a/cli/docs/flags.go b/cli/docs/flags.go index 66cb5c6f7..2016cdf10 100644 --- a/cli/docs/flags.go +++ b/cli/docs/flags.go @@ -22,7 +22,6 @@ const ( DockerScan = "docker scan" Audit = "audit" CurationAudit = "curation-audit" - CurationActions = "curate-gh-actions" GitAudit = "git-audit" GitCountContributors = "count-contributors" Enrich = "sbom-enrich" @@ -239,8 +238,6 @@ var commandFlags = map[string][]string{ CurationAudit: { CurationOutput, WorkingDirs, Threads, RequirementsFile, InsecureTls, useWrapperAudit, UseIncludedBuilds, SolutionPath, DockerImageName, HuggingFaceModel, IncludeCachedPackages, MvnIncludePluginDeps, LegacyPeerDeps, RunNative, Script, }, - // curate-gh-actions takes every input from the runner environment; it has no flags. - CurationActions: {}, GitCountContributors: { InputFile, ScmType, ScmApiUrl, Token, Owner, RepoName, Months, DetailedSummary, InsecureTls, GitThreads, CacheValidity, }, diff --git a/cli/docs/scan/curationactions/help.go b/cli/docs/scan/curationactions/help.go deleted file mode 100644 index 867442634..000000000 --- a/cli/docs/scan/curationactions/help.go +++ /dev/null @@ -1,53 +0,0 @@ -package curationactions - -func GetDescription() string { - return "Curate the third-party GitHub Actions resolved on this job's runner." -} - -func GetAIDescription() string { - return `Inspect every GitHub Action that GitHub's runner actually downloaded for this job (its _actions cache directory) and report a curation Approved/Rejected status per action, including actions pulled in transitively by another action's own action.yml. - -Scope: the actions in this job's runner cache. Each job runs on its own runner with its own cache, so that is exactly what this job resolved. The workflow file (GITHUB_WORKFLOW_REF) and job (GITHUB_JOB) are used to attribute those actions to the uses: lines that declared them, not to decide which ones get curated. - -When to use: -- Run as an early step in a GitHub Actions job to curate third-party actions before the rest of the job executes. -- Produce a curation report of every resolved action (direct and transitive) for the current job. - -Which policies apply: the Artifactory repository governing the job, looked up from the GitHub repository running it (GITHUB_REPOSITORY). The mapping is curation-side configuration. If it cannot be resolved the command fails. - -Prerequisites: -- Must run on a GitHub Actions runner. The command takes no flags: the action cache, workflow, job and repository all come from the runner environment (RUNNER_WORKSPACE, GITHUB_WORKFLOW_REF, GITHUB_JOB, GITHUB_REPOSITORY). Outside a runner those are unset and the command reports an error rather than guessing. - -Common patterns: - $ jf curate-gh-actions - -Gotchas: -- If the action cache cannot be read - RUNNER_WORKSPACE is unset, or the directory it points at is absent the command reports an error rather. -- Subpath and parent attribution is best-effort and additive: it adds a Parent when it can explain where an action came from. Actions pulled in transitively by a composite action's own action.yml uses: lines are attributed and reported with that action as their Parent. One it cannot place - pulled in by an action.yml this parser cannot read - is reported with an empty Parent, still curated, just unexplained. -- Steps that run a container image rather than an action are not curated. A step using 'uses: docker://' resolves to a container reference, so the runner pulls the image during job setup instead of into the action cache and it never appears in the scan. Curate those images with 'jf curation-audit --image '. -- An action that itself runs in a container ('runs: using: docker' in its action.yml) is curated as an action, but the image it pulls is not. The same 'jf curation-audit --image' applies. -- An action reached only through a local composite action ('uses: ./...') is not curated. The runner cannot resolve a local action's own references until the workspace is checked out, so it downloads them when that step runs - after this command has already read the cache. An action pulled in via a 'run:' step is never resolved into the cache at all. The same applies to a 'uses: ./...' inside a third-party composite action's own action.yml: a relative path there names a directory in YOUR repository rather than one inside the action, so it resolves from the workspace at step time too. The report says so rather than leaving it to this page: when a workflow file is available it carries a 'Not covered' line naming each local step the job reaches - from the workflow itself and from any composite action's action.yml, at any depth, with the declaring action named - and when no workflow file is available (the normal case, since the check runs before the checkout) it carries that line unconditionally, because it cannot tell whether one is reached. -- Actions used by a called reusable workflow (jobs..uses:) are not curated by the calling job. A called workflow's jobs run on their own runners with their own action caches, so those actions never reach this runner. Add jfrog/setup-jfrog-cli as a step in the reusable workflow to curate them. -- Attribution can be wrong inside a called reusable workflow and is a known limitation in parsing because of incorrect information github refs hold. -- If no workflow file can be used, curation still runs against every action in the runner's cache, but without parent attribution - the report omits the Parent column and carries the 'Not covered' line described above. That covers every case where the file cannot be read or understood: nothing identified a workflow; GITHUB_WORKFLOW_REF named a file that is not on disk, which is normal early in a job since the workspace holds no checkout yet; the file does not declare the job being curated; or the YAML cannot be parsed. Coverage never changes - every action in the cache is decided either way - only the report's detail does. -- A workflow file that cannot be parsed does not fail the command but produces a non attributed report. - -Related: jf curation-audit - -QA: -Q: What's the command to curate the GitHub Actions used in this job? -A: jf curate-gh-actions - -Q: Can I run it outside a GitHub Actions runner? -A: No. It has no flags and reads everything from the runner environment, so off a runner it reports an error rather than curating something it cannot identify. - -Q: Does this curate the actions used by a reusable workflow my job calls? -A: No - a called reusable workflow runs its jobs on their own runners, so add jf curate-gh-actions as a step inside that reusable workflow. - -Q: Why does a green report still say "Not covered"? -A: An all-Approved table covers the actions the runner had already resolved when the check ran. Local composite actions ('uses: ./...') resolve their own references later in the job, so the report states that boundary rather than letting the table read as full coverage. - -Q: Does this curate a step that uses docker://? -A: No - the runner pulls that image during job setup rather than into the action cache, so it never reaches this scan. Curate it with jf curation-audit --image . -` -} diff --git a/cli/scancommands.go b/cli/scancommands.go index f26fd29a2..6469663f2 100644 --- a/cli/scancommands.go +++ b/cli/scancommands.go @@ -30,7 +30,6 @@ import ( auditDocs "github.com/jfrog/jfrog-cli-security/cli/docs/scan/audit" buildScanDocs "github.com/jfrog/jfrog-cli-security/cli/docs/scan/buildscan" curationDocs "github.com/jfrog/jfrog-cli-security/cli/docs/scan/curation" - curationActionsDocs "github.com/jfrog/jfrog-cli-security/cli/docs/scan/curationactions" dockerScanDocs "github.com/jfrog/jfrog-cli-security/cli/docs/scan/dockerscan" scanDocs "github.com/jfrog/jfrog-cli-security/cli/docs/scan/scan" uploadCdxDocs "github.com/jfrog/jfrog-cli-security/cli/docs/upload" @@ -132,17 +131,6 @@ func getAuditAndScansCommands() []components.Command { Category: securityCategory, Action: CurationCmd, }, - { - // Hidden until Catalog/Artifactory add support for VCS package type for GitHub Actions. Until then the - // curation decision is a stand-in, so the command must not be discoverable to users. - Name: "curate-gh-actions", - Flags: flags.GetCommandFlags(flags.CurationActions), - Description: curationActionsDocs.GetDescription(), - AIDescription: curationActionsDocs.GetAIDescription(), - Category: securityCategory, - Action: CurationActionsCmd, - Hidden: true, - }, { Name: "source-mcp", Description: mcpDocs.GetDescription(), @@ -658,13 +646,6 @@ func CurationCmd(c *components.Context) error { return progressbar.ExecWithProgress(curationAuditCommand) } -// CurationActionsCmd curates the GitHub Actions resolved on this job's runner. -func CurationActionsCmd(c *components.Context) error { - // No flags: every input comes from the runner environment. The setters the command - // exposes are for tests, which construct it directly rather than through the CLI. - return curation.NewCurationActionsCommand().Run() -} - var supportedCommandsForPostInstallationFailure = datastructures.MakeSetFromElements[string]( "install", "build", "i", "add", "ci", "get", "mod", ) diff --git a/commands/curation/curationactions.go b/commands/curation/curationactions.go deleted file mode 100644 index 8abd69dfa..000000000 --- a/commands/curation/curationactions.go +++ /dev/null @@ -1,296 +0,0 @@ -package curation - -import ( - "context" - "errors" - "fmt" - "os" - "path/filepath" - "strings" - - "github.com/jfrog/jfrog-cli-core/v2/utils/coreutils" - "github.com/jfrog/jfrog-client-go/utils/errorutils" - "github.com/jfrog/jfrog-client-go/utils/log" - - "github.com/jfrog/jfrog-cli-security/commands/curation/githubactions" - "github.com/jfrog/jfrog-cli-security/utils/formats" - "github.com/jfrog/jfrog-cli-security/utils/results/output" -) - -// CurationActionsCommand curates the GitHub Actions that actually resolved on this job's -// runner, taking the runner's action cache as the source of truth. -type CurationActionsCommand struct { - workingDir string - actionsCacheDir string - workflowFile string - jobID string - githubRepo string - decider githubactions.ActionCurationDecider - vcsRepoResolver githubactions.ArtifactoryVcsRepoResolver -} - -func NewCurationActionsCommand() *CurationActionsCommand { - return &CurationActionsCommand{ - decider: githubactions.NewMockActionCurationDecider(), - vcsRepoResolver: githubactions.NewMockArtifactoryVcsRepoResolver(), - } -} - -// SetWorkingDir overrides the repo root; defaults to the process's working directory. No flag -// sets this - it exists so a test can anchor the path GITHUB_WORKFLOW_REF derives. -func (c *CurationActionsCommand) SetWorkingDir(dir string) *CurationActionsCommand { - c.workingDir = dir - return c -} - -// SetActionsCacheDir overrides the runner's action cache directory; defaults to -// githubactions.DefaultActionsCacheDir() (derived from RUNNER_WORKSPACE). -func (c *CurationActionsCommand) SetActionsCacheDir(dir string) *CurationActionsCommand { - c.actionsCacheDir = dir - return c -} - -// SetWorkflowFile overrides the workflow file to cross-reference against; defaults to the -// running workflow derived from GITHUB_WORKFLOW_REF. -func (c *CurationActionsCommand) SetWorkflowFile(path string) *CurationActionsCommand { - c.workflowFile = path - return c -} - -// SetJobID overrides the workflow job to scope curation to; defaults to the running job's -// job_id from GITHUB_JOB. -func (c *CurationActionsCommand) SetJobID(jobID string) *CurationActionsCommand { - c.jobID = jobID - return c -} - -// SetGithubRepo overrides the GitHub repository to resolve the Artifactory VCS repository from; -// defaults to GITHUB_REPOSITORY. -func (c *CurationActionsCommand) SetGithubRepo(githubRepo string) *CurationActionsCommand { - c.githubRepo = githubRepo - return c -} - -// SetVcsRepoResolver overrides the Artifactory VCS repository resolver -func (c *CurationActionsCommand) SetVcsRepoResolver(resolver githubactions.ArtifactoryVcsRepoResolver) *CurationActionsCommand { - c.vcsRepoResolver = resolver - return c -} - -// SetDecider overrides the curation decider -func (c *CurationActionsCommand) SetDecider(decider githubactions.ActionCurationDecider) *CurationActionsCommand { - c.decider = decider - return c -} - -func (c *CurationActionsCommand) CommandName() string { - return "curate_gh_actions" -} - -// Run discovers the actions resolved on this job's runner, decides a curation outcome per -// action, prints and records the report, and returns an error unless every action was Approved. -// Only that exact status clears the gate - a rejection withholds the job, and so would a status -// this code does not recognize, which is what keeps a future decider's unhandled outcome from -// reading as a pass. -// -// If any action cannot be decided at all Run returns that error and produces no report and no job summary. -// -// With a workflow file cross-referencing entries for Parent and Subpath -// metadata and rendering a Parent column; without one, STRUCTURE-ONLY. -func (c *CurationActionsCommand) Run() (err error) { - // A one-shot CLI invocation, so this is the root of the call tree, and no deadline is imposed - // here. Artifactory carries a fail-open / fail-close setting that governs what happens when - // curation cannot reach a verdict - a timeout, or a decision service that is unreachable. - // Fetching that setting and honouring it lands with the real decision client; until then this - // command is unconditionally fail-closed. - ctx := context.Background() - - workingDir := c.workingDir - if workingDir == "" { - if workingDir, err = coreutils.GetWorkingDirectory(); err != nil { - return err - } - } - - actionsCacheDir := c.actionsCacheDir - if actionsCacheDir == "" { - if actionsCacheDir, err = githubactions.DefaultActionsCacheDir(); err != nil { - return err - } - } - - scan, err := githubactions.DiscoverActionCache(actionsCacheDir) - if err != nil { - return err - } - if err = scan.UnaccountedError(); err != nil { - return err - } - discovered := scan.Refs - if len(discovered) == 0 { - return githubactions.ErrCacheNotReadable() - } - - used, attributed, err := c.parseWorkflowUses(workingDir) - if err != nil { - return err - } - var localUses []githubactions.LocalUse - if attributed { - discovered, localUses = githubactions.CrossReference(discovered, used) - } - // Resolved once, after the early return above: a job with nothing to curate makes no call. - artifactoryVcsRepo, err := c.resolveArtifactoryVcsRepo(ctx) - if err != nil { - return err - } - - rows := make([]githubactions.ActionReportRow, 0, len(discovered)) - var decideErrs error - for _, ref := range discovered { - result, decideErr := c.decider.Decide(ctx, artifactoryVcsRepo, ref) - if decideErr != nil { - decideErrs = errors.Join(decideErrs, fmt.Errorf("deciding curation status for %s/%s@%s: %w", ref.Owner, ref.Repo, ref.Ref, decideErr)) - continue - } - rows = append(rows, githubactions.NewActionReportRow(ref, result)) - } - if decideErrs != nil { - return decideErrs - } - - curated := curatedActions(rows, attributed, localUses) - caveat := formats.RenderActionsException([]formats.CuratedActions{curated}) - log.Info(fmt.Sprintf("GitHub Actions Curation Report:\n%s%s", githubactions.RenderReportTable(rows, attributed), caveat)) - - // Warn rather than fail: every action above was decided, so the verdicts are complete and - // already reported. Only the job summary is lost, and failing a job over the summary - // directory being unwritable would fail it for a reporting problem rather than a curation one. - if recordErr := c.recordSummary(curated); recordErr != nil { - log.Warn(fmt.Sprintf("Failed to record the GitHub Actions curation summary, so the job summary will not show "+ - "the curation section - the report above is the complete result: %v", recordErr)) - } - - if notApproved := githubactions.NotApproved(rows); len(notApproved) > 0 { - var msg strings.Builder - msg.WriteString("curation policy did not approve every GitHub Action this job resolved:") - for _, row := range notApproved { - fmt.Fprintf(&msg, "\n %s@%s: status %q", row.Action, row.Ref, row.Status) - if row.Notes != "" { - fmt.Fprintf(&msg, " - %s", row.Notes) - } - } - return errors.New(msg.String()) - } - return nil -} - -// parseWorkflowUses resolves which workflow file to attribute against, returning its uses: -// refs and whether attribution is possible at all. Resolution order: -// -// 1. SetWorkflowFile (+ SetJobID) - set explicitly, by a test. The path must be absolute: the -// caller named one specific file, so there is deliberately nothing to resolve it against. -// 2. GITHUB_WORKFLOW_REF (+ GITHUB_JOB) - the running workflow and job on a runner. GitHub sets -// this to a repo-relative path, so it resolves against workingDir - the process's working -// directory, which is the runner's workspace. -// 3. neither. -// -// Nothing about the workflow file fails the command. Curating the cache is the job; attribution -// only explains what is already being curated, so a file that is absent, unreadable, unparsable -// or silent about this job costs the Parent column and nothing else - the cache is still the -// complete account of what will execute, and every entry in it is decided either way. That holds -// for an explicitly set file too: a job gated on this command must not fail because a path -// was wrong. It is not silent either way, because a run without attribution says so in the -// report - see formats.RenderActionsException. -// -// The one error returned is an explicitly set path that is not absolute, which is a malformed -// argument rather than a condition of the run, and is rejected before anything is read. -// -// This also matches parseCompositeActionUses, which takes the same view of an action.yml it -// cannot read. -func (c *CurationActionsCommand) parseWorkflowUses(workingDir string) (used githubactions.JobUses, attributed bool, err error) { - jobID := c.jobID - if jobID == "" { - jobID = githubactions.DefaultJobID() - } - workflowFile := c.workflowFile - if explicit := workflowFile != ""; explicit { - if !filepath.IsAbs(workflowFile) { - return githubactions.JobUses{}, false, errorutils.CheckErrorf("the workflow file must be an absolute path, got %q", workflowFile) - } - } else { - workflowFile = githubactions.DefaultWorkflowFile() - if workflowFile == "" { - log.Info("No workflow file was identified - curating the runner's action cache as-is, without parent attribution.") - return githubactions.JobUses{}, false, nil - } - workflowFile = filepath.Join(workingDir, workflowFile) - } - if used, err = githubactions.ParseWorkflowUses(workflowFile, jobID); err == nil { - return used, true, nil - } - switch { - case errors.Is(err, githubactions.ErrJobUnknown): - log.Info(fmt.Sprintf("Cannot identify job %q in workflow file %q - curating the runner's action cache as-is, without parent attribution.", jobID, workflowFile)) - case errors.Is(err, githubactions.ErrWorkflowUnparsable): - log.Warn(fmt.Sprintf("Cannot parse workflow file %q - curating the runner's action cache as-is, without parent attribution: %v", workflowFile, err)) - case errors.Is(err, os.ErrNotExist): - log.Warn(fmt.Sprintf("Workflow file %q is not on disk - curating the runner's action cache as-is, without parent attribution. "+ - "The workspace has no checkout this early in the job, so there is nothing to attribute against.", - workflowFile)) - default: - // Permissions, an I/O error, a path that is a directory. Distinct from the cases above - // only in cause, not in consequence: nothing can be attributed, and everything is still - // curated. - log.Warn(fmt.Sprintf("Cannot read workflow file %q - curating the runner's action cache as-is, without parent attribution: %v", workflowFile, err)) - } - return githubactions.JobUses{}, false, nil -} - -// resolveArtifactoryVcsRepo returns the Artifactory VCS repository whose curation policies -// govern this job, looked up from the GitHub repository running it. GITHUB_REPOSITORY is set on -// every runner; SetGithubRepo overrides it for tests. -func (c *CurationActionsCommand) resolveArtifactoryVcsRepo(ctx context.Context) (string, error) { - githubRepo := c.githubRepo - if githubRepo == "" { - githubRepo = githubactions.DefaultGithubRepo() - } - if githubRepo == "" { - return "", errorutils.CheckErrorf("cannot determine which GitHub repository this job belongs to: "+ - "%s is not set", githubactions.GithubRepoEnvVar) - } - repo, err := c.vcsRepoResolver.Resolve(ctx, githubRepo) - if err != nil { - return "", fmt.Errorf("resolving the Artifactory VCS repository governing %q: %w", githubRepo, err) - } - log.Debug(fmt.Sprintf("github-actions curation: %q is governed by Artifactory VCS repository %q", githubRepo, repo)) - return repo, nil -} - -// curatedActions assembles this run's result in the job summary's wire shape - the facts the -// report is rendered from, in one value, so the console report and the job summary are given -// the same thing rather than each being handed a different summary of it. -// -// Converted rather than shared: one set of types is this command's view, the other is what gets -// serialized, and a field added to either should have to be reconciled at compile time. -func curatedActions(rows []githubactions.ActionReportRow, attributed bool, localUses []githubactions.LocalUse) formats.CuratedActions { - actions := make([]formats.CuratedAction, 0, len(rows)) - for _, row := range rows { - // A conversion rather than a field-by-field copy: the two types are deliberately separate - - // one is this package's report row, the other the job summary's wire shape with its json - // tags - but they describe the same five columns. Converting makes them diverge at compile - // time rather than silently dropping a column from the job summary. - actions = append(actions, formats.CuratedAction(row)) - } - converted := formats.CuratedActions{Actions: actions, Attributed: attributed} - for _, use := range localUses { - converted.LocalCompositeActions = append(converted.LocalCompositeActions, - formats.LocalCompositeAction{Path: use.Raw, DeclaredBy: use.DeclaredBy}) - } - return converted -} - -// recordSummary records the report through the "security" job-summary manager -func (c *CurationActionsCommand) recordSummary(curated formats.CuratedActions) error { - return output.RecordSecurityCommandSummary(output.NewCurationActionsSummary(curated)) -} diff --git a/commands/curation/curationactions_test.go b/commands/curation/curationactions_test.go deleted file mode 100644 index b0536cafe..000000000 --- a/commands/curation/curationactions_test.go +++ /dev/null @@ -1,947 +0,0 @@ -package curation - -import ( - "bytes" - "context" - "errors" - "os" - "path/filepath" - "runtime" - "slices" - "testing" - - "github.com/jfrog/jfrog-cli-core/v2/utils/coreutils" - "github.com/jfrog/jfrog-client-go/utils/errorutils" - "github.com/jfrog/jfrog-client-go/utils/log" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - "github.com/jfrog/jfrog-cli-security/commands/curation/githubactions" -) - -const ( - curationActionsFixture = "../../tests/testdata/projects/githubactions/curation-project" - testGithubRepo = "my-org/my-repo" - // derivedWorkflowRef is the shape GITHUB_WORKFLOW_REF carries on a runner; its path component - // is repo-relative, so it resolves against the working directory. - derivedWorkflowRef = "my-org/my-repo/.github/workflows/ci.yml@refs/heads/main" -) - -// fixtureCacheEntries is what the curation-project fixture's _actions tree holds. -var fixtureCacheEntries = []string{"actions/checkout@v4", "github/codeql-action@v3", "some-org/transitive-action@v1"} - -// scriptedDecider stands in for the real decision service. It fails to decide the keys in -// undecidable, rejects the keys in rejected, approves everything else, and records what it was -// asked so a test can assert on curation SCOPE rather than only on the command's exit status. -// Keys are "owner/repo@ref". undecidable wins over rejected: no decision is not a decision. -type scriptedDecider struct { - rejected []string - undecidable []string - asked []string - // vcsRepos records the Artifactory VCS repository each decision was made under, so tests can - // prove the resolved value actually reached the decider rather than being computed and dropped. - vcsRepos []string -} - -func (d *scriptedDecider) Decide(_ context.Context, artifactoryVcsRepo string, ref githubactions.ActionRef) (githubactions.ActionCurationResult, error) { - key := ref.Owner + "/" + ref.Repo + "@" + ref.Ref - d.asked = append(d.asked, key) - d.vcsRepos = append(d.vcsRepos, artifactoryVcsRepo) - if slices.Contains(d.undecidable, key) { - return githubactions.ActionCurationResult{}, errors.New("decision service unavailable") - } - if slices.Contains(d.rejected, key) { - return githubactions.ActionCurationResult{Status: githubactions.ActionRejected, Notes: "rejected in test"}, nil - } - return githubactions.ActionCurationResult{Status: githubactions.ActionApproved}, nil -} - -// fixedResolver returns a known key, or err when the mapping API is meant to be unreachable, -// and records what it was asked about. -type fixedResolver struct { - repo string - err error - askedAbout []string -} - -func (f *fixedResolver) Resolve(_ context.Context, githubRepo string) (string, error) { - f.askedAbout = append(f.askedAbout, githubRepo) - if f.err != nil { - return "", f.err - } - return f.repo, nil -} - -// pinRunnerEnv fixes every GitHub environment variable the command reads, so a test's result -// never depends on whether it happens to be running inside GitHub Actions - where all of them -// are set, and would otherwise leak into these tests. Pass "" to represent unset. -func pinRunnerEnv(t *testing.T, githubRepo, workflowRef, jobID string) { - t.Helper() - t.Setenv(githubactions.GithubRepoEnvVar, githubRepo) - t.Setenv(githubactions.WorkflowRefEnvVar, workflowRef) - t.Setenv(githubactions.JobIDEnvVar, jobID) -} - -// workflowFileMode selects what SetWorkflowFile points at, if anything. -type workflowFileMode int - -const ( - noWorkflowFile workflowFileMode = iota // omit the flag; resolution falls to the environment - writtenWorkflowFile // the ci.yml runnerSpec wrote into the working directory - fixtureWorkflowFile // the curation-project fixture's own ci.yml - missingWorkflowFile // a path that does not exist - relativeWorkflowFile // a relative path - SetWorkflowFile must be absolute -) - -// runnerSpec describes the runner state a test starts from, as data rather than as setup code: -// what sits in the action cache, and what the workspace holds at .github/workflows/ci.yml. -type runnerSpec struct { - // fixtureCache seeds the cache from the curation-project fixture's _actions tree. - fixtureCache bool - // cacheDirs are "owner/repo/ref" entries to create on top of that. Each is an action root, so - // build writes the .completed watermark a runner would leave beside it - without one, - // discovery cannot read a ref from the entry and refuses to curate the cache. - cacheDirs []string - // cacheFiles are files to write inside the cache, keyed by "owner/repo/ref/name". - cacheFiles map[string]string - // workflowYAML, when set, is written to /.github/workflows/ci.yml - the path - // derivedWorkflowRef resolves to. - workflowYAML string - // unreadableWorkflow strips read permission from that file, for the case where the path - // resolves and opening it still fails. - unreadableWorkflow bool -} - -func (s runnerSpec) build(t *testing.T) (workingDir, actionsCacheDir string) { - t.Helper() - workingDir = t.TempDir() - actionsCacheDir = filepath.Join(t.TempDir(), "_actions") - require.NoError(t, os.MkdirAll(actionsCacheDir, 0755)) - if s.fixtureCache { - require.NoError(t, os.CopyFS(actionsCacheDir, os.DirFS(filepath.Join(curationActionsFixture, "_work", "_actions")))) - } - for _, dir := range s.cacheDirs { - path := filepath.Join(actionsCacheDir, filepath.FromSlash(dir)) - require.NoError(t, os.MkdirAll(path, 0755)) - require.NoError(t, os.WriteFile(path+".completed", []byte("ts"), 0600)) - } - for name, content := range s.cacheFiles { - path := filepath.Join(actionsCacheDir, filepath.FromSlash(name)) - require.NoError(t, os.MkdirAll(filepath.Dir(path), 0755)) - require.NoError(t, os.WriteFile(path, []byte(content), 0600)) - } - if s.workflowYAML != "" { - workflowsDir := filepath.Join(workingDir, ".github", "workflows") - require.NoError(t, os.MkdirAll(workflowsDir, 0755)) - workflowPath := filepath.Join(workflowsDir, "ci.yml") - require.NoError(t, os.WriteFile(workflowPath, []byte(s.workflowYAML), 0600)) - if s.unreadableWorkflow { - require.NoError(t, os.Chmod(workflowPath, 0000)) - // Restored so the temp dir can be cleaned up on platforms that need read access. - t.Cleanup(func() { _ = os.Chmod(workflowPath, 0600) }) - } - } - return workingDir, actionsCacheDir -} - -// newCommand builds the command every Run test exercises, wiring SetWorkflowFile per mode. -func (s runnerSpec) newCommand(t *testing.T, mode workflowFileMode, jobID string, decider githubactions.ActionCurationDecider) *CurationActionsCommand { - t.Helper() - workingDir, actionsCacheDir := s.build(t) - cmd := NewCurationActionsCommand(). - SetWorkingDir(workingDir). - SetActionsCacheDir(actionsCacheDir). - SetDecider(decider) - switch mode { - case writtenWorkflowFile: - cmd.SetWorkflowFile(filepath.Join(workingDir, ".github", "workflows", "ci.yml")) - case fixtureWorkflowFile: - // Absolute on purpose: a relative SetWorkflowFile resolves against the working directory, - // which here is a temp dir, not the package the fixture path is written relative to. - abs, err := filepath.Abs(filepath.Join(curationActionsFixture, ".github", "workflows", "ci.yml")) - require.NoError(t, err) - cmd.SetWorkflowFile(abs) - case missingWorkflowFile: - cmd.SetWorkflowFile(filepath.Join(workingDir, "no-such-workflow.yml")) - case relativeWorkflowFile: - cmd.SetWorkflowFile(filepath.Join(".github", "workflows", "ci.yml")) - case noWorkflowFile: - } - if jobID != "" { - cmd.SetJobID(jobID) - } - return cmd -} - -func TestCurationActionsCommand_Run_CurationScope(t *testing.T) { - // What actually gets decided, per resolution path. The invariant across every row: the - // runner's cache IS this job's action list, so every entry in it is decided regardless of - // what the workflow file does or does not explain. - const twoJobs = "jobs:\n" + - " build:\n steps:\n - uses: actions/checkout@v4\n" + - " publish:\n steps:\n - uses: some-other-org/publisher@v9\n" - - tests := []struct { - name string - spec runnerSpec - mode workflowFileMode - jobID string - envWorkflow string - wantAsked []string - }{ - { - name: "verify when no workflow file is identified then every cache entry is still decided", - spec: runnerSpec{fixtureCache: true}, - wantAsked: fixtureCacheEntries, - }, - { - name: "verify when an entry appears in no workflow file then it is still decided", - spec: runnerSpec{cacheDirs: []string{"some-org/unreferenced/v9"}}, - wantAsked: []string{"some-org/unreferenced@v9"}, - }, - { - name: "verify when only GITHUB_WORKFLOW_REF identifies the workflow then attribution still runs", - spec: runnerSpec{fixtureCache: true, workflowYAML: "jobs:\n build:\n steps:\n" + - " - uses: actions/checkout@v4\n - uses: github/codeql-action/analyze@v3\n"}, - jobID: "build", - envWorkflow: derivedWorkflowRef, - wantAsked: fixtureCacheEntries, - }, - { - name: "verify when a sibling job declares an action then it is decided but not attributed", - spec: runnerSpec{cacheDirs: []string{"actions/checkout/v4", "some-other-org/publisher/v9"}, workflowYAML: twoJobs}, - jobID: "build", - envWorkflow: derivedWorkflowRef, - wantAsked: []string{"actions/checkout@v4", "some-other-org/publisher@v9"}, - }, - { - name: "verify when the cache holds the action delivering this check then it is curated too", - spec: runnerSpec{ - cacheDirs: []string{"jfrog/setup-jfrog-cli/v4", "actions/checkout/v4"}, - workflowYAML: "jobs:\n build:\n steps:\n - uses: jfrog/setup-jfrog-cli@v4\n - uses: actions/checkout@v4\n", - }, - mode: writtenWorkflowFile, - jobID: "build", - wantAsked: []string{"actions/checkout@v4", "jfrog/setup-jfrog-cli@v4"}, - }, - { - // The reusable-workflow case. Attributing from the file's other jobs once curated an - // action only they declared, and dropped one this job really used. - name: "verify when the workflow does not declare this job then this cache is curated and other jobs ignored", - spec: runnerSpec{ - cacheDirs: []string{"actions/checkout/v4", "actions/setup-node/v4"}, - workflowYAML: "jobs:\n some-other-job:\n steps:\n - uses: actions/checkout@v4\n", - }, - mode: writtenWorkflowFile, - jobID: "the-job-this-command-runs-in", - wantAsked: []string{"actions/checkout@v4", "actions/setup-node@v4"}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - pinRunnerEnv(t, testGithubRepo, tt.envWorkflow, "") - decider := &scriptedDecider{} - - require.NoError(t, tt.spec.newCommand(t, tt.mode, tt.jobID, decider).Run()) - - assert.ElementsMatch(t, tt.wantAsked, decider.asked, - "every entry the runner resolved into this job's cache must be decided") - }) - } -} - -func TestCurationActionsCommand_Run_ExitStatus(t *testing.T) { - // The job either continues or it does not. A Rejected action must fail it, whether or not - // attribution could explain why that action is in the cache. - tests := []struct { - name string - spec runnerSpec - mode workflowFileMode - jobID string - rejected []string - wantErr bool - }{ - { - // SetJobID is what puts this in ATTRIBUTED mode - a workflow file alone is not enough, - // since attribution also needs to know which job it is describing. - name: "verify when every action is approved then the command succeeds", - spec: runnerSpec{fixtureCache: true}, - mode: fixtureWorkflowFile, - jobID: "build", - }, - { - name: "verify when an action is rejected then the command fails", - spec: runnerSpec{fixtureCache: true}, - mode: fixtureWorkflowFile, - rejected: []string{"some-org/transitive-action@v1"}, - wantErr: true, - }, - { - // It will execute, so failing to explain why it is there is no grounds for skipping it. - name: "verify when an entry no workflow explains is rejected then the command fails", - spec: runnerSpec{fixtureCache: true, cacheDirs: []string{"some-other-org/unexplained-action/v9"}}, - mode: fixtureWorkflowFile, - jobID: "build", - rejected: []string{"some-other-org/unexplained-action@v9"}, - wantErr: true, - }, - { - // Not "nothing to curate": the command is invoked by an action, which is itself a cache - // entry, so an empty cache means the wrong directory was read. Passing off it would be - // indistinguishable from a real pass. - name: "verify when the cache is empty then the command fails rather than reporting a pass", - spec: runnerSpec{}, - mode: fixtureWorkflowFile, - wantErr: true, - }, - { - // No action is rejected here: the cache holds an entry the walk cannot resolve to an - // identity, and curating the rest would report a clean run over an action whose status - // was never established. feature/my-branch carries no watermark while v4 does. - name: "verify when a cache entry cannot be accounted for then the command fails without deciding", - spec: runnerSpec{ - cacheDirs: []string{"actions/checkout/v4"}, - // Written through cacheFiles so the directory exists with no watermark beside it. - cacheFiles: map[string]string{"actions/checkout/feature/my-branch/action.yml": "runs:\n using: node20\n"}, - }, - mode: noWorkflowFile, - wantErr: true, - }, - { - // Dropping the unattributable entry once turned a Rejected action into a green build. - name: "verify when the workflow parses to no action reference then a rejected cache entry still fails", - spec: runnerSpec{ - cacheDirs: []string{"evil-org/backdoor/v1"}, - workflowYAML: "jobs:\n build:\n steps:\n - uses: ./.github/actions/setup\n", - }, - mode: writtenWorkflowFile, - jobID: "build", - rejected: []string{"evil-org/backdoor@v1"}, - wantErr: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - pinRunnerEnv(t, testGithubRepo, "", "") - decider := &scriptedDecider{rejected: tt.rejected} - - err := tt.spec.newCommand(t, tt.mode, tt.jobID, decider).Run() - - if tt.wantErr { - assert.Error(t, err) - return - } - assert.NoError(t, err) - }) - } -} - -func TestCurationActionsCommand_Run_UndecidableActions(t *testing.T) { - // An action that cannot be decided has an unknown status, and a report that quietly omitted - // it would read as a clean run. So the whole run fails and emits nothing - every row asserts - // the job summary directory stays empty, not just the one that motivated the rule. - twoActions := runnerSpec{cacheDirs: []string{"actions/checkout/v4", "actions/setup-node/v4"}} - - tests := []struct { - name string - spec runnerSpec - mode workflowFileMode - undecidable []string - wantErrContains []string - wantErrNotContains []string - }{ - { - name: "verify when a decision fails then the error names the action and the cause", - spec: runnerSpec{fixtureCache: true}, - mode: fixtureWorkflowFile, - undecidable: fixtureCacheEntries, - wantErrContains: []string{"deciding curation status for", "decision service unavailable"}, - }, - { - name: "verify when several decisions fail then the error names every one of them", - spec: twoActions, - undecidable: []string{"actions/checkout@v4", "actions/setup-node@v4"}, - wantErrContains: []string{"actions/checkout@v4", "actions/setup-node@v4"}, - }, - { - name: "verify when only one decision fails then the error names it alone", - spec: twoActions, - undecidable: []string{"actions/setup-node@v4"}, - wantErrContains: []string{"actions/setup-node@v4"}, - wantErrNotContains: []string{"actions/checkout@v4"}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - pinRunnerEnv(t, testGithubRepo, "", "") - // Recording is a no-op unless this is set, so set it and assert nothing lands there. - summaryDir := t.TempDir() - t.Setenv(coreutils.SummaryOutputDirPathEnv, summaryDir) - decider := &scriptedDecider{undecidable: tt.undecidable} - - err := tt.spec.newCommand(t, tt.mode, "", decider).Run() - - require.Error(t, err) - for _, want := range tt.wantErrContains { - assert.ErrorContains(t, err, want) - } - for _, notWant := range tt.wantErrNotContains { - assert.NotContains(t, err.Error(), notWant) - } - entries, readErr := os.ReadDir(summaryDir) - require.NoError(t, readErr) - assert.Empty(t, entries, "no job summary may be recorded when an action could not be decided") - }) - } -} - -func TestCurationActionsCommand_Run_AttributedAndStructureOnlyCurateTheSameSet(t *testing.T) { - // The two modes differ in report detail, never in coverage. Same cache, same job: both must - // decide every entry, including one no workflow references. - spec := runnerSpec{fixtureCache: true, cacheDirs: []string{"some-org/unreferenced/v9"}} - wantAsked := append(slices.Clone(fixtureCacheEntries), "some-org/unreferenced@v9") - - tests := []struct { - name string - mode workflowFileMode - jobID string - }{ - {name: "verify when a workflow file is supplied then every cache entry is decided", mode: fixtureWorkflowFile, jobID: "build"}, - {name: "verify when no workflow file is supplied then the same entries are decided", mode: noWorkflowFile}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - pinRunnerEnv(t, testGithubRepo, "", "") - decider := &scriptedDecider{} - - require.NoError(t, spec.newCommand(t, tt.mode, tt.jobID, decider).Run()) - - assert.ElementsMatch(t, wantAsked, decider.asked) - }) - } -} - -func TestCurationActionsCommand_Run_WorkflowFileResolution(t *testing.T) { - // The branches of parseWorkflowUses, exercised through Run in the shape a real runner - // produces: _actions populated, workspace empty because checkout has not run. - oneAction := []string{"actions/checkout/v4"} - - tests := []struct { - name string - spec runnerSpec - mode workflowFileMode - workflowRef string - wantAsked []string - wantErrContains string - }{ - { - // GITHUB_WORKFLOW_REF is always set on a runner, so without this fallback every job - // that did not pass SetWorkflowFile would fail here. - name: "verify when the derived workflow path is absent then curation falls back to structure-only", - spec: runnerSpec{cacheDirs: oneAction}, - workflowRef: derivedWorkflowRef, - wantAsked: []string{"actions/checkout@v4"}, - }, - { - name: "verify when an explicit workflow path is absent then curation falls back to structure-only", - spec: runnerSpec{cacheDirs: oneAction}, - mode: missingWorkflowFile, - wantAsked: []string{"actions/checkout@v4"}, - }, - { - // The one thing that still fails: a malformed flag, rejected before anything is read. - // SetWorkflowFile is an explicit, single-file assertion; only the derived path - // (GITHUB_WORKFLOW_REF) resolves against the working directory. - name: "verify when an explicit workflow path is relative then the command fails", - spec: runnerSpec{cacheDirs: oneAction}, - mode: relativeWorkflowFile, - wantErrContains: "must be an absolute path", - }, - { - // A workflow this parser cannot read costs attribution and nothing else, so the cache - // is still curated in full. - name: "verify when the derived workflow file is malformed then curation falls back to structure-only", - spec: runnerSpec{cacheDirs: oneAction, workflowYAML: "jobs:\n\t- this is not valid yaml\n"}, - workflowRef: derivedWorkflowRef, - wantAsked: []string{"actions/checkout@v4"}, - }, - { - // Naming the file asserts it exists, not that this parser can read it - so the same - // divergence degrades the same way whichever route resolved the path. - name: "verify when an explicit workflow file is malformed then curation falls back to structure-only", - spec: runnerSpec{cacheDirs: oneAction, workflowYAML: "jobs:\n\t- this is not valid yaml\n"}, - mode: writtenWorkflowFile, - wantAsked: []string{"actions/checkout@v4"}, - }, - { - name: "verify when the derived workflow file is valid then attribution runs normally", - spec: runnerSpec{cacheDirs: oneAction, workflowYAML: "jobs:\n build:\n steps:\n - uses: actions/checkout@v4\n"}, - workflowRef: derivedWorkflowRef, - wantAsked: []string{"actions/checkout@v4"}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - pinRunnerEnv(t, testGithubRepo, tt.workflowRef, "build") - decider := &scriptedDecider{} - - err := tt.spec.newCommand(t, tt.mode, "", decider).Run() - - if tt.wantErrContains != "" { - assert.ErrorContains(t, err, tt.wantErrContains) - assert.Empty(t, decider.asked) - return - } - require.NoError(t, err) - assert.Equal(t, tt.wantAsked, decider.asked, - "every entry in the cache must be curated, whichever branch resolution took") - }) - } -} - -func TestCurationActionsCommand_Run_ArtifactoryVcsRepoResolution(t *testing.T) { - spec := runnerSpec{cacheDirs: []string{"actions/checkout/v4", "actions/setup-node/v4"}} - - tests := []struct { - name string - envRepo string - flagRepo string - resolverRepo string - resolverErr error - // wantAskedAbout is what the mapping API was called with - once per run, never per action. - wantAskedAbout []string - // wantVcsRepos is the repository that reached each decision, so a resolved value that is - // computed and then dropped fails here rather than passing silently. - wantVcsRepos []string - wantErrContains []string - }{ - { - name: "verify when GITHUB_REPOSITORY is set then the resolved repository reaches every decision", - envRepo: testGithubRepo, - resolverRepo: "my-org-github-remote", - wantAskedAbout: []string{testGithubRepo}, - wantVcsRepos: []string{"my-org-github-remote", "my-org-github-remote"}, - }, - { - name: "verify when the repository is set explicitly then it overrides the environment", - envRepo: testGithubRepo, - flagRepo: "flag-org/flag-repo", - resolverRepo: "resolved", - wantAskedAbout: []string{"flag-org/flag-repo"}, - wantVcsRepos: []string{"resolved", "resolved"}, - }, - { - name: "verify when repository resolution fails then the command fails and nothing is decided", - envRepo: testGithubRepo, - resolverErr: errors.New("mapping service unavailable"), - wantAskedAbout: []string{testGithubRepo}, - wantErrContains: []string{"resolving the Artifactory VCS repository governing", "mapping service unavailable"}, - }, - { - name: "verify when no GitHub repository is known then the error names the variable that supplies it", - envRepo: "", // GITHUB_REPOSITORY unset, e.g. a local run - wantErrContains: []string{githubactions.GithubRepoEnvVar}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - pinRunnerEnv(t, tt.envRepo, "", "") - resolver := &fixedResolver{repo: tt.resolverRepo, err: tt.resolverErr} - decider := &scriptedDecider{} - cmd := spec.newCommand(t, noWorkflowFile, "", decider).SetVcsRepoResolver(resolver) - if tt.flagRepo != "" { - cmd.SetGithubRepo(tt.flagRepo) - } - - err := cmd.Run() - - assert.Equal(t, tt.wantAskedAbout, resolver.askedAbout, "resolution happens once per run, not once per action") - if len(tt.wantErrContains) > 0 { - for _, want := range tt.wantErrContains { - assert.ErrorContains(t, err, want) - } - assert.Empty(t, decider.asked, "nothing may be decided when the governing repository is unknown") - return - } - require.NoError(t, err) - assert.Equal(t, tt.wantVcsRepos, decider.vcsRepos, "the resolved repository must reach every decision") - }) - } -} - -func TestCurationActionsCommand_Run_EmptyCacheFailsBeforeResolving(t *testing.T) { - pinRunnerEnv(t, testGithubRepo, "", "") - resolver := &fixedResolver{repo: "unused"} - - err := runnerSpec{}.newCommand(t, noWorkflowFile, "", &scriptedDecider{}). - SetVcsRepoResolver(resolver).Run() - - assert.ErrorContains(t, err, githubactions.RunnerWorkspaceEnvVar, - "the failure must name the variable the cache location comes from") - assert.Empty(t, resolver.askedAbout, "a cache that cannot be read must fail before any mapping call") -} - -func TestCurationActionsCommand_Run_ActionsAttributionCannotExplainAreStillCurated(t *testing.T) { - // The two ways the runner's cache can hold an entry this command cannot trace back to a - // uses: line. Both once caused the entry to be dropped, so the job passed with an action - // that executes having never been decided. - tests := []struct { - name string - spec runnerSpec - }{ - { - name: "verify when a local composite action's child is already in the cache then it is still decided", - // uses: ./... is read from the workspace, so there is nothing to walk outward from - - // the entry is unattributable, and must still be curated. This covers only the case - // where the runner had already resolved the child by the time this command ran; the - // case where it has not is TestCurationActionsCommand_Run_LocalCompositeActionIsDeclaredUncovered. - spec: runnerSpec{ - cacheDirs: []string{"actions/setup-node/v4"}, - workflowYAML: "jobs:\n build:\n steps:\n - uses: ./.github/actions/setup\n", - }, - }, - { - name: "verify when a composite action.yml cannot be parsed then its child is still decided", - // Duplicate mapping keys: accepted by GitHub's runner, rejected by yaml.v3. - spec: runnerSpec{ - cacheDirs: []string{"actions/setup-node/v4", "some-org/wrapper/v1"}, - cacheFiles: map[string]string{ - "some-org/wrapper/v1/action.yml": "name: w\nname: w\nruns:\n using: composite\n steps:\n - uses: actions/setup-node@v4\n", - }, - workflowYAML: "jobs:\n build:\n steps:\n - uses: some-org/wrapper@v1\n", - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - pinRunnerEnv(t, testGithubRepo, "", "") - decider := &scriptedDecider{} - - require.NoError(t, tt.spec.newCommand(t, writtenWorkflowFile, "build", decider).Run()) - - assert.Contains(t, decider.asked, "actions/setup-node@v4", - "an action the runner resolved will execute, so it must be decided even when it cannot be attributed") - }) - } -} - -// captureReport runs cmd with the logger redirected, and returns everything it wrote. The -// report and its caveat only exist as log output, so that is where a test has to read them. -func captureReport(t *testing.T, cmd *CurationActionsCommand) (report string, err error) { - t.Helper() - var buf bytes.Buffer - original := log.Logger - log.SetLogger(log.NewLogger(log.INFO, &buf)) - defer log.SetLogger(original) - err = cmd.Run() - return buf.String(), err -} - -func TestCurationActionsCommand_Run_LocalCompositeActionIsDeclaredUncovered(t *testing.T) { - // The gap this caveat exists for, in the shape that actually bites: the job declares a local - // composite action, and the action it pulls in is NOT in the cache, because the runner cannot - // resolve a local action's own references until the workspace is checked out - it downloads - // them when that step runs, after this command has read the cache. - // - // Nothing here can be decided, so the command is right to pass. What it must not do is let an - // all-Approved table stand as the whole account of what the job will execute. - pinRunnerEnv(t, testGithubRepo, "", "") - decider := &scriptedDecider{} - spec := runnerSpec{ - cacheDirs: []string{"actions/checkout/v4"}, - workflowYAML: "jobs:\n build:\n steps:\n" + - " - uses: actions/checkout@v4\n" + - " - uses: ./.github/actions/setup\n", - } - - report, err := captureReport(t, spec.newCommand(t, writtenWorkflowFile, "build", decider)) - - require.NoError(t, err, "nothing was rejected, so the gate is right to open - the caveat is what keeps that honest") - assert.Equal(t, []string{"actions/checkout@v4"}, decider.asked, - "the local action's own references are not in the cache yet, so there is nothing more to decide") - assert.Contains(t, report, "./.github/actions/setup", - "the report must name the local step whose references it never saw") - assert.Contains(t, report, "not curated here") -} - -func TestCurationActionsCommand_Run_LocalStepInsideACompositeActionIsDeclaredUncovered(t *testing.T) { - // One level down from the workflow: a third-party composite action in the cache declares - // "uses: ./...". That path resolves against this repository's checkout, not the action's own - // directory, so the runner fetches whatever it references when the step runs - after this - // command read the cache. The composite itself is curated; what it reaches that way is not. - pinRunnerEnv(t, testGithubRepo, "", "") - decider := &scriptedDecider{} - spec := runnerSpec{ - cacheDirs: []string{"some-org/wrapper/v1"}, - cacheFiles: map[string]string{ - "some-org/wrapper/v1/action.yml": "runs:\n using: composite\n steps:\n - uses: ./.github/actions/build\n", - }, - workflowYAML: "jobs:\n build:\n steps:\n - uses: some-org/wrapper@v1\n", - } - - report, err := captureReport(t, spec.newCommand(t, writtenWorkflowFile, "build", decider)) - - require.NoError(t, err) - assert.Equal(t, []string{"some-org/wrapper@v1"}, decider.asked, "the composite action itself is still curated") - assert.Contains(t, report, "./.github/actions/build") - assert.Contains(t, report, "declared by some-org/wrapper@v1", - "the path names a directory in this repository, so the report has to say which action reached it") -} - -func TestCurationActionsCommand_Run_CoverageCaveatPerResolutionPath(t *testing.T) { - // The caveat has to track what was actually knowable on each path, not merely appear. - tests := []struct { - name string - spec runnerSpec - mode workflowFileMode - jobID string - wantContains []string - wantNotContains []string - }{ - { - name: "verify when the workflow declares no local action then the report claims full coverage", - spec: runnerSpec{ - cacheDirs: []string{"actions/checkout/v4"}, - workflowYAML: "jobs:\n build:\n steps:\n - uses: actions/checkout@v4\n", - }, - mode: writtenWorkflowFile, - jobID: "build", - wantNotContains: []string{"Not covered"}, - }, - { - name: "verify when no workflow file is available then the caveat is unconditional", - // The ordinary case on a runner: the check runs before the checkout, so the file is - // not on disk and the command cannot tell whether a local action is declared. - spec: runnerSpec{cacheDirs: []string{"actions/checkout/v4"}}, - mode: noWorkflowFile, - wantContains: []string{"Not covered", "no workflow file was available"}, - }, - { - name: "verify when the workflow does not declare the job then the caveat is unconditional", - // Attribution failed for a different reason, but the command knows exactly as little. - spec: runnerSpec{ - cacheDirs: []string{"actions/checkout/v4"}, - workflowYAML: "jobs:\n publish:\n steps:\n - uses: actions/checkout@v4\n", - }, - mode: writtenWorkflowFile, - jobID: "build", - wantContains: []string{"Not covered", "no workflow file was available"}, - }, - { - name: "verify when a composite declares a local step then the caveat names it and its declarer", - spec: runnerSpec{ - cacheDirs: []string{"some-org/wrapper/v1"}, - cacheFiles: map[string]string{ - "some-org/wrapper/v1/action.yml": "runs:\n using: composite\n steps:\n - uses: ./scripts/build\n", - }, - workflowYAML: "jobs:\n build:\n steps:\n - uses: some-org/wrapper@v1\n", - }, - mode: writtenWorkflowFile, - jobID: "build", - wantContains: []string{"Not covered", "./scripts/build", "declared by some-org/wrapper@v1"}, - }, - { - name: "verify when attribution is unavailable then a composite's local step is not detected at all", - // Detection needs the walk, and the walk needs a workflow file. Without one the - // command genuinely does not know - which is why that caveat is unconditional rather - // than a list, and why it must not name a path it never read. - spec: runnerSpec{ - cacheDirs: []string{"some-org/wrapper/v1"}, - cacheFiles: map[string]string{ - "some-org/wrapper/v1/action.yml": "runs:\n using: composite\n steps:\n - uses: ./scripts/build\n", - }, - }, - mode: noWorkflowFile, - wantContains: []string{"Not covered", "no workflow file was available"}, - wantNotContains: []string{"./scripts/build"}, - }, - { - name: "verify when a sibling job declares the local action then this job does not claim it", - // Each job runs on its own runner. A local step in another job says nothing about the - // coverage of this one, and naming it here would be a false lead. - spec: runnerSpec{ - cacheDirs: []string{"actions/checkout/v4"}, - workflowYAML: "jobs:\n" + - " build:\n steps:\n - uses: actions/checkout@v4\n" + - " publish:\n steps:\n - uses: ./.github/actions/release\n", - }, - mode: writtenWorkflowFile, - jobID: "build", - wantNotContains: []string{"Not covered", "./.github/actions/release"}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - pinRunnerEnv(t, testGithubRepo, "", "") - - report, err := captureReport(t, tt.spec.newCommand(t, tt.mode, tt.jobID, &scriptedDecider{})) - - require.NoError(t, err) - for _, want := range tt.wantContains { - assert.Contains(t, report, want) - } - for _, notWant := range tt.wantNotContains { - assert.NotContains(t, report, notWant) - } - }) - } -} - -func TestCurationActionsCommand_Run_EveryParentNamesARowInTheTable(t *testing.T) { - // Parent is a cross-reference into the report's own rows, so a row it names has to be there. - // The action delivering this check is the one that could break that: it is a parent like any - // other, and it is curated like any other rather than being filtered out of the table after - // attribution has already pointed at it. - pinRunnerEnv(t, testGithubRepo, "", "") - decider := &scriptedDecider{} - spec := runnerSpec{ - cacheDirs: []string{"jfrog/setup-jfrog-cli/v4", "some-org/pulled-by-delivery/v1"}, - cacheFiles: map[string]string{ - "jfrog/setup-jfrog-cli/v4/action.yml": "runs:\n using: composite\n steps:\n - uses: some-org/pulled-by-delivery@v1\n", - }, - workflowYAML: "jobs:\n build:\n steps:\n - uses: jfrog/setup-jfrog-cli@v4\n", - } - - report, err := captureReport(t, spec.newCommand(t, writtenWorkflowFile, "build", decider)) - - require.NoError(t, err) - assert.ElementsMatch(t, []string{"jfrog/setup-jfrog-cli@v4", "some-org/pulled-by-delivery@v1"}, decider.asked, - "no action is exempt from the gate, including the one that installed this CLI") - // The child names its parent, and that parent has a row of its own above it. - assert.Contains(t, report, "| some-org/pulled-by-delivery | v1 | jfrog/setup-jfrog-cli@v4 |") - assert.Contains(t, report, "| jfrog/setup-jfrog-cli | v4 |") -} - -func TestCurationActionsCommand_Run_UnreadableWorkflowFileFallsBackRatherThanFailing(t *testing.T) { - // The path resolves and opening it still fails - a permission error rather than an absent - // file. It reaches the command as neither ErrNotExist nor a parse error, which is the one - // route that used to abort the run. Curation is the job, so it degrades like every other - // workflow-file problem. - if runtime.GOOS == "windows" { - t.Skip("os.Chmod on Windows only toggles the read-only attribute, so reads are not denied and the failure cannot be produced") - } - if os.Geteuid() == 0 { - t.Skip("running as root: file permissions do not deny access, so the failure cannot be produced") - } - pinRunnerEnv(t, testGithubRepo, derivedWorkflowRef, "") - decider := &scriptedDecider{} - spec := runnerSpec{ - cacheDirs: []string{"actions/checkout/v4"}, - workflowYAML: "jobs:\n build:\n steps:\n - uses: actions/checkout@v4\n", - unreadableWorkflow: true, - } - - report, err := captureReport(t, spec.newCommand(t, noWorkflowFile, "build", decider)) - - require.NoError(t, err, "a job gated on this command must not fail because the workflow file could not be opened") - assert.Equal(t, []string{"actions/checkout@v4"}, decider.asked, "the cache is curated in full regardless") - // Attribution is gone, and the report says so rather than passing silently. - assert.NotContains(t, report, "| Action | Ref | Parent |", "a run with no attribution must not render a Parent column") - assert.Contains(t, report, "no workflow file was available") -} - -func TestCurationActionsCommand_Run_ErrorHandlingHookAppliesToFailuresNotOutcomes(t *testing.T) { - // errorutils.CheckError is a hook, not a wrapper: it is the identity function until - // JFROG_CLI_ERROR_HANDLING=panic replaces it with one that panics at the error site, which - // is how this repository gets a stack trace pointing at an origin. That makes WHERE it is - // applied the whole decision - an operational failure should reach it, while a normal - // outcome expressed as an error must not, or debugging with that flag set turns working - // runs into panics. - origCheckError := errorutils.CheckError - errorutils.CheckError = func(err error) error { - if err != nil { - panic(err) - } - return nil - } - defer func() { errorutils.CheckError = origCheckError }() - - tests := []struct { - name string - spec runnerSpec - mode workflowFileMode - rejected []string - undecidable []string - unresolvableVcs bool - wantPanic bool - wantErr bool - }{ - { - // The gate's verdict. The command worked exactly as designed; the error is only how - // a rejection reaches the exit code. - name: "verify when an action is rejected then the verdict is not treated as a failure", - spec: runnerSpec{cacheDirs: []string{"evil-org/backdoor/v1"}}, - mode: noWorkflowFile, - rejected: []string{"evil-org/backdoor@v1"}, - wantErr: true, - }, - { - // The ordinary case on a runner: no checkout yet, so the workflow file is absent and - // the run degrades to structure-only. Nothing failed. - name: "verify when the workflow file is absent then degrading is not treated as a failure", - spec: runnerSpec{cacheDirs: []string{"actions/checkout/v4"}}, - mode: missingWorkflowFile, - }, - { - // A remote seam. Reaching no verdict is an anticipated condition of calling a - // service, and the fail-open / fail-close setting is what decides its consequence - - // so it must not crash, least of all on a transient fault. - name: "verify when a decision cannot be reached then the remote failure is not treated as a failure of this command", - spec: runnerSpec{cacheDirs: []string{"actions/checkout/v4"}}, - mode: noWorkflowFile, - undecidable: []string{"actions/checkout@v4"}, - wantErr: true, - }, - { - // The other remote seam, for the same reason. - name: "verify when the vcs repository cannot be resolved then the remote failure does not crash", - spec: runnerSpec{cacheDirs: []string{"actions/checkout/v4"}}, - mode: noWorkflowFile, - unresolvableVcs: true, - wantErr: true, - }, - { - // An operational failure: the cache holds an entry that cannot be resolved to an - // action, so the run cannot be reported as curated. This one must reach the hook. - name: "verify when the cache cannot be accounted for then the failure reaches the hook", - spec: runnerSpec{ - cacheDirs: []string{"actions/checkout/v4"}, - cacheFiles: map[string]string{"actions/checkout/feature/my-branch/action.yml": "runs:\n using: node20\n"}, - }, - mode: noWorkflowFile, - wantPanic: true, - }, - { - // A malformed flag, rejected before anything is read. - name: "verify when the workflow path is relative then the usage error reaches the hook", - spec: runnerSpec{cacheDirs: []string{"actions/checkout/v4"}}, - mode: relativeWorkflowFile, - wantPanic: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - pinRunnerEnv(t, testGithubRepo, "", "") - run := func() error { - cmd := tt.spec.newCommand(t, tt.mode, "", - &scriptedDecider{rejected: tt.rejected, undecidable: tt.undecidable}) - if tt.unresolvableVcs { - cmd.SetVcsRepoResolver(&fixedResolver{err: errors.New("mapping API unreachable")}) - } - return cmd.Run() - } - - if tt.wantPanic { - assert.Panics(t, func() { _ = run() }) - return - } - var err error - require.NotPanics(t, func() { err = run() }) - if tt.wantErr { - assert.Error(t, err) - return - } - assert.NoError(t, err) - }) - } -} diff --git a/commands/curation/githubactions/decision.go b/commands/curation/githubactions/decision.go deleted file mode 100644 index 193fe7733..000000000 --- a/commands/curation/githubactions/decision.go +++ /dev/null @@ -1,37 +0,0 @@ -package githubactions - -import "context" - -// ActionCurationStatus is the curation outcome for one action. -type ActionCurationStatus string - -const ( - ActionApproved ActionCurationStatus = "Approved" - ActionRejected ActionCurationStatus = "Rejected" -) - -// ActionCurationResult is the decision for one resolved action. -type ActionCurationResult struct { - Status ActionCurationStatus - Notes string -} - -// ActionCurationDecider decides the curation outcome for a single action reference. Only the -// mock implementation exists till support exists at Artifactory/Catalog. -// -// An implementation must normalize ref before looking it up: lower-case Owner and Repo, and a Ref -// that is a hex SHA. Discovery reports what the cache directory is named, and the runner names it -// verbatim from the uses: line, so one action reaches Decide under as many identities as the -// workflow spelled it. Measured on a hosted runner: `uses: Actions/Checkout@v4` alongside -// `uses: actions/checkout@v4` produces both _actions/Actions/Checkout/v4 and -// _actions/actions/checkout/v4, and the same commit pinned in upper- and lower-case hex produces -// two directories likewise. GitHub resolves either spelling, so both are the same action to -// curate - but a case-sensitive catalog lookup would give one of them a different verdict, or no -// verdict at all, and fail a job over a spelling. Normalizing here rather than in discovery keeps -// the verbatim casing that attribution matches uses: lines on (see refKey in workflow.go). -type ActionCurationDecider interface { - // Decide returns the curation outcome for one action reference under the policies of - // artifactoryVcsRepo. A non-nil error means no decision was reached - distinct from - // Rejected, and fatal to the command. - Decide(ctx context.Context, artifactoryVcsRepo string, ref ActionRef) (ActionCurationResult, error) -} diff --git a/commands/curation/githubactions/decision_mock.go b/commands/curation/githubactions/decision_mock.go deleted file mode 100644 index 603df6bcf..000000000 --- a/commands/curation/githubactions/decision_mock.go +++ /dev/null @@ -1,31 +0,0 @@ -package githubactions - -import ( - "context" - "fmt" -) - -// mockActionCurationDecider stands in for the decision service, which does not exist yet. The -// parity of the action name's length decides the outcome (even -> Approved, odd -> Rejected), -// A Rejected result is simply recorded and once CVS is implemented content is overridden with compliant -// version. Docker based actions need special handling: the runner pulls or -// builds the action's image during job setup, before curation decides anything here, so -// selecting a compliant version also means rebuilding that runner-built image and replacing -// otherwise the job goes on running the image produced from the version CVS moved off. -type mockActionCurationDecider struct{} - -// NewMockActionCurationDecider returns the name-parity stand-in decider. -func NewMockActionCurationDecider() ActionCurationDecider { - return mockActionCurationDecider{} -} - -func (mockActionCurationDecider) Decide(_ context.Context, _ string, ref ActionRef) (ActionCurationResult, error) { - action := ref.Owner + "/" + ref.Repo - if len(action)%2 == 0 { - return ActionCurationResult{Status: ActionApproved}, nil - } - return ActionCurationResult{ - Status: ActionRejected, - Notes: fmt.Sprintf("mock decision: rejected %s@%s", action, ref.Ref), - }, nil -} diff --git a/commands/curation/githubactions/decision_mock_test.go b/commands/curation/githubactions/decision_mock_test.go deleted file mode 100644 index 802fbc060..000000000 --- a/commands/curation/githubactions/decision_mock_test.go +++ /dev/null @@ -1,54 +0,0 @@ -package githubactions - -import ( - "context" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestMockActionCurationDecider(t *testing.T) { - decider := NewMockActionCurationDecider() - - tests := []struct { - name string - ref ActionRef - wantStatus ActionCurationStatus - wantNotes bool - }{ - { - name: "verify when the action name has an even length then it is approved", - ref: ActionRef{Owner: "actions", Repo: "checkout", Ref: "v4"}, // "actions/checkout" is 16 - wantStatus: ActionApproved, - }, - { - name: "verify when the action name has an odd length then it is rejected", - ref: ActionRef{Owner: "actions", Repo: "cache", Ref: "v3"}, // "actions/cache" is 13 - wantStatus: ActionRejected, - wantNotes: true, - }, - { - name: "verify when only the ref differs then the decision is unchanged", - ref: ActionRef{Owner: "actions", Repo: "checkout", Ref: "8f4b7f8"}, - wantStatus: ActionApproved, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := decider.Decide(context.Background(), "vcs-repo", tt.ref) - require.NoError(t, err) - - assert.Equal(t, tt.wantStatus, got.Status) - if tt.wantNotes { - assert.NotEmpty(t, got.Notes, "a rejection must say why") - } else { - assert.Empty(t, got.Notes) - } - - again, err := decider.Decide(context.Background(), "vcs-repo", tt.ref) - require.NoError(t, err) - assert.Equal(t, got, again, "the same action must decide the same way on every call") - }) - } -} diff --git a/commands/curation/githubactions/discovery.go b/commands/curation/githubactions/discovery.go deleted file mode 100644 index 1f445aaef..000000000 --- a/commands/curation/githubactions/discovery.go +++ /dev/null @@ -1,361 +0,0 @@ -package githubactions - -import ( - "errors" - "fmt" - "os" - "path/filepath" - "strings" - - "github.com/jfrog/jfrog-client-go/utils/errorutils" - "github.com/jfrog/jfrog-client-go/utils/log" -) - -// What GitHub Actions sets on every runner. -const ( - // RunnerWorkspaceEnvVar is the workspace directory, e.g. /home/runner/work/, whose - // sibling is _actions. - RunnerWorkspaceEnvVar = "RUNNER_WORKSPACE" - // WorkflowRefEnvVar is the running workflow's ref path, e.g. - // "octocat/hello-world/.github/workflows/ci.yml@main". - WorkflowRefEnvVar = "GITHUB_WORKFLOW_REF" - // JobIDEnvVar is the running job's job_id - its key under `jobs:` in the workflow YAML. - JobIDEnvVar = "GITHUB_JOB" - // GithubRepoEnvVar is the repository running the job, as "/". - GithubRepoEnvVar = "GITHUB_REPOSITORY" -) - -const errCacheNotReadable = "cannot read the GitHub Actions cache derived from " + RunnerWorkspaceEnvVar + ", so this job " + - "cannot be reported as curated - this command must run as a step on a GitHub Actions runner, where the action " + - "invoking it has already resolved" - -// ErrCacheNotReadable reports a cache that is absent, or that resolved to no action at all -func ErrCacheNotReadable() error { - return errorutils.CheckError(errors.New(errCacheNotReadable)) -} - -// ActionRef is one resolved action instance found in the runner's action cache. -type ActionRef struct { - Owner string - Repo string - // Ref is taken verbatim from the cache directory name; it may be a SHA, tag or branch. - Ref string - // Path is the absolute path to _work/_actions///. - Path string - // Subpaths holds every distinct subpath the job invoked this action through - a monorepo - // action such as github/codeql-action can be used via several from one owner/repo/ref. - Subpaths []string - // Parent is the composite action that pulled this one in, "" when unattributed. - Parent string -} - -// UnaccountedEntry is a cache entry that exists but could not be resolved to an action. -type UnaccountedEntry struct { - Path string - Reason string -} - -// ActionCacheScan is what one walk of the runner's action cache found. -type ActionCacheScan struct { - // Refs holds one entry per action the walk resolved. - Refs []ActionRef - // Unaccounted holds the entries it could not. - Unaccounted []UnaccountedEntry -} - -// UnaccountedError returns an error naming every entry the walk could not resolve, or nil when -// there are none. -func (s ActionCacheScan) UnaccountedError() error { - if len(s.Unaccounted) == 0 { - return nil - } - var msg strings.Builder - msg.WriteString("cannot account for every entry in the runner's action cache, so this job cannot be reported as curated:") - for _, entry := range s.Unaccounted { - fmt.Fprintf(&msg, "\n %s: %s", entry.Path, entry.Reason) - } - return errorutils.CheckError(errors.New(msg.String())) -} - -// DiscoverActionCache walks actionsCacheDir (the runner's _work/_actions root) and returns one -// ActionRef per action the runner resolved. -// -// The runner downloads resolved actions here before the job's steps run, including transitive -// ones pulled in by another action's action.yml that never appear in the job's own workflow -// file - so the directory is the account of what actually resolved. -// -// The layout is //, but is a git ref and may contain "/" - a branch such -// as copilot/backport-v4 lands at //copilot/backport-v4. So the depth of a ref is -// not fixed, and the walk asks the runner where each one ends. -// -// An entry the walk understands but that holds no action - a stray file, an owner directory with -// no repositories, a watermark - is skipped. An entry it cannot examine, or cannot resolve to a -// ref, is recorded in Unaccounted instead. -func DiscoverActionCache(actionsCacheDir string) (ActionCacheScan, error) { - scan := ActionCacheScan{Refs: []ActionRef{}} - - ownerEntries, err := os.ReadDir(actionsCacheDir) - if err != nil { - if os.IsNotExist(err) { - return ActionCacheScan{}, errorutils.CheckError(errors.New(errCacheNotReadable)) - } - return ActionCacheScan{}, errorutils.CheckError(fmt.Errorf("reading the GitHub Actions cache derived from %s: %w", RunnerWorkspaceEnvVar, err)) - } - - for _, ownerEntry := range ownerEntries { - owner := ownerEntry.Name() - ownerPath := filepath.Join(actionsCacheDir, owner) - walkable, err := walkableDir(ownerPath, "owner") - if err != nil { - scan.Unaccounted = append(scan.Unaccounted, unresolvedEntry(ownerPath, err)) - } - if !walkable { - continue - } - - repoEntries, err := os.ReadDir(ownerPath) - if err != nil { - scan.Unaccounted = append(scan.Unaccounted, unlistableEntry(ownerPath, err)) - continue - } - for _, repoEntry := range repoEntries { - repo := repoEntry.Name() - repoPath := filepath.Join(ownerPath, repo) - walkable, err := walkableDir(repoPath, "repo") - if err != nil { - scan.Unaccounted = append(scan.Unaccounted, unresolvedEntry(repoPath, err)) - } - if !walkable { - continue - } - - roots, unaccounted, err := scanRepo(repoPath) - if err != nil { - scan.Unaccounted = append(scan.Unaccounted, unlistableEntry(repoPath, err)) - continue - } - scan.Unaccounted = append(scan.Unaccounted, unaccounted...) - for _, root := range roots { - scan.Refs = append(scan.Refs, ActionRef{Owner: owner, Repo: repo, Ref: root.ref, Path: root.path}) - } - } - } - - // Distinct from Unaccounted: every entry here was understood, and none of them held an action. - // Odd enough in a populated cache to say once, and cheaper than a per-entry log nobody reads. - if len(scan.Refs) == 0 && len(scan.Unaccounted) == 0 && len(ownerEntries) > 0 { - log.Warn(fmt.Sprintf("github-actions curation: the cache holds %d entries but none resolved to an // action, "+ - "so nothing will be curated. The debug log names every entry that was skipped.", len(ownerEntries))) - } - return scan, nil -} - -// watermarkSuffix names the file the runner writes beside an action it extracted: -// _actions///.completed, at whatever depth lands. -const watermarkSuffix = ".completed" - -// actionRoot is one resolved action: the ref that names it, and the directory holding it. -type actionRoot struct { - ref string - path string -} - -// scanRepo finds the action roots under one / directory, and the entries under it -// that no action root could be made of. -func scanRepo(repoPath string) (roots []actionRoot, unaccounted []UnaccountedEntry, err error) { - refEntries, err := os.ReadDir(repoPath) - if err != nil { - return nil, nil, err - } - watermarked := watermarkedNames(refEntries) - for _, refEntry := range refEntries { - if isWatermarkFile(refEntry) { - continue - } - refPath := filepath.Join(repoPath, refEntry.Name()) - walkable, statErr := walkableDir(refPath, "ref") - if statErr != nil { - unaccounted = append(unaccounted, unresolvedEntry(refPath, statErr)) - } - if !walkable { - continue - } - entryRoots, entryUnaccounted := actionRootsUnder(refPath, refEntry.Name(), watermarked) - unaccounted = append(unaccounted, entryUnaccounted...) - switch { - case len(entryRoots) > 0: - roots = append(roots, entryRoots...) - case len(entryUnaccounted) > 0: - // Already recorded, with the reason the descent stopped. Naming it again for a missing - // marker would report one entry twice and blame the marker for a failure to read. - default: - unaccounted = append(unaccounted, UnaccountedEntry{ - Path: refPath, - Reason: fmt.Sprintf("nothing at or below it carries a %s marker or is a symlink, so no action ref can be read from it", watermarkSuffix), - }) - } - } - return roots, unaccounted, nil -} - -// actionRootsUnder returns every action root at or below dir, with ref accumulated from the path -// segments walked to reach it. It returns no roots when no marker is found, leaving what that -// means to the caller. -// -// watermarked holds the names in dir's own parent that carry a marker, so dir's root-ness comes -// out of a listing the caller already read. -// -// The descent is bounded by the cache's own shape rather than by a depth limit: a ref's -// intermediate directories hold nothing but the next segment, so the first directory holding -// content of its own is where a ref can no longer continue. A guess at a maximum segment count -// would instead have to be wrong in one direction or the other - stopping short of a marker on a -// legitimately deep branch ref, or walking several levels into an action that carries no marker. -func actionRootsUnder(dir, ref string, watermarked map[string]bool) (roots []actionRoot, unaccounted []UnaccountedEntry) { - if isActionRoot(dir, watermarked) { - return []actionRoot{{ref: ref, path: dir}}, nil - } - entries, err := os.ReadDir(dir) - if err != nil { - return nil, []UnaccountedEntry{unlistableEntry(dir, err)} - } - if !isRefPathSegment(entries) { - log.Debug(fmt.Sprintf("github-actions curation: not descending past %q - it holds content of its own, so no ref continues through it", dir)) - return nil, nil - } - childWatermarks := watermarkedNames(entries) - for _, entry := range entries { - if isWatermarkFile(entry) { - continue - } - child := filepath.Join(dir, entry.Name()) - walkable, err := walkableDir(child, "ref") - if err != nil { - unaccounted = append(unaccounted, unresolvedEntry(child, err)) - } - if !walkable { - continue - } - childRoots, childUnaccounted := actionRootsUnder(child, ref+"/"+entry.Name(), childWatermarks) - roots = append(roots, childRoots...) - unaccounted = append(unaccounted, childUnaccounted...) - } - return roots, unaccounted -} - -// isActionRoot reports whether dir is where a ref ends and an action's own content begins. -// -// The runner marks that boundary itself, in the only two ways it materializes an entry: it -// writes .completed beside a directory it extracted, or - when serving from the archive -// cache - it makes dir a symlink to the unpacked copy and returns before writing any watermark. -// Neither marker appears on a subpath inside an action, which is what keeps a monorepo action -// like github/codeql-action@v3 from being reported as v3/analyze and v3/init. -func isActionRoot(dir string, watermarked map[string]bool) bool { - return watermarked[filepath.Base(dir)] || isSymlink(dir) -} - -func isSymlink(path string) bool { - info, err := os.Lstat(path) - return err == nil && info.Mode()&os.ModeSymlink != 0 -} - -// watermarkedNames returns the names in one directory listing that a .completed file marks. -func watermarkedNames(entries []os.DirEntry) map[string]bool { - watermarked := map[string]bool{} - for _, entry := range entries { - if isWatermarkFile(entry) { - watermarked[strings.TrimSuffix(entry.Name(), watermarkSuffix)] = true - } - } - return watermarked -} - -// isWatermarkFile reports whether entry is a watermark rather than something to walk. The -// directory test matters: a ref may legitimately end in ".completed". -func isWatermarkFile(entry os.DirEntry) bool { - return !entry.IsDir() && strings.HasSuffix(entry.Name(), watermarkSuffix) -} - -// isRefPathSegment reports whether a directory holding these entries is one a ref passes through, -// as opposed to one an action's own content begins in. A ref's intermediate directories hold only -// the next segment, plus - at the last of them - that segment's watermark. Any other regular file -// is content, and a ref never continues below an action's content. -// -// Only regular files count. A symlinked entry is a ref the runner served from the archive cache, -// and DirEntry reports it by the directory entry's own type rather than the target's - so testing -// for a directory here would end the descent on exactly those entries. -func isRefPathSegment(entries []os.DirEntry) bool { - for _, entry := range entries { - if entry.Type().IsRegular() && !isWatermarkFile(entry) { - return false - } - } - return true -} - -// unresolvedEntry records an entry that exists but cannot be classified at all. -func unresolvedEntry(path string, err error) UnaccountedEntry { - return UnaccountedEntry{Path: path, Reason: fmt.Sprintf("cannot be resolved: %v", err)} -} - -// unlistableEntry records a directory whose contents could not be read. -func unlistableEntry(path string, err error) UnaccountedEntry { - return UnaccountedEntry{Path: path, Reason: fmt.Sprintf("cannot be listed: %v", err)} -} - -// walkableDir reports whether path resolves to a directory. -func walkableDir(path, level string) (bool, error) { - info, err := os.Stat(path) - if err != nil { - return false, err - } - if !info.IsDir() { - log.Debug(fmt.Sprintf("github-actions curation: skipping non-directory entry %q at %s level", path, level)) - return false, nil - } - return true, nil -} - -// DefaultActionsCacheDir derives the runner's _actions cache path from RUNNER_WORKSPACE -// (<_work>/) - _actions is its sibling, i.e. dirname(RUNNER_WORKSPACE)/_actions. -func DefaultActionsCacheDir() (string, error) { - runnerWorkspace := os.Getenv(RunnerWorkspaceEnvVar) - if runnerWorkspace == "" { - return "", errorutils.CheckErrorf("%s is not set - cannot derive the actions cache directory", RunnerWorkspaceEnvVar) - } - return filepath.Join(runnerWorkspace, "..", "_actions"), nil -} - -// DefaultWorkflowFile derives the repo-relative path of the running workflow from -// GITHUB_WORKFLOW_REF, whose shape is "//@". -// -// Returns "" - never an error - when the variable is unset or doesn't have that shape. An -// unrecognized value must not fail the command: the caller falls back to curating the action -// cache structure alone, without parent attribution. -func DefaultWorkflowFile() string { - workflowRef := os.Getenv(WorkflowRefEnvVar) - if workflowRef == "" { - return "" - } - // The trailing "@" is a git ref and may itself contain "/" (refs/heads/my/branch). - if atIdx := strings.LastIndex(workflowRef, "@"); atIdx >= 0 { - workflowRef = workflowRef[:atIdx] - } - // Drop the leading "//"; the rest is the path within the repository. - segments := strings.SplitN(workflowRef, "/", 3) - if len(segments) < 3 || segments[2] == "" { - log.Debug(fmt.Sprintf("github-actions curation: %s=%q is not in //@ form - cannot derive the workflow file from it", WorkflowRefEnvVar, os.Getenv(WorkflowRefEnvVar))) - return "" - } - return segments[2] -} - -// DefaultJobID returns the running job's job_id from GITHUB_JOB, or "" when unset. -func DefaultJobID() string { - return os.Getenv(JobIDEnvVar) -} - -// DefaultGithubRepo returns the running job's repository from GITHUB_REPOSITORY ("/"), -// or "" when unset. -func DefaultGithubRepo() string { - return os.Getenv(GithubRepoEnvVar) -} diff --git a/commands/curation/githubactions/discovery_test.go b/commands/curation/githubactions/discovery_test.go deleted file mode 100644 index da06b20ec..000000000 --- a/commands/curation/githubactions/discovery_test.go +++ /dev/null @@ -1,510 +0,0 @@ -package githubactions - -import ( - "bytes" - "os" - "path/filepath" - "testing" - - "github.com/jfrog/jfrog-client-go/utils/log" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -const fixturesRoot = "../../../tests/testdata/projects/githubactions" - -// How the runner lays out _actions, which is what the cases below are built to mirror. Taken from -// actions/runner, src/Runner.Worker/ActionManager.cs: -// -// - The destination for every resolved action is <_actions>/// (:1193). The ref -// is joined verbatim, so one containing "/" - a branch such as copilot/backport-v4 - nests a -// level deeper than a tag does. -// - On a cache miss the archive is extracted there and .completed is written beside it -// (:1378, named at :1402). -// - With ACTIONS_RUNNER_SYMLINK_CACHED_ACTIONS set and a hit in ACTIONS_RUNNER_ACTION_ARCHIVE_CACHE, -// that same path is instead made a symlink to the already-unpacked copy (:1259) - the single -// nested folder inside /_/, since a GitHub repository archive -// always unpacks to exactly one. This branch returns before writing a watermark, which is why -// the link itself has to mark where the ref ends. -// -// The link therefore lands on the ref directory and never on the owner or repo directory above it: -// ActionManager.cs:1259 is the only place the runner creates a symlink under _actions. So no case -// here links at those higher levels to stand for an action - that would describe a cache nothing -// produces. The higher-level links below are all dangling, used only to fabricate an entry that -// exists but cannot be classified, which a broken mount or a pruned target produces just as well. -const archiveCacheSHA = "e1b2c3d4e5f60718293a4b5c6d7e8f9012345678" - -// symlinkOrSkip links newname -> oldname, skipping the test where the OS won't allow it -// (Windows needs privileges for symlink creation). -func symlinkOrSkip(t *testing.T, oldname, newname string) { - t.Helper() - if err := os.Symlink(oldname, newname); err != nil { - t.Skipf("cannot create symlinks on this platform: %v", err) - } -} - -// captureWarnings redirects the package logger into a buffer for the rest of one test. -func captureWarnings(t *testing.T) *bytes.Buffer { - t.Helper() - buf := &bytes.Buffer{} - original := log.Logger - log.SetLogger(log.NewLogger(log.WARN, buf)) - t.Cleanup(func() { log.SetLogger(original) }) - return buf -} - -// actionCache is the cache a discovery case starts from, expressed as data. Either a checked-in -// fixture, or a tree built under a temp base - where the cache root is /_actions and every -// other path is relative to , so a symlink target can sit outside the cache the way the -// runner's archive cache does. -type actionCache struct { - fixture string // a project under fixturesRoot; its _work/_actions is the cache - dirs []string // directories to create, relative to - files map[string]string // files to write, relative to - symlinks map[string]string // link path -> target path, both relative to - // unreadable lists directories to strip of every permission, relative to , so that - // listing them fails the way a runner filesystem error would. - unreadable []string -} - -func (c actionCache) build(t *testing.T) (cacheRoot string) { - t.Helper() - if c.fixture != "" { - return filepath.Join(fixturesRoot, c.fixture, "_work", "_actions") - } - base := t.TempDir() - for _, dir := range c.dirs { - require.NoError(t, os.MkdirAll(filepath.Join(base, filepath.FromSlash(dir)), 0755)) - } - for name, content := range c.files { - require.NoError(t, os.WriteFile(filepath.Join(base, filepath.FromSlash(name)), []byte(content), 0600)) - } - for link, target := range c.symlinks { - symlinkOrSkip(t, filepath.Join(base, filepath.FromSlash(target)), filepath.Join(base, filepath.FromSlash(link))) - } - for _, dir := range c.unreadable { - makeUnreadableOrSkip(t, filepath.Join(base, filepath.FromSlash(dir))) - } - return filepath.Join(base, "_actions") -} - -// makeUnreadableOrSkip strips every permission from dir, skipping the test where that still -// leaves it listable - running as root, or on a platform that ignores the mode. -func makeUnreadableOrSkip(t *testing.T, dir string) { - t.Helper() - require.NoError(t, os.Chmod(dir, 0o000)) - // Registered after t.TempDir's own cleanup, so it runs first and leaves the tree removable. - t.Cleanup(func() { _ = os.Chmod(dir, 0o755) }) - if _, err := os.ReadDir(dir); err == nil { - t.Skipf("cannot make %q unreadable on this platform", dir) - } -} - -func TestDiscoverActionCache(t *testing.T) { - tests := []struct { - name string - cache actionCache - wantEntries []string - // wantUnaccounted lists every entry the walk must refuse to skip, as a path relative to - // the cache root. A non-empty list means the command fails rather than reporting. - wantUnaccounted []string - // wantWarn is a substring the run must warn about; "" asserts nothing. - wantWarn string - // wantErr marks a cache the walk cannot begin on at all, as distinct from one it walks - // and cannot fully resolve. - wantErr bool - }{ - { - name: "verify when the cache is well-formed then one entry per owner repo ref is returned", - cache: actionCache{fixture: "curation-project"}, - wantEntries: []string{"actions/checkout@v4", "github/codeql-action@v3", "some-org/transitive-action@v1"}, - }, - { - // An absent cache is not an empty one: this command is invoked by an action, which is - // itself an entry, so a cache without any is not the cache this job resolved from. - name: "verify when the cache directory does not exist then the walk fails rather than reporting an empty cache", - cache: actionCache{fixture: "does-not-exist"}, - wantErr: true, - }, - { - // stray-file.txt (owner level), actions/stray-file-at-repo-level.txt (repo level) and - // onlyowner/ (an owner dir with no repo subdirectories). - name: "verify when an entry is not a well-formed triple then it is skipped without error", - cache: actionCache{fixture: "malformed-project"}, - wantEntries: []string{"actions/checkout@v4"}, - }, - { - // What ACTIONS_RUNNER_SYMLINK_CACHED_ACTIONS produces, in the shape the runner writes - // it. Both entries of one cache: a hit, served from the archive cache as a symlink, and - // a miss, extracted in place beside its watermark. - name: "verify when an entry is symlinked into the archive cache then it is followed", - cache: actionCache{ - dirs: []string{ - "_actions/actions/checkout/v4", // a miss: extracted, so a real directory - "_actions/actions/setup-node", - // /_/, holding the single nested folder a - // GitHub repository archive always unpacks to. - "archive-cache/actions_setup-node/" + archiveCacheSHA + "/setup-node-" + archiveCacheSHA, - }, - // The watermark the runner drops beside an extracted entry is still not an action. - files: map[string]string{"_actions/actions/checkout/v4.completed": "ts"}, - // The link lands on the ref directory itself, and nothing above it. - symlinks: map[string]string{ - "_actions/actions/setup-node/v4": "archive-cache/actions_setup-node/" + archiveCacheSHA + "/setup-node-" + archiveCacheSHA, - }, - }, - wantEntries: []string{"actions/checkout@v4", "actions/setup-node@v4"}, - }, - { - // Measured on a hosted runner: a branch ref nests, and the runner puts its watermark - // beside the real root rather than at a fixed depth. Reading only three levels deep - // would report actions/checkout@copilot - an action that does not exist - and leave - // the one that actually executes undiscovered. - name: "verify when the ref contains a slash then the whole ref is recovered", - cache: actionCache{ - dirs: []string{"_actions/actions/checkout/copilot/backport-2518-releases-v4"}, - files: map[string]string{ - "_actions/actions/checkout/copilot/backport-2518-releases-v4.completed": "ts", - "_actions/actions/checkout/copilot/backport-2518-releases-v4/action.yml": "runs:\n using: node20\n", - }, - }, - wantEntries: []string{"actions/checkout@copilot/backport-2518-releases-v4"}, - }, - { - // The same tree the probe produced: a slashed ref and a plain one under one repo. - name: "verify when a repo holds both a plain and a slashed ref then each is reported once", - cache: actionCache{ - dirs: []string{"_actions/actions/checkout/v4", "_actions/actions/checkout/copilot/backport-v4"}, - files: map[string]string{ - "_actions/actions/checkout/v4.completed": "ts", - "_actions/actions/checkout/copilot/backport-v4.completed": "ts", - }, - }, - wantEntries: []string{"actions/checkout@v4", "actions/checkout@copilot/backport-v4"}, - }, - { - // Three monorepo actions, each carrying subpath manifests. Only the ref root is - // watermarked, which is what stops analyze/ and init/ surfacing as their own actions - - // manifest presence could not tell them apart. - name: "verify when a monorepo action has subpaths then only the ref root is reported", - cache: actionCache{ - dirs: []string{ - "_actions/github/codeql-action/v3/analyze", "_actions/github/codeql-action/v3/init", - "_actions/anchore/sbom-action/v0/download-syft", "_actions/anchore/sbom-action/v0/publish-sbom", - "_actions/anchore/scan-action/v7/download-grype", - }, - files: map[string]string{ - "_actions/github/codeql-action/v3.completed": "ts", - "_actions/github/codeql-action/v3/action.yml": "runs:\n using: node20\n", - "_actions/github/codeql-action/v3/analyze/action.yml": "runs:\n using: node20\n", - "_actions/github/codeql-action/v3/init/action.yml": "runs:\n using: node20\n", - "_actions/anchore/sbom-action/v0.completed": "ts", - "_actions/anchore/sbom-action/v0/download-syft/action.yml": "runs:\n using: node20\n", - "_actions/anchore/sbom-action/v0/publish-sbom/action.yml": "runs:\n using: node20\n", - "_actions/anchore/scan-action/v7.completed": "ts", - "_actions/anchore/scan-action/v7/download-grype/action.yml": "runs:\n using: node20\n", - }, - }, - wantEntries: []string{"github/codeql-action@v3", "anchore/sbom-action@v0", "anchore/scan-action@v7"}, - }, - { - // A symlinked root carries no watermark - the runner returns before writing one - so - // the link itself has to end the ref, at a plain depth or a nested one. - name: "verify when a symlinked root sits at a slashed ref then the whole ref is recovered", - cache: actionCache{ - dirs: []string{"archive-cache/unpacked", "_actions/actions/checkout/feature"}, - symlinks: map[string]string{"_actions/actions/checkout/feature/my-branch": "archive-cache/unpacked"}, - }, - wantEntries: []string{"actions/checkout@feature/my-branch"}, - }, - { - // The marker is what identifies a root, so an entry missing one in a cache whose other - // entries have them cannot be placed. Reporting actions/checkout@feature would name an - // action that does not exist while the one that does stays unexamined, so it is dropped - // and said out loud instead. - name: "verify when one entry in a marked cache has no marker then it is unaccounted for", - cache: actionCache{ - dirs: []string{"_actions/actions/checkout/v4", "_actions/actions/checkout/feature/my-branch"}, - files: map[string]string{ - "_actions/actions/checkout/v4.completed": "ts", - "_actions/actions/checkout/feature/my-branch/action.yml": "runs:\n using: node20\n", - }, - }, - wantEntries: []string{"actions/checkout@v4"}, - wantUnaccounted: []string{"actions/checkout/feature"}, - }, - { - // Two refs of one monorepo, each reached through a different subpath - init@v2 and - // analyze@v3. Both roots are watermarked, so each ref is named in full and neither - // subpath is mistaken for a root of its own. - name: "verify when one monorepo has two refs then each is reported under its own ref", - cache: actionCache{ - dirs: []string{"_actions/github/codeql-action/v2/init", "_actions/github/codeql-action/v3/analyze"}, - files: map[string]string{ - "_actions/github/codeql-action/v2.completed": "ts", - "_actions/github/codeql-action/v2/init/action.yml": "runs:\n using: node20\n", - "_actions/github/codeql-action/v3.completed": "ts", - "_actions/github/codeql-action/v3/analyze/action.yml": "runs:\n using: node20\n", - }, - }, - wantEntries: []string{"github/codeql-action@v2", "github/codeql-action@v3"}, - }, - { - // A branch may carry any number of slashes, so there is no depth at which the descent - // can stop counting and still be right. What ends it is the cache's own shape: each - // intermediate holds only the next segment, so the marker is reached however deep it is. - name: "verify when a ref spans many segments then the whole ref is recovered", - cache: actionCache{ - dirs: []string{"_actions/actions/checkout/v4", "_actions/actions/checkout/release/2024/q1/hotfix/v2"}, - files: map[string]string{ - "_actions/actions/checkout/v4.completed": "ts", - "_actions/actions/checkout/release/2024/q1/hotfix/v2.completed": "ts", - "_actions/actions/checkout/release/2024/q1/hotfix/v2/action.yml": "runs:\n using: node20\n", - }, - }, - wantEntries: []string{"actions/checkout@v4", "actions/checkout@release/2024/q1/hotfix/v2"}, - }, - { - // The descent stops where content starts, so a file left in an intermediate ends it - // early. The marker is still what names a ref, so the entry is dropped and said out - // loud rather than reported under the prefix reached so far. - name: "verify when an intermediate holds a stray file then the entry is unaccounted rather than truncated", - cache: actionCache{ - dirs: []string{"_actions/actions/checkout/v4", "_actions/actions/checkout/release/hotfix/v2"}, - files: map[string]string{ - "_actions/actions/checkout/v4.completed": "ts", - "_actions/actions/checkout/release/.DS_Store": "junk", - "_actions/actions/checkout/release/hotfix/v2.completed": "ts", - "_actions/actions/checkout/release/hotfix/v2/action.yml": "runs:\n using: node20\n", - }, - }, - wantEntries: []string{"actions/checkout@v4"}, - wantUnaccounted: []string{"actions/checkout/release"}, - }, - { - // Every shape one cache can hold at once: a tag ref at depth one, a deep branch ref, and - // a monorepo whose subpaths must not surface as refs of their own. The marker decides all - // three; the walk never reads inside any of them. - name: "verify when one cache mixes ref depths and a monorepo then each action is reported once", - cache: actionCache{ - dirs: []string{ - "_actions/actions/checkout/v4/dist", - "_actions/actions/checkout/release/2024/q1/hotfix/v2/dist", - "_actions/github/codeql-action/v3/init", - "_actions/github/codeql-action/v3/analyze", - }, - files: map[string]string{ - "_actions/actions/checkout/v4.completed": "ts", - "_actions/actions/checkout/v4/action.yml": "runs:\n using: node20\n", - "_actions/actions/checkout/v4/README.md": "x", - "_actions/actions/checkout/release/2024/q1/hotfix/v2.completed": "ts", - "_actions/actions/checkout/release/2024/q1/hotfix/v2/action.yml": "runs:\n using: node20\n", - "_actions/github/codeql-action/v3.completed": "ts", - "_actions/github/codeql-action/v3/action.yml": "runs:\n using: node20\n", - "_actions/github/codeql-action/v3/init/action.yml": "runs:\n using: node20\n", - "_actions/github/codeql-action/v3/analyze/action.yml": "runs:\n using: node20\n", - }, - }, - wantEntries: []string{"actions/checkout@v4", "actions/checkout@release/2024/q1/hotfix/v2", "github/codeql-action@v3"}, - }, - { - // Nothing stops a branch from being called release.completed, so the watermark test - // has to distinguish the marker file from a directory that shares its suffix. - name: "verify when a ref itself ends in the watermark suffix then it is still an action", - cache: actionCache{ - dirs: []string{"_actions/actions/checkout/release.completed"}, - files: map[string]string{ - "_actions/actions/checkout/release.completed.completed": "ts", - "_actions/actions/checkout/release.completed/action.yml": "runs:\n using: node20\n", - }, - }, - wantEntries: []string{"actions/checkout@release.completed"}, - }, - { - // The entry names an action, so it cannot be waved through: its content is gone, and - // nothing here can say what curating it would have concluded. - name: "verify when a symlink resolves to nothing then it is unaccounted rather than skipped", - cache: actionCache{ - dirs: []string{"_actions/actions/checkout"}, - symlinks: map[string]string{"_actions/actions/checkout/v4": "nowhere"}, - }, - wantUnaccounted: []string{"actions/checkout/v4"}, - }, - { - // A runner filesystem error at owner and at repo level. One action still resolves, so - // this pins that a partial walk is a failure rather than a short report. - name: "verify when an owner or repo directory cannot be listed then it is unaccounted for", - cache: actionCache{ - dirs: []string{ - "_actions/actions/checkout/v4", - "_actions/github/codeql-action/v3", - "_actions/good-org/good-action/v1", - }, - files: map[string]string{ - "_actions/github/codeql-action/v3.completed": "ts", - "_actions/good-org/good-action/v1.completed": "ts", - }, - unreadable: []string{"_actions/actions", "_actions/github/codeql-action"}, - }, - wantEntries: []string{"good-org/good-action@v1"}, - wantUnaccounted: []string{"actions", "github/codeql-action"}, - }, - { - // Dangling links at owner and repo level, where the walk learns an entry exists before - // it can classify it. Nothing names an action yet, but something is there. - name: "verify when an owner or repo entry cannot be resolved then it is unaccounted for", - cache: actionCache{ - dirs: []string{"_actions/good-org/good-action/v1"}, - files: map[string]string{"_actions/good-org/good-action/v1.completed": "ts"}, - symlinks: map[string]string{"_actions/ghost-owner": "nowhere", "_actions/good-org/ghost-repo": "nowhere"}, - }, - wantEntries: []string{"good-org/good-action@v1"}, - wantUnaccounted: []string{"ghost-owner", "good-org/ghost-repo"}, - }, - { - // Below the ref level, inside what could be a slashed ref: one intermediate that cannot - // be listed, one that can but holds a dangling link. Each is named once, by the reason - // the descent stopped. branch itself is not also reported as unmarked - the dangling - // entry under it is the specific fact, and a missing marker would just restate it. - name: "verify when a nested ref segment cannot be read then it is unaccounted exactly once", - cache: actionCache{ - dirs: []string{"_actions/actions/checkout/v4", "_actions/actions/checkout/unlistable", "_actions/actions/checkout/branch"}, - files: map[string]string{"_actions/actions/checkout/v4.completed": "ts"}, - symlinks: map[string]string{"_actions/actions/checkout/branch/ghost": "nowhere"}, - unreadable: []string{"_actions/actions/checkout/unlistable"}, - }, - wantEntries: []string{"actions/checkout@v4"}, - wantUnaccounted: []string{ - "actions/checkout/unlistable", // cannot be listed - "actions/checkout/branch/ghost", // listed, but the entry does not resolve - }, - }, - { - name: "verify when the cache root is not a directory then the walk fails rather than reporting nothing", - cache: actionCache{files: map[string]string{"_actions": "not a directory"}}, - wantErr: true, - }, - { - // Every entry was understood and none held an action, which is a clean result rather - // than a blind spot - so it warns instead of going unaccounted. - name: "verify when a populated cache holds no action at all then it warns without failing", - cache: actionCache{ - dirs: []string{"_actions/onlyowner"}, - files: map[string]string{"_actions/stray.txt": "x"}, - }, - wantWarn: "none resolved to an", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - cacheRoot := tt.cache.build(t) - warnings := captureWarnings(t) - - scan, err := DiscoverActionCache(cacheRoot) - - if tt.wantErr { - assert.Error(t, err) - return - } - assert.NoError(t, err) - if tt.wantWarn != "" { - assert.Contains(t, warnings.String(), tt.wantWarn) - } - assert.NotNil(t, scan.Refs, "an empty result must still be a usable slice") - - unaccounted := make([]string, len(scan.Unaccounted)) - for i, entry := range scan.Unaccounted { - rel, relErr := filepath.Rel(cacheRoot, entry.Path) - require.NoError(t, relErr) - unaccounted[i] = filepath.ToSlash(rel) - assert.NotEmpty(t, entry.Reason, "an unaccounted entry must say why") - } - assert.ElementsMatch(t, tt.wantUnaccounted, unaccounted) - assert.Equal(t, len(tt.wantUnaccounted) > 0, scan.UnaccountedError() != nil, - "UnaccountedError must be non-nil exactly when an entry went unaccounted") - - found := make([]string, len(scan.Refs)) - for i, ref := range scan.Refs { - found[i] = ref.Owner + "/" + ref.Repo + "@" + ref.Ref - } - assert.ElementsMatch(t, tt.wantEntries, found) - for _, ref := range scan.Refs { - assert.Equal(t, filepath.Join(cacheRoot, ref.Owner, ref.Repo, ref.Ref), ref.Path) - assert.Empty(t, ref.Subpaths, "DiscoverActionCache must not set Subpaths - that's CrossReference's job") - assert.Empty(t, ref.Parent, "DiscoverActionCache must not set Parent - that's CrossReference's job") - } - }) - } -} - -func TestDefaultActionsCacheDir(t *testing.T) { - tests := []struct { - name string - runnerWorkspace string - want string - wantErr bool - }{ - { - name: "verify when RUNNER_WORKSPACE is set then the cache path is its sibling", - runnerWorkspace: "/home/runner/work/my-repo", - want: filepath.Clean("/home/runner/work/_actions"), - }, - { - name: "verify when RUNNER_WORKSPACE is unset then an error is returned rather than a guess", - runnerWorkspace: "", - wantErr: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Setenv(RunnerWorkspaceEnvVar, tt.runnerWorkspace) - - dir, err := DefaultActionsCacheDir() - - if tt.wantErr { - assert.Error(t, err) - return - } - assert.NoError(t, err) - assert.Equal(t, tt.want, dir) - }) - } -} - -func TestDefaultWorkflowFile(t *testing.T) { - tests := []struct { - name string - workflowRef string - want string - }{ - {"verify when the ref is standard then the repo-relative path is returned", "octocat/hello-world/.github/workflows/ci.yml@refs/heads/main", ".github/workflows/ci.yml"}, - {"verify when the ref contains slashes then the path is still returned", "octocat/hello-world/.github/workflows/ci.yml@refs/heads/my/feature", ".github/workflows/ci.yml"}, - {"verify when the ref is a tag then the path is still returned", "octocat/hello-world/.github/workflows/release.yaml@refs/tags/v1.2.3", ".github/workflows/release.yaml"}, - {"verify when the variable is unset then the path is empty", "", ""}, - {"verify when the value carries no path then the result is empty", "octocat/hello-world@refs/heads/main", ""}, - {"verify when the value has no ref suffix then the path is still returned", "octocat/hello-world/.github/workflows/ci.yml", ".github/workflows/ci.yml"}, - {"verify when the value is malformed then the result is empty rather than an error", "nonsense", ""}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Setenv(WorkflowRefEnvVar, tt.workflowRef) - assert.Equal(t, tt.want, DefaultWorkflowFile()) - }) - } -} - -func TestDefaultJobID(t *testing.T) { - t.Setenv(JobIDEnvVar, "build") - assert.Equal(t, "build", DefaultJobID()) - - t.Setenv(JobIDEnvVar, "") - assert.Empty(t, DefaultJobID()) -} - -func TestDefaultGithubRepo(t *testing.T) { - t.Setenv(GithubRepoEnvVar, "octocat/hello-world") - assert.Equal(t, "octocat/hello-world", DefaultGithubRepo()) - - t.Setenv(GithubRepoEnvVar, "") - assert.Empty(t, DefaultGithubRepo()) -} diff --git a/commands/curation/githubactions/reporesolver.go b/commands/curation/githubactions/reporesolver.go deleted file mode 100644 index d80554ee8..000000000 --- a/commands/curation/githubactions/reporesolver.go +++ /dev/null @@ -1,19 +0,0 @@ -package githubactions - -import "context" - -// ArtifactoryVcsRepoResolver maps the GitHub repository running the current job to the -// Artifactory VCS repository whose curation policies govern it - per onboarded repository. -// Only the mock implementation exists. -type ArtifactoryVcsRepoResolver interface { - // Resolve returns the Artifactory VCS repository key for githubRepo ("/", the - // shape GITHUB_REPOSITORY carries). - // - // githubRepo decides which policies judge the job, so its only legitimate source is the - // runner's own GITHUB_REPOSITORY - see DefaultGithubRepo. The runner does not let a - // workflow-, job- or step-level env: block override that variable, which is what makes it - // trustworthy; a value reaching this call from anywhere a workflow author can write would - // let the subject of the check choose the policies applied to it. SetGithubRepo exists for - // tests and is deliberately not reachable from the CLI. - Resolve(ctx context.Context, githubRepo string) (string, error) -} diff --git a/commands/curation/githubactions/reporesolver_mock.go b/commands/curation/githubactions/reporesolver_mock.go deleted file mode 100644 index 16cd4472f..000000000 --- a/commands/curation/githubactions/reporesolver_mock.go +++ /dev/null @@ -1,27 +0,0 @@ -package githubactions - -import ( - "context" - "fmt" - "strings" -) - -const mockVcsRepoSuffix = "-github-remote-stand-in" - -// mockArtifactoryVcsRepoResolver stands in for the curation service's repository-mapping API, -// which does not exist yet. -type mockArtifactoryVcsRepoResolver struct{} - -// NewMockArtifactoryVcsRepoResolver returns a resolver that derives the repository key from -// the GitHub owner. -func NewMockArtifactoryVcsRepoResolver() ArtifactoryVcsRepoResolver { - return mockArtifactoryVcsRepoResolver{} -} - -func (mockArtifactoryVcsRepoResolver) Resolve(_ context.Context, githubRepo string) (string, error) { - owner, repo, found := strings.Cut(githubRepo, "/") - if !found || owner == "" || repo == "" || strings.Contains(repo, "/") { - return "", fmt.Errorf("github repository %q is not in / form", githubRepo) - } - return owner + mockVcsRepoSuffix, nil -} diff --git a/commands/curation/githubactions/reporesolver_mock_test.go b/commands/curation/githubactions/reporesolver_mock_test.go deleted file mode 100644 index fede8b289..000000000 --- a/commands/curation/githubactions/reporesolver_mock_test.go +++ /dev/null @@ -1,38 +0,0 @@ -package githubactions - -import ( - "context" - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestMockArtifactoryVcsRepoResolver(t *testing.T) { - tests := []struct { - name string - githubRepo string - want string - wantErr bool - }{ - {"verify when the repository is owner slash repo then the key derives from the owner", "my-org/my-repo", "my-org" + mockVcsRepoSuffix, false}, - {"verify when the owner differs then the key differs", "other-org/my-repo", "other-org" + mockVcsRepoSuffix, false}, - {"verify when only the repo differs then the key is the same", "my-org/another-repo", "my-org" + mockVcsRepoSuffix, false}, - {"verify when the value has no slash then an error is returned", "my-org", "", true}, - {"verify when the value is empty then an error is returned", "", "", true}, - {"verify when the owner is missing then an error is returned", "/my-repo", "", true}, - {"verify when the repo is missing then an error is returned", "my-org/", "", true}, - {"verify when the value has an extra path segment then an error is returned", "my-org/my-repo/extra", "", true}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := NewMockArtifactoryVcsRepoResolver().Resolve(context.Background(), tt.githubRepo) - if tt.wantErr { - assert.Error(t, err, "an unmappable repository must not resolve to a default") - assert.Empty(t, got) - return - } - assert.NoError(t, err) - assert.Equal(t, tt.want, got) - }) - } -} diff --git a/commands/curation/githubactions/report.go b/commands/curation/githubactions/report.go deleted file mode 100644 index 5ddc6503b..000000000 --- a/commands/curation/githubactions/report.go +++ /dev/null @@ -1,87 +0,0 @@ -package githubactions - -import ( - "strings" - - "github.com/jfrog/jfrog-cli-security/utils/formats" -) - -// ActionReportRow is one row of the curation report, already resolved from an ActionRef and -// its ActionCurationResult. -type ActionReportRow struct { - Action string // "owner/repo", plus " (subpath[, subpath...])" when invoked via subpaths - Ref string // verbatim from the cache directory name, uninterpreted - Parent string // "" when directly referenced, or when attribution could not place it - Status string - Notes string -} - -// NewActionReportRow builds one report row. A monorepo action invoked via several subpaths -// (codeql-action's init@v3 and analyze@v3) shares one cache entry and one decision, so it is -// one row - every subpath used is listed so neither invocation is silently lost. -func NewActionReportRow(ref ActionRef, result ActionCurationResult) ActionReportRow { - action := ref.Owner + "/" + ref.Repo - if len(ref.Subpaths) > 0 { - action += " (" + strings.Join(ref.Subpaths, ", ") + ")" - } - return ActionReportRow{ - Action: action, - Ref: ref.Ref, - Parent: ref.Parent, - Status: string(result.Status), - Notes: result.Notes, - } -} - -// RenderReportTable renders rows as the console report's table. withParent controls whether the -// Parent column appears at all. -// -// Pipe-delimited like the job summary's table, so the two read alike and either can be pasted -// where the other is expected - but the cells are escaped for a terminal, because this one is -// printed to the job log and read there. The job summary renders its own table from the recorded -// summary files; the shared piece is the cell escaping, not the table. -func RenderReportTable(rows []ActionReportRow, withParent bool) string { - var sb strings.Builder - if withParent { - sb.WriteString("| Action | Ref | Parent | Status | Notes |\n") - sb.WriteString("|--------|-----|--------|--------|-------|\n") - } else { - sb.WriteString("| Action | Ref | Status | Notes |\n") - sb.WriteString("|--------|-----|--------|-------|\n") - } - for _, row := range rows { - sb.WriteString("| ") - sb.WriteString(formats.EscapeTerminalTableCell(row.Action)) - sb.WriteString(" | ") - sb.WriteString(formats.EscapeTerminalTableCell(row.Ref)) - if withParent { - sb.WriteString(" | ") - sb.WriteString(formats.EscapeTerminalTableCell(row.Parent)) - } - sb.WriteString(" | ") - sb.WriteString(formats.EscapeTerminalTableCell(row.Status)) - sb.WriteString(" | ") - sb.WriteString(formats.EscapeTerminalTableCell(row.Notes)) - sb.WriteString(" |\n") - } - return sb.String() -} - -// NotApproved returns every row whose Status is not exactly ActionApproved, for the command's -// exit-code decision. -// -// An allow-list, deliberately, rather than a test for ActionRejected: ActionCurationStatus is an -// open string type, so a status this code does not recognize - one a later decider introduces, or -// the zero value of a result returned without one - would pass a deny-list while rendering as an -// empty cell. Only an explicit approval may clear a gate whose purpose is to stop whatever it has -// not cleared. This is not part of the mocked seam; the real decider replaces the verdict, not -// the enforcement. -func NotApproved(rows []ActionReportRow) []ActionReportRow { - var notApproved []ActionReportRow - for _, row := range rows { - if row.Status != string(ActionApproved) { - notApproved = append(notApproved, row) - } - } - return notApproved -} diff --git a/commands/curation/githubactions/report_test.go b/commands/curation/githubactions/report_test.go deleted file mode 100644 index c4c275b1a..000000000 --- a/commands/curation/githubactions/report_test.go +++ /dev/null @@ -1,161 +0,0 @@ -package githubactions - -import ( - "strings" - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestNewActionReportRow(t *testing.T) { - tests := []struct { - name string - ref ActionRef - result ActionCurationResult - want ActionReportRow - }{ - { - name: "verify when the action has no subpath then the action cell is owner and repo", - ref: ActionRef{Owner: "actions", Repo: "checkout", Ref: "v4"}, - result: ActionCurationResult{Status: ActionApproved}, - want: ActionReportRow{Action: "actions/checkout", Ref: "v4", Status: "Approved"}, - }, - { - name: "verify when the action has one subpath then it is appended to the action cell", - ref: ActionRef{Owner: "github", Repo: "codeql-action", Ref: "v3", Subpaths: []string{"analyze"}}, - result: ActionCurationResult{Status: ActionApproved}, - want: ActionReportRow{Action: "github/codeql-action (analyze)", Ref: "v3", Status: "Approved"}, - }, - { - // init@v3 and analyze@v3 collapse to one cache entry, so neither invocation may be lost. - name: "verify when the action has several subpaths then every one is listed", - ref: ActionRef{Owner: "github", Repo: "codeql-action", Ref: "v3", Subpaths: []string{"init", "analyze"}}, - result: ActionCurationResult{Status: ActionApproved}, - want: ActionReportRow{Action: "github/codeql-action (init, analyze)", Ref: "v3", Status: "Approved"}, - }, - { - name: "verify when the decision carries parent and notes then both reach the row", - ref: ActionRef{Owner: "some-org", Repo: "transitive-action", Ref: "v1", Parent: "github/codeql-action@v3"}, - result: ActionCurationResult{Status: ActionRejected, Notes: "policy failure"}, - want: ActionReportRow{Action: "some-org/transitive-action", Ref: "v1", Parent: "github/codeql-action@v3", - Status: "Rejected", Notes: "policy failure"}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, NewActionReportRow(tt.ref, tt.result)) - }) - } -} - -func TestRenderReportTable(t *testing.T) { - tests := []struct { - name string - rows []ActionReportRow - withParent bool - want string - // wantPipes is the pipe count the header and every data row must agree on, or the table - // renders broken. - wantPipes int - }{ - { - name: "verify when the run was attributed then the table carries a Parent column", - rows: []ActionReportRow{ - {Action: "actions/checkout", Ref: "v4", Status: "Approved"}, - {Action: "some-org/transitive-action", Ref: "v1", Parent: "github/codeql-action@v3", Status: "Rejected", Notes: "policy failure"}, - }, - withParent: true, - want: "| Action | Ref | Parent | Status | Notes |\n" + - "|--------|-----|--------|--------|-------|\n" + - "| actions/checkout | v4 | | Approved | |\n" + - "| some-org/transitive-action | v1 | github/codeql-action@v3 | Rejected | policy failure |\n", - wantPipes: 6, - }, - { - // Present-and-blank would read as "nothing pulled in transitively". - name: "verify when the run was structure-only then the Parent column is omitted", - rows: []ActionReportRow{ - {Action: "actions/checkout", Ref: "v4", Status: "Approved"}, - {Action: "some-org/some-action", Ref: "v1", Status: "Rejected", Notes: "policy failure"}, - }, - withParent: false, - want: "| Action | Ref | Status | Notes |\n" + - "|--------|-----|--------|-------|\n" + - "| actions/checkout | v4 | Approved | |\n" + - "| some-org/some-action | v1 | Rejected | policy failure |\n", - wantPipes: 5, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := RenderReportTable(tt.rows, tt.withParent) - - assert.Equal(t, tt.want, got) - for _, line := range strings.Split(strings.TrimSuffix(got, "\n"), "\n") { - assert.Equal(t, tt.wantPipes, strings.Count(line, "|"), "row %q has the wrong cell count", line) - } - }) - } -} - -func TestNotApproved(t *testing.T) { - tests := []struct { - name string - rows []ActionReportRow - want []string // the Ref of each row that must not clear the gate - }{ - {name: "verify when every action is approved then none is withheld", rows: []ActionReportRow{{Ref: "v4", Status: "Approved"}}}, - {name: "verify when no action was decided then none is withheld", rows: nil}, - { - name: "verify when an action is rejected then it is withheld", - rows: []ActionReportRow{{Ref: "v4", Status: "Approved"}, {Ref: "v1", Status: "Rejected"}}, - want: []string{"v1"}, - }, - { - // A deny-list would let these through while rendering an empty or unfamiliar cell: the - // zero value of a result returned without a status, and a status a later decider adds. - name: "verify when a status is blank or unrecognized then it is withheld", - rows: []ActionReportRow{{Ref: "v4", Status: "Approved"}, {Ref: "v9"}, {Ref: "v2", Status: "NeedsReview"}}, - want: []string{"v9", "v2"}, - }, - { - // Case matters: only the exact constant approves. - name: "verify when a status differs only in case then it is withheld", - rows: []ActionReportRow{{Ref: "v4", Status: "approved"}}, - want: []string{"v4"}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - var got []string - for _, row := range NotApproved(tt.rows) { - got = append(got, row.Ref) - } - assert.Equal(t, tt.want, got) - }) - } -} - -func TestRenderReportTable_CellsThatWouldReshapeTheTableAreEscaped(t *testing.T) { - // Ref is a directory name the runner created and "|" is legal in a git refname; Notes comes - // from the decision service. Unescaped, either would add a column or split the row, so the - // table would report a status against the wrong action. - rows := []ActionReportRow{ - {Action: "some-org/some-action", Ref: "feature|v2", Parent: "org/wrap|per@v1", Status: "Rejected", Notes: "blocked:\nCVE-2024-0001"}, - } - - got := RenderReportTable(rows, true) - - lines := strings.Split(strings.TrimSuffix(got, "\n"), "\n") - if assert.Len(t, lines, 3, "header, separator, one data row") { - assert.Equal(t, 6, strings.Count(lines[2], "|")-strings.Count(lines[2], `\|`), - "the data row must still have exactly the header's cell count") - } - assert.Contains(t, got, `feature\|v2`) - assert.Contains(t, got, `org/wrap\|per@v1`) - // The row is still one line, but as text a terminal can read: this table is printed to the - // job log, where "
" would show verbatim. - assert.Contains(t, got, "blocked: CVE-2024-0001") - assert.NotContains(t, got, "
", "markdown markup must not reach the console report") - assert.NotContains(t, got, "\nCVE-2024-0001", "a newline in Notes must never end the row early") -} diff --git a/commands/curation/githubactions/workflow.go b/commands/curation/githubactions/workflow.go deleted file mode 100644 index 183ad947f..000000000 --- a/commands/curation/githubactions/workflow.go +++ /dev/null @@ -1,330 +0,0 @@ -package githubactions - -import ( - "errors" - "fmt" - "maps" - "os" - "path/filepath" - "slices" - "strings" - - "gopkg.in/yaml.v3" - - "github.com/jfrog/jfrog-client-go/utils/log" -) - -// WorkflowUse is one `uses:` value parsed out of a workflow (or composite action) YAML file. -type WorkflowUse struct { - Owner string - Repo string - Subpath string // "" handle mono repo github actions, e.g. github/codeql-action/analyze@v3 - Ref string - Raw string // the original "uses:" string, for diagnostics -} - -type rawWorkflow struct { - Jobs map[string]rawJob `yaml:"jobs"` -} - -type rawJob struct { - Steps []rawStep `yaml:"steps"` -} - -type rawStep struct { - Uses string `yaml:"uses"` -} - -// parseUsesString parses a single `uses:` value into owner/repo/ref, plus an optional subpath -// (empty for most actions - only present for monorepo-style actions like -// github/codeql-action/analyze@v3). Returns false for shapes that don't resolve to at least an -// owner/repo/ref triple -func parseUsesString(raw string) (WorkflowUse, bool) { - if raw == "" || IsLocalUse(raw) || strings.HasPrefix(raw, "docker://") { - return WorkflowUse{}, false - } - atIdx := strings.LastIndex(raw, "@") - if atIdx < 0 || atIdx == len(raw)-1 { - return WorkflowUse{}, false - } - path, ref := raw[:atIdx], raw[atIdx+1:] - segments := strings.Split(path, "/") - if len(segments) < 2 || segments[0] == "" || segments[1] == "" { - return WorkflowUse{}, false - } - subpath := "" - if len(segments) > 2 { - subpath = strings.Join(segments[2:], "/") - } - return WorkflowUse{Owner: segments[0], Repo: segments[1], Subpath: subpath, Ref: ref, Raw: raw}, true -} - -func IsLocalUse(raw string) bool { - return strings.HasPrefix(raw, "./") -} - -// LocalUse is one `uses: ./...` step the job will run, and who declared it. -type LocalUse struct { - // Raw is the `uses:` value verbatim, e.g. "./.github/actions/setup". - Raw string - // DeclaredBy is the composite action that declares this step, as "/@" - DeclaredBy string -} - -// JobUses is what one job's steps reference. -type JobUses struct { - // Remote holds the owner/repo/ref references this job declares, in file order. - Remote []WorkflowUse - // Local holds the `uses: ./...` steps, in file order, deduplicated per declarer. - Local []LocalUse -} - -// ErrJobUnknown reports that the job being curated could not be identified in this workflow -// file - either no job id was given, or the file does not declare the one that was. It is not a -// failure of the run: callers treat it as "cannot attribute" and curate the cache as-is. -var ErrJobUnknown = errors.New("cannot identify the job being curated in the workflow file") - -// ErrWorkflowUnparsable reports that the workflow file was read but could not be parsed as YAML. -// The runner already accepted this file, so it is a divergence between its YAML reader and ours -// rather than a broken workflow - the same position parseCompositeActionUses is in one level -// down, and handled the same way. It is not a failure of the run: callers treat it as "cannot -// attribute" and curate the cache as-is. -var ErrWorkflowUnparsable = errors.New("cannot parse the workflow file") - -// ParseWorkflowUses parses the step-level `uses:` values of ONE job in a workflow YAML file. -// Docker-URI actions (uses: docker://...) are skipped - the runner pulls those images during job -// setup rather than into the action cache. Local actions (uses: ./path) are not parsed as -// references either, but are returned in JobUses.Local so the report can declare them uncovered. -// -// jobID must name a job the file declares; otherwise it returns ErrJobUnknown and parses -// nothing. There is deliberately no fallback to the file's other jobs: each ran on its own -// runner with its own cache, so attributing from them would label an entry with a parent that -// never pulled it in. Attribution therefore needs both a file and a job id - no constraint on a -// runner, where GITHUB_JOB is always set. -// -// KNOWN FAILURE - a called reusable workflow can attribute against the wrong job because -// GITHUB_WORKFLOW_REF holds the caller file name and GITHUB_JOB holds the callee job id. -// only attribution error and is unfixed at the moment. -func ParseWorkflowUses(workflowPath, jobID string) (JobUses, error) { - data, err := os.ReadFile(workflowPath) - if err != nil { - return JobUses{}, fmt.Errorf("reading workflow file %q: %w", workflowPath, err) - } - var wf rawWorkflow - if err = yaml.Unmarshal(data, &wf); err != nil { - return JobUses{}, fmt.Errorf("%w %q: %w", ErrWorkflowUnparsable, workflowPath, err) - } - if jobID == "" { - return JobUses{}, fmt.Errorf("%w: no job id given for %q", ErrJobUnknown, workflowPath) - } - job, declared := wf.Jobs[jobID] - if !declared { - return JobUses{}, fmt.Errorf("%w: %q is not among %v in %q", ErrJobUnknown, jobID, slices.Sorted(maps.Keys(wf.Jobs)), workflowPath) - } - // One job's steps: a slice, so the order is the file's, with no map iteration to sort away. - // DeclaredBy is left empty: the job's own workflow declares these. - return parseSteps(job.Steps, ""), nil -} - -// parseSteps splits one steps: list into the remote references to curate and the local steps -// that cannot be, attributing the local ones to declaredBy ("" for a job's own steps). -// -// Local steps are deduplicated because the report names each once: two steps invoking the same -// local action are one uncovered action, not two. -func parseSteps(steps []rawStep, declaredBy string) (uses JobUses) { - seenLocal := map[string]bool{} - for _, step := range steps { - if IsLocalUse(step.Uses) { - if !seenLocal[step.Uses] { - seenLocal[step.Uses] = true - uses.Local = append(uses.Local, LocalUse{Raw: step.Uses, DeclaredBy: declaredBy}) - } - continue - } - if parsed, ok := parseUsesString(step.Uses); ok { - uses.Remote = append(uses.Remote, parsed) - } - } - return uses -} - -type rawActionFile struct { - Runs rawActionRuns `yaml:"runs"` -} - -type rawActionRuns struct { - Using string `yaml:"using"` - Steps []rawStep `yaml:"steps"` -} - -// parseCompositeActionUses reads /action.yml (or action.yaml) and, if it's a -// composite action, returns what its own steps reference - one hop outward from actionPath. -// -// JobUses.Remote is what the walk attributes from. JobUses.Local is the part which are detected -// and reported as not curated. -func parseCompositeActionUses(actionPath, declaredBy string) JobUses { - for _, name := range []string{"action.yml", "action.yaml"} { - data, err := os.ReadFile(filepath.Join(actionPath, name)) - if err != nil { - continue - } - var af rawActionFile - if err := yaml.Unmarshal(data, &af); err != nil { - log.Debug(fmt.Sprintf("github-actions curation: cannot parse %q - no transitive references attributed from it: %v", filepath.Join(actionPath, name), err)) - return JobUses{} - } - if af.Runs.Using != "composite" { - return JobUses{} - } - return parseSteps(af.Runs.Steps, declaredBy) - } - return JobUses{} -} - -// CrossReference enriches discovered entries with Subpaths and best-effort Parent metadata, and -// returns the enriched slice along with every local step it met on the way out. -// The local steps are a coverage statement, not attribution. -// -// A directly-used entry takes its Subpaths from the job's own uses: lines. Every other entry is -// attributed by parsing the action.yaml of composite actions. -// -// An action key can be invoked through more than one metadata location - its cache root, and/or -// one or more subpaths - and not always by the same parent: two different composites may each -// reference the same child at a different subpath. Every distinct location any parent references -// is scanned, regardless of which parent gets credited as Parent; only the Parent field is -// first-wins. -// -// Rounds are consumed by pairs, not by cache entries: one key contributes a pair per location it -// is referenced through, so a monorepo action reached through a chain of its own subpaths can -// need more rounds than the cache holds entries. Any bound derived from the entry count is -// therefore too small, and truncates silently - the unscanned subpath is still listed in -// Subpaths, so the result reads as complete. -// -// KNOWN LIMITATION: an action pulling others in via a run: step rather than its own uses:, and -// actions used by a called reusable workflow (jobs..uses:), are never attributed - Parent -// stays empty, never guessed. -func CrossReference(discovered []ActionRef, used JobUses) ([]ActionRef, []LocalUse) { - var localUses []LocalUse - // Deduplicated on the pair, not on the path: the same "./x" declared by two different - // composite actions is one uncovered action reached two ways, and a reader chasing it needs - // both declarers, while a repeat of the identical statement is noise. - seenLocal := map[LocalUse]bool{} - recordLocal := func(uses []LocalUse) { - for _, use := range uses { - if seenLocal[use] { - continue - } - seenLocal[use] = true - localUses = append(localUses, use) - } - } - - // The job's own local steps first, so the report lists them before the ones found deeper. - recordLocal(used.Local) - - byKey := make(map[string]int, len(discovered)) - for i := range discovered { - byKey[refKey(discovered[i].Owner, discovered[i].Repo, discovered[i].Ref)] = i - } - - // scanned tracks, per key, every location (a subpath, or "" for the cache root) whose - // action.yml has already been scanned or queued to be - across every parent that references - // the key, not just the first. Recording a location here is the signal that it is new: the - // guard against merging it into Subpaths twice, and against scanning it twice. - scanned := map[string]map[string]bool{} - markLocation := func(key, location string) (isNew bool) { - if scanned[key] == nil { - scanned[key] = map[string]bool{} - } - if scanned[key][location] { - return false - } - scanned[key][location] = true - return true - } - - // hasParent marks every key whose Parent is already settled - directly used by the job (no - // Parent to attribute), or attributed by an earlier, shallower round. First parent wins: once - // a key is here, a later round finding the same child through a different composite never - // overwrites Parent - but the location that reference was found at is still merged into - // Subpaths and scanned, since attribution and "what still needs reading" are separate guards. - hasParent := map[string]bool{} - - // pending pairs a key with the locations newly discovered for it this round, so the round - // loop below only re-reads metadata that is actually new. - type pending struct { - key string - locations []string - } - queueLocation := func(list *[]pending, idx map[string]int, key, location string) { - if i, ok := idx[key]; ok { - (*list)[i].locations = append((*list)[i].locations, location) - return - } - idx[key] = len(*list) - *list = append(*list, pending{key: key, locations: []string{location}}) - } - - var frontier []pending - frontierIdx := map[string]int{} - for _, u := range used.Remote { - key := refKey(u.Owner, u.Repo, u.Ref) - hasParent[key] = true // directly used by the job itself - no Parent to attribute - isNew := markLocation(key, u.Subpath) - if idx, ok := byKey[key]; ok && isNew && u.Subpath != "" { - discovered[idx].Subpaths = append(discovered[idx].Subpaths, u.Subpath) - } - if isNew { - queueLocation(&frontier, frontierIdx, key, u.Subpath) - } - } - - for len(frontier) > 0 { - var nextFrontier []pending - nextIdx := map[string]int{} - - for _, p := range frontier { - parentIdx, ok := byKey[p.key] - if !ok { - continue - } - parentIdentity := fmt.Sprintf("%s/%s@%s", discovered[parentIdx].Owner, discovered[parentIdx].Repo, discovered[parentIdx].Ref) - - // For owner/repo/subpath@ref, the action's own metadata lives at - // /////action.yml, not at the cache root - the root - // is only correct for a location that is itself the root (empty subpath). - for _, location := range p.locations { - metadataDir := discovered[parentIdx].Path - if location != "" { - metadataDir = filepath.Join(metadataDir, location) - } - compositeUses := parseCompositeActionUses(metadataDir, parentIdentity) - recordLocal(compositeUses.Local) - for _, cu := range compositeUses.Remote { - childKey := refKey(cu.Owner, cu.Repo, cu.Ref) - childIdx, ok := byKey[childKey] - if !ok { - continue - } - if !hasParent[childKey] { - discovered[childIdx].Parent = parentIdentity - hasParent[childKey] = true - } - isNew := markLocation(childKey, cu.Subpath) - if isNew && cu.Subpath != "" { - discovered[childIdx].Subpaths = append(discovered[childIdx].Subpaths, cu.Subpath) - } - if isNew { - queueLocation(&nextFrontier, nextIdx, childKey, cu.Subpath) - } - } - } - } - frontier = nextFrontier - } - return discovered, localUses -} - -func refKey(owner, repo, ref string) string { - return owner + "/" + repo + "@" + ref -} diff --git a/commands/curation/githubactions/workflow_test.go b/commands/curation/githubactions/workflow_test.go deleted file mode 100644 index 0302e77b9..000000000 --- a/commands/curation/githubactions/workflow_test.go +++ /dev/null @@ -1,826 +0,0 @@ -package githubactions - -import ( - "fmt" - "os" - "path/filepath" - "sort" - "strings" - "testing" - "time" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestParseWorkflowUses(t *testing.T) { - workflowPath := filepath.Join(fixturesRoot, "curation-project", ".github", "workflows", "ci.yml") - - uses, err := ParseWorkflowUses(workflowPath, "build") - assert.NoError(t, err) - - remote := uses.Remote - sort.Slice(remote, func(i, j int) bool { return remote[i].Owner+remote[i].Repo < remote[j].Owner+remote[j].Repo }) - - if assert.Len(t, remote, 2) { - assert.Equal(t, WorkflowUse{Owner: "actions", Repo: "checkout", Ref: "v4", Raw: "actions/checkout@v4"}, remote[0]) - assert.Equal(t, WorkflowUse{Owner: "github", Repo: "codeql-action", Subpath: "analyze", Ref: "v3", Raw: "github/codeql-action/analyze@v3"}, remote[1]) - } - // The local step is not a reference to curate, but it is the reason the cache is not the - // whole story - it has to survive parsing for the report to be able to say so. - assert.Equal(t, []LocalUse{{Raw: "./.github/actions/build-prep"}}, uses.Local, - "the job's own workflow declares it, so there is no declaring action to attribute it to") -} - -func TestParseUsesString(t *testing.T) { - tests := []struct { - name string - raw string - want WorkflowUse - ok bool - }{ - {"verify when the reference is owner repo and ref then it parses", "actions/checkout@v4", WorkflowUse{Owner: "actions", Repo: "checkout", Ref: "v4", Raw: "actions/checkout@v4"}, true}, - {"verify when the reference carries a subpath then the subpath is captured", "github/codeql-action/analyze@v3", WorkflowUse{Owner: "github", Repo: "codeql-action", Subpath: "analyze", Ref: "v3", Raw: "github/codeql-action/analyze@v3"}, true}, - {"verify when another subpath of the same repo is used then it parses independently", "github/codeql-action/init@v3", WorkflowUse{Owner: "github", Repo: "codeql-action", Subpath: "init", Ref: "v3", Raw: "github/codeql-action/init@v3"}, true}, - {"verify when the subpath is nested then the whole remainder is captured", "a/b/c/d@v1", WorkflowUse{Owner: "a", Repo: "b", Subpath: "c/d", Ref: "v1", Raw: "a/b/c/d@v1"}, true}, - {"verify when the reference is a local action then it is skipped", "./.github/actions/build-prep", WorkflowUse{}, false}, - {"verify when the reference is a docker uri then it is skipped", "docker://alpine:3", WorkflowUse{}, false}, - {"verify when the reference has no ref then it is skipped", "actions/checkout", WorkflowUse{}, false}, - {"verify when the reference is empty then it is skipped", "", WorkflowUse{}, false}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, ok := parseUsesString(tt.raw) - assert.Equal(t, tt.ok, ok) - if tt.ok { - assert.Equal(t, tt.want, got) - } - }) - } -} - -// writeCompositeAction writes a composite action.yml at dir referencing usesRaw (empty for a -// non-composite leaf action), for building multi-level transitive chains in tests. -func writeCompositeAction(t *testing.T, dir, usesRaw string) { - t.Helper() - if usesRaw == "" { - require.NoError(t, os.WriteFile(filepath.Join(dir, "action.yml"), []byte("runs:\n using: node20\n"), 0600)) - return - } - content := "runs:\n using: composite\n steps:\n - uses: " + usesRaw + "\n" - require.NoError(t, os.WriteFile(filepath.Join(dir, "action.yml"), []byte(content), 0600)) -} - -// buildChain writes n composite actions (org/action1@v1 -> org/action2@v1 -> ... -> org/actionN@v1, -// the last one non-composite) into fresh temp dirs and returns the []ActionRef for all of them. -func buildChain(t *testing.T, n int) []ActionRef { - t.Helper() - discovered := make([]ActionRef, n) - for i := 1; i <= n; i++ { - dir := t.TempDir() - discovered[i-1] = ActionRef{Owner: "org", Repo: fmt.Sprintf("action%d", i), Ref: "v1", Path: dir} - if i < n { - writeCompositeAction(t, dir, fmt.Sprintf("org/action%d@v1", i+1)) - } else { - writeCompositeAction(t, dir, "") - } - } - return discovered -} - -// discoveredAction is one entry in a cross-reference case's action cache: the triple the walk -// would have found, plus the action.yml bodies to plant under it keyed by subpath ("" for the -// cache root). Expressing the fixture this way keeps the cases data rather than per-row setup. -type discoveredAction struct { - key string // "owner/repo@ref" - yamls map[string]string // subpath -> action.yml body -} - -// compositeYAML is an action.yml for a composite action whose single step references uses. -func compositeYAML(uses string) string { - return "runs:\n using: composite\n steps:\n - uses: " + uses + "\n" -} - -// unreadableYAML is accepted by GitHub's runner but rejected by yaml.v3 (duplicate mapping keys). -const unreadableYAML = "name: w\nname: w\nruns:\n using: composite\n steps:\n - uses: actions/setup-node@v4\n" - -// buildDiscovered materializes each action into its own directory and returns the []ActionRef -// CrossReference would have been handed. -func buildDiscovered(t *testing.T, actions []discoveredAction) []ActionRef { - t.Helper() - refs := make([]ActionRef, 0, len(actions)) - for _, a := range actions { - owner, rest, _ := strings.Cut(a.key, "/") - repo, ref, _ := strings.Cut(rest, "@") - dir := t.TempDir() - for subpath, body := range a.yamls { - target := filepath.Join(dir, filepath.FromSlash(subpath)) - require.NoError(t, os.MkdirAll(target, 0755)) - require.NoError(t, os.WriteFile(filepath.Join(target, "action.yml"), []byte(body), 0600)) - } - refs = append(refs, ActionRef{Owner: owner, Repo: repo, Ref: ref, Path: dir}) - } - return refs -} - -func TestCrossReference(t *testing.T) { - tests := []struct { - name string - discovered []discoveredAction - used []WorkflowUse - wantRepos []string // every entry that must survive; attribution is additive - wantParents map[string]string // repo -> Parent ("" means none may be guessed) - wantSubpaths map[string][]string // repo -> Subpaths - }{ - { - name: "verify when an entry is used directly then it gets no parent and no unused subpath", - discovered: []discoveredAction{{key: "actions/checkout@v4"}}, - used: []WorkflowUse{{Owner: "actions", Repo: "checkout", Ref: "v4"}}, - wantRepos: []string{"checkout"}, - wantParents: map[string]string{"checkout": ""}, - wantSubpaths: map[string][]string{"checkout": nil}, - }, - { - name: "verify when no workflow explains an entry then its parent stays empty rather than guessed", - discovered: []discoveredAction{{key: "some-org/mystery-action@v1"}}, - wantRepos: []string{"mystery-action"}, - wantParents: map[string]string{"mystery-action": ""}, - wantSubpaths: map[string][]string{"mystery-action": nil}, - }, - { - name: "verify when an entry cannot be attributed then it is still not dropped", - discovered: []discoveredAction{ - {key: "actions/checkout@v4"}, - {key: "some-other-org/unexplained@v9"}, - }, - used: []WorkflowUse{{Owner: "actions", Repo: "checkout", Ref: "v4"}}, - wantRepos: []string{"checkout", "unexplained"}, - wantParents: map[string]string{"unexplained": ""}, - }, - { - // codeql-action is commonly invoked twice in one job, init@v3 then analyze@v3. - name: "verify when a monorepo action is invoked via several subpaths then all of them are collected", - discovered: []discoveredAction{{key: "github/codeql-action@v3"}}, - used: []WorkflowUse{ - {Owner: "github", Repo: "codeql-action", Ref: "v3", Subpath: "init"}, - {Owner: "github", Repo: "codeql-action", Ref: "v3", Subpath: "analyze"}, - }, - wantRepos: []string{"codeql-action"}, - wantSubpaths: map[string][]string{"codeql-action": {"init", "analyze"}}, - }, - { - name: "verify when a transitive reference carries a subpath then the subpath survives", - discovered: []discoveredAction{ - {key: "my-org/wrapper-action@v1", yamls: map[string]string{"": compositeYAML("github/codeql-action/analyze@v3")}}, - {key: "github/codeql-action@v3"}, - }, - used: []WorkflowUse{{Owner: "my-org", Repo: "wrapper-action", Ref: "v1"}}, - wantRepos: []string{"wrapper-action", "codeql-action"}, - wantParents: map[string]string{"codeql-action": "my-org/wrapper-action@v1"}, - wantSubpaths: map[string][]string{"codeql-action": {"analyze"}}, - }, - { - // The cache root is deliberately non-composite, so falling back to it would leave both - // transitive entries unattributed. - name: "verify when subpaths have their own metadata then each is read rather than the cache root", - discovered: []discoveredAction{ - {key: "github/codeql-action@v3", yamls: map[string]string{ - "": "runs:\n using: node20\n", - "init": compositeYAML("org/from-init@v1"), - "analyze": compositeYAML("org/from-analyze@v1"), - }}, - {key: "org/from-init@v1"}, - {key: "org/from-analyze@v1"}, - }, - used: []WorkflowUse{ - {Owner: "github", Repo: "codeql-action", Ref: "v3", Subpath: "init"}, - {Owner: "github", Repo: "codeql-action", Ref: "v3", Subpath: "analyze"}, - }, - wantRepos: []string{"codeql-action", "from-init", "from-analyze"}, - wantParents: map[string]string{ - "from-init": "github/codeql-action@v3", - "from-analyze": "github/codeql-action@v3", - }, - }, - { - name: "verify when a composite action.yml cannot be read then its child survives unattributed", - discovered: []discoveredAction{ - {key: "some-org/wrapper@v1", yamls: map[string]string{"": unreadableYAML}}, - {key: "actions/setup-node@v4"}, - }, - used: []WorkflowUse{{Owner: "some-org", Repo: "wrapper", Ref: "v1"}}, - wantRepos: []string{"wrapper", "setup-node"}, - wantParents: map[string]string{"setup-node": ""}, - }, - { - name: "verify when nothing was discovered then nothing is returned", - wantRepos: nil, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, _ := CrossReference(buildDiscovered(t, tt.discovered), JobUses{Remote: tt.used}) - - byRepo := make(map[string]ActionRef, len(got)) - repos := make([]string, len(got)) - for i, ref := range got { - byRepo[ref.Repo] = ref - repos[i] = ref.Repo - } - assert.ElementsMatch(t, tt.wantRepos, repos, "attribution is additive - it may never drop an entry") - for repo, wantParent := range tt.wantParents { - assert.Equal(t, wantParent, byRepo[repo].Parent, "parent of %s", repo) - } - for repo, wantSubpaths := range tt.wantSubpaths { - assert.Equal(t, wantSubpaths, byRepo[repo].Subpaths, "subpaths of %s", repo) - } - }) - } -} - -func TestParseCompositeActionUses(t *testing.T) { - tests := []struct { - name string - yaml string - why string - }{ - { - name: "verify when the action.yml cannot be parsed then nothing is attributed and the run continues", - yaml: unreadableYAML, - why: "a file this parser cannot read attributes nothing, and is not a failure of the run", - }, - { - name: "verify when the action is not composite then nothing is referenced", - yaml: "runs:\n using: node20\n", - why: "only composite actions declare uses: steps of their own", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - dir := t.TempDir() - require.NoError(t, os.WriteFile(filepath.Join(dir, "action.yml"), []byte(tt.yaml), 0600)) - - got := parseCompositeActionUses(dir, "org/declarer@v1") - - assert.Empty(t, got.Remote, tt.why) - assert.Empty(t, got.Local, "and no local step either - "+tt.why) - }) - } -} - -func TestParseWorkflowUses_JobMustBeIdentified(t *testing.T) { - // Attribution needs to know which job it is describing. Every other job in the file ran on - // its own runner with its own cache, so attributing from them is worse than not attributing: - // it would label an entry with a parent that never pulled it in. - workflowPath := filepath.Join(writeWorkflows(t, map[string]string{ - "ci.yml": "jobs:\n" + - jobWithUses("build", "actions/checkout@v4") + - jobWithUses("publish", "actions/upload-artifact@v4"), - }), "ci.yml") - - tests := []struct { - name string - // jobID is the job to attribute against; "" is the local-invocation case, since a runner - // always sets GITHUB_JOB. - jobID string - // wantErrContains names what the message must surface so the mismatch is diagnosable - // from the log alone. - wantErrContains []string - }{ - { - name: "verify when the file does not declare the job then the error names it and the jobs that exist", - jobID: "a-job-declared-somewhere-else", - wantErrContains: []string{"a-job-declared-somewhere-else", "build", "publish"}, - }, - { - name: "verify when no job id is given then attribution is refused", - jobID: "", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - uses, err := ParseWorkflowUses(workflowPath, tt.jobID) - - assert.ErrorIs(t, err, ErrJobUnknown) - assert.Empty(t, uses.Remote, "no uses: may be returned from jobs that ran on other runners") - assert.Empty(t, uses.Local, "nor may a local step, for the same reason") - for _, want := range tt.wantErrContains { - assert.ErrorContains(t, err, want) - } - }) - } -} - -func TestCrossReference_LongChainFullyAttributedWithNoFixedDepthLimit(t *testing.T) { - // A 6-node chain: action1 (direct) -> action2 -> ... -> action6, each composite referencing - // the next. There is no fixed depth constant to satisfy here - the walk's bound scales with - // len(discovered), so every hop must be attributed regardless of chain length. - const n = 6 - discovered := buildChain(t, n) - used := JobUses{Remote: []WorkflowUse{{Owner: "org", Repo: "action1", Ref: "v1"}}} - - got, _ := CrossReference(discovered, used) - - byRepo := map[string]ActionRef{} - for _, ref := range got { - byRepo[ref.Repo] = ref - } - assert.Empty(t, byRepo["action1"].Parent) - for i := 2; i <= n; i++ { - assert.Equal(t, fmt.Sprintf("org/action%d@v1", i-1), byRepo[fmt.Sprintf("action%d", i)].Parent, "hop %d must be attributed", i-1) - } -} - -func TestCrossReference_CycleDoesNotHang(t *testing.T) { - // action1 (direct) -> action2 -> action1: a cycle back to an already-direct entry. - // visited/attributed dedup must stop this from looping forever, independent of the - // len(discovered)-based round bound. - path1, path2 := t.TempDir(), t.TempDir() - writeCompositeAction(t, path1, "org/action2@v1") - writeCompositeAction(t, path2, "org/action1@v1") - - discovered := []ActionRef{ - {Owner: "org", Repo: "action1", Ref: "v1", Path: path1}, - {Owner: "org", Repo: "action2", Ref: "v1", Path: path2}, - } - used := JobUses{Remote: []WorkflowUse{{Owner: "org", Repo: "action1", Ref: "v1"}}} - - done := make(chan []ActionRef, 1) - go func() { - got, _ := CrossReference(discovered, used) - done <- got - }() - select { - case got := <-done: - byRepo := map[string]ActionRef{} - for _, ref := range got { - byRepo[ref.Repo] = ref - } - assert.Empty(t, byRepo["action1"].Parent, "action1 is direct - the cycle must not overwrite that") - assert.Equal(t, "org/action1@v1", byRepo["action2"].Parent) - case <-time.After(5 * time.Second): - t.Fatal("CrossReference did not return - cycle handling regressed") - } -} - -func TestCrossReference_SharedChildParentFollowsWorkflowOrder(t *testing.T) { - pathA, pathB, pathChild := t.TempDir(), t.TempDir(), t.TempDir() - writeCompositeAction(t, pathA, "org/shared-child@v1") - writeCompositeAction(t, pathB, "org/shared-child@v1") - - seen := map[string]bool{} - for range 200 { - got, _ := CrossReference( - []ActionRef{ - {Owner: "org", Repo: "parent-a", Ref: "v1", Path: pathA}, - {Owner: "org", Repo: "parent-b", Ref: "v1", Path: pathB}, - {Owner: "org", Repo: "shared-child", Ref: "v1", Path: pathChild}, - }, - JobUses{Remote: []WorkflowUse{ - {Owner: "org", Repo: "parent-a", Ref: "v1"}, - {Owner: "org", Repo: "parent-b", Ref: "v1"}, - }}) - for _, ref := range got { - if ref.Repo == "shared-child" { - seen[ref.Parent] = true - } - } - } - - assert.Equal(t, map[string]bool{"org/parent-a@v1": true}, seen, - "when two parents pull in the same child, the first in the file's order must win, every run") -} - -// Two refs of one monorepo, each invoked through a different subpath - github/codeql-action -// init@v2 and analyze@v3. Subpaths are keyed on owner/repo@ref, so this has its own test rather -// than a table row: TestCrossReference keys its expectations by repo alone, which cannot tell the -// two apart. -func TestCrossReference_SubpathsDoNotBleedBetweenRefsOfOneRepo(t *testing.T) { - discovered := buildDiscovered(t, []discoveredAction{ - {key: "github/codeql-action@v2"}, - {key: "github/codeql-action@v3"}, - }) - - got, _ := CrossReference(discovered, JobUses{Remote: []WorkflowUse{ - {Owner: "github", Repo: "codeql-action", Ref: "v2", Subpath: "init"}, - {Owner: "github", Repo: "codeql-action", Ref: "v3", Subpath: "analyze"}, - }}) - - byRef := map[string][]string{} - for _, ref := range got { - byRef[ref.Ref] = ref.Subpaths - } - assert.Equal(t, map[string][]string{"v2": {"init"}, "v3": {"analyze"}}, byRef, - "each ref must carry only the subpath it was invoked through") -} - -func TestCrossReference_TransitiveParentAttributedFromCompositeActionYml(t *testing.T) { - actionsDir := filepath.Join(fixturesRoot, "curation-project", "_work", "_actions") - scan, err := DiscoverActionCache(actionsDir) - assert.NoError(t, err) - require.Empty(t, scan.Unaccounted) - discovered := scan.Refs - - workflowPath := filepath.Join(fixturesRoot, "curation-project", ".github", "workflows", "ci.yml") - used, err := ParseWorkflowUses(workflowPath, "build") - assert.NoError(t, err) - - got, _ := CrossReference(discovered, used) - - byRepo := map[string]ActionRef{} - for _, ref := range got { - byRepo[ref.Repo] = ref - } - - assert.Empty(t, byRepo["checkout"].Parent, "directly-used, non-composite action must have no parent") - assert.Equal(t, []string{"analyze"}, byRepo["codeql-action"].Subpaths) - assert.Empty(t, byRepo["codeql-action"].Parent, "the top-level composite action itself has no parent") - assert.Equal(t, "github/codeql-action@v3", byRepo["transitive-action"].Parent, "pulled in only via codeql-action's own action.yml") -} - -func TestCrossReference_RootAndSubpathBothUsed_BothMetadataLocationsAreRead(t *testing.T) { - // github/codeql-action is invoked once at its root and once through a subpath in the same - // job. Both locations carry their own action.yml with a different transitive child, so both - // must be read - not just the subpath, which is what collectSubpaths' root-drop bug left out. - discovered := buildDiscovered(t, []discoveredAction{ - {key: "github/codeql-action@v3", yamls: map[string]string{ - "": compositeYAML("org/from-root@v1"), - "init": compositeYAML("org/from-init@v1"), - }}, - {key: "org/from-root@v1"}, - {key: "org/from-init@v1"}, - }) - - got, _ := CrossReference(discovered, JobUses{Remote: []WorkflowUse{ - {Owner: "github", Repo: "codeql-action", Ref: "v3"}, - {Owner: "github", Repo: "codeql-action", Ref: "v3", Subpath: "init"}, - }}) - - byRepo := map[string]ActionRef{} - for _, ref := range got { - byRepo[ref.Repo] = ref - } - assert.Equal(t, []string{"init"}, byRepo["codeql-action"].Subpaths, "the root use itself is not a subpath") - assert.Equal(t, "github/codeql-action@v3", byRepo["from-root"].Parent, "the root's own action.yml must be read, not skipped") - assert.Equal(t, "github/codeql-action@v3", byRepo["from-init"].Parent) -} - -func TestCrossReference_ChildReferencedByTwoParentsAtDifferentSubpaths_BothAreScanned(t *testing.T) { - // parent-a references shared-child@v1 directly (root), parent-b references it via a subpath. - // The first parent in file order wins Parent, but shared-child's own subpath metadata (pulled - // in only through parent-b's reference) must still be scanned rather than dropped once - // shared-child is already attributed via parent-a. - discovered := buildDiscovered(t, []discoveredAction{ - {key: "org/parent-a@v1", yamls: map[string]string{"": compositeYAML("org/shared-child@v1")}}, - {key: "org/parent-b@v1", yamls: map[string]string{"": compositeYAML("org/shared-child/sub@v1")}}, - {key: "org/shared-child@v1", yamls: map[string]string{"sub": compositeYAML("org/from-sub@v1")}}, - {key: "org/from-sub@v1"}, - }) - - got, _ := CrossReference(discovered, JobUses{Remote: []WorkflowUse{ - {Owner: "org", Repo: "parent-a", Ref: "v1"}, - {Owner: "org", Repo: "parent-b", Ref: "v1"}, - }}) - - byRepo := map[string]ActionRef{} - for _, ref := range got { - byRepo[ref.Repo] = ref - } - assert.Equal(t, "org/parent-a@v1", byRepo["shared-child"].Parent, "first parent in file order still wins") - assert.Equal(t, []string{"sub"}, byRepo["shared-child"].Subpaths, "parent-b's subpath reference must still be merged in") - assert.Equal(t, "org/shared-child@v1", byRepo["from-sub"].Parent, "shared-child/sub's own metadata must still be scanned") -} - -// writeWorkflows writes each name->content pair as a file in a fresh temp dir and returns it. -func writeWorkflows(t *testing.T, files map[string]string) string { - t.Helper() - dir := t.TempDir() - for name, content := range files { - require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte(content), 0600)) - } - return dir -} - -// jobWithUses renders a minimal workflow job declaring one uses: step per ref. -func jobWithUses(jobID string, refs ...string) string { - job := " " + jobID + ":\n steps:\n" - for _, ref := range refs { - job += " - uses: " + ref + "\n" - } - return job -} - -func TestParseWorkflowUses_ScopesToTheRunningJob(t *testing.T) { - // The command curates the job it runs in. An action referenced only by a sibling job in - // the same workflow file is not part of this job's dependency graph. - dir := writeWorkflows(t, map[string]string{ - "ci.yml": "jobs:\n" + - jobWithUses("build", "actions/checkout@v4") + - jobWithUses("publish", "actions/upload-artifact@v4"), - }) - workflowPath := filepath.Join(dir, "ci.yml") - - tests := []struct { - name string - jobID string - wantRepos []string - }{ - {"verify when the job is build then only its own uses are returned", "build", []string{"checkout"}}, - {"verify when the job is publish then only its own uses are returned", "publish", []string{"upload-artifact"}}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - uses, err := ParseWorkflowUses(workflowPath, tt.jobID) - require.NoError(t, err) - assert.ElementsMatch(t, tt.wantRepos, repoNames(uses.Remote)) - }) - } -} - -func TestParseWorkflowUses_SubpathOrderIsStable(t *testing.T) { - // A monorepo action invoked twice in the same job collapses to one cache entry carrying both - // subpaths, and that order is rendered into the report's Action cell. Steps are a slice, so - // the order is the file's - this pins that nothing downstream reintroduces map iteration. - workflowPath := filepath.Join(writeWorkflows(t, map[string]string{ - "ci.yml": "jobs:\n" + jobWithUses("build", - "github/codeql-action/init@v3", "github/codeql-action/analyze@v3"), - }), "ci.yml") - - seen := map[string]bool{} - for range 200 { - used, err := ParseWorkflowUses(workflowPath, "build") - require.NoError(t, err) - got, _ := CrossReference([]ActionRef{{Owner: "github", Repo: "codeql-action", Ref: "v3", Path: "/nonexistent"}}, used) - seen[NewActionReportRow(got[0], ActionCurationResult{Status: ActionApproved}).Action] = true - } - assert.Equal(t, map[string]bool{"github/codeql-action (init, analyze)": true}, seen, - "the rendered report cell must follow the file's step order, every run") -} - -func repoNames(uses []WorkflowUse) []string { - if len(uses) == 0 { - return nil - } - repos := make([]string, len(uses)) - for i, u := range uses { - repos[i] = u.Repo - } - return repos -} - -// compositeYAMLWithSteps is an action.yml for a composite action with several uses: steps, for -// mixing remote references and local ones in a single action. -func compositeYAMLWithSteps(uses ...string) string { - yaml := "runs:\n using: composite\n steps:\n" - for _, use := range uses { - yaml += " - uses: " + use + "\n" - } - return yaml -} - -func TestCrossReference_LocalStepsDeclaredByCompositeActionsAreCollected(t *testing.T) { - // A relative uses: inside a composite action names a path in the CALLER's repository, so the - // runner resolves it from the workspace when that step runs rather than into the action cache - // this command read. It is the same coverage gap as a local step in the workflow, one level - // down, and only this walk can see it - so it has to come back out of here. - tests := []struct { - name string - discovered []discoveredAction - used JobUses - want []LocalUse - }{ - { - name: "verify when a composite action declares a local step then it is reported against that action", - discovered: []discoveredAction{ - {key: "some-org/wrapper@v1", yamls: map[string]string{"": compositeYAML("./scripts/build")}}, - }, - used: JobUses{Remote: []WorkflowUse{{Owner: "some-org", Repo: "wrapper", Ref: "v1"}}}, - want: []LocalUse{{Raw: "./scripts/build", DeclaredBy: "some-org/wrapper@v1"}}, - }, - { - name: "verify when the local step sits several hops out then the walk still reaches it", - discovered: []discoveredAction{ - {key: "org/outer@v1", yamls: map[string]string{"": compositeYAML("org/inner@v1")}}, - {key: "org/inner@v1", yamls: map[string]string{"": compositeYAML("./deep/local")}}, - }, - used: JobUses{Remote: []WorkflowUse{{Owner: "org", Repo: "outer", Ref: "v1"}}}, - want: []LocalUse{{Raw: "./deep/local", DeclaredBy: "org/inner@v1"}}, - }, - { - name: "verify when a composite mixes remote and local steps then only the local one is reported uncovered", - discovered: []discoveredAction{ - {key: "org/wrapper@v1", yamls: map[string]string{ - "": compositeYAMLWithSteps("actions/setup-node@v4", "./scripts/build"), - }}, - {key: "actions/setup-node@v4"}, - }, - used: JobUses{Remote: []WorkflowUse{{Owner: "org", Repo: "wrapper", Ref: "v1"}}}, - want: []LocalUse{{Raw: "./scripts/build", DeclaredBy: "org/wrapper@v1"}}, - }, - { - name: "verify when the metadata lives at a subpath then the local step under it is still found", - // The root action.yml is not the one invoked, so a local step declared only by the - // subpath's metadata is exactly the one a root-only read would miss. - discovered: []discoveredAction{ - {key: "github/codeql-action@v3", yamls: map[string]string{ - "": compositeYAML("org/from-root@v1"), - "analyze": compositeYAML("./scripts/analyze"), - }}, - {key: "org/from-root@v1"}, - }, - used: JobUses{Remote: []WorkflowUse{ - {Owner: "github", Repo: "codeql-action", Ref: "v3", Subpath: "analyze"}, - }}, - want: []LocalUse{{Raw: "./scripts/analyze", DeclaredBy: "github/codeql-action@v3"}}, - }, - { - name: "verify when the job and a composite both declare local steps then the job's comes first", - // Order is the report's reading order: what the workflow itself declares is what a - // reader can act on directly, so it should not be buried under transitive findings. - discovered: []discoveredAction{ - {key: "org/wrapper@v1", yamls: map[string]string{"": compositeYAML("./from-composite")}}, - }, - used: JobUses{ - Remote: []WorkflowUse{{Owner: "org", Repo: "wrapper", Ref: "v1"}}, - Local: []LocalUse{{Raw: "./from-workflow"}}, - }, - want: []LocalUse{ - {Raw: "./from-workflow"}, - {Raw: "./from-composite", DeclaredBy: "org/wrapper@v1"}, - }, - }, - { - name: "verify when two composites declare the same path then both declarers are reported", - // "./x" resolves against the workspace either way, so it is one action reached two - // ways - but a reader chasing it needs to know both places it is referenced from. - discovered: []discoveredAction{ - {key: "org/parent-a@v1", yamls: map[string]string{"": compositeYAML("./shared")}}, - {key: "org/parent-b@v1", yamls: map[string]string{"": compositeYAML("./shared")}}, - }, - used: JobUses{Remote: []WorkflowUse{ - {Owner: "org", Repo: "parent-a", Ref: "v1"}, - {Owner: "org", Repo: "parent-b", Ref: "v1"}, - }}, - want: []LocalUse{ - {Raw: "./shared", DeclaredBy: "org/parent-a@v1"}, - {Raw: "./shared", DeclaredBy: "org/parent-b@v1"}, - }, - }, - { - name: "verify when one composite declares the same local step twice then it is reported once", - discovered: []discoveredAction{ - {key: "org/wrapper@v1", yamls: map[string]string{ - "": compositeYAMLWithSteps("./scripts/build", "./scripts/build"), - }}, - }, - used: JobUses{Remote: []WorkflowUse{{Owner: "org", Repo: "wrapper", Ref: "v1"}}}, - want: []LocalUse{{Raw: "./scripts/build", DeclaredBy: "org/wrapper@v1"}}, - }, - { - name: "verify when an action.yml cannot be parsed then no local step is invented from it", - discovered: []discoveredAction{ - {key: "org/wrapper@v1", yamls: map[string]string{"": unreadableYAML}}, - }, - used: JobUses{Remote: []WorkflowUse{{Owner: "org", Repo: "wrapper", Ref: "v1"}}}, - want: nil, - }, - { - name: "verify when no composite declares a local step then nothing is reported uncovered", - discovered: []discoveredAction{ - {key: "org/wrapper@v1", yamls: map[string]string{"": compositeYAML("actions/setup-node@v4")}}, - {key: "actions/setup-node@v4"}, - }, - used: JobUses{Remote: []WorkflowUse{{Owner: "org", Repo: "wrapper", Ref: "v1"}}}, - want: nil, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - _, got := CrossReference(buildDiscovered(t, tt.discovered), tt.used) - - assert.Equal(t, tt.want, got) - }) - } -} - -func TestCrossReference_LocalStepsDoNotDisturbAttribution(t *testing.T) { - // The local steps are collected on the same walk that attributes Parent. A composite whose - // steps are part local, part remote must still attribute the remote ones. - discovered := buildDiscovered(t, []discoveredAction{ - {key: "org/wrapper@v1", yamls: map[string]string{ - "": compositeYAMLWithSteps("./scripts/build", "org/child@v1"), - }}, - {key: "org/child@v1"}, - }) - - got, localUses := CrossReference(discovered, JobUses{ - Remote: []WorkflowUse{{Owner: "org", Repo: "wrapper", Ref: "v1"}}, - }) - - byRepo := map[string]ActionRef{} - for _, ref := range got { - byRepo[ref.Repo] = ref - } - assert.Equal(t, "org/wrapper@v1", byRepo["child"].Parent, "a local sibling step must not cost the remote one its parent") - assert.Equal(t, []LocalUse{{Raw: "./scripts/build", DeclaredBy: "org/wrapper@v1"}}, localUses) -} - -func TestCrossReference_SubpathChainLongerThanTheCacheIsFullyWalked(t *testing.T) { - // ONE cache entry reached through a chain of its own subpaths: every hop is a new (key, - // location) pair on the same key, so the walk needs more rounds than the cache has entries. - // Any bound derived from the entry count truncates this, and does it invisibly - the - // unscanned subpath is still listed in Subpaths, so the Action cell reads as complete while - // the local step that subpath's metadata declares is never collected. - tests := []struct { - name string - yamls map[string]string - wantSubpaths []string - wantLocalUses []LocalUse - }{ - { - name: "verify when the chain is two subpaths deep then the last one's metadata is read", - yamls: map[string]string{ - "": compositeYAML("org/mono/s1@v1"), - "s1": compositeYAML("org/mono/s2@v1"), - "s2": compositeYAML("./local-at-s2"), - }, - wantSubpaths: []string{"s1", "s2"}, - wantLocalUses: []LocalUse{{Raw: "./local-at-s2", DeclaredBy: "org/mono@v1"}}, - }, - { - name: "verify when the chain is four subpaths deep then the walk still reaches the end", - yamls: map[string]string{ - "": compositeYAML("org/mono/s1@v1"), - "s1": compositeYAML("org/mono/s2@v1"), - "s2": compositeYAML("org/mono/s3@v1"), - "s3": compositeYAML("org/mono/s4@v1"), - "s4": compositeYAML("./local-at-s4"), - }, - wantSubpaths: []string{"s1", "s2", "s3", "s4"}, - wantLocalUses: []LocalUse{{Raw: "./local-at-s4", DeclaredBy: "org/mono@v1"}}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - discovered := buildDiscovered(t, []discoveredAction{{key: "org/mono@v1", yamls: tt.yamls}}) - - got, localUses := CrossReference(discovered, JobUses{ - Remote: []WorkflowUse{{Owner: "org", Repo: "mono", Ref: "v1"}}, - }) - - assert.Equal(t, tt.wantSubpaths, got[0].Subpaths) - assert.Equal(t, tt.wantLocalUses, localUses, - "a subpath listed in the report must have had its metadata read, or the report overstates coverage") - }) - } -} - -func TestCrossReference_ChainLongerThanTheCacheAttributesEveryHop(t *testing.T) { - // The Parent half of the same shape: two cache entries, but the chain to the second runs - // through three subpath locations, so attribution needs more rounds than there are entries. - discovered := buildDiscovered(t, []discoveredAction{ - {key: "org/mono@v1", yamls: map[string]string{ - "": compositeYAML("org/mono/s1@v1"), - "s1": compositeYAML("org/mono/s2@v1"), - "s2": compositeYAML("org/mono/s3@v1"), - "s3": compositeYAML("org/leaf@v1"), - }}, - {key: "org/leaf@v1"}, - }) - - got, _ := CrossReference(discovered, JobUses{ - Remote: []WorkflowUse{{Owner: "org", Repo: "mono", Ref: "v1"}}, - }) - - byRepo := map[string]ActionRef{} - for _, ref := range got { - byRepo[ref.Repo] = ref - } - assert.Equal(t, "org/mono@v1", byRepo["leaf"].Parent, "every hop must be attributed, however long the chain") -} - -func TestCrossReference_SubpathCycleDoesNotHang(t *testing.T) { - // Termination rests entirely on markLocation. A cycle through SUBPATHS of one key is the - // hardest case for it: every hop is a new location on a key that is already attributed, so - // nothing but the location dedup stops the walk. - discovered := buildDiscovered(t, []discoveredAction{ - {key: "org/mono@v1", yamls: map[string]string{ - "": compositeYAML("org/mono/s1@v1"), - "s1": compositeYAML("org/mono/s2@v1"), - "s2": compositeYAML("org/mono/s1@v1"), // back to s1 - }}, - }) - used := JobUses{Remote: []WorkflowUse{{Owner: "org", Repo: "mono", Ref: "v1"}}} - - done := make(chan []string, 1) - go func() { - got, _ := CrossReference(discovered, used) - done <- got[0].Subpaths - }() - select { - case subpaths := <-done: - assert.Equal(t, []string{"s1", "s2"}, subpaths, "each location is scanned once, and the cycle adds nothing new") - case <-time.After(5 * time.Second): - t.Fatal("CrossReference did not return - a subpath cycle must terminate on markLocation alone") - } -} diff --git a/tests/testdata/projects/githubactions/curation-project/.github/workflows/ci.yml b/tests/testdata/projects/githubactions/curation-project/.github/workflows/ci.yml deleted file mode 100644 index 2b4c4a08a..000000000 --- a/tests/testdata/projects/githubactions/curation-project/.github/workflows/ci.yml +++ /dev/null @@ -1,10 +0,0 @@ -name: CI Pipeline -on: [push, pull_request] -jobs: - build: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - - uses: github/codeql-action/analyze@v3 - - uses: ./.github/actions/build-prep - - run: echo "build" diff --git a/tests/testdata/projects/githubactions/curation-project/_work/_actions/actions/checkout/v4.completed b/tests/testdata/projects/githubactions/curation-project/_work/_actions/actions/checkout/v4.completed deleted file mode 100644 index f39465da2..000000000 --- a/tests/testdata/projects/githubactions/curation-project/_work/_actions/actions/checkout/v4.completed +++ /dev/null @@ -1 +0,0 @@ -1970-01-01T00:00:00.0000000Z \ No newline at end of file diff --git a/tests/testdata/projects/githubactions/curation-project/_work/_actions/actions/checkout/v4/action.yml b/tests/testdata/projects/githubactions/curation-project/_work/_actions/actions/checkout/v4/action.yml deleted file mode 100644 index 5f210f36d..000000000 --- a/tests/testdata/projects/githubactions/curation-project/_work/_actions/actions/checkout/v4/action.yml +++ /dev/null @@ -1,5 +0,0 @@ -name: 'Checkout' -description: 'Checkout a Git repository' -runs: - using: 'node20' - main: 'dist/index.js' diff --git a/tests/testdata/projects/githubactions/curation-project/_work/_actions/github/codeql-action/v3.completed b/tests/testdata/projects/githubactions/curation-project/_work/_actions/github/codeql-action/v3.completed deleted file mode 100644 index f39465da2..000000000 --- a/tests/testdata/projects/githubactions/curation-project/_work/_actions/github/codeql-action/v3.completed +++ /dev/null @@ -1 +0,0 @@ -1970-01-01T00:00:00.0000000Z \ No newline at end of file diff --git a/tests/testdata/projects/githubactions/curation-project/_work/_actions/github/codeql-action/v3/action.yml b/tests/testdata/projects/githubactions/curation-project/_work/_actions/github/codeql-action/v3/action.yml deleted file mode 100644 index c13e3e027..000000000 --- a/tests/testdata/projects/githubactions/curation-project/_work/_actions/github/codeql-action/v3/action.yml +++ /dev/null @@ -1,5 +0,0 @@ -name: 'CodeQL' -description: 'Root action.yml - not composite, and must not be consulted when a subpath (e.g. analyze) is the one actually invoked' -runs: - using: 'node20' - main: 'dist/index.js' diff --git a/tests/testdata/projects/githubactions/curation-project/_work/_actions/github/codeql-action/v3/analyze/action.yml b/tests/testdata/projects/githubactions/curation-project/_work/_actions/github/codeql-action/v3/analyze/action.yml deleted file mode 100644 index 4a2cef836..000000000 --- a/tests/testdata/projects/githubactions/curation-project/_work/_actions/github/codeql-action/v3/analyze/action.yml +++ /dev/null @@ -1,8 +0,0 @@ -name: 'CodeQL: analyze' -description: 'Composite action bundling the analyze subpath' -runs: - using: 'composite' - steps: - - uses: some-org/transitive-action@v1 - - run: echo "analyze" - shell: bash diff --git a/tests/testdata/projects/githubactions/curation-project/_work/_actions/some-org/transitive-action/v1.completed b/tests/testdata/projects/githubactions/curation-project/_work/_actions/some-org/transitive-action/v1.completed deleted file mode 100644 index f39465da2..000000000 --- a/tests/testdata/projects/githubactions/curation-project/_work/_actions/some-org/transitive-action/v1.completed +++ /dev/null @@ -1 +0,0 @@ -1970-01-01T00:00:00.0000000Z \ No newline at end of file diff --git a/tests/testdata/projects/githubactions/curation-project/_work/_actions/some-org/transitive-action/v1/action.yml b/tests/testdata/projects/githubactions/curation-project/_work/_actions/some-org/transitive-action/v1/action.yml deleted file mode 100644 index 65888a184..000000000 --- a/tests/testdata/projects/githubactions/curation-project/_work/_actions/some-org/transitive-action/v1/action.yml +++ /dev/null @@ -1,5 +0,0 @@ -name: 'Transitive Action' -description: 'Only pulled in via codeql-action, not referenced directly in ci.yml' -runs: - using: 'node20' - main: 'index.js' diff --git a/tests/testdata/projects/githubactions/malformed-project/_work/_actions/actions/checkout/v4.completed b/tests/testdata/projects/githubactions/malformed-project/_work/_actions/actions/checkout/v4.completed deleted file mode 100644 index f39465da2..000000000 --- a/tests/testdata/projects/githubactions/malformed-project/_work/_actions/actions/checkout/v4.completed +++ /dev/null @@ -1 +0,0 @@ -1970-01-01T00:00:00.0000000Z \ No newline at end of file diff --git a/tests/testdata/projects/githubactions/malformed-project/_work/_actions/actions/checkout/v4/action.yml b/tests/testdata/projects/githubactions/malformed-project/_work/_actions/actions/checkout/v4/action.yml deleted file mode 100644 index cecdb5e46..000000000 --- a/tests/testdata/projects/githubactions/malformed-project/_work/_actions/actions/checkout/v4/action.yml +++ /dev/null @@ -1,4 +0,0 @@ -name: 'Checkout' -runs: - using: 'node20' - main: 'dist/index.js' diff --git a/tests/testdata/projects/githubactions/malformed-project/_work/_actions/actions/stray-file-at-repo-level.txt b/tests/testdata/projects/githubactions/malformed-project/_work/_actions/actions/stray-file-at-repo-level.txt deleted file mode 100644 index e69de29bb..000000000 diff --git a/tests/testdata/projects/githubactions/malformed-project/_work/_actions/onlyowner/.gitkeep b/tests/testdata/projects/githubactions/malformed-project/_work/_actions/onlyowner/.gitkeep deleted file mode 100644 index e69de29bb..000000000 diff --git a/tests/testdata/projects/githubactions/malformed-project/_work/_actions/stray-file.txt b/tests/testdata/projects/githubactions/malformed-project/_work/_actions/stray-file.txt deleted file mode 100644 index e69de29bb..000000000 diff --git a/utils/formats/markdown.go b/utils/formats/markdown.go deleted file mode 100644 index 0e5f69c79..000000000 --- a/utils/formats/markdown.go +++ /dev/null @@ -1,100 +0,0 @@ -package formats - -import ( - "fmt" - "strings" -) - -// The backslash rule comes first so an input already containing "\|" keeps both characters -// literal instead of yielding an unescaped separator. -var markdownTableCellEscaper = strings.NewReplacer( - `\`, `\\`, - `|`, `\|`, - "\r\n", "
", - "\n", "
", - "\r", "
", -) - -// EscapeMarkdownTableCell renders value safely inside one GitHub-flavored markdown table cell: -// "|" would open a new cell and a newline would end the row. -func EscapeMarkdownTableCell(value string) string { - return markdownTableCellEscaper.Replace(value) -} - -// The pipe rules are markdownTableCellEscaper's: a table printed to a log still needs its shape, -// or a cell containing "|" reads as a column break. Only the newline rule differs - "
" is -// markup a terminal shows verbatim, so a multi-line note renders as one line instead. -var terminalTableCellEscaper = strings.NewReplacer( - `\`, `\\`, - `|`, `\|`, - "\r\n", " ", - "\n", " ", - "\r", " ", -) - -// EscapeTerminalTableCell renders value inside one table cell that a person reads in a terminal -// rather than a markdown renderer. -func EscapeTerminalTableCell(value string) string { - return terminalTableCellEscaper.Replace(value) -} - -// RenderActionsException returns the line a GitHub Actions curation report carries when the -// command cannot account for every action the job will execute, or "" when nothing needs saying. -// This is the case where either a run is structure-only, or a local action was found in the -// attributed flow. -func RenderActionsException(actions []CuratedActions) string { - localUses, anyUnattributed := knownLocalCompositeActions(actions) - switch { - case len(localUses) == 0 && !anyUnattributed: - return "" - case len(localUses) == 0: - return "\nNot covered: no workflow file was available, so this report cannot tell whether this job reaches a local " + - "composite action (uses: ./...) which are not curated.\n" - } - - subject := "a local composite action" - if len(localUses) > 1 { - subject = "local composite actions" - } - described := make([]string, 0, len(localUses)) - for _, use := range localUses { - // The declarer matters most for a path a third-party composite action names: it points - // into this repository, so without it a reader has nowhere to start looking. - if use.DeclaredBy != "" { - described = append(described, fmt.Sprintf("%q declared by %s", use.Path, use.DeclaredBy)) - continue - } - described = append(described, fmt.Sprintf("%q", use.Path)) - } - message := fmt.Sprintf("\nNot covered: this job reaches %s (%s). The runner resolves those from the workspace when the step "+ - "runs - after this check read the action cache - so an action reached only that way is not curated here.", - subject, strings.Join(described, ", ")) - if anyUnattributed { - // The list is what was found, not what exists. Reporting it without this would claim a - // completeness that the part of the run with no workflow file cannot support. - message += " No workflow file was available for part of this report, so there may be others it could not see." - } - return message + "\n" -} - -// knownLocalCompositeActions returns every local composite action across actions, deduplicated -// in first-seen order, and whether any of them was produced without a workflow file. -// -// anyUnattributed cannot be inferred from the list. An empty list means "attributed, and there -// are genuinely none" or "nothing could be read", and those two have to read differently. -func knownLocalCompositeActions(actions []CuratedActions) (localUses []LocalCompositeAction, anyUnattributed bool) { - seen := map[LocalCompositeAction]bool{} - for _, curated := range actions { - if !curated.Attributed { - anyUnattributed = true - } - for _, use := range curated.LocalCompositeActions { - if seen[use] { - continue - } - seen[use] = true - localUses = append(localUses, use) - } - } - return localUses, anyUnattributed -} diff --git a/utils/formats/markdown_test.go b/utils/formats/markdown_test.go deleted file mode 100644 index d437924e0..000000000 --- a/utils/formats/markdown_test.go +++ /dev/null @@ -1,171 +0,0 @@ -package formats - -import ( - "strings" - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestEscapeMarkdownTableCell(t *testing.T) { - tests := []struct { - name string - value string - want string - }{ - {"verify when the text is ordinary then it is untouched", "actions/checkout", "actions/checkout"}, - {"verify when the value is empty then it stays empty", "", ""}, - {"verify when the value contains a pipe then the pipe is escaped", "refs|heads", `refs\|heads`}, - {"verify when the value contains several pipes then every one is escaped", "a|b|c", `a\|b\|c`}, - {"verify when the value contains a backslash then it is escaped first", `a\b`, `a\\b`}, - {"verify when the value contains an already-escaped pipe then both characters stay literal", `a\|b`, `a\\\|b`}, - {"verify when the value contains a newline then it becomes a line break", "first\nsecond", "first
second"}, - {"verify when the value contains a carriage return newline then it becomes one line break", "first\r\nsecond", "first
second"}, - {"verify when the value contains a bare carriage return then it becomes a line break", "first\rsecond", "first
second"}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, EscapeMarkdownTableCell(tt.value)) - }) - } -} - -func TestRenderActionsException(t *testing.T) { - // One renderer serves the console report and the job summary, so these strings are the whole - // statement of what an all-Approved GitHub Actions table does not cover. It is given the - // curated-actions data rather than a caller's reading of it, so every case here is expressed - // as the facts a run produced. - attributed := func(localUses ...LocalCompositeAction) CuratedActions { - return CuratedActions{Attributed: true, LocalCompositeActions: localUses} - } - unattributed := func(localUses ...LocalCompositeAction) CuratedActions { - return CuratedActions{Attributed: false, LocalCompositeActions: localUses} - } - tests := []struct { - name string - actions []CuratedActions - wantEmpty bool - wantContains []string - wantNotContains []string - }{ - { - name: "verify when there is no data then nothing is said", - actions: nil, - wantEmpty: true, - }, - { - name: "verify when attribution succeeded and no local action is declared then nothing is said", - actions: []CuratedActions{attributed()}, - wantEmpty: true, - }, - { - name: "verify when one local action is declared then it is named in the singular", - actions: []CuratedActions{attributed(LocalCompositeAction{Path: "./.github/actions/setup"})}, - // The path is the only thing a reader can act on: it is the step whose own references - // this report never saw. - wantContains: []string{"a local composite action", `"./.github/actions/setup"`, "not curated here"}, - }, - { - name: "verify when several local actions are declared then every one is named", - actions: []CuratedActions{attributed( - LocalCompositeAction{Path: "./.github/actions/setup"}, - LocalCompositeAction{Path: "./.github/actions/teardown", DeclaredBy: "some-org/wrapper@v1"}, - )}, - wantContains: []string{ - "local composite actions", - `"./.github/actions/setup"`, - `"./.github/actions/teardown" declared by some-org/wrapper@v1`, - }, - }, - { - name: "verify when a composite action declares the local step then the declaring action is named", - // The path points into the caller's repository, not into the composite action, so - // without the declarer a reader has no way to tell where the reference came from. - actions: []CuratedActions{attributed(LocalCompositeAction{Path: "./scripts/build", DeclaredBy: "some-org/wrapper@v1"})}, - wantContains: []string{ - "a local composite action", - `"./scripts/build" declared by some-org/wrapper@v1`, - }, - }, - { - name: "verify when attribution failed then the caveat is unconditional rather than a detection", - // The ordinary case on a runner: the check runs before the checkout, so there is no - // workflow file to read and no way to tell whether a local action is declared at all. - // Saying nothing here would be the one case where an all-Approved table is most - // likely to be incomplete and least likely to admit it. - actions: []CuratedActions{unattributed()}, - wantContains: []string{"no workflow file was available", "uses: ./...", "not curated"}, - }, - { - // Two runs merged into one section: one found a local step, the other had no workflow - // file. Reporting only the second discards a path that IS known, in the case with the - // most to report - which is what happens when a caller decides for this function - // whether its knowledge is complete. - name: "verify when one run found a local action and another could not look then both are stated", - actions: []CuratedActions{ - attributed(LocalCompositeAction{Path: "./.github/actions/setup"}), - unattributed(), - }, - wantContains: []string{ - `"./.github/actions/setup"`, - "not curated here", - "there may be others it could not see", - }, - }, - { - name: "verify when every run was attributed then no completeness caveat is added", - actions: []CuratedActions{ - attributed(LocalCompositeAction{Path: "./.github/actions/setup"}), - attributed(), - }, - wantContains: []string{`"./.github/actions/setup"`}, - wantNotContains: []string{"there may be others it could not see"}, - }, - { - name: "verify when two runs report the same local action then it is named once", - actions: []CuratedActions{ - attributed(LocalCompositeAction{Path: "./.github/actions/setup"}), - attributed(LocalCompositeAction{Path: "./.github/actions/setup"}), - }, - // Singular, because one path seen twice is one uncovered action. - wantContains: []string{"a local composite action"}, - wantNotContains: []string{"local composite actions"}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := RenderActionsException(tt.actions) - - if tt.wantEmpty { - assert.Equal(t, "", got) - return - } - for _, want := range tt.wantContains { - assert.Contains(t, got, want) - } - for _, notWant := range tt.wantNotContains { - assert.NotContains(t, got, notWant) - } - // Rendered right after a markdown table, so it needs its own block, and it must not - // carry markup a terminal would show verbatim. - assert.True(t, strings.HasPrefix(got, "\n"), "the caveat must start its own block, got %q", got) - assert.NotContains(t, got, "[!NOTE]") - assert.NotContains(t, got, "|") - }) - } -} - -func TestRenderActionsException_SameLocalActionFromTwoDeclarersKeepsBoth(t *testing.T) { - // The path resolves the same way either way, but a reader chasing it needs every place it is - // referenced from - so dedup is on the whole value, not on the path. - got := RenderActionsException([]CuratedActions{{ - Attributed: true, - LocalCompositeActions: []LocalCompositeAction{ - {Path: "./shared", DeclaredBy: "org/parent-a@v1"}, - {Path: "./shared", DeclaredBy: "org/parent-b@v1"}, - }, - }}) - - assert.Contains(t, got, `"./shared" declared by org/parent-a@v1`) - assert.Contains(t, got, `"./shared" declared by org/parent-b@v1`) -} diff --git a/utils/formats/summary.go b/utils/formats/summary.go index 4dcc698d6..1f38e3d4f 100644 --- a/utils/formats/summary.go +++ b/utils/formats/summary.go @@ -34,7 +34,6 @@ type ScanSummary struct { Vulnerabilities *ScanResultSummary `json:"vulnerabilities,omitempty"` Violations *ScanViolationsSummary `json:"violations,omitempty"` CuratedPackages *CuratedPackages `json:"curated,omitempty"` - CuratedActions *CuratedActions `json:"curated_actions,omitempty"` } type ScanResultSummary struct { @@ -76,33 +75,6 @@ type BlockedPackages struct { Packages map[string]int `json:"packages"` } -// CuratedActions holds the GitHub Actions curation result for one job. -type CuratedActions struct { - Actions []CuratedAction `json:"actions,omitempty"` - // Attributed reports whether a workflow file is present to do parent attribution. - // else Parent column is dropped in the report. - Attributed bool `json:"attributed"` - // LocalCompositeActions holds the `uses: ./...` steps which are evaluated lazily - LocalCompositeActions []LocalCompositeAction `json:"local_composite_actions,omitempty"` -} - -// LocalCompositeAction is one `uses: ./...` step a curated job will run, and who declared it. -type LocalCompositeAction struct { - Path string `json:"path"` - // DeclaredBy is the composite action declaring this step, as "/@", or "" - // when the job's own workflow declares it. - DeclaredBy string `json:"declared_by,omitempty"` -} - -// CuratedAction is the curation outcome for one resolved GitHub Action. -type CuratedAction struct { - Action string `json:"action"` // "owner/repo", plus " (subpath[, subpath...])" when invoked via subpaths - Ref string `json:"ref"` // verbatim from the cache directory name, uninterpreted - Parent string `json:"parent,omitempty"` // "" when directly referenced, or when attribution could not place it - Status string `json:"status"` - Notes string `json:"notes,omitempty"` -} - func (cp *CuratedPackages) GetApprovedCount() int { return cp.PackageCount - cp.GetBlockedCount() } @@ -170,10 +142,6 @@ func (sc *ScanSummary) HasBlockedPackages() bool { return sc.CuratedPackages != nil && len(sc.CuratedPackages.Blocked) > 0 } -func (sc *ScanSummary) HasCuratedActions() bool { - return sc.CuratedActions != nil && len(sc.CuratedActions.Actions) > 0 -} - func (sc *ScanSummary) HasViolations() bool { return sc.Violations != nil && sc.Violations.GetTotal() > 0 } diff --git a/utils/results/output/securityJobSummary.go b/utils/results/output/securityJobSummary.go index df8a591c7..939ed51ca 100644 --- a/utils/results/output/securityJobSummary.go +++ b/utils/results/output/securityJobSummary.go @@ -12,15 +12,9 @@ import ( "golang.org/x/exp/maps" "golang.org/x/exp/slices" - "github.com/owenrumney/go-sarif/v3/pkg/report/v210/sarif" - "github.com/jfrog/jfrog-cli-core/v2/artifactory/utils/commandsummary" "github.com/jfrog/jfrog-cli-core/v2/utils/config" "github.com/jfrog/jfrog-cli-core/v2/utils/coreutils" - "github.com/jfrog/jfrog-client-go/utils/errorutils" - "github.com/jfrog/jfrog-client-go/utils/log" - "github.com/jfrog/jfrog-client-go/xray/services" - "github.com/jfrog/jfrog-cli-security/resources" "github.com/jfrog/jfrog-cli-security/utils" "github.com/jfrog/jfrog-cli-security/utils/formats" @@ -29,6 +23,10 @@ import ( "github.com/jfrog/jfrog-cli-security/utils/results" "github.com/jfrog/jfrog-cli-security/utils/results/conversion" "github.com/jfrog/jfrog-cli-security/utils/severityutils" + "github.com/jfrog/jfrog-client-go/utils/errorutils" + "github.com/jfrog/jfrog-client-go/utils/log" + "github.com/jfrog/jfrog-client-go/xray/services" + "github.com/owenrumney/go-sarif/v3/pkg/report/v210/sarif" ) const ( @@ -154,14 +152,6 @@ func NewCurationSummary(cmdResult formats.ResultsSummary) (summary ScanCommandRe return } -// NewCurationActionsSummary wraps a GitHub Actions curation report for the job-summary -// pipeline. -func NewCurationActionsSummary(curated formats.CuratedActions) (summary ScanCommandResultSummary) { - summary.ResultType = utils.CurationActions - summary.Summary = formats.ResultsSummary{Scans: []formats.ScanSummary{{CuratedActions: &curated}}} - return -} - type ResultSummaryArgs struct { BaseJfrogUrl string `json:"base_jfrog_url,omitempty"` // Args to id the result @@ -404,25 +394,13 @@ func (js *SecurityJobSummary) GetNonScannedResult() (generator EmptyMarkdownGene return EmptyMarkdownGenerator{} } -// GenerateMarkdownFromFiles - Generate the Security section (Curation, GitHub Actions Curation) +// Generate the Security section (Curation) func (js *SecurityJobSummary) GenerateMarkdownFromFiles(dataFilePaths []string) (markdown string, err error) { curationData, _, err := loadContent(dataFilePaths, utils.Curation) if err != nil { return } - if markdown, err = GenerateSecuritySectionMarkdown(curationData); err != nil { - return - } - actionsData, _, err := loadContent(dataFilePaths, utils.CurationActions) - if err != nil { - return - } - actionsMarkdown, err := GenerateActionsCurationSectionMarkdown(actionsData) - if err != nil { - return - } - markdown += actionsMarkdown - return + return GenerateSecuritySectionMarkdown(curationData) } func GenerateSecuritySectionMarkdown(curationData []formats.ResultsSummary) (markdown string, err error) { @@ -444,58 +422,6 @@ func GenerateSecuritySectionMarkdown(curationData []formats.ResultsSummary) (mar return } -// GenerateActionsCurationSectionMarkdown renders the GitHub Actions curation report as its own -// collapsible block. The Parent column appears only when every scan was attributed. Mixed data drops it, since one -// table cannot honestly caption both. -func GenerateActionsCurationSectionMarkdown(actionsData []formats.ResultsSummary) (markdown string, err error) { - curated := collectCuratedActions(actionsData) - if len(curated) == 0 { - return - } - // Scoped to the table: it decides whether a Parent column can be captioned honestly, and - // nothing else may borrow it to mean that the report is complete. - withParent := true - for _, actions := range curated { - if !actions.Attributed { - withParent = false - break - } - } - if withParent { - markdown += "\n\n| Action | Ref | Parent | Status | Notes |\n|--------|-----|--------|--------|-------|" - } else { - markdown += "\n\n| Action | Ref | Status | Notes |\n|--------|-----|--------|-------|" - } - for _, actions := range curated { - for _, action := range actions.Actions { - cell := formats.EscapeMarkdownTableCell - if withParent { - markdown += fmt.Sprintf("\n| %s | %s | %s | %s | %s |", cell(action.Action), cell(action.Ref), cell(action.Parent), cell(action.Status), cell(action.Notes)) - continue - } - markdown += fmt.Sprintf("\n| %s | %s | %s | %s |", cell(action.Action), cell(action.Ref), cell(action.Status), cell(action.Notes)) - } - } - markdown += formats.RenderActionsException(curated) - markdown = "\n" + DetailsOpenWithSummary.Format("🔒 GitHub Actions Curation", markdown) - return -} - -// collectCuratedActions returns the curated-actions data of every scan carrying any, in the -// order the summary files were read. The facts go to the renderers as they are: what each one -// needs to conclude from them is the renderer's to decide, not this function's. -func collectCuratedActions(data []formats.ResultsSummary) (curated []formats.CuratedActions) { - for _, summary := range data { - for _, scan := range summary.Scans { - if !scan.HasCuratedActions() { - continue - } - curated = append(curated, *scan.CuratedActions) - } - } - return -} - func hasCurationCommand(data []formats.ResultsSummary) bool { for _, summary := range data { for _, scan := range summary.Scans { diff --git a/utils/results/output/securityJobSummary_actions_test.go b/utils/results/output/securityJobSummary_actions_test.go deleted file mode 100644 index 98e9edd4a..000000000 --- a/utils/results/output/securityJobSummary_actions_test.go +++ /dev/null @@ -1,286 +0,0 @@ -package output - -import ( - "encoding/json" - "os" - "path/filepath" - "strings" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - "github.com/jfrog/jfrog-cli-security/utils" - "github.com/jfrog/jfrog-cli-security/utils/formats" -) - -// writeSummaryDataFile writes a recorded ScanCommandResultSummary to a temp file, mirroring -// what commandsummary.CommandSummary.Record produces on disk, so loadContent can read it back. -func writeSummaryDataFile(t *testing.T, content ScanCommandResultSummary) string { - t.Helper() - data, err := json.Marshal(content) - assert.NoError(t, err) - filePath := filepath.Join(t.TempDir(), string(content.ResultType)+".json") - assert.NoError(t, os.WriteFile(filePath, data, 0600)) - return filePath -} - -func TestGenerateActionsCurationSectionMarkdown(t *testing.T) { - actions := func(attributed bool, entries ...formats.CuratedAction) *formats.CuratedActions { - return &formats.CuratedActions{Attributed: attributed, Actions: entries} - } - tests := []struct { - name string - data []formats.ResultsSummary - // wantEmpty covers the cases the append in GenerateMarkdownFromFiles depends on: they must - // return the empty string, not a newline and not an empty collapsible block. - wantEmpty bool - wantContains []string - wantNotContains []string - }{ - {name: "verify when there is no data then nothing is rendered", data: nil, wantEmpty: true}, - { - name: "verify when a local composite action was declared then the section carries the caveat naming it", - data: []formats.ResultsSummary{{Scans: []formats.ScanSummary{{ - CuratedActions: &formats.CuratedActions{ - Attributed: true, - Actions: []formats.CuratedAction{{Action: "actions/checkout", Ref: "v4", Status: "Approved"}}, - LocalCompositeActions: []formats.LocalCompositeAction{{Path: "./.github/actions/setup"}}, - }, - }}}}, - wantContains: []string{"Not covered", "./.github/actions/setup", "actions/checkout"}, - }, - { - // Where the conflation lived: the table drops its Parent column when ANY scan is - // unattributed, and the caveat once borrowed that same flag - discarding a local - // action one scan had definitely found. The two questions are separate, so the - // section must state both the known path and the incomplete knowledge. - name: "verify when one scan found a local action and another was not attributed then both are stated", - data: []formats.ResultsSummary{{Scans: []formats.ScanSummary{ - {CuratedActions: &formats.CuratedActions{ - Attributed: true, - Actions: []formats.CuratedAction{{Action: "actions/checkout", Ref: "v4", Status: "Approved"}}, - LocalCompositeActions: []formats.LocalCompositeAction{{Path: "./.github/actions/setup"}}, - }}, - {CuratedActions: actions(false, formats.CuratedAction{Action: "actions/cache", Ref: "v4", Status: "Approved"})}, - }}}, - wantContains: []string{ - "| Action | Ref | Status | Notes |", // the Parent column still drops, as before - "./.github/actions/setup", - "there may be others it could not see", - }, - }, - { - name: "verify when a scan was not attributed then the section carries the unconditional caveat", - data: []formats.ResultsSummary{{Scans: []formats.ScanSummary{{ - CuratedActions: actions(false, formats.CuratedAction{Action: "actions/checkout", Ref: "v4", Status: "Approved"}), - }}}}, - wantContains: []string{"Not covered", "no workflow file was available"}, - }, - { - name: "verify when attribution succeeded and no local action was declared then no caveat is rendered", - data: []formats.ResultsSummary{{Scans: []formats.ScanSummary{{ - CuratedActions: actions(true, formats.CuratedAction{Action: "actions/checkout", Ref: "v4", Status: "Approved"}), - }}}}, - wantNotContains: []string{"Not covered"}, - }, - { - // Two jobs' summary files merged into one section. The table already drops the Parent - // column on mixed data; the caveat has to name every local action across them, since - // dropping one would understate the gap in exactly the case with most to state. - name: "verify when several scans declare local actions then every one is named", - data: []formats.ResultsSummary{{Scans: []formats.ScanSummary{ - {CuratedActions: &formats.CuratedActions{ - Attributed: true, - Actions: []formats.CuratedAction{{Action: "actions/checkout", Ref: "v4", Status: "Approved"}}, - LocalCompositeActions: []formats.LocalCompositeAction{{Path: "./.github/actions/setup"}}, - }}, - {CuratedActions: &formats.CuratedActions{ - Attributed: true, - Actions: []formats.CuratedAction{{Action: "actions/cache", Ref: "v4", Status: "Approved"}}, - LocalCompositeActions: []formats.LocalCompositeAction{{Path: "./.github/actions/setup"}, {Path: "./.github/actions/teardown", DeclaredBy: "some-org/wrapper@v1"}}, - }}, - }}}, - wantContains: []string{"./.github/actions/setup", "./.github/actions/teardown"}, - }, - {name: "verify when the result set is empty then nothing is rendered", data: []formats.ResultsSummary{}, wantEmpty: true}, - { - name: "verify when only package-curation data is present then nothing is rendered", - data: []formats.ResultsSummary{{Scans: []formats.ScanSummary{ - {Target: "npm-project", CuratedPackages: &formats.CuratedPackages{PackageCount: 1}}, - }}}, - wantEmpty: true, - }, - { - name: "verify when every scan was attributed then the Parent column is rendered", - data: []formats.ResultsSummary{{Scans: []formats.ScanSummary{{ - Target: ".github/workflows/ci.yml", - CuratedActions: actions(true, - formats.CuratedAction{Action: "actions/checkout", Ref: "v4", Status: "Approved"}, - formats.CuratedAction{Action: "some-org/transitive-action", Ref: "v1", Parent: "github/codeql-action@v3", Status: "Rejected", Notes: "policy failure"}, - ), - }}}}, - wantContains: []string{ - "GitHub Actions Curation", "| Action | Ref | Parent | Status | Notes |", - "actions/checkout", "Approved", - "some-org/transitive-action", "github/codeql-action@v3", "Rejected", "policy failure", - }, - }, - { - name: "verify when no scan was attributed then the Parent column is omitted", - data: []formats.ResultsSummary{{Scans: []formats.ScanSummary{{ - Target: "/home/runner/work/_actions", - CuratedActions: actions(false, - formats.CuratedAction{Action: "actions/checkout", Ref: "v4", Status: "Approved"}, - formats.CuratedAction{Action: "some-org/some-action", Ref: "v1", Status: "Rejected", Notes: "policy failure"}, - ), - }}}}, - wantContains: []string{"GitHub Actions Curation", "| Action | Ref | Status | Notes |", "actions/checkout", "policy failure"}, - wantNotContains: []string{"Parent"}, - }, - { - // Two recorded runs, which is the shape loadContent produces - one summary per data - // file, each carrying the single scan NewCurationActionsSummary emits. One - // unattributed run is enough: a single table cannot honestly caption both. - name: "verify when attribution is mixed then the Parent column is dropped for the whole table", - data: []formats.ResultsSummary{ - {Scans: []formats.ScanSummary{{Target: "ci.yml", CuratedActions: actions(true, formats.CuratedAction{Action: "a/b", Ref: "v1", Parent: "c/d@v2", Status: "Approved"})}}}, - {Scans: []formats.ScanSummary{{Target: "_actions", CuratedActions: actions(false, formats.CuratedAction{Action: "e/f", Ref: "v3", Status: "Approved"})}}}, - }, - wantContains: []string{"| Action | Ref | Status | Notes |"}, - wantNotContains: []string{"c/d@v2"}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - markdown, err := GenerateActionsCurationSectionMarkdown(tt.data) - assert.NoError(t, err) - - if tt.wantEmpty { - assert.Equal(t, "", markdown) - return - } - for _, want := range tt.wantContains { - assert.Contains(t, markdown, want) - } - for _, notWant := range tt.wantNotContains { - assert.NotContains(t, markdown, notWant) - } - }) - } -} - -func TestNewCurationActionsSummary(t *testing.T) { - summary := NewCurationActionsSummary(formats.CuratedActions{Attributed: true, Actions: []formats.CuratedAction{{Action: "actions/checkout", Ref: "v4", Status: "Approved"}}}) - - assert.Equal(t, "curate_gh_actions", string(summary.ResultType)) - if assert.Len(t, summary.Summary.Scans, 1) { - assert.Empty(t, summary.Summary.Scans[0].Target, - "Target names a scanned path; this command curates the runner's action cache, which no renderer shows") - assert.True(t, summary.Summary.Scans[0].HasCuratedActions()) - } -} - -func TestNewCurationActionsSummary_CarriesLocalCompositeActions(t *testing.T) { - // The caveat is rendered from the summary file, so what the command knew about local - // composite actions has to survive the round trip rather than stopping at the console. - summary := NewCurationActionsSummary(formats.CuratedActions{ - Attributed: true, - Actions: []formats.CuratedAction{{Action: "actions/checkout", Ref: "v4", Status: "Approved"}}, - LocalCompositeActions: []formats.LocalCompositeAction{{Path: "./.github/actions/setup"}}, - }) - - require.Len(t, summary.Summary.Scans, 1) - assert.Equal(t, []formats.LocalCompositeAction{{Path: "./.github/actions/setup"}}, summary.Summary.Scans[0].CuratedActions.LocalCompositeActions) -} - -func TestSecurityJobSummary_GenerateMarkdownFromFiles_CombinesCurationAndActions(t *testing.T) { - curationFile := writeSummaryDataFile(t, NewCurationSummary(formats.ResultsSummary{Scans: []formats.ScanSummary{{ - Target: "npm-project", - CuratedPackages: &formats.CuratedPackages{PackageCount: 1}, - }}})) - actionsFile := writeSummaryDataFile(t, NewCurationActionsSummary(formats.CuratedActions{Attributed: true, Actions: []formats.CuratedAction{{Action: "actions/checkout", Ref: "v4", Status: "Approved"}}})) - - js := &SecurityJobSummary{} - markdown, err := js.GenerateMarkdownFromFiles([]string{curationFile, actionsFile}) - assert.NoError(t, err) - assert.Contains(t, markdown, "Curation Audit") - assert.Contains(t, markdown, "GitHub Actions Curation") - assert.True(t, strings.Index(markdown, "Curation Audit") < strings.Index(markdown, "GitHub Actions Curation")) -} - -func TestSecurityJobSummary_GenerateMarkdownFromFiles_CurationAuditOnlyIsUnchanged(t *testing.T) { - // curate-gh-actions shares the "security" command-summary manager with curation-audit and - // appends to the same markdown. A run where only curation-audit executed must therefore be - // byte-for-byte what it was before the actions section existed - no stray heading, no - // trailing newline, nothing. - curationOnly := writeSummaryDataFile(t, NewCurationSummary(formats.ResultsSummary{Scans: []formats.ScanSummary{{ - Target: "npm-project", - CuratedPackages: &formats.CuratedPackages{PackageCount: 3}, - }}})) - - js := &SecurityJobSummary{} - combined, err := js.GenerateMarkdownFromFiles([]string{curationOnly}) - assert.NoError(t, err) - - // The curation section rendered on its own, which is what the pipeline produced before. - curationData, _, err := loadContent([]string{curationOnly}, utils.Curation) - assert.NoError(t, err) - expected, err := GenerateSecuritySectionMarkdown(curationData) - assert.NoError(t, err) - - assert.Equal(t, expected, combined, "a curation-audit-only run must gain nothing from the actions section") - assert.NotContains(t, combined, "GitHub Actions Curation") -} - -func TestGenerateActionsCurationSectionMarkdown_CellsThatWouldReshapeTheTableAreEscaped(t *testing.T) { - // Same contract as RenderReportTable's console output: the job summary is rendered by - // GitHub, so an unescaped "|" or newline reshapes the table a reviewer actually reads. - data := []formats.ResultsSummary{ - {Scans: []formats.ScanSummary{{ - Target: ".github/workflows/ci.yml", - CuratedActions: &formats.CuratedActions{ - Attributed: true, - Actions: []formats.CuratedAction{ - {Action: "some-org/some-action", Ref: "feature|v2", Parent: "org/wrap|per@v1", Status: "Rejected", Notes: "blocked:\nCVE-2024-0001"}, - }, - }, - }}}, - } - - markdown, err := GenerateActionsCurationSectionMarkdown(data) - assert.NoError(t, err) - - assert.Contains(t, markdown, `feature\|v2`) - assert.Contains(t, markdown, `org/wrap\|per@v1`) - assert.Contains(t, markdown, "blocked:
CVE-2024-0001") - for _, line := range strings.Split(markdown, "\n") { - if !strings.HasPrefix(line, "| some-org/some-action") { - continue - } - assert.Equal(t, 6, strings.Count(line, "|")-strings.Count(line, `\|`), - "the data row must keep the header's cell count") - } -} - -func TestGenerateActionsCurationSectionMarkdown_LocalActionSharedBySeveralScansIsNamedOnce(t *testing.T) { - // Merged job-summary files repeat the same local action when two jobs declare it. The caveat - // is prose, so a repeated path reads as two separate gaps rather than one seen twice. - data := []formats.ResultsSummary{{Scans: []formats.ScanSummary{ - {CuratedActions: &formats.CuratedActions{ - Attributed: true, - Actions: []formats.CuratedAction{{Action: "actions/checkout", Ref: "v4", Status: "Approved"}}, - LocalCompositeActions: []formats.LocalCompositeAction{{Path: "./.github/actions/setup"}}, - }}, - {CuratedActions: &formats.CuratedActions{ - Attributed: true, - Actions: []formats.CuratedAction{{Action: "actions/cache", Ref: "v4", Status: "Approved"}}, - LocalCompositeActions: []formats.LocalCompositeAction{{Path: "./.github/actions/setup"}}, - }}, - }}} - - markdown, err := GenerateActionsCurationSectionMarkdown(data) - - require.NoError(t, err) - assert.Equal(t, 1, strings.Count(markdown, "./.github/actions/setup")) -} diff --git a/utils/utils.go b/utils/utils.go index 0bd98d267..c73547612 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -115,13 +115,12 @@ func SubScanTypesToStrings(scanTypes []SubScanType) []string { } const ( - SourceCode CommandType = "source_code" - Binary CommandType = "binary" - DockerImage CommandType = "docker_image" - Build CommandType = "build" - Curation CommandType = "curation" - CurationActions CommandType = "curate_gh_actions" - SBOM CommandType = "SBOM" + SourceCode CommandType = "source_code" + Binary CommandType = "binary" + DockerImage CommandType = "docker_image" + Build CommandType = "build" + Curation CommandType = "curation" + SBOM CommandType = "SBOM" ) type CommandType string