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

fix: return content length mismatch error #742

Closed
wants to merge 1 commit into from
Closed

fix: return content length mismatch error #742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 16 additions & 12 deletions docs/api/Errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,19 @@ You can find all the error objects inside the `errors` key.
const { errors } = require('undici')
```

| Error | Error Codes | Description |
| -----------------------------|-----------------------------------|------------------------------------------------|
| `InvalidArgumentError` | `UND_ERR_INVALID_ARG` | passed an invalid argument. |
| `InvalidReturnValueError` | `UND_ERR_INVALID_RETURN_VALUE` | returned an invalid value. |
| `RequestAbortedError` | `UND_ERR_ABORTED` | the request has been aborted by the user |
| `ClientDestroyedError` | `UND_ERR_DESTROYED` | trying to use a destroyed client. |
| `ClientClosedError` | `UND_ERR_CLOSED` | trying to use a closed client. |
| `SocketError` | `UND_ERR_SOCKET` | there is an error with the socket. |
| `NotSupportedError` | `UND_ERR_NOT_SUPPORTED` | encountered unsupported functionality. |
| `ContentLengthMismatchError` | `UND_ERR_CONTENT_LENGTH_MISMATCH`| body does not match content-length header |
| `InformationalError` | `UND_ERR_INFO` | expected error with reason |
| `TrailerMismatchError` | `UND_ERR_TRAILER_MISMATCH` | trailers did not match specification |
| Error | Error Codes | Description |
| -----------------------------|--------------------------------------------|-------------------------------------------------------|
| `InvalidArgumentError` | `UND_ERR_INVALID_ARG` | passed an invalid argument. |
| `InvalidReturnValueError` | `UND_ERR_INVALID_RETURN_VALUE` | returned an invalid value. |
| `RequestAbortedError` | `UND_ERR_ABORTED` | the request has been aborted by the user |
| `ClientDestroyedError` | `UND_ERR_DESTROYED` | trying to use a destroyed client. |
| `ClientClosedError` | `UND_ERR_CLOSED` | trying to use a closed client. |
| `SocketError` | `UND_ERR_SOCKET` | there is an error with the socket. |
| `NotSupportedError` | `UND_ERR_NOT_SUPPORTED` | encountered unsupported functionality. |
| `RequestContentLengthError` | `UND_ERR_REQUEST_CONTENT_LENGTH_MISMATCH` | body does not match content-length header in request |
| `ResponseContentLengthError` | `UND_ERR_RESPONSE_CONTENT_LENGTH_MISMATCH`| body does not match content-length header in response * |
| `InformationalError` | `UND_ERR_INFO` | expected error with reason |
| `TrailerMismatchError` | `UND_ERR_TRAILER_MISMATCH` | trailers did not match specification |

\* The `UND_ERR_RESPONSE_CONTENT_LENGTH_MISMATCH` is returned when the received body is less than the value in the received header `Content-Length`.
If the server try to send a body larger than `Content-Length`, Undici closes the connection when the length is received and return an `UND_ERR_INFO`, to prevent a cache poisoning attack.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please note that at https://github.com/nodejs/undici/blob/e2a9ddc07d9e0ff6be25d243523b53d1d71a3e9e/deps/llhttp/include/llhttp.h

If we want to allow body larger than content-length, we probably should expose this option

42 changes: 27 additions & 15 deletions lib/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ const util = require('./core/util')
const Request = require('./core/request')
const Dispatcher = require('./dispatcher')
const {
ContentLengthMismatchError,
RequestContentLengthError,
ResponseContentLengthError,
TrailerMismatchError,
InvalidArgumentError,
RequestAbortedError,
Expand Down Expand Up @@ -450,6 +451,8 @@ class Parser {
this.headersSize = 0
this.headersMaxSize = client[kMaxHeadersSize]
this.shouldKeepAlive = false
this.bodySize = 0
this.contentLength = 0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.contentLength = 0
this.contentLength = null

this.paused = false
this.resume = this.resume.bind(this)
}
Expand Down Expand Up @@ -640,9 +643,6 @@ class Parser {

const request = client[kQueue][client[kRunningIdx]]
assert(request)

// TODO: Check for content-length mismatch from server?

assert(!this.upgrade)
assert(this.statusCode < 200)

Expand Down Expand Up @@ -689,23 +689,28 @@ class Parser {
}

assert(this.headers.length % 2 === 0)

this.headers = []
this.headersSize = 0

let keepAlive
let trailers
this.contentLength = 0

let looking = true
for (let n = 0; n < rawHeaders.length && looking; n += 2) {
for (let n = 0, looking = 0; n < rawHeaders.length && looking < 3; n += 2) {
const key = rawHeaders[n]
const val = rawHeaders[n + 1]

if (!keepAlive && key.length === 10 && key.toString().toLowerCase() === 'keep-alive') {
keepAlive = val
looking = !trailers
looking++
} else if (!trailers && key.length === 7 && key.toString().toLowerCase() === 'trailer') {
trailers = val
looking = !keepAlive
looking++
} else if (!this.contentLength && key.length === 14 && key.toString().toLowerCase() === 'content-length') {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
} else if (!this.contentLength && key.length === 14 && key.toString().toLowerCase() === 'content-length') {
} else if (this.contentLength == null && key.length === 14 && key.toString().toLowerCase() === 'content-length') {

// content-length here is always valid because lhttp validate it
this.contentLength = Number(val)
looking++
}
}

Expand Down Expand Up @@ -757,6 +762,8 @@ class Parser {
return -1
}

this.bodySize += buf.length

const request = client[kQueue][client[kRunningIdx]]
assert(request)

Expand Down Expand Up @@ -918,6 +925,11 @@ function onSocketError (err) {
}

function onSocketEnd () {
if (this[kParser].bodySize > 0 && this[kParser].contentLength > 0 &&
this[kParser].bodySize !== this[kParser].contentLength) {
util.destroy(this, new ResponseContentLengthError('other side closed'))
return
}
Copy link
Member

Choose a reason for hiding this comment

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

This check should be in onBody and onMessageComplete

Copy link
Member

Choose a reason for hiding this comment

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

Should probably also check for the strictcontentlength flag

Copy link
Contributor Author

@simone-sanfratello simone-sanfratello Apr 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

That flag refers to the sending headers, I think we should rename it, because in the current situation there are 2 cases and both return an error:

  • the server content-length is greater than the actual body => connection is closed by the server, client returns "UND_ERR_SOCKET" >> fix sending "UND_ERR_RESPONSE_CONTENT_LENGTH_MISMATCH"
  • the server content-length is less than the actual body => connection is closed by the client, client returns "UND_ERR_INFO" >> ? fix sending "UND_ERR_RESPONSE_CONTENT_LENGTH_MISMATCH"

We can add another flag to suppress these errors, because I think those flags have different meanings: as client we can control what we send, so if we see an error/warning on sending headers we can fix it, but we can't control what the server is sending.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only place the check works is in onSocketEnd, doing in onMessage and/or onBody don't work, and also break other tests.

I think we can't use the strictContentLength option, or another one, because the connection always ends with an error, so we can only send the right one instead of the generic

Copy link
Member

Choose a reason for hiding this comment

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

Doing it in onSocketEnd is unfortunately just plain wrong. It should basically only be call if the server unexpectedly closes the connection.

Copy link
Member

@ronag ronag Apr 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

With keep-alive the server can give a body that doesn't match content-length without closing the connection.

Copy link
Member

Choose a reason for hiding this comment

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

I've seen body can't be larger than content-length because of https://github.com/nodejs/undici/blob/e2a9ddc07d9e0ff6be25d243523b53d1d71a3e9e/deps/llhttp/include/llhttp.h - unless we want to add this option

I think you were intending to link to a specific line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, sorry

undici/deps/llhttp/include/llhttp.h

Line 472 in e2a9ddc

/* Enables/disables lenient handling of `Connection: close` and HTTP/1.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With keep-alive the server can give a body that doesn't match content-length without closing the connection.

ok, I'll work on that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ronag to me it's working fine also with keep-alive option

https://github.com/nodejs/undici/pull/742/files#diff-3058feaff6f3b2902370ee08a7528340aa7a98e5b76063f0176cb61972fadbb4R347

util.destroy(this, new SocketError('other side closed'))
}

Expand Down Expand Up @@ -1233,12 +1245,12 @@ function write (client, request) {

if (request.contentLength !== null && request.contentLength !== contentLength) {
if (client[kStrictContentLength]) {
request.onError(new ContentLengthMismatchError())
request.onError(new RequestContentLengthError())
assert(request.aborted)
return false
}

process.emitWarning(new ContentLengthMismatchError())
process.emitWarning(new RequestContentLengthError())
}

const socket = client[kSocket]
Expand Down Expand Up @@ -1305,7 +1317,7 @@ function write (client, request) {

if (!body) {
if (contentLength === 0) {
socket.write(`${header}content-length: ${contentLength}\r\n\r\n\r\n`, 'ascii')
socket.write(`${header}content-length: 0\r\n\r\n\r\n`, 'ascii')
} else {
assert(contentLength === null, 'no body must not have content length')
socket.write(`${header}\r\n`, 'ascii')
Expand Down Expand Up @@ -1344,11 +1356,11 @@ function write (client, request) {
// We should defer writing chunks.
if (contentLength !== null && bytesWritten + len > contentLength) {
if (client[kStrictContentLength]) {
util.destroy(socket, new ContentLengthMismatchError())
util.destroy(socket, new RequestContentLengthError())
return
}

process.emitWarning(new ContentLengthMismatchError())
process.emitWarning(new RequestContentLengthError())
}

if (bytesWritten === 0) {
Expand Down Expand Up @@ -1398,9 +1410,9 @@ function write (client, request) {

if (!err && contentLength !== null && bytesWritten !== contentLength) {
if (client[kStrictContentLength]) {
err = new ContentLengthMismatchError()
err = new RequestContentLengthError()
} else {
process.emitWarning(new ContentLengthMismatchError())
process.emitWarning(new RequestContentLengthError())
}
}

Expand Down
21 changes: 16 additions & 5 deletions lib/core/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,13 +88,23 @@ class InformationalError extends UndiciError {
}
}

class ContentLengthMismatchError extends UndiciError {
class RequestContentLengthError extends UndiciError {
constructor (message) {
super(message)
Error.captureStackTrace(this, ContentLengthMismatchError)
this.name = 'ContentLengthMismatchError'
Error.captureStackTrace(this, RequestContentLengthError)
this.name = 'RequestContentLengthError'
this.message = message || 'Request body length does not match content-length header'
this.code = 'UND_ERR_CONTENT_LENGTH_MISMATCH'
this.code = 'UND_ERR_REQUEST_CONTENT_LENGTH_MISMATCH'
}
}

class ResponseContentLengthError extends UndiciError {
constructor (message) {
super(message)
Error.captureStackTrace(this, ResponseContentLengthError)
this.name = 'ResponseContentLengthError'
this.message = message || 'Response body length does not match content-length header'
this.code = 'UND_ERR_RESPONSE_CONTENT_LENGTH_MISMATCH'
}
}

Expand Down Expand Up @@ -153,7 +163,8 @@ module.exports = {
HeadersTimeoutError,
HeadersOverflowError,
BodyTimeoutError,
ContentLengthMismatchError,
RequestContentLengthError,
ResponseContentLengthError,
ConnectTimeoutError,
TrailerMismatchError,
InvalidArgumentError,
Expand Down
Loading