Incorporate review feedback

crates/test-programs/src/bin/api_proxy.rs

@@ -20,8 +20,7 @@ impl bindings::exports::wasi::http::incoming_handler::Guest for T {
         let resp = bindings::wasi::http::types::OutgoingResponse::new(200, &hdrs);
         let body = resp.write().expect("outgoing response");
 
-        bindings::wasi::http::types::ResponseOutparam::set(outparam, Ok(resp))
-            .expect("sending the response");
+        bindings::wasi::http::types::ResponseOutparam::set(outparam, Ok(resp));
 
         let out = body.write().expect("outgoing stream");
         out.blocking_write_and_flush(b"hello, world!")

crates/test-programs/src/bin/api_proxy_streaming.rs

@@ -57,7 +57,7 @@ async fn handle_request(request: IncomingRequest, response_out: ResponseOutparam
             let mut body =
                 executor::outgoing_body(response.write().expect("response should be writable"));
 
-            ResponseOutparam::set(response_out, Ok(response)).expect("sending the response");
+            ResponseOutparam::set(response_out, Ok(response));
 
             while let Some((url, result)) = results.next().await {
                 let payload = match result {
@@ -86,7 +86,7 @@ async fn handle_request(request: IncomingRequest, response_out: ResponseOutparam
             let mut body =
                 executor::outgoing_body(response.write().expect("response should be writable"));
 
-            ResponseOutparam::set(response_out, Ok(response)).expect("sending the response");
+            ResponseOutparam::set(response_out, Ok(response));
 
             let mut stream =
                 executor::incoming_body(request.consume().expect("request should be readable"));
@@ -112,7 +112,7 @@ async fn handle_request(request: IncomingRequest, response_out: ResponseOutparam
 
             let body = response.write().expect("response should be writable");
 
-            ResponseOutparam::set(response_out, Ok(response)).expect("sending the response");
+            ResponseOutparam::set(response_out, Ok(response));
 
             OutgoingBody::finish(body, None);
         }

crates/test-programs/src/bin/http_outbound_request_invalid_header.rs

@@ -1,24 +1,45 @@
-use test_programs::wasi::http::types::{Headers, Method, Scheme};
+use test_programs::wasi::http::types::{HeaderError, Headers};
 
 fn main() {
     let hdrs = Headers::new(&[]);
-    assert!(hdrs
-        .append(&"malformed header name".to_owned(), &b"bad".to_vec())
-        .is_err());
+    assert!(matches!(
+        hdrs.append(&"malformed header name".to_owned(), &b"ok value".to_vec()),
+        Err(HeaderError::InvalidSyntax)
+    ));
 
-    let addr = std::env::var("HTTP_SERVER").unwrap();
-    let err = test_programs::http::request(
-        Method::Get,
-        Scheme::Http,
-        &addr,
-        "/get?some=arg&goes=here",
-        None,
-        Some(&[("transfer-encoding".to_owned(), b"bad".to_vec())]),
-    )
-    .expect_err("invalid request");
+    assert!(matches!(
+        hdrs.append(&"ok-header-name".to_owned(), &b"ok value".to_vec()),
+        Ok(())
+    ));
 
-    assert_eq!(
-        err.to_string(),
-        "Error::HeaderNameError(\"forbidden header\")"
-    );
+    assert!(matches!(
+        hdrs.append(&"ok-header-name".to_owned(), &b"bad\nvalue".to_vec()),
+        Err(HeaderError::InvalidSyntax)
+    ));
+
+    assert!(matches!(
+        hdrs.append(&"Connection".to_owned(), &b"keep-alive".to_vec()),
+        Err(HeaderError::Forbidden)
+    ));
+
+    assert!(matches!(
+        hdrs.append(&"Keep-Alive".to_owned(), &b"stuff".to_vec()),
+        Err(HeaderError::Forbidden)
+    ));
+
+    assert!(matches!(
+        hdrs.append(
+            &"custom-forbidden-header".to_owned(),
+            &b"keep-alive".to_vec()
+        ),
+        Err(HeaderError::Forbidden)
+    ));
+
+    assert!(matches!(
+        hdrs.append(
+            &"Custom-Forbidden-Header".to_owned(),
+            &b"keep-alive".to_vec()
+        ),
+        Err(HeaderError::Forbidden)
+    ));
 }

crates/wasi-http/src/http_impl.rs

@@ -1,6 +1,6 @@
 use crate::bindings::http::{
     outgoing_handler,
-    types::{self as http_types, RequestOptions, Scheme},
+    types::{RequestOptions, Scheme},
 };
 use crate::types::{self, HostFutureIncomingResponse, OutgoingRequest};
 use crate::WasiHttpView;
@@ -77,12 +77,6 @@ impl<T: WasiHttpView> outgoing_handler::Host for T {
             .header(hyper::header::HOST, &authority);
 
         for (k, v) in req.headers.iter() {
-            if self.is_forbidden_request_header(k) {
-                return Ok(Err(http_types::Error::HeaderNameError(
-                    "forbidden header".to_owned(),
-                )));
-            }
-
             builder = builder.header(k, v);
         }
 

crates/wasi-http/src/types.rs

@@ -67,12 +67,8 @@ pub trait WasiHttpView: Send {
         default_send_request(self, request)
     }
 
-    fn is_forbidden_request_header(&mut self, name: &HeaderName) -> bool {
-        default_is_forbidden_request_header(name)
-    }
-
-    fn is_forbidden_response_header(&mut self, name: &HeaderName) -> bool {
-        default_is_forbidden_response_header(name)
+    fn is_forbidden_header(&mut self, _name: &HeaderName) -> bool {
+        false
     }
 }
 
@@ -176,22 +172,6 @@ pub fn default_send_request(
     Ok(fut)
 }
 
-pub fn default_is_forbidden_request_header(name: &HeaderName) -> bool {
-    use hyper::header;
-
-    const FORBIDDEN: &'static [HeaderName] = &[header::TRANSFER_ENCODING];
-
-    FORBIDDEN.contains(name)
-}
-
-pub fn default_is_forbidden_response_header(name: &HeaderName) -> bool {
-    use hyper::header;
-
-    const FORBIDDEN: &'static [HeaderName] = &[header::TRANSFER_ENCODING];
-
-    FORBIDDEN.contains(name)
-}
-
 pub fn timeout_error(kind: &str) -> anyhow::Error {
     anyhow::anyhow!(crate::bindings::http::types::Error::TimeoutError(format!(
         "{kind} timed out"

crates/wasi-http/src/types_impl.rs

@@ -1,4 +1,6 @@
-use crate::bindings::http::types::{Error, Headers, Method, Scheme, StatusCode, Trailers};
+use crate::bindings::http::types::{
+    Error, HeaderError, Headers, Method, Scheme, StatusCode, Trailers,
+};
 use crate::body::{FinishMessage, HostFutureTrailers, HostFutureTrailersState};
 use crate::types::{HostIncomingRequest, HostOutgoingResponse};
 use crate::WasiHttpView;
@@ -10,6 +12,7 @@ use crate::{
     },
 };
 use anyhow::Context;
+use hyper::header::HeaderName;
 use std::any::Any;
 use wasmtime::component::Resource;
 use wasmtime_wasi::preview2::{
@@ -38,6 +41,22 @@ fn get_fields_mut<'a>(
     }
 }
 
+fn is_forbidden_header<T: WasiHttpView>(view: &mut T, name: &HeaderName) -> bool {
+    static FORBIDDEN_HEADERS: [HeaderName; 9] = [
+        hyper::header::CONNECTION,
+        HeaderName::from_static("keep-alive"),
+        hyper::header::PROXY_AUTHENTICATE,
+        hyper::header::PROXY_AUTHORIZATION,
+        HeaderName::from_static("proxy-connection"),
+        hyper::header::TE,
+        hyper::header::TRANSFER_ENCODING,
+        hyper::header::UPGRADE,
+        HeaderName::from_static("http2-settings"),
+    ];
+
+    FORBIDDEN_HEADERS.contains(name) || view.is_forbidden_header(name)
+}
+
 impl<T: WasiHttpView> crate::bindings::http::types::HostFields for T {
     fn new(&mut self, entries: Vec<(String, Vec<u8>)>) -> wasmtime::Result<Resource<HostFields>> {
         let mut map = hyper::HeaderMap::new();
@@ -81,29 +100,36 @@ impl<T: WasiHttpView> crate::bindings::http::types::HostFields for T {
             .into_iter()
             .map(|val| val.as_bytes().to_owned())
             .collect();
-
         Ok(res)
     }
 
     fn set(
         &mut self,
         fields: Resource<HostFields>,
         name: String,
-        values: Vec<Vec<u8>>,
-    ) -> wasmtime::Result<Result<(), Error>> {
+        byte_values: Vec<Vec<u8>>,
+    ) -> wasmtime::Result<Result<(), HeaderError>> {
         let header = match hyper::header::HeaderName::from_bytes(name.as_bytes()) {
             Ok(header) => header,
-            Err(_) => {
-                return Ok(Err(Error::HeaderNameError(
-                    "invalid header name".to_owned(),
-                )))
-            }
+            Err(_) => return Ok(Err(HeaderError::InvalidSyntax)),
         };
 
-        let m = get_fields_mut(self.table(), &fields)?;
+        if is_forbidden_header(self, &header) {
+            return Ok(Err(HeaderError::Forbidden));
+        }
+
+        let mut values = Vec::with_capacity(byte_values.len());
+        for value in byte_values {
+            match hyper::header::HeaderValue::from_bytes(&value) {
+                Ok(value) => values.push(value),
+                Err(_) => return Ok(Err(HeaderError::InvalidSyntax)),
+            }
+        }
+
+        let m =
+            get_fields_mut(self.table(), &fields).context("[fields_set] getting mutable fields")?;
         m.remove(&header);
         for value in values {
-            let value = hyper::header::HeaderValue::from_bytes(&value)?;
             m.append(&header, value);
         }
 
@@ -126,19 +152,24 @@ impl<T: WasiHttpView> crate::bindings::http::types::HostFields for T {
         fields: Resource<HostFields>,
         name: String,
         value: Vec<u8>,
-    ) -> wasmtime::Result<Result<(), Error>> {
+    ) -> wasmtime::Result<Result<(), HeaderError>> {
         let header = match hyper::header::HeaderName::from_bytes(name.as_bytes()) {
             Ok(header) => header,
-            Err(_) => {
-                return Ok(Err(Error::HeaderNameError(
-                    "invalid header name".to_owned(),
-                )))
-            }
+            Err(_) => return Ok(Err(HeaderError::InvalidSyntax)),
+        };
+
+        if is_forbidden_header(self, &header) {
+            return Ok(Err(HeaderError::Forbidden));
+        }
+
+        let value = match hyper::header::HeaderValue::from_bytes(&value) {
+            Ok(value) => value,
+            Err(_) => return Ok(Err(HeaderError::InvalidSyntax)),
         };
 
         let m = get_fields_mut(self.table(), &fields)
             .context("[fields_append] getting mutable fields")?;
-        let value = hyper::header::HeaderValue::from_bytes(&value)?;
+
         m.append(header, value);
         Ok(Ok(()))
     }
@@ -156,10 +187,9 @@ impl<T: WasiHttpView> crate::bindings::http::types::HostFields for T {
     }
 
     fn clone(&mut self, fields: Resource<HostFields>) -> wasmtime::Result<Resource<HostFields>> {
-        let fields =
-            get_fields_mut(self.table(), &fields).context("[fields_clone] getting fields")?;
-
-        let fields = fields.clone();
+        let fields = get_fields_mut(self.table(), &fields)
+            .context("[fields_clone] getting fields")?
+            .clone();
 
         let id = self
             .table()
@@ -337,32 +367,17 @@ impl<T: WasiHttpView> crate::bindings::http::types::HostResponseOutparam for T {
         &mut self,
         id: Resource<HostResponseOutparam>,
         resp: Result<Resource<HostOutgoingResponse>, Error>,
-    ) -> wasmtime::Result<Result<(), (Resource<HostResponseOutparam>, Error)>> {
+    ) -> wasmtime::Result<()> {
         let val = match resp {
-            Ok(resp) => {
-                let resp: hyper::Response<_> = self.table().delete(resp)?.try_into()?;
-
-                for name in resp.headers().keys() {
-                    if self.is_forbidden_response_header(name) {
-                        return Ok(Err((
-                            id,
-                            Error::HeaderNameError("forbidden header".to_owned()),
-                        )));
-                    }
-                }
-
-                Ok(resp)
-            }
+            Ok(resp) => Ok(self.table().delete(resp)?.try_into()?),
             Err(e) => Err(e),
         };
 
         self.table()
             .delete(id)?
             .result
             .send(val)
-            .map_err(|_| anyhow::anyhow!("failed to initialize response"))?;
-
-        Ok(Ok(()))
+            .map_err(|_| anyhow::anyhow!("failed to initialize response"))
     }
 }
 

crates/wasi-http/tests/all/main.rs

@@ -72,6 +72,10 @@ impl WasiHttpView for Ctx {
             types::default_send_request(self, request)
         }
     }
+
+    fn is_forbidden_header(&mut self, name: &hyper::header::HeaderName) -> bool {
+        name.as_str() == "custom-forbidden-header"
+    }
 }
 
 fn store(engine: &Engine, server: &Server) -> Store<Ctx> {

crates/wasi-http/wit/deps/http/types.wit

@@ -34,7 +34,13 @@ interface types {
     timeout-error(string),
     protocol-error(string),
     unexpected-error(string),
-    header-name-error(string),
+  }
+
+  /// This tyep enumerates the different kinds of errors that may occur when
+  /// setting or appending to a `fields` resource.
+  variant header-error {
+    invalid-syntax,
+    forbidden,
   }
 
   /// Field keys are always strings.
@@ -67,15 +73,21 @@ interface types {
 
     /// Set all of the values for a key. Clears any existing values for that
     /// key, if they have been set.
-    set: func(name: field-key, value: list<field-value>) -> result<_, error>;
+    ///
+    /// The operation can fail if the name or value arguments are invalid, or if
+    /// the name is forbidden.
+    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);
 
     /// Append a value for a key. Does not change or delete any existing
     /// values for that key.
-    append: func(name: field-key, value: field-value) -> result<_, error>;
+    ///
+    /// The operation can fail if the name or value arguments are invalid, or if
+    /// 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
@@ -176,13 +188,11 @@ interface types {
     ///
     /// This method consumes the `response-outparam` to ensure that it is
     /// called at most once. If it is never called, the implementation
-    /// will respond with an error. If there is an error sending the request,
-    /// the error result will contain both a description of the error and the
-    /// `response-outparam`, allowing the `set` operation to be retried.
+    /// will respond with an error.
     set: static func(
       param: response-outparam,
       response: result<outgoing-response, error>,
-    ) -> result<_, tuple<response-outparam, error>>;
+    );
   }
 
   /// This type corresponds to the HTTP standard Status Code.