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

[andrewwhitecdw]

Summary

Two small robustness fixes in L7 network policy handling:

1. 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.
2. 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)

[copy-pr-bot]

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]

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]

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

[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.

[andrewwhitecdw]

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

• docs(sandboxes): add IPv6 loopback example to Port Forwarding #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]

Both review items addressed:

• Critical (opa.rs): proto_to_opa_data_json now filters zero ports in the production path, same as the YAML preprocess path — ports: [0, 443] → [443], zero-only arrays fall back to a positive scalar port, and a zero scalar port is not promoted. Zero ports can no longer reach OPA from OpaEngine::from_proto.
• Warning (policy_local.rs): added proposal_chunks_from_body tests for mixed [0, 443], zero-only [0] rejection, and [0] with positive scalar fallback, asserting the resulting port/ports.

Verified: cargo test -p openshell-supervisor-network --lib → 977 passed.

[johntmyers]

gator-agent

PR Review Status

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

Thanks @andrewwhitecdw. I checked your update about filtering zero ports in the production proto-to-OPA path and adding focused proposal-parser coverage. Those prior gaps are addressed, but the independent review found two remaining normalization cases that can reinterpret malformed or zero-only authorization input as a scalar-port allow; both are anchored inline. A focused preset-expansion regression gap is also anchored inline.

Review findings:

• Two blocking CWE-20 warnings are anchored inline: malformed JSON array elements can be discarded before scalar fallback, and zero-only proto arrays can activate a shadowed scalar despite documented ports precedence.
• One focused test gap is anchored inline for the removed expand_access_presets unwraps.

Docs: Fern docs are not needed for the intended malformed-input robustness fix. If scalar fallback for an explicitly present zero-only ports array is intentional, the schema and proto precedence contract would need updating.

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

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

[johntmyers]

gator-agent

Author Follow-Up Nudge

This PR has been in gator:in-review for more than 48 business hours with unresolved review feedback.

@andrewwhitecdw, please respond to the two blocking review comments about malformed array elements and zero-only proto-array precedence, or push an update. If this is no longer planned, please say so and a maintainer can close it out.

[johntmyers]

gator-agent

Author Follow-Up Nudge

This PR has been in gator:in-review for more than 48 business hours with unresolved review feedback.

@andrewwhitecdw, please respond to the two blocking review comments about malformed array elements and zero-only proto-array precedence, or push an update. If this is no longer planned, please say so and a maintainer can close it out.

[johntmyers]

/ok to test 805f189

[johntmyers]

gator-agent

PR Review Status

Validation: This remains project-valid small, concentrated robustness work in supervisor network-policy handling.
Head SHA: 805f189c9a35f241bcbf3395bbd0cea1bfaa5d7a
Base SHA: 0c7e59a95355cabc15ccaddb86fcbe6a1d30eaaa
Merge base SHA: 21da343c9f838bd9ac85dc61bf44889de1a72873
Patch ID: d8044cf19d18d7dd4ad2547d4467f81feea7a843
Gator payload: 3
Review mode: follow_up
Previous reviewed SHA: ed4a4b63649a5857e2405a8f72bbb62f99a93282

Thanks @andrewwhitecdw. I checked the update pushed after the malformed-array and zero-only proto-array feedback. The raw-policy normalizer now preserves malformed non-numeric entries for fail-closed validation, the proto conversion preserves explicit ports precedence, and focused regression coverage now exercises both cases plus preset expansion with a non-object endpoint.

Blocking findings:

• No blocking findings remain.

Carried findings:

• gator-inline-3679152284: resolved by preserving non-numeric array entries through normalization.
• gator-inline-3679152285: resolved by branching on whether the original proto ports array is present.

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

E2E: test:e2e is applied. The mirror was absent, so an authorized maintainer command /ok to test 805f189c9a35f241bcbf3395bbd0cea1bfaa5d7a was posted.

Next state: gator:watch-pipeline

[github-actions]

Label test:e2e applied for 805f189. Open Branch E2E Checks, find the run for commit 805f189, and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

[johntmyers]

/ok to test 805f189

[johntmyers]

gator-agent

Merge Decision Nudge

This PR has been in gator:merge-ready for more than 48 business hours with maintainer approval present and no merge or close decision.

@NVIDIA/openshell-maintainers @NVIDIA/openshell-codeowners @mrunalp @maxamillion @derekwaynecarr, can someone merge this PR or close/request changes if it should not proceed?

[andrewwhitecdw]

Closing this sweep-generated PR: PR has 7 commits; sweep requires exactly one commit per PR. It does not meet the sweep requirements (single signed-off commit).

[johntmyers]

gator-agent

Monitoring Complete

Head SHA: e71e6af814aae046be1b93fc31e8818ff214aedb
Gator payload: 4

Monitoring is complete because this PR has been closed without merge.

Final status: The durable finding ledger records all review findings addressed or non-blocking, no remediation scope growth, and no newly introduced Critical defect. Required Branch Checks, Helm Lint, and E2E status contexts were green, and maintainer approval was present. The author closed the PR because the sweep requires exactly one signed-off commit and this PR contains seven commits.

I removed the active gator:* label because there is nothing left for gator to monitor on this PR.

[github-actions]

This pull request has had no activity for 14 days and is now marked stale. It may be closed in 7 days if there is no further activity.

[andrewwhitecdw]

Thanks for the walkthrough and the convergence checkpoints — much appreciated. Confirming I'm still actively engaging on this one (and on the other awaiting-feedback PRs): all five got updates before Jul 30, and #2449 has its status summarized here.

Current state of this PR: the reviewer obligations from the checkpoints (zero-port normalization across the proto-to-OPA path, fail-closed malformed array elements, ports-array precedence over scalar port, and the regression tests) are addressed in the pushed commits; the Clippy-driven boolean rewrite is in. Nothing is pending on my side — let me know if you'd like anything rebased or reworked.

[github-actions]

This pull request has had no activity for 14 days and is now marked stale. It may be closed in 7 days if there is no further activity.

[andrewwhitecdw]

Bumping this to keep it from going stale — still actively engaged and the branch remains ready for review. All prior review feedback is addressed in the current head; nothing is waiting on my side. Happy to make further changes if anything new comes up.

[github-actions]

This pull request has had no activity for 14 days and is now marked stale. It may be closed in 7 days if there is no further activity.

[johntmyers]

gator-agent

Follow-Up Needed

This PR is partially applicable to current main, rather than wholly obsolete or ready to advance unchanged. I fetched GitHub's latest main tip, fe316067dc2503062235106b886c206e9d8ca079, and compared it with PR head a3d00b967498c4c8e0e452e64fa6110ecb2d9d53. There is no directly linked issue in the PR body or closing-issue references.

Thanks @andrewwhitecdw. Your September updates say the prior normalization, precedence, and regression-test feedback is addressed. I checked the current head and durable review history: all six recorded Gator threads are resolved by verified maintainer @johntmyers. Those resolutions remain intact; this is an applicability check against newer main, not a reopening of those findings.

What has already been covered on main:

• Non-object endpoints: #3337, landed as 8751e35e28782246de9c01721ea4e48155e0331e, added structural rejection before normalization or preset expansion. Current opa.rs:1448 checks endpoint object-array entries; preprocess_yaml_data calls it at line 1670. yaml_containers_reject_malformed_objects_and_collections at line 3070 covers null, string, numeric, boolean, and mixed endpoint entries. The original supported-loader problem is therefore covered. The unwraps remain in l7/mod.rs:1853,1878, but non-object values also lack the protocol/access fields needed to enter those branches; their presence alone does not establish a reachable panic.
• Production protobuf zero ports: #2711, landed as 4d7f402ce2ac88958c3fb3ae0018d7e1a17a9783, added effective-port validation. Current openshell-policy/src/lib.rs:1492 rejects every effective port outside 1..=65535. OpaEngine::from_proto validates/canonicalizes at opa.rs:475 before conversion at line 496. The regression validate_rejects_raw_endpoint_zero_and_out_of_range_ports at lib.rs:3649 covers zero. Thus the PR's conversion-time filtering and helper test at PR opa.rs:8106 no longer demonstrate a gap in the supported production loader: invalid input is rejected earlier.

Still-relevant scope:

• Raw YAML/JSON loading: main's opa.rs:1783 normalizer retains [0,443] and [0]; it does not implement the PR's zero filtering. The newer #3788 (798500ccdba8b064361cd1d45320d887424fd813) validates raw leaf types through opa/raw_schema.rs:46 before normalization, but the shared DTO uses u16/Vec<u16> (openshell-policy-schema/src/lib.rs:264,267), which still admit numeric zero. This remaining raw-loader behavior needs an explicit decision about rejection versus sanitization, including explicit-array precedence and malformed values. The PR's raw-normalizer tests at opa.rs:7984–8061 are relevant starting points, not proof of current-main loader behavior.
• Agent proposals: main's policy_local.rs:1137–1164 still retains zero entries when constructing proposal chunks; the PR filters them and adds tests at PR lines 1467,1476,1486. This parser difference remains relevant, but production policy validation now rejects zero ports. Decide whether proposals should reject malformed ports consistently or sanitize them, including the PR's [0] plus positive-scalar fallback.

The L7/OPA paths remain in use; shared semantics were extracted by #2389, rather than the entire affected architecture disappearing.

Action required: @NVIDIA/openshell-maintainers, decide whether to retain a narrowed raw-loader/proposal fix and specify rejection versus sanitization, or retire this implementation in favor of a separate current-main fix. @andrewwhitecdw, if retained, update the PR against current main around that scope, revise the summary, and add supported-loader/proposal regressions while preserving the existing resolutions. A maintainer owns any close decision; Gator will not close this PR.

This check used fetched code and existing tests; local tests were not executed because no Rust toolchain is available in this sandbox. Current-head Branch Checks, Helm Lint, and E2E statuses are pending. No new test dispatch or independent code-review round was started while applicability remains undecided.

Gator metadata

• Head SHA: a3d00b967498c4c8e0e452e64fa6110ecb2d9d53
• Current main SHA: fe316067dc2503062235106b886c206e9d8ca079
• Base SHA: 877ddbacb4b915fa1d5bc6f02302d9a5c13a18e1
• Merge base SHA: 0310cbed6c809e8950fc513d0a25c2ec03946198
• Patch ID: aba86ebabccc7c0f27b4f86ab62c0fc87dacf748
• Gator payload: 12
• Ledger review mode: critical_only; three finding-bearing rounds, zero duplicate finding IDs
• Review budget exhaustion is not the blocker; the concrete pending decision is current-main applicability and remaining input semantics.
• Next state: gator:follow-up-needed
• Follow-up reason: partial_current_main_applicability
• No automatic closure authorized; continue supervised monitoring.

[johntmyers]

Closing because newer validation supersedes much of this implementation. The remaining raw-policy and agent-proposal behavior should use consistent rejection of invalid ports rather than silently filtering them with different fallback rules. This closure does not assert that every input path already handles zero ports consistently.

[johntmyers]

gator-agent

Monitoring Complete

Monitoring is complete because PR #2464 was closed without merge by verified maintainer @johntmyers.

Thanks @johntmyers. Your closure explains that newer validation supersedes much of this implementation, while remaining raw-policy and agent-proposal behavior should consistently reject invalid ports rather than silently filter them with different fallback rules. I checked the latest PR state and your closure comment. The current-main applicability assessment and existing resolved review findings remain intact; closure does not establish that every input path already handles zero ports consistently.

The latest GitHub main tip fetched this cycle is 82977e27a438c8a6a1072498432f9f7c75a33a85. Because the PR is already closed, terminal reconciliation takes precedence over another applicability/code review or test dispatch. Any remaining work belongs in a maintainer-directed current-main follow-up using consistent rejection semantics.

I am removing the active gator:* label because there is nothing left for Gator to monitor on this PR.

Gator metadata

• Head SHA: a3d00b967498c4c8e0e452e64fa6110ecb2d9d53
• Gator payload: 12
• Terminal state: closed without merge