block / block/buzz

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

Open
#6,175 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.