buffer: add mask() and unmask()

[jasnell]

Add http.websocketMask(source, mask, output[, offset[, length]]) and http.websocketUnmask(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.93939% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.36%. Comparing base (f71d644) to head (ac760bc).
⚠️ Report is 28 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.36%            
========================================
  Files         792      792            
  Lines      275564   275728   +164     
  Branches    52832    52867    +35     
========================================
+ Hits       249016   249174   +158     
+ Misses      16965    16957     -8     
- Partials     9583     9597    +14     

Files with missing lines	Coverage Δ
lib/http.js	99.13% <100.00%> (+0.28%)	⬆️
src/node_buffer.cc	71.40% <87.17%> (+1.00%)	⬆️

... and 22 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]

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

[jasnell]

Ok, I've renamed and move the apis. They are now:

• http.websocketMask(...)
• http.websocketUnmask(...)

I went ahead and kept the internal binding as is (on the buffer internal binding and called mask) because that didn't feel as important but happy to switch that to if anyone feels strongly about it.

[nodejs-github-bot]

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

[lpinca]

RSLGTM

[ronag]

TBH we should not do masking in websocket at all. Just use a 0 mask and we're still spec compliant. The use case for masking is not really relevant in the real world.

[jasnell]

Ok, my main motivation here is around the performance improvement, not the merits of the masking at all. Whether it's best to use a zero mask or not is something I'd need to leave to you (@ronag), @lpinca, @KhafraDev, etc to work out. For now, I'll mark this blocked pending some form of decision/consensus.

[KhafraDev]

ws has a generateMask option to skip it iirc. This will make masking and unmasking faster for everyone else.

[KhafraDev]

cc @tsctx I know you were working on some masking optimizations

[lpinca]

TBH we should not do masking in websocket at all. Just use a 0 mask and we're still spec compliant. The use case for masking is not really relevant in the real world.

Perhaps, but I would argue that it is not spec compliant.

When preparing a masked frame, the client MUST pick a fresh masking
key from the set of allowed 32-bit values. The masking key needs to
be unpredictable; thus, the masking key MUST be derived from a strong
source of entropy, and the masking key for a given frame MUST NOT
make it simple for a server/proxy to predict the masking key for a
subsequent frame. The unpredictability of the masking key is
essential to prevent authors of malicious applications from selecting
the bytes that appear on the wire.

[pipobscure]

TBH we should not do masking in websocket at all. Just use a 0 mask and we're still spec compliant. The use case for masking is not really relevant in the real world.

Yeah, I wish masking wasn’t in the spec as well, but I think part of the goal is to make websocket implementations not have to rely on something like bufferutil.

Given that such an implementation can implement both the client as well as the server side, the side masking could be on the other end, so just because we don’t mask (by masking with 0) does not actually mean we don’t have to deal with actually masked frames. So I’m pretty sure this suggestion doesn’t actually hold.

It sort of works so long as we only think about client sockets, but as soon as you have server sockets a client will send actually masked frames. And arguably on the server side performance is even more relevant.

[jasnell]

Sounds like we managed to reach a decision/consensus faster than I thought 😆 ...

It looks like there's general agreement. But I'm not going to be in a rush to land. Will leave this open for another day or two in case there are any other thoughts that come up. Assuming things still look good I'll run CI again, pray to the flaky test gods, then get this landed.