From ded3fa307fe023de7b4dfe026a3748f0d43ac4e9 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 20:53:38 +0000 Subject: [PATCH] Make the sandbox name pattern per-supervisor, not a mutable global Supervisor.spawn declared `global NAME_PATTERN`, read it, and reset it to DEFAULT_NAME_PATTERN on every successful spawn. Two problems: * A caller that installed a custom pattern silently lost it after one sandbox, and two threads spawning concurrently raced over the value -- one thread's reset could reject the other's legitimate name. * The reset existed only so tests/test_metrics.py could rebind the global without restoring it. Production code was carrying a test affordance. Add a name_pattern argument to Supervisor so the rule belongs to the instance that enforces it, exposed as a .name_pattern property. When it is not given the supervisor falls back to the module-level NAME_PATTERN at spawn time, so the documented global override still works for the process-wide supervisor -- but spawn no longer writes to it. Restoration moves to where it belongs: the metrics test uses monkeypatch.setattr, which undoes itself. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012ebvMQ3vLxdK3joymz6Feg --- pyisolate/supervisor.py | 23 ++++++++++++---- tests/test_metrics.py | 7 +++-- tests/test_supervisor.py | 59 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 8 deletions(-) diff --git a/pyisolate/supervisor.py b/pyisolate/supervisor.py index 3bf9fd8..2b16e00 100644 --- a/pyisolate/supervisor.py +++ b/pyisolate/supervisor.py @@ -33,7 +33,12 @@ logger = logging.getLogger(__name__) -# Allowed sandbox name pattern: alphanumerics, hyphen, underscore +# Allowed sandbox name pattern: alphanumerics, hyphen, underscore. +# +# ``NAME_PATTERN`` is the process-wide default a Supervisor falls back to when +# it was not given its own. Prefer ``Supervisor(name_pattern=...)``: a module +# global cannot express two supervisors with different rules, and rebinding it +# at runtime races with concurrent spawns. DEFAULT_NAME_PATTERN = re.compile(r"^[A-Za-z0-9_-]+$") NAME_PATTERN = DEFAULT_NAME_PATTERN @@ -187,7 +192,12 @@ def __init__( self, warm_pool: int = 0, rollout_mode: str = "dev", + name_pattern: Optional[re.Pattern[str]] = None, ): + # None means "use whatever the module-level default is at spawn time", + # which keeps the documented global override working for the + # process-wide supervisor without this instance owning that decision. + self._name_pattern = name_pattern self._sandboxes: Dict[str, SandboxThread] = {} # Process-backed sandboxes live in a parallel registry: they are not # SandboxThread instances, so the watchdog/warm-pool/cgroup machinery @@ -282,6 +292,11 @@ def _recover_state(self) -> None: cgroup.cleanup_orphans(set()) recovery.cleanup_temp_orphans(set()) + @property + def name_pattern(self) -> "re.Pattern[str]": + """Pattern sandbox names must match, per supervisor.""" + return self._name_pattern if self._name_pattern is not None else NAME_PATTERN + def register_alert_handler(self, callback) -> None: """Subscribe to policy violation alerts.""" self._alerts.register(callback) @@ -311,7 +326,6 @@ def spawn( ``backend="microvm"`` are explicit boundary modes; they are reserved API choices and fail closed until native launchers are available. """ - global NAME_PATTERN backend = _normalize_backend(backend) if backend == "microvm": # Dedicated fail-closed path: probe for a real VMM/KVM boundary and @@ -323,7 +337,7 @@ def spawn( raise ValueError("Sandbox name must be non-empty string") if len(name) > 64: raise ValueError("Sandbox name too long") - pattern = NAME_PATTERN + pattern = self.name_pattern if pattern.fullmatch(name) is None: raise ValueError("Sandbox name contains invalid characters") self._cleanup() @@ -454,9 +468,6 @@ def spawn( raise # Remove references to any terminated sandboxes self._cleanup() - # Reset any temporary overrides of the name validation pattern to avoid - # leaking state across sandboxes. - NAME_PATTERN = DEFAULT_NAME_PATTERN return Sandbox(thread, self) def _apply_kernel_policy(self, cg_path: Any, policy: Any) -> None: diff --git a/tests/test_metrics.py b/tests/test_metrics.py index ae98235..288c33c 100644 --- a/tests/test_metrics.py +++ b/tests/test_metrics.py @@ -91,13 +91,16 @@ def test_export_sandbox_order_is_stable(): sb.close() -def test_export_sanitizes_sandbox_name(): +def test_export_sanitizes_sandbox_name(monkeypatch): name = 'weird "sand\\box\nname' import re import pyisolate.supervisor as supervisor - supervisor.NAME_PATTERN = re.compile(r".+", re.DOTALL) + # monkeypatch restores the default when the test ends. This used to rely on + # Supervisor.spawn resetting the global itself, which meant production code + # carried a test affordance and raced with concurrent spawns. + monkeypatch.setattr(supervisor, "NAME_PATTERN", re.compile(r".+", re.DOTALL)) sb = iso.spawn(name) try: sb.exec("post(1)") diff --git a/tests/test_supervisor.py b/tests/test_supervisor.py index f19080a..0b76e02 100644 --- a/tests/test_supervisor.py +++ b/tests/test_supervisor.py @@ -421,3 +421,62 @@ def fail_update(*_args, **_kwargs): assert sup_replay._tenant_usage.get("acme", 0) == 0 finally: sup_replay.shutdown() + + +# -- sandbox name validation ------------------------------------------------ + + +def test_supervisor_accepts_a_custom_name_pattern(): + import re + + sup = iso.Supervisor(name_pattern=re.compile(r"^ok\.[a-z]+$")) + try: + sb = sup.spawn("ok.alpha") + sb.close() + with pytest.raises(ValueError, match="invalid characters"): + sup.spawn("nope!") + finally: + sup.shutdown() + + +def test_name_pattern_is_per_supervisor_not_global(): + import re + + from pyisolate import supervisor as supervisor_mod + + permissive = iso.Supervisor(name_pattern=re.compile(r".+")) + strict = iso.Supervisor() + try: + sb = permissive.spawn("has.dots.and.stuff") + sb.close() + # A second supervisor keeps the default rules: one supervisor's naming + # policy must not leak into another's. + with pytest.raises(ValueError, match="invalid characters"): + strict.spawn("has.dots.and.stuff") + assert supervisor_mod.NAME_PATTERN is supervisor_mod.DEFAULT_NAME_PATTERN + finally: + permissive.shutdown() + strict.shutdown() + + +def test_spawn_does_not_rebind_the_module_level_pattern(monkeypatch): + # Regression: Supervisor.spawn used to `global NAME_PATTERN` and reset it to + # the default on every successful spawn, so a caller that installed a custom + # pattern silently lost it after one sandbox -- and two threads spawning + # concurrently raced over the value. + import re + + from pyisolate import supervisor as supervisor_mod + + custom = re.compile(r".+", re.DOTALL) + monkeypatch.setattr(supervisor_mod, "NAME_PATTERN", custom) + sup = iso.Supervisor() + try: + sb = sup.spawn("weird.name with spaces") + sb.close() + assert supervisor_mod.NAME_PATTERN is custom + # Still honored on the next spawn rather than reset behind the caller. + sb2 = sup.spawn("another weird one") + sb2.close() + finally: + sup.shutdown()