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.

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

Allow fully qualified multipart content-type header #1423

Merged
merged 1 commit into from Feb 18, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 13 additions & 8 deletions lib/multipart.js
Expand Up @@ -21,14 +21,14 @@ Multipart.prototype.isChunked = function (options) {
throw new Error('Argument error, options.multipart.')
}

if (self.request.getHeader('transfer-encoding') === 'chunked') {
chunked = true
}

if (options.chunked !== undefined) {
chunked = options.chunked
}

if (self.request.getHeader('transfer-encoding') === 'chunked') {
chunked = true
}

if (!chunked) {
parts.forEach(function (part) {
if(typeof part.body === 'undefined') {
Expand All @@ -51,11 +51,16 @@ Multipart.prototype.setHeaders = function (chunked) {
}

var header = self.request.getHeader('content-type')
var contentType = (!header || header.indexOf('multipart') === -1)
? 'multipart/related'
: header.split(';')[0]

self.request.setHeader('content-type', contentType + '; boundary=' + self.boundary)
if (!header || header.indexOf('multipart') === -1) {
self.request.setHeader('content-type', 'multipart/related; boundary=' + self.boundary)
} else {
if (header.indexOf('boundary') !== -1) {
self.boundary = header.replace(/.*boundary=([^\s;])+.*/, '$1')
Copy link

Choose a reason for hiding this comment

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

([^\s;])+ picks the first character of the boundary, should be ([^\s;]+)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing that out @assaf the bug was fixed here #1505 I had a faulty test for that case.

} else {
self.request.setHeader('content-type', header + '; boundary=' + self.boundary)
}
}
}

Multipart.prototype.build = function (parts, chunked) {
Expand Down
149 changes: 149 additions & 0 deletions tests/test-multipart-encoding.js
@@ -0,0 +1,149 @@
'use strict'

var http = require('http')
, path = require('path')
, request = require('../index')
, fs = require('fs')
, tape = require('tape')


var localFile = path.join(__dirname, 'unicycle.jpg')
var cases = {
// based on body type
'+array -stream': {
options: {
multipart: [{name: 'field', body: 'value'}]
},
expected: {chunked: false}
},
'+array +stream': {
options: {
multipart: [{name: 'file', body: null}]
},
expected: {chunked: true}
},
// encoding overrides body value
'+array +encoding': {
options: {
headers: {'transfer-encoding': 'chunked'},
multipart: [{name: 'field', body: 'value'}]
},
expected: {chunked: true}
},

// based on body type
'+object -stream': {
options: {
multipart: {data: [{name: 'field', body: 'value'}]}
},
expected: {chunked: false}
},
'+object +stream': {
options: {
multipart: {data: [{name: 'file', body: null}]}
},
expected: {chunked: true}
},
// encoding overrides body value
'+object +encoding': {
options: {
headers: {'transfer-encoding': 'chunked'},
multipart: {data: [{name: 'field', body: 'value'}]}
},
expected: {chunked: true}
},

// based on body type
'+object -chunked -stream': {
options: {
multipart: {chunked: false, data: [{name: 'field', body: 'value'}]}
},
expected: {chunked: false}
},
'+object -chunked +stream': {
options: {
multipart: {chunked: false, data: [{name: 'file', body: null}]}
},
expected: {chunked: true}
},
// chunked overrides body value
'+object +chunked -stream': {
options: {
multipart: {chunked: true, data: [{name: 'field', body: 'value'}]}
},
expected: {chunked: true}
},
// encoding overrides chunked
'+object +encoding -chunked': {
options: {
headers: {'transfer-encoding': 'chunked'},
multipart: {chunked: false, data: [{name: 'field', body: 'value'}]}
},
expected: {chunked: true}
}
}

function runTest(t, test) {

var server = http.createServer(function(req, res) {

t.ok(req.headers['content-type'].match(/^multipart\/related; boundary=[^\s;]+$/))

if (test.expected.chunked) {
t.ok(req.headers['transfer-encoding'] === 'chunked')
t.notOk(req.headers['content-length'])
} else {
t.ok(req.headers['content-length'])
t.notOk(req.headers['transfer-encoding'])
}

// temp workaround
var data = ''
req.setEncoding('utf8')

req.on('data', function(d) {
data += d
})

req.on('end', function() {
// check for the fields traces
if (test.expected.chunked && data.indexOf('name: file') !== -1) {
// file
t.ok(data.indexOf('name: file') !== -1)
// check for unicycle.jpg traces
t.ok(data.indexOf('2005:06:21 01:44:12') !== -1)
} else {
// field
t.ok(data.indexOf('name: field') !== -1)
var parts = test.options.multipart.data || test.options.multipart
t.ok(data.indexOf(parts[0].body) !== -1)
}

res.writeHead(200)
res.end()
})
})

server.listen(6767, function() {
// @NOTE: multipartData properties must be set here
// so that file read stream does not leak in node v0.8
var parts = test.options.multipart.data || test.options.multipart
if (parts[0].name === 'file') {
parts[0].body = fs.createReadStream(localFile)
}

request.post('http://localhost:6767', test.options, function (err, res, body) {
t.equal(err, null)
t.equal(res.statusCode, 200)
server.close(function () {
t.end()
})
})
})
}

Object.keys(cases).forEach(function (name) {
tape('multipart-encoding ' + name, function(t) {
runTest(t, cases[name])
})
})
143 changes: 48 additions & 95 deletions tests/test-multipart.js
Expand Up @@ -6,11 +6,11 @@ var http = require('http')
, fs = require('fs')
, tape = require('tape')


function runTest(t, a) {
var remoteFile = path.join(__dirname, 'googledoodle.jpg')
, localFile = path.join(__dirname, 'unicycle.jpg')
, multipartData = []
, chunked = a.stream || a.chunked || a.encoding

var server = http.createServer(function(req, res) {
if (req.url === '/file') {
Expand All @@ -19,18 +19,15 @@ function runTest(t, a) {
return
}

if (a.mixed) {
t.ok(req.headers['content-type'].match(/multipart\/mixed/))
} else {
t.ok(req.headers['content-type'].match(/multipart\/related/))
}

if (chunked) {
t.ok(req.headers['transfer-encoding'] === 'chunked')
t.notOk(req.headers['content-length'])
if (a.header) {
if (a.header.indexOf('mixed') !== -1) {
t.ok(req.headers['content-type'].match(/^multipart\/mixed; boundary=[^\s;]+$/))
} else {
t.ok(req.headers['content-type']
.match(/^multipart\/related; boundary=XXX; type=text\/xml; start="<root>"$/))
}
} else {
t.ok(req.headers['content-length'])
t.notOk(req.headers['transfer-encoding'])
t.ok(req.headers['content-type'].match(/^multipart\/related; boundary=[^\s;]+$/))
}

// temp workaround
Expand All @@ -42,26 +39,28 @@ function runTest(t, a) {
})

req.on('end', function() {
// check for the fields' traces
// check for the fields traces

// 1st field : my_field
t.ok( data.indexOf('name: my_field') !== -1 )
t.ok( data.indexOf(multipartData[0].body) !== -1 )
t.ok(data.indexOf('name: my_field') !== -1)
t.ok(data.indexOf(multipartData[0].body) !== -1)

// 2nd field : my_buffer
t.ok( data.indexOf('name: my_buffer') !== -1 )
t.ok( data.indexOf(multipartData[1].body) !== -1 )

if (chunked) {
// 3rd field : my_file
t.ok( data.indexOf('name: my_file') !== -1 )
// check for unicycle.jpg traces
t.ok( data.indexOf('2005:06:21 01:44:12') !== -1 )

// 4th field : remote_file
t.ok( data.indexOf('name: remote_file') !== -1 )
// check for http://localhost:6767/file traces
t.ok( data.indexOf('Photoshop ICC') !== -1 )
t.ok(data.indexOf('name: my_buffer') !== -1)
t.ok(data.indexOf(multipartData[1].body) !== -1)

// 3rd field : my_file
t.ok(data.indexOf('name: my_file') !== -1)
// check for unicycle.jpg traces
t.ok(data.indexOf('2005:06:21 01:44:12') !== -1)

// 4th field : remote_file
t.ok(data.indexOf('name: remote_file') !== -1)
// check for http://localhost:6767/file traces
t.ok(data.indexOf('Photoshop ICC') !== -1)

if (a.header && a.header.indexOf('mixed') !== -1) {
t.ok(data.indexOf('boundary=XXX'))
}

res.writeHead(200)
Expand All @@ -72,33 +71,21 @@ function runTest(t, a) {
server.listen(6767, function() {

// @NOTE: multipartData properties must be set here so that my_file read stream does not leak in node v0.8
multipartData = chunked
? [
multipartData = [
{name: 'my_field', body: 'my_value'},
{name: 'my_buffer', body: new Buffer([1, 2, 3])},
{name: 'my_file', body: fs.createReadStream(localFile)},
{name: 'remote_file', body: request('http://localhost:6767/file')}
]
: [
{name: 'my_field', body: 'my_value'},
{name: 'my_buffer', body: new Buffer([1, 2, 3])}
]

var reqOptions = {
url: 'http://localhost:6767/upload',
headers: (function () {
var headers = {}
if (a.mixed) {
headers['content-type'] = 'multipart/mixed'
}
if (a.encoding) {
headers['transfer-encoding'] = 'chunked'
}
return headers
}()),
multipart: a.array
? multipartData
: {chunked: a.chunked, data: multipartData}
multipart: multipartData
}
if (a.header) {
reqOptions.headers = {
'content-type': a.header
}
}
if (a.json) {
reqOptions.json = true
Expand All @@ -111,60 +98,26 @@ function runTest(t, a) {
t.end()
})
})

})
}

// array - multipart option is array
// object - multipart option is object
// encoding - headers option have transfer-encoding set to chunked
// mixed - headers option have content-type set to something different than multipart/related
// json - json option
// stream - body contains streams or not
// chunked - chunked is set when multipart is object

// var methods = ['post', 'get']
var cases = [
// based on body type
{name: '+array -stream', args: {array: true, encoding: false, stream: false}},
{name: '+array +stream', args: {array: true, encoding: false, stream: true}},
// encoding overrides stream
{name: '+array +encoding', args: {array: true, encoding: true, stream: false}},

// based on body type
{name: '+object -stream', args: {object: true, encoding: false, stream: false}},
{name: '+object +stream', args: {object: true, encoding: false, stream: true}},
// encoding overrides stream
{name: '+object +encoding', args: {object: true, encoding: true, stream: false}},

// based on body type
{name: '+object -chunked -stream', args: {object: true, encoding: false, chunked: false, stream: false}},
{name: '+object -chunked +stream', args: {object: true, encoding: false, chunked: false, stream: true}},
// chunked overrides stream
{name: '+object +chunked -stream', args: {object: true, encoding: false, chunked: true, stream: false}},
// chunked overrides encoding
{name: '+object +encoding -chunked', args: {object: true, encoding: true, chunked: false, stream: false}},
// stream overrides chunked
{name: '+object +encoding -chunked +stream', args: {object: true, encoding: true, chunked: false, stream: true}}
var testHeaders = [
null,
'multipart/mixed',
'multipart/related; boundary=XXX; type=text/xml; start="<root>"'
]

var suite = ['post', 'get'].forEach(function(method) {
[true, false].forEach(function(json) {
[true, false].forEach(function(mixed) {
cases.forEach(function (test) {
var name = [
'multipart related', method,
(json ? '+' : '-') + 'json',
(mixed ? '+' : '-') + 'mixed',
test.name
].join(' ')

tape(name, function(t) {
test.args.method = method
test.args.json = json
test.args.mixed = mixed
runTest(t, test.args)
})
testHeaders.forEach(function(header) {
[true, false].forEach(function(json) {
var name = [
'multipart-related', method.toUpperCase(),
(header || 'default'),
(json ? '+' : '-') + 'json'
].join(' ')

tape(name, function(t) {
runTest(t, {method: method, header: header, json: json})
})
})
})
Expand Down