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

fix: define conditions when content-length should be sent #2305

Merged
merged 7 commits into from
Oct 26, 2023
Merged
21 changes: 20 additions & 1 deletion lib/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -1495,6 +1495,23 @@ function _resume (client, sync) {
}
}

function shouldSendContentLength (contentLength, method) {
if (contentLength > 0) {
return true
}
// Many servers expect a Content-Length for these methods
if (method === 'POST' || method === 'PUT' || method === 'PATCH') {
return true
}
if (contentLength === 0) {
if (method === 'GET' || method === 'HEAD') {
return false
}
return true
}
return false
metcoder95 marked this conversation as resolved.
Show resolved Hide resolved
}

function write (client, request) {
if (client[kHTTPConnVersion] === 'h2') {
writeH2(client, client[kHTTP2Session], request)
Expand Down Expand Up @@ -1538,7 +1555,9 @@ function write (client, request) {
contentLength = null
}

if (request.contentLength !== null && request.contentLength !== contentLength) {
// https://github.com/nodejs/undici/issues/2046
// A user agent may send a Content-Length header with 0 value, this should be allowed.
if (shouldSendContentLength(contentLength, method) && request.contentLength !== null && request.contentLength !== contentLength) {
Copy link
Member

Choose a reason for hiding this comment

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

Following previous comment, it is better if we offload the contentLength to the if statement instead. In that sense if contentLength is greater than 0, and the method has explicit semantics to handle the request's body; we attach it. Otherwise, we just ignore it.

if (client[kStrictContentLength]) {
errorRequest(client, request, new RequestContentLengthMismatchError())
return false
Expand Down
93 changes: 72 additions & 21 deletions test/content-length.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ const { Readable } = require('stream')
const { maybeWrapStream, consts } = require('./utils/async-iterators')

test('request invalid content-length', (t) => {
t.plan(10)
t.plan(8)

const server = createServer((req, res) => {
res.end()
Expand Down Expand Up @@ -61,26 +61,6 @@ test('request invalid content-length', (t) => {
t.type(err, errors.RequestContentLengthMismatchError)
})

client.request({
path: '/',
method: 'HEAD',
headers: {
'content-length': 10
}
}, (err, data) => {
t.type(err, errors.RequestContentLengthMismatchError)
})

client.request({
path: '/',
method: 'GET',
headers: {
'content-length': 0
}
}, (err, data) => {
t.type(err, errors.RequestContentLengthMismatchError)
})

client.request({
path: '/',
method: 'GET',
Expand Down Expand Up @@ -329,3 +309,74 @@ test('request streaming with Readable.from(buf)', (t) => {
})
})
})

test('request DELETE and content-length=0', (t) => {
t.plan(2)
const server = createServer((req, res) => {
res.end()
metcoder95 marked this conversation as resolved.
Show resolved Hide resolved
})
t.teardown(server.close.bind(server))
server.listen(0, () => {
const client = new Client(`http://localhost:${server.address().port}`)
t.teardown(client.destroy.bind(client))

client.request({
path: '/',
method: 'DELETE',
headers: {
'content-length': 0
}
}, (err) => {
t.error(err)
})
client.on('disconnect', () => {
t.pass()
})
})
})

test('content-length shouldSendContentLength=false', (t) => {
t.plan(5)
const server = createServer((req, res) => {
res.end()
metcoder95 marked this conversation as resolved.
Show resolved Hide resolved
})
t.teardown(server.close.bind(server))
server.listen(0, () => {
const client = new Client(`http://localhost:${server.address().port}`)
t.teardown(client.destroy.bind(client))

client.request({
path: '/',
method: 'HEAD',
headers: {
'content-length': 10
}
}, (err) => {
t.error(err)
})

client.request({
path: '/',
method: 'GET',
headers: {
'content-length': 0
}
}, (err) => {
t.error(err)
})

client.request({
path: '/',
method: 'DELETE',
headers: {
'content-length': 4
}
}, (err) => {
t.error(err)
})

client.on('disconnect', () => {
t.pass()
})
})
})