fix: fix bugs with rendering of authority in serve urls (#29009)

- preserve authority from protocol
- reject some invalid combinations of request lines (e.g. `GET *`)
- modify rendering of OPTIONS and CONNECT so that they don't cause `new
URL` to raise.

ext/http/00_serve.ts

@@ -295,32 +295,24 @@ class InnerRequest {
       this.#methodAndUri = op_http_get_request_method_and_url(this.#external);
     }
 
+    const method = this.#methodAndUri[0];
+    const scheme = this.#methodAndUri[5] !== undefined
+      ? `${this.#methodAndUri[5]}://`
+      : this.#context.scheme;
+    const authority = this.#methodAndUri[1] ?? this.#context.fallbackHost;
     const path = this.#methodAndUri[2];
 
     // * is valid for OPTIONS
-    if (path === "*") {
-      return (this.#urlValue = "*");
-    }
-
-    // If the path is empty, return the authority (valid for CONNECT)
-    if (path == "") {
-      return (this.#urlValue = this.#methodAndUri[1]);
+    if (method === "OPTIONS" && path === "*") {
+      return (this.#urlValue = scheme + authority + "/" + path);
     }
 
     // CONNECT requires an authority
-    if (this.#methodAndUri[0] == "CONNECT") {
-      return (this.#urlValue = this.#methodAndUri[1]);
+    if (method === "CONNECT") {
+      return (this.#urlValue = scheme + this.#methodAndUri[1]);
     }
 
-    const hostname = this.#methodAndUri[1];
-    if (hostname) {
-      // Construct a URL from the scheme, the hostname, and the path
-      return (this.#urlValue = this.#context.scheme + hostname + path);
-    }
-
-    // Construct a URL from the scheme, the fallback hostname, and the path
-    return (this.#urlValue = this.#context.scheme + this.#context.fallbackHost +
-      path);
+    return this.#urlValue = scheme + authority + path;
   }
 
   get completed() {

ext/http/http_next.rs

@@ -344,10 +344,10 @@ where
   .unwrap()
   .into();
 
-  let authority: v8::Local<v8::Value> = match request_properties.authority {
-    Some(authority) => v8::String::new_from_utf8(
+  let scheme: v8::Local<v8::Value> = match request_parts.uri.scheme_str() {
+    Some(scheme) => v8::String::new_from_utf8(
       scope,
-      authority.as_bytes(),
+      scheme.as_bytes(),
       v8::NewStringType::Normal,
     )
     .unwrap()
@@ -355,6 +355,27 @@ where
     None => v8::undefined(scope).into(),
   };
 
+  let authority: v8::Local<v8::Value> =
+    if let Some(authority) = request_parts.uri.authority() {
+      v8::String::new_from_utf8(
+        scope,
+        authority.as_str().as_ref(),
+        v8::NewStringType::Normal,
+      )
+      .unwrap()
+      .into()
+    } else if let Some(authority) = request_properties.authority {
+      v8::String::new_from_utf8(
+        scope,
+        authority.as_bytes(),
+        v8::NewStringType::Normal,
+      )
+      .unwrap()
+      .into()
+    } else {
+      v8::undefined(scope).into()
+    };
+
   // Only extract the path part - we handle authority elsewhere
   let path = match request_parts.uri.path_and_query() {
     Some(path_and_query) => {
@@ -389,7 +410,7 @@ where
     None => v8::undefined(scope).into(),
   };
 
-  let vec = [method, authority, path, peer_address, port];
+  let vec = [method, authority, path, peer_address, port, scheme];
   v8::Array::new_with_elements(scope, vec.as_slice())
 }
 

ext/http/request_properties.rs

@@ -282,14 +282,6 @@ fn req_host<'a>(
   addr_type: NetworkStreamType,
   port: u32,
 ) -> Option<Cow<'a, str>> {
-  // Unix sockets always use the socket address
-  #[cfg(unix)]
-  if addr_type == NetworkStreamType::Unix
-    || addr_type == NetworkStreamType::Vsock
-  {
-    return None;
-  }
-
   // It is rare that an authority will be passed, but if it does, it takes priority
   if let Some(auth) = uri.authority() {
     match addr_type {

ext/http/service.rs

@@ -182,6 +182,25 @@ impl Drop for HttpRequestBodyAutocloser {
   }
 }
 
+#[allow(clippy::collapsible_if)] // for logic clarity
+fn validate_request(req: &Request) -> bool {
+  if req.uri() == "*" {
+    if req.method() != http::Method::OPTIONS {
+      return false;
+    }
+  } else if req.uri().path().is_empty() {
+    if req.method() != http::Method::CONNECT {
+      return false;
+    }
+  }
+
+  if req.method() == http::Method::CONNECT && req.uri().authority().is_none() {
+    return false;
+  }
+
+  true
+}
+
 pub(crate) async fn handle_request(
   request: Request,
   request_info: HttpConnectionProperties,
@@ -189,6 +208,13 @@ pub(crate) async fn handle_request(
   tx: tokio::sync::mpsc::Sender<Rc<HttpRecord>>,
   legacy_abort: bool,
 ) -> Result<Response, hyper_v014::Error> {
+  if !validate_request(&request) {
+    let mut response = Response::new(HttpRecordResponse(None));
+    *response.version_mut() = request.version();
+    *response.status_mut() = http::StatusCode::BAD_REQUEST;
+    return Ok(response);
+  }
+
   let otel_info = if let Some(otel) = deno_telemetry::OTEL_GLOBALS
     .get()
     .filter(|o| o.has_metrics())
@@ -510,7 +536,7 @@ impl HttpRecord {
   /// Take the response.
   fn into_response(self: Rc<Self>) -> Response {
     let parts = self.self_mut().response_parts.take().unwrap();
-    let body = HttpRecordResponse(ManuallyDrop::new(self));
+    let body = HttpRecordResponse(Some(ManuallyDrop::new(self)));
     Response::from_parts(parts, body)
   }
 
@@ -592,8 +618,10 @@ impl HttpRecord {
   }
 }
 
+// `None` variant used when no body is present, for example
+// when we want to return a synthetic 400 for invalid requests.
 #[repr(transparent)]
-pub struct HttpRecordResponse(ManuallyDrop<Rc<HttpRecord>>);
+pub struct HttpRecordResponse(Option<ManuallyDrop<Rc<HttpRecord>>>);
 
 impl Body for HttpRecordResponse {
   type Data = BufView;
@@ -604,7 +632,9 @@ impl Body for HttpRecordResponse {
     cx: &mut Context<'_>,
   ) -> Poll<Option<Result<Frame<Self::Data>, Self::Error>>> {
     use crate::response_body::PollFrame;
-    let record = &self.0;
+    let Some(record) = &self.0 else {
+      return Poll::Ready(None);
+    };
 
     let res = loop {
       let mut inner = record.self_mut();
@@ -648,7 +678,7 @@ impl Body for HttpRecordResponse {
     }
 
     if let ResponseStreamResult::NonEmptyBuf(buf) = &res {
-      let mut http = self.0 .0.borrow_mut();
+      let mut http = record.0.borrow_mut();
       if let Some(otel_info) = &mut http.as_mut().unwrap().otel_info {
         if let Some(response_size) = &mut otel_info.response_size {
           *response_size += buf.len() as u64;
@@ -660,7 +690,10 @@ impl Body for HttpRecordResponse {
   }
 
   fn is_end_stream(&self) -> bool {
-    let inner = self.0.self_ref();
+    let Some(record) = &self.0 else {
+      return true;
+    };
+    let inner = record.self_ref();
     matches!(
       inner.response_body,
       ResponseBytesInner::Done | ResponseBytesInner::Empty
@@ -668,16 +701,22 @@ impl Body for HttpRecordResponse {
   }
 
   fn size_hint(&self) -> SizeHint {
+    let Some(record) = &self.0 else {
+      return SizeHint::with_exact(0);
+    };
     // The size hint currently only used in the case where it is exact bounds in hyper, but we'll pass it through
     // anyways just in case hyper needs it.
-    self.0.self_ref().response_body.size_hint()
+    record.self_ref().response_body.size_hint()
   }
 }
 
 impl Drop for HttpRecordResponse {
   fn drop(&mut self) {
+    let Some(record) = &mut self.0 else {
+      return;
+    };
     // SAFETY: this ManuallyDrop is not used again.
-    let record = unsafe { ManuallyDrop::take(&mut self.0) };
+    let record = unsafe { ManuallyDrop::take(record) };
     http_trace!(record, "HttpRecordResponse::drop");
     record.finish();
   }