304 responses should contain etag

[maxdewald]

Describe the bug

Currently the correct headers are missing on 304 responses.

The IETF standards i founds say this:
The server generating a 304 response MUST generate any of the following header fields that would have been sent in a 200 (OK) response to the same request: Cache-Control, Content-Location, Date, ETag, Expires, and Vary.
https://datatracker.ietf.org/doc/html/rfc7232#section-4.1

kit/packages/kit/src/runtime/server/index.js

Lines 118 to 140 in b10ba93

	if (response) {
	// inject ETags for 200 responses
	if (response.status === 200) {
	const cache_control = get_single_valued_header(response.headers, 'cache-control');
	if (!cache_control || !/(no-store|immutable)/.test(cache_control)) {
	let if_none_match_value = request.headers['if-none-match'];
	// ignore W/ prefix https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/If-None-Match#directives
	if (if_none_match_value?.startsWith('W/"')) {
	if_none_match_value = if_none_match_value.substring(2);
	}
	const etag = `"${hash(response.body || '')}"`;
	if (if_none_match_value === etag) {
	return {
	status: 304,
	headers: {}
	};
	}
	response.headers['etag'] = etag;
	}
	}

For me this causes cloudflare caching to not revalidate my api responses correctly when a cache-control: max-age=60 header was set. Cloudflare support was so kind to point me to this issue.

Reproduction

Can create a repo if requested

Logs

No response

System Info

"@sveltejs/adapter-node": "1.0.0-next.61",
"@sveltejs/kit": "1.0.0-next.222",

Severity

serious, but I can work around it

Additional Information

I would suggest the following change to fix this issue.

const etag = `"${hash(response.body || '')}"`;
response.headers['etag'] = etag
if (if_none_match_value === etag) {
  return {
    status: 304,
    headers: response.headers
  };
}								

[Conduitry]

Interesting, I wasn't aware of this. It does indeed sound in the spec that we should be returning an ETag here. Thanks for the report!

[vicky1999]

Can I work on this issue?

[bluwy]

@vicky1999 feel free if no one created a PR yet.

[bluwy]

#3133 fixed this