Skip to content

Commit 2ddf676

Browse files
authored
fix(policy): reject invalid custom preset YAML (Fixes #9406) (#9409)
<!-- markdownlint-disable MD041 --> ## Summary Custom preset application now fails closed when the full preset document cannot be parsed into a `network_policies` mapping. This prevents malformed metadata above a valid policy tail from bypassing the custom-preset guards. ## Related Issue Fixes #9406 ## Changes - Reject invalid or missing custom `network_policies` before reserved-key, `allowed_ips`, semantic, disclosure, or mutation work. - Add a regression for a duplicate-key preset whose extracted policy tail remains valid. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Review pending. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable. - Station profile/scenario: Not applicable. - Result: Not applicable. - Supporting evidence: Not applicable. ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [ ] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run --project cli src/lib/policy/preset-allowed-ips.test.ts` passed 14 tests. - [x] Applicable broad gate passed — `npm run test:changed` passed 139 files and 1,944 tests after the 25-test growth-guardrail lane passed. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Additional validation: - `npm run build:cli` passed. - `npm run typecheck:cli` passed. - `git diff --check` passed. --- Signed-off-by: Deepak Jain <deepujain@gmail.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Custom policy presets are now rejected when policy data is missing, malformed, or invalid. - Presets with duplicate metadata or invalid policy definitions no longer continue through the application process. - Clear error reporting helps identify rejected preset configurations and prevents unintended policy changes. - Valid custom presets continue to be applied and recorded as provided. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Deepak Jain <deepujain@gmail.com>
1 parent c0b1862 commit 2ddf676

3 files changed

Lines changed: 37 additions & 5 deletions

File tree

src/lib/policy/index.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2073,14 +2073,18 @@ function applyPresetContent(
20732073

20742074
if (options.custom) {
20752075
const np = parseNetworkPolicies(presetContent);
2076+
if (!np) {
2077+
console.error(` Preset '${presetName}' has invalid or missing network_policies.`);
2078+
return false;
2079+
}
20762080
const reservedKey = [OPENCLAW_NPM_PRESET_KEY, PERSONAL_OPEN_INTERNET_POLICY_KEY].find(
2077-
(key) => np && Object.prototype.hasOwnProperty.call(np, key),
2081+
(key) => Object.prototype.hasOwnProperty.call(np, key),
20782082
);
20792083
if (reservedKey) {
20802084
console.error(` Custom presets cannot own reserved network policy key '${reservedKey}'.`);
20812085
return false;
20822086
}
2083-
const hasGeneratedPins = np !== null && networkPoliciesHasAllowedIps(np);
2087+
const hasGeneratedPins = networkPoliciesHasAllowedIps(np);
20842088
const trustedPrivatePinsValid = isTrustedPrivatePolicyPinCapability(
20852089
presetContent,
20862090
options.custom.trustedPrivatePinCapability,

src/lib/policy/preset-allowed-ips.test.ts

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import * as fs from "node:fs";
66
import * as os from "node:os";
77
import * as path from "node:path";
88

9-
import { afterEach, beforeEach, describe, expect, it } from "vitest";
9+
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
1010

1111
import { applyPresetContent, loadPresetFromFile, networkPoliciesHasAllowedIps } from ".";
1212

@@ -209,6 +209,28 @@ describe("networkPoliciesHasAllowedIps prototype-chain guard (#6072)", () => {
209209
});
210210

211211
describe("applyPresetContent allowed_ips guard (#6073)", () => {
212+
it("rejects a custom preset when the full YAML document is invalid (#9406)", () => {
213+
const content = `preset: corp
214+
preset: corp
215+
network_policies:
216+
wide_open:
217+
endpoints:
218+
- allowed_ips: ["169.254.169.254"]
219+
`;
220+
const error = vi.spyOn(console, "error").mockImplementation(() => undefined);
221+
222+
expect(
223+
applyPresetContent("test-sandbox", "invalid-full-document", content, {
224+
custom: { sourcePath: "invalid.yaml" },
225+
}),
226+
).toBe(false);
227+
expect(error).toHaveBeenCalledWith(
228+
" Preset 'invalid-full-document' has invalid or missing network_policies.",
229+
);
230+
231+
error.mockRestore();
232+
});
233+
212234
it("rejects custom preset content containing allowed_ips before any side effects", () => {
213235
const content = `\
214236
preset:

test/policies.test.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -741,7 +741,13 @@ exit 1
741741
const registryModule = requireForTest(
742742
path.join(REPO_ROOT, "src", "lib", "state", "registry.ts"),
743743
) as Record<string, any>;
744-
const CUSTOM_CONTENT = "network_policies:\n slack-files-upload:\n host: files.slack.com\n";
744+
const CUSTOM_CONTENT = `preset:
745+
name: slack-files-upload
746+
description: Allow Slack file uploads
747+
network_policies:
748+
slack-files-upload:
749+
host: files.slack.com
750+
`;
745751
const BUILTIN_CONTENT = "network_policies:\n github:\n host: github.com\n";
746752
const SOURCE_PATH = "/tmp/slack-files-upload-case.yaml";
747753

@@ -841,7 +847,7 @@ exit 1
841847
}
842848
});
843849

844-
it("records the custom preset and returns true when the sandbox is registered", () => {
850+
it("applies a well-formed custom preset and records it verbatim (#9406)", () => {
845851
registryModule.getSandbox = (name: string) => ({ name });
846852
const addSpy = vi.fn(() => true);
847853
registryModule.addCustomPolicy = addSpy;

0 commit comments

Comments
 (0)