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(xrpc-server): properly parse & process content-encoding #2464

Merged
merged 3 commits into from
Sep 11, 2024

Conversation

matthieusieben
Copy link
Contributor

@matthieusieben matthieusieben commented May 2, 2024

content-encoding can contain multiple values (comma separated).

  • The current implementation ignores any unknown value it received (including comma separated values). This PR generates an error when unknown algorithms are provided.
  • The current implementation uses createDeflate instead of createInflate to decompress bytes encoded using the deflate algo. This PR changes that.
  • The current implementation will allow content-length headers that are not a number. This PR ensures that the parsing of the number did not result in NaN.
  • Fixes Undefined behaviour due to error handing in xrpc-server #2204, which caused a test to be unable to run properly

@matthieusieben matthieusieben force-pushed the fix-content-encoding branch from ac67843 to cf04e48 Compare May 2, 2024 19:19
@matthieusieben matthieusieben changed the title fix(xrpc-server): properly parse & process content-encoding ✅ fix(xrpc-server): properly parse & process content-encoding May 3, 2024
Copy link
Collaborator

@devinivy devinivy left a comment

Choose a reason for hiding this comment

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

Looks good!

@matthieusieben matthieusieben force-pushed the fix-content-encoding branch 3 times, most recently from 2eaf59c to 14dbb29 Compare September 10, 2024 10:28
Comment on lines -266 to -269
if (input?.body instanceof Readable) {
// If the body stream errors at any time, abort the request
input.body.once('error', next)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line breaks Express' flow control. Indeed, when the stream is being consumed, the consumer might will also catch that error. The consumer can then decide to do multiple things:

  • call next() or next(err)
  • handle the error and write to the response

in any case, the error listener here will not "abort" the request, but only trigger the next error middleware, and will cause that error middleware to be called while the body consumer might try to do stuff with the response.

finishFlush: zlib.constants.Z_SYNC_FLUSH,
})
case 'deflate':
return zlib.createInflate()
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh nice snag on switching that out

{
encoding: 'application/octet-stream',
headers: {
'content-encoding': 'gzip, identity, deflate, identity, br, identity',
Copy link
Collaborator

Choose a reason for hiding this comment

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

wowee ✨

Copy link
Collaborator

@dholms dholms left a comment

Choose a reason for hiding this comment

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

looks great - super thorough 👌

@matthieusieben matthieusieben merged commit 98711a1 into main Sep 11, 2024
10 checks passed
@matthieusieben matthieusieben deleted the fix-content-encoding branch September 11, 2024 07:46
@github-actions github-actions bot mentioned this pull request Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Undefined behaviour due to error handing in xrpc-server
3 participants