fix: assorted small validation and robustness fixes - #2450
fix: assorted small validation and robustness fixes#2450andrewwhitecdw wants to merge 9 commits into
Conversation
|
I have read the DCO document and I hereby sign the DCO. |
|
All contributors have signed the DCO ✍️ ✅ |
johntmyers
left a comment
There was a problem hiding this comment.
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_errorcrossing 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.
| "·".dimmed(), | ||
| resp.version, | ||
| &resp.policy_hash[..12] | ||
| short_hash(&resp.policy_hash) |
There was a problem hiding this comment.
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.
| }; | ||
| 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 |
There was a problem hiding this comment.
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>
BlockedHead SHA: 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 Next action: @andrewwhitecdw, please rebase or merge |
45b4249 to
70cd90c
Compare
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>
|
Rebased onto |
johntmyers
left a comment
There was a problem hiding this comment.
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) andcrates/openshell-cli/src/run.rs:6733(sandbox_draft_approve). These unchanged lines still byte-slicepolicy_hash; the latter'smin(len)protects length but not UTF-8 boundaries. Please replace them withshort_hash(&response.policy_hash)andshort_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
left a comment
There was a problem hiding this comment.
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.
| // High port numbers may indicate ephemeral services. | ||
| if port > 49152 { | ||
| // The IANA dynamic/private (ephemeral) range is 49152-65535 inclusive. | ||
| if port >= 49152 { |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
|
/ok to test 389dc8c |
|
Label |
|
Fixed — corrected the comment for |
BlockedHead SHA: Thanks @andrewwhitecdw. I checked your latest update and confirmed that the corrected GitHub currently reports this new head as conflicting with Next action: please rebase or merge |
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>
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>
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>
351cf55 to
11668cd
Compare
Review Convergence CheckpointHead SHA: Three finding-bearing review rounds have completed. Thanks @andrewwhitecdw. I checked your latest rebase and the author-only delta: the earlier seven PR commits remain equivalent, and the new test commit corrects the Root-cause findings:
Scope growth:
Reviewer-quality signals:
Maintainer action: @NVIDIA/openshell-maintainers, please accept the current scope, split follow-up work, waive an obligation, or explicitly authorize another autonomous review round. Next state: |
|
/ok to test 11668cd |
Review Convergence CheckpointHead SHA: Three finding-bearing review rounds have completed. The latest author-only delta from Root-cause findings:
Scope growth:
Reviewer-quality signals:
Maintainer action: @NVIDIA/openshell-maintainers, please accept the current scope, split follow-up work, waive an obligation, or explicitly authorize another autonomous review round. Next state: |
|
/ok to test c6648a1 |
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>
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>
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>
aaaaaaaaaaaaé has a valid boundary at byte 12, so the old byte-slice would not panic for that input; correct the comment and add aaaaaaaaaaaéx (11 'a' + 2-byte 'é' + 'x'), where a byte-slice at 12 would split 'é', expecting aaaaaaaaaaaé. Verified with cargo test -p openshell-cli --lib short_hash (3 passed). Signed-off-by: Andrew White <andrewh@cdw.com>
c6648a1 to
5d14ccd
Compare
Review Convergence CheckpointHead SHA: Three finding-bearing review rounds have completed. This head rebases the same effective author patch reviewed at the prior convergence checkpoint: its stable patch ID still matches Root-cause findings:
Scope growth:
Reviewer-quality signals:
E2E: Maintainer action: @NVIDIA/openshell-maintainers, please accept the current scope, split follow-up work, waive an obligation, or explicitly authorize another autonomous review round. Next state: |
|
/ok to test 5d14ccd |
Summary
Three small, independent robustness fixes found during code review, one commit each:
is_alphanumeric(), so labels likecafé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.print_policy_revision_tabletruncated server-suppliedload_errorstrings 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).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: useis_ascii_alphanumeric(); added Unicode rejection testsprint_policy_revision_table: back off to a char boundary when truncatingload_errormechanistic_mapper:port >= 49152for the ephemeral range noteTesting
mise run pre-commitpasses (mise unavailable in this environment; ran equivalentcargo fmt+cargo clippyon all touched crates — clean)cargo test -p openshell-server validate_label— 40 passed, incl. 2 new)Checklist