Node 4.3.1 or higer is required for proper multipart parse #32
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Until Node commit nodejs/node#4738 Buffer.byteLength function did not work correctly. (This commit was entered in Node 4.3.1 https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V4.md#4.3.1) On hapijs/pez project, the commit hapijs/pez@9f0042b#diff-6d186b954a58d5bb740f73d84fe39073R162 was depending on Buffer.byteLength to work correctly. This resulted in a misbehaving multipart parsing when the payload maxBytes is close to the uploaded binary file (Inside the multipart form post payload). Meaning that Subtext library parsing a multipart payload with a binary file that its content-length is smaller than the maxBytes parameter but is close enough than the inner hapijs/pez libary will count mistakenly for a bigger buffer size and will return an error "Maximum size exceeded" (Subtext will return "Invalid multipart payload format"). I wrote a little program that will help you understand the described scenario. Notice: 1. You need to put a binary file inside the project for the program to work. 2. In my example the file was 28MB and the limit I entered was 30MB. Package.json: { "name": "subtext_example", "version": "1.0.0", "description": "", "main": "index.js", "scripts": { "test": "echo \"Error: no test specified\" && exit 1" }, "author": "", "license": "ISC", "dependencies": { "hapi": "^13.4.1", "restler": "^3.4.0", "subtext": "^4.0.4" } } Index.json 'use strict'; const Hapi = require('hapi'); const Subtext = require('subtext'); const Restler = require('restler'); const Fs = require('fs'); const Path = require('path'); const server = new Hapi.Server(); server.connection({ port: 3000 }); var demoFilePath = Path.join(__dirname,"binary_file.exe"); var demoFileMimeType = "application/x-msdownload"; var postUrl = "http://localhost:3000/test_subtext"; var payloadMaxBytes = (30 * 1024 * 1024); server.route({ method: 'POST', path: '/test_subtext', config : { payload : { output: 'stream', parse : false, timeout : false, maxBytes : (1024 * 1024 * 1024) } }, handler: function (request, reply) { var subtextParseOptions = { output: 'file', parse : true, timeout : false, maxBytes : payloadMaxBytes }; const onParsed = (err, parsed) => { request.mime = parsed.mime; request.payload = parsed.payload || null; if (!err) { return reply(); } const failAction = request.route.settings.payload.failAction; // failAction: 'error', 'log', 'ignore' if (failAction !== 'ignore') { request._log(['payload', 'error'], err); } if (failAction === 'error') { return reply(err); } return reply(); }; request._isPayloadPending = true; Subtext.parse(request.raw.req, request._tap(), subtextParseOptions, (err, parsed) => { if (!err || !request._isPayloadPending) { request._isPayloadPending = false; return onParsed(err, parsed); } // Flush out any pending request payload not consumed due to errors const stream = request.raw.req; const read = () => { stream.read(); }; const end = () => { stream.removeListener('readable', read); stream.removeListener('error', end); stream.removeListener('end', end); request._isPayloadPending = false; return onParsed(err, parsed); }; stream.on('readable', read); stream.once('error', end); stream.once('end', end); }); } }); server.start((err) => { Fs.stat(demoFilePath, function(err, stats) { Restler.post(postUrl, { multipart: true, data: { "firstKey": "firstValue", "filename": Restler.file(demoFilePath, null, stats.size, null, demoFileMimeType) } }).on("complete", function(data) { console.log('Completed: ' + JSON.stringify(data)); }); }); });
|
Hi @shugigo - sincere apologies, I completely missed this issue somehow. I will try review everything over the weekend - thanks for raising this. |
|
Hi @johnbrett, I'm available for any question. Thanks! |
|
@hueniverse - have you any thoughts on this? hapi then wouldn't be able to support Node < 4.3.1 either right? |
|
We only support latest 4.x |
|
Hi @shugigo - apologies for the delay in response to this, and thanks for the detailed investigation. This took some time as it was an awkward one to make a decision on, as bumping the engine field would need to be done in After some thought and discussion, I think the best path here is just to put a warning in the README alerting users to this node bug, as really this isn't an issue that will affect everyone and doesn't warrant the overhead of build/install warnings and breaking changes in each module. |
|
Closed via 511758f |
johnbrett
added
documentation
|
This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Until Node commit nodejs/node#4738 Buffer.byteLength function did not work correctly. (This commit was entered in Node 4.3.1 https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V4.md#4.3.1)
On hapijs/pez project, the commit hapijs/pez@9f0042b#diff-6d186b954a58d5bb740f73d84fe39073R162 was depending on Buffer.byteLength to work correctly.
This resulted in a misbehaving multipart parsing when the payload maxBytes is close to the uploaded binary file (Inside the multipart form post payload).
Meaning that Subtext library parsing a multipart payload with a binary file that its content-length is smaller than the maxBytes parameter but is close enough that the inner hapijs/pez libary will count mistakenly for a bigger buffer size and will return an error "Maximum size exceeded" https://github.com/hapijs/pez/blob/master/lib/index.js#L163 (Subtext will return "Invalid multipart payload format" https://github.com/hapijs/subtext/blob/master/lib/index.js#L301).
I attached a little program that will help you understand the described scenario. (example.zip)
Notice:
example.zip