[beardthelion]

Summary

parse_ref_updates accepted 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_cid rows, 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

• Bug fix
• Feature
• Security fix
• Docs
• Tests / CI
• Refactor (no behavior change)
• Breaking or protocol change (issue required first)

What changed

• gitlawb-node: added is_hex_sha (length 40 + all is_ascii_hexdigit) and applied it to both fields in parse_ref_updates.
• Added parse_ref_updates_requires_hex_shas covering: 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_shas

The non-hex assertions fail on the pre-change parser (verified by reverting the condition).

Before you request review

• Scope is one logical change; no unrelated churn
• cargo test --workspace passes locally
• New behavior is covered by tests (required for fixes)
• cargo fmt --all and cargo clippy --workspace --all-targets -- -D warnings are clean
• Commit titles use Conventional Commits (feat(...), fix(...), docs(...))
• Docs / .env.example updated if behavior or config changed (or N/A)
• Checked existing PRs so this isn't a duplicate

Protocol & signing impact

• Touches DID / did:key, Ed25519 / RFC 9421 signatures, UCAN, ref certs, or P2P wire formats
• Discussed in an issue before implementation
• Backward-compatible with existing nodes and previously signed history

None: 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

• Bug Fixes
  • Improved validation of repository reference updates by rejecting malformed object IDs, including values with invalid characters or incorrect lengths.
  • Added coverage for valid, invalid, and mixed reference-update inputs.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

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

Changes: Receive-pack SHA validation

Layer / File(s)	Summary
SHA validation and regression tests crates/gitlawb-node/src/api/repos.rs	parse_ref_updates now validates both object IDs with is_hex_sha. Tests cover valid hexadecimal IDs, invalid characters, invalid lengths, and mixed input lines.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Severity of issue fixed: Medium

Suggested reviewers: vasanthdev2004

Merge Risk: Minimal · previous review f9942

The change meets the stated validation objective without introducing an unresolved merge-blocking risk.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: validating ref-update SHAs as hexadecimal values instead of checking length only.
Description check	✅ Passed	The description is complete. It documents the bug, motivation, issue link, implementation, tests, verification command, checklist status, and protocol impact.
Linked Issues check	✅ Passed	The changes satisfy issue #398. Both SHA fields require exactly 40 ASCII hexadecimal characters, malformed updates are dropped individually, and tests cover valid, non-hex, and invalid-length values.
Out of Scope Changes check	✅ Passed	The changes are limited to parser validation and focused tests in gitlawb-node. They directly support issue #398 and contain no unrelated scope.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 1 files.

Finishing Touches

Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/issue-398-hex-sha-validation

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Essentials

Run ID: c24a1c72-e890-4db4-ad8f-8fd4b8995ac9

📥 Commits

Reviewing files that changed from the base of the PR and between bfc44f9 and f9942ee.

📒 Files selected for processing (1)

• crates/gitlawb-node/src/api/repos.rs

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.

[greptile-apps]

Greptile Summary

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

• Adds a focused is_hex_sha validation helper.
• Applies validation independently to old and new object IDs.
• Adds regression coverage for valid, non-hex, incorrectly sized, and mixed packet sequences.

Confidence Score: 5/5

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

Important Files Changed

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}

[jatmn]

lgtm

[euxarist]

Correct in scope: is_hex_sha on both pkt-line fields with a real deny test (junk, 39/41-byte, mixed batch). One caveat I want on the record: this covers only the HTTP receive-pack source. The gossip receiving path still deserializes RefUpdateEvent and inserts old_sha/new_sha with no hex or signature check at HEAD, and this PR's body reads broader than that. Source-side hygiene worth merging; the gossip half belongs to #325/#334 and neither has landed yet.