fix: assorted small validation and robustness fixes by andrewwhitecdw · Pull Request #2450 · NVIDIA/OpenShell

andrewwhitecdw · 2026-07-23T23:41:43Z

Summary

Three small, independent robustness fixes found during code review, one commit each:

server: Kubernetes label key/value validation used Unicode-aware is_alphanumeric(), so labels like café or 日本語 passed gateway validation but would be rejected by the Kubernetes API server (the label spec is ASCII-only). The same functions already validate the key prefix with ASCII-only checks.
cli: print_policy_revision_table truncated server-supplied load_error strings at a fixed byte index (&rev.load_error[..40]), panicking when the index lands inside a multi-byte UTF-8 character (same bug class as fix(cli): avoid panic on multi-byte UTF-8 in --since duration #2446).
sandbox: the ephemeral-port advisory check used port > 49152, omitting port 49152 itself from the IANA dynamic/private range (49152–65535 inclusive).

This PR supersedes #2410, which was auto-closed by the vouch-check workflow before I was vouched.

Related Issue

N/A — small fixes found during code review.

Changes

validate_label_key/validate_label_value: use is_ascii_alphanumeric(); added Unicode rejection tests
print_policy_revision_table: back off to a char boundary when truncating load_error
mechanistic_mapper: port >= 49152 for the ephemeral range note

Testing

mise run pre-commit passes (mise unavailable in this environment; ran equivalent cargo fmt + cargo clippy on all touched crates — clean)
Unit tests added/updated (cargo test -p openshell-server validate_label — 40 passed, incl. 2 new)
E2E tests added/updated (if applicable)

Checklist

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

copy-pr-bot · 2026-07-23T23:41:46Z

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-23T23:42:05Z

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

github-actions · 2026-07-23T23:42:14Z

All contributors have signed the DCO ✍️ ✅
_{Posted by the DCO Assistant Lite bot.}

johntmyers

gator-agent

PR Review Status

Validation: This is project-valid small, concentrated robustness work. PR #2410 is the same author's auto-closed predecessor, not competing active work.
Head SHA: 949b5b6be5d0634296691cf826d0c81e4f48c7f1

Review findings:

Two warning-level UTF-8 panic paths remain in CLI rendering; see the inline comments.
Suggested coverage: exercise a multibyte load_error crossing byte 40 and the ephemeral-port boundary at 49151/49152.

Docs: Fern docs and navigation are not needed because these fixes add no command, option, workflow, or documented contract.

Next state: gator:in-review; author changes are needed before pipeline/E2E gating.

johntmyers · 2026-07-24T18:19:39Z

            "·".dimmed(),
            resp.version,
-            &resp.policy_hash[..12]
+            short_hash(&resp.policy_hash)


gator-agent

Warning: short_hash still uses &hash[..12], so a server response such as aaaaaaaaaaaé panics when byte 12 splits a UTF-8 character (CWE-248). Make short_hash select the twelfth character boundary using char_indices() and add short/multibyte regression cases; that fixes both new call sites.

johntmyers · 2026-07-24T18:19:39Z

        };
        let error_short = if rev.load_error.len() > 40 {
-            format!("{}...", &rev.load_error[..40])
+            // Back off to a char boundary: byte-index slicing panics on


gator-agent

Warning: The revision table still byte-slices the server-supplied policy hash, preserving the same UTF-8 panic (CWE-248). After fixing short_hash, replace this branch with let hash_short = short_hash(&rev.policy_hash);.

Addresses gator-agent review feedback on NVIDIA#2450: - Make short_hash() slice at the 13th character boundary so multi-byte UTF-8 cannot panic. - Use short_hash() in the policy revision table instead of byte-index slicing. - Extract error-message truncation into truncate_error_message() and keep char-boundary backoff. - Add regression tests for short_hash, truncate_error_message, and the ephemeral-port inclusive boundary (49152). Signed-off-by: Andrew White <andrewh@cdw.com>

johntmyers · 2026-07-28T20:25:15Z

gator-agent

Blocked

Head SHA: 45b4249a3df044ed5ed459e4fbf1d056824bf645

Andrew’s latest commit appears to address the earlier UTF-8 slicing and boundary-test requests, but GitHub currently reports this branch as conflicting with main (mergeable: CONFLICTING, mergeStateStatus: DIRTY). Gator cannot complete a fresh independent review of the updated head until the conflict is resolved.

Next action: @andrewwhitecdw, please rebase or merge main, resolve the conflicts, and push the resulting update. Gator will review the new head and then make the E2E/test-label decision.

validate_label_key and validate_label_value used Unicode-aware char::is_alphanumeric(), so values like 'café' or '日本語' passed gateway validation even though the Kubernetes label spec (([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])? is ASCII-only and the API server rejects such labels later. The same functions already validate the key prefix with ASCII-only checks. Use is_ascii_alphanumeric() and add regression tests for Unicode keys and values. Signed-off-by: Andrew White <andrewh@cdw.com>

Two display paths in sandbox policy commands sliced server-supplied strings without guarding: - sandbox_policy_set used &resp.policy_hash[..12] unconditionally; an empty or short proto3 hash field would panic. Use the existing short_hash() helper, as nearby call sites already do. - The policy revision table truncated load_error at a fixed byte index (&rev.load_error[..40]), panicking on multi-byte UTF-8 in server error messages. Back off to a char boundary instead. Signed-off-by: Andrew White <andrewh@cdw.com>

The IANA dynamic/private (ephemeral) port range is 49152-65535 inclusive, but the check used port > 49152, silently omitting the advisory note for port 49152 itself. Signed-off-by: Andrew White <andrewh@cdw.com>

Addresses gator-agent review feedback on NVIDIA#2450: - Make short_hash() slice at the 13th character boundary so multi-byte UTF-8 cannot panic. - Use short_hash() in the policy revision table instead of byte-index slicing. - Extract error-message truncation into truncate_error_message() and keep char-boundary backoff. - Add regression tests for short_hash, truncate_error_message, and the ephemeral-port inclusive boundary (49152). Signed-off-by: Andrew White <andrewh@cdw.com>

andrewwhitecdw · 2026-07-28T21:11:21Z

Rebased onto main and resolved the merge conflict in crates/openshell-cli/src/run.rs (kept both the new sandbox_detail_to_json_* tests from main and the truncate_error_message_* tests from this PR). All relevant tests pass and the branch is now clean.

johntmyers

gator-agent

PR Review Status

Validation: This remains project-valid small, concentrated robustness work. PR #2410 is the same author's auto-closed predecessor, not competing active work.
Head SHA: 70cd90c823df9d6d8271779dc4c9c6d7f4d88fc8

Thanks @andrewwhitecdw. I checked your rebase and GitHub now reports the branch mergeable, so the earlier conflict blocker is resolved. I also verified that the updated short_hash, UTF-8 error truncation, and 49151/49152 boundary tests address the previous gator findings in the changed paths.

Review finding:

Warning (CWE-248): the same server-supplied UTF-8 panic pattern remains at crates/openshell-cli/src/run.rs:5381 (sandbox_policy_set_global) and crates/openshell-cli/src/run.rs:6733 (sandbox_draft_approve). These unchanged lines still byte-slice policy_hash; the latter's min(len) protects length but not UTF-8 boundaries. Please replace them with short_hash(&response.policy_hash) and short_hash(&inner.policy_hash) respectively. These locations are outside the current diff, so this is a general finding rather than an inline comment.

Docs: Fern docs and navigation are not needed because these are conformance and crash fixes, not new commands, options, workflows, or documented contracts.

Next state: gator:in-review; the two remaining hash display paths need an author update before pipeline/E2E gating.

Replaces the two remaining byte-slice policy_hash renderings in sandbox_policy_set_global and sandbox_draft_approve with the existing short_hash helper, avoiding UTF-8 panic paths on multibyte hashes. Also fixes a clippy map_unwrap_or warning in short_hash itself. Addresses review feedback in NVIDIA#2450. Signed-off-by: Andrew White <andrewh@cdw.com>

johntmyers

gator-agent

PR Review Status

Validation: This remains project-valid small, concentrated robustness work. PR #2410 is the same author's auto-closed predecessor, not competing active work.
Head SHA: 7d08bbe6526e87aad64fc2bd2802b49084a0f586

Thanks @andrewwhitecdw. I checked your current update and verified that both remaining policy-hash byte slices now use short_hash, resolving the prior UTF-8 panic finding.

Review finding:

One blocking gateway consistency defect remains; see the inline comment. The sandbox-side 49152 boundary is corrected, but the gateway's separate security-note recomputation still excludes that boundary.

Docs: Fern docs and navigation are not needed because these are conformance and crash fixes, not new commands, options, workflows, or documented contracts.

Next state: gator:in-review; the gateway boundary and regression coverage need an author update before pipeline/E2E gating.

johntmyers · 2026-07-28T22:10:55Z

    // High port numbers may indicate ephemeral services.
-    if port > 49152 {
+    // The IANA dynamic/private (ephemeral) range is 49152-65535 inclusive.
+    if port >= 49152 {


gator-agent

Critical (CWE-193; OWASP LLM06): This corrects only the sandbox-generated note. The gateway discards and recomputes notes through current_draft_chunk_security_notes(), while crates/openshell-server/src/grpc/policy.rs:3769 still uses port > 49152. Consequently, a port-49152 proposal can be considered note-free and auto-approved. Please update the gateway predicate to >= and add gateway boundary/auto-approval coverage.

The sandbox mechanistic_mapper already treated 49152 as ephemeral (port >= 49152), but the gateway's generate_security_notes used port > 49152, so the boundary port was inconsistently unflagged. Change the gateway check to port >= 49152 and add a regression test for the 49151/49152 boundary. Addresses review feedback in NVIDIA#2450. Signed-off-by: Andrew White <andrewh@cdw.com>

johntmyers

gator-agent

PR Review Status

Validation: This remains project-valid small, concentrated robustness work. PR #2410 is the same author's auto-closed predecessor, not competing active work.
Head SHA: 389dc8c138458107998e548ac5e3f9ff48aaa534

Thanks @andrewwhitecdw. I reviewed the current update and verified that the gateway now flags port 49152 consistently with the sandbox mapper, with gateway boundary coverage for 49151/49152. This resolves the prior security-note consistency finding. The independent code review found no remaining actionable findings; the UTF-8-safe hash and error truncation changes and their regression tests also remain sound.

Docs: Fern docs and navigation are not needed because these are conformance, crash, and security-note fixes, not new commands, options, workflows, or documented contracts.

E2E: test:e2e is required because this changes sandbox policy/security-note behavior.

Next state: gator:watch-pipeline.

johntmyers · 2026-07-28T22:32:36Z

/ok to test 389dc8c

github-actions · 2026-07-28T22:33:09Z

Label test:e2e applied for 389dc8c. Open the existing run 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

gator-agent

PR Review Status

Validation: This remains project-valid small, concentrated robustness work. PR #2410 is the same author's auto-closed predecessor, not competing active work.
Head SHA: 2c58ac294bfabe6e03ab4e4c2bc21fb268e212bf

The current head has the same tree as the previously reviewed head. The independent code review verified that the UTF-8-safe hash and error truncation, ASCII-only Kubernetes label validation, and consistent 49152 boundary handling remain sound, with no blocking findings.

One non-blocking test-comment correction is noted inline; it does not prevent pipeline monitoring.

Docs: Fern docs and navigation are not needed because these are conformance, crash, and security-note fixes, not new commands, options, workflows, or documented contracts.

E2E: test:e2e remains required because this changes sandbox policy/security-note behavior.

Next state: gator:watch-pipeline.

johntmyers · 2026-07-29T00:32:07Z

+    #[test]
+    fn short_hash_handles_multibyte_characters() {
+        // "aaaaaaaaaaaaé" is 14 bytes (12 'a' + one 2-byte 'é') and 13 chars.
+        // The old byte-slice at 12 would split 'é' and panic.


gator-agent

Suggestion: aaaaaaaaaaaaé has a valid UTF-8 boundary at byte 12, so the old implementation would not panic for this input. Please correct the comment or use a genuinely splitting case such as aaaaaaaaaaaéx, expecting aaaaaaaaaaaé. The subsequent assertion with all-multibyte input already exercises the panic regression, so this is non-blocking.

johntmyers · 2026-07-29T00:32:19Z

/ok to test 2c58ac2

andrewwhitecdw requested review from a team, derekwaynecarr, maxamillion and mrunalp as code owners July 23, 2026 23:41

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

andrewwhitecdw mentioned this pull request Jul 28, 2026

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

Open

6 tasks

johntmyers added gator:blocked Gator is blocked by process or repository gates and removed gator:in-review Gator is reviewing or awaiting PR review feedback labels Jul 28, 2026

andrewwhitecdw added 4 commits July 28, 2026 16:06

fix(sandbox): include port 49152 in ephemeral range note

0529632

The IANA dynamic/private (ephemeral) port range is 49152-65535 inclusive, but the check used port > 49152, silently omitting the advisory note for port 49152 itself. Signed-off-by: Andrew White <andrewh@cdw.com>

andrewwhitecdw force-pushed the fix-assorted-validation-robustness/aw branch from 45b4249 to 70cd90c Compare July 28, 2026 21:11

johntmyers reviewed Jul 28, 2026

View reviewed changes

johntmyers added gator:in-review Gator is reviewing or awaiting PR review feedback and removed gator:blocked Gator is blocked by process or repository gates labels Jul 28, 2026

johntmyers reviewed Jul 28, 2026

View reviewed changes

johntmyers added test:e2e Requires end-to-end coverage gator:watch-pipeline Gator is monitoring PR CI/CD status and removed gator:in-review Gator is reviewing or awaiting PR review feedback labels Jul 28, 2026

johntmyers added gator:in-review Gator is reviewing or awaiting PR review feedback and removed gator:watch-pipeline Gator is monitoring PR CI/CD status labels Jul 28, 2026

ci: trigger fresh CI run

2c58ac2

johntmyers reviewed Jul 29, 2026

View reviewed changes

johntmyers added gator:watch-pipeline Gator is monitoring PR CI/CD status and removed gator:in-review Gator is reviewing or awaiting PR review feedback labels Jul 29, 2026

johntmyers added gator:in-review Gator is reviewing or awaiting PR review feedback and removed gator:watch-pipeline Gator is monitoring PR CI/CD status labels Jul 29, 2026

Uh oh!

Conversation

andrewwhitecdw commented Jul 23, 2026

Summary

Related Issue

Changes

Testing

Checklist

Uh oh!

copy-pr-bot Bot commented Jul 23, 2026

Uh oh!

andrewwhitecdw commented Jul 23, 2026

Uh oh!

github-actions Bot commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

johntmyers Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

johntmyers commented Jul 28, 2026

Blocked

Uh oh!

andrewwhitecdw commented Jul 28, 2026

Uh oh!

johntmyers left a comment

Choose a reason for hiding this comment

PR Review Status

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 left a comment

Choose a reason for hiding this comment

PR Review Status

Uh oh!

johntmyers commented Jul 28, 2026

Uh oh!

github-actions Bot commented Jul 28, 2026

Uh oh!

johntmyers left a comment

Choose a reason for hiding this comment

PR Review Status

Uh oh!

johntmyers Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

johntmyers commented Jul 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Jul 23, 2026 •

edited

Loading