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 support for HTTP Trailers #146

Closed
clelland opened this issue Jun 8, 2023 · 9 comments · Fixed by #148
Closed

Remove support for HTTP Trailers #146

clelland opened this issue Jun 8, 2023 · 9 comments · Fixed by #148

Comments

@clelland
Copy link
Contributor

clelland commented Jun 8, 2023

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 added a commit that referenced this issue Jun 9, 2023
@clelland
Copy link
Contributor Author

clelland commented Jun 9, 2023

@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
Copy link
Contributor

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

@LPardue
Copy link

LPardue commented Jun 12, 2023

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
Copy link
Contributor Author

@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
Copy link

LPardue commented Jun 14, 2023

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
Copy link
Contributor Author

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
Copy link

LPardue commented Jun 21, 2023

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 w3c/server-timing@9670b35 (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
Copy link
Contributor Author

From the WG discussion yesterday:

  • Trailer support was mentioned in Server Timing, but was removed. @noamr removed that mention, and may have context here.
  • Trailers are supported by Firefox, but that engine does not support NEL
  • No NEL implementation supports request_headers or response_headers, but there was some discussion that those could be useful (at least response_headers).

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.

clelland added a commit that referenced this issue Jun 23, 2023
@noamr
Copy link
Contributor

noamr commented Jun 23, 2023

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:

  • with trailers, it was never clear when server timing entries are created
  • trailers are not exactly specified nor implemented in an interoperable way IIRC

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.

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

Successfully merging a pull request may close this issue.

4 participants