Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 94 additions & 0 deletions e2e/tests/up-features/up_features.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,100 @@ var _ = ginkgo.Describe("testing up command", ginkgo.Label("up-features", "suite
framework.ExpectNoError(err)
}, ginkgo.SpecTimeout(framework.GetTimeout()))

ginkgo.It(
"direct tar feature uses cached download with integrity verification",
func(ctx context.Context) {
server := ghttp.NewServer()
ginkgo.DeferCleanup(server.Close)

tempDir1, err := framework.CopyToTempDir(
"tests/up-features/testdata/docker-features-http-headers",
)
framework.ExpectNoError(err)
ginkgo.DeferCleanup(framework.CleanupTempDir, initialDir, tempDir1)

// CopyToTempDir changes cwd; restore so the second copy resolves its relative path.
err = os.Chdir(initialDir)
framework.ExpectNoError(err)

tempDir2, err := framework.CopyToTempDir(
"tests/up-features/testdata/docker-features-http-headers",
)
framework.ExpectNoError(err)
ginkgo.DeferCleanup(framework.CleanupTempDir, initialDir, tempDir2)

featureArchiveFilePath := path.Join(tempDir1, "devcontainer-feature-hello.tgz")
featureFiles := []string{
path.Join(tempDir1, "devcontainer-feature.json"),
path.Join(tempDir1, "install.sh"),
}
err = createTarGzArchive(featureArchiveFilePath, featureFiles)
framework.ExpectNoError(err)

for _, dir := range []string{tempDir1, tempDir2} {
devContainerFile := filepath.Clean(path.Join(dir, ".devcontainer.json"))
devContainerFileBuf, err := os.ReadFile(devContainerFile)
framework.ExpectNoError(err)

output := strings.ReplaceAll(
string(devContainerFileBuf),
"#{server_url}",
server.URL(),
)
// #nosec G306 -- test file, permissive mode is acceptable.
err = os.WriteFile(path.Join(dir, ".devcontainer.json"), []byte(output), 0o644)
framework.ExpectNoError(err)
}

respHeader := http.Header{}
respHeader.Set(
"Content-Disposition",
"attachment; filename=devcontainer-feature-hello.tgz",
)

featureArchiveFileBuf, err := os.ReadFile(filepath.Clean(featureArchiveFilePath))
framework.ExpectNoError(err)

server.AppendHandlers(
ghttp.CombineHandlers(
ghttp.VerifyRequest("GET", "/devcontainer-feature-hello.tgz"),
ghttp.RespondWith(http.StatusOK, featureArchiveFileBuf, respHeader),
),
)

f := framework.NewDefaultFramework(initialDir + "/bin")
_ = f.DevsyProviderDelete(ctx, "docker")

err = f.DevsyProviderAdd(ctx, "docker")
framework.ExpectNoError(err)

err = f.DevsyProviderUse(ctx, "docker")
framework.ExpectNoError(err)

// First workspace: downloads feature, stores .sha256 sidecar
wsName1 := filepath.Base(tempDir1)
ginkgo.DeferCleanup(f.DevsyWorkspaceDelete, wsName1)

err = f.DevsyUp(ctx, tempDir1)
framework.ExpectNoError(err)

// Delete first workspace; feature cache persists across deletions
err = f.DevsyWorkspaceDelete(ctx, wsName1)
framework.ExpectNoError(err)

// Second workspace: cache hit, integrity verification passes, no download
wsName2 := filepath.Base(tempDir2)
ginkgo.DeferCleanup(f.DevsyWorkspaceDelete, wsName2)

err = f.DevsyUp(ctx, tempDir2)
framework.ExpectNoError(err)

// Only one HTTP request was made — proves cache was reused with passing integrity
gomega.Expect(server.ReceivedRequests()).To(gomega.HaveLen(1))
},
ginkgo.SpecTimeout(framework.GetTimeout()),
)

ginkgo.It("should install with lifecycle hooks", func(ctx context.Context) {
f, err := setupDockerProvider(initialDir+"/bin", "docker")
framework.ExpectNoError(err)
Expand Down
101 changes: 78 additions & 23 deletions pkg/devcontainer/feature/features.go
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,70 @@ func downloadLayer(img v1.Image, id, destFile string) error {
return writeLayerToFile(data, destFile)
}

// verifyCacheIntegrity checks a cached tarball against its stored
// SHA-256 sidecar. Returns true when the cache is safe to use.
func verifyCacheIntegrity(featureFolder, id string) bool {
hashFile := filepath.Join(featureFolder, "feature.sha256")
storedBytes, err := os.ReadFile(filepath.Clean(hashFile))
if err != nil {
log.Warnf(
"No integrity hash for cached feature (backward compat): featureId=%s",
id,
)
return true
}

tarball := filepath.Join(featureFolder, "feature.tgz")
computed, err := hash.File(tarball)
if err != nil {
log.Errorf("Failed to hash cached tarball: error=%v, featureId=%s", err, id)
return false
}

if computed != strings.TrimSpace(string(storedBytes)) {
log.Errorf(
"Integrity check failed for cached feature: featureId=%s",
id,
)
return false
}

log.Debugf("Integrity check passed for cached feature: featureId=%s", id)
return true
}

// storeIntegrityHash computes and persists the SHA-256 of a downloaded tarball.
func storeIntegrityHash(featureFolder, tarballPath, id string) {
computed, err := hash.File(tarballPath)
if err != nil {
log.Errorf("Failed to compute tarball hash: error=%v, featureId=%s", err, id)
return
}

hashFile := filepath.Join(featureFolder, "feature.sha256")
if err := os.WriteFile(hashFile, []byte(computed), 0o600); err != nil {
log.Errorf("Failed to write hash sidecar: error=%v, featureId=%s", err, id)
return
}

log.Infof("Feature tarball integrity: featureId=%s, sha256=%s", id, computed)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

func extractTarball(downloadFile, dest string) error {
file, err := os.Open(filepath.Clean(downloadFile))
if err != nil {
return fmt.Errorf("open tarball: %w", err)
}
defer func() { _ = file.Close() }()
Comment on lines +335 to +340

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.


if err := extract.Extract(file, dest); err != nil {
_ = os.RemoveAll(dest)
return fmt.Errorf("extract folder: %w", err)
}

return nil
}

func processDirectTarFeature(
id string,
httpHeaders map[string]string,
Expand All @@ -299,45 +363,36 @@ func processDirectTarFeature(
)
}

// feature already exists?
featureFolder, err := getFeaturesTempFolder(id)
if err != nil {
return "", fmt.Errorf("resolve feature cache dir: %w", err)
}

featureExtractedFolder := filepath.Join(featureFolder, "extracted")
_, err = os.Stat(featureExtractedFolder)
if err == nil && !forceDownload {
log.Debugf("direct tar feature already cached: folder=%s", featureExtractedFolder)
return featureExtractedFolder, nil

// 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)
}
Comment on lines +372 to 381

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
// 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.


// download feature tarball
// Download feature tarball.
downloadFile := filepath.Join(featureFolder, "feature.tgz")
err = downloadFeatureFromURL(id, downloadFile, httpHeaders)
if err != nil {
log.Errorf("failed to download feature tarball: error=%v, url=%s", err, id)
return "", err
}

// extract file
file, err := os.Open(downloadFile)
if err != nil {
log.Errorf("failed to open downloaded tarball: error=%v, file=%s", err, downloadFile)
return "", err
}
defer func() { _ = file.Close() }()
storeIntegrityHash(featureFolder, downloadFile, id)

// extract tar.gz
err = extract.Extract(file, featureExtractedFolder)
if err != nil {
log.Errorf(
"failed to extract tarball: error=%v, destination=%s",
err,
featureExtractedFolder,
)
_ = os.RemoveAll(featureExtractedFolder)
return "", fmt.Errorf("extract folder: %w", err)
if err := extractTarball(downloadFile, featureExtractedFolder); err != nil {
log.Errorf("failed to extract tarball: error=%v, featureId=%s", err, id)
return "", err
}

log.Infof(
Expand Down
74 changes: 74 additions & 0 deletions pkg/devcontainer/feature/features_integrity_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package feature

import (
"os"
"path/filepath"
"testing"

"github.com/devsy-org/devsy/pkg/hash"
"github.com/stretchr/testify/suite"
)

type IntegrityTestSuite struct {
suite.Suite
}

func TestIntegrityTestSuite(t *testing.T) {
suite.Run(t, new(IntegrityTestSuite))
}

// createTestTarball writes a fake tarball file and returns its path.
func createTestTarball(dir string) (string, error) {
tarball := filepath.Join(dir, "feature.tgz")
return tarball, os.WriteFile(tarball, []byte("fake-tarball-content"), 0o600)
}

func (s *IntegrityTestSuite) TestStoreIntegrityHash_WritesCorrectSidecar() {
dir := s.T().TempDir()
tarball, err := createTestTarball(dir)
s.Require().NoError(err)

storeIntegrityHash(dir, tarball, "test-feature")

hashFile := filepath.Join(dir, "feature.sha256")
stored, err := os.ReadFile(filepath.Clean(hashFile))
s.Require().NoError(err)
Comment on lines +33 to +35

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.


expected, err := hash.File(tarball)
s.Require().NoError(err)
s.Equal(expected, string(stored))
}

func (s *IntegrityTestSuite) TestVerifyCacheIntegrity_ValidHash() {
dir := s.T().TempDir()
tarball, err := createTestTarball(dir)
s.Require().NoError(err)

computed, err := hash.File(tarball)
s.Require().NoError(err)

hashFile := filepath.Join(dir, "feature.sha256")
s.Require().NoError(os.WriteFile(hashFile, []byte(computed), 0o600))

s.True(verifyCacheIntegrity(dir, "test-feature"))
}

func (s *IntegrityTestSuite) TestVerifyCacheIntegrity_CorruptedHash() {
dir := s.T().TempDir()
_, err := createTestTarball(dir)
s.Require().NoError(err)

hashFile := filepath.Join(dir, "feature.sha256")
s.Require().NoError(os.WriteFile(hashFile, []byte("bad-hash"), 0o600))

s.False(verifyCacheIntegrity(dir, "test-feature"))
}

func (s *IntegrityTestSuite) TestVerifyCacheIntegrity_MissingHashFile() {
dir := s.T().TempDir()
_, err := createTestTarball(dir)
s.Require().NoError(err)

// No .sha256 file — backward compat: should return true.
s.True(verifyCacheIntegrity(dir, "test-feature"))
}
Loading