Handle missing etag in 304 Not Modified response

[matthewbauer]

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

[lilyball]

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.

[lilyball]

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.

[matthewbauer]

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.

That's interesting - I assumed it was an optional response header.

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.

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.

[zimbatm]

does this need to be back-ported as well?

[matthewbauer]

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.

[matthewbauer]

Backport at #4475

[kirelagin]

@lilyball I agree, my understanding of the spec is that this makes GitLab non-compliant. Has anyone reported it to them?