git-sign-nostr: off-curve pubkeys accepted since nostr 0.44 bump — dead BIP-340 check, deterministic test failure on main, not run in CI
- Dominant language
- Rust
- Stars
- 32.7k
- Forks
- 4.3k
- Avg merge
- 1d 13h
- Merged PRs (30d)
- 253
Description
## Summary
`git-sign-nostr::tests::test_parse_envelope_rejects_invalid_oa_pubkey` fails deterministically on `main` and has done since the `nostr` 0.36 → 0.44 bump. Nothing in CI runs the test, so it only surfaces when someone runs `cargo test --workspace` by hand — where it ambushes unrelated verification runs (it did exactly that during review of #6024, which touches zero bytes in this crate).
The failing assertion is the messenger. The real defect is that `PublicKey::from_hex` is used as a *BIP-340 validity* check at four call sites and no longer performs one.
## Reproduce
```
git worktree add /tmp/wt-gsn # verified at a282e0643 and 7f61cf431
cd /tmp/wt-gsn && cargo test -p git-sign-nostr --lib
```
```
thread 'tests::test_parse_envelope_rejects_invalid_oa_pubkey' panicked at
crates/git-sign-nostr/src/lib.rs:2136:9:
assertion failed: result.is_err()
test result: FAILED. 55 passed; 1 failed; 0 ignored
```
Clean worktree, no local modifications. Reproduced independently at both merge-base `a282e0643` and current tip `7f61cf431af1d8f0480a0baf525881a12f2be7f2`.
## Root cause
`nostr` changed `PublicKey::from_hex` between 0.36 and 0.44 from a curve-point parse to a plain 32-byte hex decode. In 0.44.7 (`src/key/public_key.rs`):
```rust
pub fn from_hex(hex: &str) -> Result {
let mut bytes: [u8; Self::LEN] = [0u8; Self::LEN];
hex::decode_to_slice(hex, &mut bytes)?; // hex only — never touches the curve
Ok(Self::from_byte_array(bytes))
}
```
Curve validation moved to the separate `PublicKey::xonly()` accessor.
Measured with a standalone probe against both crate versions, same input:
| `nostr` | `PublicKey::from_hex("0".repeat(64)).is_ok()` | `.xonly().is_ok()` |
|---|---|---|
| 0.36.0 | `false` | — |
| 0.44.7 | **`true`** | `false` |
The 0.36.0 source confirms the probe (`nostr-0.36.0/src/key/public_key.rs`): `from_hex` was `XOnlyPublicKey::from_str(hex)`, i.e. a curve parse.
Controls in the same probe: a real key parses and converts (`true`/`true`); a non-hex string fails `from_hex` (`false`). So the change is specific to *off-curve but well-formed hex*, which is exactly the input the test uses.
The test was written in #528 (2026-05-11), when `Cargo.lock` pinned `nostr` 0.36.0. The bump to 0.44.3 landed in #798. The assertion has been wrong since that bump.
## Impact — a dead structural check, not a signature bypass
`PublicKey::from_hex(x).is_err()` is used as a validity gate at four sites in `crates/git-sign-nostr/src/lib.rs`, and at 0.44 none of them reject an off-curve key:
- `:1020` — sign path, `load_auth_tag()` owner key. Intended to **fail closed** before embedding the auth tag; now passes an off-curve owner through into a signed envelope.
- `:1246` — verify path, `oa[0]`. Intended to emit `ERRSIG` for a structurally invalid owner; now falls through to `verify_oa`, which returns `false` with a *warning* rather than a structural error. The NIP-GS spec calls this a structural error.
- `:1424` — `parse_envelope`. The site the test covers.
- `:2265` — test helper.
Scoped negative: signature verification itself still fails closed. Both `verify_sig` (`:1225`) and `verify_oa` (`:1533`) call `.xonly()` and bail when it errors, so an off-curve key cannot verify a signature. The regression is in *severity and timing* — a rejection became a warning, and a sign-time fail-closed became a pass-through — not in signature soundness.
## Suggested fix — two parts, both needed
### Part 1 — restore the check
One predicate, applied at all four sites:
```rust
PublicKey::from_hex(x).and_then(|k| k.xonly())
```
That restores 0.36 semantics under 0.44 and makes the existing test pass for the reason it was written.
### Part 2 — put the crate behind a gate that runs
Add `git-sign-nostr` to the `test-unit` enumeration in `Justfile` (the `Unit Tests` job), alongside the other explicitly-listed packages. **Part 1 without Part 2 just resets the three-month clock** — the predicate would be correct today and nothing would notice the next time it stopped being correct. The crate's tests are infra-free, so they belong in that job by the same reasoning the file already gives for `buzz-backend-kubernetes` ("enumerated explicitly because nothing in CI runs `cargo test --workspace` — workspace membership alone buys clippy/check, not a single executed test").
## Why CI never caught it
`just test-unit` (the `Unit Tests` job, `.github/workflows/ci.yml:119-139`) enumerates packages explicitly and `git-sign-nostr` is not among them. The only CI reference to the crate is `server-cross-compile` (`ci.yml:982`), which `check`s/`build`s it and runs no tests. Scoped to `.github/workflows/` and `Justfile`, there is no job that executes this crate's tests.
This is Part 2 of the fix above, not an optional follow-up.
## Not related to #6024
`git diff origin/main...9a8128ba4 -- crates/git-sign-nostr` is empty (likewise `crates/buzz-relay` and `crates/buzz-pair-relay`). Filed separately so an honest `rc=101` in a reviewer's workspace log is not read as that PR's failure.
Contributor guide
Assessment
This issue has not been assessed yet.