feat(runtime,wpt): improve headers conformance (#622)

lagonapp · Feb 26, 2023 · c0cd90f · c0cd90f · vercel · Feb 26, 2023
1 parent 82621bd
commit c0cd90f
Show file tree

Hide file tree

Showing 12 changed files with 108 additions and 43 deletions.
diff --git a/.changeset/clever-hairs-stare.md b/.changeset/clever-hairs-stare.md
@@ -0,0 +1,7 @@
+---
+'@lagon/cli': patch
+'@lagon/runtime': patch
+'@lagon/serverless': patch
+---
+
+Support multiple request/response headers for set-cookie
diff --git a/.changeset/fair-years-sleep.md b/.changeset/fair-years-sleep.md
@@ -0,0 +1,6 @@
+---
+'@lagon/wpt-runner': patch
+'@lagon/js-runtime': patch
+---
+
+Improve getSetCookie conformance
diff --git a/crates/cli/src/commands/dev.rs b/crates/cli/src/commands/dev.rs
@@ -6,7 +6,7 @@ use hyper::server::conn::AddrStream;
 use hyper::service::{make_service_fn, service_fn};
 use hyper::{Body, Request as HyperRequest, Response as HyperResponse, Server};
 use lagon_runtime::{options::RuntimeOptions, Runtime};
-use lagon_runtime_http::{Request, Response, RunResult};
+use lagon_runtime_http::{Request, Response, RunResult, X_FORWARDED_FOR};
 use lagon_runtime_isolate::{options::IsolateOptions, Isolate};
 use lagon_runtime_utils::assets::{find_asset, handle_asset};
 use lagon_runtime_utils::response::{handle_response, ResponseEvent, FAVICON_URL};
@@ -115,7 +115,7 @@ async fn handle_request(
     } else {
         match Request::from_hyper(req).await {
             Ok(mut request) => {
-                request.add_header("X-Forwarded-For".into(), ip);
+                request.set_header(X_FORWARDED_FOR.to_string(), ip);
 
                 pool.spawn_pinned_by_idx(
                     move || async move {

diff --git a/crates/runtime/tests/runtime.rs b/crates/runtime/tests/runtime.rs
@@ -194,7 +194,7 @@ async fn get_headers() {
     );
 
     let mut headers = HashMap::new();
-    headers.insert("x-auth".into(), "token".into());
+    headers.insert("x-auth".into(), vec!["token".into()]);
 
     let (tx, rx) = flume::unbounded();
     isolate
@@ -234,8 +234,8 @@ async fn return_headers() {
     );
 
     let mut headers = HashMap::new();
-    headers.insert("content-type".into(), "text/html".into());
-    headers.insert("x-test".into(), "test".into());
+    headers.insert("content-type".into(), vec!["text/html".into()]);
+    headers.insert("x-test".into(), vec!["test".into()]);
 
     let (tx, rx) = flume::unbounded();
     isolate.run(Request::default(), tx).await;
@@ -269,8 +269,8 @@ async fn return_headers_from_headers_api() {
     );
 
     let mut headers = HashMap::new();
-    headers.insert("content-type".into(), "text/html".into());
-    headers.insert("x-test".into(), "test".into());
+    headers.insert("content-type".into(), vec!["text/html".into()]);
+    headers.insert("x-test".into(), vec!["test".into()]);
 
     let (tx, rx) = flume::unbounded();
     isolate.run(Request::default(), tx).await;

diff --git a/crates/runtime/tests/streams.rs b/crates/runtime/tests/streams.rs
@@ -136,7 +136,7 @@ async fn custom_response() {
         RunResult::Stream(StreamResult::Done)
     );
     let mut headers = HashMap::new();
-    headers.insert("x-lagon".into(), "test".into());
+    headers.insert("x-lagon".into(), vec!["test".into()]);
     assert_eq!(
         rx.recv_async().await.unwrap(),
         RunResult::Stream(StreamResult::Start(Response {

diff --git a/crates/runtime_http/src/request.rs b/crates/runtime_http/src/request.rs
@@ -16,7 +16,7 @@ use super::{FromV8, IntoV8, Method};
 
 #[derive(Debug)]
 pub struct Request {
-    pub headers: Option<HashMap<String, String>>,
+    pub headers: Option<HashMap<String, Vec<String>>>,
     pub method: Method,
     pub body: Bytes,
     pub url: String,
@@ -139,7 +139,10 @@ impl TryFrom<&Request> for http::request::Builder {
 
         if let Some(headers) = &request.headers {
             for (key, value) in headers {
-                builder_headers.insert(HeaderName::from_str(key)?, HeaderValue::from_str(value)?);
+                for value in value {
+                    builder_headers
+                        .append(HeaderName::from_str(key)?, HeaderValue::from_str(value)?);
+                }
             }
         }
 
@@ -158,19 +161,22 @@ impl Request {
     }
 
     pub async fn from_hyper(request: HyperRequest<Body>) -> Result<Self> {
-        let mut headers = HashMap::new();
+        let mut headers = HashMap::<String, Vec<String>>::new();
 
         for (key, value) in request.headers().iter() {
             if key != X_LAGON_ID {
-                headers.insert(key.to_string(), value.to_str().unwrap().to_string());
+                headers
+                    .entry(key.to_string())
+                    .or_default()
+                    .push(value.to_str()?.to_string());
             }
         }
 
         let method = Method::from(request.method());
-        let host = headers
-            .get("host")
-            .map(|host| host.to_string())
-            .unwrap_or_default();
+        let host = headers.get("host").map_or_else(String::new, |host| {
+            host.get(0)
+                .map_or_else(String::new, |value| value.to_string())
+        });
         let url = format!("http://{}{}", host, request.uri().to_string().as_str());
 
         let body = body::to_bytes(request.into_body()).await?;
@@ -187,9 +193,9 @@ impl Request {
         })
     }
 
-    pub fn add_header(&mut self, key: String, value: String) {
+    pub fn set_header(&mut self, key: String, value: String) {
         if let Some(ref mut headers) = self.headers {
-            headers.insert(key, value);
+            headers.insert(key, vec![value]);
         }
     }
 }
diff --git a/crates/runtime_http/src/response.rs b/crates/runtime_http/src/response.rs
@@ -17,7 +17,7 @@ static READABLE_STREAM_STR: &[u8] = b"[object ReadableStream]";
 
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct Response {
-    pub headers: Option<HashMap<String, String>>,
+    pub headers: Option<HashMap<String, Vec<String>>>,
     pub body: Bytes,
     pub status: u16,
 }
@@ -139,7 +139,10 @@ impl TryFrom<&Response> for http::response::Builder {
 
         if let Some(headers) = &response.headers {
             for (key, value) in headers {
-                builder_headers.insert(HeaderName::from_str(key)?, HeaderValue::from_str(value)?);
+                for value in value {
+                    builder_headers
+                        .append(HeaderName::from_str(key)?, HeaderValue::from_str(value)?);
+                }
             }
         }
 
@@ -162,10 +165,13 @@ impl Response {
     }
 
     pub async fn from_hyper(response: HyperResponse<Body>) -> Result<Self> {
-        let mut headers = HashMap::new();
+        let mut headers = HashMap::<String, Vec<String>>::new();
 
         for (key, value) in response.headers().iter() {
-            headers.insert(key.to_string(), value.to_str().unwrap().to_string());
+            headers
+                .entry(key.to_string())
+                .or_default()
+                .push(value.to_str()?.to_string());
         }
 
         let status = response.status().as_u16();

diff --git a/crates/runtime_utils/src/assets.rs b/crates/runtime_utils/src/assets.rs
@@ -40,7 +40,7 @@ pub fn handle_asset(root: PathBuf, asset: &String) -> Result<Response> {
     );
 
     let mut headers = HashMap::new();
-    headers.insert("content-type".into(), content_type.into());
+    headers.insert("content-type".into(), vec![content_type.into()]);
 
     Ok(Response {
         status: 200,

diff --git a/crates/runtime_v8_utils/src/lib.rs b/crates/runtime_v8_utils/src/lib.rs
@@ -24,7 +24,7 @@ pub fn extract_v8_integer(value: v8::Local<v8::Value>, scope: &mut v8::HandleSco
 pub fn extract_v8_headers_object(
     value: v8::Local<v8::Value>,
     scope: &mut v8::HandleScope,
-) -> Result<Option<HashMap<String, String>>> {
+) -> Result<Option<HashMap<String, Vec<String>>>> {
     if !value.is_map() {
         return Err(anyhow!("Value is not of type 'Map'"));
     }
@@ -42,13 +42,37 @@ pub fn extract_v8_headers_object(
 
             let key = headers_keys
                 .get_index(scope, index)
-                .map_or_else(|| "".to_string(), |key| key.to_rust_string_lossy(scope));
+                .map_or_else(String::new, |key| key.to_rust_string_lossy(scope));
+
             index += 1;
-            let value = headers_keys
+
+            let values = headers_keys
                 .get_index(scope, index)
-                .map_or_else(|| "".to_string(), |value| value.to_rust_string_lossy(scope));
+                .map_or_else(Vec::new, |value| {
+                    let mut result = Vec::new();
+
+                    if value.is_array() {
+                        let values = unsafe { v8::Local::<v8::Array>::cast(value) };
+
+                        for i in 0..values.length() {
+                            let value = values
+                                .get_index(scope, i)
+                                .map_or_else(String::new, |value| {
+                                    value.to_rust_string_lossy(scope)
+                                });
+
+                            result.push(value);
+                        }
+                    } else {
+                        let value = value.to_rust_string_lossy(scope);
 
-            headers.insert(key, value);
+                        result.push(value);
+                    }
+
+                    result
+                });
+
+            headers.insert(key, values);
         }
 
         return Ok(Some(headers));
@@ -95,15 +119,21 @@ pub fn v8_uint8array<'a>(
 
 pub fn v8_headers_object<'a>(
     scope: &mut v8::HandleScope<'a>,
-    value: HashMap<String, String>,
+    value: HashMap<String, Vec<String>>,
 ) -> v8::Local<'a, v8::Object> {
     let headers = v8::Object::new(scope);
 
-    for (key, value) in value.iter() {
+    for (key, values) in value.iter() {
         let key = v8_string(scope, key);
-        let value = v8_string(scope, value);
 
-        headers.set(scope, key.into(), value.into());
+        let array = v8::Array::new(scope, values.len() as i32);
+
+        for (i, value) in values.iter().enumerate() {
+            let value = v8_string(scope, value);
+            array.set_index(scope, i as u32, value.into());
+        }
+
+        headers.set(scope, key.into(), array.into());
     }
 
     headers

diff --git a/crates/serverless/src/main.rs b/crates/serverless/src/main.rs
@@ -189,11 +189,15 @@ async fn handle_request(
                 let ip = request
                     .headers
                     .as_ref()
-                    .map_or(&ip, |headers| headers.get(X_REAL_IP).unwrap_or(&ip))
+                    .map_or(&ip, |headers| {
+                        headers
+                            .get(X_REAL_IP)
+                            .map_or(&ip, |x_real_ip| x_real_ip.get(0).unwrap_or(&ip))
+                    })
                     .to_string();
 
-                request.add_header(X_FORWARDED_FOR.to_string(), ip);
-                request.add_header(X_LAGON_REGION.to_string(), REGION.to_string());
+                request.set_header(X_FORWARDED_FOR.to_string(), ip);
+                request.set_header(X_LAGON_REGION.to_string(), REGION.to_string());
 
                 let thread_id = get_thread_id(thread_ids, &hostname).await;
 

diff --git a/crates/wpt-runner/current-results.md b/crates/wpt-runner/current-results.md
@@ -1,14 +1,14 @@
 Running ../../tools/wpt/fetch/api/headers/header-setcookie.any.js
 TEST DONE 0 Headers.prototype.get combines set-cookie headers in order
-TEST DONE 1 Headers iterator does not combine set-cookie headers
+TEST DONE 0 Headers iterator does not combine set-cookie headers
 TEST DONE 0 Headers iterator does not special case set-cookie2 headers
-TEST DONE 1 Headers iterator does not combine set-cookie & set-cookie2 headers
-TEST DONE 1 Headers iterator preserves set-cookie ordering
-TEST DONE 1 Headers iterator preserves per header ordering, but sorts keys alphabetically
-TEST DONE 1 Headers iterator preserves per header ordering, but sorts keys alphabetically (and ignores value ordering)
+TEST DONE 0 Headers iterator does not combine set-cookie & set-cookie2 headers
+TEST DONE 0 Headers iterator preserves set-cookie ordering
+TEST DONE 0 Headers iterator preserves per header ordering, but sorts keys alphabetically
+TEST DONE 0 Headers iterator preserves per header ordering, but sorts keys alphabetically (and ignores value ordering)
 TEST DONE 1 Headers iterator is correctly updated with set-cookie changes
 TEST DONE 0 Headers.prototype.has works for set-cookie
-TEST DONE 1 Headers.prototype.append works for set-cookie
+TEST DONE 0 Headers.prototype.append works for set-cookie
 TEST DONE 0 Headers.prototype.set works for set-cookie
 TEST DONE 0 Headers.prototype.delete works for set-cookie
 TEST DONE 0 Headers.prototype.getSetCookie with no headers present
@@ -1700,5 +1700,5 @@ Running ../../tools/wpt/urlpattern/urlpattern-compare.https.any.js
 Running ../../tools/wpt/urlpattern/urlpattern.any.js
 Running ../../tools/wpt/urlpattern/urlpattern.https.any.js
 
-1576 tests, 450 passed, 1117 failed
+1576 tests, 456 passed, 1111 failed
  -> 28% conformance
diff --git a/packages/js-runtime/src/runtime/http/Headers.ts b/packages/js-runtime/src/runtime/http/Headers.ts
@@ -77,7 +77,13 @@
       const sorted = [...this.h.entries()].sort(([a], [b]) => a.localeCompare(b));
 
       for (const [key, values] of sorted) {
-        yield [key, values.join(', ')];
+        if (key == SET_COOKIE) {
+          for (const value of values) {
+            yield [key, value];
+          }
+        } else {
+          yield [key, values.join(', ')];
+        }
       }
     }