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

Remove content-length for notModified responses #183

Merged
merged 1 commit into from May 10, 2021

Conversation

natebosch
Copy link
Member

It is an error to include a non-zero content-length when there is no
body. In some places, especially those where we are implementing a
proxy, we were previously relying on behavior in shelf which would zero
out the content length when there is no body. After allowing a content
length without a body for HEAD requests we lost the safety for other
request types.

The errors that have surfaced since published are related to
Response.notModified which should never have a content-length, so
handling it there should retain most of the safety we had before. There
is still the possibility for problems in other response types which will
need to be handled at the usage site.

It is an error to include a non-zero content-length when there is no
body. In some places, especially those where we are implementing a
proxy, we were previously relying on behavior in shelf which would zero
out the content length when there is no body. After allowing a content
length without a body for `HEAD` requests we lost the safety for other
request types.

The errors that have surfaced since published are related to
`Response.notModified` which should never have a `content-length`, so
handling it there should retain most of the safety we had before. There
is still the possibility for problems in other response types which will
need to be handled at the usage site.
@natebosch natebosch requested review from kevmoo and grouma May 10, 2021 20:57
@google-cla google-cla bot added the cla: yes label May 10, 2021
@natebosch natebosch merged commit 37a5c2d into master May 10, 2021
@natebosch natebosch deleted the not-modified-content-length branch May 10, 2021 21:01
@kevmoo
Copy link
Member

kevmoo commented May 10, 2021

Nit: it's totally fine for a 304 to include a content-length – as long as it matches what WOULD be sent

@natebosch
Copy link
Member Author

Nit: it's totally fine for a 304 to include a content-length – as long as it matches what WOULD be sent

Is that a bug in dart:io in that case?

@kevmoo
Copy link
Member

kevmoo commented May 10, 2021 via email

@natebosch
Copy link
Member Author

natebosch commented May 10, 2021

From my reading of the spec including this header is either a "SHOULD NOT" or a "MUST NOT" depending on the request.

https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.3.5

If the conditional GET used a strong cache validator (see section 13.3.3), the response SHOULD NOT include other entity-headers. Otherwise (i.e., the conditional GET used a weak validator), the response MUST NOT include other entity-headers; this prevents inconsistencies between cached entity-bodies and updated headers.

I think we mostly use strong cache validators, but given that we still "SHOULD NOT" send the header I'm not inclined to try to bake in support for it in shelf. If we want to file a bug against the SDK we could.

@mraleph
Copy link
Member

mraleph commented May 11, 2021

FYI: dart-lang/sdk#45871

@kevmoo
Copy link
Member

kevmoo commented May 11, 2021 via email

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

Successfully merging this pull request may close these issues.

None yet

4 participants