From e541a3be93624e132e5be9b9f14802384b8fa604 Mon Sep 17 00:00:00 2001 From: Jonathan M Hsieh Date: Wed, 9 Sep 2026 17:33:59 +0000 Subject: [PATCH] fix(secrets): complete the binding and value contract at the Rust boundary `validate` checked only the binding count, so a caller reaching the low-level entry point past `bind_secrets` could register an environment variable name the runtime cannot deliver, or bind a Secret to a name `runtime.env` already sets -- which would resolve by delivery order, with a value visible in the Function's record and a value that is not. Secret values are bounded here too, at the limit the service enforces, so an oversized credential is refused before a request body is built rather than after it has been serialized and uploaded. Sandbox-reserved names are deliberately still the service's alone: that list belongs to the runtime that owns it, and a copy here would drift from it silently. Both gate reproducers now fail closed with no request reaching the service. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UfmeJ533rQDnPBkMtjerV6 --- .../tests/test_first_class_function_slice2.py | 29 +++++++ rust/lancedb/src/connection.rs | 12 +-- rust/lancedb/src/function.rs | 68 ++++++++++++++++ .../tests/first_class_function_slice2.rs | 81 ++++++++++++++++++- 4 files changed, 183 insertions(+), 7 deletions(-) diff --git a/python/python/tests/test_first_class_function_slice2.py b/python/python/tests/test_first_class_function_slice2.py index 6db462ae2..d04a2b5f5 100644 --- a/python/python/tests/test_first_class_function_slice2.py +++ b/python/python/tests/test_first_class_function_slice2.py @@ -179,6 +179,35 @@ def test_a_function_binds_at_most_sixteen_secrets(): assert state["requests"] == [] +def test_binding_names_are_validated_below_the_python_api(): + """The low-level entry point reaches the same validator the typed API does. + + Registration envelopes can be hand-rolled past ``bind_secrets``, so the + grammar and the disjointness rule live in Rust, above the backend. + """ + with _mock_remote_function_catalog() as (host, state): + db = lancedb.connect( + "db://dev", + api_key="fake", + host_override=host, + client_config={"retry_config": {"retries": 0}}, + ) + envelope = json.loads(analyze_caption.registration_request.to_canonical_json()) + envelope["secret_env_bindings"] = { + "BAD=NAME": "openai-prod", + "TOKEN_0": "secret-0", + } + envelope["runtime"]["env"]["TOKEN_0"] = "public" + + async def submit_envelope(): + return await db._conn._inner.create_function_async(json.dumps(envelope)) + + with pytest.raises(ValueError, match="portable"): + LOOP.run(submit_envelope()) + + assert state["requests"] == [] + + _SECRET_DEBUG_LOG_SOURCE = """ import http.server import json diff --git a/rust/lancedb/src/connection.rs b/rust/lancedb/src/connection.rs index 1a10d0386..6f901fa8d 100644 --- a/rust/lancedb/src/connection.rs +++ b/rust/lancedb/src/connection.rs @@ -657,9 +657,9 @@ impl Connection { /// consumer is a Function that binds the Secret by name. Local databases /// return [`Error::NotSupported`]. pub async fn create_secret(&self, name: impl AsRef, value: impl AsRef) -> Result<()> { - self.internal - .create_secret(name.as_ref(), value.as_ref()) - .await + let value = value.as_ref(); + crate::function::validate_secret_value(value)?; + self.internal.create_secret(name.as_ref(), value).await } /// Replace the credential behind an existing Secret. @@ -670,9 +670,9 @@ impl Connection { /// version registered before it. Local databases return /// [`Error::NotSupported`]. pub async fn alter_secret(&self, name: impl AsRef, value: impl AsRef) -> Result<()> { - self.internal - .alter_secret(name.as_ref(), value.as_ref()) - .await + let value = value.as_ref(); + crate::function::validate_secret_value(value)?; + self.internal.alter_secret(name.as_ref(), value).await } /// The names of every Secret in this database. diff --git a/rust/lancedb/src/function.rs b/rust/lancedb/src/function.rs index a9bc343ce..51f6509d0 100644 --- a/rust/lancedb/src/function.rs +++ b/rust/lancedb/src/function.rs @@ -499,6 +499,50 @@ pub struct FunctionArtifactRequest { /// the count needs a bound for the same reason a credential needs a size limit. pub const MAX_FUNCTION_SECRET_ENV_BINDINGS: usize = 16; +/// Largest credential a Secret may hold, matching the limit the service +/// enforces. Bounded because the value is destined for a process environment. +pub const MAX_SECRET_VALUE_BYTES: usize = 64 * 1024; + +/// Whether `name` is a portable POSIX environment variable name. +/// +/// Leading letter or underscore, then letters, digits, or underscores. Names +/// reserved by the execution sandbox are deliberately not checked here: that +/// list belongs to the runtime that owns it, and a copy in the client would +/// drift from it silently. +fn is_portable_env_name(name: &str) -> bool { + let mut bytes = name.bytes(); + bytes + .next() + .is_some_and(|byte| byte == b'_' || byte.is_ascii_alphabetic()) + && bytes.all(|byte| byte == b'_' || byte.is_ascii_alphanumeric()) +} + +/// Reject a credential the service would refuse on size alone. +/// +/// Checked before the request body is built, so an oversized value is never +/// serialized or uploaded. +pub(crate) fn validate_secret_value(value: &str) -> Result<()> { + if value.is_empty() { + return Err(Error::InvalidInput { + message: "a Secret value must not be empty".to_string(), + }); + } + if value.contains('\0') { + return Err(Error::InvalidInput { + message: "a Secret value must not contain NUL".to_string(), + }); + } + if value.len() > MAX_SECRET_VALUE_BYTES { + return Err(Error::InvalidInput { + message: format!( + "a Secret value is at most {MAX_SECRET_VALUE_BYTES} bytes, not {}", + value.len() + ), + }); + } + Ok(()) +} + /// Stable request envelope for remote immutable Function registration. /// /// Credential values deliberately have no field here. The only secret-shaped @@ -532,6 +576,30 @@ impl FunctionRegistrationRequest { ), }); } + for variable in self.secret_env_bindings.keys() { + if !is_portable_env_name(variable) { + return Err(Error::InvalidInput { + message: format!( + "secret_env_bindings key '{variable}' is not a portable \ + environment variable name" + ), + }); + } + // `env` travels with the Function and is readable wherever its + // record is; a bound Secret is not. One name carrying both would + // resolve by delivery order, so refuse rather than pick. + if self + .runtime + .env() + .is_some_and(|env| env.contains_key(variable)) + { + return Err(Error::InvalidInput { + message: format!( + "secret_env_bindings key '{variable}' is already set by runtime.env" + ), + }); + } + } Ok(()) } } diff --git a/rust/lancedb/tests/first_class_function_slice2.rs b/rust/lancedb/tests/first_class_function_slice2.rs index 500028d75..1e7ba1611 100644 --- a/rust/lancedb/tests/first_class_function_slice2.rs +++ b/rust/lancedb/tests/first_class_function_slice2.rs @@ -5,7 +5,9 @@ use std::fs; use std::path::PathBuf; use lancedb::Error; -use lancedb::function::{FunctionRegistrationRequest, MAX_FUNCTION_SECRET_ENV_BINDINGS}; +use lancedb::function::{ + FunctionRegistrationRequest, MAX_FUNCTION_SECRET_ENV_BINDINGS, MAX_SECRET_VALUE_BYTES, +}; use serde_json::Value; fn fixture(name: &str) -> String { @@ -138,3 +140,80 @@ async fn a_function_binds_at_most_sixteen_secrets() { Error::InvalidInput { message } if message.contains("at most 16 secrets") )); } + +/// The binding contract is enforced above the backend in full, not just its +/// count: a caller that skips a language binding still cannot register a name +/// the runtime could not deliver. +#[tokio::test] +async fn binding_names_are_validated_before_dispatch() { + let directory = tempfile::tempdir().unwrap(); + let connection = lancedb::connect(directory.path().to_str().unwrap()) + .execute() + .await + .unwrap(); + + let mut invalid_name = FunctionRegistrationRequest::from_json(&fixture( + "remote_function_registration_request.json", + )) + .unwrap(); + invalid_name.secret_env_bindings = [("BAD=NAME".to_string(), "openai-prod".to_string())].into(); + let error = connection + .create_function_async(invalid_name) + .await + .unwrap_err(); + assert!(matches!( + error, + Error::InvalidInput { message } if message.contains("portable") + )); + + // `env` is readable wherever the Function's record is; a bound Secret is + // not. The same name cannot mean both. + let mut overlapping = FunctionRegistrationRequest::from_json(&fixture( + "remote_function_registration_request.json", + )) + .unwrap(); + let bound = overlapping + .runtime + .env() + .and_then(|env| env.keys().next().cloned()) + .expect("fixture runtime declares env"); + overlapping.secret_env_bindings = [(bound.clone(), "openai-prod".to_string())].into(); + let error = connection + .create_function_async(overlapping) + .await + .unwrap_err(); + assert!(matches!( + error, + Error::InvalidInput { message } if message.contains("already set by runtime.env") + )); +} + +/// An oversized credential is refused before a body is built, so it is never +/// serialized or uploaded to be refused by the service instead. +#[tokio::test] +async fn an_oversized_secret_value_is_refused_before_the_wire() { + let directory = tempfile::tempdir().unwrap(); + let connection = lancedb::connect(directory.path().to_str().unwrap()) + .execute() + .await + .unwrap(); + + for value in ["", &"x".repeat(MAX_SECRET_VALUE_BYTES + 1)] { + let error = connection + .create_secret("openai-prod", value) + .await + .unwrap_err(); + assert!( + matches!(error, Error::InvalidInput { .. }), + "expected InvalidInput, got {error:?}" + ); + } + + // A local database refuses the verb outright, which is what proves the + // size check ran ahead of the backend rather than instead of it. + let error = connection + .create_secret("openai-prod", "x".repeat(MAX_SECRET_VALUE_BYTES)) + .await + .unwrap_err(); + assert!(matches!(error, Error::NotSupported { .. })); +}