Skip to content

fix(ext/node): normalize recursive fs mkdir dot-segment paths#35463

Open
nathanwhitbot wants to merge 8 commits into
denoland:mainfrom
nathanwhitbot:orcha/impl-aba89889
Open

fix(ext/node): normalize recursive fs mkdir dot-segment paths#35463
nathanwhitbot wants to merge 8 commits into
denoland:mainfrom
nathanwhitbot:orcha/impl-aba89889

Conversation

@nathanwhitbot

@nathanwhitbot nathanwhitbot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Normalize paths passed to Deno's filesystem API for recursive mkdir so lexical dot-segments like a trailing /. do not make recursive creation fail (matches mkdir -p). The fix lives in RealFs::mkdir.
  • Add focused coverage for sync and callback recursive node:fs mkdir paths containing literal dot segments, plus Deno.mkdir unit coverage.

Closes #35450

Scope

Because the fix lives in RealFs::mkdir, it changes behavior for both node:fs mkdir/mkdirSync and Deno.mkdir/Deno.mkdirSync — e.g. Deno.mkdir("foo/.", { recursive: true }) now succeeds instead of failing. This is intentional (covered by tests/unit/mkdir_test.ts) and is a reasonable correctness improvement matching mkdir -p, though it is broader than the ext/node title suggests.

.. segments are deliberately preserved: Path::components() strips . but keeps ParentDir, so a/../b/c/.a/../b/c and the filesystem resolves .. itself, avoiding lexical symlink-flattening issues. Pinned by the "preserves parent segment" test.

Tests

  • env -u RUSTC_WRAPPER cargo test -p unit_node_tests --test unit_node -- _fs::_fs_mkdir_test
  • ./target/debug/deno fmt --check tests/unit_node/_fs/_fs_mkdir_test.ts
  • git diff --check

Regression coverage was also checked by temporarily reverting the implementation, rebuilding deno, and confirming the new dot-segment tests fail with ENOENT before restoring the fix.

@nathanwhitbot

Copy link
Copy Markdown
Contributor Author

Pushed fix(node): preserve mkdir parent segments to fix the Node compat test-fs-mkdir.js failure. Recursive mkdir now only strips trailing /. segments instead of resolving the whole path, so a/../b/c keeps Node’s lexical parent creation behavior.

@nathanwhitbot

Copy link
Copy Markdown
Contributor Author

Pushed a CI follow-up: replaced the direct path.slice() call with StringPrototypeSlice for prefer-primordials, and adjusted the new mkdir dot-segment test so the lexical parent assertion only runs on non-Windows while still checking the requested target path.

@nathanwhitbot

Copy link
Copy Markdown
Contributor Author

Pushed a formatter-only follow-up for the recursive mkdir path normalization helper. The failing lint job was dprint asking for the long return expression in stripTrailingCurrentDirSegments() to be split over multiple lines.

@nathanwhit

Copy link
Copy Markdown
Member

are you sure this shouldn't be fixed in the rust part of the impl instead of a js normalization? this affect Deno.mkdir as well...

@nathanwhitbot

Copy link
Copy Markdown
Contributor Author

Good catch. I moved the recursive mkdir path normalization into the Rust ext/fs implementation so it covers Deno.mkdir/mkdirSync too, and left Node’s wrapper using the original path for return/error semantics. Added Deno namespace coverage for recursive /. paths.

@nathanwhitbot

Copy link
Copy Markdown
Contributor Author

Pushed fix(node): remove unused fs primordial import to address the failing lint jobs. The CI failure was no-unused-vars for StringPrototypeSlice in ext/node/polyfills/fs.ts, and the new commit removes that stale import.

@bartlomieju bartlomieju changed the title fix(node): normalize recursive fs mkdir dot-segment paths fix(ext/node): normalize recursive fs mkdir dot-segment paths Jun 29, 2026
@bartlomieju

Copy link
Copy Markdown
Member

Nice, small targeted fix. A few notes:

Drop the ext/node/polyfills/fs.ts hunks — they're pure formatting churn. Both changes only reflow Deno.mkdir(path, { recursive, mode }) from one line onto three; there's no functional change. The PR description says these "preserve original validated paths," but that preservation actually comes for free from not normalizing on the JS side — it isn't implemented by these hunks. Removing them keeps the PR minimal.

Scope note for the description: because the fix lives in RealFs::mkdir, this also changes Deno.mkdir/Deno.mkdirSync (e.g. Deno.mkdir("foo/.", { recursive: true }) now succeeds), not just node:fs. That looks intentional (you added tests/unit/mkdir_test.ts coverage) and is a reasonable correctness improvement matching mkdir -p, but the title says ext/node while the behavior change is broader — worth calling out.

Add a one-line comment on normalize_recursive_mkdir_path noting it works around DirBuilder::create failing on trailing dot-segments (link #35450). It reads as generic normalization otherwise.

Otherwise LGTM. Good call preserving ..Path::components() strips . but keeps ParentDir, so a/../b/c/.a/../b/c and the filesystem still resolves .. itself, avoiding lexical symlink-flattening issues. The "preserves parent segment" test pins that nicely.

…tion

Revert the ext/node/polyfills/fs.ts reflow hunks (no functional change;
path preservation comes from not normalizing on the JS side). Add a
comment on normalize_recursive_mkdir_path explaining it works around
DirBuilder::create failing on trailing dot-segments (denoland#35450).
@nathanwhitbot

Copy link
Copy Markdown
Contributor Author

Thanks! Addressed all three:

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.

fs.mkdirSync({ recursive: true }) fails for paths containing dot-segments (e.g. "./a/b/c/./")

3 participants