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

Define policy for which header name constants to include #573

Open
seanmonstar opened this issue Nov 8, 2022 · 10 comments
Open

Define policy for which header name constants to include #573

seanmonstar opened this issue Nov 8, 2022 · 10 comments
Labels
A-headers Area: HTTP headers B-rfc Blocked: request for comments. More discussion would help move this along.

Comments

@seanmonstar
Copy link
Member

We include many standard header names as constants in http::header. The exact criteria has not been defined, and it would be wise to do so. This stops it from being an unwritten rule in a few people's heads. The policy likely should consider the pros of adding any name versus the costs of adding a name.

@seanmonstar seanmonstar added B-rfc Blocked: request for comments. More discussion would help move this along. A-headers Area: HTTP headers labels Nov 8, 2022
@paolobarbolini
Copy link

paolobarbolini commented Nov 8, 2022

Thanks for creating this issue.

I went through the list of existing constants and here are some thoughts on the existing ones:

Speaking for me I would very well do without most of them if that meant adding newer security or otherwise useful headers.

@seanmonstar
Copy link
Member Author

Thanks for listing those, it may be worth considering quicker if we should chop out some of the obscure ones that are currently in the crate before 1.0 (which is coming soon!).

@acfoltzer
Copy link
Member

acfoltzer commented Jan 23, 2023

A good place to start is the IANA Field Name Registry. The registry itself is here, and the specification of the registry is here in RFC 9110.

In this gist I compared the names in the registry vs the names defined as constants in http. There are quite a few more in the registry than we currently have as constants. I'm in favor of including any registry headers that aren't provisional as constants in this crate, as they are very unlikely to ever go away.

There are three sets that I think are more debatable.

provisional headers in the IANA registry

amp-cache-transform
configuration-context
ediint-features
isolation
repeatability-client-id
repeatability-first-sent
repeatability-request-id
repeatability-result
sec-gpc
timing-allow-origin

I'm inclined to not proactively make constants for these given their lack of stability, but we should be open to their inclusion if folks request them and it appears they're on track to standardize. For example, Timing-Allow-Origin is part of the Resource Timing W3C Editor's Draft.

Header constants currently in http but not the registry

dnt
referrer-policy
upgrade-insecure-requests
x-dns-prefetch-control
x-xss-protection

These probably need to be handled on a case-by-case basis, as they might be referenced in other specs. For example, dnt is part of the Fetch standard.

Headers in neither the registry nor http

...but present in widely-cited sources such as MDN. For example, Accept-CH-Lifetime. These probably also need to be treated on a case-by-case basis.

@seanmonstar
Copy link
Member Author

Hm, even the MDN DNT page says the header is deprecated. Perhaps it's worth just removing the non-standard ones entirely.

Could we get away with removing all the deprecated/obsolete headers even if in the registry?

I'm in favor of including any registry headers that aren't provisional as constants in this crate, as they are very unlikely to ever go away.

It's a fair position to take. If many others prefer it, we can do that.

I'm personally skeptical of adding obscure names even if they are in the registry. They seem like a potential distraction in the docs. But I can't really articulate a good reason otherwise. Perhaps at least I'd wait to add the odd ones until someone really asked?

@acfoltzer
Copy link
Member

Could we get away with removing all the deprecated/obsolete headers even if in the registry?

I'm definitely coming from a biased perspective as an implementer of a reverse proxy. We don't control the nature of requests from clients or the responses from origins, so we try to accept as much actual existing HTTP traffic as possible without compromising security.

It's also common for current, active specs to refer to deprecated and obsolete headers. This is often in MAY or SHOULD contexts, but nonetheless comprehensive, modern implementations frequently need to at least refer to and understand the cruft even if they don't produce the cruft themselves.

I'm personally skeptical of adding obscure names even if they are in the registry. They seem like a potential distraction in the docs.

This raises a higher-level question: what is our goal for including these constants? For me, the value comes from being able to avoid repeating HeaderName::from_static() invocations for standardized headers across my different crates. And while that is less painful since that method became const, there are still some odd limitations that crop up (particularly around static references) that the crate-defined constants avoid. I don't personally come to the http rustdoc for information about the semantics of the headers, but I know I am not the typical user in that regard, and therefore am probably not a good judge about the level of distraction imposed by additional definitions in the docs.

Perhaps at least I'd wait to add the odd ones until someone really asked?

I think being proactive about adding registered names would help us avoid a trickle of one-off PRs and requests for releases (like I am doing now in #583 😬). And while I doubt we want to impose a proc macro dependency for http, we could also consider automatically generating the constants based on the IANA registry's CSV, which I got IANA and IETF to license as CC0.

@d2718
Copy link

d2718 commented Jan 25, 2023

What if a core, standard, forward-looking set of constants were included by default, but then the rest of the obscure/deprecated/provisional/nonstandard-but-in-use ones were hidden behind a feature flag, like extra-headers?

@acfoltzer
Copy link
Member

@d2718 the complexity of wrangling a Cargo feature would outweigh the benefit of less clutter in the namespace, in my opinion.

I suppose we could add some namespacing via child modules of http::header: IANA permanent header names could stay in http::header, but we could stash other categories in http::header::obsolete, http::header::provisional, etc. I kinda like that as a quick signal to the user that they're straying off the beaten path.

@seanmonstar
Copy link
Member Author

Another thing that came to my mind today was that now that from_static is a const fn, we could potentially provide constants that don't require being in the internal StandardHeader enum. A small concern I had had before was around growing that enum and match statements. But since they can also be from a static string, it does feel like we could include all easily.

Then I suppose a last question is how to determine whether something should be in StandardHeader, or if it should be from a string. A value in StandardHeader will mean that hashing and comparing it will be faster than a static string, but growing the enum may mean the match in hash and cmp (and as_str) will be more code, and could be miss optimizations. Probably worth measuring or determining some heuristic? I think most people would prefer that the most common headers are as fast as possible.

@acfoltzer
Copy link
Member

Then I suppose a last question is how to determine whether something should be in StandardHeader, or if it should be from a string.

I think we should start with a larger set in StandardHeader, since we should be able to "demote" them to from_static invocations without semver breakage if it turns out performance is a problem. We can check the microbenchmarks right away to have some idea of impact, but concerns about missed optimizations are hard to definitively disprove.

@GlenDC
Copy link

GlenDC commented Mar 1, 2024

Would you be open for me to make a PR for Client Hint headers, or do these count as provisional, even if browsers do all support them now a days (the most common type of user agent among them all).

I would understand if you do not want it yet though, even though servers and proxies do need these for content negotiation. It also impacts other aspects such as Caching.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-headers Area: HTTP headers B-rfc Blocked: request for comments. More discussion would help move this along.
Projects
None yet
Development

No branches or pull requests

5 participants