buffer: add mask() and unmask()

[jasnell]

Add buffer.mask(source, mask, output[, offset[, length]]) and buffer.unmask(buffer, mask), which XOR data with a repeating 4-byte key. This is the masking that WebSocket clients apply to every frame they send (RFC 6455, Section 5.3), and that servers undo on every frame they receive.

undici and ws (without optional dependencies) both need mask and unmask. The bufferutil native addon makes it faster but it's an optional dependency that appears to only be installed with ws about 3% of the time. So almost all users run the JS loop.

Since we're shipping undici and WebSocket now, it's worth making this faster.

I put this on buffer because it's an operation on a buffer but it's likely only useful for WebSocket, so we could just as easily expose it via http. I have no particular preference.

For inputs < 24, the JS loop is still faster because we hit the floor of the C++ binding, even with fast apis. There's not much we can do about that. At larger sizes the perf boost is significant.

[nodejs-github-bot]

Review requested:

• @nodejs/performance

[jasnell]

@lpinca

[codecov]

Codecov Report

❌ Patch coverage is 93.50649% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.35%. Comparing base (75e4bbe) to head (3c3b67f).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_buffer.cc	87.17%	0 Missing and 10 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #66335      +/-   ##
==========================================
- Coverage   90.36%   90.35%   -0.02%     
==========================================
  Files         792      792              
  Lines      275398   275578     +180     
  Branches    52776    52819      +43     
==========================================
+ Hits       248877   249011     +134     
- Misses      16937    16976      +39     
- Partials     9584     9591       +7     

Files with missing lines	Coverage Δ
lib/buffer.js	99.75% <100.00%> (+0.01%)	⬆️
src/node_buffer.cc	71.40% <87.17%> (+1.06%)	⬆️

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

[pipobscure]

First off: I like it! (LGTM)

Question: unmask is just mask(buffer,mask,buffer) to mask in place and as you said likely only useful to people that understand that well-enough to actually implement websocket. (And masking is the easy part. Ask me how I know). Is that really worth a separate function? (And yes this is an extremely nitpicking thing that can well be ignored)

[jasnell]

I kept it as a separate function to keep the API shape the same as bufferutil. Happy to coalesce but would like to see what @lpinca thinks

[lpinca]

The discussion about whether to add this feature to the Node.js core dates back to 2015 (#1010 (comment), #1010 (comment), #1202) and I think it would be a valuable addition. Every WebSocket implementation would benefit from this just like buffer.isUtf8() which was originally added for the same reason.

I don't have a strong option about unmask(). I think it is nice sugar and the client only masks while the server only unmasks even though it is basically the same thing. Shipping only mask() is also ok.

Some tests are hard to follow.

[jasnell]

I think there's value in matching the current API so that there's just less for someone migrating from bufferutil to think about.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/78001/ 💛

[lpinca]

RSLGTM

[bakkot]

As a consumer of this API I would be extremely surprised to find that it requires the mask to be exactly 4 bytes. Nothing about the name buffer.mask suggests that to me.

If it's only a specialized utility for WebSocket, I don't think buffer.mask is the right place to put it.

[lpinca]

If it's only a specialized utility for WebSocket, I don't think buffer.mask is the right place to put it.

Fair enough, but even though buffer.isUtf8() isn't specific to WebSockets, the same argument can be raised against it as well.

I'm not sure that exposing it via http is a better solution, it has nothing to do with HTTP. How about util?

[lpinca]

Unrelated, is AI assistance disclosure needed? It seems it was used at least for tests. Feel free to ignore this comment if I'm wrong.

[bakkot]

Fair enough, but even though buffer.isUtf8() isn't specific to WebSockets, the same argument can be raised against it as well.

... Can it? That name is fairly obvious about what it does, and it's useful for a wide variety of things. Not so here.

Anyway I'd also be ok with simply calling it something that makes it clearer that this is not actually a generic mask utility. buffer.maskForWebSocket would be fine, for example.

[jasnell]

I actually don't think it needs to be specific to WebSocket, it's just that currently it's generally the only usage and the impl is optimized for that. It could be made generically useful if someone came up with the usecase. I think, tho, it likely needs to be on either buffer or http, but will go with whatever folks say.

... is AI assistance disclosure needed? It seems it was used at least for tests. Feel free to ignore this comment if I'm wrong.

Ah, totally forgot to add it. Yeah, the AI agent did the tests and the docs.

[jasnell]

Took the "author ready" and "commit queue" labels back off while we figure out the naming and where to expose it.

To be clear, I'm ambivalent on the color of this particular bikeshed so more than happy to go with whatever people dislike the least :-)

[lpinca]

... Can it? That name is fairly obvious about what it does, and it's useful for a wide variety of things. Not so here.

I agree, my point is that it is exposed via buffer only because it works on buffers just like buffer.isUtf8() which was originally intended to be exposed via a new node:encoding module (#45823).

Using buffer.maskForWebSocket() would certainly clarify the purpose but that name hurts my head.

[bakkot]

maskWebSocketPayload, maybe?

[pipobscure]

maskWebSocketPayload, maybe?

That would actually be a much worse name, since a WebSocketFrame consists of much more stuff. There’s a whole header section with all kinds of flags (type, flags, size, extended size, whether it is actually masked). So that would be quite misleading.

[bakkot]

I did almost immediately edit from maskWebSocketFrame to maskWebSocketPayload for that reason :)

[jasnell]

If it's going to be maskWebSocket{Anything} then it should like on http. Given the fairly niche usage... how about we swap it around.. http.webSocketMask(...) and http.webSocketUnmask(...)

[bakkot]

wfm