fs: return Buffer from mkdtemp when prefix is a Buffer

[hamidrezaghavami]

This PR fixes an issue where passing a Buffer as the prefix to the fs.mkdtemp family of functions incorrectly returned a string, which could mangle non-UTF8 paths.

The options encoding is now safely forced to 'buffer' when a Buffer prefix is detected for both the asynchronous, synchronous, and disposable versions.

Fixes: #58795

[bakkot]

Thanks!

• Should be BufferIsBuffer not Buffer.isBuffer
• Can't mutate the options object; you'll need to do options = { ...options, encoding: buffer } or something
• Needs tests
• Possibly it should be an error if encoding is explicitly specified (and not buffer)?

[codecov]

Codecov Report

❌ Patch coverage is 80.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.13%. Comparing base (ccd648e) to head (9d77adc).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/fs/promises.js	50.00%	3 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #64397 +/- ##
==========================================
- Coverage 90.14% 90.13% -0.01% 
==========================================
Files 769 769 Lines 261626 261654 +28 Branches 49663 49673 +10 ==========================================
+ Hits 235832 235851 +19 - Misses 16805 16811 +6 - Partials 8989 8992 +3 

Files with missing lines	Coverage Δ
lib/fs.js	97.30% <100.00%> (+<0.01%)	⬆️
lib/internal/fs/promises.js	91.12% <50.00%> (-0.12%)	⬇️

... and 25 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[hamidrezaghavami]

@bakkot The CI is green! Here is a quick update:

Buffer check: Switched to using the internal BufferIsBuffer.
Immutability: Fixed the code to avoid mutating the user's options object.
Regression test: Added a full test suite for the Buffer prefix (test-fs-mkdtemp-buffer.js).
Original tests: Patched a string coercion bug in the original test-fs-mkdtemp.js file that was crashing the CI.

Let me know if there is anything else I need to adjust!

[bakkot]

What happened to these tests?

[bakkot]

I'm not a maintainer but the new tests look good, thanks.

Still not sure about, should it be an error if encoding is explicitly specified and not buffer. I don't know if this comes up anywhere else; if so we could look at what the established pattern is. If there isn't an established pattern I would be inclined to throw here.

[hamidrezaghavami]

My mistake! I accidentally deleted them while fixing the string coercion bug above. Just restored them in the latest push.

[hamidrezaghavami]

@nodejs/collaborators All JavaScript changes and tests are perfectly clean locally!
However, I am getting a format-cpp and lint-js-and-md error from the recent upstream merge conflict on SECURITY.md and node_trace_buffer.h. Could a maintainer please help run the formatters for me since I don't have the local C++ tooling installed? (Also, test-internet appears to be a flaky failure).
Ready for your final review!

[Renegade334]

Please do not add merge commits, it breaks the tooling.

https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#step-5-rebase

[hamidrezaghavami]

Thank you so much @aduh95 for stepping in and pushing that final fix—you are a lifesaver! 🙏 All 31 checks are completely green now! 🚀
@Renegade334 Since the CI checks have all passed, could either of you please give this a final approving review whenever you have a moment? Ready to merge!

[Renegade334]

Looks like a response is outstanding to #64397 (comment)

[hamidrezaghavami]

@Renegade334 Thanks for pointing that out! Since bakkot mentioned he isn't a core maintainer, I wasn't sure if his question was a required change or just a general discussion, especially since @aduh95 already reviewed and force-pushed the final code today. Could you clarify if there is still anything specific you would like me to change, or are we good to go? 🙏

[bakkot]

@hamidrezaghavami The link goes to my comment about deleted tests, not my question about an error. You said you'd restored the deleted tests but I think that got lost in your rebase. Those tests are still missing and you should presumably put them back.

[hamidrezaghavami]

@bakkot Looks like @aduh95 already restored those missing tests when they force-pushed the branch earlier today! Everything is back in place now. 🙏

[bakkot]

No, they're still missing.

[hamidrezaghavami]

@bakkot Good catch! Ah, they were sitting unsaved in my local editor when I checked earlier. I just pushed commit f30698f restoring all three missing async buffer tests (bar., options object, and bar. X). All CI checks are green again! 🙏

[aduh95]

We still want a doc update