Reject NUL bytes in Git paths and consolidate validation tests - #60043
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Great work! 👋 This PR looks ready for review. The fix correctly rejects NUL bytes in Git paths (addressing the security concern from #60027), and the test consolidation is solid—you have modernized the validation tests into table-driven form and removed duplicate coverage across test files while keeping distinct edge cases intact. The implementation is focused, well-tested, and clearly documented. This looks good to merge once the draft status is addressed and CI passes. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
No ADR enforcement needed: PR does not have the 'implementation' label and has <=100 new lines of code in business logic directories.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
There was a problem hiding this comment.
🟢 Approval recommended
The focused validation fix is correct and retains comprehensive consolidated coverage.
Pull request overview
Rejects NUL bytes in Git paths and consolidates validation coverage.
Changes:
- Adds NUL-byte validation for Git paths.
- Consolidates ref/path cases into table-driven tests.
- Adds a shared validation-error assertion helper.
File summaries
| File | Description |
|---|---|
pkg/gitutil/gitutil.go |
Rejects NUL bytes in Git paths. |
pkg/gitutil/gitutil_test.go |
Removes duplicate validation tests. |
pkg/gitutil/gitutil_ctr_formal_test.go |
Centralizes validation cases and assertions. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — no blocking issues found.
📋 Analysis
/diagnosing-bugs
The NUL-byte fix in ValidateGitPath mirrors the existing ValidateGitRef check exactly (same strings.ContainsRune(path, '\x00') pattern, same error message style), addressing the root cause (unsanitized paths reaching the git subprocess) rather than a symptom. Placement before the IsAbs/traversal checks is correct since NUL can terminate C-string parsing in some git internals before those checks would matter.
/tdd
The table-driven consolidation is a genuine improvement:
- All previously distinct test cases (safe paths/refs, hyphen-prefix, absolute path, traversal, empty, NUL byte) are preserved — verified none were silently dropped when merging
gitutil_test.gointogitutil_ctr_formal_test.go. - The new
assertValidationErrorhelper removes duplicatedrequire.Error/ErrorContainsboilerplate across bothTestValidateGitRefandTestValidateGitPath. - Test names read as clear specifications (e.g. "leading double dash is rejected", "nested path traversal is rejected").
- A new case (
--output=/etc/passwd,--upload-pack=malicious) strengthens double-dash-prefix coverage beyond the original single-dash case.
Minor observation (non-blocking)
No test exercises a NUL byte combined with another violation (e.g. -evil\x00) to confirm check ordering, but this is low value given the checks are independent and already well covered individually.
Nice cleanup — consolidating duplicate test files into one table-driven suite with a shared assertion helper is exactly the kind of maintainability win /tdd calls for.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 25.9 AIC · ⌖ 13.8 AIC · ⊞ 10.4K
Comment /matt to run again
Test Quality Sentinel Report - PR #60043SummaryPR: Reject NUL bytes in Git paths and consolidate validation tests Key FindingsStrengths
Test Quality Score: 88/100 (Excellent)Verdict✅ APPROVE This PR demonstrates excellent test quality:
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Reviewed the NUL-byte validation fix in ValidateGitPath and the test consolidation in pkg/gitutil.
- The new NUL-byte check mirrors the existing
ValidateGitRefcheck and is placed consistently with the other validations. - Test consolidation into table-driven
TestValidateGitRef/TestValidateGitPathpreserves all prior coverage (dash-injection, traversal, absolute path, empty, NUL byte) and adds new cases (double-dash injection, nested traversal, NUL byte for paths) without duplication. - Verified locally:
go build ./pkg/gitutil/...andgo test ./pkg/gitutil/...both pass.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 32.4 AIC · ⌖ 12.5 AIC · ⊞ 8.4K
|
🎉 This pull request is included in a new release. Release: |
ValidateGitPathaccepted NUL bytes, and its coverage duplicated cases across two test files.