fix(node): validate ref-update SHAs are hex, not just length 40 - #433
fix(node): validate ref-update SHAs are hex, not just length 40#433beardthelion wants to merge 1 commit into
Conversation
parse_ref_updates accepted any 40-byte string as an object id, so a peer-supplied pkt-line with non-hex content was forwarded into RefUpdateEvents on gossip, branch_cid rows, and sync-notify JSON as a canonical-looking identifier. Require [0-9a-fA-F] on both fields. Closes #398
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughReceive-pack ref-update parsing now accepts only updates with two 40-character hexadecimal object IDs. Tests cover valid IDs, malformed lengths, non-hex characters, and mixed valid and invalid lines. ChangesReceive-pack SHA validation
Priority: ➖ Normal Estimated code review effort: 1 (Trivial) | ~5 minutes Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change meets the stated validation objective without introducing an unresolved merge-blocking risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR hardens receive-pack ref-update parsing so both object-ID fields must contain exactly 40 ASCII hexadecimal characters, preventing malformed peer input from reaching downstream events and persistence.
Confidence Score: 5/5The PR appears safe to merge; it narrows malformed input acceptance while preserving valid SHA-1 ref updates. The validation accepts hexadecimal 40-character object IDs, including uppercase and all-zero sentinel values, and the tests exercise both fields and per-line malformed-input handling with valid pkt-line framing.
|
| Filename | Overview |
|---|---|
| crates/gitlawb-node/src/api/repos.rs | Tightens ref-update object-ID validation and adds correctly framed regression tests without changing valid SHA-1 behavior. |
Reviews (1): Last reviewed commit: "fix(node): validate ref-update SHAs are ..." | Re-trigger Greptile
beardthelion
added
crate:node
2 participants
Summary
parse_ref_updatesaccepted any 40-byte string as an object id; it now requires both SHA fields to be 40 ASCII hex chars.Motivation & context
Closes #398
A peer-supplied pkt-line whose old/new fields are non-hex was forwarded into
RefUpdateEvents on gossip,branch_cidrows, and sync-notify JSON as a canonical-looking identifier. The git layer still gates the actual write, so this is wire/persistence pollution rather than a write-surface compromise.Kind of change
What changed
gitlawb-node: addedis_hex_sha(length 40 + allis_ascii_hexdigit) and applied it to both fields inparse_ref_updates.parse_ref_updates_requires_hex_shascovering: valid pair accepted, non-hex old/new dropped, 39- and 41-byte fields dropped, and a bad line among good ones drops only the bad line.How a reviewer can verify
cargo test -p gitlawb-node parse_ref_updates_requires_hex_shasThe non-hex assertions fail on the pre-change parser (verified by reverting the condition).
Before you request review
cargo test --workspacepasses locallycargo fmt --allandcargo clippy --workspace --all-targets -- -D warningsare cleanfeat(...),fix(...),docs(...)).env.exampleupdated if behavior or config changed (or N/A)Protocol & signing impact
did:key, Ed25519 / RFC 9421 signatures, UCAN, ref certs, or P2P wire formatsNone: parser-side validation only; malformed lines were already droppable, so dropping a non-hex one changes no wire format.
Notes for reviewers
Dropping a malformed line (rather than rejecting the whole push) matches the parser's existing per-line behavior for unparseable lines.
Summary by CodeRabbit