Skip to content

Reject fetch paths outside the App working directory - #1885

Open
himsngh wants to merge 1 commit into
carvel-dev:developfrom
himsngh:himsngh/fix-fetch-path-traversal
Open

himsngh wants to merge 1 commit into
carvel-dev:developfrom
himsngh:himsngh/fix-fetch-path-traversal

Conversation

@himsngh

@himsngh himsngh commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

spec.fetch[].path is passed to vendir as the directory path without any validation. vendir resolves it relative to the App's temporary working directory and cleans the target directory before syncing, so a value such as "../

" or an absolute path makes vendir delete and replace content outside the working directory, on the controller (or sidecarexec) container filesystem.

Validate fetch.Path in Vendir.AddDir with filepath.IsLocal and return an error for absolute paths and paths that escape the working directory. The reconcile fails at the fetch step with a clear message before vendir is invoked. Relative paths that stay within the working directory (e.g. "subdir", "a/b", "./a", "a/../b") keep working as before.

This brings fetch.path in line with other user-supplied paths (template values, helm path, kbld paths), which are already restricted to the working directory via memdir.ScopedPath.

Changes:

  1. pkg/fetch/vendir.go

    • Reject non-local fetch.Path values in AddDir
  2. pkg/fetch/vendir_test.go

    • Add Test_AddDir_FetchPath covering accepted and rejected paths
  3. pkg/app/app_fetch_test.go (new)

    • Add Test_Fetch_PathOutsideWorkingDir_IsRejected, which runs the fetch step with vendir and verifies that a file outside the working directory is left untouched. Without this change the file is removed by vendir.

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?


Additional Notes for your reviewer:

Review Checklist:
  • Follows the developer guidelines
  • Relevant tests are added or updated
  • Relevant docs in this repo added or updated
  • Relevant carvel.dev docs added or updated in a separate PR and there's
    a link to that PR
  • Code is at least as readable and maintainable as it was before this
    change

Additional documentation e.g., Proposal, usage docs, etc.:


spec.fetch[].path is passed to vendir as the directory path without any
validation. vendir resolves it relative to the App's temporary working
directory and cleans the target directory before syncing, so a value
such as "../<dir>" or an absolute path makes vendir delete and replace
content outside the working directory, on the controller (or
sidecarexec) container filesystem.

Validate fetch.Path in Vendir.AddDir with filepath.IsLocal and return an
error for absolute paths and paths that escape the working directory.
The reconcile fails at the fetch step with a clear message before vendir
is invoked. Relative paths that stay within the working directory
(e.g. "subdir", "a/b", "./a", "a/../b") keep working as before.

This brings fetch.path in line with other user-supplied paths (template
values, helm path, kbld paths), which are already restricted to the
working directory via memdir.ScopedPath.

Changes:

1. pkg/fetch/vendir.go
   - Reject non-local fetch.Path values in AddDir

2. pkg/fetch/vendir_test.go
   - Add Test_AddDir_FetchPath covering accepted and rejected paths

3. pkg/app/app_fetch_test.go (new)
   - Add Test_Fetch_PathOutsideWorkingDir_IsRejected, which runs the
     fetch step with vendir and verifies that a file outside the
     working directory is left untouched. Without this change the
     file is removed by vendir.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Himanshu Singh <himansh.singh3@gmail.com>
@himsngh
himsngh marked this pull request as ready for review October 27, 2026 05:26

This branch has not been deployed

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

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant