133 lines
4.2 KiB
Diff
133 lines
4.2 KiB
Diff
|
commit 2eee90e959ca4abaf53caf238d063c396f2ea17c
|
|||
|
Author: Sam Roberts <vieuxtech@gmail.com>
|
|||
|
Date: Fri Jan 10 15:00:11 2020 -0800
|
|||
|
|
|||
|
http: strip trailing OWS from header values
|
|||
|
|
|||
|
HTTP header values can have trailing OWS, but it should be stripped. It
|
|||
|
is not semantically part of the header's value, and if treated as part
|
|||
|
of the value, it can cause spurious inequality between expected and
|
|||
|
actual header values.
|
|||
|
|
|||
|
Note that a single SPC of leading OWS is common before the field-value,
|
|||
|
and it is already handled by the HTTP parser by stripping all leading
|
|||
|
OWS. It is only the trailing OWS that must be stripped by the parser
|
|||
|
user.
|
|||
|
|
|||
|
header-field = field-name ":" OWS field-value OWS
|
|||
|
; https://tools.ietf.org/html/rfc7230#section-3.2
|
|||
|
OWS = *( SP / HTAB )
|
|||
|
; https://tools.ietf.org/html/rfc7230#section-3.2.3
|
|||
|
|
|||
|
Fixes: https://hackerone.com/reports/730779
|
|||
|
|
|||
|
PR-URL: https://github.com/nodejs-private/node-private/pull/191
|
|||
|
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
|
|||
|
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
|
|||
|
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
|
|||
|
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
|
|||
|
|
|||
|
diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc
|
|||
|
index c2cd7a213b..420e94564e 100644
|
|||
|
--- a/src/node_http_parser.cc
|
|||
|
+++ b/src/node_http_parser.cc
|
|||
|
@@ -74,6 +74,10 @@ const uint32_t kOnMessageComplete = 3;
|
|||
|
const uint32_t kOnExecute = 4;
|
|||
|
|
|||
|
|
|||
|
+inline bool IsOWS(char c) {
|
|||
|
+ return c == ' ' || c == '\t';
|
|||
|
+}
|
|||
|
+
|
|||
|
// helper class for the Parser
|
|||
|
struct StringPtr {
|
|||
|
StringPtr() {
|
|||
|
@@ -133,13 +137,22 @@ struct StringPtr {
|
|||
|
|
|||
|
|
|||
|
Local<String> ToString(Environment* env) const {
|
|||
|
- if (str_)
|
|||
|
+ if (size_ != 0)
|
|||
|
return OneByteString(env->isolate(), str_, size_);
|
|||
|
else
|
|||
|
return String::Empty(env->isolate());
|
|||
|
}
|
|||
|
|
|||
|
|
|||
|
+ // Strip trailing OWS (SPC or HTAB) from string.
|
|||
|
+ Local<String> ToTrimmedString(Environment* env) {
|
|||
|
+ while (size_ > 0 && IsOWS(str_[size_ - 1])) {
|
|||
|
+ size_--;
|
|||
|
+ }
|
|||
|
+ return ToString(env);
|
|||
|
+ }
|
|||
|
+
|
|||
|
+
|
|||
|
const char* str_;
|
|||
|
bool on_heap_;
|
|||
|
size_t size_;
|
|||
|
@@ -669,7 +682,7 @@ class Parser : public AsyncWrap, public StreamListener {
|
|||
|
size_t j = 0;
|
|||
|
while (i < num_values_ && j < arraysize(argv) / 2) {
|
|||
|
argv[j * 2] = fields_[i].ToString(env());
|
|||
|
- argv[j * 2 + 1] = values_[i].ToString(env());
|
|||
|
+ argv[j * 2 + 1] = values_[i].ToTrimmedString(env());
|
|||
|
i++;
|
|||
|
j++;
|
|||
|
}
|
|||
|
diff --git a/test/parallel/test-http-header-owstext.js b/test/parallel/test-http-header-owstext.js
|
|||
|
new file mode 100644
|
|||
|
index 0000000000..bc094137a2
|
|||
|
--- /dev/null
|
|||
|
+++ b/test/parallel/test-http-header-owstext.js
|
|||
|
@@ -0,0 +1,49 @@
|
|||
|
+'use strict';
|
|||
|
+const common = require('../common');
|
|||
|
+
|
|||
|
+// This test ensures that the http-parser strips leading and trailing OWS from
|
|||
|
+// header values. It sends the header values in chunks to force the parser to
|
|||
|
+// build the string up through multiple calls to on_header_value().
|
|||
|
+
|
|||
|
+const assert = require('assert');
|
|||
|
+const http = require('http');
|
|||
|
+const net = require('net');
|
|||
|
+
|
|||
|
+function check(hdr, snd, rcv) {
|
|||
|
+ const server = http.createServer(common.mustCall((req, res) => {
|
|||
|
+ assert.strictEqual(req.headers[hdr], rcv);
|
|||
|
+ req.pipe(res);
|
|||
|
+ }));
|
|||
|
+
|
|||
|
+ server.listen(0, common.mustCall(function() {
|
|||
|
+ const client = net.connect(this.address().port, start);
|
|||
|
+ function start() {
|
|||
|
+ client.write('GET / HTTP/1.1\r\n' + hdr + ':', drain);
|
|||
|
+ }
|
|||
|
+
|
|||
|
+ function drain() {
|
|||
|
+ if (snd.length === 0) {
|
|||
|
+ return client.write('\r\nConnection: close\r\n\r\n');
|
|||
|
+ }
|
|||
|
+ client.write(snd.shift(), drain);
|
|||
|
+ }
|
|||
|
+
|
|||
|
+ const bufs = [];
|
|||
|
+ client.on('data', function(chunk) {
|
|||
|
+ bufs.push(chunk);
|
|||
|
+ });
|
|||
|
+ client.on('end', common.mustCall(function() {
|
|||
|
+ const head = Buffer.concat(bufs)
|
|||
|
+ .toString('latin1')
|
|||
|
+ .split('\r\n')[0];
|
|||
|
+ assert.strictEqual(head, 'HTTP/1.1 200 OK');
|
|||
|
+ server.close();
|
|||
|
+ }));
|
|||
|
+ }));
|
|||
|
+}
|
|||
|
+
|
|||
|
+check('host', [' \t foo.com\t'], 'foo.com');
|
|||
|
+check('host', [' \t foo\tcom\t'], 'foo\tcom');
|
|||
|
+check('host', [' \t', ' ', ' foo.com\t', '\t '], 'foo.com');
|
|||
|
+check('host', [' \t', ' \t'.repeat(100), '\t '], '');
|
|||
|
+check('host', [' \t', ' - - - - ', '\t '], '- - - -');
|