fix(res.send): add Content-Length header only if Transfer-Encoding is not present

[YuryShkoda]

Intent

• Because Content-Length and Transfer-Encoding can't be present in the response headers together, Content-Length should be added only if there is no Transfer-Encoding header.

[dougwilson]

Please add a test that fails without this change and passes with it so the fix does not regress.

[YuryShkoda]

Hey @dougwilson ,
Does this PR look good to you or do I need to adjust something else?

[dougwilson]

Hey, sorry I was just trying to get the new release out. It looks like you still need to add a test for the change to res.redirect, as that was changed too.

I also took a look in the Node.js source and they also have logic for content-length vs transfer-encoding, but node.js only does stuff if transfer-encoding contains chunked while here is it any value (even a single space character). Is that intentional? Should we follow what the node.js logic is? It may help if the underlying issue you ran in to for this change was disclosed.

[YuryShkoda]

Hey @dougwilson,
No worries. As far as I understand it doesn't matter if transfer-encoding contains chunked or not. transfer-encoding header can't be present with content-length in response headers at the same time. Please correct me if I'm wrong.

About the issue we are running in.
We are building this https://github.com/sasjs/server NodeJS wrapper for calling the SAS binary executable and we are building some endpoints for test purposes that mock some external API. And to do so we need to respond with exact same headers and payload. And this external API uses transfer-encoding header that I can't set because in such case I'll have both transfer-encoding header and content-length header in my response. I've been also trying to delete content-length header from response before setting transfer-encoding, but it didn't work.

[dougwilson]

Ah, ok. Can you add tests for other values of transfer-encoding besides chunked as well, then? We want to make sure that res.send will work with other values too.

As for mocking exact replicas of other apis, you typically want to use the lower-level apis for that like res.write and res.end instead of res.send.

[YuryShkoda]

Hey @dougwilson,
Sorry for the late responce- I've been on vacation.
And thank you for suggestion to use res.write.
I reverted changes to res.redirect to dedicate current PR to res.send. I adjusted my unit test to cover all tranfer-encodings.
Please let me know if something else is needed to be done.

Regards,
Yury

[dougwilson]

Hey! It's no problem at all :) Thanks for adding to the tests. But I think there is an issue with them: though you added other values, it does not seem like res.send is actually honoring them. For example, if you have Transfer-Encoding: gzip set, can the test show that res.send will actually send it as gzip? Example I would expect res.set('transfer-encoding').send({foo:'bar'}) to be a gzip response with this changeset, right?

[nukuutos]

Hey @dougwilson!

Can we actually test that our response transforms to gzip? I think its very low level and I'm not sure that nodejs can transform our response to gzip/compress/deflate. Because I did not find any mentions about transfer-encoding that it can be a gzip/compress/deflate response, only it’s a chucked(nodejs documentation). But okay, we exactly know that our response can be transformed to chunks and how we can test it? I don’t have a clue.

Of course tools like zlib can be used for content-encoding but it's not the same as transfer-encoding. (mention it to not to be confused)

I have a suggestion to check our response for having a transfer-encoding header with value that we set on the server but I’m not sure about do we need this. And if we need this check, can I do this commit? I want to make my first contribution to open source!

Please, correct me guys if I’m wrong. Thank you!

[dougwilson]

You can check the spec for transfer-encoding at https://datatracker.ietf.org/doc/html/rfc9112#name-transfer-encoding

The very first example shows how it can invlue gzip.

[nukuutos]

@dougwilson Do you expect that response has corresponding Transfer-Encoding? e.g. if we set Transfer-Encoding: gzip on server, we should get it in response on client?

[bjohansebas]

LGTM. Some considerations to keep in mind are that Transfer-Encoding might indicate that the response is gzip or some other encoding, while the data being sent is actually not transformed to that encoding. I’d say that responsibility lies with the developer to ensure the data is sent in the correct format—there’s no real way for Express to validate that, and I don’t think we should limit ourselves to only checking whether transfer-encoding has the value chunked. Regardless of the value, those two headers must not be present together.

[jonchurch]

Im on the fence here. The response is malformed bc of user action.

Using res.send was the wrong tool for the reporter’s use case. But since res.send already tries to set appropriate headers and respects other user set headers, checking for this edge case makes sense for consistency.

Compliant clients/intermediaries favor Transfer-Encoding over Content-Length, so removing the conflict only reduces risk.

Im trying to think through if taking a change here increases or reduces possible malformed responses folks can send, hence being on the fence.

Edit: I can only see how it reduces the number of malformed responses possible. As any compliant client should already be ignoring content length when transfer-encoding is present. So removing content-length when transfer-encoding is present should be a noop for compliant clients.