fix(ext/node): return errors instead of panicking in ECDH ops

crates/base/test_cases/concurrent-redirect/index.ts

@@ -1,10 +1,15 @@
 // NOTE(Nyannyacha): This is the same test case as described in denoland/deno_core#762, but it is a
 // minimal reproducible sample of what happens in the field.
 //
-// `@1.x` suffixes cause forced redirects for specifiers.
+// The point of this test case is that multiple specifiers of the same module
+// graph are redirected concurrently. Version-less `deno.land/x` specifiers are
+// used because they always answer with a redirect to the latest tag.
+//
+// It used to use `https://lib.deno.dev/x/grammy@1.x/...`, but that host no
+// longer resolves any path, so the redirect never happened in the first place.
 
-import * as A from "https://lib.deno.dev/x/grammy@1.x/mod.ts";
-import * as B from "https://lib.deno.dev/x/grammy@1.x/types.ts";
+import * as A from "https://deno.land/x/grammy/mod.ts";
+import * as B from "https://deno.land/x/grammy/types.ts";
 
 console.log(A, B);
 

crates/base/tests/integration_tests.rs

@@ -1266,16 +1266,7 @@ async fn req_failure_case_op_cancel_from_server_due_to_cpu_resource_limit() {
     120 * MB,
     None,
     |resp| async {
-      let res = resp.unwrap();
-
-      assert_eq!(res.status().as_u16(), 503);
-      assert_eq!(
-        res
-          .headers()
-          .get("x-served-by")
-          .map(|v| v.to_str().unwrap()),
-        Some(concat!(env!("CARGO_PKG_NAME"), "/server"))
-      );
+      assert_op_cancel_from_server_response(resp).await;
     },
   )
   .await;
@@ -1289,19 +1280,64 @@ async fn req_failure_case_op_cancel_from_server_due_to_cpu_resource_limit_2() {
     10 * MB,
     Some("image/png"),
     |resp| async {
-      let res = resp.unwrap();
+      assert_op_cancel_from_server_response(resp).await;
+    },
+  )
+  .await;
+}
 
-      assert_eq!(res.status().as_u16(), 503);
+/// When the supervisor tears down a user worker that exceeded its CPU limit,
+/// two paths race each other and both outcomes are correct:
+///
+/// 1. The connection token is canceled before the main worker's response is
+///    handed back to the server, so the server serves 503 on its own.
+/// 2. The main worker's `WorkerRequestCancelled` handler wins the race, and its
+///    own 500 payload is relayed to the client untouched.
+///
+/// Which one wins depends on scheduling alone, so accept either, but keep
+/// asserting the shape of the response so that unrelated failures (most
+/// notably the request body being detached from its receiver) are still caught.
+async fn assert_op_cancel_from_server_response(
+  resp: Result<Response, reqwest::Error>,
+) {
+  let res = resp.unwrap();
+  let status = res.status().as_u16();
+  let served_by = res
+    .headers()
+    .get("x-served-by")
+    .map(|v| v.to_str().unwrap().to_owned());
+
+  match status {
+    503 => {
       assert_eq!(
-        res
-          .headers()
-          .get("x-served-by")
-          .map(|v| v.to_str().unwrap()),
+        served_by.as_deref(),
         Some(concat!(env!("CARGO_PKG_NAME"), "/server"))
       );
-    },
-  )
-  .await;
+    }
+
+    500 => {
+      assert_eq!(served_by, None);
+
+      let payload = res.json::<ErrorResponsePayload>().await;
+
+      assert!(payload.is_ok());
+
+      let msg = payload.unwrap().msg;
+
+      assert!(
+        !msg.starts_with("TypeError: request body receiver not connected"),
+        "unexpected error message: {msg}"
+      );
+      assert!(
+        msg
+          == "WorkerRequestCancelled: request has been cancelled by supervisor"
+          || msg == "broken pipe",
+        "unexpected error message: {msg}"
+      );
+    }
+
+    _ => panic!("unexpected status code: {status}"),
+  }
 }
 
 async fn test_oak_file_upload<F, R>(

deno/runtime/errors.rs

@@ -1077,6 +1077,7 @@ mod node {
   pub use ext_node::ops::crypto::x509::X509Error;
   pub use ext_node::ops::crypto::DiffieHellmanError;
   pub use ext_node::ops::crypto::EcdhEncodePubKey;
+  pub use ext_node::ops::crypto::EcdhError;
   pub use ext_node::ops::crypto::HkdfError;
   pub use ext_node::ops::crypto::Pbkdf2Error;
   pub use ext_node::ops::crypto::PrivateEncryptDecryptError;
@@ -1528,6 +1529,10 @@ mod node {
     }
   }
 
+  pub fn get_ecdh_error(_: &EcdhError) -> &'static str {
+    "TypeError"
+  }
+
   pub fn get_diffie_hellman_error(_: &DiffieHellmanError) -> &'static str {
     "TypeError"
   }
@@ -1719,6 +1724,10 @@ pub fn get_error_class_name(e: &AnyError) -> Option<&'static str> {
       e.downcast_ref::<node::EcdhEncodePubKey>()
         .map(node::get_ecdh_encode_pub_key_error)
     })
+    .or_else(|| {
+      e.downcast_ref::<node::EcdhError>()
+        .map(node::get_ecdh_error)
+    })
     .or_else(|| {
       e.downcast_ref::<node::DiffieHellmanError>()
         .map(node::get_diffie_hellman_error)

ext/node/ops/crypto/mod.rs

@@ -784,25 +784,34 @@ pub fn op_node_ecdh_generate_keys(
   }
 }
 
+#[derive(Debug, thiserror::Error)]
+pub enum EcdhError {
+  #[error("Public key is not valid for specified curve")]
+  InvalidPublicKey,
+  #[error("Private key is not valid for specified curve")]
+  InvalidPrivateKey,
+  #[error("Unsupported curve")]
+  UnsupportedCurve,
+}
+
 #[op2]
 pub fn op_node_ecdh_compute_secret(
   #[string] curve: &str,
   #[buffer] this_priv: Option<JsBuffer>,
   #[buffer] their_pub: &mut [u8],
   #[buffer] secret: &mut [u8],
-) {
+) -> Result<(), EcdhError> {
+  let this_priv = this_priv.ok_or(EcdhError::InvalidPrivateKey)?;
   match curve {
     "secp256k1" => {
       let their_public_key =
         elliptic_curve::PublicKey::<k256::Secp256k1>::from_sec1_bytes(
           their_pub,
         )
-        .expect("bad public key");
+        .map_err(|_| EcdhError::InvalidPublicKey)?;
       let this_private_key =
-        elliptic_curve::SecretKey::<k256::Secp256k1>::from_slice(
-          &this_priv.expect("must supply private key"),
-        )
-        .expect("bad private key");
+        elliptic_curve::SecretKey::<k256::Secp256k1>::from_slice(&this_priv)
+          .map_err(|_| EcdhError::InvalidPrivateKey)?;
       let shared_secret = elliptic_curve::ecdh::diffie_hellman(
         this_private_key.to_nonzero_scalar(),
         their_public_key.as_affine(),
@@ -812,11 +821,10 @@ pub fn op_node_ecdh_compute_secret(
     "prime256v1" | "secp256r1" => {
       let their_public_key =
         elliptic_curve::PublicKey::<NistP256>::from_sec1_bytes(their_pub)
-          .expect("bad public key");
-      let this_private_key = elliptic_curve::SecretKey::<NistP256>::from_slice(
-        &this_priv.expect("must supply private key"),
-      )
-      .expect("bad private key");
+          .map_err(|_| EcdhError::InvalidPublicKey)?;
+      let this_private_key =
+        elliptic_curve::SecretKey::<NistP256>::from_slice(&this_priv)
+          .map_err(|_| EcdhError::InvalidPrivateKey)?;
       let shared_secret = elliptic_curve::ecdh::diffie_hellman(
         this_private_key.to_nonzero_scalar(),
         their_public_key.as_affine(),
@@ -826,11 +834,10 @@ pub fn op_node_ecdh_compute_secret(
     "secp384r1" => {
       let their_public_key =
         elliptic_curve::PublicKey::<NistP384>::from_sec1_bytes(their_pub)
-          .expect("bad public key");
-      let this_private_key = elliptic_curve::SecretKey::<NistP384>::from_slice(
-        &this_priv.expect("must supply private key"),
-      )
-      .expect("bad private key");
+          .map_err(|_| EcdhError::InvalidPublicKey)?;
+      let this_private_key =
+        elliptic_curve::SecretKey::<NistP384>::from_slice(&this_priv)
+          .map_err(|_| EcdhError::InvalidPrivateKey)?;
       let shared_secret = elliptic_curve::ecdh::diffie_hellman(
         this_private_key.to_nonzero_scalar(),
         their_public_key.as_affine(),
@@ -840,58 +847,61 @@ pub fn op_node_ecdh_compute_secret(
     "secp224r1" => {
       let their_public_key =
         elliptic_curve::PublicKey::<NistP224>::from_sec1_bytes(their_pub)
-          .expect("bad public key");
-      let this_private_key = elliptic_curve::SecretKey::<NistP224>::from_slice(
-        &this_priv.expect("must supply private key"),
-      )
-      .expect("bad private key");
+          .map_err(|_| EcdhError::InvalidPublicKey)?;
+      let this_private_key =
+        elliptic_curve::SecretKey::<NistP224>::from_slice(&this_priv)
+          .map_err(|_| EcdhError::InvalidPrivateKey)?;
       let shared_secret = elliptic_curve::ecdh::diffie_hellman(
         this_private_key.to_nonzero_scalar(),
         their_public_key.as_affine(),
       );
       secret.copy_from_slice(shared_secret.raw_secret_bytes());
     }
-    &_ => todo!(),
+    _ => return Err(EcdhError::UnsupportedCurve),
   }
+
+  Ok(())
 }
 
 #[op2(fast)]
 pub fn op_node_ecdh_compute_public_key(
   #[string] curve: &str,
   #[buffer] privkey: &[u8],
   #[buffer] pubkey: &mut [u8],
-) {
+) -> Result<(), EcdhError> {
   match curve {
     "secp256k1" => {
       let this_private_key =
         elliptic_curve::SecretKey::<k256::Secp256k1>::from_slice(privkey)
-          .expect("bad private key");
+          .map_err(|_| EcdhError::InvalidPrivateKey)?;
       let public_key = this_private_key.public_key();
       pubkey.copy_from_slice(public_key.to_sec1_bytes().as_ref());
     }
     "prime256v1" | "secp256r1" => {
       let this_private_key =
         elliptic_curve::SecretKey::<NistP256>::from_slice(privkey)
-          .expect("bad private key");
+          .map_err(|_| EcdhError::InvalidPrivateKey)?;
       let public_key = this_private_key.public_key();
       pubkey.copy_from_slice(public_key.to_sec1_bytes().as_ref());
     }
     "secp384r1" => {
       let this_private_key =
         elliptic_curve::SecretKey::<NistP384>::from_slice(privkey)
-          .expect("bad private key");
+          .map_err(|_| EcdhError::InvalidPrivateKey)?;
       let public_key = this_private_key.public_key();
       pubkey.copy_from_slice(public_key.to_sec1_bytes().as_ref());
     }
     "secp224r1" => {
       let this_private_key =
         elliptic_curve::SecretKey::<NistP224>::from_slice(privkey)
-          .expect("bad private key");
+          .map_err(|_| EcdhError::InvalidPrivateKey)?;
       let public_key = this_private_key.public_key();
       pubkey.copy_from_slice(public_key.to_sec1_bytes().as_ref());
     }
-    &_ => todo!(),
+    _ => return Err(EcdhError::UnsupportedCurve),
   }
+
+  Ok(())
 }
 
 #[inline]