Repository: websockets/ws
Pull request #2285 (2025-04-30 21:04:36)

Fix zlib reset misuse on maxPayload error to prevent callback race and incorrect error code
# Description
This merge request fixes a long-standing issue in the handling of maxPayload violations during permessage-deflate decompression in the ws package.

Previously, when the payload exceeded the configured `maxPayload` size, the internal `zlib.InflateRaw` stream was reset via `.reset()` after removing the 'data' listener. However, this approach left unprocessed data in zlib's internal buffer queue. On large payloads (e.g., zip bombs that inflate to hundreds of megabytes), this residual data could be processed after the reset, causing zlib to throw a fatal error (`Z_DATA_ERROR`) due to missing dictionary state. note that the payload must be large enough so that zlib should fill its buffer and chunks in the buffer should have back-refrence to chunks before `.reset()`.

# Changes
Replaced `this.reset()` with `this.close()` when the maxPayload is exceeded.

This ensures the inflater is safely and fully cleaned up, avoiding race conditions and crashes due to leftover queued chunks.

Fixes the misleading error code: previously, zlib would throw a generic decompression error with status 1007. We now correctly emit a RangeError with status 1009 as per RFC 6455, Section 7.4.1 for "Message too big".

# Security
There is no security impact if the consumer handles the 'error' event properly on the WebSocket. However, without this fix, status code on large payloads would not be `Max payload size exceeded` and would show: 
```
Error: invalid code lengths set
    at genericNodeError (node:internal/errors:984:15)
    at wrappedFn (node:internal/errors:538:14)
    at Zlib.zlibOnError [as onerror] (node:zlib:190:17) {
  errno: -3,
  code: 'Z_DATA_ERROR',
  [Symbol(status-code)]: 1007
}
```

# Testing
Added a test to ensure proper error message is shown when maxPayload is exceeded on large payloads (1GB).
Since this is a race condition (between zlib buffer and reset), I generated a large test case ensure this is always checked. also better resource cleanup on high traffic environments.

Why this matters
This bug has existed undetected for over 8 years due to being masked by zlib's internal buffering behavior and only triggers under high-inflation payloads. Fixing this ensures:

Proper lifecycle management of the inflater.

Compliance with WebSocket error handling standards.

Stability and robustness under malicious or extreme input conditions.

Let me know if you'd like to add credits or reference an issue number.

lpinca: Good catch. There is no race condition, it is just undocumented behavior of `inflate.reset()`. I think it should just discard the data without raising an error. Anyway, `inflate.close()` is already called when the `inflate.flush()` callback is called so I think it is ok to call it sooner. Can you please update as follows?

```diff
diff --git a/lib/permessage-deflate.js b/lib/permessage-deflate.js
index 77d918b..62bded4 100644
--- a/lib/permessage-deflate.js
+++ b/lib/permessage-deflate.js
@@ -366,7 +366,9 @@ class PerMessageDeflate {
       const err = this._inflate[kError];
 
       if (err) {
-        this._inflate.close();
+        //
+        // The stream was already closed in `inflateOnData()`.
+        //
         this._inflate = null;
         callback(err);
         return;
@@ -494,7 +496,7 @@ function inflateOnData(chunk) {
   this[kError].code = 'WS_ERR_UNSUPPORTED_MESSAGE_LENGTH';
   this[kError][kStatusCode] = 1009;
   this.removeListener('data', inflateOnData);
-  this.reset();
+  this.close();
 }
 
 /**

```
Amirali-Amirifar: The workflow fails because of the timeout.
```
  1) PerMessageDeflate
       #compress and #decompress
         doesn't call the callback twice when `maxPayload` is exceeded:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/ws/ws/test/permessage-deflate.test.js)
      at listOnTimeout (internal/timers.js:554:17)
      at processTimers (internal/timers.js:497:7)

  2) Receiver
       emits an error if the message length exceeds `maxPayload`:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/ws/ws/test/receiver.test.js)
      at listOnTimeout (internal/timers.js:554:17)
      at processTimers (internal/timers.js:497:7)

```
Amirali-Amirifar: @lpinca I have added the updated logic, also squashed the commits. The tests now pass.

Patch:
diff --git a/lib/permessage-deflate.js b/lib/permessage-deflate.js
index 77d918b5..41ff70e2 100644
--- a/lib/permessage-deflate.js
+++ b/lib/permessage-deflate.js
@@ -494,6 +494,14 @@ function inflateOnData(chunk) {
   this[kError].code = 'WS_ERR_UNSUPPORTED_MESSAGE_LENGTH';
   this[kError][kStatusCode] = 1009;
   this.removeListener('data', inflateOnData);
+
+  //
+  // The choice to employ `zlib.reset()` over `zlib.close()` is dictated by the
+  // fact that in Node.js versions prior to 13.10.0, the callback for
+  // `zlib.flush()` is not called if `zlib.close()` is used. Utilizing
+  // `zlib.reset()` ensures that either the callback is invoked or an error is
+  // emitted.
+  //
   this.reset();
 }
 
@@ -509,6 +517,12 @@ function inflateOnError(err) {
   // closed when an error is emitted.
   //
   this[kPerMessageDeflate]._inflate = null;
+
+  if (this[kError]) {
+    this[kCallback](this[kError]);
+    return;
+  }
+
   err[kStatusCode] = 1007;
   this[kCallback](err);
 }


Test patch:
diff --git a/test/permessage-deflate.test.js b/test/permessage-deflate.test.js
index a9c9bf16..cf795b79 100644
--- a/test/permessage-deflate.test.js
+++ b/test/permessage-deflate.test.js
@@ -590,9 +590,27 @@ describe('PerMessageDeflate', () => {
       });
     });
 
-    it("doesn't call the callback twice when `maxPayload` is exceeded", (done) => {
+    it('calls the callback when `maxPayload` is exceeded (1/2)', (done) => {
       const perMessageDeflate = new PerMessageDeflate({}, false, 25);
-      const buf = Buffer.from('A'.repeat(50));
+      const buf = Buffer.alloc(50, 'A');
+
+      perMessageDeflate.accept([{}]);
+      perMessageDeflate.compress(buf, true, (err, data) => {
+        if (err) return done(err);
+
+        perMessageDeflate.decompress(data, true, (err) => {
+          assert.ok(err instanceof RangeError);
+          assert.strictEqual(err.message, 'Max payload size exceeded');
+          done();
+        });
+      });
+    });
+
+    it('calls the callback when `maxPayload` is exceeded (2/2)', (done) => {
+      // A copy of the previous test but with a larger input. See
+      // https://github.com/websockets/ws/pull/2285.
+      const perMessageDeflate = new PerMessageDeflate({}, false, 25);
+      const buf = Buffer.alloc(1024 * 1024, 'A');
 
       perMessageDeflate.accept([{}]);
       perMessageDeflate.compress(buf, true, (err, data) => {