search

w3c / network-error-logging

Network Error Logging
https://w3c.github.io/network-error-logging/
Other
81 stars 18 forks source link

Remove support for HTTP Trailers #146

Closed clelland closed 1 year ago

clelland commented 1 year ago

HTTP Trailers are not supported in Fetch, and I suspect that they are not actually implemented in any implementation of this spec. (Chromium, at least, does not support request_headers and response_headers policy keys at all)

The "Extract Response Headers" iterates over the list that fetch used to export; we'd need to rewrite that, possibly with a new parsing algorithm, using just RFC9110. I don't think it makes sense to rewrite the header parsing algorithm here to try to continue to include them without referring to Fetch, and so it should probably be removed, given the general lack of support for trailers on the Web in general.

Thoughts?

clelland commented 1 year ago

@yoavweiss @LPardue Do you know of any reason why we would want to continue supporting trailers in the spec? (Non-browser uses of NEL, perhaps? Or just for completeness for as long as they're in HTTP?)

yoavweiss commented 1 year ago

I don't think we should unless it represents actual NEL implementations.

LPardue commented 1 year ago

It seems to stick out how the algorithm calls out trailers specifically. That seems like it should be defined elsewhere as a standard action.

Whether NEL should only be in the header section or the trailer section is a slightly different question. There's an argument that putting in the headers could help caputre problems where the rest of the response is broken somehow (if it were in trailers and the UA gave up before parsing those, oops!). Since NEL is origin-oriented, I can't think of a good reason why someone would want to send NEL late on in a single request - there's no information in there that is only determinable while sending HTTP body. That's unlike something such as Server-Timing, which is totally related to the request.

clelland commented 1 year ago

@LPardue, I think that 4.2 Process policy headers step 4 means that NEL policies can only be processed if they are found in the HTTP header. The only trailer processing in the spec is in the handling of response_headers, which is supposed to be configurable so that you can opt to have specific header/trailer values included in a network error report.

However, to @yoavweiss's question, I'm not aware of any non-Chromium NEL implementation, and Chromium does not follow this, and in fact does not support request_headers or response_headers at all.

LPardue commented 1 year ago

Ah thanks for the clarification.

Cloudflare has an open source NEL client https://github.com/cloudflare/nel-rs. I'm not sure exactly what it does but should be easy enough to figure out.

clelland commented 1 year ago

It doesn't appear that that implementation handles request_headers or response_headers either.

I suppose the question at this point is whether we should:

I'd lean towards the second or third option; the third is probably most practical, especially since this hasn't been implemented in the ~5 years since it was added to the spec. Probably worth a discussion in the WG before we go that far, though.

LPardue commented 1 year ago

FWIW a prime use case for trailers is Server-Timing. That used to be called out explicitly in the spec (see https://www.fastly.com/blog/supercharging-server-timing-http-trailers) but the text was removed in https://github.com/w3c/server-timing/commit/9670b356bf593d89f6fa47a706aff0e40849bf3f (not sure why). And I could see how, in theory, that server-timing send in trailers be useful for NEL reports, but now I'm even more confused :D

Option 3 seems pragmatic to me. Its simpler and represents running code. Maybe in future if there is sufficient interest to justify the effort it can be added back and the work done then.

clelland commented 1 year ago

From the WG discussion yesterday:

For now, I'll remove trailers from the spec. If there is sufficient interest, and someone is also willing to do the implementation work, then it can always be added back.

noamr commented 1 year ago

From the WG discussion yesterday:

  • Trailer support was mentioned in Server Timing, but was removed. @noamr removed that mention, and may have context here.

We removed trailer wording when we did the fetch integration, as:

The idea was that if browsers were to support trailers in an interoperable way we can define the proper integration with fetch and put this back in the server timing spec.