Repository: bytecodealliance/wasmtime
Pull request #8671 (2024-05-21 10:15:08)

wasi-http: add the port to authority when opening a TCP connection
<!--
Please make sure you include the following information:

- If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

- Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

#8563 removes the port from the authority, it makes the target address cannot be correctly resolved when opening a TCP connection. This results in that if we want to send an HTTP request in the wasi component, an error `ErrorCode::ConnectionRefused` will be reported directly. The underlying error is `invalid socket address`:

<img width="521" alt="Screenshot 2024-05-21 at 17 21 56" src="https://github.com/bytecodealliance/wasmtime/assets/9134003/6473efef-4306-45c7-8ca4-e5101d14f071">

/cc @alexcrichton @elliottt 

bjorn3: What if the program explicitly specified a port different from 80 or 443? Wouldn't this change break that by creating a nonsensical address like `127.0.0.1:1234:80` when trying to request `http://127.0.0.1:1234/some_page`?
iawia002: > What if the program explicitly specified a port different from 80 or 443? Wouldn't this change break that by creating a nonsensical address like `127.0.0.1:1234:80` when trying to request `http://127.0.0.1:1234/some_page`?

Indeed, I didn't consider that. I will update it again.
alexcrichton: Does this mean that `default_send_request_handler` basically doesn't work by default? If so, that seems bad!

Would you be up for adding a test for this?
morenol: > Does this mean that `default_send_request_handler` basically previously didn't work by default? If so, that seems bad!
> 
> Would you be up for adding a test for this?

I think that that is the case, can we have a fix like this one on a 21.0.1 version?
iawia002: I have added a test for this case, following @elliottt's advice in https://github.com/bytecodealliance/wasmtime/pull/8676#discussion_r1609131895, PTAL @alexcrichton @elliottt 

Patch:
diff --git a/cranelift/src/souper_harvest.rs b/cranelift/src/souper_harvest.rs
index 8aeb50a12..d8a559c43 100644
--- a/cranelift/src/souper_harvest.rs
+++ b/cranelift/src/souper_harvest.rs
@@ -7,6 +7,7 @@ use cranelift_codegen::Context;
 use cranelift_reader::parse_sets_and_triple;
 use rayon::iter::{IntoParallelIterator, ParallelBridge, ParallelIterator};
 use std::collections::HashSet;
+use std::hash::{BuildHasher, BuildHasherDefault};
 use std::io::Write;
 use std::path::PathBuf;
 use std::{fs, io};
@@ -143,7 +144,5 @@ fn parse_input(path: PathBuf) -> Result<Vec<Function>> {
 /// A convenience function for a quick usize hash
 #[inline]
 pub fn hash<T: std::hash::Hash + ?Sized>(v: &T) -> usize {
-    let mut state = rustc_hash::FxHasher::default();
-    v.hash(&mut state);
-    std::hash::Hasher::finish(&state) as usize
+    BuildHasherDefault::<rustc_hash::FxHasher>::default().hash_one(v) as usize
 }
diff --git a/crates/wasi-http/src/types.rs b/crates/wasi-http/src/types.rs
index acc401d30..e7e9c6973 100644
--- a/crates/wasi-http/src/types.rs
+++ b/crates/wasi-http/src/types.rs
@@ -187,7 +187,14 @@ pub async fn default_send_request_handler(
         between_bytes_timeout,
     }: OutgoingRequestConfig,
 ) -> Result<IncomingResponse, types::ErrorCode> {
-    let Some(authority) = request.uri().authority().map(ToString::to_string) else {
+    let authority = if let Some(authority) = request.uri().authority() {
+        if authority.port().is_some() {
+            authority.to_string()
+        } else {
+            let port = if use_tls { 443 } else { 80 };
+            format!("{}:{port}", authority.to_string())
+        }
+    } else {
         return Err(types::ErrorCode::HttpRequestUriInvalid);
     };
     let tcp_stream = timeout(connect_timeout, TcpStream::connect(&authority))


Test patch:
diff --git a/crates/test-programs/src/bin/api_proxy_forward_request.rs b/crates/test-programs/src/bin/api_proxy_forward_request.rs
new file mode 100644
index 000000000..8fb926cfe
--- /dev/null
+++ b/crates/test-programs/src/bin/api_proxy_forward_request.rs
@@ -0,0 +1,42 @@
+use test_programs::wasi::http::types::{
+    Headers, IncomingRequest, OutgoingBody, OutgoingResponse, ResponseOutparam,
+};
+
+struct T;
+
+test_programs::proxy::export!(T);
+
+impl test_programs::proxy::exports::wasi::http::incoming_handler::Guest for T {
+    fn handle(request: IncomingRequest, outparam: ResponseOutparam) {
+        let res = test_programs::http::request(
+            request.method(),
+            request.scheme().unwrap(),
+            request.authority().unwrap().as_str(),
+            request.path_with_query().unwrap().as_str(),
+            None,
+            None,
+            None,
+            None,
+            None,
+        )
+        .unwrap();
+
+        let hdrs = Headers::from_list(&res.headers).unwrap();
+        let resp = OutgoingResponse::new(hdrs);
+        resp.set_status_code(res.status).expect("status code");
+        let body = resp.body().expect("outgoing response");
+
+        ResponseOutparam::set(outparam, Ok(resp));
+
+        let out = body.write().expect("outgoing stream");
+        out.blocking_write_and_flush(res.body.as_ref())
+            .expect("writing response");
+
+        drop(out);
+        OutgoingBody::finish(body, None).expect("outgoing-body.finish");
+    }
+}
+
+// Technically this should not be here for a proxy, but given the current
+// framework for tests it's required since this file is built as a `bin`
+fn main() {}
diff --git a/crates/wasi-http/tests/all/main.rs b/crates/wasi-http/tests/all/main.rs
index c7e526483..68de94195 100644
--- a/crates/wasi-http/tests/all/main.rs
+++ b/crates/wasi-http/tests/all/main.rs
@@ -527,6 +527,27 @@ async fn do_wasi_http_echo(uri: &str, url_header: Option<&str>) -> Result<()> {
     Ok(())
 }
 
+#[test_log::test(tokio::test)]
+// test uses TLS but riscv/s390x don't support that yet
+#[cfg_attr(any(target_arch = "riscv64", target_arch = "s390x"), ignore)]
+async fn wasi_http_without_port() -> Result<()> {
+    let req = hyper::Request::builder()
+        .method(http::Method::GET)
+        .uri("https://httpbin.org/get");
+
+    let response = run_wasi_http(
+        test_programs_artifacts::API_PROXY_FORWARD_REQUEST_COMPONENT,
+        req.body(body::empty())?,
+        None,
+        None,
+    )
+    .await??;
+
+    assert_eq!(response.status(), StatusCode::OK);
+
+    Ok(())
+}
+
 mod body {
     use http_body_util::{combinators::BoxBody, BodyExt, Empty, Full};
     use hyper::body::Bytes;
diff --git a/crates/wasi/tests/all/api.rs b/crates/wasi/tests/all/api.rs
index 96e2837eb..a5b4d2950 100644
--- a/crates/wasi/tests/all/api.rs
+++ b/crates/wasi/tests/all/api.rs
@@ -120,6 +120,11 @@ fn api_proxy() {}
 #[allow(dead_code)]
 fn api_proxy_streaming() {}
 
+// This is tested in the wasi-http crate, but need to satisfy the `foreach_api!`
+// macro above.
+#[allow(dead_code)]
+fn api_proxy_forward_request() {}
+
 wasmtime::component::bindgen!({
     world: "test-reactor",
     async: true,