fix(nip-oa): accept raw Nostr tag form in parse_json_array - #4203
Conversation
parse_json_array only accepted well-formed JSON arrays, so parse_auth_tag / verify_auth_tag rejected BUZZ_AUTH_TAG values stored in the raw Nostr tag form ([auth,hex,,hex]) — the unquoted, comma-delimited serialization used inside Nostr events and commonly written to .env files and shell variables. This forced every consumer (buzz-acp harness, agent shells sourcing BUZZ_AUTH_TAG from .env) to re-quote the value into JSON before the CLI would accept it, or the CLI failed with 'BUZZ_AUTH_TAG is malformed: invalid JSON'. Add a fallback in parse_json_array: when strict JSON parsing fails and the trimmed input is bracket-delimited, split on commas and treat each field as a string (empty field -> empty string, matching the JSON form ["auth","hex","","hex"]). Well-formed JSON still takes the fast path; only non-JSON bracketed input triggers the fallback. This is the lowest layer, so all consumers (parse_auth_tag, verify_auth_tag, the CLI, buzz-acp) benefit from one change. Tests: 3 new (raw form with conditions, raw form with empty conditions, raw form with whitespace) + all 21 existing nip_oa tests pass. Signed-off-by: amanning3390 <adam.manning@pro-serveinc.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53de09881f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _ => Err(SdkError::InvalidInput( | ||
| "auth tag must be a JSON array".into(), | ||
| )), | ||
| let trimmed = s.trim(); |
There was a problem hiding this comment.
The reviewed commit object has no Signed-off-by trailer, so the repository's required DCO check will fail and prevent this change from merging; recreate or rebase the commit with --signoff.
AGENTS.md reference: AGENTS.md:L111-L111
Useful? React with 👍 / 👎.
| if !arr.is_empty() { | ||
| return Ok(arr); |
There was a problem hiding this comment.
When the newly supported raw form is supplied to buzz, parsing succeeds here, but run() still stores the original string in BuzzClient::auth_tag_json, which with_auth_tag() sends verbatim as x-auth-tag. During a client-first or rolling upgrade, relays predating this parser change still require JSON and reject every membership-scoped HTTP operation even though the CLI accepted and verified the tag locally; carry a canonical JSON serialization of arr for transport instead of the raw input.
Useful? React with 👍 / 👎.
…g header The CLI stored the raw BUZZ_AUTH_TAG input string and sent it verbatim as the x-auth-tag header value (client.rs:618). When the tag was in the raw Nostr form ([auth,hex,,hex]), the relay's verify_auth_tag — which expects JSON — rejected it with 403 relay_membership_required, even though the CLI had parsed it locally. Add canonicalize_auth_tag in buzz-sdk: parse either form (JSON or raw), re-serialize to canonical JSON. The CLI now canonicalizes the tag before storing it as auth_tag_json, so the header is always valid JSON regardless of input form. This closes the loop with the parse_json_array fallback: local parse + wire canonicalization means the raw form works end-to-end. Signed-off-by: amanning3390 <adam.manning@pro-serveinc.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d574fe4a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if trimmed.starts_with('[') && trimmed.ends_with(']') { | ||
| let inner = &trimmed[1..trimmed.len() - 1]; | ||
| let arr: Vec<Value> = inner | ||
| .split(',') | ||
| .map(|part| Value::String(part.trim().to_owned())) |
There was a problem hiding this comment.
When BUZZ_AUTH_TAG uses the newly accepted raw form, agent Git operations still reject it locally: git-credential-nostr::load_auth_tag calls serde_json::from_str at crates/git-credential-nostr/src/lib.rs:84-86, and git-sign-nostr::load_auth_tag does the same at crates/git-sign-nostr/src/lib.rs:490-492. These tools receive the same harness-injected environment variable, so fetch/push and signing fail before reaching the relay even though the CLI now accepts the credential. Fresh evidence in this target is that only the CLI transport was canonicalized; either canonicalize the injected value centrally or teach both Git parsers the same raw syntax.
Useful? React with 👍 / 👎.
…ign-nostr Address Codex review P2: git-credential-nostr::load_auth_tag and git-sign-nostr::load_auth_tag parsed BUZZ_AUTH_TAG with serde_json::from_str directly, so the raw Nostr tag form ([auth,hex,,hex]) newly accepted by the SDK and CLI still failed for Git fetch/push/sign operations. Add a parse helper in each crate that mirrors the SDK's parse_json_array behavior: JSON fast path, then a raw-form fallback that splits on commas. No new dependencies — each git crate keeps its existing dep set. Also fix a clippy needless_question_mark warning in canonicalize_auth_tag. Test: new test_load_auth_tag_accepts_raw_nostr_form covers raw form with empty conditions, conditions, and whitespace in git-sign-nostr. Note: test_parse_envelope_rejects_invalid_oa_pubkey is a pre-existing failure on upstream/main (unrelated to this change) — confirmed by running it with these commits stashed. Signed-off-by: amanning3390 <adam.manning@pro-serveinc.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88471dd668
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let sig = "b".repeat(128); | ||
|
|
||
| // Raw form with empty conditions (`,,`). | ||
| std::env::set_var("BUZZ_AUTH_TAG", format!("[auth,{owner},,{sig}]")); |
There was a problem hiding this comment.
Under Cargo's default parallel test execution, this new test can run alongside test_load_auth_tag_rejects_bad_conditions, and both mutate the same process-wide BUZZ_AUTH_TAG. Either test can therefore read or remove the other's value, causing assertions to fail nondeterministically; protect both tests with a shared mutex or test the parser directly without modifying the environment.
Useful? React with 👍 / 👎.
tlongwell-block
left a comment
tlongwell-block
left a comment
There was a problem hiding this comment.
Requesting changes for one concrete regression and one API-contract issue at 88471dd668c633deae2ef0ae5e2658e1dd70b9d7.
-
The new
test_load_auth_tag_accepts_raw_nostr_formmutates process-globalBUZZ_AUTH_TAGconcurrently with the existingtest_load_auth_tag_rejects_bad_conditions. This makes the package suite flaky. Runningcargo test -q -p git-sign-nostr --lib test_load_auth_tag -- --test-threads=2repeatedly failed on iteration 2: the existing test expectedkind = 9to be rejected but read the new test's valid value instead. Please test a pure parsing helper, or serialize all tests that mutate this env var behind the same lock. -
canonicalize_auth_tagis public and documents that it errors unless its input is a valid 4-elementauthtag, but the implementation only checks array length. For example,["not-auth","x","y","z"]canonicalizes successfully. The CLI happens to callparse_auth_tagandverify_auth_tagfirst, so the current CLI path remains fail-closed, but the public helper does not uphold its stated contract and invites unsafe standalone use. Either make this a private serialization helper or have it validate via the same typed/validated path before returning.
The production direction is otherwise sound for the exact accepted grammar: strict JSON remains the fast path, raw fallback fields are still checked for label/count/lowercase hex/conditions/signature, CLI verification remains before header construction, and canonical JSON fixes the wire header. Splitting on commas is safe only because the current conditions grammar forbids commas; that dependency should be explicit.
Also, the repeated parser implementations in SDK, credential helper, and signer are drift-prone, and the claim that [auth,...] is how a tag serializes "inside a Nostr event" is inaccurate: an event serializes tags as JSON arrays. This may be an accepted local/env shorthand, but it is not Nostr event JSON.
…SDK and wire grammar strict Review follow-up to the previous three commits, keeping their intent (accept the hand-authored unquoted BUZZ_AUTH_TAG shorthand) while narrowing where the leniency lives: - Revert buzz-sdk parse_json_array to strict JSON. verify_auth_tag is the relay's x-auth-tag entry point (extract_nip_oa_owner and the bridge/api handlers), so the lenient grammar had widened the public wire format; probing that entry point base-vs-head showed raw form going rejected->accepted. Wire grammar is JSON per NIP-GS; it stays strict. - Remove canonicalize_auth_tag. The CLI now derives the wire string from the already parsed-and-verified Tag via serde_json::to_string(tag.as_slice()) — the same shape buzz-acp's RestClient has always used — instead of re-parsing unverified input. This also removes the helper whose docs promised full validation but whose body only checked element count. - Revert the hand-copied fallback parsers in git-credential-nostr and git-sign-nostr. The copies already diverged from the SDK (Vec<String> vs Value fast paths) and NIP-GS specifies the JSON form for these consumers. Reverting git-sign-nostr also removes the second BUZZ_AUTH_TAG-mutating test that raced the existing one under the default parallel test runner. - Add normalize_auth_tag_input in buzz-cli: a small, explicit raw->JSON rewrite applied only to hand-authored configuration input, before the unchanged strict parse/verify path. Valid JSON passes through untouched; unrecognizable input is left for the strict parser to reject with an error about the original bytes. Net: BUZZ_AUTH_TAG in raw form still works end-to-end via the CLI (verified live against a relay with a real delegated credential, plus JSON control and fail-closed garbage control), and the SDK, relay, and git binaries keep the strict grammar they had before this PR. Tests: buzz-cli 274, buzz-sdk 241, git-credential-nostr 8, buzz-relay 835 lib tests pass; git-sign-nostr suite no longer flakes under the default runner (8/8 repeat runs; the one remaining failure is test_parse_envelope_rejects_invalid_oa_pubkey, which fails identically on base ac4fa13 on macOS and is unrelated). fmt and clippy clean. Co-authored-by: Tyler <109685178+tlongwell-block@users.noreply.github.com> Co-authored-by: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> Signed-off-by: Tyler <109685178+tlongwell-block@users.noreply.github.com> Signed-off-by: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz>
|
Pushed a maintainer follow-up commit (0b74eee) that keeps this PR's intent — the unquoted
Verified locally at 0b74eee: buzz-cli 274, buzz-sdk 241, git-credential-nostr 8, buzz-relay lib 835 pass; git-sign-nostr no longer flakes across 8 consecutive default-settings runs; fmt/clippy clean; live CLI run against a relay with a real delegated credential in both JSON and raw forms, plus a fail-closed garbage control. Thanks for the report and the repro — the shorthand support was worth having; it just needed to live at the config edge rather than in the shared grammar. |
- Incorporate upstream desktop changes required by the pre-push overlap guard - Preserve the link preview thumbnail stabilization work on the updated base Co-authored-by: Taylor Ho <taylorkmho@gmail.com> * origin/main: (26 commits) docs: formal spec for remote agents and their management (#3748) fix(nip-oa): accept raw Nostr tag form in parse_json_array (#4203) perf(relay): serve relay-membership checks from the read replica (#4124) chore(deps): bump nostr-relay-pool for RUSTSEC-2026-0224 (#4139) docs(nostr): document #h requirement for live reaction subscriptions (#3487) docs(chart): fix ArgoCD example for native OCI sources (full artifact repoURL + path) (#3426) docs(readme): clarify which release asset to download per platform (#3481) fix(relay): allow open relays to set their NIP-11 workspace icon (kind:9033) (#3998) docs: note that addressable channel events scope by d, not h (#4103) docs: fix stale kind count, quick-start numbering, and empty Further Reading (#2613) fix(desktop): keep thread-open affordance in archived channels (#4012) docs: add one-click Railway deploy for a hosted relay (#2733) fix(desktop): point Oh My Pi preset at omp.sh (#3516) fix(mesh): stop restarting a busy or loading shared-compute node (#3909) fix(desktop): preserve first huddle speech (#3962) feat(desktop): Agent Trading Cards — mintable agent-snapshot card PNGs with optional NIP-44 lock (#3278) fix(buzz-acp): thread cache-read tokens into NIP-AM kind:44200 events (#3999) feat(relay): accept kind:30621 multi-repo projects at ingest (#3171) fix(release): preserve main in desktop PR body (#3979) chore(release): release Buzz Desktop version 0.5.3 (#3972) ... Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
2 participants
What
BUZZ_AUTH_TAGstored in the raw Nostr tag form[auth,hex,,hex](unquoted, comma-delimited — how anauthtag serializes inside a Nostr event and how.envfiles commonly store it) was rejected by the CLI:…and even when the CLI could parse it, it forwarded the raw string as the
x-auth-tagheader, so the relay'sverify_auth_tag(which expects JSON) rejected it with403 relay_membership_required.Two commits close both gaps.
Commits
1.
fix(nip-oa): accept raw Nostr tag form in parse_json_arrayparse_json_array(crates/buzz-sdk/src/nip_oa.rs) only accepted well-formed JSON arrays. Added a fallback: when strict JSON parsing fails and the trimmed input is bracket-delimited, split on,and treat each field as a string (empty field,,→ empty string, matching["auth","hex","","hex"]). All consumers (parse_auth_tag,verify_auth_tag, the CLI,buzz-acp) benefit from one change at the lowest layer.2.
fix(cli): canonicalize BUZZ_AUTH_TAG to JSON before sending x-auth-tag headerThe CLI stored the raw input string and sent it verbatim as the
x-auth-tagheader (client.rs:618). Addedcanonicalize_auth_taginbuzz-sdk: parse either form, re-serialize to canonical JSON. The CLI now canonicalizes before storing asauth_tag_json, so the header is always valid JSON regardless of input form.Together: local parse + wire canonicalization means the raw form works end-to-end.
Why
The raw form
[auth,hex,,hex]is exactly how anauthtag serializes inside a Nostr event. That shape leaks into.envfiles and shell variables because there's no canonical "stored form" outside an event. The SDK + CLI should accept it rather than push quoting/conversion logic onto every consumer (harnesses, agent shells, external tools).Security
Both changes are purely syntactic — they only change how a 4-element string array is extracted and containerized. All downstream validation is unchanged:
parse_auth_tag: still checks exactly 4 elements,"auth"label, 64-char lowercase-hex pubkey, 128-char signature.verify_auth_tag: still reconstructs the preimage and verifies the BIP-340 Schnorr signature against the owner pubkey.No new attack surface — a malformed or forged tag is still rejected at the same validation points.
Tests
4 new tests in
nip_oa::tests:test_parse_auth_tag_raw_nostr_form— raw form with conditions + empty conditionstest_parse_auth_tag_raw_form_with_whitespace— raw form with surrounding whitespacetest_canonicalize_auth_tag_raw_to_json— raw→JSON and JSON→JSON normalizationAll 25
nip_oatests pass (21 existing + 4 new).cargo fmt --checkandcargo clippy -p buzz-sdk -p buzz-cliclean.Verification
Confirmed end-to-end against a live community relay (
wss://hermesagent.communities.buzz.xyz):BUZZ_AUTH_TAG→ CLI parse error, or403 relay_membership_requiredif somehow parsed.BUZZ_AUTH_TAG→ CLI parses it, canonicalizes to JSON for the header, relay accepts via NIP-OA owner delegation,buzz channels membersreturns the full roster.Context
Originated from a community investigation where agent-side relay access was failing because the harness-exported
BUZZ_AUTH_TAG(raw Nostr form) was rejected by the CLI (expecting JSON). This removes the impedance mismatch at the source.