Strict field validation and scoped per-workflow Parse - #3
Conversation
Parse now enforces that required action fields carry meaningful values, not just that the keys are present: - branch must be a non-empty string - owner_id and repo_id must be positive integers (> 0) A present-but-zero-value field would silently disable the security check it is meant to enforce (e.g. owner_id: 0 bypasses owner identity verification). The JSON schema is tightened with minLength and minimum constraints to match. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
⚠️ Not ready to approve
Scoped validation currently matches dependencies using raw (pre-canonicalization) pin strings, which can incorrectly skip validation for referenced dependencies when casing differs between workflows entries and dependencies keys.
Pull request overview
Tightens the GitHub Actions lockfile parser/schema to reject meaningless zero values in required action metadata and adds a scoped-validation mode so consumers can validate only dependencies referenced by selected workflows (reducing blast radius in shared lockfiles).
Changes:
- Tighten JSON schema constraints (
branchminLength,owner_id/repo_idminimum) and align Go validation to reject empty/zero/negative required fields. - Add
Parse(contents, paths... )to scope per-dependency validation to dependencies referenced by specified workflows, while keeping document-level invariants global. - Add tests covering both zero-value rejection and scoped validation behavior.
File summaries
| File | Description |
|---|---|
| schema/lockfile-v0.0.1.json | Adds minLength/minimum constraints to prevent empty/zero values in required fields. |
| go/pkg/lockfile/schema_test.go | Adds unit tests for zero-value rejection and per-workflow scoped validation matrix. |
| go/pkg/lockfile/schema_gen.go | Regenerates embedded schema constant to match updated root JSON schema. |
| go/pkg/lockfile/lockfile.go | Implements scoped validation and new zero-value enforcement in parser. |
Copilot's findings
Files not reviewed (1)
- go/pkg/lockfile/schema_gen.go: Generated file
- Files reviewed: 3/4 changed files
- Comments generated: 2
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add variadic paths parameter to Parse(contents, paths...) so callers can limit per-dependency checks (unknown keys, required keys, zero-value rejection) to only the entries referenced by the named workflows. When paths is empty, every dependency is validated — back-compat for whole-file tooling (CLI regen, Dependabot, existing callers compile unchanged). When paths is non-empty, entries outside the referenced set are skipped. A path absent from f.Workflows contributes zero entries and validates nothing (fail-open for non-onboarded workflows). Document-level invariants (version, unknown top-level keys) always run regardless of paths. This fixes the blast-radius bug where one corrupt dependency entry in a shared lockfile fails every workflow that loads it, including workflows that don't reference the corrupt entry at all.
eeb3453 to
8930c62
Compare
Strict field validation and scoped per-workflow Parse
What
Two changes that tighten lockfile validation and fix a blast-radius bug:
1. Zero-value rejection —
Parsenow enforces that required action fields carry meaningful values, not just that the keys are present:branchmust be a non-empty stringowner_idandrepo_idmust be positive integers (> 0)A present-but-zero-value field silently disables the security check it meant to enforce (e.g.
owner_id: 0bypasses owner identity verification). The JSON schema is tightened withminLengthandminimumconstraints to match.2. Scoped per-workflow validation —
Parsegains a variadicpaths ...stringparameter. Per-dependency checks (unknown keys, required keys, zero-value rejection) are scoped to the union off.Workflows[p]for the requested paths:paths(zero args) — validate every dependency entry. Back-compatible for CLI regen, Dependabot, and any whole-file tooling. Existing callers compile unchanged.paths— only validate entries referenced by the named workflows. Entries outside that set are skipped.f.Workflowscontributes zero entries and validates nothing — fail-open by design for non-onboarded workflows.Document-level invariants (version required/supported, unknown top-level keys) always run regardless of
paths.canonicalizeActionsis left global — a conflicting canonical pin with different metadata is a structural file defect, not a per-workflow concern.Why
In co-located shared lockfiles,
Parsevalidates the entiredependencies:map up front and returns on the first failure. Downstream consumers (actions-workflow-parser, launch) callParseonce per workflow being compiled, then look up just that workflow's pins. One corrupt dependency entry — even one no active workflow references — fails every workflow that loads the lockfile. A single bad entry takes down an entire repo's CI instead of just the workflow that actually consumes it.Downstream
actions-workflow-parser will change its
lockfile.Parse(contents)call tolockfile.Parse(contents, workflowKey)to scope validation per-workflow. That PR is up and validated against a dev pseudo-version of this commit; after this merges, the parser re-pins to a release, then launch re-pins to the parser.Tests
17 new tests across both commits: