Skip to content

fix(shields): keep registered network routes when shields down swaps policy #7963

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
harjothkhara wants to merge 8 commits into from
Closed

fix(shields): keep registered network routes when shields down swaps policy #7963

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ed1115c
fix(shields): keep registered network routes when shields down swaps …
harjothkhara Jul 31, 2026
2dd8c6b
test(shields): compare the inlined provider filter with the canonical…
harjothkhara Jul 31, 2026
8c98627
docs(shields): state the collision assumption in the permissive merge
harjothkhara Jul 31, 2026
bfe11fe
Merge branch 'main' into fix/7952-shields-down-network-routes
cv Aug 4, 2026
b55ebe3
fix(shields): carry only registry-owned routes and report a degraded …
harjothkhara Aug 4, 2026
8c80c76
fix(shields): name the lost routes and their real recovery command
harjothkhara Aug 4, 2026
fbf6ac9
Merge remote-tracking branch 'origin/main' into fix/7952-shields-down…
harjothkhara Aug 4, 2026
8a82528
Merge branch 'main' into fix/7952-shields-down-network-routes
apurvvkumaria Aug 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 56 additions & 3 deletions src/lib/shields/flow.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import os from "node:os";
import path from "node:path";

import { afterEach, beforeEach, describe, expect, it, type MockInstance, vi } from "vitest";
import YAML from "yaml";

const requireDist = createRequire(import.meta.url);
const shieldsModulePath = "./index.js";
Expand All @@ -26,6 +27,9 @@ setInterval(() => {}, 60000);
`;

type ShieldsHarness = {
// Every document handed to `openshell policy set`, read at call time —
// shieldsDown deletes a temp policy file as soon as the run returns.
appliedPolicies: string[];
auditSpy: MockInstance;
errorSpy: MockInstance;
logSpy: MockInstance;
Expand All @@ -40,8 +44,10 @@ type ShieldsHarness = {
let tmpDir: string;

type HarnessOptions = {
basePermissiveYaml?: string;
directSandboxUnavailable?: boolean;
dockerExecFileSync?: (argv: unknown) => string;
livePolicyYaml?: string;
failOpenClawGuardActions?: Array<"lock" | "unlock">;
invokedAs?: "nemoclaw" | "nemohermes";
openClawGuardFailure?: {
Expand All @@ -61,14 +67,21 @@ type HarnessOptions = {
send: () => boolean;
kill: () => boolean;
};
livePolicyYaml?: string;
run?: (cmd: unknown) => { status: number };
};

function throwHarnessError(error: Error): never {
throw error;
}

function readIfPresent(file: string): string {
try {
return fs.readFileSync(file, "utf-8");
} catch {
return "";
}
}

function createHarness(options: HarnessOptions = {}): ShieldsHarness {
vi.stubEnv("NEMOCLAW_INVOKED_AS", options.invokedAs ?? "nemoclaw");
delete require.cache[requireDist.resolve(shieldsModulePath)];
Expand Down Expand Up @@ -99,12 +112,19 @@ function createHarness(options: HarnessOptions = {}): ShieldsHarness {
});
options.fork && vi.spyOn(childProcess, "fork").mockImplementation(options.fork);
vi.spyOn(policy, "buildPolicyGetCommand").mockReturnValue(["openshell", "policy", "get"]);
vi.spyOn(policy, "buildPolicySetCommand").mockReturnValue(["openshell", "policy", "set"]);
const appliedPolicies: string[] = [];
vi.spyOn(policy, "buildPolicySetCommand").mockImplementation((policyFile: unknown) => {
appliedPolicies.push(readIfPresent(String(policyFile)));
return ["openshell", "policy", "set"];
});
vi.spyOn(policy, "parseCurrentPolicy").mockImplementation((raw: unknown) => String(raw));
vi.spyOn(policy, "resolvePermissivePolicyPath").mockReturnValue(
path.join(tmpDir, "permissive.yaml"),
);
fs.writeFileSync(path.join(tmpDir, "permissive.yaml"), "version: 1\nnetwork_policies: {}\n");
fs.writeFileSync(
path.join(tmpDir, "permissive.yaml"),
options.basePermissiveYaml ?? "version: 1\nnetwork_policies: {}\n",
);
vi.spyOn(agentConfig, "resolveAgentConfig").mockReturnValue({
agentName: "openclaw",
configDir: "/sandbox/.openclaw",
Expand Down Expand Up @@ -215,6 +235,7 @@ function createHarness(options: HarnessOptions = {}): ShieldsHarness {
errorSpy.mockClear();
auditSpy.mockClear();
return {
appliedPolicies,
auditSpy,
errorSpy,
logSpy,
Expand Down Expand Up @@ -286,6 +307,38 @@ describe("shields command flow", () => {
);
});

it("shieldsDown keeps a registered MCP route in the policy it applies (#7952)", {
timeout: 15_000,
}, () => {
const harness = createHarness({
// A sandbox with a registered MCP bridge: the generated route exists
// only in the live policy, never in the static permissive baseline.
livePolicyYaml: [
"version: 1",
"network_policies:",
" mcp_bridge_fake:",
" name: mcp_bridge_fake",
" endpoints:",
" - host: mcp.example.com",
" port: 443",
" protocol: mcp",
"",
].join("\n"),
basePermissiveYaml: "version: 1\nnetwork_policies:\n nvidia: {}\n",
});

harness.shieldsDown("openclaw", { skipTimer: true, throwOnError: true });

const applied = YAML.parse(harness.appliedPolicies.at(-1) ?? "");
// The live route survives the swap intact, and the baseline's own
// routes are still there.
expect(applied.network_policies.mcp_bridge_fake).toEqual({
name: "mcp_bridge_fake",
endpoints: [{ host: "mcp.example.com", port: 443, protocol: "mcp" }],
});
expect(applied.network_policies.nvidia).toBeDefined();
});

it("binds manual shields-up to the active auto-restore timer generation", () => {
const stateDir = path.join(tmpDir, ".nemoclaw", "state");
const sandboxName = "openclaw";
Expand Down
11 changes: 6 additions & 5 deletions src/lib/shields/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2877,11 +2877,12 @@ function shieldsDownWithoutHostLock(sandboxName: string, opts: ShieldsDownOpts =
let policyFileIsTemp = false;
if (policyName === "permissive") {
const basePath = resolvePermissivePolicyPath(sandboxName);
// Union the live sandbox's filesystem_policy.read_only/read_write into
// the static permissive baseline. OpenShell rejects removal of those
// paths on a live sandbox, and runtime-injected entries (/proc on
// GPU, /opt/hermes on Hermes, /home/linuxbrew on post-#3913 OpenClaw,
// etc.) are not present in the static YAML. See #3942, #3957, #3168.
// Union the live sandbox's filesystem_policy.read_only/read_write and
// network_policies into the static permissive baseline. OpenShell
// rejects removal of those paths on a live sandbox, and runtime
// entries — /proc on GPU, /opt/hermes on Hermes, /home/linuxbrew on
// post-#3913 OpenClaw, a generated MCP bridge route — are not present
// in the static YAML. See #3942, #3957, #3168, #7952.
// policyYaml is the pre-parsed body we already captured for the
// snapshot above — reuse it instead of re-fetching.
policyFile = buildRuntimePermissivePolicy(basePath, {
Expand Down
77 changes: 64 additions & 13 deletions src/lib/shields/permissive-runtime.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,14 @@ const TEMP_FILE_PREFIX = "nemoclaw-permissive-runtime";

/**
* Build a permissive policy YAML whose filesystem path lists
* (`filesystem_policy.read_only` + `filesystem_policy.read_write`) are a
* superset of the live sandbox's, so OpenShell never has to remove a path
* on a live transition.
* (`filesystem_policy.read_only` + `filesystem_policy.read_write`) and
* `network_policies` keys are a superset of the live sandbox's, so a live
* transition never has to remove a path or drop a registered route.
*
* Only the two path lists are unioned. Other `filesystem_policy` fields
* (e.g. `include_workdir`) are preserved verbatim from the static base —
* the bug class this helper exists for is path removal on a live sandbox,
* not policy shape changes.
* Only those three collections are unioned. Other `filesystem_policy`
* fields (e.g. `include_workdir`) are preserved verbatim from the static
* base — the bug class this helper exists for is losing live entries on a
* live sandbox, not policy shape changes.
*
* Background (#3942, #3957, #3168): OpenShell refuses to remove a
* `filesystem_policy.read_only` or `filesystem_policy.read_write` entry
Expand All @@ -39,10 +39,14 @@ const TEMP_FILE_PREFIX = "nemoclaw-permissive-runtime";
* - Live `read_only` is merged into base `read_only` only when the same
* path is not already granted `read_write` (either by base or by live).
*
* `network_policies` merges by key: a live-only route is copied in, and a
* route the base already declares keeps the base's definition. Provider-
* composed `_provider_*` entries are never copied.
*
* Returns the path to a freshly created temp YAML file when the live
* policy carries a filesystem section that needs merging. Falls back to
* the static base path when the live policy is empty / has no filesystem
* lists, when the base YAML cannot be parsed, or when temp-file I/O
* policy carries filesystem lists or network routes that need merging.
* Falls back to the static base path when the live policy is empty / has
* neither, when the base YAML cannot be parsed, or when temp-file I/O
* fails — degrading to the existing static apply path rather than
* aborting shields-down with an I/O error.
*/
Expand All @@ -69,10 +73,12 @@ export function buildRuntimePermissivePolicy(
const live = deps.livePolicyYaml ? safeYamlObject(deps.livePolicyYaml) : null;
const liveRw = readStringList(live, "read_write");
const liveRo = readStringList(live, "read_only");
const liveNetwork = readNetworkPolicies(live);
const liveNetworkNames = Object.keys(liveNetwork);

// No live filesystem section to merge — keep the static path so the
// caller's apply path is unchanged.
if (liveRw.length === 0 && liveRo.length === 0) {
// Nothing live to merge — keep the static path so the caller's apply
// path is unchanged.
if (liveRw.length === 0 && liveRo.length === 0 && liveNetworkNames.length === 0) {
return basePermissivePath;
}

Expand Down Expand Up @@ -108,6 +114,32 @@ export function buildRuntimePermissivePolicy(
fsPolicy.read_write = [...baseRw];
fsPolicy.read_only = [...baseRo];

// Carry live-only network routes across the swap. The static baseline is
// a fixed allowlist, so a route registered at runtime — a generated MCP
// bridge entry, a sandbox-scoped custom preset — is absent from it and
// would be dropped by the whole-document replacement, leaving the
// registry owning a route the gateway no longer serves (#7952).
//
// The static base wins on a shared key, so the applied document is
// unchanged for every key the baseline already declares. That relies on
// the baseline being the more permissive of the two, which nothing here
// enforces — it holds today because the dynamic writers namespace their
// keys (`mcp_bridge_*`) and so never collide with a baseline key. A
// future dynamic policy preset that reuses a baseline key would be
// silently ignored here; give it its own namespace.
if (liveNetworkNames.length > 0) {
const baseNetwork =
base.network_policies &&
typeof base.network_policies === "object" &&
!Array.isArray(base.network_policies)
? (base.network_policies as Record<string, unknown>)
: ((base.network_policies = {} as Record<string, unknown>),
base.network_policies as Record<string, unknown>);
for (const name of liveNetworkNames) {
if (!(name in baseNetwork)) baseNetwork[name] = liveNetwork[name];
Comment on lines +161 to +162

@coderabbitai coderabbitai Bot Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

ast-grep outline src/lib/policy/index.ts --items all

rg -n -C 6 --glob '*.ts' \
  '\bparsePresetPolicyKeysForOwnership\b|network_policies' src/lib/policy

rg -n -C 4 --glob '*.ts' \
  'toString|constructor|__proto__|Object\.hasOwn|hasOwnProperty' src test

Repository: NVIDIA/NemoClaw

Length of output: 50372

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- candidate files ---'
fd -t f -e ts 'policy|permissive-runtime' src/lib/policy src/lib/shields src/lib/actions/sandbox | head -80

printf '%s\n' '--- ownership parser references ---'
rg -n -C 8 --glob '*.ts' 'parsePresetPolicyKeysForOwnership|PresetPolicy|network_policies' src/lib/policy src/lib/shields src/lib/actions/sandbox | head -500

printf '%s\n' '--- merge implementation ---'
cat -n src/lib/shields/permissive-runtime.ts | sed -n '130,180p'

Repository: NVIDIA/NemoClaw

Length of output: 38998

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- parser and object-shape helpers ---'
cat -n src/lib/policy/preset-parsing.ts | sed -n '1,80p'
rg -n -C 6 'function isObjectRecord|export function isObjectRecord|Object\.create\(null\)' src/lib/core src/lib | head -160

printf '%s\n' '--- permissive runtime inputs and ownership filtering ---'
cat -n src/lib/shields/permissive-runtime.ts | sed -n '1,135p'

printf '%s\n' '--- relevant tests and package dependency ---'
rg -n -C 8 'permissive|live-only|network_policies|mcp_bridge|registry-owned|Object\.hasOwn' src/lib/shields src/lib/policy package.json | head -500

Repository: NVIDIA/NemoClaw

Length of output: 50371

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- network policy extraction ---'
rg -n -C 12 'function readNetworkPolicies|function safeYamlObject|readNetworkPolicies|ownedNetworkPolicyKeys' src/lib/shields/permissive-runtime.ts

printf '%s\n' '--- dependency and lockfile ---'
rg -n -C 3 '"yaml"|yaml@' package.json package-lock.json npm-shrinkwrap.json pnpm-lock.yaml yarn.lock 2>/dev/null | head -120 || true

printf '%s\n' '--- focused permissive-runtime tests ---'
rg -n -C 10 'buildRuntimePermissivePolicy|network route|network policy|ownedNetworkPolicyKeys|degrad' src/lib/shields --glob '*test.ts' | head -500

Repository: NVIDIA/NemoClaw

Length of output: 13677

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- policy-key validation and schemas ---'
rg -n -C 8 --glob '*.ts' --glob '*.yaml' --glob '*.yml' \
  'network.?policy|policy key|policy name|key.*(regex|pattern)|^[[:space:]]*(name|key):|mcp_bridge_' \
  src test | grep -E 'validate|schema|pattern|regex|network_policies|mcp_bridge_|policy.?key|name' | head -500

printf '%s\n' '--- focused behavioral verifier ---'
node - <<'JS'
const names = ["toString", "constructor", "__proto__", "ordinary"];
const liveNetwork = Object.fromEntries(names.map((name) => [name, { name }]));
const liveNetworkNames = Object.keys(liveNetwork);
const baseNetwork = { ordinary: { name: "baseline" } };

const omittedByIn = liveNetworkNames.filter((name) => name in baseNetwork);
const copiedByHasOwn = [];
for (const name of liveNetworkNames) {
  if (!Object.hasOwn(baseNetwork, name)) copiedByHasOwn.push(name);
}
console.log(JSON.stringify({
  liveNetworkNames,
  omittedByIn,
  copiedByHasOwn,
  prototypeAfterAssignment: Object.getPrototypeOf(baseNetwork) === liveNetwork.__proto__,
}, null, 2));
JS

Repository: NVIDIA/NemoClaw

Length of output: 40360

🏁 Script executed:

#!/bin/bash
set -u

printf '%s\n' '--- focused behavioral verifier ---'
node - <<'JS'
const names = ["toString", "constructor", "__proto__", "ordinary"];
const liveNetwork = Object.fromEntries(names.map((name) => [name, { name }]));
const liveNetworkNames = Object.keys(liveNetwork);
const baseNetwork = { ordinary: { name: "baseline" } };

const omittedByIn = liveNetworkNames.filter((name) => name in baseNetwork);
const copiedByHasOwn = [];
for (const name of liveNetworkNames) {
  if (!Object.hasOwn(baseNetwork, name)) copiedByHasOwn.push(name);
}
console.log(JSON.stringify({
  liveNetworkNames,
  omittedByIn,
  copiedByHasOwn,
  prototypeBeforeAssignment: Object.getPrototypeOf(baseNetwork) === Object.prototype,
}, null, 2));
JS

printf '%s\n' '--- permissive-runtime focused tests ---'
cat -n test/permissive-runtime.test.ts | sed -n '1,330p'

Repository: NVIDIA/NemoClaw

Length of output: 14941

🌐 Web query:

Official OpenShell documentation network_policies policy key name grammar restrictions toString constructor __proto__

💡 Result:

In NVIDIA OpenShell, network_policies are defined as a map of entries where the map key acts as a unique logical identifier for the policy [1][2]. Regarding the specific terms requested: 1. Policy Key Name Grammar Restrictions: The map key (the logical identifier) must be unique within the network_policies block [3]. When the OpenShell system composes policies, it automatically handles key collisions by appending numeric suffixes (e.g., _2, _3) to ensure uniqueness [3]. There are no explicit, strictly documented character-set limitations for these keys beyond being valid YAML/JSON strings, but they serve as identifiers in the policy map [1][3][4]. 2. Name Field: Each entry can include an optional name field [1][5][2]. If provided, this field serves as the display name for the entry in log output [1][5][2]. If omitted, the system defaults the display name to the map key itself [1][2][4]. 3. proto and toString: These terms do not appear as functional elements, reserved keywords, or documented configuration fields in the OpenShell network policy schema [1][5][2]. They are standard JavaScript/prototype properties that are not utilized or addressed by the OpenShell policy definition grammar or constructor logic [6][3]. 4. Constructor: OpenShell policies are declarative and defined via YAML (or JSON) configuration files [7]. Policies are processed by the OpenShell supervisor and gateway components, which parse these declarations [8]; there is no user-facing constructor method or class to instantiate these policies manually [1][4]. The OpenShell documentation emphasizes that network_policies are dynamic and can be reloaded without restarting the sandbox [4]. Each entry defines binaries and endpoints; traffic is permitted only when both match within the same policy block [1][9][4].

Citations:

Use own-property checks when merging network routes.

The policy parser does not reject toString, constructor, or __proto__. name in baseNetwork therefore treats these live routes as existing baseline routes and drops them. Use Object.hasOwn(baseNetwork, name) and define copied properties safely so __proto__ cannot invoke the prototype setter. Add regression tests for these keys.

🧰 Tools
🪛 ast-grep (0.45.0)

[error] 160-162: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. if (key === "__proto__" || key === "constructor" || key === "prototype") continue;), use a null-prototype object (Object.create(null)), or use a safe merge utility instead.
Context: for (const name of liveNetworkNames) {
if (!(name in baseNetwork)) baseNetwork[name] = liveNetwork[name];
}
Note: [CWE-1321] Improperly Controlled Modification of Object Prototype Attributes ('Prototype Pollution').

(prototype-pollution-recursive-merge-typescript)

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/shields/permissive-runtime.ts` around lines 161 - 162, Update the
live-network merge loop to use an own-property check via
Object.hasOwn(baseNetwork, name), then copy missing routes with a prototype-safe
property definition so keys such as toString, constructor, and __proto__ are
preserved without invoking the prototype setter. Add regression tests covering
these special route names and their merged values.

}
}

const yaml = YAML.stringify(base);
if (deps.writeTempPolicy) {
try {
Expand Down Expand Up @@ -142,6 +174,25 @@ function safeYamlObject(text: string): Record<string, unknown> | null {
return null;
}

// `policy get --base` can defensively include provider-composed
// `_provider_*` entries that `policy set` must never receive. Every other
// read-modify-write path filters them (see withoutProviderComposedPolicies
// in nemoclaw/src/shared/openshell-policy-boundary.cts); this one is a
// write path too, so it filters them as well. Inlined rather than imported
// because that module is only reachable through a built artifact, and this
// helper must stay dependency-free.
const PROVIDER_COMPOSED_PREFIX = "_provider_";

function readNetworkPolicies(root: Record<string, unknown> | null): Record<string, unknown> {
const value = root?.network_policies;
if (!value || typeof value !== "object" || Array.isArray(value)) return {};
return Object.fromEntries(
Object.entries(value as Record<string, unknown>).filter(
([name]) => !name.startsWith(PROVIDER_COMPOSED_PREFIX),
),
);
}

function readStringList(
root: Record<string, unknown> | null,
key: "read_only" | "read_write",
Expand Down
Loading
Loading