Skip to content

confine GCS downloads to destination root#10060

Draft
1seal wants to merge 1 commit into
GoogleContainerTools:mainfrom
1seal:codex/f-skaffold-gcs-dst-root
Draft

confine GCS downloads to destination root#10060
1seal wants to merge 1 commit into
GoogleContainerTools:mainfrom
1seal:codex/f-skaffold-gcs-dst-root

Conversation

@1seal
Copy link
Copy Markdown

@1seal 1seal commented Apr 22, 2026

what changed

  • validate each resolved GCS download target before creating parent directories or writing files
  • reject cleaned object paths that are absolute or that escape the requested destination root
  • add regression coverage for traversal-style object names and absolute-path object names

why

Native.DownloadRecursive derives local download targets from remote object names. before this change, the code joined dst with the derived path and wrote the result without checking that the cleaned destination still stayed under dst.

that meant a crafted object name could resolve outside the intended download root. this patch keeps the existing relative-path behavior for valid objects, but adds an explicit confinement check before any filesystem write.

impact

valid downloads keep their current layout. object names that resolve outside the requested destination root now fail fast instead of creating directories or writing files in unexpected locations.

validation

  • go test ./pkg/skaffold/gcs/client
  • go test -run 'TestResolveDestinationPath|TestDownloadRecursiveRejectsEscapingPaths' -v ./pkg/skaffold/gcs/client

@1seal 1seal changed the title [codex] confine GCS downloads to destination root confine GCS downloads to destination root Apr 22, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request implements path traversal protection in the GCS client by introducing a resolveDestinationPath function to validate that downloaded files remain within the intended destination directory. Corresponding unit tests were added to verify these security constraints. Feedback suggests optimizing performance by resolving the destination root's absolute path outside the file processing loop and removing redundant filepath.Clean calls where filepath.Join or filepath.FromSlash already handle path normalization.


for uri, localPath := range files {
fullPath := filepath.Join(dst, localPath)
fullPath, err := resolveDestinationPath(dst, localPath)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The resolveDestinationPath function performs expensive operations like filepath.Abs (which involves a system call to get the current working directory). Since the destination root dst is constant for all files in the loop, you should resolve the absolute path of dst once outside the loop and pass it to a validation helper. This will significantly improve performance when downloading many files.

}

func resolveDestinationPath(dst, localPath string) (string, error) {
normalizedLocalPath := filepath.Clean(filepath.FromSlash(localPath))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The call to filepath.Clean here is redundant because filepath.FromSlash does not introduce uncleaned segments, and filepath.IsAbs (the only consumer of this variable) does not require a cleaned path to function correctly.

Suggested change
normalizedLocalPath := filepath.Clean(filepath.FromSlash(localPath))
normalizedLocalPath := filepath.FromSlash(localPath)

return "", fmt.Errorf("gcs object path %q escapes destination root %q", localPath, dst)
}

joinedPath := filepath.Clean(filepath.Join(dst, localPath))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The call to filepath.Clean is redundant here because filepath.Join automatically cleans the resulting path.

Suggested change
joinedPath := filepath.Clean(filepath.Join(dst, localPath))
joinedPath := filepath.Join(dst, localPath)

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant