Add proxies::Forwarded header

[jbr]

Experimented a bit with a COW-like interface for the forwarded header. Let me know if you'd prefer to switch to just the Owned version. Alternatively, if y'all like this pattern, we might want to use it for other headers

edit: switching to draft — noticed some additional complexity in the spec that this version doesn't handle. Interested in feedback on the Cow-like interface, though

Ok! This pr now includes is now an efficient and correct hand-rolled parser for tokens and quoted strings as defined by https://tools.ietf.org/html/rfc7230#section-3.2.6, and for the Forwarded header, including a spec-conforming normalization from the x-forwarded-{for, proto, and by} headers. Hopefully the token and quoted-string parsers will be helpful for other header parsers in http-types

Ref #99

[yoshuawuyts]

I think using a Cow under the hood might make sense, but we probably shouldn't surface it to the end-user. So far I've been trying to make enums expose HTTP terminology only, and caring a bit less about internal optimizations.

Also for fn new: would it make sense to use a TryInto<URL> bound? We may need to ensure we correctly serialize it, but to me that seems like a fairly natural fit?

[jbr]

@yoshuawuyts How do you think x-forwarded-for, x-forwarded-proto, and x-forwarded-host should interact with the forwarded header? It would be nice for there to be a "meta header" that pulled from whatever was available, but I'm not sure if Forwarded should try to populate itself from the x- variants if they exist

[jbr]

I guess the question is how 1:1 the headers types should be with a single header

[yoshuawuyts]

It's my understanding that those are no longer used much, and mostly exist for backwards compat purposes? Afaik they haven't been specced either.

Perhaps let's not care about it now, and if we ever want to do something there we can add some higher-level "proxy" submodule (perhaps in Tide?) that is aware of how to handle this?

[jbr]

Unfortunately, I think the standardization of forwarded isn't yet complete, and the x- variants are still common in the wild. A quick search turned up references like https://devcenter.heroku.com/articles/http-routing#heroku-headers, and there are "de facto" specs:

• https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-Proto
• https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-Host
• https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For

However, the forwarded spec also encourages implementers to convert from the x- headers to forwarded: https://tools.ietf.org/html/rfc7239.html#section-7.4, so it seems legit to consider them "the same thing" and accept either but only output Forwarded (like good spec-abiding implementers)

[yoshuawuyts]

Forwarded is part of RFC 7239, dated 2014. The iana HTTP header overview lists it as an accepted standard (experimental headers are highlighted).

Given how long it's been around it would be surprising if it this wasn't supported by modern servers. Perhaps a compromise we can make here is that we try and detect X-Forwarded-For as a fallback in from_headers, but never serialize it ourselves in value. That way we can detect it if we must, but don't propagate it ourselves. Wdyt?

[jbr]

Exactly what I suggested in the edit to #221 (comment), which I might have made at the same time as you posted this 👍

Also switching to making each field an Option<Cow<'_, str>>, making the parser a bit more lax (I thought all four were required but it turns out they're not), replacing the new(a,b,c,d) with chainable setters, and making apply fallible because it's basically a builder finalizer. phew there was a bunch wrong with the first version of this pr.

[yoshuawuyts]

and making apply fallible because it's basically a builder finalizer

Ah interesting; none of the other headers has a fallibly apply. They all assume that once they contain a value it's valid; this makes it easy to reason about the inner state for both parsing and encoding.

What would the error be that surfaces for apply? Is there a way we could surface it sooner?

[jbr]

@yoshuawuyts @Fishrock123 This is now ready for review. Although I do hope that this is fairly readable as far as hand-rolled nearly-zero-copy parsers go, I think the most valuable way to review this would be to take a look at the spec and the test cases before looking at the implementation.

The general approach to parsing in this pr is for parsers to return (Option<&str>, &str) or (Option<Cow<'_, str>>, &str), depending on whether the parser might need to sometimes allocate. The first value is the thing if it can be parsed, and the second str is the rest of the input string, split at the end of the consumed bytes.

It was unnecessary in this PR, but it would not be terribly hard to write higher order logic based on a Fn(&str) -> (Option<Cow<'_, str>>, &str) trait interface at some later point

[Fishrock123]

These examples may be more useful written in the way we will likely suggest of their use in real code, I'm thinking:

if let Some(forwarded) = Forwarded::from_headers(&request)? {
    // ...
}

[jbr]

Halfway addressed. Switched to from_headers(&request)?.unwrap() — it doesn't make sense to have a doctest that includes a conditional we know to be true

[Fishrock123]

Is this lifetime necessary on the struct, or can these be '_?

[jbr]

Necessary, I believe. I'll try it to be sure, though

[jbr]

If this were '_, I believe there would be no assurance to the compiler that the forwarded has the same lifetime as the &impl AsRef<Headers>, and that all the Cows have same lifetime as the request/response. And we need that because in the borrowed state, we're just pointing at data held in the request/response

[Fishrock123]

Ok, I really don't understand lifetimes in structures yet, I think.

[Fishrock123]

(Note: I did not review the spec)

[jbr]

I've removed the with_… chainable methods from this, should be good to go, cc @yoshuawuyts

CI failing for unrelated reasons (async-std 1.6.3 doesn't compile on wasm)