Spoofable extractors are used with the knowledge of the risks

[yanns]

The ticket follows the discussion in #2507 (comment)

Some extractors, like Host or Scheme, can use the values of some HTTP headers that could be spoofed by malicious users.

We should find a way to make users aware of the risks of using those extractors.

Some ideas:

• using unsafe. This is not the idea of unsafe and we would be mis-using it. I think that this can be discarded.
• encapsulating the value in a new struct like SpoofableValue so that users have to call some function to get the value. The name and the documentation of the function should make the user aware of the risk. Example:

async fn handler(Host(host): Host) -> String {
  val value = host.spoofable_value();
  value
}

[bengsparks]

Perhaps something along the lines of:

/// Wrap spoofable extractor
pub struct Spoofable<E>(pub E);

/// Allow `Spoofable` to be used with spoofable extractors in handlers
impl <S, E> FromRequestParts<S> for Spoofable<E> where E: FromSpoofableRequestParts<S> {

}

/// axum private trait
trait FromSpoofableRequestParts<S>: Sized {
    type Rejection: IntoResponse;

    async fn from_request_parts(
        parts: &mut Parts, 
        state: &S
    ) -> impl Future<Output = Result<Self, Self::Rejection>> + Send;
}

/// Mark `Host` as a spoofable extractor
impl <S> FromSpoofableRequestParts<S> for Host { ... } 

/// Use spoofable extractor
async fn handler(Spoofable(Host(host)): Spoofable<Host>) -> String {
    println("{host}");
}

[yanns]

I've made one PoC so that we can better imagine how the API would be: #3000

@bengsparks could you also make one for the approach you're suggesting. It seems very interesting!

[mladedav]

Is it possible to add on either of the extractors something like Host::unspoofable_value(&self) -> Option<String>?

I don't think host can be extracted from anything that cannot be spoofed and scheme could theoretically be extracted from connect info, but the way it is implemented now, it prefers the scheme the client used originally if the server is behind a proxy, i.e. it tries to extract from the proxy headers first which might be what the user is interested in.

If we can only return values extracted from spoofable sources, I feel like the destructuring is the nicer syntax from the current two options, but that's just my opinion. Getting rid of the Spoofable wrapper first also allows users to pass around Host in type-safe manner and we can implement Deref and Into for convenience. If we go with the first option, users would either have to call spoofable_value at every usage site or they would have to pass around a String. Implementing Into or Deref would completely circumvent forcing users to be explicit about acknowledging the spoofable scenario so that could never be added.

For completeness, would you be opposed to just having spoofable-extractors feature which would gate Host and Scheme in their current implementation? It would reduce the noise in handler signatures and users still have to opt-in, although just once for all of them and not explicitly for each use. I guess the question is if it's explicit enough.

[jplatte]

How about Host<WithProxyHeaders> and Host<WithoutProxyHeaders> as an alternative? I find "spoofable" sounds a bit awkward, and while the proxy thing may not sound as dangerous, it would still get people thinking.

[yanns]

I personal like having to change the usage site.
I guess it would be very easy to have a function taking a Scheme and forgetting about the risks of using it.
Being force to call spoofable_value makes sure that the person taking care of this particular implementation will be reminded of the consequences.

[mladedav]

How about Host<WithProxyHeaders> and Host<WithoutProxyHeaders>

I would see that as another dimension because both the proxy headers and the host header can be spoofed.

[jplatte]

Okay so I don't hate any of the options presented so far. They all seem a bit weird but that's almost the point, so not too surprising. @yanns, @mladedav if you can agree on a best solution, feel free to go ahead and merge the corresponding PR and close this issue.