fix(dockerfile): analyze all FROM lines via batch analysis#546
Open
a-oren wants to merge 5 commits into
Open
Conversation
…ysis Add DockerfileProvider that parses FROM instructions to extract base image references and generates CycloneDX SBOMs via syft. Supports multi-stage builds (uses final FROM), suffixed filenames (Dockerfile.dev), multiple --flag tokens, and rejects ARG substitution and FROM scratch. Also normalize Docker Hub image references in ImageRef.getPackageURL() so bare names (node) and library-prefixed names (docker.io/library/node) produce the same PURL (docker.io/node), aligning with the JS client. Implements: TC-4938 Assisted-by: Claude Code Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Aligns with the JavaScript client by allowing users to set TRUSTIFY_DA_RECOMMEND=false to append ?recommend=false to analysis URLs, disabling Trusted Content recommendations in responses. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Change the DockerfileProvider from analyzing only the last FROM line to
analyzing all FROM lines in multi-stage Dockerfiles. Each image gets its
own SBOM via syft, and results are sent as a batch request to
/api/v5/batch-analysis.
- Add batch flag to Provider.Content to route between single and batch endpoints
- Resolve ARG substitutions (both ${VAR} and $VAR syntax) with default values
- Skip FROM scratch and unresolvable ARG references
- Route batch content through ExhortApi to the batch-analysis endpoint
Fixes: TC-5071
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Reviewer's GuideAdds a DockerfileProvider that parses all FROM lines for multi-stage Dockerfiles and produces batch SBOM content, wires Dockerfile manifests into the Ecosystem provider/ExhortApi batch-analysis flow (including recommend flag handling), and normalizes Docker Hub image references in ImageRef PURLs, with comprehensive tests for Dockerfile parsing, batch behavior, and URI construction. Sequence diagram for Dockerfile DockerfileProvider batch analysis flowsequenceDiagram
participant User
participant ExhortApi
participant Ecosystem
participant DockerfileProvider
participant Backend as ExhortBackend
User->>ExhortApi: componentAnalysis(manifestFile)
ExhortApi->>Ecosystem: getProvider(manifestPath)
Ecosystem-->>ExhortApi: DockerfileProvider
ExhortApi->>DockerfileProvider: provideComponent()
DockerfileProvider->>DockerfileProvider: generateBatchSbomContent()
DockerfileProvider-->>ExhortApi: Content(batch=true)
ExhortApi->>ExhortApi: resolveAnalysisUri(content)
ExhortApi->>ExhortApi: buildAnalysisUri(S_API_V_5_BATCH_ANALYSIS, endpoint)
ExhortApi->>Backend: sendAsync(request)
Backend-->>ExhortApi: batch response
ExhortApi->>ExhortApi: getBatchStackAnalysisReports(response)
ExhortApi-->>User: AnalysisReport (first from batch)
Sequence diagram for resolveAnalysisUri and recommend flag handlingsequenceDiagram
participant ExhortApi
participant Environment
participant Backend as ExhortBackend
ExhortApi->>ExhortApi: resolveAnalysisUri(content)
alt content.batch == true
ExhortApi->>ExhortApi: buildAnalysisUri(S_API_V_5_BATCH_ANALYSIS, endpoint)
else content.batch == false
ExhortApi->>ExhortApi: buildAnalysisUri(S_API_V_5_ANALYSIS, endpoint)
end
ExhortApi->>Environment: getBoolean(TRUSTIFY_DA_RECOMMEND, true)
alt TRUSTIFY_DA_RECOMMEND == false
ExhortApi->>ExhortApi: append ?recommend=false
else TRUSTIFY_DA_RECOMMEND == true
ExhortApi->>ExhortApi: use base URI
end
ExhortApi->>Backend: sendAsync(request with URI)
Backend-->>ExhortApi: response
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #546 +/- ##
=======================================
Coverage ? 69.30%
Complexity ? 1035
=======================================
Files ? 66
Lines ? 4365
Branches ? 770
=======================================
Hits ? 3025
Misses ? 994
Partials ? 346
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In DockerfileProvider.parseAllFromImages, the special-case handling for
scratchis case-sensitive; consider normalizing the image token (e.g., to lower-case) before comparing so thatFROM SCRATCHorFrom Scratchare also skipped consistently. - buildAnalysisUri currently appends
?recommend=falsedirectly to the template URL; if these endpoints ever gain their own query parameters this will produce invalid URIs, so it may be safer to build the URI via a dedicated builder that can merge query parameters correctly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In DockerfileProvider.parseAllFromImages, the special-case handling for `scratch` is case-sensitive; consider normalizing the image token (e.g., to lower-case) before comparing so that `FROM SCRATCH` or `From Scratch` are also skipped consistently.
- buildAnalysisUri currently appends `?recommend=false` directly to the template URL; if these endpoints ever gain their own query parameters this will produce invalid URIs, so it may be safer to build the URI via a dedicated builder that can merge query parameters correctly.
## Individual Comments
### Comment 1
<location path="src/main/java/io/github/guacsec/trustifyda/providers/DockerfileProvider.java" line_range="140" />
<code_context>
+ continue;
+ }
+ }
+ if ("scratch".equals(image)) {
+ LOG.info(String.format("Skipping FROM scratch in %s", dockerfile));
+ continue;
</code_context>
<issue_to_address>
**issue (bug_risk):** Handle `scratch` base image in a case-insensitive way.
`FROM` is already handled case-insensitively, but this comparison to `"scratch"` is not. As a result, `FROM Scratch` or `FROM SCRATCH` would still be analyzed. Please make this comparison case-insensitive (e.g., `equalsIgnoreCase("scratch")` or by normalizing `image` to lowercase) so all variants are skipped consistently.
</issue_to_address>
### Comment 2
<location path="src/main/java/io/github/guacsec/trustifyda/image/ImageRef.java" line_range="155" />
<code_context>
+ // node → docker.io/node
+ // docker.io/library/node → docker.io/node
+ if (repositoryUrl != null) {
+ var lower = repositoryUrl.toLowerCase();
+ if (lower.equals(simpleName.toLowerCase())) {
+ repositoryUrl = "docker.io/" + simpleName;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Use a locale-independent lowercase operation when normalizing repository URLs.
`toLowerCase()` without a locale can behave differently on systems with non-English defaults (e.g., Turkish), causing incorrect normalization. Please use `toLowerCase(Locale.ROOT)` for both `repositoryUrl` and `simpleName` to ensure consistent behavior across environments.
Suggested implementation:
```java
if (repositoryUrl != null) {
var lower = repositoryUrl.toLowerCase(java.util.Locale.ROOT);
if (lower.equals(simpleName.toLowerCase(java.util.Locale.ROOT))) {
repositoryUrl = "docker.io/" + simpleName;
} else if (lower.startsWith(DOCKER_HUB_LIBRARY_PREFIX)) {
repositoryUrl = "docker.io/" + lower.substring(DOCKER_HUB_LIBRARY_PREFIX.length());
}
}
if (repositoryUrl != null && !repositoryUrl.equalsIgnoreCase(simpleName)) {
qualifiers.put(REPOSITORY_QUALIFIER, repositoryUrl.toLowerCase(java.util.Locale.ROOT));
}
```
If you prefer using a direct import instead of qualifying `Locale`, add:
- `import java.util.Locale;`
and then replace `java.util.Locale.ROOT` with `Locale.ROOT` in the edited code.
</issue_to_address>
### Comment 3
<location path="src/test/java/io/github/guacsec/trustifyda/providers/Dockerfile_Provider_Test.java" line_range="326-288" />
<code_context>
+ /** Verifies that batch content skips images that fail SBOM generation and includes the rest. */
</code_context>
<issue_to_address>
**suggestion (testing):** Cover the case where all images fail SBOM generation to assert the IOException from generateBatchSbomContent
`provide_stack_skips_failing_images_in_batch` already covers mixed success/failure, but there’s no coverage for the branch where *all* SBOM generations fail and `generateBatchSbomContent` throws.
Please add a test that:
- Uses a multi-stage Dockerfile (e.g., `multi_stage/Dockerfile`).
- Mocks `ImageUtils.generateImageSBOM(..)` to throw for every image.
- Asserts `provider.provideStack()` (and optionally `provider.provideComponent()`) throws an `IOException` whose message contains `"No analyzable FROM images found"`.
This will exercise the error path when SBOM generation fails for all images and protect against regressions.
Suggested implementation:
```java
/** Verifies that batch content skips images that fail SBOM generation and includes the rest. */
@Test
void provide_stack_skips_failing_images_in_batch() throws Exception {
// Given a multi-stage Dockerfile where one image fails SBOM generation
var manifestPath = TEST_MANIFESTS.resolve("multi_stage/Dockerfile");
var provider = new DockerfileProvider(manifestPath);
ImageRef nodeRef = Mockito.mock(ImageRef.class);
JsonNode nginxSbom = MAPPER.createObjectNode().put("bomFormat", "CycloneDX");
ImageRef nginxRef = Mockito.mock(ImageRef.class);
PackageURL nginxPurl = new PackageURL("pkg:oci/nginx@alpine");
}
/**
* Verifies that when all images fail SBOM generation, provideStack throws an IOException
* with a helpful message instead of returning an empty batch.
*/
@Test
void provide_stack_throws_when_all_images_fail_sbom_generation() throws Exception {
// Given a multi-stage Dockerfile where every FROM image fails SBOM generation
var manifestPath = TEST_MANIFESTS.resolve("multi_stage/Dockerfile");
var provider = new DockerfileProvider(manifestPath);
try (MockedStatic<ImageUtils> imageUtilsMock = Mockito.mockStatic(ImageUtils.class)) {
imageUtilsMock
.when(() -> ImageUtils.generateImageSBOM(Mockito.any(ImageRef.class)))
.thenThrow(new IOException("SBOM generation failed"));
assertThatThrownBy(provider::provideStack)
.isInstanceOf(IOException.class)
.hasMessageContaining("No analyzable FROM images found");
// Optionally also cover provideComponent to ensure it propagates the same error
assertThatThrownBy(provider::provideComponent)
.isInstanceOf(IOException.class)
.hasMessageContaining("No analyzable FROM images found");
}
```
I could only see part of the existing `provide_stack_skips_failing_images_in_batch` test body, so in the replacement block above I preserved the visible portion verbatim and closed the method before adding the new test.
To integrate this cleanly with the rest of your file, please:
1. Ensure the class already imports `ImageUtils`, `ImageRef`, `MockedStatic`, `Mockito`, `IOException`, and AssertJ's `assertThatThrownBy`. If not, add:
- `import java.io.IOException;`
- `import org.mockito.MockedStatic;`
- `import org.mockito.Mockito;`
- `import static org.assertj.core.api.Assertions.assertThatThrownBy;`
- `import io.github.guacsec.trustifyda.util.ImageUtils;` (or the correct package for `ImageUtils` in your project)
2. If `ImageUtils.generateImageSBOM(..)` has a different signature (e.g., additional parameters), adjust the `when` stub accordingly:
- Replace `Mockito.any(ImageRef.class)` with the appropriate argument matchers.
3. If `provider.provideComponent()` is not expected to propagate the same exception, you can remove the second `assertThatThrownBy` block or adapt it to the desired behavior.
4. If your test class uses a different static mocking mechanism (e.g., PowerMockito or a JUnit 4 runner), adapt the `try (MockedStatic<...>)` block to match the established pattern.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| assertThat(batchJson.isObject()).isTrue(); | ||
| assertThat(batchJson.size()).isEqualTo(2); | ||
| assertThat(batchJson.has(nodePurl.toString())).isTrue(); | ||
| assertThat(batchJson.has(nginxPurl.toString())).isTrue(); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Cover the case where all images fail SBOM generation to assert the IOException from generateBatchSbomContent
provide_stack_skips_failing_images_in_batch already covers mixed success/failure, but there’s no coverage for the branch where all SBOM generations fail and generateBatchSbomContent throws.
Please add a test that:
- Uses a multi-stage Dockerfile (e.g.,
multi_stage/Dockerfile). - Mocks
ImageUtils.generateImageSBOM(..)to throw for every image. - Asserts
provider.provideStack()(and optionallyprovider.provideComponent()) throws anIOExceptionwhose message contains"No analyzable FROM images found".
This will exercise the error path when SBOM generation fails for all images and protect against regressions.
Suggested implementation:
/** Verifies that batch content skips images that fail SBOM generation and includes the rest. */
@Test
void provide_stack_skips_failing_images_in_batch() throws Exception {
// Given a multi-stage Dockerfile where one image fails SBOM generation
var manifestPath = TEST_MANIFESTS.resolve("multi_stage/Dockerfile");
var provider = new DockerfileProvider(manifestPath);
ImageRef nodeRef = Mockito.mock(ImageRef.class);
JsonNode nginxSbom = MAPPER.createObjectNode().put("bomFormat", "CycloneDX");
ImageRef nginxRef = Mockito.mock(ImageRef.class);
PackageURL nginxPurl = new PackageURL("pkg:oci/nginx@alpine");
}
/**
* Verifies that when all images fail SBOM generation, provideStack throws an IOException
* with a helpful message instead of returning an empty batch.
*/
@Test
void provide_stack_throws_when_all_images_fail_sbom_generation() throws Exception {
// Given a multi-stage Dockerfile where every FROM image fails SBOM generation
var manifestPath = TEST_MANIFESTS.resolve("multi_stage/Dockerfile");
var provider = new DockerfileProvider(manifestPath);
try (MockedStatic<ImageUtils> imageUtilsMock = Mockito.mockStatic(ImageUtils.class)) {
imageUtilsMock
.when(() -> ImageUtils.generateImageSBOM(Mockito.any(ImageRef.class)))
.thenThrow(new IOException("SBOM generation failed"));
assertThatThrownBy(provider::provideStack)
.isInstanceOf(IOException.class)
.hasMessageContaining("No analyzable FROM images found");
// Optionally also cover provideComponent to ensure it propagates the same error
assertThatThrownBy(provider::provideComponent)
.isInstanceOf(IOException.class)
.hasMessageContaining("No analyzable FROM images found");
}I could only see part of the existing provide_stack_skips_failing_images_in_batch test body, so in the replacement block above I preserved the visible portion verbatim and closed the method before adding the new test.
To integrate this cleanly with the rest of your file, please:
- Ensure the class already imports
ImageUtils,ImageRef,MockedStatic,Mockito,IOException, and AssertJ'sassertThatThrownBy. If not, add:import java.io.IOException;import org.mockito.MockedStatic;import org.mockito.Mockito;import static org.assertj.core.api.Assertions.assertThatThrownBy;import io.github.guacsec.trustifyda.util.ImageUtils;(or the correct package forImageUtilsin your project)
- If
ImageUtils.generateImageSBOM(..)has a different signature (e.g., additional parameters), adjust thewhenstub accordingly:- Replace
Mockito.any(ImageRef.class)with the appropriate argument matchers.
- Replace
- If
provider.provideComponent()is not expected to propagate the same exception, you can remove the secondassertThatThrownByblock or adapt it to the desired behavior. - If your test class uses a different static mocking mechanism (e.g., PowerMockito or a JUnit 4 runner), adapt the
try (MockedStatic<...>)block to match the established pattern.
Contributor
Author
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as suggestion — this proposes additional test coverage for the all-images-fail SBOM generation path. While reasonable, CONVENTIONS.md does not require exhaustive error-path coverage, and the existing test suite already covers the error path in parseAllFromImages (all_invalid scenario). No sub-task created.
Contributor
Author
|
[sdlc-workflow/verify-pr] Re: @sourcery-ai[bot] review — Classified as suggestion — (1) scratch case-sensitivity: addressed via sub-task TC-5098 created from the inline comment. (2) buildAnalysisUri URI building: this proposes using a URI builder instead of string concatenation, which is a valid robustness suggestion but has no established codebase pattern or CONVENTIONS.md backing. No sub-task created for item (2). |
Contributor
Author
Verification Report for TC-5071 (commit 3b43c23)
Overall: WARNIssues requiring attention:
This comment was AI-generated by sdlc-workflow/verify-pr v0.12.2. |
Change "scratch".equals(image) to "scratch".equalsIgnoreCase(image) so FROM SCRATCH, FROM Scratch, and other case variants are properly skipped during Dockerfile parsing, matching the case-insensitive handling of the FROM keyword itself. Fixes: TC-5098 Assisted-by: Claude Code
ruromero
requested changes
Jul 8, 2026
ruromero
left a comment
ruromero
left a comment
Collaborator
There was a problem hiding this comment.
Review Summary
The PR adds Dockerfile/Containerfile support by parsing FROM lines, generating per-image SBOMs via Syft, and sending them to the batch-analysis endpoint. The Dockerfile parsing logic (ARG resolution, scratch skipping, multi-stage support) is solid and well-tested. However, the analysis routing has an architectural issue that breaks parity with how both IDE extensions currently consume image results.
Critical: Dockerfile analysis must route through imageAnalysis(), not stackAnalysis()
Both IDE extensions currently handle Dockerfiles by calling imageAnalysis(Set<ImageRef>), which returns Map<ImageRef, AnalysisReport> — one report per FROM image:
- IntelliJ (
image/ApiService.java:69):exhortApi.imageAnalysis(imageRefs) - VS Code (
rhda.ts:116-117):fileType === 'docker'→executeDockerImageAnalysis()→exhort.imageAnalysis()
This PR routes Dockerfile content through stackAnalysis()/componentAnalysis() instead, which return CompletableFuture<AnalysisReport> (single report). The batch results are collapsed with:
reports.values().iterator().next(); // takes only the FIRST image's reportFor a multi-FROM Dockerfile, all image reports after the first are silently dropped.
Suggestion: Keep stack Dockerfile as a valid CLI command, but internally detect that the manifest is a Dockerfile and delegate to the existing imageAnalysis(Set<ImageRef>) path. The DockerfileProvider parsing logic (FROM extraction, ARG resolution) is valuable — it should produce Set<ImageRef> that feeds into imageAnalysis(), preserving the Map<ImageRef, AnalysisReport> return type.
This also eliminates the duplicated batch handling code in stackAnalysis (inline) and getBatchAnalysisReportForComponent — the existing imageAnalysis already does all of this correctly.
Additionally, stackAnalysisHtml() was not updated for batch content at all (still uses buildStackRequest), so generating an HTML report for a Dockerfile via this path would not use the batch endpoint.
Bug: Docker Hub normalization casing inconsistency in ImageRef.getPackageURL()
The library-prefix branch uses lower.substring() which lowercases the image name, while the bare-name branch preserves original casing via simpleName:
if (lower.equals(simpleName.toLowerCase())) {
repositoryUrl = "docker.io/" + simpleName; // preserves casing
} else if (lower.startsWith(DOCKER_HUB_LIBRARY_PREFIX)) {
repositoryUrl = "docker.io/" + lower.substring(...); // lowercased
}docker.io/library/MyImage → docker.io/myimage (lowercased)
MyImage → docker.io/MyImage (preserved)
Both should behave the same. The library-prefix branch should use repositoryUrl.substring(DOCKER_HUB_LIBRARY_PREFIX.length()) to preserve original casing, consistent with the bare-name branch. (Note: repositoryUrl is later lowercased when put into qualifiers anyway, but the PURL name field should be consistent.)
Low: Recommend env var naming mismatch across clients
Java uses TRUSTIFY_DA_RECOMMEND, JavaScript uses TRUSTIFY_DA_RECOMMENDATIONS_ENABLED. These should be aligned so users don't need different env vars per client. The JS client's name is already established, so consider adopting TRUSTIFY_DA_RECOMMENDATIONS_ENABLED in the Java client as well.
Also note the semantic inversion: Java defaults to true (recommend enabled unless explicitly set to false), while JS checks for the string 'false'. They happen to have the same default behavior, but the var names should match.
Low: Silent license check skip for batch content
if (!isLicenseCheckEnabled() || content.batch) {
return CompletableFuture.completedFuture(new ComponentAnalysisResult(report, null));
}License checking is silently skipped when content.batch is true, with no logging. This is likely intentional (license check doesn't apply to images), but a debug log would help troubleshooting.
Positive
- Dockerfile parsing is well-structured: correct handling of multi-stage builds, ARG substitution, scratch skipping, and case-insensitive FROM matching
- Excellent test coverage: 26 tests for DockerfileProvider with 13 fixture Dockerfiles covering edge cases
- The
Ecosystem.isDockerfile()correctly requires a dot separator, avoiding false matches on files likeDockerfileignore
…mage results Dockerfile/Containerfile manifests now use imageAnalysis() instead of stackAnalysis(), preserving per-image Map<ImageRef, AnalysisReport> output. This prevents multi-FROM results from being collapsed to a single report. - Add DockerfileProvider.parseImageRefs() to convert FROM lines to ImageRefs - Detect Dockerfile in CLI before command routing, redirect to image analysis - Use image full names as JSON keys in formatImageAnalysisResult() - Block sbom/license commands for Dockerfiles with clear error message - Make Ecosystem.isDockerfile() public for CLI detection Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
3 participants
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.
Summary
batchflag toProvider.Contentto route between/api/v5/analysisand/api/v5/batch-analysisendpoints${VAR}and$VARsyntax) using default values; skipFROM scratchand unresolvable ARGsExhortApifor component, stack, and license analysis flowsscratchcomparison soFROM SCRATCH,FROM Scratch, etc. are properly skipped (TC-5098)Test plan
syftscenarios: hardened recommendations + recommendations disabled)Fixes: TC-5071
Fixes: TC-5098
🤖 Generated with Claude Code