diff --git a/go/pkg/lockfile/action_meta.go b/go/pkg/lockfile/action_meta.go index 0fda6ae..58e3075 100644 --- a/go/pkg/lockfile/action_meta.go +++ b/go/pkg/lockfile/action_meta.go @@ -26,6 +26,10 @@ type ActionMeta struct { NestedUses []string } +// MaxActionMetaSize is the maximum byte length ParseActionMeta will accept. +// action.yml files in the wild are well under 1 MiB. +const MaxActionMetaSize = 1 << 20 // 1 MiB + // ParseActionMeta parses the contents of an action.yml file into an // ActionMeta. Composite actions emit their nested step `uses:` strings // in NestedUses; non-composite actions return an empty NestedUses. @@ -33,6 +37,19 @@ type ActionMeta struct { // Returns an error only on malformed YAML — unknown `runs.using` values // resolve to ExecUnknown rather than failing. func ParseActionMeta(content string) (*ActionMeta, error) { + if len(content) > MaxActionMetaSize { + return nil, fmt.Errorf("action.yml too large: %d bytes (max %d)", len(content), MaxActionMetaSize) + } + + var doc yaml.Node + if err := yaml.Unmarshal([]byte(content), &doc); err != nil { + return nil, fmt.Errorf("parsing action.yml: %w", err) + } + + if err := rejectYAMLAnchors(&doc); err != nil { + return nil, err + } + var raw struct { Name string `yaml:"name"` Runs struct { @@ -43,7 +60,7 @@ func ParseActionMeta(content string) (*ActionMeta, error) { } `yaml:"runs"` } - if err := yaml.Unmarshal([]byte(content), &raw); err != nil { + if err := doc.Decode(&raw); err != nil { return nil, fmt.Errorf("parsing action.yml: %w", err) } @@ -54,9 +71,10 @@ func ParseActionMeta(content string) (*ActionMeta, error) { case using == "composite": meta.Execution = ExecComposite for _, step := range raw.Runs.Steps { - if step.Uses != "" { - meta.NestedUses = append(meta.NestedUses, step.Uses) + if step.Uses == "" { + continue } + meta.NestedUses = append(meta.NestedUses, step.Uses) } case using == "docker": meta.Execution = ExecDocker @@ -68,3 +86,24 @@ func ParseActionMeta(content string) (*ActionMeta, error) { return meta, nil } + +// rejectYAMLAnchors walks a yaml.Node tree and returns an error if any anchor +// definition or alias reference is found. action.yml does not use YAML anchors, +// so their presence is either a mistake or an attempted exploit. +func rejectYAMLAnchors(n *yaml.Node) error { + if n == nil { + return nil + } + if n.Kind == yaml.AliasNode { + return fmt.Errorf("action.yml: YAML anchors and aliases are not supported (line %d)", n.Line) + } + if n.Anchor != "" { + return fmt.Errorf("action.yml: YAML anchors and aliases are not supported (line %d)", n.Line) + } + for _, child := range n.Content { + if err := rejectYAMLAnchors(child); err != nil { + return err + } + } + return nil +} diff --git a/go/pkg/lockfile/action_meta_test.go b/go/pkg/lockfile/action_meta_test.go index 732bcb3..c75731a 100644 --- a/go/pkg/lockfile/action_meta_test.go +++ b/go/pkg/lockfile/action_meta_test.go @@ -3,6 +3,7 @@ package lockfile import ( "errors" "os" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -35,3 +36,35 @@ func TestParseActionMeta(t *testing.T) { }) } } + +func TestParseActionMeta_YAMLAnchorsRejected(t *testing.T) { + // Anchor definition + withAnchor := ` +name: anchored +runs: + using: composite + steps: + - &step + uses: actions/checkout@v4 + - *step +` + _, err := ParseActionMeta(withAnchor) + require.Error(t, err) + assert.Contains(t, err.Error(), "anchors and aliases are not supported") +} + +func TestParseActionMeta_OversizedInputRejected(t *testing.T) { + // Build an input just over MaxActionMetaSize. + oversized := "name: big\n" + strings.Repeat("# padding\n", (MaxActionMetaSize/10)+1) + _, err := ParseActionMeta(oversized) + require.Error(t, err) + assert.Contains(t, err.Error(), "too large") +} + +func TestParseActionMeta_ExactMaxSizeAccepted(t *testing.T) { + // A valid minimal composite action padded to exactly MaxActionMetaSize must be accepted. + base := "name: padded\nruns:\n using: composite\n steps:\n - uses: actions/checkout@v4\n" + pad := strings.Repeat("#", MaxActionMetaSize-len(base)) + _, err := ParseActionMeta(base + pad) + require.NoError(t, err) +} diff --git a/go/pkg/lockfile/lockfile.go b/go/pkg/lockfile/lockfile.go index 1380e8f..ff5a85d 100644 --- a/go/pkg/lockfile/lockfile.go +++ b/go/pkg/lockfile/lockfile.go @@ -5,6 +5,7 @@ import ( "fmt" "regexp" "strconv" + "strings" "gopkg.in/yaml.v3" ) @@ -244,6 +245,11 @@ type Action struct { // // The optional paths parameter scopes per-dependency validation to only the // entries referenced by the named workflow paths (via f.Workflows[p]). +// MaxParseSize is the maximum number of bytes Parse will accept. Inputs larger +// than this are rejected before any YAML parsing takes place to prevent +// memory-exhaustion DoS from oversized or yaml-bomb documents. +const MaxParseSize = 1 << 20 // 1 MiB + // When paths is empty, every dependency entry is validated — the default for // whole-file tooling (CLI regen, Dependabot). When paths is non-empty, a // dependency entry outside the referenced set is left unchecked so one @@ -261,6 +267,11 @@ type Action struct { // can flag them via diagnostics. Workflow path keys are NOT canonicalized // — filesystem paths are case-sensitive on the platforms we run on. func Parse(contents []byte, paths ...string) (File, error) { + if len(contents) > MaxParseSize { + return File{}, &ParseError{ + Msg: fmt.Sprintf("lockfile too large: %d bytes (max %d)", len(contents), MaxParseSize), + } + } var root yaml.Node if err := yaml.Unmarshal(contents, &root); err != nil { return File{}, newYAMLParseError(err) @@ -301,6 +312,9 @@ func Parse(contents []byte, paths ...string) (File, error) { if pe := validateKnownFields(&f, paths); pe != nil { return File{}, pe } + if pe := validateWorkflowPaths(&f); pe != nil { + return File{}, pe + } if conflictKey, err := canonicalizeActions(&f); err != nil { pe := &ParseError{Msg: err.Error(), err: err} if l, c, ok := f.KeyPosition("dependencies", conflictKey); ok { @@ -309,9 +323,126 @@ func Parse(contents []byte, paths ...string) (File, error) { return File{}, pe } canonicalizeWorkflowDependencies(&f) + if cycle, err := detectUsesCycle(&f); err != nil { + pe := &ParseError{Msg: err.Error()} + if l, c, ok := f.KeyPosition("dependencies", cycle); ok { + pe.Line, pe.Column = l, c + } + return File{}, pe + } return f, nil } +// detectUsesCycle reports a cycle in the action uses graph using +// recursive DFS with three-colour marking. It returns the key of the node +// that forms the back-edge, or ("", nil) when the graph is acyclic. +// +// The recursion depth is bounded by the number of unique keys in +// f.Dependencies, which is itself bounded by MaxParseSize (a 1 MiB file +// can hold at most ~5,000 dependency entries). Go's default goroutine stack +// grows dynamically up to 1 GB, so 5,000 frames is well within budget. +// +// The runner rejects cycles at execution time via CompositeActionsMaxDepth +// (actions/runner: src/Runner.Common/Constants.cs). Detecting them at parse +// time shifts the failure left so consumers never receive a File whose uses +// graph is not a DAG. +func detectUsesCycle(f *File) (cycleKey string, err error) { + const ( + white = 0 // unvisited + grey = 1 // on the current DFS stack + black = 2 // fully processed + ) + color := make(map[string]int, len(f.Dependencies)) + + var visit func(key string) bool + visit = func(key string) bool { + if color[key] == grey { + cycleKey = key + return true + } + if color[key] == black { + return false + } + color[key] = grey + action, ok := f.Dependencies[key] + if ok { + for _, dep := range action.Uses { + if visit(dep) { + if cycleKey == "" { + cycleKey = key + } + return true + } + } + } + color[key] = black + return false + } + + for key := range f.Dependencies { + if color[key] == white { + if visit(key) { + return cycleKey, fmt.Errorf("uses cycle detected at dependency %q", cycleKey) + } + } + } + return "", nil +} + +// validateWorkflowPaths checks that every key in f.Workflows is a safe +// repo-relative file path. Workflow keys are used by consumers as file paths +// (e.g. to open the workflow file on disk), so a crafted lockfile with a key +// like "../../../etc/passwd" or an absolute path "/etc/shadow" would give +// any consumer that calls os.Open(key) an arbitrary-read primitive. +// +// Rules: +// - Must not be empty. +// - Must not be an absolute path (no leading "/"). +// - Must not contain ".." as a path segment — rejects traversal after +// path.Clean even if embedded in a longer path. +// - Must not contain null bytes or other control characters. +func validateWorkflowPaths(f *File) *ParseError { + _, workflowsNode := mappingEntry(docMapping(f.node), "workflows") + for key := range f.Workflows { + if err := checkWorkflowPathKey(key); err != nil { + pe := &ParseError{Msg: err.Error()} + if workflowsNode != nil { + if k, _ := mappingEntry(workflowsNode, key); k != nil { + pe.Line, pe.Column = k.Line, k.Column + } + } + return pe + } + } + return nil +} + +func checkWorkflowPathKey(p string) error { + if p == "" { + return fmt.Errorf("workflow path key must not be empty") + } + if strings.HasPrefix(p, "/") { + return fmt.Errorf("workflow path key must be repo-relative, not absolute: %q", p) + } + for _, c := range p { + if c <= 0x1F || c == 0x7F { + return fmt.Errorf("workflow path key contains control characters: %q", p) + } + // Reject backslash and colon to prevent Windows-style absolute paths + // (e.g. "C:\..." or UNC "\\server\...") and backslash-based traversal + // (e.g. "..\\..\\.." ) from bypassing the forward-slash checks above. + if c == '\\' || c == ':' { + return fmt.Errorf("workflow path key contains invalid character %q: %q", string(c), p) + } + } + for _, seg := range strings.Split(p, "/") { + if seg == ".." { + return fmt.Errorf("workflow path key contains path traversal: %q", p) + } + } + return nil +} + // allowedFileKeys is the set of permitted top-level lockfile keys. It mirrors // the document-level properties declared in lockfile-v0.0.1.json. var allowedFileKeys = map[string]struct{}{ @@ -428,6 +559,7 @@ func validateKnownFields(f *File, paths []string) *ParseError { // nonEmptyStringKeys lists action fields that must be non-empty strings. var nonEmptyStringKeys = map[string]struct{}{ "branch": {}, + "commit": {}, } // positiveIntKeys lists action fields that must be positive integers (> 0). @@ -437,9 +569,12 @@ var positiveIntKeys = map[string]struct{}{ } // rejectZeroValues checks that required action fields carry meaningful values: -// string fields like "branch" must be non-empty, and integer ID fields like -// "owner_id" and "repo_id" must be positive. A present-but-zero-value field -// would silently disable the security check it's meant to enforce. +// string fields like "branch" must be non-empty, integer ID fields like +// "owner_id" and "repo_id" must be positive, the "commit" field must be +// a valid algo-hex digest string, and the "branch"/"tag" fields must not +// contain characters that are unsafe for downstream interpolation. A +// present-but-zero-value or injection-bearing field would silently disable +// the security check it's meant to enforce, or arm a downstream injection. func rejectZeroValues(action *yaml.Node, dep string) *ParseError { for j := 0; j+1 < len(action.Content); j += 2 { key := action.Content[j] @@ -455,6 +590,31 @@ func rejectZeroValues(action *yaml.Node, dep string) *ParseError { } } + if key.Value == "commit" && val.Value != "" { + if !isValidAlgoHex(val.Value) { + return &ParseError{ + Line: val.Line, + Column: val.Column, + Msg: fmt.Sprintf("action field \"commit\" must be an algo-hex digest (e.g. \"sha1-abc...\") for dependency %q, got %q", dep, val.Value), + } + } + } + + // branch and tag values are used in GraphQL queries, log output, + // and sometimes shell commands by consumers. Validate them with + // the same denylist that ParseActionRef applies to refs so that a + // crafted lockfile cannot arm a downstream injection through these + // fields. + if (key.Value == "branch" || key.Value == "tag") && val.Value != "" { + if !isValidRef(val.Value) { + return &ParseError{ + Line: val.Line, + Column: val.Column, + Msg: fmt.Sprintf("action field %q contains unsafe characters for dependency %q: %q", key.Value, dep, val.Value), + } + } + } + if _, ok := positiveIntKeys[key.Value]; ok { n, err := strconv.ParseInt(val.Value, 10, 64) if err != nil || n <= 0 { @@ -483,6 +643,18 @@ func canonicalizeActions(f *File) (string, error) { canonical := key if pin, ok := ParsePin(key); ok { canonical = pin.String() + // The commit field in the action body must match the digest + // embedded in the pin key. A mismatch is a trust-confusion + // attack: the caller looks up the action by key (and its + // embedded hash) but the body carries a different hash that + // a different downstream check might trust. + pinDigest := pin.Algo + "-" + pin.Hex + if action.Commit != "" && action.Commit != pinDigest { + return key, fmt.Errorf( + "action %q commit field %q disagrees with pin key digest %q", + canonical, action.Commit, pinDigest, + ) + } } // Canonicalize Uses entries too so cross-references resolve. if len(action.Uses) > 0 { @@ -585,3 +757,17 @@ func parseSchemaVersion(v string) ([3]int, bool) { } return out, true } + +// isValidAlgoHex reports whether s is a properly-formed algo-hex digest string +// in the lockfile's "algo-hexdigest" format (e.g. "sha1-abc123..." or +// "sha256-abc123..."). It delegates hex and length validation to isValidDigest +// so the two never drift apart. +func isValidAlgoHex(s string) bool { + dashIdx := strings.IndexByte(s, '-') + if dashIdx <= 0 || dashIdx == len(s)-1 { + return false + } + algo := strings.ToLower(s[:dashIdx]) + hex := strings.ToLower(s[dashIdx+1:]) + return isValidDigest(algo, hex) +} diff --git a/go/pkg/lockfile/lockfile_test.go b/go/pkg/lockfile/lockfile_test.go index f1185fa..01de15f 100644 --- a/go/pkg/lockfile/lockfile_test.go +++ b/go/pkg/lockfile/lockfile_test.go @@ -261,3 +261,273 @@ func mapKeys[V any](m map[string]V) []string { } return out } + +// ── Security hardening tests ────────────────────────────────────────────────── + +func TestParse_CommitEmptyStringRejected(t *testing.T) { + // commit:"" must be rejected: an empty commit SHA disables every downstream + // integrity check that reads Action.Commit, silently converting a + // required field into a no-op. + yaml := `version: v0.0.1 +dependencies: + actions/checkout@v4:sha1-11bd71901bbe5b1630ceea73d27597364c9af683: + branch: main + commit: "" + owner_id: 1 + repo_id: 1 +` + _, err := Parse([]byte(yaml)) + require.Error(t, err) + assert.Contains(t, err.Error(), `"commit"`) + assert.Contains(t, err.Error(), "must not be empty") +} + +func TestParse_CommitInvalidFormatRejected(t *testing.T) { + // A non-empty commit that isn't a valid algo-hex digest must be rejected. + // "notadigest", "sha1-", and "HEAD" look plausible but carry no integrity + // guarantee; consumers checking the algo and hex individually would silently + // accept them, defeating the lockfile's purpose. + cases := []struct { + name string + commit string + }{ + {"arbitrary string", "notadigest"}, + {"no hex after dash", "sha1-"}, + {"wrong length hex", "sha1-abc123"}, + {"symbolic ref", "HEAD"}, + {"sha1 prefix only", "sha1"}, + {"non-hex chars", "sha1-zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + y := "version: v0.0.1\ndependencies:\n" + + " actions/checkout@v4:sha1-11bd71901bbe5b1630ceea73d27597364c9af683:\n" + + " branch: main\n" + + " commit: " + tc.commit + "\n" + + " owner_id: 1\n" + + " repo_id: 1\n" + _, err := Parse([]byte(y)) + require.Error(t, err, "commit %q should be rejected", tc.commit) + assert.Contains(t, err.Error(), "commit") + }) + } +} + +func TestParse_CommitValidFormatsAccepted(t *testing.T) { + cases := []string{ + "sha1-11bd71901bbe5b1630ceea73d27597364c9af683", + "sha256-a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2", + } + for _, commit := range cases { + t.Run(commit[:10], func(t *testing.T) { + pin := "actions/checkout@v4:" + commit + y := "version: v0.0.1\ndependencies:\n " + pin + ":\n" + + " branch: main\n commit: " + commit + "\n owner_id: 1\n repo_id: 1\n" + _, err := Parse([]byte(y)) + require.NoError(t, err) + }) + } +} + +func TestParse_CommitMismatchWithPinKeyRejected(t *testing.T) { + // The commit field in the action body must match the digest embedded in + // the pin key. A mismatch is a trust-confusion attack: a consumer that + // checks action.Commit trusts a different hash than the pin key they + // used to look up the action. + yaml := `version: v0.0.1 +dependencies: + actions/checkout@v4:sha1-11bd71901bbe5b1630ceea73d27597364c9af683: + branch: main + commit: sha1-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + owner_id: 1 + repo_id: 1 +` + _, err := Parse([]byte(yaml)) + require.Error(t, err) + assert.Contains(t, err.Error(), "disagrees with pin key digest") +} + +func TestParse_CommitMatchingPinKeyAccepted(t *testing.T) { + // When commit matches the pin key digest, parse must succeed. + yaml := `version: v0.0.1 +dependencies: + actions/checkout@v4:sha1-11bd71901bbe5b1630ceea73d27597364c9af683: + branch: main + commit: sha1-11bd71901bbe5b1630ceea73d27597364c9af683 + owner_id: 1 + repo_id: 1 +` + _, err := Parse([]byte(yaml)) + require.NoError(t, err) +} + +func TestParse_WorkflowPathTraversalRejected(t *testing.T) { + // Workflow map keys are consumed as file paths by callers — accepting + // "../../../etc/passwd" or "/etc/shadow" as a key is an arbitrary read + // primitive for any consumer that calls os.Open(key). + cases := []struct { + name string + key string + }{ + {"parent traversal", "../../../etc/passwd"}, + {"embedded traversal", ".github/../../../etc/passwd"}, + {"absolute path", "/etc/shadow"}, + {"double-dot segment", ".github/workflows/../../evil.yml"}, + {"windows absolute", "C:/Windows/system.ini"}, + {"backslash traversal", "..\\\\..\\\\secret"}, + {"UNC path", "\\\\\\\\server\\\\share"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + y := "version: v0.0.1\ndependencies: {}\nworkflows:\n " + tc.key + ": []\n" + _, err := Parse([]byte(y)) + require.Error(t, err, "workflow key %q should be rejected", tc.key) + assert.Contains(t, err.Error(), "workflow path key") + }) + } +} + +func TestParse_WorkflowPathLegitimateKeysAccepted(t *testing.T) { + legit := []string{ + ".github/workflows/ci.yml", + ".github/workflows/release.yaml", + "custom/path/workflow.yml", + } + for _, key := range legit { + t.Run(key, func(t *testing.T) { + y := "version: v0.0.1\ndependencies: {}\nworkflows:\n " + key + ": []\n" + _, err := Parse([]byte(y)) + require.NoError(t, err) + }) + } +} + +func TestParse_BranchInjectionCharsRejected(t *testing.T) { + // branch values are used in GraphQL queries, log output, and sometimes + // shell commands. Characters that survive YAML parsing but are unsafe + // for downstream interpolation (backslash, `..`, whitespace) must be + // rejected by our validator. + cases := []struct { + name string + yamlBranch string // value as it appears in YAML (double-quoted to reach our validator) + }{ + // YAML double-quoted escape \\ → literal backslash in parsed value + {"backslash", `"main\\evil"`}, + // YAML double-quoted \t → literal tab + {"tab", `"main\tevil"`}, + // YAML double-quoted \n → literal newline + {"newline", `"main\nevil"`}, + // unquoted dotdot traversal + {"dotdot", "../main"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + y := "version: v0.0.1\ndependencies:\n" + + " actions/checkout@v4:sha1-11bd71901bbe5b1630ceea73d27597364c9af683:\n" + + " branch: " + tc.yamlBranch + "\n" + + " commit: sha1-11bd71901bbe5b1630ceea73d27597364c9af683\n" + + " owner_id: 1\n repo_id: 1\n" + _, err := Parse([]byte(y)) + require.Error(t, err, "branch %q should be rejected", tc.yamlBranch) + assert.Contains(t, err.Error(), "unsafe characters") + }) + } +} + +func TestParse_TagInjectionCharsRejected(t *testing.T) { + // tag is optional but when present must not carry injection characters. + y := "version: v0.0.1\ndependencies:\n" + + " actions/checkout@v4:sha1-11bd71901bbe5b1630ceea73d27597364c9af683:\n" + + " branch: main\n" + + " tag: \"../../etc/passwd; rm -rf /\"\n" + + " commit: sha1-11bd71901bbe5b1630ceea73d27597364c9af683\n" + + " owner_id: 1\n repo_id: 1\n" + _, err := Parse([]byte(y)) + require.Error(t, err) + assert.Contains(t, err.Error(), "unsafe characters") +} + +func TestParse_OversizedInputRejected(t *testing.T) { + // An input larger than MaxParseSize must be rejected before any YAML + // parsing so that memory-exhaustion DoS from oversized documents is + // prevented at the library boundary. + oversized := make([]byte, MaxParseSize+1) + for i := range oversized { + oversized[i] = 'x' + } + _, err := Parse(oversized) + require.Error(t, err) + assert.Contains(t, err.Error(), "too large") +} + +func TestParse_ExactMaxSizeAccepted(t *testing.T) { + // A document at exactly MaxParseSize must not be size-rejected + // (it will fail for other reasons, but not the size check). + atMax := make([]byte, MaxParseSize) + for i := range atMax { + atMax[i] = 'x' + } + _, err := Parse(atMax) + require.Error(t, err) + assert.NotContains(t, err.Error(), "too large") +} + +func TestParse_UsesCycleRejected(t *testing.T) { + yaml := `version: v0.0.1 +dependencies: + actions/a@v1:sha1-11bd71901bbe5b1630ceea73d27597364c9af683: + branch: main + commit: sha1-11bd71901bbe5b1630ceea73d27597364c9af683 + owner_id: 1 + repo_id: 1 + uses: + - actions/b@v1:sha1-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + actions/b@v1:sha1-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: + branch: main + commit: sha1-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + owner_id: 2 + repo_id: 2 + uses: + - actions/a@v1:sha1-11bd71901bbe5b1630ceea73d27597364c9af683 +` + _, err := Parse([]byte(yaml)) + require.Error(t, err) + assert.Contains(t, err.Error(), "cycle") +} + +func TestParse_UsesSelfCycleRejected(t *testing.T) { + yaml := `version: v0.0.1 +dependencies: + actions/a@v1:sha1-11bd71901bbe5b1630ceea73d27597364c9af683: + branch: main + commit: sha1-11bd71901bbe5b1630ceea73d27597364c9af683 + owner_id: 1 + repo_id: 1 + uses: + - actions/a@v1:sha1-11bd71901bbe5b1630ceea73d27597364c9af683 +` + _, err := Parse([]byte(yaml)) + require.Error(t, err) + assert.Contains(t, err.Error(), "cycle") +} + +func TestParse_UsesAcyclicAccepted(t *testing.T) { + // A valid DAG (A uses B, B has no uses) must parse successfully. + yaml := `version: v0.0.1 +dependencies: + actions/a@v1:sha1-11bd71901bbe5b1630ceea73d27597364c9af683: + branch: main + commit: sha1-11bd71901bbe5b1630ceea73d27597364c9af683 + owner_id: 1 + repo_id: 1 + uses: + - actions/b@v1:sha1-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + actions/b@v1:sha1-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: + branch: main + commit: sha1-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + owner_id: 2 + repo_id: 2 +` + _, err := Parse([]byte(yaml)) + require.NoError(t, err) +} diff --git a/go/pkg/lockfile/pin.go b/go/pkg/lockfile/pin.go index a2ec105..0b2a73a 100644 --- a/go/pkg/lockfile/pin.go +++ b/go/pkg/lockfile/pin.go @@ -98,6 +98,14 @@ func ParsePin(s string) (Pin, bool) { if strings.ContainsRune(ref, ':') { return Pin{}, false } + // Validate the ref with the same denylist used by ParseActionRef to + // reject shell metacharacters, whitespace, and traversal sequences. + // This does NOT make the ref safe for verbatim interpolation into URLs + // or shell commands -- callers must still escape appropriately for their + // context. The goal is to reject obviously-malicious refs at parse time. + if !isValidRef(ref) { + return Pin{}, false + } hashSpec := refHash[colonIdx+1:] dashIdx := strings.IndexByte(hashSpec, '-') diff --git a/go/pkg/lockfile/pin_test.go b/go/pkg/lockfile/pin_test.go index d120441..be51016 100644 --- a/go/pkg/lockfile/pin_test.go +++ b/go/pkg/lockfile/pin_test.go @@ -128,3 +128,28 @@ func TestPin_IndexKey(t *testing.T) { }) } } + +func TestParsePin_RefInjectionRejected(t *testing.T) { + // ParsePin must reject refs containing characters that are unsafe for + // URL paths and GraphQL string literals (spaces, quotes, backticks, + // backslash). Without this check a caller receives a Pin.Ref that + // injects into downstream shell commands or API calls. + cases := []struct { + name string + input string + }{ + {"space in ref", "actions/checkout@v4 evil:sha1-11bd71901bbe5b1630ceea73d27597364c9af683"}, + {"tab in ref", "actions/checkout@v4\tevil:sha1-11bd71901bbe5b1630ceea73d27597364c9af683"}, + {"double-quote in ref", "actions/checkout@v4\"evil:sha1-11bd71901bbe5b1630ceea73d27597364c9af683"}, + {"single-quote in ref", "actions/checkout@v4'evil:sha1-11bd71901bbe5b1630ceea73d27597364c9af683"}, + {"backtick in ref", "actions/checkout@v4`evil:sha1-11bd71901bbe5b1630ceea73d27597364c9af683"}, + {"backslash in ref", "actions/checkout@v4\\evil:sha1-11bd71901bbe5b1630ceea73d27597364c9af683"}, + {"dotdot in ref", "actions/checkout@../evil:sha1-11bd71901bbe5b1630ceea73d27597364c9af683"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, ok := ParsePin(tc.input) + assert.False(t, ok, "ParsePin should reject ref with injection chars: %q", tc.input) + }) + } +}