Skip to content

fix(services): confine compfs/monoiofs object keys to root by rejecting parent-dir traversal#7702

Merged
erickguan merged 2 commits into
apache:mainfrom
tonghuaroot:fix/confine-compfs-monoiofs-root
Jun 10, 2026
Merged

fix(services): confine compfs/monoiofs object keys to root by rejecting parent-dir traversal#7702
erickguan merged 2 commits into
apache:mainfrom
tonghuaroot:fix/confine-compfs-monoiofs-root

Conversation

@tonghuaroot

@tonghuaroot tonghuaroot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Continuation of #7684; discussed with the PMC on the security list rather than a public issue. Happy to open one if preferred.

Rationale for this change

#7684 confined the fs backend against .. traversal. The two other local-filesystem backends, compfs and monoiofs, have the same gap: both join keys onto root with no .. reject and resolution happens against the host kernel, so a key like ../../etc/passwd escapes root. Remote backends (webdav, sftp, hdfs, …) are out of scope — their paths resolve server-side.

What changes are included in this PR?

  • compfs / monoiofs: prepare_path now rejects keys whose components include .., returning NotFound; every key-join site already routes through it. Normal keys, ., trailing slashes, and a..b-substring keys are unchanged.
  • Regression tests in both crates.

Are there any user-facing changes?

No API change. A key containing .. is now rejected with NotFound instead of resolving outside root.

@tonghuaroot tonghuaroot requested a review from Xuanwo as a code owner June 6, 2026 05:30
@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. releases-note/fix The PR fixes a bug or has a title that begins with "fix" labels Jun 6, 2026
@tonghuaroot tonghuaroot force-pushed the fix/confine-compfs-monoiofs-root branch from 59442fc to a86bab4 Compare June 6, 2026 06:04
Comment thread core/services/monoiofs/src/core.rs Outdated
/// Join a caller-supplied key onto `self.root` while keeping the result
/// confined to that root.
///
/// `normalize_path` (opendal-core) strips leading `/` and empty segments but

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Xuanwo I don't like LLM's code explanation as documentation. I would modify the comments to document the decision why we are doing this.

The code itself is of course good.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point, thanks — done in d734de6. Trimmed the prepare_path comments in both crates to say why the .. reject is there (root confinement; core's normalize_path leaves ./.. unresolved) instead of restating the mechanics. Also tightened the PR description.

@erickguan erickguan self-requested a review June 9, 2026 02:58

@erickguan erickguan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Xuanwo I made another PR with changes. We can merge this and #7712 in sequence.

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Jun 9, 2026
…-only

Address review on apache#7702: the prepare_path doc comments now state why the
`..` reject exists (root confinement; core's normalize_path deliberately
leaves `.`/`..` unresolved) rather than narrating PathBuf/normalize_path
mechanics.
@tonghuaroot

Copy link
Copy Markdown
Contributor Author

Gentle ping — this has been approved by @erickguan and CI is green. Anything else needed before merge?

@erickguan erickguan merged commit 8a89522 into apache:main Jun 10, 2026
100 checks passed
@erickguan

Copy link
Copy Markdown
Member

Thanks!

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

Labels

lgtm This PR has been approved by a maintainer releases-note/fix The PR fixes a bug or has a title that begins with "fix" size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants