optim(utils): sanitize and clamp subpaths to handle path escaping - #6159
optim(utils): sanitize and clamp subpaths to handle path escaping#6159TrafalgarZZZ wants to merge 3 commits into
Conversation
…oots Signed-off-by: 东伝 <dongyun.xzh@alibaba-inc.com>
TrafalgarZZZ
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6159 +/- ##
==========================================
- Coverage 65.13% 65.11% -0.02%
==========================================
Files 485 485
Lines 34039 34090 +51
==========================================
+ Hits 22171 22198 +27
- Misses 10127 10144 +17
- Partials 1741 1748 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Sanitizes Fluid dataset subpaths to prevent lexical parent-directory traversal.
Changes:
- Adds reusable subpath normalization with unit tests.
- Applies normalization across dataset references, runtime metadata, CSI mounts, and sidecar injection.
- Rejects absolute CSI subpaths.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/utils/mount.go |
Adds subpath normalization helper. |
pkg/utils/mount_test.go |
Tests normalization cases. |
pkg/ddc/base/runtime_helper.go |
Normalizes PV subpaths. |
pkg/ddc/base/dataset.go |
Normalizes referenced-dataset subpaths. |
pkg/ddc/base/dataset_test.go |
Tests reference-path clamping. |
pkg/csi/plugins/nodeserver.go |
Validates and clamps CSI subpaths. |
pkg/application/inject/fuse/mutator/mutator_default.go |
Safely joins sidecar host paths. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Reject relative fluid_path values to ensure absolute paths - Validate fluid_path to be within the configured mount root only - Disallow fluid_path paths outside mount root Signed-off-by: 东伝 <dongyun.xzh@alibaba-inc.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: 东伝 <dongyun.xzh@alibaba-inc.com>
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
pkg/ddc/base/dataset.go:61
- This error is emitted when there is exactly one mount but its scheme is not
dataset://, so saying the dataset should have one mount is inaccurate and hides the actual validation failure.
return "", fmt.Errorf("the dataset \"%s/%s\" should only have one mount", virtualDataset.Namespace, virtualDataset.Name)
|
|
||
| // 2. Reject mountPath if it is a symlink. A symlink planted under the FUSE mount point could | ||
| // otherwise redirect the bind mount or the symlink to an arbitrary path on the host. | ||
| if isSymlinkFile, err := checkSymlinkFile(mountPath); err != nil { |
| path = pv.Spec.CSI.VolumeAttributes[common.VolumeAttrFluidPath] | ||
| mountType = pv.Spec.CSI.VolumeAttributes[common.VolumeAttrMountType] | ||
| subpath = pv.Spec.CSI.VolumeAttributes[common.VolumeAttrFluidSubPath] | ||
| if len(subpath) != 0 && !filepath.IsLocal(subpath) { |
| return errors.Wrapf(err, "failed to get mount root for validating %s \"%s\"", attrName, path) | ||
| } | ||
|
|
||
| if !utils.IsSubPath(mountRoot, path) { |
Ⅰ. Describe what this PR does
Ⅱ. Does this pull request fix one issue?
fixes #XXXX
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews