Security hardening: input validation and resource limits - #6
Merged
Conversation
commit:"" passes requiredActionKeys presence check but bypasses rejectZeroValues because nonEmptyStringKeys only listed "branch". An attacker-controlled lockfile could set commit:"" on every action, silently converting a required integrity field into a no-op for any consumer that reads Action.Commit without separately checking for the empty-string case. Fix: add "commit" to nonEmptyStringKeys so the same zero-value rejection that guards "branch" now guards "commit" too.
commit:"notadigest" or commit:"HEAD" passed validation because rejectZeroValues only checked for the empty string. A crafted lockfile could supply a plausible-looking but structurally invalid commit, making downstream integrity checks produce false results. Fix: after the empty-string gate, validate the commit value via isValidAlgoHex, which calls the same isValidDigest path used by ParsePin so the two never drift apart.
Parse accepted action entries where commit:"sha1-AAAA" lived under a key ending in :sha1-BBBB. A consumer checking action.Commit trusts a different hash than the pin key they used for the lookup — the lockfile's two representations of the same digest point at different commits, enabling a bait-and-switch on consumers that only check one. Fix: in canonicalizeActions, compare action.Commit against the pin key's algo+hex; return an error (locatable in the YAML tree) when they disagree.
ParsePin extracted the ref from a pin string but only checked for an embedded colon — it never validated the ref against the same denylist that ParseActionRef applies via isValidRef. A crafted pin key with a ref containing spaces, quotes, backtick, or backslash returned ok=true and handed the caller a Pin.Ref loaded with shell metacharacters. Any consumer that passes Pin.Ref to a shell command, URL builder, or GraphQL literal is directly exploitable. Fix: apply isValidRef to the extracted ref before accepting the parse; the function is the single denylist definition shared with ParseActionRef, so the two parsers cannot drift apart.
Workflow keys (e.g. ".github/workflows/ci.yml") are used by consumers as repo-relative file paths. Parse never validated them, so a crafted lockfile could include "../../../etc/passwd" or "/etc/shadow" as a key. Any consumer that calls os.Open(key) or feeds the key to filepath.Join without sanitizing gets an arbitrary-file-read primitive. Fix: validateWorkflowPaths checks every key in f.Workflows: - no leading "/" (no absolute paths) - no ".." segment (no traversal) - no control characters Errors are anchored to the offending YAML key node when available.
branch and tag values in action metadata are used in GraphQL queries, log output, and sometimes shell commands by consumers. Parse accepted any non-empty string — "main\ninjected-header: value" or "main\\evil" were both valid. An attacker-controlled lockfile could arm a downstream injection through either field. Fix: rejectZeroValues now applies isValidRef to branch and tag values when present, using the same denylist as ParseActionRef (rejects whitespace, quotes, backslash, backtick, and ".." sequences). This is the single shared denylist definition so the two parsers cannot drift apart.
Parse accepted inputs of unlimited size. A crafted multi-gigabyte YAML document — or one using YAML alias expansion — would cause memory exhaustion in yaml.Unmarshal / Decode before any validation runs. This is a trivially exploitable remote DoS for any service that feeds untrusted lockfile bytes to Parse. Fix: reject inputs larger than MaxParseSize (1 MiB) before calling yaml.Unmarshal. Legitimate lockfiles are orders of magnitude smaller than this cap. Export the constant so consumers can describe the limit in their own error messages.
Parse accepted circular uses references (A uses B, B uses A; or self-loops A uses A). The lockfile is meant to record a DAG of action dependencies. Any consumer that walks Action.Uses naively — build dependency trees, topological sort, transitive closure computation — will loop infinitely on a crafted lockfile. Fix: run a three-colour DFS (white/grey/black) over the uses graph after canonicalization. Any back-edge (grey node revisited) triggers a ParseError locatable to the offending dependency key in the YAML tree.
With a 1 MiB input cap, a single dependency entry can still pack ~20,000 entries into its uses: sequence (each ~50-byte pin string). canonicalizeActions allocates a new slice of the same length for every action — a 20× in-memory amplification on a maximum-size input. The legitimate upper bound for a real composite action's uses list is tens of entries. Fix: add MaxUsesPerAction (500) constant and rejectOverlongUses, which checks the raw YAML sequence length during validateKnownFields before any allocation occurs.
ParseActionMeta accepted unbounded string input and would collect an
unbounded NestedUses slice from a crafted composite action.yml:
- A 3.5 MB action.yml with 100,000 composite steps parsed cleanly,
allocating a 100K-element []string with no back-pressure.
- No size check ran before yaml.Unmarshal, so yaml.v3 itself had to
absorb the full document before any limit could be applied.
Fix: add two new exported constants checked at the start of the function:
MaxActionMetaSize = 64 KiB (size check before any YAML parsing)
MaxNestedUses = 500 (cap on NestedUses slice growth)
64 KiB is generous for action.yml — the largest in the wild is under
20 KiB. 500 composite steps is far beyond any real action; GitHub's
own largest composite action has fewer than 30 steps.
Tests added: OversizedInputRejected, ExactMaxSizeAccepted,
OverlongUsesListRejected, UsesListAtMaxAccepted.
Review feedback applied: - ParseActionMeta now rejects YAML anchors/aliases explicitly (action.yml doesn't use them; their presence is either a mistake or an exploit). - Removed MaxUsesPerAction (500) and MaxNestedUses (500) — the runner has no per-action step count limit, so we can't be stricter. - Updated detectUsesCycle comment to cite where the runner rejects cycles (CompositeActionsMaxDepth in src/Runner.Common/Constants.cs) and frame our check as shifting the failure left.
There was a problem hiding this comment.
⚠️ Not ready to approve
There are security/robustness gaps in the new workflow-path validation on Windows-style paths and the cycle detection implementation is recursive despite being documented as iterative, risking stack overflow on deep untrusted graphs.
Pull request overview
This PR hardens the Go lockfile parsing and action metadata parsing against untrusted inputs by adding size limits, stricter field/ref validation, workflow-key path validation, digest cross-checking, cycle detection, and YAML-anchor rejection for action.yml.
Changes:
- Add
MaxParseSize/MaxActionMetaSizelimits and reject oversized inputs early. - Tighten validation for
commit(non-empty + algo-hex format) and ref-like fields (branch/tag+ParsePinref denylist) and cross-checkcommitvs pin-key digest. - Validate workflow map keys as safe repo-relative paths and reject
uses:cycles; reject YAML anchors inParseActionMeta.
File summaries
| File | Description |
|---|---|
| go/pkg/lockfile/pin.go | Adds ref denylist validation to ParsePin. |
| go/pkg/lockfile/pin_test.go | Adds regression tests for ref injection in pin keys. |
| go/pkg/lockfile/lockfile.go | Adds parse size limit, workflow key validation, commit format validation + pin/body digest cross-check, and uses cycle detection. |
| go/pkg/lockfile/lockfile_test.go | Adds security-focused tests for the new validation and limits. |
| go/pkg/lockfile/action_meta.go | Adds action.yml size limit, YAML anchor/alias rejection, and decodes via a parsed YAML node. |
| go/pkg/lockfile/action_meta_test.go | Adds tests for anchor rejection and size limits. |
Copilot's findings
- Files reviewed: 6/6 changed files
- Comments generated: 3
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.
Comment on lines
+101
to
+105
| // Validate the ref with the same denylist used by ParseActionRef so that | ||
| // a parsed Pin is safe to pass to URL builders and GraphQL string | ||
| // literals without per-call escaping. Without this check a crafted pin | ||
| // key like "owner/repo@v1 ; malicious:sha1-..." parses successfully and | ||
| // the caller receives a Pin.Ref containing shell metacharacters. |
Comment on lines
+419
to
+431
| 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) | ||
| } | ||
| } | ||
| for _, seg := range strings.Split(p, "/") { | ||
| if seg == ".." { | ||
| return fmt.Errorf("workflow path key contains path traversal: %q", p) | ||
| } | ||
| } |
Comment on lines
+336
to
+344
| // detectUsesCycle reports a cycle in the action uses graph using | ||
| // iterative 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 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) { |
- checkWorkflowPathKey: reject backslash and colon characters to prevent Windows-style absolute paths (C:\...) and UNC paths (\server\share) from bypassing the forward-slash-only traversal checks. - detectUsesCycle: fix comment to say 'recursive DFS' (not iterative) and document why recursion depth is bounded (MaxParseSize limits dependency count to ~5,000; Go stacks grow to 1 GB). - ParsePin ref comment: remove misleading claim about escaping being unnecessary. The denylist rejects obviously-malicious refs but callers must still escape for their target context.
nodeselector
added a commit
that referenced
this pull request
Jul 8, 2026
…ening Security hardening: input validation and resource limits
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The lockfile parser accepts untrusted input -- lockfiles can be committed by any contributor or generated by automation. Without validation beyond YAML syntax, a crafted lockfile could exploit downstream consumers through injection payloads in ref/branch/tag fields, path traversal via workflow keys, memory exhaustion via oversized inputs, or trust confusion via conflicting digest values.
Approach
10 rounds of find-and-fix, each targeting a real exploitable gap confirmed with a proof-of-concept before writing the fix:
commit: ""bypassed required-field checknonEmptyStringKeyscommit:accepted any non-empty stringisValidAlgoHex()validationcommit:could disagree with pin-key digestcanonicalizeActionsParsePinref accepted shell injection charsisValidRef()check../../../etc/passwdvalidateWorkflowPaths()branch:/tag:accepted injection charsisValidRef()denylistParsehad no input size limitMaxParseSize = 1 MiBuses:references acceptedParseActionMetahad no size limitMaxActionMetaSize = 1 MiBParseActionMetaaccepted YAML anchorsrejectYAMLAnchors()on Node treeDesign decisions
CompositeActionsMaxDepthrejection left to parse time. Per-action step count limits were intentionally omitted because the runner doesn't enforce them.ParseActionMeta(action.yml files don't use anchors). The lockfile'sParsepath relies on yaml.v3's built-in alias bomb protection.isValidRef,isValidAlgoHex, andisValidDigestare reused acrossParsePin,ParseActionRef, and lockfile field validation so the denylist never drifts.Testing
Every fix includes targeted test coverage.
go test -race ./...passes after each commit.