Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Malformed header bug #34

Merged
merged 6 commits into from
Nov 29, 2021
Merged

Fix Malformed header bug #34

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8a8343e
fix malformed header bug, red phase
Uzlopak Nov 29, 2021
3cef321
fix malformed header bug, green phase
Uzlopak Nov 29, 2021
8c8d6d4
modify test
Uzlopak Nov 29, 2021
fa0fc22
use var instead of let
Uzlopak Nov 29, 2021
c7c2cf6
revert finsh changes in dicer
Uzlopak Nov 29, 2021
9b3bde8
remove timeout
Uzlopak Nov 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 6 additions & 15 deletions deps/dicer/lib/Dicer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ function Dicer(cfg) {
this._dashes = 0;
this._parts = 0;
this._finished = false;
this._realFinish = false;
this._isPreamble = true;
this._justMatched = false;
this._firstWrite = true;
Expand All @@ -54,7 +53,7 @@ function Dicer(cfg) {
inherits(Dicer, WritableStream);

Dicer.prototype.emit = function(ev) {
if (ev === 'finish' && !this._realFinish) {
if (ev === 'finish') {
if (!this._finished) {
var self = this;
process.nextTick(function() {
Expand All @@ -64,15 +63,11 @@ Dicer.prototype.emit = function(ev) {
self._part.emit('error', new Error(type + ' terminated early due to unexpected end of multipart data'));
self._part.push(null);
process.nextTick(function() {
self._realFinish = true;
self.emit('finish');
self._realFinish = false;
WritableStream.prototype.emit.call(self, 'finish');
Uzlopak marked this conversation as resolved.
Show resolved Hide resolved
});
return;
}
self._realFinish = true;
self.emit('finish');
self._realFinish = false;
WritableStream.prototype.emit.call(self, 'finish');
});
}
} else
Expand Down Expand Up @@ -160,9 +155,7 @@ Dicer.prototype._oninfo = function(isMatch, data, start, end) {
this._finished = true;
// no more parts will be added
if (self._parts === 0) {
self._realFinish = true;
self.emit('finish');
self._realFinish = false;
WritableStream.prototype.emit.call(self, 'finish');
}
}
if (this._dashes)
Expand Down Expand Up @@ -190,7 +183,7 @@ Dicer.prototype._oninfo = function(isMatch, data, start, end) {
shouldWriteMore = this._part.push(data.slice(start, end));
if (!shouldWriteMore)
this._pause = true;
} else if (!this._isPreamble && this._inHeader) {
} else {
if (buf)
this._hparser.push(buf);
r = this._hparser.push(data.slice(start, end));
Expand All @@ -207,9 +200,7 @@ Dicer.prototype._oninfo = function(isMatch, data, start, end) {
this._part.on('end', function() {
if (--self._parts === 0) {
if (self._finished) {
self._realFinish = true;
self.emit('finish');
self._realFinish = false;
WritableStream.prototype.emit.call(self, 'finish');
} else {
self._unpause();
}
Expand Down
48 changes: 23 additions & 25 deletions deps/dicer/lib/HeaderParser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ function HeaderParser(cfg) {
this.ss.on('info', function(isMatch, data, start, end) {
if (data && !self.maxed) {
if (self.nread + (end - start) > MAX_HEADER_SIZE) {
end = (MAX_HEADER_SIZE - self.nread);
end = MAX_HEADER_SIZE - self.nread + start;
self.nread = MAX_HEADER_SIZE;
} else
self.nread += (end - start);
Expand Down Expand Up @@ -72,39 +72,37 @@ HeaderParser.prototype._parseHeader = function() {
if (this.npairs === this.maxHeaderPairs)
return;

var lines = this.buffer.split(RE_CRLF), len = lines.length, m, h,
modded = false;
const lines = this.buffer.split(RE_CRLF),
len = lines.length;
let m, h;

for (var i = 0; i < len; ++i) {
for (let i = 0; i < len; ++i) {
Uzlopak marked this conversation as resolved.
Show resolved Hide resolved
if (lines[i].length === 0)
continue;
if (lines[i][0] === '\t' || lines[i][0] === ' ') {
// folded header content
// RFC2822 says to just remove the CRLF and not the whitespace following
// it, so we follow the RFC and include the leading whitespace ...
this.header[h][this.header[h].length - 1] += lines[i];
} else {
m = RE_HDR.exec(lines[i]);
if (m) {
h = m[1].toLowerCase();
if (m[2]) {
if (this.header[h] === undefined)
this.header[h] = [m[2]];
else
this.header[h].push(m[2]);
} else
this.header[h] = [''];
if (++this.npairs === this.maxHeaderPairs)
break;
} else {
this.buffer = lines[i];
modded = true;
break;
if (h) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the intention of this rewrite?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you see a potential regression?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand neither the original code, nor the fix, which is why the change makes me pause. I guess I would need to reread carefully both the original and the change, to fully understand what is happening in both scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.rfc-editor.org/rfc/rfc2616#section-2.2

   HTTP/1.1 header field values can be folded onto multiple lines if the
   continuation line begins with a space or horizontal tab. All linear
   white space, including folding, has the same semantics as SP. A
   recipient MAY replace any linear white space with a single SP before
   interpreting the field value or forwarding the message downstream.

And for this part there is the test "Folded values" in "dicer-headerparser". If it was wrong the test there would fail.

this.header[h][this.header[h].length - 1] += lines[i];
continue;
}
}
m = RE_HDR.exec(lines[i]);
if (m) {
h = m[1].toLowerCase();
if (m[2]) {
if (this.header[h] === undefined)
this.header[h] = [m[2]];
else
this.header[h].push(m[2]);
} else
this.header[h] = [''];
if (++this.npairs === this.maxHeaderPairs)
break;
} else
return;
}
if (!modded)
this.buffer = '';
};

module.exports = HeaderParser;
module.exports = HeaderParser;
25 changes: 25 additions & 0 deletions test/dicer-malformed-header.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
const Dicer = require('../deps/dicer/lib/Dicer');
const { expect } = require('chai');

describe('dicer-malformed-header', () => {

it("should gracefully handle headers with leading whitespace", done => {
var d = new Dicer({ boundary: "----WebKitFormBoundaryoo6vortfDzBsDiro" });

d.on('part', function (p) {
p.on('header', function (header) {
expect(header).has.property(" content-disposition");
expect(header[" content-disposition"]).to.be.eql(['form-data; name="bildbeschreibung"'])
});
p.on('data', function (data) {
});
p.on('end', function () {
});
});
d.on('finish', function () {
done();
});

d.write(Buffer.from('------WebKitFormBoundaryoo6vortfDzBsDiro\r\n Content-Disposition: form-data; name="bildbeschreibung"\r\n\r\n\r\n------WebKitFormBoundaryoo6vortfDzBsDiro--'));
kibertoad marked this conversation as resolved.
Show resolved Hide resolved
}).timeout(10000);
Uzlopak marked this conversation as resolved.
Show resolved Hide resolved
});