-
Notifications
You must be signed in to change notification settings - Fork 11
fix(providers): analyze all FROM lines in multi-stage Dockerfiles via batch analysis #577
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
a-oren
wants to merge
1
commit into
guacsec:main
Choose a base branch
from
a-oren:TC-5070
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ import fs from 'node:fs' | |
|
|
||
| import { generateImageSBOM, parseImageRef } from '../oci_image/utils.js' | ||
|
|
||
| import { getFromQuery, getParser } from './containerfile_parser.js' | ||
| import { getArgQuery, getFromQuery, getParser } from './containerfile_parser.js' | ||
|
|
||
| export default { isSupported, validateLockFile, provideComponent, provideStack, readLicenseFromManifest, packageManagerName() { return 'oci' } } | ||
|
|
||
|
|
@@ -51,43 +51,103 @@ function containsExpansion(node) { | |
| } | ||
|
|
||
| /** | ||
| * Parse the last FROM instruction from a Dockerfile using tree-sitter to extract the base image reference. | ||
| * In multi-stage builds, the last FROM represents the final stage. | ||
| * Collect ARG key-value pairs from the Dockerfile AST. | ||
| * Only ARGs with a default value are collected (ARGs without defaults cannot be resolved statically). | ||
| * @param {import('web-tree-sitter').Tree} tree the parsed Dockerfile tree | ||
| * @param {import('web-tree-sitter').Query} argQuery the tree-sitter query for arg_instruction nodes | ||
| * @returns {Map<string, string>} map of ARG names to their default values | ||
| * @private | ||
| */ | ||
| function collectArgs(tree, argQuery) { | ||
| const args = new Map() | ||
| for (const match of argQuery.matches(tree.rootNode)) { | ||
| const name = match.captures.find(c => c.name === 'name')?.node.text | ||
| const defaultValue = match.captures.find(c => c.name === 'default')?.node.text | ||
| if (name && defaultValue) { | ||
| args.set(name, defaultValue) | ||
| } | ||
| } | ||
| return args | ||
| } | ||
|
|
||
| /** | ||
| * Resolve ARG substitutions in an image spec text using the collected ARG map. | ||
| * Replaces ${VAR} and $VAR patterns with their ARG default values. | ||
| * @param {string} text the image spec text potentially containing variable references | ||
| * @param {Map<string, string>} args map of ARG names to their default values | ||
| * @returns {string|null} the resolved text, or null if any variable could not be resolved | ||
| * @private | ||
| */ | ||
| function resolveArgs(text, args) { | ||
| let resolved = text | ||
| let hasUnresolved = false | ||
| resolved = resolved.replace(/\$\{([^}]+)\}|\$([A-Za-z_]\w*)/g, (_match, braced, plain) => { | ||
| const key = braced || plain | ||
| if (args.has(key)) { | ||
| return args.get(key) | ||
| } | ||
| hasUnresolved = true | ||
| return _match | ||
| }) | ||
| return hasUnresolved ? null : resolved | ||
| } | ||
|
|
||
| /** | ||
| * Parse all FROM instructions from a Dockerfile using tree-sitter to extract base image references. | ||
| * In multi-stage builds, every FROM line is returned so each image can be analyzed independently. | ||
| * ARG substitutions are resolved using declared default values. FROM lines with unresolvable | ||
| * variables (ARGs without defaults) are silently skipped. | ||
| * @param {string} manifestContent the content of the Dockerfile | ||
| * @returns {Promise<string>} the image reference from the last FROM instruction | ||
| * @throws {Error} when no FROM instruction is found or when ARG substitution is used | ||
| * @returns {Promise<string[]>} array of image references from all resolvable FROM instructions | ||
| * @throws {Error} when no FROM instruction is found or when no FROM lines can be resolved | ||
| */ | ||
| export async function parseFromImage(manifestContent) { | ||
| const [parser, fromQuery] = await Promise.all([getParser(), getFromQuery()]) | ||
| export async function parseAllFromImages(manifestContent) { | ||
| const [parser, fromQuery, argQuery] = await Promise.all([getParser(), getFromQuery(), getArgQuery()]) | ||
| const tree = parser.parse(manifestContent) | ||
| const matches = fromQuery.matches(tree.rootNode) | ||
| if (matches.length === 0) { | ||
| throw new Error('No FROM line found in Dockerfile') | ||
| } | ||
| const lastMatch = matches[matches.length - 1] | ||
| const imageSpec = lastMatch.captures.find(c => c.name === 'image').node | ||
| if (containsExpansion(imageSpec)) { | ||
| throw new Error('Dockerfile uses ARG substitution in FROM line — cannot resolve variable references') | ||
| const args = collectArgs(tree, argQuery) | ||
| const images = [] | ||
| for (const match of matches) { | ||
| const imageSpec = match.captures.find(c => c.name === 'image').node | ||
| if (containsExpansion(imageSpec)) { | ||
| const resolved = resolveArgs(imageSpec.text, args) | ||
| if (resolved != null) { | ||
| images.push(resolved) | ||
| } | ||
| continue | ||
| } | ||
| images.push(imageSpec.text) | ||
| } | ||
| if (images.length === 0) { | ||
| throw new Error('Dockerfile uses ARG substitution in all FROM lines — cannot resolve variable references') | ||
| } | ||
| return imageSpec.text | ||
| return images | ||
| } | ||
|
|
||
| /** | ||
| * Generate an image SBOM from a Dockerfile manifest using syft. | ||
| * Generate image SBOMs for all FROM instructions in a Dockerfile manifest using syft. | ||
| * Returns a batch result with a purl-to-SBOM map suitable for the batch-analysis endpoint. | ||
| * @param {string} manifest path to the Dockerfile | ||
| * @param {{}} [opts={}] optional various options to pass along the application | ||
| * @returns {Promise<{ecosystem: string, content: string, contentType: string}>} | ||
| * @returns {Promise<{ecosystem: string, content: string, contentType: string, batch: boolean}>} | ||
| * @private | ||
| */ | ||
| async function getImageSBOM(manifest, opts = {}) { | ||
| const manifestContent = fs.readFileSync(manifest, 'utf-8') | ||
| const image = await parseFromImage(manifestContent) | ||
| const imageRef = parseImageRef(image, opts) | ||
| const sbom = generateImageSBOM(imageRef, opts) | ||
| const images = await parseAllFromImages(manifestContent) | ||
| const sbomByPurl = {} | ||
| for (const image of images) { | ||
| const imageRef = parseImageRef(image, opts) | ||
| sbomByPurl[imageRef.getPackageURL().toString()] = generateImageSBOM(imageRef, opts) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a containerfile lists the same image multiple times, this is rerun unnecessarily |
||
| } | ||
| return { | ||
| ecosystem, | ||
| content: JSON.stringify(sbom), | ||
| contentType: 'application/vnd.cyclonedx+json' | ||
| content: JSON.stringify(sbomByPurl), | ||
| contentType: 'application/vnd.cyclonedx+json', | ||
| batch: true | ||
| } | ||
| } | ||
|
|
||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): ARG query only captures unquoted defaults, missing valid quoted ARG defaults.
By restricting both
nameanddefaulttounquoted_string, this misses valid cases likeARG FOO="bar"(or other representations tree-sitter may use). Those ARGs will be invisible tocollectArgs, causing FROM lines that depend on them to be treated as unresolvable and skipped. Please broaden the matched node types fordefault(and possiblyname) so quoted and alternative forms are included.Suggested implementation:
If downstream code assumes
@nameand@defaultare alwaysunquoted_stringnodes, you may need to adjust it to handle other node types (e.g., string literals) by inspecting the captured nodes and normalizing their text (stripping quotes where needed).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[sdlc-workflow/verify-pr] Classified as suggestion — the current query correctly captures only ARGs with unquoted defaults, which is sufficient for static resolution. Broadening to wildcard node types would also require downstream normalization (quote stripping). No sub-task created.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although Ive never seen quoted ones in the wild(I was thinking of something else), we should ideally handle them. Removing the quotes will be necessaryThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, handling them would make this more robust, and the extra normalization is pretty contained.
Concretely, I’d suggest two small tweaks:
collectArgsso we strip surrounding quotes from defaults (and optionally names, though they’re usually bare identifiers):That keeps the rest of the pipeline unchanged while covering
ARG FOO="bar",ARG FOO='bar', and the existing unquoted cases.If you’d like, I can also suggest a follow-up test case for a Dockerfile with quoted ARG defaults to ensure this path is exercised.