fix(supervisor-network): L7 endpoint validation edge cases by andrewwhitecdw · Pull Request #2464 · NVIDIA/OpenShell

andrewwhitecdw · 2026-07-24T11:32:09Z

Summary

Two small robustness fixes in L7 network policy handling:

validate_l7_policies assumed every entry in network_policies.<name>.endpoints was a JSON object, and expand_access_presets called ep.as_object_mut().unwrap(). A malformed non-object endpoint caused a panic. This change adds an object-shape validation error and replaces the unwraps with safe pattern matching.
The scalar port field was already validated to be > 0, but the ports array accepted 0 values silently. The same inconsistency existed in the agent-proposal endpoint parser. Both paths now filter out zero ports.

Related Issue

N/A — small fixes found during code review.

Changes

l7/mod.rs: validate endpoint is an object; replace as_object_mut().unwrap() with safe matching; filter ports array to > 0
policy_local.rs: filter zero entries from endpoint.ports

Testing

mise run pre-commit passes (mise unavailable in this environment; ran equivalent cargo fmt + cargo clippy -p openshell-supervisor-network --all-targets — clean)
Unit tests pass (cargo test -p openshell-supervisor-network --lib — 969 passed)
E2E tests added/updated (if applicable)

Checklist

Follows Conventional Commits
Commits are signed off (DCO)
Architecture docs updated (if applicable)

…orts validate_l7_policies assumed every endpoint entry was a JSON object and used ep.as_object_mut().unwrap() in expand_access_presets. A non-object endpoint would panic. Add an object-shape validation error and replace the unwraps with safe pattern matching. Also filter zero values out of the ports array to match the scalar port validation (which already rejects port == 0). Signed-off-by: Andrew White <andrewh@cdw.com>

…arser network_endpoint_from_json accepted port 0 from the ports array without filtering, creating an endpoint with no usable ports. Drop zero entries so the array behaves consistently with the scalar port field. Signed-off-by: Andrew White <andrewh@cdw.com>

copy-pr-bot · 2026-07-24T11:32:15Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

andrewwhitecdw · 2026-07-24T11:32:18Z

I have read the DCO document and I hereby sign the DCO.

johntmyers

gator-agent

PR Review Status

Validation: This is project-valid small, concentrated work in supervisor network-policy handling: it replaces a malformed-input panic with fail-closed validation and attempts to make zero-port handling consistent. No duplicate work was found.
Head SHA: 583f62ef7019a4c54b23fe91e6ef289c2ca02d4a

Review findings:

One correctness issue is anchored inline: the raw-policy zero-port filter does not normalize the policy data later supplied to OPA, so mixed arrays still retain zero.
Focused regression coverage is missing for all three changed contracts. Please add unit tests showing that a non-object endpoint reports the expected validation error and preset expansion remains panic-free; raw-policy [0] and [0, 443] follow the intended contract; and agent proposals reject an all-zero array while retaining only positive entries from mixed arrays, including scalar fallback.

Docs: Fern docs are not needed for this malformed-input robustness fix; the existing policy reference already describes endpoint objects and concrete MCP ports.

E2E: The changed network-policy path requires test:e2e after review findings are resolved.

Next state: gator:in-review pending an author update.

johntmyers · 2026-07-24T18:20:43Z

+                |arr| {
+                    arr.iter()
+                        .filter_map(serde_json::Value::as_u64)
+                        .filter(|p| *p > 0)


gator-agent

Warning: This filters only a temporary vector whose sole consumer already checks any(|port| *port > 0). Consequently, [0] was already rejected, while [0, 443] still passes and the zero remains in the JSON supplied to OPA. Please either reject any zero-valued ports member or remove zeros during endpoint normalization/proto serialization, then cover all-zero and mixed arrays with regression tests.

I will get to these later today thanks for raising this issue.

johntmyers · 2026-07-28T14:50:30Z

@andrewwhitecdw Several of your PRs are awaiting feedback, please let us know if you plan on looking so we can plan accordingly

…ion tests Addresses gator-agent review feedback on NVIDIA#2464: - Filter zero values from endpoint ports arrays in normalize_endpoint_ports so OPA never sees a zero port. - Promote positive scalar port to ports array; leave all-zero arrays empty. - Skip non-object endpoints during normalization instead of panicking. - Add regression tests for non-object endpoint validation, zero-port filtering, and scalar fallback. Signed-off-by: Andrew White <andrewh@cdw.com>

johntmyers

gator-agent

PR Review Status

Validation: This remains project-valid small, concentrated robustness work in supervisor network-policy handling.
Head SHA: 200ecd2f0444b5ef60d33c2332990c546e521dc5

Thanks @andrewwhitecdw. I checked the update you pushed after the earlier zero-port normalization and test feedback. The raw-YAML mixed-array case is now normalized and the added helper tests cover that path, but the production proto load/reload path still bypasses the new normalizer.

Review findings:

One critical correctness/security-boundary issue is anchored inline: zero ports still reach OPA through OpaEngine::from_proto.
One warning is anchored inline: the separate agent-proposal parser behavior still needs focused regression coverage.

Docs: Fern docs are not needed because this hardens malformed input without changing supported policy syntax or commands.

E2E: The network-policy path still requires test:e2e after review findings are resolved.

Next state: gator:in-review pending an author update.

johntmyers · 2026-07-28T20:10:13Z

+            // If "ports" already exists, filter out zero values so OPA never
+            // sees a zero port. An all-zero array becomes empty and falls back
+            // to scalar "port" promotion below.
+            if let Some(ports) = ep_obj.get_mut("ports").and_then(|v| v.as_array_mut()) {


gator-agent

Critical (CWE-20): This normalizer runs only from preprocess_yaml_data; production OpaEngine::from_proto loads proto_to_opa_data_json without calling it. validate_l7_policies filters only a temporary vector, so a CLI policy with ports: [0, 443] still reaches OPA containing zero during normal sandbox load/reload. Please filter in proto_to_opa_data_json or apply shared normalization to proto-generated JSON, then add an OpaEngine::from_proto regression test.

johntmyers · 2026-07-28T20:10:14Z

    }

    let mut ports = endpoint.ports;
+    ports.retain(|p| *p > 0);


gator-agent

Warning (CWE-20): This changes the untrusted agent-proposal parser without testing that path. Please add proposal_chunks_from_body cases for mixed [0, 443], zero-only [0] rejection, and [0] with positive scalar fallback, asserting the resulting proto port and ports. The new OPA helper tests cannot protect this separate parser.

andrewwhitecdw · 2026-07-28T20:10:45Z

Thanks for the nudge — I’ve gone through all five open PRs:

fix(core): bracket IPv6 bind addresses in forward arg and access URL #2449: reused bracket_ipv6() in format_gateway_url() and added an IPv6 example to the Port Forwarding docs.
fix: assorted numeric overflow fixes #2462: tightened Docker/Podman numeric boundary checks so inputs rounding to 2^63/2^64 are rejected instead of saturating, and replaced saturating_mul in the Podman health-check interval.
fix: assorted small validation and robustness fixes #2450: made short_hash() slice on character boundaries, switched the policy revision table to use it, and added tests for the ephemeral-port boundary.
fix(supervisor-network): L7 endpoint validation edge cases #2464 (this PR): normalized zero ports before OPA and added regression tests for non-object endpoints and mixed/all-zero port arrays.
fix(cli): avoid panic on multi-byte UTF-8 in --since duration #2446: no actionable gator findings; I replied to elezar’s question about using an external duration crate.

All relevant test suites pass. Please re-review when you have a chance.

andrewwhitecdw added 2 commits July 24, 2026 06:31

andrewwhitecdw requested review from a team, derekwaynecarr, maxamillion and mrunalp as code owners July 24, 2026 11:32

johntmyers reviewed Jul 24, 2026

View reviewed changes

johntmyers added the gator:in-review Gator is reviewing or awaiting PR review feedback label Jul 24, 2026

johntmyers reviewed Jul 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(supervisor-network): L7 endpoint validation edge cases - #2464

fix(supervisor-network): L7 endpoint validation edge cases#2464
andrewwhitecdw wants to merge 3 commits into
NVIDIA:mainfrom
andrewwhitecdw:fix-l7-policy-validation-edge-cases/aw

andrewwhitecdw commented Jul 24, 2026

Uh oh!

copy-pr-bot Bot commented Jul 24, 2026

Uh oh!

andrewwhitecdw commented Jul 24, 2026

Uh oh!

johntmyers left a comment

Uh oh!

johntmyers Jul 24, 2026

Uh oh!

andrewwhitecdw Jul 28, 2026

Uh oh!

johntmyers commented Jul 28, 2026

Uh oh!

johntmyers left a comment

Uh oh!

johntmyers Jul 28, 2026

Uh oh!

johntmyers Jul 28, 2026

Uh oh!

andrewwhitecdw commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

andrewwhitecdw commented Jul 24, 2026

Summary

Related Issue

Changes

Testing

Checklist

Uh oh!

copy-pr-bot Bot commented Jul 24, 2026

Uh oh!

andrewwhitecdw commented Jul 24, 2026

Uh oh!

johntmyers left a comment

Choose a reason for hiding this comment

PR Review Status

Uh oh!

johntmyers Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

andrewwhitecdw Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

johntmyers commented Jul 28, 2026

Uh oh!

johntmyers left a comment

Choose a reason for hiding this comment

PR Review Status

Uh oh!

johntmyers Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

johntmyers Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

andrewwhitecdw commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants