From 0b89494f62e7e38378123bc7af3ba4ea3a53a0c7 Mon Sep 17 00:00:00 2001 From: Nyannyacha Date: Fri, 31 Jul 2026 01:06:54 +0000 Subject: [PATCH 1/2] fix(ext_node): return errors instead of panicking in ECDH ops `op_node_ecdh_compute_secret` and `op_node_ecdh_compute_public_key` panicked via `.expect()` on malformed keys and `todo!()` on unknown curves, taking down the runtime on user-supplied input. Both ops now return a new `EcdhError`, which is mapped to `TypeError` for JS. Ref: https://github.com/denoland/deno/pull/33751 Co-Authored-By: Claude Opus 5 (1M context) --- deno/runtime/errors.rs | 9 ++++++ ext/node/ops/crypto/mod.rs | 66 ++++++++++++++++++++++---------------- 2 files changed, 47 insertions(+), 28 deletions(-) diff --git a/deno/runtime/errors.rs b/deno/runtime/errors.rs index f206400ae..b32d705b4 100644 --- a/deno/runtime/errors.rs +++ b/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::() .map(node::get_ecdh_encode_pub_key_error) }) + .or_else(|| { + e.downcast_ref::() + .map(node::get_ecdh_error) + }) .or_else(|| { e.downcast_ref::() .map(node::get_diffie_hellman_error) diff --git a/ext/node/ops/crypto/mod.rs b/ext/node/ops/crypto/mod.rs index e90e82090..83afd88bb 100644 --- a/ext/node/ops/crypto/mod.rs +++ b/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, #[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::::from_sec1_bytes( their_pub, ) - .expect("bad public key"); + .map_err(|_| EcdhError::InvalidPublicKey)?; let this_private_key = - elliptic_curve::SecretKey::::from_slice( - &this_priv.expect("must supply private key"), - ) - .expect("bad private key"); + elliptic_curve::SecretKey::::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::::from_sec1_bytes(their_pub) - .expect("bad public key"); - let this_private_key = elliptic_curve::SecretKey::::from_slice( - &this_priv.expect("must supply private key"), - ) - .expect("bad private key"); + .map_err(|_| EcdhError::InvalidPublicKey)?; + let this_private_key = + elliptic_curve::SecretKey::::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::::from_sec1_bytes(their_pub) - .expect("bad public key"); - let this_private_key = elliptic_curve::SecretKey::::from_slice( - &this_priv.expect("must supply private key"), - ) - .expect("bad private key"); + .map_err(|_| EcdhError::InvalidPublicKey)?; + let this_private_key = + elliptic_curve::SecretKey::::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,19 +847,20 @@ pub fn op_node_ecdh_compute_secret( "secp224r1" => { let their_public_key = elliptic_curve::PublicKey::::from_sec1_bytes(their_pub) - .expect("bad public key"); - let this_private_key = elliptic_curve::SecretKey::::from_slice( - &this_priv.expect("must supply private key"), - ) - .expect("bad private key"); + .map_err(|_| EcdhError::InvalidPublicKey)?; + let this_private_key = + elliptic_curve::SecretKey::::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)] @@ -860,38 +868,40 @@ 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::::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::::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::::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::::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] From 693710d58b36965cbc14658a4933af59803ace01 Mon Sep 17 00:00:00 2001 From: Nyannyacha Date: Fri, 31 Jul 2026 03:10:33 +0000 Subject: [PATCH 2/2] fix: flaky tests --- .../test_cases/concurrent-redirect/index.ts | 11 ++- crates/base/tests/integration_tests.rs | 74 ++++++++++++++----- 2 files changed, 63 insertions(+), 22 deletions(-) diff --git a/crates/base/test_cases/concurrent-redirect/index.ts b/crates/base/test_cases/concurrent-redirect/index.ts index 23d9102fc..0d595f37f 100644 --- a/crates/base/test_cases/concurrent-redirect/index.ts +++ b/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); diff --git a/crates/base/tests/integration_tests.rs b/crates/base/tests/integration_tests.rs index be801955f..1e76dfbab 100644 --- a/crates/base/tests/integration_tests.rs +++ 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, +) { + 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::().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(