diff --git a/cli/docs/flags.go b/cli/docs/flags.go index 502f6cd66..3024c59f9 100644 --- a/cli/docs/flags.go +++ b/cli/docs/flags.go @@ -22,6 +22,7 @@ const ( DockerScan = "docker scan" Audit = "audit" CurationAudit = "curation-audit" + CurationActions = "curate-gh-actions" GitAudit = "git-audit" GitCountContributors = "count-contributors" Enrich = "sbom-enrich" @@ -169,6 +170,12 @@ const ( MvnIncludePluginDeps = "mvn-include-plugin-deps" Script = "script" + // Unique curate-gh-actions flags + ActionsCacheDir = "actions-cache-dir" + WorkflowFile = "workflow-file" + WorkflowJob = "workflow-job" + GithubRepo = "github-repo" + // Unique git flags gitPrefix = "git-" InputFile = "input-file" @@ -234,6 +241,9 @@ var commandFlags = map[string][]string{ CurationAudit: { CurationOutput, WorkingDirs, Threads, RequirementsFile, InsecureTls, useWrapperAudit, UseIncludedBuilds, SolutionPath, DockerImageName, HuggingFaceModel, IncludeCachedPackages, MvnIncludePluginDeps, LegacyPeerDeps, RunNative, Script, }, + CurationActions: { + ActionsCacheDir, WorkflowFile, WorkflowJob, GithubRepo, + }, GitCountContributors: { InputFile, ScmType, ScmApiUrl, Token, Owner, RepoName, Months, DetailedSummary, InsecureTls, GitThreads, CacheValidity, }, @@ -375,6 +385,11 @@ var flagsMap = map[string]components.Flag{ UseConfigProfile: components.NewBoolFlag(UseConfigProfile, "Set to false to override config profile for the audit.", components.WithBoolDefaultValue(true), components.SetHiddenBoolFlag()), Workspace: components.NewStringFlag(Workspace, "Optional workspace name for repositories with multiple config profiles. Used with the repository URL to fetch the matching config profile from the JFrog Platform."), + ActionsCacheDir: components.NewStringFlag(ActionsCacheDir, "Overrides the runner's GitHub Actions cache directory (defaults to the _actions directory derived from RUNNER_WORKSPACE). Mainly useful for local runs outside an actual GitHub Actions runner."), + WorkflowFile: components.NewStringFlag(WorkflowFile, "Overrides the workflow YAML file to curate (defaults to the running workflow, derived from GITHUB_WORKFLOW_REF). Must be an absolute path."), + WorkflowJob: components.NewStringFlag(WorkflowJob, "Overrides the workflow job to curate, as its job_id key under 'jobs:' (defaults to the running job, from GITHUB_JOB)."), + GithubRepo: components.NewStringFlag(GithubRepo, "The GitHub repository whose curation policies apply, as '/' (defaults to the running repository, from GITHUB_REPOSITORY)."), + // Docker flags DockerImageName: components.NewStringFlag(DockerImageName, "Specifies the Docker image name to audit. Uses the same format as the Docker CLI, including Artifactory-hosted images."), HuggingFaceModel: components.NewStringFlag(HuggingFaceModel, "Hugging Face model(s) to audit, in '[:revision]' format. Multiple models can be comma-separated."), diff --git a/cli/docs/scan/curationactions/help.go b/cli/docs/scan/curationactions/help.go new file mode 100644 index 000000000..0029148f4 --- /dev/null +++ b/cli/docs/scan/curationactions/help.go @@ -0,0 +1,58 @@ +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, or --github-repo). The mapping is curation-side configuration. If it cannot be resolved the command fails. + +Prerequisites: +- Must run on a GitHub Actions runner (or point --actions-cache-dir at a directory shaped like the runner's _actions cache for local testing: //, with a '.completed' file beside each action, since that marker is what identifies one). Outside a runner, GITHUB_REPOSITORY is unset, so pass --github-repo. + +Common patterns: + $ jf curate-gh-actions + $ jf curate-gh-actions --actions-cache-dir=/path/to/_actions + $ jf curate-gh-actions --workflow-file=/path/to/repo/.github/workflows/ci.yml + $ jf curate-gh-actions --workflow-file=/path/to/repo/.github/workflows/ci.yml --workflow-job=build + $ jf curate-gh-actions --github-repo=my-org/my-repo + +Gotchas: +- jfrog/setup-jfrog-cli is excluded from the report at every version. It installs this CLI and invokes the check, so curating it would let the check fail a job on the tool performing it rather than on a third-party action. +- If the action cache directory cannot be located at all - not running under GitHub Actions, so RUNNER_WORKSPACE is unset, and no --actions-cache-dir given - the command reports an error. A directory that is located but absent is different: it reads as an empty cache, so the command reports nothing to curate and succeeds. Check the path if you passed --actions-cache-dir and expected entries. +- 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. +- 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. +- 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. 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 is not an error, whether it was named with --workflow-file or derived from GITHUB_WORKFLOW_REF. GitHub's YAML reader accepts input this one rejects (duplicate mapping keys, for instance), and the runner has already accepted the file, so the run degrades to curating the cache alone rather than failing. +- --workflow-file is an assertion that the file exists and is readable: if it cannot be read, the command fails rather than falling back, since a path you named and this command cannot open is a mistake worth surfacing. Pass it when you have fetched the workflow YAML over the API; omit it to curate the cache alone. It must be an absolute path - a relative one is rejected rather than resolved against the working directory. + +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: How do I run it outside a GitHub Actions runner, where GITHUB_REPOSITORY is unset? +A: jf curate-gh-actions --github-repo=my-org/my-repo --actions-cache-dir=/path/to/_actions + +Q: How do I run GitHub Actions curation against a specific workflow file? +A: jf curate-gh-actions --workflow-file=/path/to/repo/.github/workflows/ci.yml + +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: 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 6469663f2..5ddfd3f79 100644 --- a/cli/scancommands.go +++ b/cli/scancommands.go @@ -30,6 +30,7 @@ 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" @@ -131,6 +132,17 @@ 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(), @@ -646,6 +658,24 @@ 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 { + curationActionsCommand := curation.NewCurationActionsCommand() + if c.IsFlagSet(flags.ActionsCacheDir) { + curationActionsCommand.SetActionsCacheDir(c.GetStringFlagValue(flags.ActionsCacheDir)) + } + if c.IsFlagSet(flags.WorkflowFile) { + curationActionsCommand.SetWorkflowFile(c.GetStringFlagValue(flags.WorkflowFile)) + } + if c.IsFlagSet(flags.WorkflowJob) { + curationActionsCommand.SetJobID(c.GetStringFlagValue(flags.WorkflowJob)) + } + if c.IsFlagSet(flags.GithubRepo) { + curationActionsCommand.SetGithubRepo(c.GetStringFlagValue(flags.GithubRepo)) + } + return curationActionsCommand.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 new file mode 100644 index 000000000..efd63eea3 --- /dev/null +++ b/commands/curation/curationactions.go @@ -0,0 +1,285 @@ +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/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" +) + +const ( + flagGithubRepo = "github-repo" + flagWorkflowFile = "workflow-file" +) + +// 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. The delivery action (jfrog/setup-jfrog-cli) is always excluded. +// +// 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 { + log.Info("No GitHub Actions found in the runner's action cache - nothing to curate.") + return nil + } + + used, attributed, err := c.parseWorkflowUses(workingDir) + if err != nil { + return err + } + if attributed { + discovered = githubactions.CrossReference(discovered, used) + } + discovered = githubactions.ExcludeDeliveryAction(discovered) + if len(discovered) == 0 { + log.Info("The runner's action cache holds only the action delivering this check - nothing to curate.") + return nil + } + + // Resolved once, after the early returns 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 + } + + log.Info(fmt.Sprintf("GitHub Actions Curation Report:\n%s", githubactions.RenderMarkdownTable(rows, attributed))) + + // 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(rows, attributed); 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. --workflow-file (+ --workflow-job) - explicit, so a file that cannot be read is an error: +// the caller asserted it exists. It must be absolute: the caller named one specific file, so +// there is deliberately nothing for it to be resolved 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. Absent from disk falls back to structure-only. +// 3. neither - structure-only. +// +// Whether the file can be read is the caller's assertion to get wrong, so it is fatal for an +// explicit path. What the file turns out to contain is not: a workflow this parser cannot parse, +// or one not declaring the job, costs attribution and nothing else, whichever way the path was +// resolved. The cache is still the complete account of what will execute, and every entry in it +// is decided either way - so the run degrades to structure-only rather than failing. That also +// matches parseCompositeActionUses, which takes the same view of an action.yml it cannot read. +func (c *CurationActionsCommand) parseWorkflowUses(workingDir string) (used []githubactions.WorkflowUse, attributed bool, err error) { + jobID := c.jobID + if jobID == "" { + jobID = githubactions.DefaultJobID() + } + // Whether the caller named the file matters below: an explicit path is an assertion that + // it exists, a derived one is not. + workflowFile, explicit := c.workflowFile, c.workflowFile != "" + if explicit { + if !filepath.IsAbs(workflowFile) { + return nil, false, fmt.Errorf("--%s must be an absolute path, got %q", flagWorkflowFile, 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 nil, false, nil + } + workflowFile = filepath.Join(workingDir, workflowFile) + } + used, err = githubactions.ParseWorkflowUses(workflowFile, jobID) + if err == nil { + return used, true, nil + } + if 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)) + return nil, false, nil + } + if 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)) + return nil, false, nil + } + if explicit || !errors.Is(err, os.ErrNotExist) { + return nil, false, err + } + log.Warn(fmt.Sprintf("Workflow file %q (from %s) 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; pass --workflow-file to attribute against a copy fetched over the API.", + workflowFile, githubactions.WorkflowRefEnvVar)) + return nil, 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; the --github-repo override exists for local and test invocations. +func (c *CurationActionsCommand) resolveArtifactoryVcsRepo(ctx context.Context) (string, error) { + githubRepo := c.githubRepo + if githubRepo == "" { + githubRepo = githubactions.DefaultGithubRepo() + } + if githubRepo == "" { + return "", fmt.Errorf("cannot determine which GitHub repository this job belongs to: "+ + "neither --%s nor %s is set", flagGithubRepo, 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 +} + +// recordSummary records the report through the "security" job-summary manager +func (c *CurationActionsCommand) recordSummary(rows []githubactions.ActionReportRow, attributed bool) error { + 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)) + } + return output.RecordSecurityCommandSummary(output.NewCurationActionsSummary(actions, attributed)) +} diff --git a/commands/curation/curationactions_test.go b/commands/curation/curationactions_test.go new file mode 100644 index 000000000..d2b64b71c --- /dev/null +++ b/commands/curation/curationactions_test.go @@ -0,0 +1,603 @@ +package curation + +import ( + "context" + "errors" + "os" + "path/filepath" + "slices" + "testing" + + "github.com/jfrog/jfrog-cli-core/v2/utils/coreutils" + "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 --workflow-file 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 - --workflow-file 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 +} + +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)) + require.NoError(t, os.WriteFile(filepath.Join(workflowsDir, "ci.yml"), []byte(s.workflowYAML), 0600)) + } + return workingDir, actionsCacheDir +} + +// newCommand builds the command every Run test exercises, wiring --workflow-file 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 --workflow-file 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 delivery action then it is excluded from curation", + 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"}, + }, + { + // 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, + }, + { + name: "verify when the cache is empty then the command succeeds", + spec: runnerSpec{}, + mode: fixtureWorkflowFile, + }, + { + // 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 --workflow-file 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 the command fails", + spec: runnerSpec{cacheDirs: oneAction}, + mode: missingWorkflowFile, + wantErrContains: "reading workflow file", + }, + { + // --workflow-file 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 --github-repo is passed 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 both ways to supply one", + envRepo: "", // GITHUB_REPOSITORY unset, e.g. a local run + wantErrContains: []string{"--github-repo", 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_EmptyCacheMakesNoResolutionCall(t *testing.T) { + pinRunnerEnv(t, testGithubRepo, "", "") + resolver := &fixedResolver{repo: "unused"} + + require.NoError(t, runnerSpec{}.newCommand(t, noWorkflowFile, "", &scriptedDecider{}). + SetVcsRepoResolver(resolver).Run()) + + assert.Empty(t, resolver.askedAbout, "a job with nothing to curate must not call the mapping API") +} + +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 pulls in a remote one then it is still decided", + // uses: ./... is read from the workspace, so there is nothing to walk outward from. + 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") + }) + } +} diff --git a/commands/curation/githubactions/decision.go b/commands/curation/githubactions/decision.go new file mode 100644 index 000000000..193fe7733 --- /dev/null +++ b/commands/curation/githubactions/decision.go @@ -0,0 +1,37 @@ +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 new file mode 100644 index 000000000..603df6bcf --- /dev/null +++ b/commands/curation/githubactions/decision_mock.go @@ -0,0 +1,31 @@ +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 new file mode 100644 index 000000000..802fbc060 --- /dev/null +++ b/commands/curation/githubactions/decision_mock_test.go @@ -0,0 +1,54 @@ +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 new file mode 100644 index 000000000..b4ba234ae --- /dev/null +++ b/commands/curation/githubactions/discovery.go @@ -0,0 +1,370 @@ +package githubactions + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" + + "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" +) + +// 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 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 scan, nil + } + return ActionCacheScan{}, fmt.Errorf("reading actions cache dir %q: %w", actionsCacheDir, 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: %q holds %d entries but none resolved to an // action, "+ + "so nothing will be curated. The debug log names every entry that was skipped.", actionsCacheDir, 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 "", fmt.Errorf("%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) +} + +const ( + deliveryActionOwner = "jfrog" + deliveryActionRepo = "setup-jfrog-cli" +) + +// ExcludeDeliveryAction drops jfrog/setup-jfrog-cli from refs, at any ref, so it is neither +// decided nor reported. Owner and repo are matched case-insensitively. +func ExcludeDeliveryAction(refs []ActionRef) []ActionRef { + kept := make([]ActionRef, 0, len(refs)) + for _, ref := range refs { + if strings.EqualFold(ref.Owner, deliveryActionOwner) && strings.EqualFold(ref.Repo, deliveryActionRepo) { + log.Debug(fmt.Sprintf("github-actions curation: skipping %s/%s@%s - it delivers and invokes this check rather than being subject to it", ref.Owner, ref.Repo, ref.Ref)) + continue + } + kept = append(kept, ref) + } + return kept +} diff --git a/commands/curation/githubactions/discovery_test.go b/commands/curation/githubactions/discovery_test.go new file mode 100644 index 000000000..5a47eae73 --- /dev/null +++ b/commands/curation/githubactions/discovery_test.go @@ -0,0 +1,591 @@ +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. +// --actions-cache-dir does not widen any of this: it is a local-testing override pointing at a +// directory shaped the same way. +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"}, + }, + { + name: "verify when the cache directory does not exist then the result is empty and no error", + cache: actionCache{fixture: "does-not-exist"}, + }, + { + // 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()) +} + +func TestExcludeDeliveryAction(t *testing.T) { + tests := []struct { + name string + refs []ActionRef + wantRepos []string + }{ + { + name: "verify when the delivery action is present at any ref then it is dropped", + refs: []ActionRef{ + {Owner: "jfrog", Repo: "setup-jfrog-cli", Ref: "v4"}, + {Owner: "jfrog", Repo: "setup-jfrog-cli", Ref: "9a4c2881"}, + {Owner: "actions", Repo: "checkout", Ref: "v4"}, + }, + wantRepos: []string{"checkout"}, + }, + { + // GitHub resolves owner and repo case-insensitively and the runner names the cache + // directory from the uses: line verbatim, so this spelling really reaches disk. + name: "verify when the delivery action is spelled with different casing then it is still dropped", + refs: []ActionRef{ + {Owner: "JFrog", Repo: "setup-jfrog-cli", Ref: "v4"}, + {Owner: "JFROG", Repo: "Setup-JFrog-CLI", Ref: "v4"}, + {Owner: "actions", Repo: "checkout", Ref: "v4"}, + }, + wantRepos: []string{"checkout"}, + }, + { + name: "verify when another jfrog action is present then it is kept", + refs: []ActionRef{ + {Owner: "jfrog", Repo: "frogbot", Ref: "v2"}, + {Owner: "jfrog", Repo: "setup-jfrog-cli", Ref: "v4"}, + }, + wantRepos: []string{"frogbot"}, + }, + { + name: "verify when another owner ships a same-named action then it is kept", + refs: []ActionRef{ + {Owner: "not-jfrog", Repo: "setup-jfrog-cli", Ref: "v4"}, + }, + wantRepos: []string{"setup-jfrog-cli"}, + }, + { + name: "verify when only the delivery action is present then nothing is left to curate", + refs: []ActionRef{{Owner: "jfrog", Repo: "setup-jfrog-cli", Ref: "v4"}}, + wantRepos: nil, + }, + { + name: "verify when there are no refs then the result stays empty", + refs: nil, + wantRepos: nil, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + kept := ExcludeDeliveryAction(tt.refs) + repos := make([]string, len(kept)) + for i, ref := range kept { + repos[i] = ref.Repo + } + if tt.wantRepos == nil { + assert.Empty(t, repos) + return + } + assert.Equal(t, tt.wantRepos, repos) + }) + } +} + +func TestExcludeDeliveryAction_PreservesTransitiveAttribution(t *testing.T) { + // Excluding the delivery action must not orphan anything it pulled in: attribution runs + // before this filter, so a child keeps its Parent even though that parent is not reported. + kept := ExcludeDeliveryAction([]ActionRef{ + {Owner: "jfrog", Repo: "setup-jfrog-cli", Ref: "v4"}, + {Owner: "some-org", Repo: "pulled-in-by-delivery", Ref: "v1", Parent: "jfrog/setup-jfrog-cli@v4"}, + }) + + if assert.Len(t, kept, 1) { + assert.Equal(t, "pulled-in-by-delivery", kept[0].Repo) + assert.Equal(t, "jfrog/setup-jfrog-cli@v4", kept[0].Parent, "attribution must survive the exclusion") + } +} diff --git a/commands/curation/githubactions/reporesolver.go b/commands/curation/githubactions/reporesolver.go new file mode 100644 index 000000000..d8d02f28d --- /dev/null +++ b/commands/curation/githubactions/reporesolver.go @@ -0,0 +1,12 @@ +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). + Resolve(ctx context.Context, githubRepo string) (string, error) +} diff --git a/commands/curation/githubactions/reporesolver_mock.go b/commands/curation/githubactions/reporesolver_mock.go new file mode 100644 index 000000000..16cd4472f --- /dev/null +++ b/commands/curation/githubactions/reporesolver_mock.go @@ -0,0 +1,27 @@ +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 new file mode 100644 index 000000000..fede8b289 --- /dev/null +++ b/commands/curation/githubactions/reporesolver_mock_test.go @@ -0,0 +1,38 @@ +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 new file mode 100644 index 000000000..4ff0dc837 --- /dev/null +++ b/commands/curation/githubactions/report.go @@ -0,0 +1,82 @@ +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, + } +} + +// RenderMarkdownTable renders rows as a GitHub-flavored markdown table. withParent controls +// whether the Parent column appears at all. +func RenderMarkdownTable(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.EscapeMarkdownTableCell(row.Action)) + sb.WriteString(" | ") + sb.WriteString(formats.EscapeMarkdownTableCell(row.Ref)) + if withParent { + sb.WriteString(" | ") + sb.WriteString(formats.EscapeMarkdownTableCell(row.Parent)) + } + sb.WriteString(" | ") + sb.WriteString(formats.EscapeMarkdownTableCell(row.Status)) + sb.WriteString(" | ") + sb.WriteString(formats.EscapeMarkdownTableCell(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 new file mode 100644 index 000000000..232becaf2 --- /dev/null +++ b/commands/curation/githubactions/report_test.go @@ -0,0 +1,158 @@ +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 TestRenderMarkdownTable(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 := RenderMarkdownTable(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 TestRenderMarkdownTable_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 := RenderMarkdownTable(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`) + assert.Contains(t, got, "blocked:
CVE-2024-0001") + 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 new file mode 100644 index 000000000..d220d8f71 --- /dev/null +++ b/commands/curation/githubactions/workflow.go @@ -0,0 +1,290 @@ +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 == "" || strings.HasPrefix(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 +} + +// 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. +// Local actions (uses: ./path) and Docker-URI actions (uses: docker://...) are skipped. +// +// 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. +func ParseWorkflowUses(workflowPath, jobID string) ([]WorkflowUse, error) { + data, err := os.ReadFile(workflowPath) + if err != nil { + return nil, fmt.Errorf("reading workflow file %q: %w", workflowPath, err) + } + var wf rawWorkflow + if err = yaml.Unmarshal(data, &wf); err != nil { + return nil, fmt.Errorf("%w %q: %w", ErrWorkflowUnparsable, workflowPath, err) + } + if jobID == "" { + return nil, fmt.Errorf("%w: no job id given for %q", ErrJobUnknown, workflowPath) + } + job, declared := wf.Jobs[jobID] + if !declared { + return nil, 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. + var uses []WorkflowUse + for _, step := range job.Steps { + if parsed, ok := parseUsesString(step.Uses); ok { + uses = append(uses, parsed) + } + } + return uses, nil +} + +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 every owner/repo/ref its own steps reference - one hop outward from +// actionPath. CrossReference calls this repeatedly, once per action per round, to walk +// arbitrarily many hops; this function itself only ever looks at the one action.yml it's given. +// +// There is no error return because there is no failure: absent metadata, metadata this parser +// cannot read, and a non-composite action all mean the same thing here - nothing to attribute +// from - and none of them may fail the run. The result is always "the references found", which +// is legitimately none. +func parseCompositeActionUses(actionPath string) []WorkflowUse { + 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 { + // The runner already accepted this file, so a parse failure here is a divergence + // between its YAML reader and ours, not a broken action. Nothing is attributed from + // it - the entries it pulled in stay unattributed and are still curated - but the + // reason has to be greppable, or the missing Parent column looks like a design choice. + log.Debug(fmt.Sprintf("github-actions curation: cannot parse %q - no transitive references attributed from it: %v", filepath.Join(actionPath, name), err)) + return nil + } + if af.Runs.Using != "composite" { + return nil + } + var uses []WorkflowUse + for _, step := range af.Runs.Steps { + if parsed, ok := parseUsesString(step.Uses); ok { + uses = append(uses, parsed) + } + } + return uses + } + return nil +} + +// CrossReference enriches discovered entries with Subpaths and best-effort Parent metadata, +// and returns the enriched slice. +// +// Attribution is purely additive: it only ever adds metadata, never removes an entry. One it +// cannot place keeps an empty Parent and is still curated - an action the runner resolved will +// execute whether or not this code can explain why it is there. +// +// A directly-used entry takes its Subpaths from the job's own uses: lines. Every other entry is +// attributed by walking outward one level at a time, reading the action.yml of each composite +// action resolved at the current depth: a step referencing an unresolved entry makes that +// entry's Parent the composite action's "/@" (first parent wins - see +// hasParent below), and that entry a source for the next level. +// +// 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. +// +// The walk has no fixed depth limit - it stops when the frontier runs dry. What guarantees that +// is markLocation: a (key, location) pair is scanned at most once, so every round must consume a +// pair not seen before, and the pairs are finite. A cycle terminates for that same reason rather +// than by being detected. Note the pair count is not len(discovered) - one key contributes a pair +// per location it is referenced through - so maxRounds below is a backstop, not the real bound. +// +// 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. Unattributed is not unreported; those entries are still curated. +func CrossReference(discovered []ActionRef, used []WorkflowUse) []ActionRef { + 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 { + 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) + } + } + + // A second, independent bound: a regression in the dedup above would hit a hard stop rather + // than spin. It is not a limit on legitimate nesting depth. + maxRounds := len(discovered) + 1 + for depth := 0; depth < maxRounds && len(frontier) > 0; depth++ { + 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) + } + for _, cu := range parseCompositeActionUses(metadataDir) { + 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 +} + +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 new file mode 100644 index 000000000..4a6476d6e --- /dev/null +++ b/commands/curation/githubactions/workflow_test.go @@ -0,0 +1,557 @@ +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) + + sort.Slice(uses, func(i, j int) bool { return uses[i].Owner+uses[i].Repo < uses[j].Owner+uses[j].Repo }) + + if assert.Len(t, uses, 2) { + assert.Equal(t, WorkflowUse{Owner: "actions", Repo: "checkout", Ref: "v4", Raw: "actions/checkout@v4"}, uses[0]) + assert.Equal(t, WorkflowUse{Owner: "github", Repo: "codeql-action", Subpath: "analyze", Ref: "v3", Raw: "github/codeql-action/analyze@v3"}, uses[1]) + } +} + +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), 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)) + + assert.Empty(t, parseCompositeActionUses(dir), 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, "no uses: may be returned from jobs that ran on other runners") + 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 := []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 := []WorkflowUse{{Owner: "org", Repo: "action1", Ref: "v1"}} + + done := make(chan []ActionRef, 1) + go func() { done <- CrossReference(discovered, used) }() + 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}, + }, + []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, []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, []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, []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)) + }) + } +} + +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 +} diff --git a/tests/testdata/projects/githubactions/curation-project/.github/workflows/ci.yml b/tests/testdata/projects/githubactions/curation-project/.github/workflows/ci.yml new file mode 100644 index 000000000..2b4c4a08a --- /dev/null +++ b/tests/testdata/projects/githubactions/curation-project/.github/workflows/ci.yml @@ -0,0 +1,10 @@ +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 new file mode 100644 index 000000000..f39465da2 --- /dev/null +++ b/tests/testdata/projects/githubactions/curation-project/_work/_actions/actions/checkout/v4.completed @@ -0,0 +1 @@ +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 new file mode 100644 index 000000000..5f210f36d --- /dev/null +++ b/tests/testdata/projects/githubactions/curation-project/_work/_actions/actions/checkout/v4/action.yml @@ -0,0 +1,5 @@ +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 new file mode 100644 index 000000000..f39465da2 --- /dev/null +++ b/tests/testdata/projects/githubactions/curation-project/_work/_actions/github/codeql-action/v3.completed @@ -0,0 +1 @@ +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 new file mode 100644 index 000000000..c13e3e027 --- /dev/null +++ b/tests/testdata/projects/githubactions/curation-project/_work/_actions/github/codeql-action/v3/action.yml @@ -0,0 +1,5 @@ +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 new file mode 100644 index 000000000..4a2cef836 --- /dev/null +++ b/tests/testdata/projects/githubactions/curation-project/_work/_actions/github/codeql-action/v3/analyze/action.yml @@ -0,0 +1,8 @@ +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 new file mode 100644 index 000000000..f39465da2 --- /dev/null +++ b/tests/testdata/projects/githubactions/curation-project/_work/_actions/some-org/transitive-action/v1.completed @@ -0,0 +1 @@ +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 new file mode 100644 index 000000000..65888a184 --- /dev/null +++ b/tests/testdata/projects/githubactions/curation-project/_work/_actions/some-org/transitive-action/v1/action.yml @@ -0,0 +1,5 @@ +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 new file mode 100644 index 000000000..f39465da2 --- /dev/null +++ b/tests/testdata/projects/githubactions/malformed-project/_work/_actions/actions/checkout/v4.completed @@ -0,0 +1 @@ +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 new file mode 100644 index 000000000..cecdb5e46 --- /dev/null +++ b/tests/testdata/projects/githubactions/malformed-project/_work/_actions/actions/checkout/v4/action.yml @@ -0,0 +1,4 @@ +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 new file mode 100644 index 000000000..e69de29bb diff --git a/tests/testdata/projects/githubactions/malformed-project/_work/_actions/onlyowner/.gitkeep b/tests/testdata/projects/githubactions/malformed-project/_work/_actions/onlyowner/.gitkeep new file mode 100644 index 000000000..e69de29bb 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 new file mode 100644 index 000000000..e69de29bb diff --git a/utils/formats/markdown.go b/utils/formats/markdown.go new file mode 100644 index 000000000..67c58d7f5 --- /dev/null +++ b/utils/formats/markdown.go @@ -0,0 +1,19 @@ +package formats + +import "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) +} diff --git a/utils/formats/markdown_test.go b/utils/formats/markdown_test.go new file mode 100644 index 000000000..8f1b783f2 --- /dev/null +++ b/utils/formats/markdown_test.go @@ -0,0 +1,30 @@ +package formats + +import ( + "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)) + }) + } +} diff --git a/utils/formats/summary.go b/utils/formats/summary.go index 1f38e3d4f..012946aec 100644 --- a/utils/formats/summary.go +++ b/utils/formats/summary.go @@ -34,6 +34,7 @@ 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 { @@ -75,6 +76,23 @@ 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"` +} + +// 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() } @@ -142,6 +160,10 @@ 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 939ed51ca..7e02dac05 100644 --- a/utils/results/output/securityJobSummary.go +++ b/utils/results/output/securityJobSummary.go @@ -12,9 +12,15 @@ 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" @@ -23,10 +29,6 @@ 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 ( @@ -152,6 +154,16 @@ func NewCurationSummary(cmdResult formats.ResultsSummary) (summary ScanCommandRe return } +// NewCurationActionsSummary wraps a GitHub Actions curation report for the job-summary +// pipeline. +func NewCurationActionsSummary(actions []formats.CuratedAction, attributed bool) (summary ScanCommandResultSummary) { + summary.ResultType = utils.CurationActions + summary.Summary = formats.ResultsSummary{Scans: []formats.ScanSummary{{ + CuratedActions: &formats.CuratedActions{Actions: actions, Attributed: attributed}, + }}} + return +} + type ResultSummaryArgs struct { BaseJfrogUrl string `json:"base_jfrog_url,omitempty"` // Args to id the result @@ -394,13 +406,25 @@ func (js *SecurityJobSummary) GetNonScannedResult() (generator EmptyMarkdownGene return EmptyMarkdownGenerator{} } -// Generate the Security section (Curation) +// GenerateMarkdownFromFiles - Generate the Security section (Curation, GitHub Actions Curation) func (js *SecurityJobSummary) GenerateMarkdownFromFiles(dataFilePaths []string) (markdown string, err error) { curationData, _, err := loadContent(dataFilePaths, utils.Curation) if err != nil { return } - return GenerateSecuritySectionMarkdown(curationData) + 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 } func GenerateSecuritySectionMarkdown(curationData []formats.ResultsSummary) (markdown string, err error) { @@ -422,6 +446,62 @@ 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) { + if !hasCurationActionsCommand(actionsData) { + return + } + withParent := allCuratedActionsAttributed(actionsData) + if withParent { + markdown += "\n\n| Action | Ref | Parent | Status | Notes |\n|--------|-----|--------|--------|-------|" + } else { + markdown += "\n\n| Action | Ref | Status | Notes |\n|--------|-----|--------|-------|" + } + for i := range actionsData { + for _, summary := range actionsData[i].Scans { + if !summary.HasCuratedActions() { + continue + } + for _, action := range summary.CuratedActions.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 = "\n" + DetailsOpenWithSummary.Format("🔒 GitHub Actions Curation", markdown) + return +} + +// allCuratedActionsAttributed reports whether every scan carrying curated actions was +// attributed against a workflow file. +func allCuratedActionsAttributed(data []formats.ResultsSummary) bool { + for _, summary := range data { + for _, scan := range summary.Scans { + if scan.HasCuratedActions() && !scan.CuratedActions.Attributed { + return false + } + } + } + return true +} + +func hasCurationActionsCommand(data []formats.ResultsSummary) bool { + for _, summary := range data { + for _, scan := range summary.Scans { + if scan.HasCuratedActions() { + return true + } + } + } + return false +} + 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 new file mode 100644 index 000000000..fc0365199 --- /dev/null +++ b/utils/results/output/securityJobSummary_actions_test.go @@ -0,0 +1,185 @@ +package output + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/jfrog/jfrog-cli-security/utils" + "github.com/jfrog/jfrog-cli-security/utils/formats" + "github.com/stretchr/testify/assert" +) + +// 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 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.CuratedAction{{Action: "actions/checkout", Ref: "v4", Status: "Approved"}}, true) + + 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 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.CuratedAction{{Action: "actions/checkout", Ref: "v4", Status: "Approved"}}, true)) + + 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 RenderMarkdownTable'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") + } +} diff --git a/utils/utils.go b/utils/utils.go index 16657e0f0..8a549a5e6 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -114,12 +114,13 @@ func SubScanTypesToStrings(scanTypes []SubScanType) []string { } const ( - SourceCode CommandType = "source_code" - Binary CommandType = "binary" - DockerImage CommandType = "docker_image" - Build CommandType = "build" - Curation CommandType = "curation" - SBOM CommandType = "SBOM" + 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" ) type CommandType string