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.

Sign up for GitHub

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

Jump to bottom

set host header on self outbound requests #2298

Merged
merged 1 commit into from
Mar 12, 2024
Merged

set host header on self outbound requests #2298

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion crates/trigger-http/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use std::{
use anyhow::{Context, Result};
use async_trait::async_trait;
use clap::Args;
use http::{uri::Scheme, StatusCode, Uri};
use http::{header::HOST, uri::Scheme, HeaderValue, StatusCode, Uri};
use http_body_util::BodyExt;
use hyper::{
body::{Bytes, Incoming},
Expand Down Expand Up @@ -490,6 +490,9 @@ impl OutboundWasiHttpHandler for HttpRuntimeData {
.parse()
// origin together with the path and query must be a valid URI
.unwrap();
let host = format!("{}:{}", uri.host().unwrap(), uri.port().unwrap());
let headers = request.request.headers_mut();
headers.insert(HOST, HeaderValue::from_str(&host)?);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about potentially overwriting an existing host header.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are only overwriting on self-requests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that but what if some mad, bad person sends a self request and explicitly sets the host header on it. (I cannot imagine why they would ever do this... I just wonder if we should respect it if they do.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will check if it is a case of overwriting always or only if the user explicitly sets it. In that case, we can check for its existence before setting it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default, the host seems to be

host = Some(
    ":443",
)

Do we want to do something like

if let Some(":443") = headers.get("host") {
    headers.insert(HOST, HeaderValue::from_str(&host)?);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Egad! That host looks extremely weird, though I guess it's what we should expect given what we saw in the issue. It makes me think it's already getting set somewhere - and that makes me wonder if we can tweak wherever it's currently getting set rather than overwriting it later in the pipeline. It reminded me of the issue @dicej found (fermyon/spin-rust-sdk#8 (comment)) so there may be some subtle difference between main and RTM anyway...

Anyway, "user explicitly sets Host header in a self-request" certainly feels like a wildly obscure edge case. Given that it's not trivial to determine, I'm happy to punt on it for the time being. Thanks for the detailed investigation!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to dig into where the header actually gets set. I will also update the rust SDK that the app uses to main instead of pinning it to v2.2 to see if that interaction differs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some digging and this originates from the parsing of the uri by hyper. The first request goes through fine because the host is set properly on the incoming request that we use that to construct the URL and then in the subsequent requests, it fails.

ref: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=fb83cc1730a01a63f2d9c70ff95074a8


request.use_tls = uri
.scheme()
Expand Down
Loading