fix: default network isolation to enabled for new workloads by peppescg · Pull Request #2332 · stacklok/toolhive-studio

peppescg · 2026-06-25T14:10:57Z

Summary

Aligns Studio with the ToolHive backend default: network isolation is now enabled by default for all new MCP server workloads.

Default networkIsolation form value flipped from false → true in both the local-server and registry-server create dialogs
insecure_allow_all is now computed dynamically: true when isolation is on but no allow-list is configured (prevents silently blocking all egress for custom servers), false when an explicit allow-list is provided
Applied consistently across all three code paths: create-local, update-local, create-registry

Closes #2326

Background

The backend now defaults network_isolation: true when the field is omitted from POST /api/v1beta/workloads. Studio was overriding this by always sending network_isolation: false, so users never benefited from the hardened default.

The insecure_allow_all: true case (isolation on, all outbound permitted except Docker gateway) maps directly to the backend's built-in "network" permission profile. Custom/local servers that have no declared allow-list get this profile by default, preserving general internet connectivity while still blocking the Docker gateway.

Registry servers retain their declared outbound permissions automatically — the form already pre-fills allowedHosts/allowedPorts from the registry entry's permissions.network.outbound metadata, so flipping the default is sufficient.

The MCP Optimizer referenced in the issue (use-create-optimizer-workload.ts) no longer exists in the codebase — that feature was previously removed. No action needed there.

Test plan

prepareCreateWorkloadData with isolation on + empty allow-lists → insecure_allow_all: true
prepareCreateWorkloadData with isolation on + populated allow-lists → insecure_allow_all: false
prepareUpdateLocalWorkloadData with isolation on + empty allow-lists → insecure_allow_all: true
Local server form defaults networkIsolation to true
Registry server form defaults networkIsolation to true
Full test suite: 2454 tests pass

🤖 Generated with Claude Code

…2326 Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR aligns ToolHive Studio’s workload creation/update payloads and UI defaults with the backend’s hardened default by enabling network isolation by default for newly created MCP server workloads, and by adjusting how outbound network permissions are derived from the allow-lists.

Changes:

Flips create-form defaults for networkIsolation to true in both local/custom and registry install dialogs.
Normalizes outbound allow-lists (filtering empty hosts and non-numeric ports) and computes permission_profile.network.outbound.insecure_allow_all based on whether allow-lists are empty.
Updates and adds unit tests to cover the new defaults and insecure_allow_all behavior across create/update paths.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
renderer/src/features/registry-servers/lib/orchestrate-run-registry-server.tsx	Filters allow-lists and computes `insecure_allow_all` when creating registry workloads.
renderer/src/features/registry-servers/lib/tests/orchestrate-run-registry-server.test.tsx	Adds coverage for `insecure_allow_all` behavior for registry workload creation.
renderer/src/features/registry-servers/components/form-run-from-registry/index.tsx	Defaults `networkIsolation` to `true` in registry install form.
renderer/src/features/registry-servers/components/tests/form-run-from-registry.test.tsx	Updates tests for the new default and adjusts flows that assumed isolation was off by default.
renderer/src/features/mcp-servers/lib/orchestrate-run-local-server.tsx	Filters allow-lists and computes `insecure_allow_all` for local create and update payloads.
renderer/src/features/mcp-servers/lib/tests/orchestrate-run-local-server.test.tsx	Adds coverage for `insecure_allow_all` behavior for local create/update payload preparation.
renderer/src/features/mcp-servers/components/local-mcp/dialog-form-local-mcp.tsx	Defaults `networkIsolation` to `true` for new local/custom servers.
renderer/src/features/mcp-servers/components/local-mcp/tests/dialog-form-local-mcp.test.tsx	Updates expectations and adds a test asserting the new default is submitted.

…en lists are empty

….test.tsx

kantord · 2026-06-25T16:06:56Z

-      )
-      await userEvent.click(switchLabel)
-
+      // Network isolation is enabled by default; no need to toggle the switch


an agent or human reading this in the future won't remember that this click was ever there, so they won't have any questions about it

instead of mentioning it in every test case, it may be better to communicate this behavior through a test case name, like

it('enables network isolation for all MCPs by default'

…ases

kantord · 2026-06-25T16:18:14Z

+              allow_host: filteredHosts,
+              allow_port: filteredPorts,
+              insecure_allow_all:
+                filteredHosts.length === 0 && filteredPorts.length === 0,


so in other words

const allowsAllNetworkAccess = hasNoAllowedPorts & hasNoAllowedHosts

this is confusing because it is equivalent to "if no ports or hosts are allowed, then all ports and hosts are allowed".

So what I'm missing is an explicit

const isNetworkIsolationEnabled = ....

that is derived from an actual form state, which we should have, because seems like we have an actual form field for this.

I'm not totally sure if there is an uncovered situation here (network isolation despite those fields being empty), but in any case if that invariant exists, I would probably enforce it as a validation, considering the explicit form field to be the authoritative arbiter of truth

eleftherias

Blocking a merge until we do further testing on the backend

Studio always sent `network_isolation: false`, silently overriding the backend's new default of isolating new workloads. A prior attempt (#2332) derived `insecure_allow_all` from whether the allow-list arrays were empty, but reviewers flagged that as ambiguous — it couldn't distinguish "isolate with no restrictions" from "isolate and block everything". Replaces the boolean toggle with an explicit `networkAccess` mode (none / host / proxy) and an explicit `allowedDestinations` (anywhere / selected) field that drives `insecure_allow_all` directly, consolidates the duplicated tab-content components into one, and wires up the previously-unused `permission_profile.network.mode: "host"` backend field. Closes #2326 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix: make network isolation mode explicit instead of inferred Studio always sent `network_isolation: false`, silently overriding the backend's new default of isolating new workloads. A prior attempt (#2332) derived `insecure_allow_all` from whether the allow-list arrays were empty, but reviewers flagged that as ambiguous — it couldn't distinguish "isolate with no restrictions" from "isolate and block everything". Replaces the boolean toggle with an explicit `networkAccess` mode (none / host / proxy) and an explicit `allowedDestinations` (anywhere / selected) field that drives `insecure_allow_all` directly, consolidates the duplicated tab-content components into one, and wires up the previously-unused `permission_profile.network.mode: "host"` backend field. Closes #2326 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: wire up allow_docker_gateway and fix network-access defaults Wires the new backend `allow_docker_gateway` field (not yet released upstream; openapi.json patched against the unreleased backend PR) through to the "Allow host machine access" checkbox in both the local-server and registry-server payload builders. Also fixes three gaps found in review: - Registry install now reads the entry's actual declared `insecure_allow_all`/`network.mode` instead of inferring destinations from allow-list array length, so entries like `fetch` (declaring `insecure_allow_all: true`) and entries recommending host networking are honored, and an entry that explicitly declares an empty allow-list defaults to restricted rather than silently allowing everything. - The edit round-trip derived `allowedDestinations` from a strict `=== false` check, which misread an omitted (rather than explicit `false`) `insecure_allow_all` as unrestricted, risking silently widening a restricted server's egress on save. - Allowed-host validation now accepts IPv4 addresses, matching what the UI tooltip already advertised. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: add breathing room between section titles and radio groups The "Network access" and "Allowed destinations" titles sat flush against their radio groups (James Tenniswood's review feedback). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: revert unsupported IPv4 host validation, collapse empty allow-lists The backend's Squid rule builder only ever emits a dstdomain ACL for allow_host entries (pkg/container/docker/squid.go), with no branch for IP-shaped values, so accepting IPv4 addresses client-side promised something the backend can't reliably enforce. Reverts to domain-only host validation and removes the "or IP addresses" tooltip claim. Also addresses review feedback on the Network access tab layout (James Tenniswood): the Allowed hosts/ports sections no longer show an empty add-a-host/add-a-port list by default, which read as if both were required. DynamicArrayField gains an opt-in collapseWhenEmpty mode that keeps the section title visible but only shows the add button until the user adds an entry, revealing the input row at that point. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(deps): bump pinned ToolHive to v0.37.0, regenerate API client v0.37.0 ships allow_docker_gateway on the create/update workload request types for real, so the hand-patched openapi.json used to develop against the unreleased backend PR is no longer needed — regenerated via `pnpm run generate-client` against the tagged release. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

peppescg and others added 3 commits June 25, 2026 15:48

test: add failing tests for network isolation default (TDD red phase) #…

bc7f0d6

…2326 Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

fix: default network isolation to enabled for new workloads (#2326)

b520d4e

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

refactor: hoist filteredHosts/filteredPorts in update path (drop IIFE)

05f8181

Copilot AI review requested due to automatic review settings June 25, 2026 14:10

github-actions Bot added the size/M label Jun 25, 2026

Copilot started reviewing on behalf of peppescg June 25, 2026 14:11 View session

Copilot AI reviewed Jun 25, 2026

View reviewed changes

Comment thread renderer/src/features/mcp-servers/lib/orchestrate-run-local-server.tsx

Comment thread renderer/src/features/registry-servers/lib/orchestrate-run-registry-server.tsx

peppescg self-assigned this Jun 25, 2026

peppescg added 2 commits June 25, 2026 16:37

fix: update network isolation warning to reflect allow-all default wh…

9786b68

…en lists are empty

fix: update remaining warning text assertion in dialog-form-local-mcp…

3b4b853

….test.tsx

kantord reviewed Jun 25, 2026

View reviewed changes

test: remove stale comments about network isolation default in test c…

2f93065

…ases

kantord reviewed Jun 25, 2026

View reviewed changes

eleftherias requested changes Jun 25, 2026

View reviewed changes

kantord mentioned this pull request Jul 15, 2026

fix: make network isolation mode explicit instead of inferred #2469

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: default network isolation to enabled for new workloads#2332

fix: default network isolation to enabled for new workloads#2332
peppescg wants to merge 6 commits into
mainfrom
issues/2326

peppescg commented Jun 25, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

kantord Jun 25, 2026

Uh oh!

kantord Jun 25, 2026

Uh oh!

eleftherias left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

peppescg commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Background

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

kantord Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

kantord Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

eleftherias left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

peppescg commented Jun 25, 2026 •

edited

Loading