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: dont crash on invalid brotli payload #3620

Closed
wants to merge 4 commits into from
Closed

fix: dont crash on invalid brotli payload #3620

wants to merge 4 commits into from

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Sep 18, 2024

Fixes #3616 by setting the corresponding flags for brotli like we did for gzip in #2126

https://nodejs.org/api/zlib.html#compressing-http-requests-and-responses

image

But why does deflate not need those flags?

Copy link
Member

@KhafraDev KhafraDev left a comment

Choose a reason for hiding this comment

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

an error listener should still be added onto pipeline(...)... this only fixes the one case
#3616 (comment)

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 19, 2024

@KhafraDev better?

Comment on lines 2181 to 2183
body: streams === undefined
? this.body.on('error', noop)
: pipeline(streams, noop).on('error', noop)
Copy link
Member

Choose a reason for hiding this comment

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

this is just hiding a bug somewhere else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how about now?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 19, 2024

@KhafraDev
@ronag ptal

Copy link
Member

@KhafraDev KhafraDev left a comment

Choose a reason for hiding this comment

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

there are quite a few unnecessary changes that make this section harder to read


this.body = new Readable({ read: resume })

const decoders = []
/** @type {[src: import('node:stream').Readable, ...target: import('node:stream').Writable[]]} */
let streams
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
let streams
const streams = []


// https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Encoding
if (codings.length !== 0 && request.method !== 'HEAD' && request.method !== 'CONNECT' && !nullBodyStatus.includes(status) && !willFollow) {
streams = [this.body]
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
streams = [this.body]

decoders.length = 0
// If the server sends the payload with a coding which his not
// supported, the body will be passed through without decoding.
streams = undefined
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
streams = undefined
streams.length = 0

body: decoders.length
? pipeline(this.body, ...decoders, noop)
: this.body.on('error', noop)
body: streams === undefined
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
body: streams === undefined
body: streams.length === 0

: this.body.on('error', noop)
body: streams === undefined
? this.body.on('error', onError)
: pipeline(streams, callback).on('error', onError)
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
: pipeline(streams, callback).on('error', onError)
: pipeline(this.body, ...streams, callback).on('error', onError)

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 20, 2024

Tbh i dont like the suggested changes. So I retract this PR. No bad feelings ;).

@Uzlopak Uzlopak closed this Sep 20, 2024
@Uzlopak Uzlopak deleted the issue-3616 branch September 20, 2024 04:31
@KhafraDev
Copy link
Member

I will work on it tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants