fix(cli): normalize hex ids used in generic tag filters - #5966
fix(cli): normalize hex ids used in generic tag filters#5966Chessing234 wants to merge 4 commits into
Conversation
`validate_hex64` is documented as validating a "64-character lowercase hex string" but accepts either case — `is_ascii_hexdigit` does — and every caller passes the string through unchanged. That is harmless where the value reaches a typed field: `ids` and `authors` are parsed into `EventId`/`PublicKey` on the relay, and `buzz-sdk`'s builders already lowercase before they write a tag. It is not harmless in a NIP-01 generic tag filter, which is compared as a string. Add `parse_hex64`, which validates and returns the value lowercased, and correct the older function's doc to say what it does and when each is right. No call sites yet. Signed-off-by: Taksh <takshkothari09@gmail.com>
`buzz messages thread --event <ID>` validated the hex and used it verbatim in two filters. `ids` is parsed into a typed `EventId` on the relay, so case there is harmless — but `#e` is a NIP-01 generic tag filter, matched by string comparison against a tag every event writes lowercase. Paste an id in uppercase and the command returns the root event and none of its replies: a partial thread, printed as if it were the whole thing. Lowercase the id once, at the boundary, and lift the two filters into `thread_filters` so the shape is pinned by a test instead of only by the network. Signed-off-by: Taksh <takshkothari09@gmail.com>
`buzz issues assign` looks up the issue's prior assignment events with an `#e` filter before deciding what to publish. Same string-compared tag filter as the thread query: an uppercase issue id finds no prior assignments, so the command builds its note from an empty history — reporting an assignment against a state it never actually read. The `ids` filter beside it is typed and would still resolve the issue itself, which is what makes the failure quiet. Signed-off-by: Taksh <takshkothari09@gmail.com>
themiguelamador
left a comment
themiguelamador
left a comment
There was a problem hiding this comment.
The new normalization boundary is correct, but the PR leaves five user-facing raw tag filters with the same silent uppercase failure:
reactions getandreactions removestill place the validated-but-unnormalized event ID into#e.patches list,pr list, andissues liststill interpolate the validated-but-unnormalized repository owner into the string-compared30617:<owner>:<repo>value used by#a.
An uppercase ID therefore still yields empty reaction results/removal failures, and an uppercase repository owner yields empty NIP-34 lists. Since this PR introduces the normalization helper specifically for generic filters, those remaining call sites should be covered in the same boundary fix.
I prepared a signed fix that normalizes every remaining user-supplied hex component in CLI #e/#a filters and adds shared regression coverage: https://github.com/Complear/buzz/commit/4fcc6659d
Verification on the fix:
cargo test -p buzz-cli: 357 passedcargo clippy -p buzz-cli --all-targets -- -D warnings: cleancargo fmt --all -- --check: clean- exhaustive CLI
#e/#p/#afilter scan completed - git diff whitespace check passes
Review feedback: `reactions get` and `reactions remove` build the same kind:7 `#e` filter from the caller's id without normalizing it. `#e` is a generic tag filter compared as a raw string, so an uppercase id silently returns nothing — `get` prints an empty reaction list and `remove` reports the reaction as missing, for an id every other command accepts. Both query shapes now go through one `reactions_filter` builder fed by `parse_hex64`, with tests pinning the normalized `#e` value and the optional author narrowing. Signed-off-by: Taksh <takshkothari09@gmail.com>
|
pushed 754842d — on the three verified: |
2 participants
Found by reading
validate_hex64against its own doc comment; no issue filed.validate_hex64is documented as validating a "64-character lowercase hex string", butis_ascii_hexdigitaccepts either case and no caller normalizes. Paste an id in uppercase — from a block explorer, a log line, a colleague's message — and it flows through unchanged.That is harmless almost everywhere, which is what makes the exceptions quiet:
idsandauthorsare parsed into typedEventId/PublicKeyon the relay, so case there doesn't matter.buzz-sdk's builders already lowercase (check_pubkey_hex,check_hex_exact) before writing a tag, so anything published through them is fine.filter_match_onecompares#e/#pvalues with==against the raw tag string, and every event in the tree writes lowercase hex. An uppercase id there matches nothing.Two commands build those filters as raw JSON:
buzz messages thread --event <ID>— the#efilter finds no replies while theidsfilter beside it still resolves the root. You get the root event and none of its thread, printed as if it were the whole thing. (Same shape as #5800, different cause.) The two filters are lifted intothread_filtersso their shape is pinned by a test rather than only by the network.buzz issues assign— reads the issue's prior assignment events over#ebefore deciding what to publish. With an uppercase id it reads an empty history and builds its note from that, reporting an assignment against a state it never saw. Theidsfilter beside it still resolves the issue, which is again what keeps it quiet.Related: I opened #5963 earlier aiming this at
moderation.rsand closed it myself — the SDK already normalizes there, so it changed nothing. This is the version that bites.Scope. Other
validate_hex64call sites still pass unnormalized values, but most reach typed fields or SDK builders. The remaining raw-JSON tag filters (social.rs,patches.rs,pr.rs) each need their own look at whether the value lands in#e/#aor only inauthors; I didn't want to sweep 14 files blind.Verified locally on the pinned 1.95.0 toolchain:
cargo test -p buzz-cli— 355 passed, 0 failed (349 before, plus the 6 new)cargo clippy --workspace --all-targets -- -D warnings— cleancargo fmt --all -- --check— cleanNote: I'm an outside contributor, so the workflow runs here sit at
action_requireduntil a maintainer approves them; only the DCO check reports on its own.