http: stricter Transfer-Encoding and header separator parsing

nodejs · mcollina · Jul 6, 2022 · Jun 22, 2022 · Jul 6, 2022 · Apr 3, 2022
commit 3097f3936bfe09be0e64e2e574c53d64c247fb50
diff --git a/src/llhttp/constants.ts b/src/llhttp/constants.ts
@@ -6,38 +6,40 @@ export type HTTPMode = 'loose' | 'strict';
 
 export enum ERROR {
   OK = 0,
-  INTERNAL,
-  STRICT,
-  LF_EXPECTED,
-  UNEXPECTED_CONTENT_LENGTH,
-  CLOSED_CONNECTION,
-  INVALID_METHOD,
-  INVALID_URL,
-  INVALID_CONSTANT,
-  INVALID_VERSION,
-  INVALID_HEADER_TOKEN,
-  INVALID_CONTENT_LENGTH,
-  INVALID_CHUNK_SIZE,
-  INVALID_STATUS,
-  INVALID_EOF_STATE,
-  INVALID_TRANSFER_ENCODING,
-
-  CB_MESSAGE_BEGIN,
-  CB_HEADERS_COMPLETE,
-  CB_MESSAGE_COMPLETE,
-  CB_CHUNK_HEADER,
-  CB_CHUNK_COMPLETE,
-
-  PAUSED,
-  PAUSED_UPGRADE,
-
-  USER,
+  INTERNAL = 1,
+  STRICT = 2,
+  CR_EXPECTED = 25,
+  LF_EXPECTED = 3,
+  UNEXPECTED_CONTENT_LENGTH = 4,
+  CLOSED_CONNECTION = 5,
+  INVALID_METHOD = 6,
+  INVALID_URL = 7,
+  INVALID_CONSTANT = 8,
+  INVALID_VERSION = 9,
+  INVALID_HEADER_TOKEN = 10,
+  INVALID_CONTENT_LENGTH = 11,
+  INVALID_CHUNK_SIZE = 12,
+  INVALID_STATUS = 13,
+  INVALID_EOF_STATE = 14,
+  INVALID_TRANSFER_ENCODING = 15,
+
+  CB_MESSAGE_BEGIN = 16,
+  CB_HEADERS_COMPLETE = 17,
+  CB_MESSAGE_COMPLETE = 18,
+  CB_CHUNK_HEADER = 19,
+  CB_CHUNK_COMPLETE = 20,
+
+  PAUSED = 21,
+  PAUSED_UPGRADE = 22,
+  // PAUSED_H2_UPGRADE = 23 in v6.x
+
+  USER = 24,
 }
 
 export enum TYPE {
   BOTH = 0,  // default
-  REQUEST,
-  RESPONSE,
+  REQUEST = 1,
+  RESPONSE = 2,
 }
 
 export enum FLAGS {
@@ -111,8 +113,8 @@ Object.keys(METHOD_MAP).forEach((key) => {
 
 export enum FINISH {
   SAFE = 0,
-  SAFE_WITH_CB,
-  UNSAFE,
+  SAFE_WITH_CB = 1,
+  UNSAFE = 2,
 }
 
 // Internal
@@ -208,15 +210,15 @@ export const MINOR = MAJOR;
 
 export enum HEADER_STATE {
   GENERAL = 0,
-  CONNECTION,
-  CONTENT_LENGTH,
-  TRANSFER_ENCODING,
-  UPGRADE,
-
-  CONNECTION_KEEP_ALIVE,
-  CONNECTION_CLOSE,
-  CONNECTION_UPGRADE,
-  TRANSFER_ENCODING_CHUNKED,
+  CONNECTION = 1,
+  CONTENT_LENGTH = 2,
+  TRANSFER_ENCODING = 3,
+  UPGRADE = 4,
+
+  CONNECTION_KEEP_ALIVE = 5,
+  CONNECTION_CLOSE = 6,
+  CONNECTION_UPGRADE = 7,
+  TRANSFER_ENCODING_CHUNKED = 8,
 }
 
 export const SPECIAL_HEADERS = {

diff --git a/src/llhttp/http.ts b/src/llhttp/http.ts
@@ -55,6 +55,7 @@ const NODES: ReadonlyArray<string> = [
   'header_value',
   'header_value_otherwise',
   'header_value_lenient',
+  'header_value_lenient_failed',
   'header_value_lws',
   'header_value_te_chunked',
   'header_value_te_chunked_last',
@@ -433,11 +434,35 @@ export class HTTP {
       .match([ ' ', '\t' ], n('header_value_discard_ws'))
       .otherwise(checkContentLengthEmptiness);
 
+    // Multiple `Transfer-Encoding` headers should be treated as one, but with
+    // values separate by a comma.
+    //
+    // See: https://tools.ietf.org/html/rfc7230#section-3.2.2
+    const toTransferEncoding = this.unsetFlag(
+      FLAGS.CHUNKED,
+      'header_value_te_chunked');
+
+    // Once chunked has been selected, no other encoding is possible in requests
+    // https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.1
+    const forbidAfterChunkedInRequest = (otherwise: Node) => {
+      return this.load('type', {
+        [TYPE.REQUEST]: this.testFlags(FLAGS.LENIENT, {
+          0: span.headerValue.end().skipTo(
+            p.error(ERROR.INVALID_TRANSFER_ENCODING, 'Invalid `Transfer-Encoding` header value'),
+          ),
+        }).otherwise(otherwise),
+      }, otherwise);
+    };
+
     n('header_value_start')
       .otherwise(this.load('header_state', {
         [HEADER_STATE.UPGRADE]: this.setFlag(FLAGS.UPGRADE, fallback),
-        [HEADER_STATE.TRANSFER_ENCODING]: this.setFlag(
-          FLAGS.TRANSFER_ENCODING, 'header_value_te_chunked'),
+        [HEADER_STATE.TRANSFER_ENCODING]: this.testFlags(
+          FLAGS.CHUNKED,
+          {
+            1: forbidAfterChunkedInRequest(this.setFlag(FLAGS.TRANSFER_ENCODING, toTransferEncoding)),
+          },
+          this.setFlag(FLAGS.TRANSFER_ENCODING, toTransferEncoding)),
         [HEADER_STATE.CONTENT_LENGTH]: n('header_value_content_length_once'),
         [HEADER_STATE.CONNECTION]: n('header_value_connection'),
       }, 'header_value'));
@@ -459,7 +484,8 @@ export class HTTP {
       .peek([ '\r', '\n' ], this.update('header_state',
         HEADER_STATE.TRANSFER_ENCODING_CHUNKED,
         'header_value_otherwise'))
-      .otherwise(n('header_value_te_chunked'));
+      .peek(',', forbidAfterChunkedInRequest(n('header_value_te_chunked')))
+      .otherwise(n('header_value_te_token'));
 
     n('header_value_te_token')
       .match(',', n('header_value_te_token_ows'))
@@ -544,18 +570,23 @@ export class HTTP {
 
     const checkLenient = this.testFlags(FLAGS.LENIENT, {
       1: n('header_value_lenient'),
-    }, p.error(ERROR.INVALID_HEADER_TOKEN, 'Invalid header value char'));
+    }, n('header_value_lenient_failed'));
 
     n('header_value_otherwise')
       .peek('\r', span.headerValue.end().skipTo(n('header_value_almost_done')))
-      .peek('\n', span.headerValue.end(n('header_value_almost_done')))
       .otherwise(checkLenient);
 
     n('header_value_lenient')
       .peek('\r', span.headerValue.end().skipTo(n('header_value_almost_done')))
       .peek('\n', span.headerValue.end(n('header_value_almost_done')))
       .skipTo(n('header_value_lenient'));
 
+    n('header_value_lenient_failed')
+      .peek('\n', span.headerValue.end().skipTo(
+        p.error(ERROR.CR_EXPECTED, 'Missing expected CR after header value')),
+      )
+      .otherwise(p.error(ERROR.INVALID_HEADER_TOKEN, 'Invalid header value char'));
+
     n('header_value_almost_done')
       .match('\n', n('header_value_lws'))
       .otherwise(p.error(ERROR.LF_EXPECTED,
@@ -833,6 +864,11 @@ export class HTTP {
     return p.invoke(p.code.or('flags', flag), this.node(next));
   }
 
+  private unsetFlag(flag: FLAGS, next: string | Node): Node {
+    const p = this.llparse;
+    return p.invoke(p.code.and('flags', ~flag), this.node(next));
+  }
+
   private testFlags(flag: FLAGS, map: { [key: number]: Node },
                     next?: string | Node): Node {
     const p = this.llparse;

diff --git a/test/request/invalid.md b/test/request/invalid.md
@@ -153,3 +153,26 @@ off=16 len=4 span[header_field]="Host"
 off=22 len=15 span[header_value]="www.example.com"
 off=52 error code=10 reason="Invalid header token"
 ```
+
+### Missing CR between headers
+
+<!-- meta={"type": "request", "noScan": true} -->
+
+```http
+GET / HTTP/1.1
+Host: localhost
+Dummy: x\nContent-Length: 23
+GET / HTTP/1.1
+Dummy: GET /admin HTTP/1.1
+Host: localhost
+```
+
+```log
+off=0 message begin
+off=4 len=1 span[url]="/"
+off=16 len=4 span[header_field]="Host"
+off=22 len=9 span[header_value]="localhost"
+off=33 len=5 span[header_field]="Dummy"
+off=40 len=1 span[header_value]="x"
+off=42 error code=25 reason="Missing expected CR after header value"
+```
diff --git a/test/request/sample.md b/test/request/sample.md
@@ -298,21 +298,7 @@ off=0 message begin
 off=4 len=1 span[url]="/"
 off=15 len=5 span[header_field]="Line1"
 off=24 len=3 span[header_value]="abc"
-off=28 len=4 span[header_value]="\tdef"
-off=33 len=4 span[header_value]=" ghi"
-off=38 len=5 span[header_value]="\t\tjkl"
-off=44 len=6 span[header_value]="  mno "
-off=51 len=6 span[header_value]="\t \tqrs"
-off=58 len=5 span[header_field]="Line2"
-off=67 len=6 span[header_value]="line2\t"
-off=74 len=5 span[header_field]="Line3"
-off=82 len=5 span[header_value]="line3"
-off=88 len=5 span[header_field]="Line4"
-off=98 len=0 span[header_value]=""
-off=98 len=10 span[header_field]="Connection"
-off=111 len=5 span[header_value]="close"
-off=118 headers complete method=1 v=1/1 flags=2 content_length=0
-off=118 message complete
+off=28 error code=25 reason="Missing expected CR after header value"
 ```
 
 ## Request starting with CRLF