Skip to content

chore: consolidate archive extraction (CN-941)#782

Open
ividalATSnyk wants to merge 2 commits intomainfrom
CN-941-consolidate-archive-extraction
Open

chore: consolidate archive extraction (CN-941)#782
ividalATSnyk wants to merge 2 commits intomainfrom
CN-941-consolidate-archive-extraction

Conversation

@ividalATSnyk
Copy link
Copy Markdown
Contributor

@ividalATSnyk ividalATSnyk commented Apr 7, 2026

  • Ready for review
  • Follows CONTRIBUTING rules
  • Reviewed by Snyk internal team

What does this PR do?

  • wrap docker and kaniko archive extraction under a generic archive extractor to reduce duplication

Where should the reviewer start?

You can take a look at the generic extractor here.

How should this be manually tested?

You can test by running a scan via cli for a Kaniko or Docker Archive.

What are the relevant tickets?

Questions?

There may be a better method for testing manually, open to ideas!

@ividalATSnyk ividalATSnyk changed the title Cn 941 consolidate archive extraction CN-941: consolidate archive extraction Apr 7, 2026
@ividalATSnyk ividalATSnyk changed the title CN-941: consolidate archive extraction chore: consolidate archive extraction (CN-941) Apr 7, 2026
@ividalATSnyk ividalATSnyk marked this pull request as ready for review April 7, 2026 15:38
@ividalATSnyk ividalATSnyk requested a review from a team as a code owner April 7, 2026 15:38
@ividalATSnyk ividalATSnyk requested a review from bgardiner April 7, 2026 15:38
@snyk-pr-review-bot
Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Regression in Archive Handling 🟠 [major]

The new generic extractor uses gunzip() directly on the input stream. The original implementations in docker-archive/layer.ts and kaniko-archive/layer.ts used gunzip-maybe(). By switching to gunzip(), the code will now fail with a 'Z_DATA_ERROR' or 'header check' error if the provided archive is an uncompressed tarball, which is a common scenario for local development or certain CI exports.

.pipe(tarExtractor);
Oversized Error Mapping 🟡 [minor]

The createGetImageIdFromManifest function catches all errors during ID extraction and throws a generic 'Failed to extract image ID' error. If config.extractImageId (which uses .split('.')[0]) is called on a null or undefined manifest.Config, the resulting TypeError is swallowed and replaced. While not a crash, it obscures the root cause (malformed manifest) during debugging.

} catch (err) {
  throw new Error("Failed to extract image ID from archive manifest");
}
📚 Repository Context Analyzed

This review considered 28 relevant code sections from 14 files (average relevance: 0.91)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant