Validate resource file paths in package operations#1065
Conversation
Signed-off-by: Aravindhan Ayyanathan <aravindhan.a@est.tech>
✅ Deploy Preview for kpt-porch ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR hardens package operations by validating resource map keys (file paths) to reject unsafe paths (e.g., traversal via .. or absolute paths), and reuses a shared safe-join helper by moving it into pkg/util.
Changes:
- Exported
FilepathSafeJoinintopkg/utiland addedValidateResourcePathsfor validating resource map keys. - Enforced path validation in resource replace/update flows and when writing resources to disk during package update.
- Added unit and e2e tests to verify invalid paths are rejected.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/api/rpkg_edit_test.go | Adds an e2e case asserting invalid resource paths are rejected on update. |
| pkg/util/safejoin.go | Moves/exports safe-join logic into util and adds resource-path validation helper. |
| pkg/util/safejoin_test.go | Updates safe-join tests for new package/function name and adds validation tests. |
| pkg/task/replaceresources.go | Validates resource keys before applying replace-resources mutation. |
| pkg/task/replace_test.go | Adds a unit test asserting replace rejects invalid resource paths. |
| pkg/repository/update.go | Uses safe-join when materializing resources to a temp directory to prevent escaping. |
| pkg/repository/update_test.go | Adds tests asserting invalid paths are rejected when writing resources to disk. |
| pkg/engine/engine.go | Validates resource keys before updating resources in no-render update flow. |
| pkg/engine/engine_test.go | Extends tests for no-render update flow to cover invalid path rejection. |
Comments suppressed due to low confidence (3)
pkg/util/safejoin.go:37
- FilepathSafeJoin currently allows relative path ".." to pass validation because the check only rejects paths starting with "../" (".." + separator). With dir="/tmp" and relative="..", filepath.Rel returns ".." and this function returns a path outside dir, which contradicts the function contract and the PR goal of rejecting any ".." traversal.
pkg/util/safejoin.go:47 - ValidateResourcePaths drops the underlying FilepathSafeJoin error and always returns "path traversal not allowed". This is inaccurate for non-traversal failures like absolute paths, and it makes debugging harder. Consider wrapping the actual error so callers see the specific reason.
pkg/util/safejoin_test.go:114 - TestValidateResourcePaths doesn’t currently cover the exact key ".." (without a trailing "/"), which should be rejected just like "../...". Adding a dedicated case helps prevent regressions in FilepathSafeJoin/ValidateResourcePaths around this edge case.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
pkg/util/safejoin.go:37
FilepathSafeJoincurrently treatsrelative == "."as valid (it returnsdir), which means a resource key of "." will pass validation but then later file operations will try to write to the package directory path itself (or otherwise behave unexpectedly). Reject "." explicitly so resource keys must refer to an actual file path within the package.
pkg/util/safejoin.go:43- The doc comment for
ValidateResourcePathssays it checks for "path traversal sequences", but the implementation also rejects absolute paths and other non-canonical relative paths (e.g. leading "./"). Update the comment so callers understand the full set of constraints being enforced.
pkg/util/safejoin_test.go:75 - Add an explicit test case for
relative == "."so the intended behavior (rejecting "." as an invalid relative path) is covered and won’t regress.
pkg/util/safejoin_test.go:129 ValidateResourcePathsshould also reject a resource key of "." (it represents the package directory, not a file). Adding a dedicated test case here will lock in that behavior.
c7e0a15 to
52ae7b6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pkg/util/safejoin.go:26
- The
FilepathSafeJoindoc comment is now narrower than the actual validation performed: the function also rejects non-canonical relative paths like./..and paths that would be cleaned (e.g. leading./,a/../b, redundant separators), not only cases where the join would escapedir. Updating the comment to match behavior will prevent callers from assuming it only blocks directory-escape traversal.
52ae7b6 to
57d7978
Compare
Signed-off-by: Aravindhan Ayyanathan <aravindhan.a@est.tech>
57d7978 to
08cdf5a
Compare
|



Description
..sequences or absolute paths.FilepathSafeJoinutility frompkg/enginetopkg/util(exported, reusable) and added aValidateResourcePathsfunction that checks all keys in a resource map. Validation is called from the resource update and replace paths.Type of Change
Checklist
AI Disclosure
If so, please describe how:
- Kiro CLI has been used for creation of PR and PR message