-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add Scheme extractor #2507
base: main
Are you sure you want to change the base?
Add Scheme extractor #2507
Conversation
The test for extracting the scheme from the URI is missing as I haven't groked how to pass a URI with a scheme to the test server. |
@davidpdrsn Is this ready for merge, or do I need to add more tests? |
May I suggest additionally supporting |
It also seems to me that the extraction from |
Thanks for your comments @Wiezzel. |
@Wiezzel Thanks for taking the time to review the PR.
Turns out this was a typo;
I have added a manual test that calls |
@bengsparks Please check out this minimal example:
[package]
name = "scheme-extractor"
version = "0.1.0"
edition = "2021"
[dependencies]
axum = "0.7.3"
tokio = { version = "1.36.0", features = ["full"] }
[patch.crates-io]
axum = { git = "https://github.com/bengsparks/axum.git" }
async fn test(scheme: axum::extract::Scheme) -> String {
scheme.0
}
#[tokio::main]
async fn main() {
let app = axum::Router::new()
.route("/", axum::routing::get(test));
let listener = tokio::net::TcpListener::bind("0.0.0.0:8000").await.unwrap();
axum::serve(listener, app).await.unwrap();
} Then run:
|
I think we've made a point to not handle any conventional, or even standard headers for "original" request data from proxies in |
@jplatte Could you concretise what this means for the PR? Does this mean |
I could suggest a fallback to HTTP in case the scheme cannot be extract neither from headers, nor from path. This sounds like a reasonable default to me, but it could be optional in case some application needs stricter check. |
Yes, that is what I meant. I didn't know we even had a |
@jplatte I will put this PR on pause then until the matter is resolved; Having a |
In my tests, the
If you wanted to walk that path to the end, you'd have to remove the But as a user, the extractors came across to me as an implementation of common practices. Which is why I would expect them to check those extra headers. Because if they didn't, they'd boil down to a one-liner like I would not expect them to be subject to security-based mitigations. |
This reverts commit 000e3b7.
I cannot see a way to test this right now, but cURL shows it works
To reiterate:
The final bullet point requires that Should the naming of |
Why is that? Regardless of the HTTP version, something in the stack has to initiate the TLS connection, and therefore knows what scheme is in use. |
Ah, I think I've mixed something up. |
Motivation
Closes #2504.
Solution
Detect the scheme in the following order:
proto
within theForwarded
headerX-Forwarded-Proto