{{ message }}
Repository navigation
Block composite local paths and detect transitive ref-moved - #73
Merged
Merged
Conversation
When a remote composite action's steps contain uses: ./path, we cannot resolve transitive dependencies behind that local reference. Previously these were silently skipped (ParseActionRef returns nil for ./ prefixes), leaving a gap where unpinned transitive deps went undetected. Now the BFS in ResolveAllRecursive emits a CompositeLocalPathError for each ./path encountered in a remote composite's NestedUses. The diagnose layer detects this error type and surfaces a LocalAction finding at error severity, blocking the workflow.
Uses the nodeselector/actions-test-fixtures/composite-with-local-path fixture which contains a composite action with uses: ./local-helper. Asserts exit 1 and output mentioning local path actions and composite.
The checkRefMovedAndForgery loop only iterated pw.Refs (direct workflow uses). Transitive deps from composite actions were already in depIndex but never checked for upstream drift. Extract the per-ref logic into checkOneRefMoved and call it for both direct refs and depIndex entries not already covered by a direct ref.
There was a problem hiding this comment.
⚠️ Not ready to approve
A newly added test assertion inspects got[0] instead of the matched RefMoved finding, which can mask failures and reduce confidence in the new behavior.
Pull request overview
This PR closes two correctness gaps in gh-actions-lock’s dependency analysis: (1) remote composite actions that reference local paths (./...) are now surfaced as an error instead of being silently skipped, and (2) ref-drift (“ref moved”) checks now include transitive dependencies recorded in the lockfile, not just direct workflow uses: refs.
Changes:
- Block remote composite actions that contain
uses: ./...by returning a typedCompositeLocalPathErrorduring recursive resolution and surfacing it as aLocalActionerror finding during diagnosis. - Extend
checkRefMovedAndForgeryto also evaluate lockfile-only (transitive) dependencies, via extracted shared logic incheckOneRefMoved. - Add/extend tests and scenario coverage for both composite-local-path blocking and transitive ref-moved detection.
File summaries
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Low
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.
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.
When a remote composite action uses a local path reference (
./helper), the BFSin
ResolveAllRecursivesilently skipped it becauseParseActionRefreturns nilfor
./prefixed uses. This meant transitive deps behind those local paths wentundetected -- no error, no warning, just a quiet gap in the lockfile.
Similarly,
checkRefMovedAndForgeryonly iterated direct workflow refs (pw.Refs),so transitive deps from composite actions were never checked for upstream drift.
A tag like
actions/setup-go@v6could move to a new SHA and--rescanwouldstill report "All N workflows valid."
Approach
Composite local path blocking:
CompositeLocalPathErrorindiscovery.gothat fires when the BFSencounters a
./prefixed uses in a remote composite'sNestedUsesdiagnoseOneParsedcatches this error and emits aLocalActionfinding aterror severity, short-circuiting further checks for that workflow
composite_local_path_blocked) backed by a fixture innodeselector/actions-test-fixturesTransitive ref-moved detection:
checkRefMovedAndForgeryto extract per-ref logic intocheckOneRefMoved, then call it for both direct refs anddepIndexentriesnot already covered by a direct ref
Notes
IsValid() -> true(warning severity), so--rescanstill reports valid. Terminal rendering intentionally swallows ref-moved warnings
(line 288 of
terminal.go) until anupdatesubcommand exists. JSON output andresolution records do include them.