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

DELETE with content-length 0 produces a technically incorrect payload #140

Open
deej-split opened this issue Feb 26, 2024 · 3 comments
Open
Labels
bug Bug or defect

Comments

@deej-split
Copy link

deej-split commented Feb 26, 2024

Runtime

node.js

Runtime version

v20.11.0

Module version

10.0.4

Last module version without issue

No response

Used with

hapi

Any other relevant information

No response

What are you trying to achieve or the steps to reproduce?

We are using h2o2 to proxy requests. We have found that sending a DELETE request with a content-length of 0 has an unexpected result:

  • We get a 0 length Buffer as the payload
  • h2o2 then concludes that since there's no transfer-encoding header in the originating request, (no encoding), there IS a payload, and this is a DELETe method that it should add a transfer-encoding: chunked header
  • Later on, the overall hapi system concludes (it seems) that since there IS a Buffer instance (even if 0) it will add a content-length header

What was the result you got?

The result is that the request that is sent on has both a transfer-encoding and a content-length header. Evidently, this is technically incorrect. We have two other servers in our systems which do not like this header combination and reject the request. (other servers don't seem to care)

What result did you expect?

Probably that if there is no content, that we do not set a transfer-encoding header?

@deej-split deej-split added the bug Bug or defect label Feb 26, 2024
@deej-split deej-split changed the title DELETE with content-length 0 produces a technically incorrectpayload DELETE with content-length 0 produces a technically incorrect payload Feb 26, 2024
@deej-split
Copy link
Author

deej-split commented Feb 27, 2024

fwiw, we worked around this by defining a custom httpClient that checks if the payload is 0 length, and if so passes a payload of undefined. I'm not sure it is 100% correct, but it works for our need.

@kanongil
Copy link
Contributor

This issue seems to be a fallout of a decision to strip content-length headers in passthrough mode, which broke DELETE proxying. A fix was applied in 3e695df, but this has issues due to Wreck incorrectly adding an extra content-length header.

As far as I can tell, this issue applies to all DELETE proxying with content-length, not only when it is 0.

As I see this, there are two bugs involved:

  1. H2o2 removes the content-length header. This removal does not really make sense, and seems to be an incorrect fix to an issue in another module.
  2. Wreck is too liberal in adding a content-length header. It should probably not add it when the headers include a transfer-encoding.

FYI, only one of these needs to be fixed, to resolve this specific issue.

@kanongil
Copy link
Contributor

kanongil commented Feb 28, 2024

Another related h2o2 issue, seems to be that it does not strip hop-by-hop headers (including the already know ones), which can mess up the connection to the proxied server.

Again content-length is not a hop-by-hop header, and it makes no sense that h2o2 strips this when it doesn't do anything to the payload.

FYI, hapi already strips them from stream responses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug or defect
Projects
None yet
Development

No branches or pull requests

2 participants