Handle missing etag in 304 Not Modified response #4470
Conversation
GitHub now omits the etag, but 304 implies it matches the one we provided. Just use that one to avoid having an etag-less resource. Fixes NixOS#4469
|
Interesting. We should also complain to GitHub because RFC 7232 §4.1 states that a 304 Not Modified MUST include an ETag that would have been included in a 200 OK. |
|
I also think we should tweak this to simply not assert if the etag doesn't match, because the server could plausibly return a different ETag than what we provided as long as the server still considers it content-equivalent. |
That's interesting - I assumed it was an optional response header.
Yeah that makes sense - asserting based on an external service is not good anyway. I think you may end up with multiple entries in the cache for each etag, but that's not necessarily bad. |
|
does this need to be back-ported as well? |
Yes - it's not fatal like with master, but it does end up passing the wrong values to If-None-Match meaning downloadCached doesn't work properly with github. |
|
Backport at #4475 |
|
@lilyball I agree, my understanding of the spec is that this makes GitLab non-compliant. Has anyone reported it to them? |
GitHub now omits the etag, but 304 implies it matches the one we
provided. Just use that one to avoid having an etag-less resource.
Fixes #4469