Add actions.lock extraction - #16
anurag6569201 wants to merge 1 commit into
Conversation
Source PR: github#22487 Source head: 7102999
⛔ Shipwright · BlockedRecommendation: do not merge PR #16 · Tier
Findings (6)
Fireworks usage: 11,723 input · 781 output · 12,504 total tokens · $0.0031 · 13s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| patterns.add("**/codeql-javascript-*.json"); | ||
|
|
||
| // exclude lock files that are not explicitly included via `LGTM_INDEX_FILTERS` | ||
| patterns.add("-**/*.lock"); |
There was a problem hiding this comment.
Shipwright · CRITICAL
The JavaScript extractor now excludes ALL '/*.lock' files via 'patterns.add("-/*.lock")', but the YAML extractor simultaneously adds '.lock' as a YAML extension.
Impact: The JavaScript extractor now excludes ALL '/*.lock' files via 'patterns.add("-/*.lock")', but the YAML extractor simultaneously adds '.lock' as a YAML extension. This creates a direct conflict: 'actions.lock' files will be excluded by the JS filter before the YAML extractor can process them, breaking the entire feature this PR claims to add. The integration test only tests the actions extractor, not the JavaScri…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| private import codeql.actions.ast.internal.Yaml | ||
|
|
||
| /** An `actions.lock` file. */ | ||
| class ActionsLock extends YamlDocument { |
There was a problem hiding this comment.
Shipwright · CRITICAL
The 'ActionsLock' class matches ANY file with basename 'actions.lock' regardless of directory or project context.
Impact: The 'ActionsLock' class matches ANY file with basename 'actions.lock' regardless of directory or project context. A malicious or accidental 'actions.lock' in a subdirectory (e.g., a vendored dependency or test fixture) will be extracted and treated as authoritative lockfile data, potentially causing false positives in security queries that rely on lockfile integrity.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| ref: '3d3c42e5aac5ba805825da76410c181273ba90b1' | ||
| commit: 'sha1-3d3c42e5aac5ba805825da76410c181273ba90b1' | ||
| owner_id: 44036562 | ||
| repo_id: 197814629 |
There was a problem hiding this comment.
Shipwright · CRITICAL
The lockfile contains 'owner_id' and 'repo_id' fields that are extracted into the CodeQL database without any validation or sanitization.
Impact: The lockfile contains 'owner_id' and 'repo_id' fields that are extracted into the CodeQL database without any validation or sanitization. If an attacker can influence the contents of 'actions.lock' (e.g., via a pull request to a repository being scanned), they can inject arbitrary YAML that gets parsed and exposed to queries, creating a potential injection vector for downstream analysis.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
|
|
||
| private import codeql.actions.ast.internal.Yaml | ||
|
|
||
| /** An `actions.lock` file. */ |
There was a problem hiding this comment.
Shipwright · HIGH
The 'ActionsLock' class extends 'YamlDocument' but provides no documentation on what fields are available, how to access 'workflows' vs 'dependencies', or what the 'version' field
Impact: The 'ActionsLock' class extends 'YamlDocument' but provides no documentation on what fields are available, how to access 'workflows' vs 'dependencies', or what the 'version' field means. A new contributor would have to reverse-engineer the YAML structure from test fixtures, which is exactly the kind of implicit knowledge this skill warns against.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| @@ -0,0 +1,4 @@ | |||
| def test_actions_lock(codeql, actions, javascript): | |||
| codeql.database.create(source_root="src", language="actions") | |||
| output = codeql.query.run("query/actions.ql", database="test-db", _capture=True) | |||
There was a problem hiding this comment.
Shipwright · HIGH
The integration test 'test_actions_lock' asserts only that the string '"actions.lock"' appears in the output, which would pass even if the query returned zero results or errored si
Impact: The integration test 'test_actions_lock' asserts only that the string '"actions.lock"' appears in the output, which would pass even if the query returned zero results or errored silently. The test does not verify that the lockfile was actually extracted or that the 'ActionsLock' class correctly identifies the file.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| 'include:**/action.yml', | ||
| 'include:**/action.yaml' | ||
| 'include:**/action.yaml', | ||
| 'include:**/actions.lock' |
There was a problem hiding this comment.
Shipwright · HIGH
The change adds 'include:**/actions.lock' to the default path filters, meaning every repository scanned by CodeQL will now extract lockfiles.
Impact: The change adds 'include:**/actions.lock' to the default path filters, meaning every repository scanned by CodeQL will now extract lockfiles. If a repository has a large number of 'actions.lock' files (e.g., in a monorepo with many workflows), this could cause unbounded database growth and performance degradation during extraction.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
1 participant
GitHub Actions databases currently skip
actions.lock, preventing queries from inspecting pinned action versions recorded in lockfiles.This change teaches the JavaScript extractor used by the Actions extractor to classify
.lockfiles as YAML. The Actions path filters limit extraction toactions.lock, while the focusedActionsLockQL class exposes those documents to queries. A normal Actions QL extraction test verifies the lockfile is available.Source merge-base:
e7181b3d8d90a44ea21584ee9d620753204b3e34Source head:
71029990432e5f3ecca846ee8bf8fe99e5c91dac