[major] Make the Sec-WebSocket-Extensions header parser stricter

Make the parser throw an error if the header field value is empty or if it begins or ends with a white space.
websockets · Jul 14, 2021 · e814110 · e814110
1 parent 1877dde
commit e814110
Show file tree

Hide file tree

Showing 4 changed files with 42 additions and 37 deletions.
diff --git a/lib/extension.js b/lib/extension.js
@@ -26,26 +26,27 @@ function push(dest, name, elem) {
  */
 function parse(header) {
   const offers = Object.create(null);
-
-  if (header === undefined || header === '') return offers;
-
   let params = Object.create(null);
   let mustUnescape = false;
   let isEscaping = false;
   let inQuotes = false;
   let extensionName;
   let paramName;
   let start = -1;
+  let code = -1;
   let end = -1;
   let i = 0;
 
   for (; i < header.length; i++) {
-    const code = header.charCodeAt(i);
+    code = header.charCodeAt(i);
 
     if (extensionName === undefined) {
       if (end === -1 && tokenChars[code] === 1) {
         if (start === -1) start = i;
-      } else if (code === 0x20 /* ' ' */ || code === 0x09 /* '\t' */) {
+      } else if (
+        i !== 0 &&
+        (code === 0x20 /* ' ' */ || code === 0x09) /* '\t' */
+      ) {
         if (end === -1 && start !== -1) end = i;
       } else if (code === 0x3b /* ';' */ || code === 0x2c /* ',' */) {
         if (start === -1) {
@@ -146,7 +147,7 @@ function parse(header) {
     }
   }
 
-  if (start === -1 || inQuotes) {
+  if (start === -1 || inQuotes || code === 0x20 || code === 0x09) {
     throw new SyntaxError('Unexpected end of input');
   }
 

diff --git a/lib/websocket-server.js b/lib/websocket-server.js
@@ -212,7 +212,6 @@ class WebSocketServer extends EventEmitter {
         ? req.headers['sec-websocket-key']
         : false;
     const version = +req.headers['sec-websocket-version'];
-    const extensions = {};
 
     if (
       req.method !== 'GET' ||
@@ -236,15 +235,21 @@ class WebSocketServer extends EventEmitter {
       }
     }
 
-    if (this.options.perMessageDeflate) {
+    const secWebSocketExtensions = req.headers['sec-websocket-extensions'];
+    const extensions = {};
+
+    if (
+      this.options.perMessageDeflate &&
+      secWebSocketExtensions !== undefined
+    ) {
       const perMessageDeflate = new PerMessageDeflate(
         this.options.perMessageDeflate,
         true,
         this.options.maxPayload
       );
 
       try {
-        const offers = extension.parse(req.headers['sec-websocket-extensions']);
+        const offers = extension.parse(secWebSocketExtensions);
 
         if (offers[PerMessageDeflate.extensionName]) {
           perMessageDeflate.accept(offers[PerMessageDeflate.extensionName]);

diff --git a/lib/websocket.js b/lib/websocket.js
@@ -801,28 +801,25 @@ function initAsClient(websocket, address, protocols, options) {
 
       const extensionNames = Object.keys(extensions);
 
-      if (extensionNames.length) {
-        if (
-          extensionNames.length !== 1 ||
-          extensionNames[0] !== PerMessageDeflate.extensionName
-        ) {
-          const message =
-            'Server indicated an extension that was not requested';
-          abortHandshake(websocket, socket, message);
-          return;
-        }
-
-        try {
-          perMessageDeflate.accept(extensions[PerMessageDeflate.extensionName]);
-        } catch (err) {
-          const message = 'Invalid Sec-WebSocket-Extensions header';
-          abortHandshake(websocket, socket, message);
-          return;
-        }
+      if (
+        extensionNames.length !== 1 ||
+        extensionNames[0] !== PerMessageDeflate.extensionName
+      ) {
+        const message = 'Server indicated an extension that was not requested';
+        abortHandshake(websocket, socket, message);
+        return;
+      }
 
-        websocket._extensions[PerMessageDeflate.extensionName] =
-          perMessageDeflate;
+      try {
+        perMessageDeflate.accept(extensions[PerMessageDeflate.extensionName]);
+      } catch (err) {
+        const message = 'Invalid Sec-WebSocket-Extensions header';
+        abortHandshake(websocket, socket, message);
+        return;
       }
+
+      websocket._extensions[PerMessageDeflate.extensionName] =
+        perMessageDeflate;
     }
 
     websocket.setSocket(socket, head, opts.maxPayload);

diff --git a/test/extension.test.js b/test/extension.test.js
@@ -6,11 +6,6 @@ const { format, parse } = require('../lib/extension');
 
 describe('extension', () => {
   describe('parse', () => {
-    it('returns an empty object if the argument is `undefined`', () => {
-      assert.deepStrictEqual(parse(), { __proto__: null });
-      assert.deepStrictEqual(parse(''), { __proto__: null });
-    });
-
     it('parses a single extension', () => {
       assert.deepStrictEqual(parse('foo'), {
         foo: [{ __proto__: null }],
@@ -73,7 +68,7 @@ describe('extension', () => {
     });
 
     it('ignores the optional white spaces', () => {
-      const header = 'foo; bar\t; \tbaz=1\t ;  bar="1"\t\t, \tqux\t ;norf ';
+      const header = 'foo; bar\t; \tbaz=1\t ;  bar="1"\t\t, \tqux\t ;norf';
 
       assert.deepStrictEqual(parse(header), {
         foo: [{ bar: [true, '1'], baz: ['1'], __proto__: null }],
@@ -105,10 +100,12 @@ describe('extension', () => {
 
     it('throws an error if a white space is misplaced', () => {
       [
+        [' foo', 0],
         ['f oo', 2],
         ['foo;ba r', 7],
         ['foo;bar =', 8],
-        ['foo;bar= ', 8]
+        ['foo;bar= ', 8],
+        ['foo;bar=ba z', 11]
       ].forEach((element) => {
         assert.throws(
           () => parse(element[0]),
@@ -147,13 +144,18 @@ describe('extension', () => {
 
     it('throws an error if the header value ends prematurely', () => {
       [
+        '',
+        'foo ',
+        'foo\t',
         'foo, ',
         'foo;',
+        'foo;bar ',
         'foo;bar,',
         'foo;bar; ',
         'foo;bar=',
         'foo;bar="baz',
-        'foo;bar="1\\'
+        'foo;bar="1\\',
+        'foo;bar="baz" '
       ].forEach((header) => {
         assert.throws(
           () => parse(header),