fix(git): widen is_safe_refname, is_emittable_ref, and RefPattern alphabets for +/@ (#4194) - #4348
Open
iroiro147 wants to merge 2 commits into
Open
fix(git): widen is_safe_refname, is_emittable_ref, and RefPattern alphabets for +/@ (#4194)#4348iroiro147 wants to merge 2 commits into
+/@ (#4194)#4348iroiro147 wants to merge 2 commits into
Conversation
…k#4194) `git check-ref-format` accepts `+` and `@` in ref components (no newline, no NUL, no control chars, no `.` at the start of a component, no trailing `/`), but `is_safe_refname` rejected anything outside `[a-zA-Z0-9_./-]`. Real-world upstream branches like `OriginTrail/dkg`'s `refs/heads/test/842+841-devnet` were un-mirrorable into Buzz git. Both characters have no meaning to the object-store key scheme or to path traversal (`.`-based traversal protection is unchanged, and refnames are still pinned to the `refs/` prefix so they cannot escape into other key spaces). The predicate is shared symmetrically by write-side `validate` and read-side hydration, so widening once fixes both sides of the seam. Keep the widening conservative — the other git-legal chars called out in the issue (`=`, `,`, `!`, `]`) are not observed failing at any upstream mirror and remain excluded to preserve a small attack surface. They can be added in a follow-up if a real failure surfaces. Regression coverage extends the existing `safe_refnames` tests in both `manifest.rs` and `hydrate.rs`: - positive: `refs/heads/test/842+841-devnet` and `refs/tags/release@v1` - negative: `=`, `,`, `!`, `]` remain rejected `cargo check -p buzz-relay` and `cargo test -p buzz-relay --lib safe_refnames` are green (2/2 tests pass; the 10 unrelated pre-existing failures in api::media + telemetry suites are untouched and out of scope). Refs block#4194 Signed-off-by: Sarthak Singh <sarthak.singh@juspay.in>
…_safe_refname (block#4194) Two sibling predicates re-spelled the pre-widening alphabet from is_safe_refname and silently dropped refs containing + or @: - is_emittable_ref in buzz-relay/src/api/git/manifest_event.rs — refs were continued out of the kind:30618 ref-state event, so refs/heads/test/842+841-devnet pushed clean, CASed, hydrated, cloned, and then never appeared in any branch lister (desktop projects/hooks.ts, web repos/use-repo-refs.ts). - RefPattern::parse in buzz-core/src/git_perms.rs:152 — literal protection rules could not name +/@ refs; those refs were pushable-but-unnameable from the enforcement side. Caught during review of block#4257 (thanks @alanshurafa). Widening is_safe_refname alone was insufficient: a ref that pushes, CASes, hydrates, and clones — but never appears in any lister — is strictly worse than today's 400-class rejection because nothing reports it. Two small items folded in: - Restore assert!(!is_safe_refname("")) in manifest.rs — load-bearing (ManifestError::EmptyHead doc relies on it). - Doc notation [a-zA-Z0-9_./+@-], not [a-zA-Z0-9_./+-@] — unambiguous (avoids char-class range interpretation +-@ which would include : ; < = > ?). Coverage: - manifest_event.rs::emits_refs_with_plus_and_at_in_component — event build includes refs with +/@ in its tag lists. - manifest_event.rs::is_emittable_ref_widened_alphabet — direct predicate test. - git_perms.rs::pattern_literal_accepts_plus_and_at — literal pattern can now name such refs; wildcard semantics unchanged. Test counts: 2/2 safe_refnames + 11/11 manifest_event + 35/35 git_perms, all green. Refs block#4194 Signed-off-by: Sarthak Singh <sarthak.singh@juspay.in>
Author
|
Standing-health check (re-verified against current
Ready for review; no action needed from contributors. |
This was referenced Aug 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem (#4194)
git check-ref-formataccepts+and@in ref components (no newline, no NUL, no control chars, no.at the start of a component, no trailing/), butis_safe_refnamerejected anything outside[a-zA-Z0-9_./-]. Real-world upstream branches likeOriginTrail/dkg'srefs/heads/test/842+841-devnetwere un-mirrorable into Buzz git.Both characters have no meaning to the object-store key scheme or to path traversal (
.-based traversal protection is unchanged, and refnames are still pinned to therefs/prefix).Fix
This PR supersedes #4257 with the review feedback from @alanshurafa folded in as ordinary commits.
Commit 1 (
90d7800f3) — widenis_safe_refnameinbuzz-relay/src/api/git/manifest.rs(and the hydrate-side test) to accept+and@. Conservative scope:=,,,!,]remain excluded.Commit 2 (
4a88e6017) — fold in review feedback:is_emittable_refinbuzz-relay/src/api/git/manifest_event.rs— re-spelled the old alphabet and silentlycontinued+/@refs out of the kind:30618 ref-state event. Without this, a widenedis_safe_refnamealone makes things worse: the ref pushes, CASes, hydrates, and clones — and then never appears in any branch lister (desktopprojects/hooks.ts, webrepos/use-repo-refs.ts— silent lister drop).RefPattern::parseinbuzz-core/src/git_perms.rs:152— literal protection rules could not name+/@refs; those refs became pushable-but-unnameable from the enforcement side.[a-zA-Z0-9_./+-@]→[a-zA-Z0-9_./+@-](+immediately before@) so it doesn't read as a char-class range+-@(which would include: ; < = > ?).assert!(!is_safe_refname(""))is load-bearing (ManifestError::EmptyHeaddoc relies on the read side rejecting"""); restored with a comment citing the consumer.Verification
Coverage added:
manifest.rs::safe_refnamesandhydrate.rs::safe_refnames— positive+/@cases + negative=,,,!,]cases.manifest_event.rs::emits_refs_with_plus_and_at_in_component— event builds include the new refs in tag lists, not silentlycontinuethem out.manifest_event.rs::is_emittable_ref_widened_alphabet— direct predicate test.git_perms.rs::pattern_literal_accepts_plus_and_at— literal protection rule can name+/@refs; wildcard semantics unchanged (refs/heads/*still matches exactly one segment;refs/heads/**crosses components).Test counts green:
cargo test -p buzz-relay --lib safe_refnames→ 2/2cargo test -p buzz-relay --lib manifest_event→ 11/11cargo test -p buzz-core --lib git_perms→ 35/35cargo fmt --check -p buzz-core -p buzz-relay→ cleanThe 10 unrelated pre-existing failures in
api::media+telemetrysuites are untouched and out of scope.Refs #4194
Supersedes #4257
Signed-off-by: Sarthak Singh sarthak.singh@juspay.in