fix(feature): add SHA-256 integrity verification for direct tar downloads#89
Conversation
✅ Deploy Preview for devsydev canceled.
|
📝 WalkthroughWalkthroughAdds SHA-256 integrity checks for cached feature tarballs, persisting/verifying sidecar hashes, deleting invalid caches and re-downloading, and centralizes extraction into a helper. Missing sidecars remain valid for backward compatibility. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
…oads (#89) Downloaded feature tarballs via HTTP had no integrity verification. Compute SHA-256 after download and store as a sidecar file; on cache hits, verify the cached tarball against the stored hash. Hash mismatch triggers re-download. Missing hash files (backward compat) log a warning and continue.
ab4bffb to
7494ed6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
pkg/devcontainer/feature/features.go (2)
124-147: Consider unifying the cache-hit path across OCI and direct-tar features.
processOCIFeaturecallscheckFeatureCache(line 152) whileprocessDirectTarFeaturenow inlines its own cache-hit logic (lines 372-381). This is where thefeature.jsonregression flagged above slipped in, and it also means any future cache-policy change has to be made in two places. A small refactor — e.g., extendingcheckFeatureCachewith an optionalintegrityCheck func(featureFolder string) boolparameter, or introducingcheckFeatureCacheWithIntegrity— would let both call sites share the "extracted exists && feature.json present && [optional integrity ok]" logic.Also applies to: 372-381
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/devcontainer/feature/features.go` around lines 124 - 147, The cache-hit logic is duplicated between checkFeatureCache and the inline code in processDirectTarFeature; refactor to unify by extending checkFeatureCache to accept an optional integrityCheck func(featureFolder string) bool (or add a new checkFeatureCacheWithIntegrity wrapper) and update both processOCIFeature and processDirectTarFeature to call the unified function; implement checkFeatureCache to verify the "extracted" folder exists, ensure config.DEVCONTAINER_FEATURE_FILE_NAME (feature.json) is present, and if provided call integrityCheck to validate contents, returning the extracted folder and true only when all checks pass and removing the cacheFolder on failure as the current logic does.
288-297: Considerlog.Debugffor the missing-sidecar branch.For every cache hit on a pre-upgrade feature, this path fires a
Warn. During migration that can flood logs with "No integrity hash..." for entries users can't act on. Since this is the documented backward-compat contract (not a real problem),Debugfis a better fit; reserveWarnfor situations the operator might want to address.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/devcontainer/feature/features.go` around lines 288 - 297, In verifyCacheIntegrity, the branch that handles a missing stored hash currently calls log.Warnf which floods logs for expected pre-upgrade cached features; change that call to log.Debugf (keeping the same message and formatting) so missing-sidecar/backward-compat cache entries are logged at debug level; locate the call that references featureFolder/hashFile and the id parameter inside verifyCacheIntegrity and replace Warnf with Debugf.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/devcontainer/feature/features_integrity_test.go`:
- Around line 33-35: Add a targeted gosec suppression for G304 on the
os.ReadFile call: in features_integrity_test.go, locate the variable/hashFile
and the call to os.ReadFile(hashFile) and append a line-level comment
"//nolint:gosec" to that call so the linter ignores the false-positive on this
test-only temporary path.
In `@pkg/devcontainer/feature/features.go`:
- Around line 318-333: storeIntegrityHash currently swallows os.WriteFile errors
which lets processDirectTarFeature continue and leaves an unverifiable cache for
future runs; change storeIntegrityHash to return an error (or a boolean) instead
of logging-and-returning, and update its caller (processDirectTarFeature) to
treat a write failure like a download failure: either propagate the error up to
abort installation or perform cleanup by removing featureFolder and the
downloaded feature.tgz so verifyCacheIntegrity cannot trust the incomplete
cache; reference storeIntegrityHash, processDirectTarFeature and
verifyCacheIntegrity when making these changes.
- Around line 335-340: The os.Open call in extractTarball is flagged by gosec
G304 but downloadFile is built internally (featureFolder + "feature.tgz");
annotate the os.Open invocation in func extractTarball with a //nolint:gosec
comment (same pattern as pkg/hash/hash.go's File method) to suppress G304,
keeping the current error handling and deferred file.Close unchanged.
- Around line 372-381: The direct-tar cache path now only checks
os.Stat(featureExtractedFolder) and verifyCacheIntegrity, but it must also
ensure the extracted folder contains config.DEVCONTAINER_FEATURE_FILE_NAME like
checkFeatureCache and processOCIFeature do; update the branch in features.go
(the block using featureExtractedFolder/featureFolder and calling
verifyCacheIntegrity) to stat for the feature JSON inside featureExtractedFolder
(config.DEVCONTAINER_FEATURE_FILE_NAME) and treat a missing file as a cache miss
by removing featureFolder (same behavior as checkFeatureCache) before
proceeding.
---
Nitpick comments:
In `@pkg/devcontainer/feature/features.go`:
- Around line 124-147: The cache-hit logic is duplicated between
checkFeatureCache and the inline code in processDirectTarFeature; refactor to
unify by extending checkFeatureCache to accept an optional integrityCheck
func(featureFolder string) bool (or add a new checkFeatureCacheWithIntegrity
wrapper) and update both processOCIFeature and processDirectTarFeature to call
the unified function; implement checkFeatureCache to verify the "extracted"
folder exists, ensure config.DEVCONTAINER_FEATURE_FILE_NAME (feature.json) is
present, and if provided call integrityCheck to validate contents, returning the
extracted folder and true only when all checks pass and removing the cacheFolder
on failure as the current logic does.
- Around line 288-297: In verifyCacheIntegrity, the branch that handles a
missing stored hash currently calls log.Warnf which floods logs for expected
pre-upgrade cached features; change that call to log.Debugf (keeping the same
message and formatting) so missing-sidecar/backward-compat cache entries are
logged at debug level; locate the call that references featureFolder/hashFile
and the id parameter inside verifyCacheIntegrity and replace Warnf with Debugf.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9cc88618-4114-44e2-980d-9550b61a144a
📒 Files selected for processing (2)
pkg/devcontainer/feature/features.gopkg/devcontainer/feature/features_integrity_test.go
| hashFile := filepath.Join(dir, "feature.sha256") | ||
| stored, err := os.ReadFile(hashFile) | ||
| s.Require().NoError(err) |
There was a problem hiding this comment.
Lint Gate failure: add a //nolint:gosec annotation for G304.
The pipeline's Lint Gate fails at line 34 on os.ReadFile(hashFile) (gosec G304). hashFile is a test-constructed path under TempDir, so this is a false positive, but CI still blocks the merge. A targeted suppression keeps the tree clean without broadening the exception.
🔧 Proposed fix
hashFile := filepath.Join(dir, "feature.sha256")
- stored, err := os.ReadFile(hashFile)
+ stored, err := os.ReadFile(hashFile) //nolint:gosec // test-controlled path under t.TempDir()
s.Require().NoError(err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| hashFile := filepath.Join(dir, "feature.sha256") | |
| stored, err := os.ReadFile(hashFile) | |
| s.Require().NoError(err) | |
| hashFile := filepath.Join(dir, "feature.sha256") | |
| stored, err := os.ReadFile(hashFile) //nolint:gosec // test-controlled path under t.TempDir() | |
| s.Require().NoError(err) |
🧰 Tools
🪛 GitHub Check: Lint Gate
[failure] 34-34:
G304: Potential file inclusion via variable (gosec)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/devcontainer/feature/features_integrity_test.go` around lines 33 - 35,
Add a targeted gosec suppression for G304 on the os.ReadFile call: in
features_integrity_test.go, locate the variable/hashFile and the call to
os.ReadFile(hashFile) and append a line-level comment "//nolint:gosec" to that
call so the linter ignores the false-positive on this test-only temporary path.
| func extractTarball(downloadFile, dest string) error { | ||
| file, err := os.Open(downloadFile) | ||
| if err != nil { | ||
| return fmt.Errorf("open tarball: %w", err) | ||
| } | ||
| defer func() { _ = file.Close() }() |
There was a problem hiding this comment.
Lint Gate failure: add a //nolint:gosec annotation for G304 on os.Open.
Same pipeline failure as the test file. downloadFile is constructed internally from featureFolder/"feature.tgz", not user input, but gosec cannot tell. pkg/hash/hash.go already uses this pattern for its own os.Open (see the relevant snippet of hash.File).
🔧 Proposed fix
func extractTarball(downloadFile, dest string) error {
- file, err := os.Open(downloadFile)
+ file, err := os.Open(downloadFile) //nolint:gosec // downloadFile is derived from the internal feature cache dir, not user input
if err != nil {
return fmt.Errorf("open tarball: %w", err)
}🧰 Tools
🪛 GitHub Check: Lint Gate
[failure] 336-336:
G304: Potential file inclusion via variable (gosec)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/devcontainer/feature/features.go` around lines 335 - 340, The os.Open
call in extractTarball is flagged by gosec G304 but downloadFile is built
internally (featureFolder + "feature.tgz"); annotate the os.Open invocation in
func extractTarball with a //nolint:gosec comment (same pattern as
pkg/hash/hash.go's File method) to suppress G304, keeping the current error
handling and deferred file.Close unchanged.
|
|
||
| // Check cache — verify integrity if present. | ||
| _, statErr := os.Stat(featureExtractedFolder) | ||
| if statErr == nil && !forceDownload { | ||
| if verifyCacheIntegrity(featureFolder, id) { | ||
| log.Debugf("direct tar feature already cached: folder=%s", featureExtractedFolder) | ||
| return featureExtractedFolder, nil | ||
| } | ||
| _ = os.RemoveAll(featureFolder) | ||
| } |
There was a problem hiding this comment.
Regression: extracted-folder sanity check (feature.json presence) was dropped for direct-tar features.
Previously the direct-tar path went through checkFeatureCache (lines 131-144), which stats extracted/ and verifies that config.DEVCONTAINER_FEATURE_FILE_NAME is present — removing the whole cache if the extraction looked incomplete. The new flow only checks os.Stat(featureExtractedFolder) plus the tarball hash. If a prior run died mid-extraction (mkdir succeeded, feature.json never written), the tarball + sidecar are still consistent with each other and this branch will happily return an empty/partial extracted directory to the caller, breaking downstream installation. processOCIFeature (line 152) still has this guard via checkFeatureCache; the two paths should behave the same.
🛡️ Proposed fix — reinstate the feature.json presence check
// Check cache — verify integrity if present.
- _, statErr := os.Stat(featureExtractedFolder)
- if statErr == nil && !forceDownload {
- if verifyCacheIntegrity(featureFolder, id) {
- log.Debugf("direct tar feature already cached: folder=%s", featureExtractedFolder)
- return featureExtractedFolder, nil
- }
- _ = os.RemoveAll(featureFolder)
+ if !forceDownload {
+ if _, statErr := os.Stat(featureExtractedFolder); statErr == nil {
+ _, fErr := os.Stat(filepath.Join(featureExtractedFolder, config.DEVCONTAINER_FEATURE_FILE_NAME))
+ if fErr == nil && verifyCacheIntegrity(featureFolder, id) {
+ log.Debugf("direct tar feature already cached: folder=%s", featureExtractedFolder)
+ return featureExtractedFolder, nil
+ }
+ _ = os.RemoveAll(featureFolder)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check cache — verify integrity if present. | |
| _, statErr := os.Stat(featureExtractedFolder) | |
| if statErr == nil && !forceDownload { | |
| if verifyCacheIntegrity(featureFolder, id) { | |
| log.Debugf("direct tar feature already cached: folder=%s", featureExtractedFolder) | |
| return featureExtractedFolder, nil | |
| } | |
| _ = os.RemoveAll(featureFolder) | |
| } | |
| // Check cache — verify integrity if present. | |
| if !forceDownload { | |
| if _, statErr := os.Stat(featureExtractedFolder); statErr == nil { | |
| _, fErr := os.Stat(filepath.Join(featureExtractedFolder, config.DEVCONTAINER_FEATURE_FILE_NAME)) | |
| if fErr == nil && verifyCacheIntegrity(featureFolder, id) { | |
| log.Debugf("direct tar feature already cached: folder=%s", featureExtractedFolder) | |
| return featureExtractedFolder, nil | |
| } | |
| _ = os.RemoveAll(featureFolder) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/devcontainer/feature/features.go` around lines 372 - 381, The direct-tar
cache path now only checks os.Stat(featureExtractedFolder) and
verifyCacheIntegrity, but it must also ensure the extracted folder contains
config.DEVCONTAINER_FEATURE_FILE_NAME like checkFeatureCache and
processOCIFeature do; update the branch in features.go (the block using
featureExtractedFolder/featureFolder and calling verifyCacheIntegrity) to stat
for the feature JSON inside featureExtractedFolder
(config.DEVCONTAINER_FEATURE_FILE_NAME) and treat a missing file as a cache miss
by removing featureFolder (same behavior as checkFeatureCache) before
proceeding.
…oads (#89) Downloaded feature tarballs via HTTP had no integrity verification. Compute SHA-256 after download and store as a sidecar file; on cache hits, verify the cached tarball against the stored hash. Hash mismatch triggers re-download. Missing hash files (backward compat) log a warning and continue.
Use filepath.Clean on file paths passed to os.Open and os.ReadFile to satisfy gosec G304 (potential file inclusion via variable).
8084957 to
bfafb7f
Compare
Extend the up-features e2e suite with a test that exercises the SHA-256 integrity verification path. The test downloads a feature tarball via HTTP, then creates a second workspace with the same feature URL and asserts that the cached tarball is reused (integrity check passes, no re-download).
Use filepath.Clean on variable paths passed to os.ReadFile to satisfy gosec G304 (potential file inclusion via variable).
CopyToTempDir changes the working directory, so the second copy could not resolve its relative testdata path. Chdir back to initialDir between the two copy calls.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/tests/up-features/up_features.go (1)
164-169: Optional: strengthen the cache-hit assertion with a positive feature check.The single-handler +
HaveLen(1)assertion already proves no second download was issued, which is the primary invariant under test. For extra signal (and to match the style of the existing"http headers download"spec above), consider adding aDevsySSHprobe after the secondDevsyUpto confirm the cached feature was actually installed into the new workspace (e.g., the same install check the neighboring specs perform). Not blocking.Also applies to: 198-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/up-features/up_features.go` around lines 164 - 169, Add a positive cache-hit check after the second DevsyUp by invoking the same DevsySSH probe used in the neighboring specs to confirm the feature was actually installed from cache (e.g., call DevsySSH to run the install verification command after the second DevsyUp); update the test that uses server.AppendHandlers / HaveLen(1) to include this DevsySSH probe so it asserts not only that no second download occurred but also that the cached feature is present—apply the same change pattern to the other occurrence around the 198-199 block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/tests/up-features/up_features.go`:
- Around line 164-169: Add a positive cache-hit check after the second DevsyUp
by invoking the same DevsySSH probe used in the neighboring specs to confirm the
feature was actually installed from cache (e.g., call DevsySSH to run the
install verification command after the second DevsyUp); update the test that
uses server.AppendHandlers / HaveLen(1) to include this DevsySSH probe so it
asserts not only that no second download occurred but also that the cached
feature is present—apply the same change pattern to the other occurrence around
the 198-199 block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4348a49d-b867-4505-abe2-3a869b61bd52
📒 Files selected for processing (3)
e2e/tests/up-features/up_features.gopkg/devcontainer/feature/features.gopkg/devcontainer/feature/features_integrity_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/devcontainer/feature/features.go
1 participant
Summary
processDirectTarFeature()hash.File()and stores it as afeature.sha256sidecar file alongside the cached tarballverifyCacheIntegrity(),storeIntegrityHash(), andextractTarball()as helper functions to keepprocessDirectTarFeature()under funlen limitsSummary by CodeRabbit
New Features
Tests