Skip to content

fix: make the constrained-host rule uniform across every sample (finding 2) - #13

Merged
arst merged 16 commits into
mainfrom
fix/untrusted-execution-boundary
Aug 26, 2026
Merged

fix: make the constrained-host rule uniform across every sample (finding 2)#13
arst merged 16 commits into
mainfrom
fix/untrusted-execution-boundary

Conversation

@arst

@arst arst commented Aug 26, 2026

Copy link
Copy Markdown
Owner

Closes finding 2 of the second external review (docs/reviews/2026-08-25-second-review.md),
plus the Stigmergic host-compile, CodeAct-cancellation and Pattern Explorer findings
from its medium and table sections.

The repo teaches one rule — a constrained host executes untrusted work. Before this
branch exactly one sample obeyed it. CodeAct ran model-written code in a locked-down
container; three other samples did the same class of thing on the host and said nothing
about it. This PR makes the rule uniform and, where a doc claimed a guarantee the code
did not enforce, changes the code rather than the copy.

What changed

Shared/Sandbox — the boundary, extracted.
SandboxOptions, SandboxResult, SandboxRunner and BoundedReader lifted out of
CodeAct behaviour-preservingly: the argument list CodeAct generates is element-for-element
identical to before, flag order included, and its pinned tests pass unchanged apart from
one added using. --read-only, --cap-drop ALL and --security-opt no-new-privileges
are unconditional — no options combination removes them — and non-positive Timeout,
PidsLimit, Memory and Cpus clamp to safe values rather than to "unlimited", because
on a type whose only job is bounding untrusted work, "the caller forgot" must not mean
"no bound". RunAsync always names its container, so kill-by-name is never optional.

MCP — the sample the review named.
Was npx -y @modelcontextprotocol/server-everything: an unpinned third-party package
downloaded at run time, executed on the host with the application's entire environment,
with every discovered tool bound to the agent. Now a pinned repo-controlled image running
inside the boundary — no host environment, no credentials, no network, non-root — with
discovery and authorization separated behind an allowlist that fails closed if the server
does not advertise what was asked for. Verified by running the container with a canary
AZURE_OPENAI_API_KEY in the parent environment and confirming printenv inside shows
neither it nor ~85 other host variables.

StigmergicCoordination — compiling untrusted source is running untrusted input.
dotnet build over model-written C# moved off the host and into the boundary (build tasks,
source generators and MSBuild targets all execute code). The gate no longer reports
PASSED when the container failed before the compiler ran — previously a developer with
umask 077 saw round 1 pass instantly and would have concluded the deliberate contract
drift wasn't caught. The workspace is now cleaned on every exit path including SIGINT.

CodeAct. Cancelling an agent run now cancels the container. Separately, its run
directory is readable by the container user regardless of the operator's umask —
Directory.CreateDirectory(path, mode) is masked by umask, so the requested 0755 was
silently becoming 0700.

Pattern Explorer. RunSession.Current was process-global, so a second browser tab
silently cancelled the first tab's run. Now a per-run registry with unguessable tokens,
bounded channel, runtime and output, an environment allowlist so child samples stop
inheriting the Explorer's credentials, token-gated endpoints under a run id, HTML-escaped
rendering with a scheme allowlist on links, mermaid strict mode, and a CSP. Cross-site
GETs can no longer start Azure-billed runs.

The image. Node/npm/npx existed only for the old npx MCP path. The base image
already ships curl, so the entire node build stage is gone and the healthcheck uses
curl -fsS — verified in both directions, including that an unserved container correctly
goes unhealthy.

What this PR deliberately does not do

It does not give Pattern Explorer a docker client or mount the host docker socket to make
MCP runnable inside the container image. That would grant root-equivalent host access to a
web UI that runs model-driven samples — the exact escalation this PR argues against. MCP
stays unrunnable there, and the README says so plainly.

Verification

  • dotnet build "Agentic Patterns.slnx" -c Release — 85 projects, 0 errors, 0 warnings
  • Full suite run three times: 213/213, 0 skipped (up from 188; +47 tests since main)
  • Zero test deletions across the branch — the test project is pure addition, +903 lines
  • Docker-gated tests confirmed to execute rather than skip, here and on CI
  • All four affected samples run end to end against real Docker and real Azure credentials
  • A sweep of all 85 projects for Process.Start, npx, Assembly.Load, CSharpScript,
    Reflection.Emit and model-reachable file writes found no fifth sample still
    executing untrusted work on the host — the uniformity claim is tested, not asserted

Known gaps

McpToolBinding is duplicated in the SemanticKernel twin and tested only in the
AgentFramework flavor. SecurityHeadersTests asserts the CSP string constant rather than
a live response header, so it would not catch deletion of the middleware — its own
docstring says so.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc

arst and others added 16 commits August 25, 2026 13:40
@arst
refactor(sandbox): extract the constrained-execution boundary into Sh…
6d13c8e 
…ared

Move ContainerCodeRunner's argument-building, bounded output reading, timeout,
and container lifecycle into Shared/Sandbox (SandboxOptions, SandboxResult,
SandboxRunner) so CodeAct is no longer the only sample with a real isolation
boundary; MCP and Stigmergic can reuse SandboxRunner instead of inventing a
weaker one. ContainerCodeRunner.BuildRunArguments keeps its exact signature
and now maps CodeExecutionOptions onto SandboxOptions, producing the same
docker argument list element-for-element and in the same order.

Task 2.1 of docs/superpowers/sdd/2026-08-25-second-review-remediation.
@arst @claude
fix(sandbox): never let optional fields turn off safety machinery
a4b0135 
Review fix round 1 on the sandbox extraction. Every finding shared one root
cause: unconditional safety behaviour in the original ContainerCodeRunner
became conditional on optional SandboxOptions fields defaulting to off.

- C1 (critical): RunAsync generated no container name when ContainerName was
  null, so timeout cleanup degraded from kill-by-name to killing only the
  docker-run CLI process, leaking the still-running container. RunAsync now
  always generates a name when the caller omits one.
- I1: Timeout <= TimeSpan.Zero now clamps to a 3-minute default instead of
  disabling the timeout outright (also fixes a real regression: ExecutionTimeout
  = TimeSpan.Zero used to fail fast and now would have run unbounded).
- I2: stdin is written after the output readers start and the timeout is
  armed, and uses a cancellable WriteAsync overload, closing a pipe-deadlock/
  uncancellable-hang path.
- I3: Interactive is now derived as `Interactive || stdin is not null` so a
  redirected pipe is never silently discarded by a missing -i.
- M3: PidsLimit <= 0 clamps to 128 instead of reading to docker as unlimited.
- M2: strengthened two weak test assertions.
- I4: added pure BuildRunArguments tests for User/Interactive/ContainerName/
  Tmpfs, plus a Docker-gated RunAsync test proving the I1 clamp.

ContainerCodeRunner.cs and both pinned test files (CodeActExecutionTests.cs,
CodeActSandboxSmokeTests.cs) are untouched by this round.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
@arst @claude
test(sandbox): assert the timeout/pids-limit clamps by value, not by …
f7d3b99 
…waiting

Fix round 2: the round-1 Docker-gated timeout test was vacuous — it ran a
fast command and asserted TimedOut: false, which passes identically whether
Timeout <= 0 clamps to 3 minutes or disables the timeout outright, since a
fast command finishes long before either bound expires.

Extract SandboxRunner.EffectiveTimeout(TimeSpan) and
EffectivePidsLimit(int) as pure static methods; RunAsync and
BuildRunArguments now call through them instead of inlining the ternary.
New pure tests assert the clamp values directly, so reverting either clamp
fails immediately with no Docker or timing involved. Renamed the surviving
Docker-gated test to what it actually proves
(ZeroTimeoutDoesNotCancelTheRunImmediately).

ContainerCodeRunner.cs and both pinned test files are untouched.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
@arst @claude
fix(mcp): pin, sandbox and allowlist the MCP server
0c9555e 
The sample no longer runs npx -y @modelcontextprotocol/server-everything
(unpinned, latest-at-run-time, executed on the host with the app's own
environment, every discovered tool bound). The server is now pinned to
an exact version baked into a Docker image, launched through the same
locked-down container boundary CodeAct uses (Shared.Sandbox.SandboxRunner),
and only an explicit allowlist (add, echo) of its discovered tools is
ever bound to the agent via the new McpToolBinding.SelectAuthorized,
which fails closed if an allowlisted tool is missing. Both the Agent
Framework and Semantic Kernel flavors get the same treatment and fail
closed with no silent host fallback when no container runtime is
available, mirroring CodeAct.AgentFramework's CodeRunnerFactory double
opt-in.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
@arst @claude
fix(mcp): fix round 1 - remove false override hint, name containers, …
af79fc5 
…fix docs

- Fail-closed message in both Program.cs no longer advertises the
  AGENTIC_PATTERNS_* override: neither variable is ever read by the MCP
  samples (only CodeRunnerFactory reads them), so the old message told a
  Docker-less user to do something that does nothing.
- SandboxOptions now gets an explicit ContainerName on the stdio path so
  the container is nameable/killable even though McpClient (not
  SandboxRunner.RunAsync) owns the process lifecycle here.
- McpToolBinding.SelectAuthorized takes HashSet<string> instead of
  IReadOnlySet<string> so allowed.Comparer governs both the match and the
  missing-check consistently, and both filter and diff run through
  Distinct/Except with that same comparer so duplicate discovered names
  don't leak into the result. Mirrored byte-identically into the SK twin.
  Added tests for case-insensitive allowlists and duplicate tool names.
- README.md now says plainly that MCP cannot run inside the Pattern
  Explorer container (no docker client/socket there) instead of shipping
  a silently-broken run button.
- MCP.md now says the sandbox image must be built manually first (unlike
  CodeAct, which builds its image on first run).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
@arst @claude
fix(stigmergic): run the build gate inside the sandbox with limits an…
649ae7d 
…d cleanup

Compiling untrusted C# is still running untrusted code (build tasks, source
generators, MSBuild targets), so the stigmergic build gate now runs through
Shared.Sandbox instead of a bare host `dotnet build`, fails closed with no
container runtime (same double opt-in AGENTIC_PATTERNS_* fallback as CodeAct),
and guarantees workspace cleanup via try/finally around the round loop
(including the success-path return that used to leak it). New BuildGate.cs
extracts the testable pieces; SandboxRunner reads stdout/stderr concurrently,
removing the sequential-ReadToEndAsync pipe deadlock. Mounts the writable
tmpfs at /tmp (not /build) - verified by hand that /build alone still fails,
because the .NET CLI's first-run mutex is hardcoded under /tmp/.dotnet/shm
regardless of HOME/DOTNET_CLI_HOME/TMPDIR.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
@arst @claude
fix(stigmergic): stop discarding the gate's exit code, fix umask-depe…
ec57258 
…ndent perms

C1: BuildGate.InterpretResult stops treating a nonzero exit with no parsed
compiler diagnostic as PASSED (permission failures, pids-limit kills, and
similar now surface as a synthetic AP0003 error) - reproduced live with
PidsLimit: 1 ("Cannot fork") and pinned as a Docker-gated regression test.

C2: workspace directories/files are now forced world-readable via explicit
File.SetUnixFileMode after creation. Directory.CreateDirectory(path, mode)
alone - the same call ContainerCodeRunner.CreateRunDirectory makes - is NOT
enough: mkdir()'s mode argument is itself masked by the process umask,
verified by hand (0755 requested, 0700 back under umask 077). chmod() is not
subject to umask, so an explicit SetUnixFileMode call after creation is what
actually makes the permissions umask-independent.

I3: added real tests that actually enter HostBuildAsync (a genuinely broken
file, and cancellation of a live process), replacing one that threw before
reaching it. Fixed the underlying orphan: HostBuildAsync now kills the
process on caller cancellation too, not just on timeout.

Minor: SIGINT now deletes the workspace directly (the round loop's
try/finally can't be trusted to run on a signal); the finally/handler also
catch UnauthorizedAccessException; docs note the stock vs. offline-cached
image difference.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
@arst
fix(codeact): propagate cancellation into script execution
4fe16e0 
AIFunctionFactory.Create injects the invocation's CancellationToken into
ExecuteCSharp's trailing parameter, so cancelling the agent run now
cancels the underlying container run instead of using
CancellationToken.None. Proven with a fake IGeneratedCodeRunner recording
the token it received, invoked through AIFunctionFactory.Create with an
already-cancelled token.
@arst
fix(codeact): force run-directory permissions instead of relying on u…
e47aa50 
…mask

Directory.CreateDirectory(path, mode)'s mode is a mkdir(2) mode, masked
by the process umask like any mkdir call (verified: umask 077 turns a
requested 0755 into 0700). Under a restrictive umask the container's
uid 65532 couldn't traverse the per-run directory or read the bind-mounted
script, so CodeAct failed loudly with "Script failed (exit N)".

Mirrors StigmergicCoordination.AgentFramework/BuildGate.cs
CreateWorkspaceDirectory, which hit and fixed the identical gap: create
the directory, then force the bits with File.SetUnixFileMode (chmod is
not subject to umask). CreateRunDirectory is now internal so a test can
call it directly and assert the actual on-disk mode, independent of
whatever umask the test process runs under.
@arst @claude
fix(explorer): per-run isolation, bounded output and runtime
cfd8073 
RunSession.Current was process-global, so a second browser tab silently
cancelled the first tab's run. Replace it with a registry keyed by
(id, token), bound the output channel and add wall-clock/output ceilings,
and stop child processes from inheriting Explorer's whole environment -
only PATH/HOME/DOTNET_* plus a per-project allowlist (Azure OpenAI config
by default, CodeAct's host-execution opt-ins only for CodeAct) survive.

Endpoints move under /api/runs/{id}/{input,cancel}, auth'd by an
X-Run-Token header; /api/run now hands back {id, token} as the first SSE
event. This intentionally breaks app.js's calls to the old /api/run/input
and /api/run/cancel routes - task 2.5b repairs the frontend.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
@arst @claude
fix(explorer): polish per-run isolation per review round 1
7585f64 
- Delete the CodeAct-only hardcoding of the two AGENTIC_PATTERNS_* opt-in
  variables in RunSession.StartProcess. StigmergicCoordination.AgentFramework
  (task 2.3) reads the same two variables for its own host-build fallback,
  so hardcoding one project silently changed its opt-out-of-fail-closed
  behavior when launched from Explorer. Both patterns now declare an
  explicit environmentAllowlist in their frontmatter instead.
- Make the MaxLiveRuns cap actually atomic (Register now locks around the
  count-check-then-insert).
- Stop projecting EnvironmentAllowlist into the /api/patterns wire shape.
- Add the TryGet(a.Id, b.Token) cross-run isolation assertion.
- Rename the internal CancellationToken test seam to IsCancelled (bool) to
  remove the Color-Color trap against the BCL type.
- Name the Windows-forwarding ceiling (PATH/HOME/DOTNET_* only) as a
  ponytail: comment instead of building support this repo doesn't claim.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
@arst @claude
fix(explorer): repair frontend for per-run tokens, sanitised renderin…
41faf65 
…g, CSP

Repairs the app.js calls task 2.5a intentionally left broken (/api/run/cancel
and /api/run/input, both deleted): cancel and stdin now read the run id/token
from the SSE `session` event and call /api/runs/{id}/cancel and
/api/runs/{id}/input with an X-Run-Token header.

Renders pattern-doc Markdown with raw HTML escaped (marked's renderer.html
override - v15 dropped the old `sanitize` option), runs Mermaid in `strict`
mode instead of `loose`, and caps terminal history at 5000 lines dropping
from the front.

Adds a same-origin Content-Security-Policy in Program.cs; verified live via
Playwright that the vendored marked/mermaid bundles still work under it with
zero console violations and a diagram actually renders to SVG. Nothing needed
loosening from the brief's starting policy.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
@arst @claude
fix(explorer): escape attribute injection, block javascript: hrefs
df40855 
Review round 1: escapeHtml didn't cover " so frontmatter (flavor, pattern
id, source file path) interpolated into data-*/href attributes could break
out and add a real event handler - fixed the escaper and the three call
sites, root-caused rather than patched per-site. Added a scheme allowlist
(http/https/mailto) on marked's link/image renderers so a javascript: link
or image target in a doc body neutralises to '#'/'' instead of staying
clickable - CSP's script-src was the only thing stopping that before.

Also: finish() now clears runId/runToken (previously only run() did), the
fire-and-forget cancel/input fetches get a no-op .catch, and the CSP gained
base-uri/form-action/frame-ancestors (none of the three covered by
default-src).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
@arst
chore(image): drop the npx machinery the MCP sample no longer uses
8f81171
@arst @claude
fix(boundary): close the last opt-outs and make every doc claim true
c7d0924 
Final review wave: no Critical, three blocking doc/copy defects plus five of
the same classes this PR exists to remove.

I1 MCP passed `User: null`, the only caller-visible opt-out of a SandboxOptions
default in the tree, while README and MCP.md both promised non-root. Verified by
running the pinned server under `--user 65532:65532` (initialize succeeds, both
flavors answer 6912 end to end), so the boundary now enforces the uid instead of
trusting the image's USER line. The options moved into McpToolBinding.Sandbox()
so both flavors share one definition and a test can pin it.

I2 MCP and Stigmergic both hardcode the docker CLI but told users to install
Docker or Podman. Stop naming Podman there - same move af79fc5 made for the
override that was never read. CodeAct keeps both, it genuinely has the knob.

I3 Stigmergic was missing from the README uniformity story and its Explorer card
carried neither risk nor note, while its frontmatter forwards the unsafe-host
pair into the child. Fixed all three, plus the README paragraph that said those
variables affect only CodeAct.

I4 The Explorer environment allowlist had no test: deleting Environment.Clear()
left all 188 green. Extracted ApplyChildEnvironment as the seam; the mutant now
fails.

I5 ZeroTimeoutDoesNotCancelTheRunImmediately asserted nothing its comment
claimed and ran an image absent on CI. Replaced with a run whose timeout must
actually fire; deleting CancelAfter fails it (and takes 125s doing so).

M1 Shared the umask fix in Shared/Sandbox/HostWorkspace instead of leaving it
copied in two samples. M3 --memory 0 / --cpus 0 read to docker as unlimited;
clamped like PidsLimit and Timeout. M4 --mount is comma-separated with no
escaping, so a comma in a path injects options; rejected. M5 /api/run was an
unauthenticated GET that starts a billed process; cross-site requests now 403.
M7 noted the cold-pull-inside-the-timeout flake in BuildGate's ponytail block.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
@arst @claude
docs(test): correct a comment that outlived the behaviour it described
bdb1382 
MCP stopped opting out of the non-root default in c7d0924, so nothing
"depends on" User: null any more. The option still needs pinning — null
must omit the flag AND its value, never emit `--user ""`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LB4jjPp7i2pxV55Vpe6tpc
@arst
arst merged commit 1d29c66 into main Aug 26, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@arst