From 1e0ef58c0c6fc2e9de0db346f342aae0bc57b079 Mon Sep 17 00:00:00 2001 From: Trezy Date: Sun, 2 Aug 2026 23:12:51 -0500 Subject: [PATCH] fix: verify_signature now raises instead of returning false when it cant check a signature Signed-off-by: Trezy --- .../docs/api-reference/lua/atproto-api.md | 23 ++++++- .../docs/guides/attestation-signing.md | 8 ++- .../script-examples/signed-record-verify.md | 12 +++- src/lua/atproto_api.rs | 65 +++++++++++++++++-- web/src/lib/lua-completions.ts | 2 +- web/src/lib/lua-hover.ts | 2 +- 6 files changed, 99 insertions(+), 13 deletions(-) diff --git a/packages/docs/content/docs/api-reference/lua/atproto-api.md b/packages/docs/content/docs/api-reference/lua/atproto-api.md index 5308d2f..a3a0076 100644 --- a/packages/docs/content/docs/api-reference/lua/atproto-api.md +++ b/packages/docs/content/docs/api-reference/lua/atproto-api.md @@ -334,7 +334,17 @@ Verifies that an inline signature was produced by this HappyView instance. Only | `signature` | table | The signature object from `atproto.sign()` | | `repo_did` | string | The repo DID | -**Returns:** `true` if the signature is valid, `false` otherwise. Returns `false` on failure rather than raising an error. +**Returns:** `true` if the signature is valid, `false` if it is not. + +**Raises** when the signature cannot be checked at all — signature bytes that aren't valid base64, a missing `key` or `$bytes` field, a record that can't be encoded. These are not the same fact: + +| Outcome | Meaning | +| -------- | -------------------------------------------------------------------- | +| `true` | We checked. The signature is genuine. | +| `false` | We checked. The signature does not match this record — it is forged. | +| _raises_ | We could not check. This says nothing about the record. | + +The distinction matters because `verify_signature` is almost always used as a guard, and treating "could not check" as "forged" turns any fault in the verification path into an accusation against a user. If your guard rejects records on `false`, wrap the call in `pcall` and handle the raise as a distinct case — retry it, count it, or alert on it, but don't record it as forgery. ### Examples @@ -347,3 +357,14 @@ if not valid then return { error = "signature verification failed" } end ``` + +```lua +-- Guard that keeps "forged" and "unverifiable" apart +local ok, valid = pcall(atproto.verify_signature, record, sig, repo_did) +if not ok then + -- valid holds the error message; we learned nothing about the record + return { error = "could not verify signature: " .. tostring(valid) } +elseif not valid then + return { error = "record signature is not ours" } +end +``` diff --git a/packages/docs/content/docs/guides/attestation-signing.md b/packages/docs/content/docs/guides/attestation-signing.md index 8325ea6..8ddbda6 100644 --- a/packages/docs/content/docs/guides/attestation-signing.md +++ b/packages/docs/content/docs/guides/attestation-signing.md @@ -79,11 +79,17 @@ function handle() return { record = record, verified = false } end - local valid = atproto.verify_signature(record, sig, record.did) + local ok, valid = pcall(atproto.verify_signature, record, sig, record.did) + if not ok then + -- Couldn't check — that is not the same as "the signature is bad". + return { record = record, verified = nil, error = tostring(valid) } + end return { record = record, verified = valid } end ``` +`verify_signature` returns `false` only when it checked the signature and it didn't match. It **raises** when it couldn't check — signature bytes that aren't valid base64, a missing field, a record that won't encode. Keep those apart: a script that treats "couldn't check" as `false` will report a fault in its own verification path as a forged record, which is a serious thing to tell a user about their own data. + ### Checking availability Both functions are `nil` when no signer is configured: diff --git a/packages/docs/content/docs/reference/script-examples/signed-record-verify.md b/packages/docs/content/docs/reference/script-examples/signed-record-verify.md index 8b3989c..2b1ec41 100644 --- a/packages/docs/content/docs/reference/script-examples/signed-record-verify.md +++ b/packages/docs/content/docs/reference/script-examples/signed-record-verify.md @@ -15,11 +15,17 @@ function handle() local verified = false if atproto.verify_signature and record.signature then - verified = atproto.verify_signature( + local ok, result = pcall( + atproto.verify_signature, { text = record.text, createdAt = record.createdAt }, record.signature, params.did ) + if not ok then + -- We could not check. Say so — don't report it as a bad signature. + return { record = record, verified = nil, error = tostring(result) } + end + verified = result end return { record = record, verified = verified } @@ -30,7 +36,9 @@ end 1. Fetch the record by AT URI. 2. If a signature is present, rebuild the same field table that was signed and verify it with [`atproto.verify_signature()`](../../api-reference/lua/atproto-api.md#atprotoverify_signature). -3. Return `verified = true` if the signature is valid, `false` if it's missing, invalid, or the signer isn't configured. +3. Return `verified = true` if the signature is valid, `false` if it's missing, doesn't match, or the signer isn't configured. + +The `pcall` matters. `verify_signature` returns `false` only when it checked and the signature didn't match; it *raises* when it couldn't check at all — unparseable signature bytes, a missing field. Assigning that to `verified` would report a decode bug as a forged record. ## Usage diff --git a/src/lua/atproto_api.rs b/src/lua/atproto_api.rs index 6bdc58f..5bd300a 100644 --- a/src/lua/atproto_api.rs +++ b/src/lua/atproto_api.rs @@ -324,13 +324,25 @@ pub fn register_atproto_api( .from_value(sig) .map_err(|e| mlua::Error::runtime(format!("atproto.verify_signature: {e}")))?; - match signer.verify_record_signature(&record_json, &sig_json, &repo_did) { - Ok(valid) => Ok(valid), - Err(e) => { - tracing::debug!(error = %e, "atproto.verify_signature failed"); - Ok(false) - } - } + // `false` and an error are different facts, and only `false` + // is a statement about the record: it means we checked and the + // signature does not match. An error means we could not check + // — malformed signature bytes, a missing field, a record that + // will not encode. Collapsing the second into the first would + // let any fault in this path present to a script as "this user + // forged their records", with nothing in the logs to say + // otherwise. Callers that want the old behaviour can `pcall`. + signer + .verify_record_signature(&record_json, &sig_json, &repo_did) + .map_err(|e| { + tracing::warn!( + repository = %repo_did, + error = %e, + "atproto.verify_signature could not check the signature — \ + this is not a statement that the record is forged" + ); + mlua::Error::runtime(format!("atproto.verify_signature: {e}")) + }) }, )?; atproto_table.set("verify_signature", verify_fn)?; @@ -949,6 +961,45 @@ mod tests { assert!(!result); } + /// "We checked and it does not match" and "we could not check" are + /// different facts, and only the first is a statement about the record. + /// A script that cannot tell them apart will accuse a user of forgery + /// because of a decode bug. + #[tokio::test] + async fn verify_signature_distinguishes_unverifiable_from_invalid() { + let state = test_state_with_signer(""); + let lua = mlua::Lua::new(); + register_atproto_api(&lua, Arc::new(state), Some("did:plc:caller")).unwrap(); + + let chunk = r#" + local record = { contributionType = "correction", changes = { name = "Original" } } + local sig = atproto.sign(record) + + -- A well-formed signature over a different payload: genuinely invalid. + local tampered = { contributionType = "correction", changes = { name = "Tampered" } } + local mismatch_ok, mismatch = pcall(atproto.verify_signature, tampered, sig, "did:plc:caller") + + -- Signature bytes that are not base64 at all: unverifiable. + sig.signature["$bytes"] = "not!valid!base64" + local undecodable_ok, undecodable = pcall(atproto.verify_signature, record, sig, "did:plc:caller") + + return mismatch_ok, mismatch, undecodable_ok, tostring(undecodable) + "#; + let (mismatch_ok, mismatch, undecodable_ok, undecodable): (bool, bool, bool, String) = + lua.load(chunk).eval_async().await.unwrap(); + + assert!(mismatch_ok, "a mismatch is an answer, not a failure"); + assert!(!mismatch, "a signature over a different payload is invalid"); + assert!( + !undecodable_ok, + "an unverifiable signature must not be reported as invalid" + ); + assert!( + undecodable.contains("invalid base64"), + "the error must name the cause, got: {undecodable}" + ); + } + #[tokio::test] async fn spaces_api_is_registered() { let state = test_state_with_plc(""); diff --git a/web/src/lib/lua-completions.ts b/web/src/lib/lua-completions.ts index 8d02644..3744b76 100644 --- a/web/src/lib/lua-completions.ts +++ b/web/src/lib/lua-completions.ts @@ -288,7 +288,7 @@ const STATIC_COMPLETIONS: LuaCompletions = { { label: "get_labels", detail: "function", description: "Get labels for a URI — atproto.get_labels(uri) → label[]", insertText: "get_labels(${1:uri})" }, { label: "get_labels_batch", detail: "function", description: "Get labels for multiple URIs — atproto.get_labels_batch({uri1, uri2}) → {[uri]: label[]}", insertText: "get_labels_batch(${1:uris})" }, { label: "sign", detail: "function", description: "Sign a record (requires attestation signer) — atproto.sign(record) → signature", insertText: "sign(${1:record})" }, - { label: "verify_signature", detail: "function", description: "Verify a record signature — atproto.verify_signature(record, sig, repo_did) → boolean", insertText: "verify_signature(${1:record}, ${2:sig}, ${3:repo_did})" }, + { label: "verify_signature", detail: "function", description: "Verify a record signature — atproto.verify_signature(record, sig, repo_did) → boolean; raises when the signature can't be checked (pcall to distinguish from a mismatch)", insertText: "verify_signature(${1:record}, ${2:sig}, ${3:repo_did})" }, { label: "spaces", detail: "table", description: "Permissioned Spaces sub-table — atproto.spaces.*" }, ], // HappyView Spaces API (atproto.spaces.*) diff --git a/web/src/lib/lua-hover.ts b/web/src/lib/lua-hover.ts index c0dd45f..600f80d 100644 --- a/web/src/lib/lua-hover.ts +++ b/web/src/lib/lua-hover.ts @@ -177,7 +177,7 @@ export const HOVER_DOCS = new Map([ ["atproto.get_labels", { signature: "atproto.get_labels(uri)", description: "Get labels for a URI — returns array of {src, uri, val, cts}", module: "atproto" }], ["atproto.get_labels_batch", { signature: "atproto.get_labels_batch({uri1, uri2, ...})", description: "Get labels for multiple URIs — returns table keyed by URI", module: "atproto" }], ["atproto.sign", { signature: "atproto.sign(record)", description: "Sign a record (requires attestation signer, procedure scripts only) — returns signature object", module: "atproto" }], - ["atproto.verify_signature", { signature: "atproto.verify_signature(record, sig, repo_did)", description: "Verify a record signature — returns boolean", module: "atproto" }], + ["atproto.verify_signature", { signature: "atproto.verify_signature(record, sig, repo_did)", description: "Verify a record signature — returns boolean; raises if the signature can't be checked at all (wrap in pcall to tell 'forged' from 'unverifiable')", module: "atproto" }], // ── HappyView Spaces API ──────────────────────────────────────────── // Note: hover lookup keys on the immediate `.` pair, not the -- 2.51.2