Skip to content

fix(s3s-fs)!: reject path aliases in object keys - #996

Open
overtrue wants to merge 1 commit into
s3s-project:mainfrom
overtrue:overtrue/fix-fs-object-path-aliases
Open

overtrue wants to merge 1 commit into
s3s-project:mainfrom
overtrue:overtrue/fix-fs-object-path-aliases

Conversation

@overtrue

@overtrue overtrue commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

AI-assisted; human review is pending.

Summary

s3s-fs normalizes object keys into filesystem paths, so deleting a key such as allowed/../victim can remove a different object. Reject dot segments, repeated separators and absolute paths before resolving them, and return per-key errors in batch deletion.

Changes

  • Validate keys in the shared object-path helper, covering reads, writes, copies, multipart completion and deletion.
  • Keep valid batch entries working before and after a rejected key; preserve the submitted key and version ID in the error.
  • Add a regression that checks the bytes of sibling and other-bucket objects, plus path validation controls for ordinary filenames and Windows prefixes.

Behavior

The filesystem backend rejects these keys with InvalidArgument. Windows backslash keys are also rejected because they alias forward-slash paths on that platform. Dot filenames such as .config and report..txt, nested keys and directory objects remain accepted. Unix backslash filenames and other backends keep the existing S3 key rules.

Tests

  • Red control on unmodified main: the batch regression fails because the seeded victim file is deleted. A separate red control on the previous PR head reproduces the repeated-separator deletion.
  • just dev: passed; 3,609 tests passed, 5 existing tests ignored.
  • just ci-rust: passed, including codegen idempotence and the clean-tree check.
  • cargo +nightly clippy --workspace --all-targets --all-features -- -D warnings: passed (exit 0).

The local runs use macOS. Windows-specific path cases are included for CI.

AI

GPT-6 assisted with the investigation, patch and tests. The checks above ran locally; human review is pending.

Copilot AI balanced review requested due to automatic review settings October 9, 2026 13:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Repeated separators remain a cross-key deletion alias despite the new validation.

1 open finding
What changed in this PR

Hardens s3s-fs object-path handling and reports invalid batch-delete entries individually.

Changes:

  • Rejects dot-segment and absolute object paths.
  • Preserves valid batch deletions around rejected entries.
  • Adds path-validation and deletion regressions.
File Description
crates/​s3s-fs/​src/​fs.rs Validates object paths and adds unit tests.
crates/​s3s-fs/​src/​s3.rs Returns per-key deletion errors and adds regression coverage.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread crates/s3s-fs/src/fs.rs Outdated
@codecov

codecov Bot commented Oct 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Reject dot segments, repeated separators and absolute or platform paths before filesystem normalization. Batch deletion reports invalid keys individually and continues with valid entries.

Assisted-by: GPT-6
@overtrue
overtrue force-pushed the overtrue/fix-fs-object-path-aliases branch from 575de4f to 0d7fa1d Compare October 9, 2026 14:07
@overtrue overtrue changed the title fix(s3s-fs)!: reject dot-segment and absolute object paths fix(s3s-fs)!: reject path aliases in object keys Oct 9, 2026
@overtrue
overtrue marked this pull request as ready for review October 9, 2026 21:42
@Nugine
Nugine self-requested a review October 10, 2026 04:54

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

None yet

Development

Successfully merging this pull request may close these issues.

2 participants