Repository: bytecodealliance/wasmtime
Pull request #7490 (2023-11-06 19:56:14)

wasi-http: Reject delete of forbidden headers
Disallow `fields.delete` on forbidden headers, and modifies its signature to return a result. As @JakeChampion pointed out, `delete` is sort of a special form of `set`, so we should return a similar result.

The included changes to the api_proxy.rs module and `wasi_http_proxy_tests` test fail on main currently.
<!--
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
-->



Patch:
diff --git a/crates/wasi-http/src/types_impl.rs b/crates/wasi-http/src/types_impl.rs
index a0531a741..89455e3aa 100644
--- a/crates/wasi-http/src/types_impl.rs
+++ b/crates/wasi-http/src/types_impl.rs
@@ -170,15 +170,23 @@ impl<T: WasiHttpView> crate::bindings::http::types::HostFields for T {
         Ok(Ok(()))
     }
 
-    fn delete(&mut self, fields: Resource<HostFields>, name: String) -> wasmtime::Result<()> {
+    fn delete(
+        &mut self,
+        fields: Resource<HostFields>,
+        name: String,
+    ) -> wasmtime::Result<Result<(), types::HeaderError>> {
         let header = match hyper::header::HeaderName::from_bytes(name.as_bytes()) {
             Ok(header) => header,
-            Err(_) => return Ok(()),
+            Err(_) => return Ok(Err(types::HeaderError::InvalidSyntax)),
         };
 
+        if is_forbidden_header(self, &header) {
+            return Ok(Err(types::HeaderError::Forbidden));
+        }
+
         let m = get_fields_mut(self.table(), &fields)?;
         m.remove(header);
-        Ok(())
+        Ok(Ok(()))
     }
 
     fn append(
diff --git a/crates/wasi-http/wit/deps/http/types.wit b/crates/wasi-http/wit/deps/http/types.wit
index e3c06ae9f..fac47f61e 100644
--- a/crates/wasi-http/wit/deps/http/types.wit
+++ b/crates/wasi-http/wit/deps/http/types.wit
@@ -88,8 +88,9 @@ interface types {
     set: func(name: field-key, value: list<field-value>) -> result<_, header-error>;
 
     /// Delete all values for a key. Does nothing if no values for the key
-    /// exist.
-    delete: func(name: field-key);
+    /// exist. Returns and error if the `field-key` is syntactically invalid, or
+    /// if the `field-key` is forbidden.
+    delete: func(name: field-key) -> result<_, header-error>;
 
     /// Append a value for a key. Does not change or delete any existing
     /// values for that key.
@@ -98,7 +99,6 @@ interface types {
     /// the name is forbidden.
     append: func(name: field-key, value: field-value) -> result<_, header-error>;
 
-
     /// Retrieve the full set of keys and values in the Fields. Like the
     /// constructor, the list represents each key-value pair. 
     ///
diff --git a/crates/wasi/wit/deps/http/types.wit b/crates/wasi/wit/deps/http/types.wit
index e3c06ae9f..fac47f61e 100644
--- a/crates/wasi/wit/deps/http/types.wit
+++ b/crates/wasi/wit/deps/http/types.wit
@@ -88,8 +88,9 @@ interface types {
     set: func(name: field-key, value: list<field-value>) -> result<_, header-error>;
 
     /// Delete all values for a key. Does nothing if no values for the key
-    /// exist.
-    delete: func(name: field-key);
+    /// exist. Returns and error if the `field-key` is syntactically invalid, or
+    /// if the `field-key` is forbidden.
+    delete: func(name: field-key) -> result<_, header-error>;
 
     /// Append a value for a key. Does not change or delete any existing
     /// values for that key.
@@ -98,7 +99,6 @@ interface types {
     /// the name is forbidden.
     append: func(name: field-key, value: field-value) -> result<_, header-error>;
 
-
     /// Retrieve the full set of keys and values in the Fields. Like the
     /// constructor, the list represents each key-value pair. 
     ///


Test patch:
diff --git a/crates/test-programs/src/bin/api_proxy.rs b/crates/test-programs/src/bin/api_proxy.rs
index 2170bdbbc..ff36e2519 100644
--- a/crates/test-programs/src/bin/api_proxy.rs
+++ b/crates/test-programs/src/bin/api_proxy.rs
@@ -15,7 +15,22 @@ use bindings::wasi::http::types::{IncomingRequest, ResponseOutparam};
 struct T;
 
 impl bindings::exports::wasi::http::incoming_handler::Guest for T {
-    fn handle(_request: IncomingRequest, outparam: ResponseOutparam) {
+    fn handle(request: IncomingRequest, outparam: ResponseOutparam) {
+        let header = String::from("custom-forbidden-header");
+        let req_hdrs = request.headers();
+
+        assert!(
+            !req_hdrs.get(&header).is_empty(),
+            "missing `custom-forbidden-header` from request"
+        );
+
+        assert!(req_hdrs.delete(&header).is_err());
+
+        assert!(
+            !req_hdrs.get(&header).is_empty(),
+            "delete of forbidden header succeeded"
+        );
+
         let hdrs = bindings::wasi::http::types::Headers::new();
         let resp = bindings::wasi::http::types::OutgoingResponse::new(hdrs);
         let body = resp.body().expect("outgoing response");
diff --git a/crates/wasi-http/tests/all/main.rs b/crates/wasi-http/tests/all/main.rs
index 7a106f4a3..45094d52e 100644
--- a/crates/wasi-http/tests/all/main.rs
+++ b/crates/wasi-http/tests/all/main.rs
@@ -199,11 +199,18 @@ async fn run_wasi_http(
 
 #[test_log::test(tokio::test)]
 async fn wasi_http_proxy_tests() -> anyhow::Result<()> {
-    let req = hyper::Request::builder()
-        .method(http::Method::GET)
-        .body(body::empty())?;
+    let mut req = hyper::Request::builder().method(http::Method::GET);
 
-    let resp = run_wasi_http(test_programs_artifacts::API_PROXY_COMPONENT, req, None).await?;
+    req.headers_mut()
+        .unwrap()
+        .append("custom-forbidden-header", "yes".parse().unwrap());
+
+    let resp = run_wasi_http(
+        test_programs_artifacts::API_PROXY_COMPONENT,
+        req.body(body::empty())?,
+        None,
+    )
+    .await?;
 
     match resp {
         Ok(resp) => println!("response: {resp:?}"),