From 6530c7a132e4400bf7a6a94a569c11ef97ecf96f Mon Sep 17 00:00:00 2001 From: Bas Alberts Date: Fri, 18 Sep 2026 11:26:56 -0400 Subject: [PATCH 1/4] Mount CONTAINER_WORKSPACE read-only by default The workspace bind mount is writable, so a command run via container_shell_exec can modify the host's copy of the tree under audit. That tree is usually the source being analyzed, and callers commonly collect it afterwards as an artifact. A writable mount therefore makes the collected artifact agent-influenced rather than a faithful copy of the input, which is the wrong default for a tool whose whole purpose is examining untrusted code. The shell is also the one surface an injected prompt can reach, so the capability is worth narrowing even though nothing exercises it today. Adds CONTAINER_WORKSPACE_MODE, defaulting to "ro", with "rw" as an explicit opt-in for any taskflow that genuinely needs to write. This follows the CONTAINER_NETWORK precedent in this module exactly: secure by default, opt-in to widen, and an empty or unrecognized value falls back to the safe value so the default cannot be silently weakened by a blank variable. Only a literal "rw" (case-insensitive, whitespace-trimmed) opts in. The mode is also folded into the persistent-container key material, for the same reason the network mode already is: a run configured for "ro" must not reuse a container that was created with a writable workspace. Evidence that read-only is a safe default rather than a guess: across a full audit run of the six taskflows that use this toolbox, all 144 container_shell_exec calls were reads (tree, cat, ls, grep, rg, wc, git log, sed -n). None wrote to /workspace, and no taskflow prompt instructs a write. Tests: read-only default and rw opt-in at the docker run layer, env parsing including blank/unrecognized/injection-shaped values, and persistent-name separation between modes. Verified the new assertions fail when the mount mode is removed, so they detect the regression rather than merely passing. Note test_start_container_success changed: it asserts membership in the argument list, which is exact-element rather than substring, so it needed the new suffix. --- .../mcp_servers/container_shell.py | 28 +++++++- tests/test_container_shell.py | 69 ++++++++++++++++++- 2 files changed, 93 insertions(+), 4 deletions(-) diff --git a/src/seclab_taskflows/mcp_servers/container_shell.py b/src/seclab_taskflows/mcp_servers/container_shell.py index 81f863e..d167fcd 100644 --- a/src/seclab_taskflows/mcp_servers/container_shell.py +++ b/src/seclab_taskflows/mcp_servers/container_shell.py @@ -8,6 +8,12 @@ - ``CONTAINER_IMAGE`` — image to run (required). - ``CONTAINER_WORKSPACE`` — host path bind-mounted at ``/workspace`` (optional). +- ``CONTAINER_WORKSPACE_MODE`` — bind-mount mode for that path, ``ro`` (default) + or ``rw``. Defaults to read-only so a command run inside the container cannot + modify the host's copy of the source under audit. Set it to ``rw`` only if a + taskflow genuinely needs to write into the workspace. An empty, unset, or + unrecognized value falls back to ``ro``, so the default cannot be silently + weakened by a blank variable. - ``CONTAINER_TIMEOUT`` — default per-command timeout in seconds (default 30). - ``CONTAINER_PERSIST`` — reuse a deterministic container across runs when truthy. - ``CONTAINER_PERSIST_KEY`` — extra key to distinguish persistent containers. @@ -66,6 +72,17 @@ CONTAINER_IMAGE = os.environ.get("CONTAINER_IMAGE", "") CONTAINER_WORKSPACE = os.environ.get("CONTAINER_WORKSPACE", "") +# Bind-mount mode for CONTAINER_WORKSPACE. Defaults to "ro" so a command run +# inside the container cannot modify the host's copy of the source under audit. +# This matters because the workspace is typically the tree being analyzed, and +# callers commonly collect it afterwards as an artifact: a writable mount makes +# that artifact agent-influenced rather than a faithful copy of the input. Set +# CONTAINER_WORKSPACE_MODE to "rw" to opt in to writes. An empty or +# unrecognized value falls back to "ro" so the default cannot be silently +# weakened by a blank variable. +CONTAINER_WORKSPACE_MODE = ( + "rw" if os.environ.get("CONTAINER_WORKSPACE_MODE", "").strip().lower() == "rw" else "ro" +) CONTAINER_TIMEOUT = int(os.environ.get("CONTAINER_TIMEOUT", "30")) CONTAINER_PERSIST = os.environ.get("CONTAINER_PERSIST", "").lower() in ("1", "true", "yes") CONTAINER_PERSIST_KEY = os.environ.get("CONTAINER_PERSIST_KEY", "") @@ -102,9 +119,14 @@ def _persistent_name() -> str: source trees. Including the network mode ensures a run configured for one network (e.g. the default "none") never reuses a persistent container that was created with a different, more permissive network (e.g. "bridge"), - which would otherwise silently re-enable egress. + which would otherwise silently re-enable egress. The workspace mount mode + is included for the same reason: a run configured for the default "ro" must + not reuse a container that was created with a writable workspace. """ - key_material = f"{CONTAINER_IMAGE}:{CONTAINER_WORKSPACE}:net={CONTAINER_NETWORK}" + key_material = ( + f"{CONTAINER_IMAGE}:{CONTAINER_WORKSPACE}" + f":net={CONTAINER_NETWORK}:ws={CONTAINER_WORKSPACE_MODE}" + ) if CONTAINER_PERSIST_KEY: key_material += f":{CONTAINER_PERSIST_KEY}" digest = hashlib.sha256(key_material.encode()).hexdigest()[:12] @@ -172,7 +194,7 @@ def _start_container() -> str: if not CONTAINER_PERSIST: cmd.append("--rm") if CONTAINER_WORKSPACE: - cmd += ["-v", f"{CONTAINER_WORKSPACE}:/workspace"] + cmd += ["-v", f"{CONTAINER_WORKSPACE}:/workspace:{CONTAINER_WORKSPACE_MODE}"] cmd += [CONTAINER_IMAGE, "tail", "-f", "/dev/null"] logging.debug(f"Starting container: {' '.join(cmd)}") result = subprocess.run(cmd, capture_output=True, text=True, timeout=_DOCKER_TIMEOUT) diff --git a/tests/test_container_shell.py b/tests/test_container_shell.py index ca1baad..17cc305 100644 --- a/tests/test_container_shell.py +++ b/tests/test_container_shell.py @@ -76,7 +76,7 @@ def test_start_container_success(self): assert "run" in cmd assert "--name" in cmd assert "-v" in cmd - assert "/host/workspace:/workspace" in cmd + assert "/host/workspace:/workspace:ro" in cmd assert "test-image:latest" in cmd assert "tail" in cmd @@ -91,6 +91,30 @@ def test_start_container_no_workspace(self): cmd = mock_run.call_args[0][0] assert "-v" not in cmd + def test_start_container_workspace_read_only_by_default(self): + """The workspace mount is read-only unless a caller opts in.""" + with ( + patch.object(cs_mod, "CONTAINER_IMAGE", "test-image:latest"), + patch.object(cs_mod, "CONTAINER_WORKSPACE", "/host/workspace"), + patch.object(cs_mod, "CONTAINER_WORKSPACE_MODE", "ro"), + patch("subprocess.run", return_value=_make_proc(returncode=0)) as mock_run, + ): + cs_mod._start_container() + cmd = mock_run.call_args[0][0] + assert "/host/workspace:/workspace:ro" in cmd + + def test_start_container_workspace_rw_opt_in(self): + """CONTAINER_WORKSPACE_MODE=rw restores a writable mount.""" + with ( + patch.object(cs_mod, "CONTAINER_IMAGE", "test-image:latest"), + patch.object(cs_mod, "CONTAINER_WORKSPACE", "/host/workspace"), + patch.object(cs_mod, "CONTAINER_WORKSPACE_MODE", "rw"), + patch("subprocess.run", return_value=_make_proc(returncode=0)) as mock_run, + ): + cs_mod._start_container() + cmd = mock_run.call_args[0][0] + assert "/host/workspace:/workspace:rw" in cmd + def test_start_container_failure(self): with ( patch.object(cs_mod, "CONTAINER_IMAGE", "missing-image:latest"), @@ -140,6 +164,36 @@ def test_start_container_opt_in_network(self): assert "--network" in cmd assert cmd[cmd.index("--network") + 1] == "bridge" + def test_workspace_mode_defaults_to_ro_when_unset(self, monkeypatch): + original = os.environ.get("CONTAINER_WORKSPACE_MODE") + monkeypatch.delenv("CONTAINER_WORKSPACE_MODE", raising=False) + try: + reloaded = _reload_cs() + assert reloaded.CONTAINER_WORKSPACE_MODE == "ro" + finally: + _restore_env_and_reload("CONTAINER_WORKSPACE_MODE", original) + + @pytest.mark.parametrize("value", ["", " ", "\t", "bogus", "readwrite", "ro"]) + def test_workspace_mode_falls_back_to_ro(self, monkeypatch, value): + """Only an explicit "rw" opts in; anything else is read-only.""" + original = os.environ.get("CONTAINER_WORKSPACE_MODE") + monkeypatch.setenv("CONTAINER_WORKSPACE_MODE", value) + try: + reloaded = _reload_cs() + assert reloaded.CONTAINER_WORKSPACE_MODE == "ro" + finally: + _restore_env_and_reload("CONTAINER_WORKSPACE_MODE", original) + + @pytest.mark.parametrize("value", ["rw", "RW", " rw "]) + def test_workspace_mode_rw_opt_in(self, monkeypatch, value): + original = os.environ.get("CONTAINER_WORKSPACE_MODE") + monkeypatch.setenv("CONTAINER_WORKSPACE_MODE", value) + try: + reloaded = _reload_cs() + assert reloaded.CONTAINER_WORKSPACE_MODE == "rw" + finally: + _restore_env_and_reload("CONTAINER_WORKSPACE_MODE", original) + def test_network_defaults_to_none_when_unset(self, monkeypatch): original = os.environ.get("CONTAINER_NETWORK") monkeypatch.delenv("CONTAINER_NETWORK", raising=False) @@ -301,6 +355,19 @@ def test_persistent_name_differs_for_different_workspaces(self): name_b = cs_mod._persistent_name() assert name_a != name_b + def test_persistent_name_varies_with_workspace_mode(self): + """A "ro" run must not reuse a container created with a writable mount.""" + with ( + patch.object(cs_mod, "CONTAINER_IMAGE", "test-image:latest"), + patch.object(cs_mod, "CONTAINER_WORKSPACE", "/source/tree"), + patch.object(cs_mod, "CONTAINER_PERSIST_KEY", ""), + ): + with patch.object(cs_mod, "CONTAINER_WORKSPACE_MODE", "ro"): + name_ro = cs_mod._persistent_name() + with patch.object(cs_mod, "CONTAINER_WORKSPACE_MODE", "rw"): + name_rw = cs_mod._persistent_name() + assert name_ro != name_rw + def test_persistent_name_varies_with_network(self): with ( patch.object(cs_mod, "CONTAINER_IMAGE", "test-image:latest"), From 1c2a227ed114f35073bdfe93703d63592263f92a Mon Sep 17 00:00:00 2001 From: Bas Alberts Date: Mon, 21 Sep 2026 12:26:39 -0400 Subject: [PATCH 2/4] Make the workspace mount mode configurable, default unchanged Review found that my first pass would have broken the shipped prompts, so this narrows the change to adding the knob without flipping any default. What went wrong: I proposed defaulting the mount to "ro" on the evidence that all 144 container_shell_exec calls in a sample audit were reads. That sample was not representative. Both container_shell_source_access and container_shell_sast instruct the model to build symbol indexes in-tree: source_access: `cd /workspace && ctags -R --fields=+ne .` `cd /workspace && gtags --compact && global -x ` sast: `cd /workspace && cscope -R -b` Those write tags, GTAGS/GRTAGS and cscope.out into the workspace. A read-only default breaks the documented workflow of both toolboxes. The sampled audit simply happened to use rg/cat on a small Go repo and never reached step 4. Compounding it, no toolbox declared CONTAINER_WORKSPACE_MODE, and this module only passes declared env entries to the server, so the "rw" escape hatch I documented was unreachable through the normal taskflow path. The default would have been unopt-out-able. So this commit ships only the mechanism: - CONTAINER_WORKSPACE_MODE, defaulting to "rw". The mount string becomes ":/workspace:rw", which is semantically identical to the previous bare ":/workspace" since rw is Docker's default. No behaviour change. - Passthrough declared in all five launching toolboxes, so "ro" is actually reachable. container_shell_remote.yaml is unchanged: it is a remote transport, so its env block is inert and the server-side environment governs. - Only a literal "ro" (case-insensitive, trimmed) opts in. Anything blank or unrecognized stays "rw", so a typo cannot silently break a workflow that needs to write. Note this is the opposite bias from CONTAINER_NETWORK, and deliberately so: there the unsafe direction is the one that needs an explicit value, here it is the breaking direction. Read-only is still worth adopting where the caller collects the workspace afterwards as an artifact, since a writable mount makes that artifact agent-influenced rather than a faithful copy of the input. Flipping the default needs the index outputs redirected first (ctags -f, cscope -f, gtags dbpath + GTAGSDBPATH). I have not verified those flags against the shipped images, so that is left as separate work rather than asserted here. Also folds the mode into the persistent-container key material, so a run configured for "ro" cannot reuse a container created writable, and reaps the pre-mode container name on upgrade so the rename does not strand it. The reap uses the same non-forcing docker rm as the existing same-name path, so a running legacy container is left alone rather than killed. Tests: 66 pass. Covers both mount modes at the docker run layer, env parsing including blank/unrecognized/injection-shaped values, persistent-name separation between modes, and the legacy reap. test_start_persistent_no_rm_flag now locates the docker run call instead of indexing into call_args_list, so inserting another docker invocation ahead of it no longer breaks it. --- .../mcp_servers/container_shell.py | 60 +++++++++---- .../toolboxes/container_shell_base.yaml | 1 + .../container_shell_malware_analysis.yaml | 1 + .../container_shell_network_analysis.yaml | 1 + .../toolboxes/container_shell_sast.yaml | 6 ++ .../container_shell_source_access.yaml | 4 + tests/test_container_shell.py | 85 ++++++++++++++----- 7 files changed, 121 insertions(+), 37 deletions(-) diff --git a/src/seclab_taskflows/mcp_servers/container_shell.py b/src/seclab_taskflows/mcp_servers/container_shell.py index d167fcd..841481e 100644 --- a/src/seclab_taskflows/mcp_servers/container_shell.py +++ b/src/seclab_taskflows/mcp_servers/container_shell.py @@ -8,12 +8,13 @@ - ``CONTAINER_IMAGE`` — image to run (required). - ``CONTAINER_WORKSPACE`` — host path bind-mounted at ``/workspace`` (optional). -- ``CONTAINER_WORKSPACE_MODE`` — bind-mount mode for that path, ``ro`` (default) - or ``rw``. Defaults to read-only so a command run inside the container cannot - modify the host's copy of the source under audit. Set it to ``rw`` only if a - taskflow genuinely needs to write into the workspace. An empty, unset, or - unrecognized value falls back to ``ro``, so the default cannot be silently - weakened by a blank variable. +- ``CONTAINER_WORKSPACE_MODE`` — bind-mount mode for that path, ``rw`` (default) + or ``ro``. The default preserves the historical writable mount, because the + shipped source-access and SAST prompts build symbol indexes in-tree + (``ctags -R .``, ``cscope -R -b``, ``gtags``). Set it to ``ro`` when the + workspace should be left pristine — for example when the caller collects the + tree afterwards as an artifact and wants a faithful copy of the input. Only a + literal ``ro`` selects read-only; any other value mounts read-write. - ``CONTAINER_TIMEOUT`` — default per-command timeout in seconds (default 30). - ``CONTAINER_PERSIST`` — reuse a deterministic container across runs when truthy. - ``CONTAINER_PERSIST_KEY`` — extra key to distinguish persistent containers. @@ -72,16 +73,17 @@ CONTAINER_IMAGE = os.environ.get("CONTAINER_IMAGE", "") CONTAINER_WORKSPACE = os.environ.get("CONTAINER_WORKSPACE", "") -# Bind-mount mode for CONTAINER_WORKSPACE. Defaults to "ro" so a command run -# inside the container cannot modify the host's copy of the source under audit. -# This matters because the workspace is typically the tree being analyzed, and -# callers commonly collect it afterwards as an artifact: a writable mount makes -# that artifact agent-influenced rather than a faithful copy of the input. Set -# CONTAINER_WORKSPACE_MODE to "rw" to opt in to writes. An empty or -# unrecognized value falls back to "ro" so the default cannot be silently -# weakened by a blank variable. +# Bind-mount mode for CONTAINER_WORKSPACE. Defaults to "rw", which preserves +# the historical behaviour: the shipped source-access and SAST prompts tell the +# model to build symbol indexes in-tree (`ctags -R .`, `cscope -R -b`, `gtags`), +# which write into the workspace. Setting this to "ro" is worth doing whenever +# the caller collects the workspace afterwards as an artifact, since a writable +# mount makes that artifact agent-influenced rather than a faithful copy of the +# input. Flipping the default would require redirecting those index outputs +# first. Only a literal "ro" selects read-only, so a blank or unrecognized +# value cannot accidentally break a workflow that needs to write. CONTAINER_WORKSPACE_MODE = ( - "rw" if os.environ.get("CONTAINER_WORKSPACE_MODE", "").strip().lower() == "rw" else "ro" + "ro" if os.environ.get("CONTAINER_WORKSPACE_MODE", "").strip().lower() == "ro" else "rw" ) CONTAINER_TIMEOUT = int(os.environ.get("CONTAINER_TIMEOUT", "30")) CONTAINER_PERSIST = os.environ.get("CONTAINER_PERSIST", "").lower() in ("1", "true", "yes") @@ -110,6 +112,28 @@ _DOCKER_TIMEOUT = 30 +def _legacy_persistent_names() -> list[str]: + """Names this config would have had before CONTAINER_WORKSPACE_MODE existed. + + The mode was added to the key material so a run configured for the default + "ro" cannot reuse a container created with a writable workspace. That + changes the deterministic name for every pre-existing persistent container, + which would otherwise be orphaned: _start_container() only ever inspects or + removes the name it computes, so the old container would linger, never + reused and never cleaned up. + + Returns the pre-mode name so callers can reap it once on upgrade. Note the + reap uses the same non-forcing ``docker rm`` as the same-name path, so a + legacy container that is still *running* is left alone rather than killed + out from under whoever is using it; only stopped leftovers are collected. + """ + legacy = f"{CONTAINER_IMAGE}:{CONTAINER_WORKSPACE}:net={CONTAINER_NETWORK}" + if CONTAINER_PERSIST_KEY: + legacy += f":{CONTAINER_PERSIST_KEY}" + digest = hashlib.sha256(legacy.encode()).hexdigest()[:12] + return [f"seclab-persist-{digest}"] + + def _persistent_name() -> str: """Derive a deterministic container name from the image for reuse across tasks. @@ -187,6 +211,12 @@ def _start_container() -> str: return name # Remove stopped leftover with the same name _remove_container(name) + # Reap the pre-CONTAINER_WORKSPACE_MODE name for this same config, so + # upgrading does not strand a stopped container that can no longer be + # reused. Non-forcing, so a running one is left alone. + for legacy in _legacy_persistent_names(): + if legacy != name: + _remove_container(legacy) else: name = f"seclab-shell-{uuid.uuid4().hex[:8]}" diff --git a/src/seclab_taskflows/toolboxes/container_shell_base.yaml b/src/seclab_taskflows/toolboxes/container_shell_base.yaml index 186a541..48b1a0a 100644 --- a/src/seclab_taskflows/toolboxes/container_shell_base.yaml +++ b/src/seclab_taskflows/toolboxes/container_shell_base.yaml @@ -12,6 +12,7 @@ server_params: env: CONTAINER_IMAGE: "ghcr.io/githubsecuritylab/seclab-shell-base:latest" CONTAINER_WORKSPACE: "{{ env('CONTAINER_WORKSPACE', required=False) }}" + CONTAINER_WORKSPACE_MODE: "{{ env('CONTAINER_WORKSPACE_MODE', 'rw') }}" CONTAINER_TIMEOUT: "{{ env('CONTAINER_TIMEOUT', '30') }}" CONTAINER_PERSIST: "{{ env('CONTAINER_PERSIST', required=False) }}" CONTAINER_PERSIST_KEY: "{{ env('CONTAINER_PERSIST_KEY', required=False) }}" diff --git a/src/seclab_taskflows/toolboxes/container_shell_malware_analysis.yaml b/src/seclab_taskflows/toolboxes/container_shell_malware_analysis.yaml index 734ecd4..7aff253 100644 --- a/src/seclab_taskflows/toolboxes/container_shell_malware_analysis.yaml +++ b/src/seclab_taskflows/toolboxes/container_shell_malware_analysis.yaml @@ -12,6 +12,7 @@ server_params: env: CONTAINER_IMAGE: "ghcr.io/githubsecuritylab/seclab-shell-malware-analysis:latest" CONTAINER_WORKSPACE: "{{ env('CONTAINER_WORKSPACE', required=False) }}" + CONTAINER_WORKSPACE_MODE: "{{ env('CONTAINER_WORKSPACE_MODE', 'rw') }}" CONTAINER_TIMEOUT: "{{ env('CONTAINER_TIMEOUT', '60') }}" CONTAINER_PERSIST: "{{ env('CONTAINER_PERSIST', required=False) }}" CONTAINER_PERSIST_KEY: "{{ env('CONTAINER_PERSIST_KEY', required=False) }}" diff --git a/src/seclab_taskflows/toolboxes/container_shell_network_analysis.yaml b/src/seclab_taskflows/toolboxes/container_shell_network_analysis.yaml index 0b5c878..1997d2c 100644 --- a/src/seclab_taskflows/toolboxes/container_shell_network_analysis.yaml +++ b/src/seclab_taskflows/toolboxes/container_shell_network_analysis.yaml @@ -12,6 +12,7 @@ server_params: env: CONTAINER_IMAGE: "ghcr.io/githubsecuritylab/seclab-shell-network-analysis:latest" CONTAINER_WORKSPACE: "{{ env('CONTAINER_WORKSPACE', required=False) }}" + CONTAINER_WORKSPACE_MODE: "{{ env('CONTAINER_WORKSPACE_MODE', 'rw') }}" CONTAINER_TIMEOUT: "{{ env('CONTAINER_TIMEOUT', '30') }}" CONTAINER_PERSIST: "{{ env('CONTAINER_PERSIST', required=False) }}" CONTAINER_PERSIST_KEY: "{{ env('CONTAINER_PERSIST_KEY', required=False) }}" diff --git a/src/seclab_taskflows/toolboxes/container_shell_sast.yaml b/src/seclab_taskflows/toolboxes/container_shell_sast.yaml index 93ae25b..5dbeb12 100644 --- a/src/seclab_taskflows/toolboxes/container_shell_sast.yaml +++ b/src/seclab_taskflows/toolboxes/container_shell_sast.yaml @@ -12,6 +12,12 @@ server_params: env: CONTAINER_IMAGE: "ghcr.io/githubsecuritylab/seclab-shell-sast:latest" CONTAINER_WORKSPACE: "{{ env('CONTAINER_WORKSPACE', required=False) }}" + # Writable: the documented workflow for this toolbox builds symbol indexes + # in-tree (`ctags -R .`, `cscope -R -b`, `gtags`), which write tags, + # cscope.out and GTAGS/GRTAGS into the workspace. Set this to "ro" and + # redirect those outputs (e.g. `ctags -f /tmp/tags`) if you need the + # workspace left pristine. + CONTAINER_WORKSPACE_MODE: "{{ env('CONTAINER_WORKSPACE_MODE', 'rw') }}" CONTAINER_TIMEOUT: "{{ env('CONTAINER_TIMEOUT', '60') }}" CONTAINER_PERSIST: "{{ env('CONTAINER_PERSIST', required=False) }}" CONTAINER_PERSIST_KEY: "{{ env('CONTAINER_PERSIST_KEY', required=False) }}" diff --git a/src/seclab_taskflows/toolboxes/container_shell_source_access.yaml b/src/seclab_taskflows/toolboxes/container_shell_source_access.yaml index badfcd6..2aea332 100644 --- a/src/seclab_taskflows/toolboxes/container_shell_source_access.yaml +++ b/src/seclab_taskflows/toolboxes/container_shell_source_access.yaml @@ -12,6 +12,10 @@ server_params: env: CONTAINER_IMAGE: "ghcr.io/githubsecuritylab/seclab-shell-source-access:latest" CONTAINER_WORKSPACE: "{{ env('CONTAINER_WORKSPACE', required=False) or env('DATA_DIR') ~ '/repo_under_test' }}" + # The prompt for this toolbox builds symbol indexes in-tree (`ctags -R .`, + # `gtags`), so the mount must stay writable by default. Set this to "ro" + # if you redirect those outputs and want the workspace left pristine. + CONTAINER_WORKSPACE_MODE: "{{ env('CONTAINER_WORKSPACE_MODE', 'rw') }}" CONTAINER_TIMEOUT: "{{ env('CONTAINER_TIMEOUT', '60') }}" CONTAINER_PERSIST: "{{ env('CONTAINER_PERSIST', 'true') }}" CONTAINER_PERSIST_KEY: "{{ env('CONTAINER_PERSIST_KEY', required=False) }}" diff --git a/tests/test_container_shell.py b/tests/test_container_shell.py index 17cc305..d636d88 100644 --- a/tests/test_container_shell.py +++ b/tests/test_container_shell.py @@ -2,6 +2,7 @@ # SPDX-License-Identifier: MIT import atexit +import hashlib import importlib import os import subprocess @@ -76,7 +77,7 @@ def test_start_container_success(self): assert "run" in cmd assert "--name" in cmd assert "-v" in cmd - assert "/host/workspace:/workspace:ro" in cmd + assert "/host/workspace:/workspace:rw" in cmd assert "test-image:latest" in cmd assert "tail" in cmd @@ -91,29 +92,60 @@ def test_start_container_no_workspace(self): cmd = mock_run.call_args[0][0] assert "-v" not in cmd - def test_start_container_workspace_read_only_by_default(self): - """The workspace mount is read-only unless a caller opts in.""" + def test_start_container_workspace_writable_by_default(self): + """The default preserves the historical writable mount.""" with ( patch.object(cs_mod, "CONTAINER_IMAGE", "test-image:latest"), patch.object(cs_mod, "CONTAINER_WORKSPACE", "/host/workspace"), - patch.object(cs_mod, "CONTAINER_WORKSPACE_MODE", "ro"), + patch.object(cs_mod, "CONTAINER_WORKSPACE_MODE", "rw"), patch("subprocess.run", return_value=_make_proc(returncode=0)) as mock_run, ): cs_mod._start_container() cmd = mock_run.call_args[0][0] - assert "/host/workspace:/workspace:ro" in cmd + assert "/host/workspace:/workspace:rw" in cmd - def test_start_container_workspace_rw_opt_in(self): - """CONTAINER_WORKSPACE_MODE=rw restores a writable mount.""" + def test_start_container_workspace_ro_opt_in(self): + """CONTAINER_WORKSPACE_MODE=ro mounts the workspace read-only.""" with ( patch.object(cs_mod, "CONTAINER_IMAGE", "test-image:latest"), patch.object(cs_mod, "CONTAINER_WORKSPACE", "/host/workspace"), - patch.object(cs_mod, "CONTAINER_WORKSPACE_MODE", "rw"), + patch.object(cs_mod, "CONTAINER_WORKSPACE_MODE", "ro"), patch("subprocess.run", return_value=_make_proc(returncode=0)) as mock_run, ): cs_mod._start_container() cmd = mock_run.call_args[0][0] - assert "/host/workspace:/workspace:rw" in cmd + assert "/host/workspace:/workspace:ro" in cmd + + def test_start_container_reaps_legacy_persistent_name(self): + """Upgrading must not strand the pre-CONTAINER_WORKSPACE_MODE container.""" + with ( + patch.object(cs_mod, "CONTAINER_IMAGE", "test-image:latest"), + patch.object(cs_mod, "CONTAINER_WORKSPACE", "/host/workspace"), + patch.object(cs_mod, "CONTAINER_WORKSPACE_MODE", "ro"), + patch.object(cs_mod, "CONTAINER_PERSIST", True), + patch.object(cs_mod, "CONTAINER_PERSIST_KEY", ""), + patch.object(cs_mod, "_is_running", return_value=False), + patch.object(cs_mod, "_remove_container") as mock_rm, + patch("subprocess.run", return_value=_make_proc(returncode=0)), + ): + cs_mod._start_container() + removed = [c[0][0] for c in mock_rm.call_args_list] + legacy = cs_mod._legacy_persistent_names()[0] + assert cs_mod._persistent_name() in removed + assert legacy in removed + + def test_legacy_persistent_name_matches_pre_mode_scheme(self): + """The legacy name is the current name computed without the ws= field.""" + with ( + patch.object(cs_mod, "CONTAINER_IMAGE", "test-image:latest"), + patch.object(cs_mod, "CONTAINER_WORKSPACE", "/host/workspace"), + patch.object(cs_mod, "CONTAINER_NETWORK", "none"), + patch.object(cs_mod, "CONTAINER_PERSIST_KEY", ""), + ): + expected = hashlib.sha256( + b"test-image:latest:/host/workspace:net=none" + ).hexdigest()[:12] + assert cs_mod._legacy_persistent_names() == [f"seclab-persist-{expected}"] def test_start_container_failure(self): with ( @@ -164,33 +196,36 @@ def test_start_container_opt_in_network(self): assert "--network" in cmd assert cmd[cmd.index("--network") + 1] == "bridge" - def test_workspace_mode_defaults_to_ro_when_unset(self, monkeypatch): + def test_workspace_mode_defaults_to_rw_when_unset(self, monkeypatch): + """Unset preserves the historical writable mount.""" original = os.environ.get("CONTAINER_WORKSPACE_MODE") monkeypatch.delenv("CONTAINER_WORKSPACE_MODE", raising=False) try: reloaded = _reload_cs() - assert reloaded.CONTAINER_WORKSPACE_MODE == "ro" + assert reloaded.CONTAINER_WORKSPACE_MODE == "rw" finally: _restore_env_and_reload("CONTAINER_WORKSPACE_MODE", original) - @pytest.mark.parametrize("value", ["", " ", "\t", "bogus", "readwrite", "ro"]) - def test_workspace_mode_falls_back_to_ro(self, monkeypatch, value): - """Only an explicit "rw" opts in; anything else is read-only.""" + @pytest.mark.parametrize( + "value", ["", " ", "\t", "bogus", "readonly", "rw", "ro; rm -rf /", "ro rw"] + ) + def test_workspace_mode_falls_back_to_rw(self, monkeypatch, value): + """Only a literal "ro" opts in; anything else stays writable.""" original = os.environ.get("CONTAINER_WORKSPACE_MODE") monkeypatch.setenv("CONTAINER_WORKSPACE_MODE", value) try: reloaded = _reload_cs() - assert reloaded.CONTAINER_WORKSPACE_MODE == "ro" + assert reloaded.CONTAINER_WORKSPACE_MODE == "rw" finally: _restore_env_and_reload("CONTAINER_WORKSPACE_MODE", original) - @pytest.mark.parametrize("value", ["rw", "RW", " rw "]) - def test_workspace_mode_rw_opt_in(self, monkeypatch, value): + @pytest.mark.parametrize("value", ["ro", "RO", " ro "]) + def test_workspace_mode_ro_opt_in(self, monkeypatch, value): original = os.environ.get("CONTAINER_WORKSPACE_MODE") monkeypatch.setenv("CONTAINER_WORKSPACE_MODE", value) try: reloaded = _reload_cs() - assert reloaded.CONTAINER_WORKSPACE_MODE == "rw" + assert reloaded.CONTAINER_WORKSPACE_MODE == "ro" finally: _restore_env_and_reload("CONTAINER_WORKSPACE_MODE", original) @@ -404,19 +439,25 @@ def test_start_persistent_no_rm_flag(self): stdout="", ) rm_proc = _make_proc(returncode=0) + legacy_rm_proc = _make_proc(returncode=0) run_proc = _make_proc(returncode=0) with ( patch.object(cs_mod, "CONTAINER_IMAGE", "test-image:latest"), patch.object(cs_mod, "CONTAINER_WORKSPACE", ""), patch.object(cs_mod, "CONTAINER_PERSIST", True), patch.object(cs_mod, "CONTAINER_PERSIST_KEY", ""), - patch("subprocess.run", side_effect=[inspect_proc, rm_proc, run_proc]) as mock_run, + patch( + "subprocess.run", + side_effect=[inspect_proc, rm_proc, legacy_rm_proc, run_proc], + ) as mock_run, ): name = cs_mod._start_container() assert name.startswith("seclab-persist-") - # The docker run call is the third one - run_cmd = mock_run.call_args_list[2][0][0] - assert "--rm" not in run_cmd + # Locate the docker run call rather than indexing, so inserting + # another docker invocation ahead of it does not break this test. + run_cmds = [c[0][0] for c in mock_run.call_args_list if c[0][0][:2] == ["docker", "run"]] + assert len(run_cmds) == 1 + assert "--rm" not in run_cmds[0] def test_stop_skips_persistent_container(self): cs_mod._container_name = "seclab-persist-abc123" From 1797c874ea33cf9bb71e85a42e4fd069130c2dae Mon Sep 17 00:00:00 2001 From: Bas Alberts Date: Mon, 21 Sep 2026 12:36:07 -0400 Subject: [PATCH 3/4] Keep persistent container names stable at the default mode Review pointed out that the legacy-name reap does not work in the common case: persistent containers are normally left running, and _remove_container() uses a non-forcing `docker rm`, so the old container is not collected. If it later stops, the _is_running() early return skips the reap entirely. The migration assumed a cleanup that would not happen. Rather than add a force-remove or a stop-then-remove policy, remove the reason the rename happens at all: only append the ws= field to the key material when the mode is not the "rw" default. At the default the derived name is now byte-identical to the pre-PR name, so no existing persistent container is orphaned and no migration is needed. An explicit "ro" run still derives a distinct name, so it cannot reuse a container created with a writable workspace, which was the point of including the mode. Verified rather than assumed: pre-PR name : seclab-persist-e54197a37c4b mode=rw : seclab-persist-e54197a37c4b identical mode=ro : seclab-persist-d4ee618f0df6 distinct Drops _legacy_persistent_names() and the reap loop. Replaces the two reap tests with one asserting the name at the default mode equals the pre-mode scheme, which is the invariant that actually matters. Also fixes docstring text that still described "ro" as the default after the default was inverted. --- .../mcp_servers/container_shell.py | 48 +++++-------------- tests/test_container_shell.py | 33 ++++--------- 2 files changed, 23 insertions(+), 58 deletions(-) diff --git a/src/seclab_taskflows/mcp_servers/container_shell.py b/src/seclab_taskflows/mcp_servers/container_shell.py index 841481e..b3564e8 100644 --- a/src/seclab_taskflows/mcp_servers/container_shell.py +++ b/src/seclab_taskflows/mcp_servers/container_shell.py @@ -112,28 +112,6 @@ _DOCKER_TIMEOUT = 30 -def _legacy_persistent_names() -> list[str]: - """Names this config would have had before CONTAINER_WORKSPACE_MODE existed. - - The mode was added to the key material so a run configured for the default - "ro" cannot reuse a container created with a writable workspace. That - changes the deterministic name for every pre-existing persistent container, - which would otherwise be orphaned: _start_container() only ever inspects or - removes the name it computes, so the old container would linger, never - reused and never cleaned up. - - Returns the pre-mode name so callers can reap it once on upgrade. Note the - reap uses the same non-forcing ``docker rm`` as the same-name path, so a - legacy container that is still *running* is left alone rather than killed - out from under whoever is using it; only stopped leftovers are collected. - """ - legacy = f"{CONTAINER_IMAGE}:{CONTAINER_WORKSPACE}:net={CONTAINER_NETWORK}" - if CONTAINER_PERSIST_KEY: - legacy += f":{CONTAINER_PERSIST_KEY}" - digest = hashlib.sha256(legacy.encode()).hexdigest()[:12] - return [f"seclab-persist-{digest}"] - - def _persistent_name() -> str: """Derive a deterministic container name from the image for reuse across tasks. @@ -143,14 +121,20 @@ def _persistent_name() -> str: source trees. Including the network mode ensures a run configured for one network (e.g. the default "none") never reuses a persistent container that was created with a different, more permissive network (e.g. "bridge"), - which would otherwise silently re-enable egress. The workspace mount mode - is included for the same reason: a run configured for the default "ro" must - not reuse a container that was created with a writable workspace. + which would otherwise silently re-enable egress. + + The workspace mount mode is included for the same reason, but only when it + differs from the "rw" default. Appending it unconditionally would change the + derived name for every persistent container that already exists, orphaning + each one on upgrade: _start_container() only ever inspects or removes the + name it computes, and persistent containers are normally left running, so a + non-forcing ``docker rm`` would not collect them either. Omitting the field + at the default keeps those names byte-identical, while an explicit "ro" run + still gets a distinct name and so cannot reuse a writable container. """ - key_material = ( - f"{CONTAINER_IMAGE}:{CONTAINER_WORKSPACE}" - f":net={CONTAINER_NETWORK}:ws={CONTAINER_WORKSPACE_MODE}" - ) + key_material = f"{CONTAINER_IMAGE}:{CONTAINER_WORKSPACE}:net={CONTAINER_NETWORK}" + if CONTAINER_WORKSPACE_MODE != "rw": + key_material += f":ws={CONTAINER_WORKSPACE_MODE}" if CONTAINER_PERSIST_KEY: key_material += f":{CONTAINER_PERSIST_KEY}" digest = hashlib.sha256(key_material.encode()).hexdigest()[:12] @@ -211,12 +195,6 @@ def _start_container() -> str: return name # Remove stopped leftover with the same name _remove_container(name) - # Reap the pre-CONTAINER_WORKSPACE_MODE name for this same config, so - # upgrading does not strand a stopped container that can no longer be - # reused. Non-forcing, so a running one is left alone. - for legacy in _legacy_persistent_names(): - if legacy != name: - _remove_container(legacy) else: name = f"seclab-shell-{uuid.uuid4().hex[:8]}" diff --git a/tests/test_container_shell.py b/tests/test_container_shell.py index d636d88..0c32b05 100644 --- a/tests/test_container_shell.py +++ b/tests/test_container_shell.py @@ -116,36 +116,24 @@ def test_start_container_workspace_ro_opt_in(self): cmd = mock_run.call_args[0][0] assert "/host/workspace:/workspace:ro" in cmd - def test_start_container_reaps_legacy_persistent_name(self): - """Upgrading must not strand the pre-CONTAINER_WORKSPACE_MODE container.""" - with ( - patch.object(cs_mod, "CONTAINER_IMAGE", "test-image:latest"), - patch.object(cs_mod, "CONTAINER_WORKSPACE", "/host/workspace"), - patch.object(cs_mod, "CONTAINER_WORKSPACE_MODE", "ro"), - patch.object(cs_mod, "CONTAINER_PERSIST", True), - patch.object(cs_mod, "CONTAINER_PERSIST_KEY", ""), - patch.object(cs_mod, "_is_running", return_value=False), - patch.object(cs_mod, "_remove_container") as mock_rm, - patch("subprocess.run", return_value=_make_proc(returncode=0)), - ): - cs_mod._start_container() - removed = [c[0][0] for c in mock_rm.call_args_list] - legacy = cs_mod._legacy_persistent_names()[0] - assert cs_mod._persistent_name() in removed - assert legacy in removed + def test_persistent_name_unchanged_at_default_mode(self): + """Upgrading must not rename existing persistent containers. - def test_legacy_persistent_name_matches_pre_mode_scheme(self): - """The legacy name is the current name computed without the ws= field.""" + The pre-CONTAINER_WORKSPACE_MODE name is the one derived without a + ws= field, so the default must reproduce it exactly or every existing + persistent container is orphaned on upgrade. + """ with ( patch.object(cs_mod, "CONTAINER_IMAGE", "test-image:latest"), patch.object(cs_mod, "CONTAINER_WORKSPACE", "/host/workspace"), patch.object(cs_mod, "CONTAINER_NETWORK", "none"), + patch.object(cs_mod, "CONTAINER_WORKSPACE_MODE", "rw"), patch.object(cs_mod, "CONTAINER_PERSIST_KEY", ""), ): - expected = hashlib.sha256( + pre_mode = hashlib.sha256( b"test-image:latest:/host/workspace:net=none" ).hexdigest()[:12] - assert cs_mod._legacy_persistent_names() == [f"seclab-persist-{expected}"] + assert cs_mod._persistent_name() == f"seclab-persist-{pre_mode}" def test_start_container_failure(self): with ( @@ -439,7 +427,6 @@ def test_start_persistent_no_rm_flag(self): stdout="", ) rm_proc = _make_proc(returncode=0) - legacy_rm_proc = _make_proc(returncode=0) run_proc = _make_proc(returncode=0) with ( patch.object(cs_mod, "CONTAINER_IMAGE", "test-image:latest"), @@ -448,7 +435,7 @@ def test_start_persistent_no_rm_flag(self): patch.object(cs_mod, "CONTAINER_PERSIST_KEY", ""), patch( "subprocess.run", - side_effect=[inspect_proc, rm_proc, legacy_rm_proc, run_proc], + side_effect=[inspect_proc, rm_proc, run_proc], ) as mock_run, ): name = cs_mod._start_container() From 1c48f3741bea5eff1d15d486a2cd89b309a242f5 Mon Sep 17 00:00:00 2001 From: Bas Alberts Date: Tue, 22 Sep 2026 14:06:40 -0400 Subject: [PATCH 4/4] Rename CONTAINER_WORKSPACE_MODE to CONTAINER_WORKSPACE_MOUNT_MODE Review suggestion from @p-. The value is the Docker bind-mount option (`-v src:/workspace:ro`), so naming it for the mount is more precise than "workspace mode", which reads as though the workspace itself has modes. Renaming is free here: the variable is introduced by this PR and has never appeared in a release, so there is nothing to migrate and no deprecation shim is warranted. Also isolates the mode in test_start_container_success. It asserted the `rw` mount without patching the constant, so the suite failed for anyone who had the variable set to `ro` in their environment. Verified by removing the patch and re-running with the variable set, which reproduces the failure the other mode-specific tests were already guarded against. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0fc0c170-07c4-4eff-b075-3e1f420c4ba3 --- .../mcp_servers/container_shell.py | 12 +++--- .../toolboxes/container_shell_base.yaml | 2 +- .../container_shell_malware_analysis.yaml | 2 +- .../container_shell_network_analysis.yaml | 2 +- .../toolboxes/container_shell_sast.yaml | 2 +- .../container_shell_source_access.yaml | 2 +- tests/test_container_shell.py | 39 ++++++++++--------- 7 files changed, 31 insertions(+), 30 deletions(-) diff --git a/src/seclab_taskflows/mcp_servers/container_shell.py b/src/seclab_taskflows/mcp_servers/container_shell.py index b3564e8..78b8874 100644 --- a/src/seclab_taskflows/mcp_servers/container_shell.py +++ b/src/seclab_taskflows/mcp_servers/container_shell.py @@ -8,7 +8,7 @@ - ``CONTAINER_IMAGE`` — image to run (required). - ``CONTAINER_WORKSPACE`` — host path bind-mounted at ``/workspace`` (optional). -- ``CONTAINER_WORKSPACE_MODE`` — bind-mount mode for that path, ``rw`` (default) +- ``CONTAINER_WORKSPACE_MOUNT_MODE`` — bind-mount mode for that path, ``rw`` (default) or ``ro``. The default preserves the historical writable mount, because the shipped source-access and SAST prompts build symbol indexes in-tree (``ctags -R .``, ``cscope -R -b``, ``gtags``). Set it to ``ro`` when the @@ -82,8 +82,8 @@ # input. Flipping the default would require redirecting those index outputs # first. Only a literal "ro" selects read-only, so a blank or unrecognized # value cannot accidentally break a workflow that needs to write. -CONTAINER_WORKSPACE_MODE = ( - "ro" if os.environ.get("CONTAINER_WORKSPACE_MODE", "").strip().lower() == "ro" else "rw" +CONTAINER_WORKSPACE_MOUNT_MODE = ( + "ro" if os.environ.get("CONTAINER_WORKSPACE_MOUNT_MODE", "").strip().lower() == "ro" else "rw" ) CONTAINER_TIMEOUT = int(os.environ.get("CONTAINER_TIMEOUT", "30")) CONTAINER_PERSIST = os.environ.get("CONTAINER_PERSIST", "").lower() in ("1", "true", "yes") @@ -133,8 +133,8 @@ def _persistent_name() -> str: still gets a distinct name and so cannot reuse a writable container. """ key_material = f"{CONTAINER_IMAGE}:{CONTAINER_WORKSPACE}:net={CONTAINER_NETWORK}" - if CONTAINER_WORKSPACE_MODE != "rw": - key_material += f":ws={CONTAINER_WORKSPACE_MODE}" + if CONTAINER_WORKSPACE_MOUNT_MODE != "rw": + key_material += f":ws={CONTAINER_WORKSPACE_MOUNT_MODE}" if CONTAINER_PERSIST_KEY: key_material += f":{CONTAINER_PERSIST_KEY}" digest = hashlib.sha256(key_material.encode()).hexdigest()[:12] @@ -202,7 +202,7 @@ def _start_container() -> str: if not CONTAINER_PERSIST: cmd.append("--rm") if CONTAINER_WORKSPACE: - cmd += ["-v", f"{CONTAINER_WORKSPACE}:/workspace:{CONTAINER_WORKSPACE_MODE}"] + cmd += ["-v", f"{CONTAINER_WORKSPACE}:/workspace:{CONTAINER_WORKSPACE_MOUNT_MODE}"] cmd += [CONTAINER_IMAGE, "tail", "-f", "/dev/null"] logging.debug(f"Starting container: {' '.join(cmd)}") result = subprocess.run(cmd, capture_output=True, text=True, timeout=_DOCKER_TIMEOUT) diff --git a/src/seclab_taskflows/toolboxes/container_shell_base.yaml b/src/seclab_taskflows/toolboxes/container_shell_base.yaml index 48b1a0a..4952a59 100644 --- a/src/seclab_taskflows/toolboxes/container_shell_base.yaml +++ b/src/seclab_taskflows/toolboxes/container_shell_base.yaml @@ -12,7 +12,7 @@ server_params: env: CONTAINER_IMAGE: "ghcr.io/githubsecuritylab/seclab-shell-base:latest" CONTAINER_WORKSPACE: "{{ env('CONTAINER_WORKSPACE', required=False) }}" - CONTAINER_WORKSPACE_MODE: "{{ env('CONTAINER_WORKSPACE_MODE', 'rw') }}" + CONTAINER_WORKSPACE_MOUNT_MODE: "{{ env('CONTAINER_WORKSPACE_MOUNT_MODE', 'rw') }}" CONTAINER_TIMEOUT: "{{ env('CONTAINER_TIMEOUT', '30') }}" CONTAINER_PERSIST: "{{ env('CONTAINER_PERSIST', required=False) }}" CONTAINER_PERSIST_KEY: "{{ env('CONTAINER_PERSIST_KEY', required=False) }}" diff --git a/src/seclab_taskflows/toolboxes/container_shell_malware_analysis.yaml b/src/seclab_taskflows/toolboxes/container_shell_malware_analysis.yaml index 7aff253..b342148 100644 --- a/src/seclab_taskflows/toolboxes/container_shell_malware_analysis.yaml +++ b/src/seclab_taskflows/toolboxes/container_shell_malware_analysis.yaml @@ -12,7 +12,7 @@ server_params: env: CONTAINER_IMAGE: "ghcr.io/githubsecuritylab/seclab-shell-malware-analysis:latest" CONTAINER_WORKSPACE: "{{ env('CONTAINER_WORKSPACE', required=False) }}" - CONTAINER_WORKSPACE_MODE: "{{ env('CONTAINER_WORKSPACE_MODE', 'rw') }}" + CONTAINER_WORKSPACE_MOUNT_MODE: "{{ env('CONTAINER_WORKSPACE_MOUNT_MODE', 'rw') }}" CONTAINER_TIMEOUT: "{{ env('CONTAINER_TIMEOUT', '60') }}" CONTAINER_PERSIST: "{{ env('CONTAINER_PERSIST', required=False) }}" CONTAINER_PERSIST_KEY: "{{ env('CONTAINER_PERSIST_KEY', required=False) }}" diff --git a/src/seclab_taskflows/toolboxes/container_shell_network_analysis.yaml b/src/seclab_taskflows/toolboxes/container_shell_network_analysis.yaml index 1997d2c..d5722dc 100644 --- a/src/seclab_taskflows/toolboxes/container_shell_network_analysis.yaml +++ b/src/seclab_taskflows/toolboxes/container_shell_network_analysis.yaml @@ -12,7 +12,7 @@ server_params: env: CONTAINER_IMAGE: "ghcr.io/githubsecuritylab/seclab-shell-network-analysis:latest" CONTAINER_WORKSPACE: "{{ env('CONTAINER_WORKSPACE', required=False) }}" - CONTAINER_WORKSPACE_MODE: "{{ env('CONTAINER_WORKSPACE_MODE', 'rw') }}" + CONTAINER_WORKSPACE_MOUNT_MODE: "{{ env('CONTAINER_WORKSPACE_MOUNT_MODE', 'rw') }}" CONTAINER_TIMEOUT: "{{ env('CONTAINER_TIMEOUT', '30') }}" CONTAINER_PERSIST: "{{ env('CONTAINER_PERSIST', required=False) }}" CONTAINER_PERSIST_KEY: "{{ env('CONTAINER_PERSIST_KEY', required=False) }}" diff --git a/src/seclab_taskflows/toolboxes/container_shell_sast.yaml b/src/seclab_taskflows/toolboxes/container_shell_sast.yaml index 5dbeb12..3aeb113 100644 --- a/src/seclab_taskflows/toolboxes/container_shell_sast.yaml +++ b/src/seclab_taskflows/toolboxes/container_shell_sast.yaml @@ -17,7 +17,7 @@ server_params: # cscope.out and GTAGS/GRTAGS into the workspace. Set this to "ro" and # redirect those outputs (e.g. `ctags -f /tmp/tags`) if you need the # workspace left pristine. - CONTAINER_WORKSPACE_MODE: "{{ env('CONTAINER_WORKSPACE_MODE', 'rw') }}" + CONTAINER_WORKSPACE_MOUNT_MODE: "{{ env('CONTAINER_WORKSPACE_MOUNT_MODE', 'rw') }}" CONTAINER_TIMEOUT: "{{ env('CONTAINER_TIMEOUT', '60') }}" CONTAINER_PERSIST: "{{ env('CONTAINER_PERSIST', required=False) }}" CONTAINER_PERSIST_KEY: "{{ env('CONTAINER_PERSIST_KEY', required=False) }}" diff --git a/src/seclab_taskflows/toolboxes/container_shell_source_access.yaml b/src/seclab_taskflows/toolboxes/container_shell_source_access.yaml index 2aea332..7c434b6 100644 --- a/src/seclab_taskflows/toolboxes/container_shell_source_access.yaml +++ b/src/seclab_taskflows/toolboxes/container_shell_source_access.yaml @@ -15,7 +15,7 @@ server_params: # The prompt for this toolbox builds symbol indexes in-tree (`ctags -R .`, # `gtags`), so the mount must stay writable by default. Set this to "ro" # if you redirect those outputs and want the workspace left pristine. - CONTAINER_WORKSPACE_MODE: "{{ env('CONTAINER_WORKSPACE_MODE', 'rw') }}" + CONTAINER_WORKSPACE_MOUNT_MODE: "{{ env('CONTAINER_WORKSPACE_MOUNT_MODE', 'rw') }}" CONTAINER_TIMEOUT: "{{ env('CONTAINER_TIMEOUT', '60') }}" CONTAINER_PERSIST: "{{ env('CONTAINER_PERSIST', 'true') }}" CONTAINER_PERSIST_KEY: "{{ env('CONTAINER_PERSIST_KEY', required=False) }}" diff --git a/tests/test_container_shell.py b/tests/test_container_shell.py index 0c32b05..360e275 100644 --- a/tests/test_container_shell.py +++ b/tests/test_container_shell.py @@ -68,6 +68,7 @@ def test_start_container_success(self): with ( patch.object(cs_mod, "CONTAINER_IMAGE", "test-image:latest"), patch.object(cs_mod, "CONTAINER_WORKSPACE", "/host/workspace"), + patch.object(cs_mod, "CONTAINER_WORKSPACE_MOUNT_MODE", "rw"), patch("subprocess.run", return_value=_make_proc(returncode=0)) as mock_run, ): name = cs_mod._start_container() @@ -97,7 +98,7 @@ def test_start_container_workspace_writable_by_default(self): with ( patch.object(cs_mod, "CONTAINER_IMAGE", "test-image:latest"), patch.object(cs_mod, "CONTAINER_WORKSPACE", "/host/workspace"), - patch.object(cs_mod, "CONTAINER_WORKSPACE_MODE", "rw"), + patch.object(cs_mod, "CONTAINER_WORKSPACE_MOUNT_MODE", "rw"), patch("subprocess.run", return_value=_make_proc(returncode=0)) as mock_run, ): cs_mod._start_container() @@ -105,11 +106,11 @@ def test_start_container_workspace_writable_by_default(self): assert "/host/workspace:/workspace:rw" in cmd def test_start_container_workspace_ro_opt_in(self): - """CONTAINER_WORKSPACE_MODE=ro mounts the workspace read-only.""" + """CONTAINER_WORKSPACE_MOUNT_MODE=ro mounts the workspace read-only.""" with ( patch.object(cs_mod, "CONTAINER_IMAGE", "test-image:latest"), patch.object(cs_mod, "CONTAINER_WORKSPACE", "/host/workspace"), - patch.object(cs_mod, "CONTAINER_WORKSPACE_MODE", "ro"), + patch.object(cs_mod, "CONTAINER_WORKSPACE_MOUNT_MODE", "ro"), patch("subprocess.run", return_value=_make_proc(returncode=0)) as mock_run, ): cs_mod._start_container() @@ -119,7 +120,7 @@ def test_start_container_workspace_ro_opt_in(self): def test_persistent_name_unchanged_at_default_mode(self): """Upgrading must not rename existing persistent containers. - The pre-CONTAINER_WORKSPACE_MODE name is the one derived without a + The pre-CONTAINER_WORKSPACE_MOUNT_MODE name is the one derived without a ws= field, so the default must reproduce it exactly or every existing persistent container is orphaned on upgrade. """ @@ -127,7 +128,7 @@ def test_persistent_name_unchanged_at_default_mode(self): patch.object(cs_mod, "CONTAINER_IMAGE", "test-image:latest"), patch.object(cs_mod, "CONTAINER_WORKSPACE", "/host/workspace"), patch.object(cs_mod, "CONTAINER_NETWORK", "none"), - patch.object(cs_mod, "CONTAINER_WORKSPACE_MODE", "rw"), + patch.object(cs_mod, "CONTAINER_WORKSPACE_MOUNT_MODE", "rw"), patch.object(cs_mod, "CONTAINER_PERSIST_KEY", ""), ): pre_mode = hashlib.sha256( @@ -186,36 +187,36 @@ def test_start_container_opt_in_network(self): def test_workspace_mode_defaults_to_rw_when_unset(self, monkeypatch): """Unset preserves the historical writable mount.""" - original = os.environ.get("CONTAINER_WORKSPACE_MODE") - monkeypatch.delenv("CONTAINER_WORKSPACE_MODE", raising=False) + original = os.environ.get("CONTAINER_WORKSPACE_MOUNT_MODE") + monkeypatch.delenv("CONTAINER_WORKSPACE_MOUNT_MODE", raising=False) try: reloaded = _reload_cs() - assert reloaded.CONTAINER_WORKSPACE_MODE == "rw" + assert reloaded.CONTAINER_WORKSPACE_MOUNT_MODE == "rw" finally: - _restore_env_and_reload("CONTAINER_WORKSPACE_MODE", original) + _restore_env_and_reload("CONTAINER_WORKSPACE_MOUNT_MODE", original) @pytest.mark.parametrize( "value", ["", " ", "\t", "bogus", "readonly", "rw", "ro; rm -rf /", "ro rw"] ) def test_workspace_mode_falls_back_to_rw(self, monkeypatch, value): """Only a literal "ro" opts in; anything else stays writable.""" - original = os.environ.get("CONTAINER_WORKSPACE_MODE") - monkeypatch.setenv("CONTAINER_WORKSPACE_MODE", value) + original = os.environ.get("CONTAINER_WORKSPACE_MOUNT_MODE") + monkeypatch.setenv("CONTAINER_WORKSPACE_MOUNT_MODE", value) try: reloaded = _reload_cs() - assert reloaded.CONTAINER_WORKSPACE_MODE == "rw" + assert reloaded.CONTAINER_WORKSPACE_MOUNT_MODE == "rw" finally: - _restore_env_and_reload("CONTAINER_WORKSPACE_MODE", original) + _restore_env_and_reload("CONTAINER_WORKSPACE_MOUNT_MODE", original) @pytest.mark.parametrize("value", ["ro", "RO", " ro "]) def test_workspace_mode_ro_opt_in(self, monkeypatch, value): - original = os.environ.get("CONTAINER_WORKSPACE_MODE") - monkeypatch.setenv("CONTAINER_WORKSPACE_MODE", value) + original = os.environ.get("CONTAINER_WORKSPACE_MOUNT_MODE") + monkeypatch.setenv("CONTAINER_WORKSPACE_MOUNT_MODE", value) try: reloaded = _reload_cs() - assert reloaded.CONTAINER_WORKSPACE_MODE == "ro" + assert reloaded.CONTAINER_WORKSPACE_MOUNT_MODE == "ro" finally: - _restore_env_and_reload("CONTAINER_WORKSPACE_MODE", original) + _restore_env_and_reload("CONTAINER_WORKSPACE_MOUNT_MODE", original) def test_network_defaults_to_none_when_unset(self, monkeypatch): original = os.environ.get("CONTAINER_NETWORK") @@ -385,9 +386,9 @@ def test_persistent_name_varies_with_workspace_mode(self): patch.object(cs_mod, "CONTAINER_WORKSPACE", "/source/tree"), patch.object(cs_mod, "CONTAINER_PERSIST_KEY", ""), ): - with patch.object(cs_mod, "CONTAINER_WORKSPACE_MODE", "ro"): + with patch.object(cs_mod, "CONTAINER_WORKSPACE_MOUNT_MODE", "ro"): name_ro = cs_mod._persistent_name() - with patch.object(cs_mod, "CONTAINER_WORKSPACE_MODE", "rw"): + with patch.object(cs_mod, "CONTAINER_WORKSPACE_MOUNT_MODE", "rw"): name_rw = cs_mod._persistent_name() assert name_ro != name_rw