Reject ambiguous resource paths with inner ".." to prevent silent misresolution#6087
Conversation
|
Hi @0xMH. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
987c662 to
2941bf1
Compare
|
/ok-to-test |
|
This PR has multiple commits, and the default merge method is: merge. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
eb00182 to
2bbf133
Compare
|
/label tide/merge-method-squash |
|
@0xMH |
|
@koba1t Done! |
|
Hi @koba1t, I think the lint errors reported in CI are all pre-existing issues in I'm happy to fix them in a separate commit on this PR, but wanted to flag that none of these are caused by my changes. What should we do in this situation? Would you prefer I clean them up here, or should they be addressed in a separate PR? |
|
@0xMH |
ec59321 to
7fd88c2
Compare
koba1t
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
/lgtm
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 0xMH, chansuke, koba1t The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I dug into #5979 where a YAML indentation error silently causes kustomize to load resources from the wrong directory.
When a kustomization.yaml has a subtle indentation mistake:
YAML parsing collapses the first two entries into a single string:
"../../base - ../../shared/prod".The combined string then hits
filepath.Clean, which processes the segments:..+..-> normal, goes up two directoriesbase - ..-> treated as a single directory name (with spaces and dash)..-> Clean sees this and cancels the previous component (base - ..), removing it..-> goes up one moreshared/prod-> descends into final pathSo Clean collapses it to
../../shared/prod. Thebase - ..segment gets absorbed by the..that follows it, as if it never existed.The root cause is that YAML merged two list entries into one string (
"../../base - ../../shared/prod"), creating a path componentbase - ..that looks like a directory name but contains..from the second entry.filepath.Cleanthen treats the subsequent..as "go up", which eatsbase - ..and erases any trace of../../base.Since
/shared/prodis a real directory,ConfirmDirpasses and kustomize silently loads the wrong resources. No error is raised. This is dangerous in CI/CD environments where missing manifests can cause resources to be deleted.Fix
I added a
hasInnerDotDotcheck inFileLoader.New()that rejects paths containing..after a non-..component. These are always either a YAML artifact or a non-canonical path with a simpler equivalent (foo/../baris justbar). No valid use case that I know of is affected.There was a prior attempt in #5980, but it checked existence after normalization, so if the wrong directory exists the check passes.
/kind bug
/fix #5979