Harden secret validation and ambiguous sandbox outcomes

Author: NikhilShahi

README.md

@@ -334,6 +334,8 @@ Things to know:
   The sync client checks the limit between requests and caps each request's
   HTTP timeouts at the time left, so a slow response or a runtime token
   refresh can overrun it; use the async client for a strict limit.
+  Secret process output comes from the receiver's retained output, so
+  `max_output_bytes` does not apply; incomplete retained output raises an error.
 - A process start with secrets that times out, or whose result the API can't
   confirm, raises a non-retryable error with code
   `secret_process_outcome_unknown`: the command may be running, so check

hyperbrowser/client/managers/async_manager/sandbox.py

@@ -1180,7 +1180,10 @@ async def _create_detail(
         data = dump_request(normalized, CreateSandboxParams)
         try:
             payload = await self._request("POST", "/sandbox", data=data)
-            return SandboxDetail(**payload)
+            detail = SandboxDetail(**payload)
+            if normalized.secrets and not detail.id.strip():
+                raise ValueError("The API accepted the create but returned no sandbox ID")
+            return detail
         except Exception as error:
             # A create that may have reached the API may be running its
             # startup command with the secrets, so it must not look retryable.

hyperbrowser/client/managers/sync_manager/sandbox.py

@@ -1158,7 +1158,10 @@ def _create_detail(
         data = dump_request(normalized, CreateSandboxParams)
         try:
             payload = self._request("POST", "/sandbox", data=data)
-            return SandboxDetail(**payload)
+            detail = SandboxDetail(**payload)
+            if normalized.secrets and not detail.id.strip():
+                raise ValueError("The API accepted the create but returned no sandbox ID")
+            return detail
         except Exception as error:
             # A create that may have reached the API may be running its
             # startup command with the secrets, so it must not look retryable.

hyperbrowser/models/secret.py

@@ -9,10 +9,11 @@
 SECRET_NAME_PATTERN = re.compile(r"^[a-zA-Z0-9][a-zA-Z0-9_-]{0,127}$")
 SECRET_ENV_NAME_PATTERN = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$")
 MAX_SECRET_VERSION = 2**31 - 1
+MAX_SECRET_BINDINGS = 32
 
 
 def validate_secret_name(name: str) -> str:
-    if not SECRET_NAME_PATTERN.match(name):
+    if not SECRET_NAME_PATTERN.fullmatch(name):
         raise ValueError(
             f"Invalid secret name {name!r}: names are 1-128 letters, digits, "
             "'_' or '-', starting with a letter or digit"
@@ -21,7 +22,11 @@ def validate_secret_name(name: str) -> str:
 
 
 def validate_secret_version(version: int) -> int:
-    if isinstance(version, bool) or not 1 <= version <= MAX_SECRET_VERSION:
+    if (
+        not isinstance(version, int)
+        or isinstance(version, bool)
+        or not 1 <= version <= MAX_SECRET_VERSION
+    ):
         raise ValueError(
             f"Invalid secret version {version!r}: expected 1 to {MAX_SECRET_VERSION}"
         )
@@ -46,10 +51,10 @@ class SecretReference(SecretBaseModel):
     def validate_name(cls, value):
         return validate_secret_name(value)
 
-    @field_validator("version")
+    @field_validator("version", mode="before")
     @classmethod
     def validate_version(cls, value):
-        if isinstance(value, int):
+        if value is not None and value != "latest":
             validate_secret_version(value)
         return value
 
@@ -62,8 +67,18 @@ def normalize_secret_references(value):
     check the environment variable names."""
     if not isinstance(value, dict):
         return value
+    if not value:
+        return None
+    if len(value) > MAX_SECRET_BINDINGS:
+        raise ValueError(
+            f"At most {MAX_SECRET_BINDINGS} secrets can be delivered at once"
+        )
     for env in value:
-        if not isinstance(env, str) or not SECRET_ENV_NAME_PATTERN.match(env):
+        if (
+            not isinstance(env, str)
+            or not SECRET_ENV_NAME_PATTERN.fullmatch(env)
+            or env == "__proto__"
+        ):
             raise ValueError(
                 f"Invalid environment variable name {env!r} for a secret: "
                 "expected letters, digits and '_', not starting with a digit"
@@ -75,10 +90,12 @@ def normalize_secret_references(value):
 
 
 class CreateSecretParams(SecretBaseModel):
+    model_config = ConfigDict(populate_by_name=True, hide_input_in_errors=True)
+
     name: str
     description: Optional[str] = None
     # Encrypted locally and uploaded as the first version; never sent as is.
-    value: Optional[Union[str, bytes]] = Field(default=None, exclude=True)
+    value: Optional[Union[str, bytes]] = Field(default=None, exclude=True, repr=False)
 
     @field_validator("name")
     @classmethod

hyperbrowser/secrets_common.py

@@ -290,7 +290,8 @@ def secret_process_id(process: Any) -> str:
     """A started process's ID; the receiver may send it as ``processId``."""
     if not isinstance(process, dict):
         return ""
-    return str(process.get("id") or process.get("processId") or "").strip()
+    process_id = process.get("id") or process.get("processId")
+    return process_id.strip() if isinstance(process_id, str) else ""
 
 
 def secret_process_failed_error(

tests/test_secrets.py

@@ -626,6 +626,23 @@ def test_a_start_without_a_process_id_is_an_unknown_outcome():
     assert error.value.code == "secret_process_outcome_unknown"
 
 
+@pytest.mark.anyio
+@pytest.mark.parametrize("async_client", [False, True])
+@pytest.mark.parametrize("process_id", [42, True, " "])
+async def test_invalid_process_ids_are_unknown_outcomes(async_client, process_id):
+    manager, http = _sandbox_manager(
+        {("POST", "/sandbox/sbx_123/processes"): (200, {"process": {"id": process_id}})},
+        async_client=async_client,
+    )
+    with pytest.raises(HyperbrowserError) as error:
+        result = manager.start_secret_process("sbx_123", {"command": "x"})
+        if async_client:
+            await result
+    assert error.value.code == "secret_process_outcome_unknown"
+    assert error.value.retryable is False
+    assert len(http.calls) == 1
+
+
 def test_a_timed_out_start_is_an_unknown_outcome_but_a_refused_one_is_not():
     timed_out = httpx.ReadTimeout("timed out")
     manager, _ = _sandbox_manager({("POST", "/sandbox/sbx_123/processes"): timed_out})
@@ -1238,6 +1255,23 @@ def test_a_name_with_hyphens_in_other_places_goes_straight_to_the_name_search():
 CREATE_WITH_SECRETS = {"image_name": "node", "secrets": {"API_KEY": "provider-key"}}
 
 
+@pytest.mark.anyio
+@pytest.mark.parametrize("async_client", [False, True])
+@pytest.mark.parametrize("sandbox_id", ["", " "])
+async def test_empty_sandbox_ids_are_unknown_outcomes(async_client, sandbox_id):
+    manager, http = _sandbox_manager(
+        {("POST", "/sandbox"): (200, dict(SANDBOX_DETAIL_PAYLOAD, id=sandbox_id))},
+        async_client=async_client,
+    )
+    with pytest.raises(HyperbrowserError) as error:
+        result = manager.create(CREATE_WITH_SECRETS)
+        if async_client:
+            await result
+    assert error.value.code == "secret_process_outcome_unknown"
+    assert error.value.retryable is False
+    assert len(http.calls) == 1
+
+
 @pytest.mark.parametrize(
     "outcome",
     [
@@ -1493,20 +1527,20 @@ async def test_use_shell_is_rejected_for_starts_with_secrets(api_type):
 
 
 def test_secret_names_env_names_and_versions_are_checked_locally():
-    for name in ["", "-leading", "has space", "a" * 129]:
+    for name in ["", "-leading", "has space", "a" * 129, "name\n"]:
         with pytest.raises(ValueError, match="Invalid secret name"):
             CreateSecretParams(name=name)
     assert CreateSecretParams(name="a" * 128).name == "a" * 128
 
-    for env in ["1ABC", "API-KEY", ""]:
+    for env in ["1ABC", "API-KEY", "", "__proto__", "KEY\n"]:
         with pytest.raises(ValueError, match="environment variable name"):
             SandboxExecParams(command="x", secrets={env: "provider-key"})
         with pytest.raises(ValueError, match="environment variable name"):
             CreateSandboxParams(image_name="node", secrets={env: "provider-key"})
     with pytest.raises(ValueError, match="Invalid secret name"):
         SandboxExecParams(command="x", secrets={"API_KEY": "bad name"})
 
-    for version in [0, -1, 2**31]:
+    for version in [0, -1, 2**31, True, 1.0, "1"]:
         with pytest.raises(ValueError, match="Invalid secret version"):
             SandboxExecParams(
                 command="x", secrets={"A": {"name": "k", "version": version}}
@@ -1524,7 +1558,7 @@ def test_list_pages_and_version_numbers_are_checked_before_any_request():
     for params in [{"limit": 0}, {"limit": 101}, {"page": 0}]:
         with pytest.raises(ValueError):
             manager.list(params)
-    for version in [0, 2**31, "1"]:
+    for version in [0, 2**31, "1", True, 1.0, 1.5]:
         with pytest.raises(HyperbrowserError) as error:
             manager.delete_version("provider-key", version)
         assert error.value.code == "invalid_secret_version"
@@ -1542,3 +1576,35 @@ async def test_async_version_numbers_are_checked_before_any_request():
     with pytest.raises(ValueError):
         await manager.list({"limit": 101})
     assert http.calls == []
+
+
+def test_secret_request_repr_does_not_expose_the_value():
+    params = CreateSecretParams(name="key", value="dummy-sensitive-value")
+    assert "dummy-sensitive-value" not in repr(params)
+    assert "dummy-sensitive-value" not in str(params)
+    assert "value" not in params.model_dump()
+    assert params.value == "dummy-sensitive-value"
+
+
+def test_secret_request_validation_message_does_not_expose_the_value():
+    with pytest.raises(ValueError) as error:
+        CreateSecretParams.model_validate({"value": "dummy-sensitive-value"})
+    assert "dummy-sensitive-value" not in str(error.value)
+
+
+@pytest.mark.parametrize(
+    "model,params",
+    [
+        (CreateSandboxParams, {"image_name": "node"}),
+        (SandboxExecParams, {"command": "x"}),
+    ],
+)
+def test_secret_bindings_enforce_the_limit_and_omit_empty_maps(model, params):
+    from hyperbrowser.client._request import dump_request
+
+    assert "secrets" not in dump_request(dict(params, secrets={}), model)
+    assert (
+        len(model(**params, secrets={f"KEY_{i}": "k" for i in range(32)}).secrets) == 32
+    )
+    with pytest.raises(ValueError, match="32"):
+        model(**params, secrets={f"KEY_{i}": "k" for i in range(33)})