Skip to content

Commit

Permalink
fix: Do not allow empty header separated by CR. (#227)
Browse files Browse the repository at this point in the history
  • Loading branch information
ShogunPanda committed Jun 21, 2023
1 parent b5f822a commit 7e18596
Show file tree
Hide file tree
Showing 7 changed files with 105 additions and 13 deletions.
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ release: clean generate
cp -rf src/llhttp.gyp release/
cp -rf src/common.gypi release/
cp -rf CMakeLists.txt release/
sed -i s/_RELEASE_/$(RELEASE)/ release/CMakeLists.txt

This comment has been minimized.

Copy link
@musicinmybrain

musicinmybrain Jun 22, 2023

Contributor

What was the purpose of this change? It seems to break the Makefile by causing sed to treat the expression s/_RELEASE_/$(RELEASE)/ as an input filename.

This comment has been minimized.

Copy link
@ShogunPanda

ShogunPanda Jun 26, 2023

Author Contributor

It was not working for me when releasing from MacOS.
Isn't that working for you now? Which platform are you on?

This comment has been minimized.

Copy link
@musicinmybrain

musicinmybrain Jun 27, 2023

Contributor

It was not working for me when releasing from MacOS. Isn't that working for you now? Which platform are you on?

Ah, that makes sense. You have BSD-sed, which requires an argument for -i, and I have GNU-sed on Fedora Linux, which doesn’t. My MacOS machine is still packed up after a recent trip, but based on some quick research, the only portable approach for in-place editing across MacOS and GNU/Linux appears to be passing a non-empty backup file suffix and allowing a backup file to be created.

This comment has been minimized.

Copy link
@ShogunPanda

ShogunPanda Jun 27, 2023

Author Contributor

Well, we might do this as well. Do you mind sending a PR, making sure the backup file is in gitignore?

This comment has been minimized.

Copy link
@musicinmybrain

musicinmybrain Jun 27, 2023

Contributor

I offered #230, which sidesteps the issue by not doing an in-place sed at all.

This comment has been minimized.

Copy link
@ShogunPanda

ShogunPanda Jul 3, 2023

Author Contributor

Just merged it. Thanks for the brilliant idea!

sed -i '' s/_RELEASE_/$(RELEASE)/ release/CMakeLists.txt
cp -rf libllhttp.pc.in release/
cp -rf README.md release/
cp -rf LICENSE-MIT release/
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "llhttp",
"version": "8.1.0",
"version": "8.1.1",
"description": "HTTP parser in LLVM IR",
"main": "lib/llhttp.js",
"types": "lib/llhttp.d.ts",
Expand Down
11 changes: 10 additions & 1 deletion src/llhttp.gyp
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
{
'variables': {
'llhttp_sources': [
'src/llhttp.c',
'src/api.c',
'src/http.c',
]
},
'targets': [
{
'target_name': 'llhttp',
Expand All @@ -7,7 +14,9 @@
'direct_dependent_settings': {
'include_dirs': [ 'include' ],
},
'sources': [ 'src/llhttp.c', 'src/api.c', 'src/http.c' ],
'sources': [
'<@(llhttp_sources)',
],
},
]
}
19 changes: 10 additions & 9 deletions src/llhttp/http.ts
Original file line number Diff line number Diff line change
Expand Up @@ -572,14 +572,13 @@ export class HTTP {
}, p.error(ERROR.INVALID_HEADER_TOKEN, 'Invalid header value char')))
.otherwise(span.headerValue.start(n('header_value_start')));

if (this.mode === 'strict') {
n('header_value_discard_ws_almost_done')
.match('\n', n('header_value_discard_lws'))
.otherwise(p.error(ERROR.STRICT, 'Expected LF after CR'));
} else {
n('header_value_discard_ws_almost_done').skipTo(
n('header_value_discard_lws'));
}
n('header_value_discard_ws_almost_done')
.match('\n', n('header_value_discard_lws'))
.otherwise(
this.testLenientFlags(LENIENT_FLAGS.HEADERS, {
1: n('header_value_discard_lws'),
}, p.error(ERROR.STRICT, 'Expected LF after CR')),
);

const onHeaderValueComplete = this.invokePausable(
'on_header_value_complete', ERROR.CB_HEADER_VALUE_COMPLETE, n('header_field_start'),
Expand All @@ -593,7 +592,9 @@ export class HTTP {
this.emptySpan(span.headerValue, onHeaderValueComplete)));

n('header_value_discard_lws')
.match([ ' ', '\t' ], n('header_value_discard_ws'))
.match([ ' ', '\t' ], this.testLenientFlags(LENIENT_FLAGS.HEADERS, {
1: n('header_value_discard_ws'),
}, p.error(ERROR.INVALID_HEADER_TOKEN, 'Invalid header value char')))
.otherwise(checkContentLengthEmptiness);

// Multiple `Transfer-Encoding` headers should be treated as one, but with
Expand Down
36 changes: 36 additions & 0 deletions test/request/invalid.md
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,42 @@ off=41 len=1 span[header_value]="x"
off=42 error code=10 reason="Invalid header value char"
```

### Empty headers separated by CR

<!-- meta={"type": "request" } -->
```http
POST / HTTP/1.1
Connection: Close
Host: localhost:5000
x:\rTransfer-Encoding: chunked
1
A
0
```

```log
off=0 message begin
off=0 len=4 span[method]="POST"
off=4 method complete
off=5 len=1 span[url]="/"
off=7 url complete
off=12 len=3 span[version]="1.1"
off=15 version complete
off=17 len=10 span[header_field]="Connection"
off=28 header_field complete
off=29 len=5 span[header_value]="Close"
off=36 header_value complete
off=36 len=4 span[header_field]="Host"
off=41 header_field complete
off=42 len=14 span[header_value]="localhost:5000"
off=58 header_value complete
off=58 len=1 span[header_field]="x"
off=60 header_field complete
off=61 error code=2 reason="Expected LF after CR"
```

### Empty headers separated by LF

<!-- meta={"type": "request"} -->
Expand Down
46 changes: 46 additions & 0 deletions test/request/lenient-headers.md
Original file line number Diff line number Diff line change
Expand Up @@ -97,3 +97,49 @@ off=27 header_field complete
off=28 len=0 span[header_value]=""
off=28 error code=10 reason="Invalid header value char"
```

### Empty headers separated by CR (Lenient)

<!-- meta={"type": "request-lenient-headers"} -->
```http
POST / HTTP/1.1
Connection: Close
Host: localhost:5000
x:\rTransfer-Encoding: chunked
1
A
0
```

```log
off=0 message begin
off=0 len=4 span[method]="POST"
off=4 method complete
off=5 len=1 span[url]="/"
off=7 url complete
off=12 len=3 span[version]="1.1"
off=15 version complete
off=17 len=10 span[header_field]="Connection"
off=28 header_field complete
off=29 len=5 span[header_value]="Close"
off=36 header_value complete
off=36 len=4 span[header_field]="Host"
off=41 header_field complete
off=42 len=14 span[header_value]="localhost:5000"
off=58 header_value complete
off=58 len=1 span[header_field]="x"
off=60 header_field complete
off=61 len=0 span[header_value]=""
off=61 header_value complete
off=61 len=17 span[header_field]="Transfer-Encoding"
off=79 header_field complete
off=80 len=7 span[header_value]="chunked"
off=89 header_value complete
off=91 headers complete method=3 v=1/1 flags=20a content_length=0
off=94 chunk header len=1
off=94 len=1 span[body]="A"
off=97 chunk complete
off=100 chunk header len=0
```
2 changes: 1 addition & 1 deletion test/request/sample.md
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ off=9 message complete

## Line folding in header value with CRLF

<!-- meta={"type": "request"} -->
<!-- meta={"type": "request-lenient-headers"} -->
```http
GET / HTTP/1.1
Line1: abc
Expand Down

0 comments on commit 7e18596

Please sign in to comment.