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.24%. Comparing base (8a3b11c) to head (40fba96).

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.24%   90.24%   -0.01%     
==========================================
  Files         741      741              
  Lines      241384   241393       +9     
  Branches    45480    45485       +5     
==========================================
- Hits       217844   217838       -6     
+ Misses      15097    15094       -3     
- Partials     8443     8461      +18     

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

... and 27 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.