From 114ea1f02e00f1ef796320e60067599824b50ce2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Ros=C5=82aniec?= Date: Mon, 20 Jul 2026 09:34:44 +0000 Subject: [PATCH 1/3] test(tbtc): cover signer approval version/algorithm rejection and finality provider error --- pkg/tbtc/covenant_signer_test.go | 33 +++++++ pkg/tbtc/signer_approval_certificate_test.go | 96 ++++++++++++++++++++ 2 files changed, 129 insertions(+) diff --git a/pkg/tbtc/covenant_signer_test.go b/pkg/tbtc/covenant_signer_test.go index 67ef7a6846..549956decb 100644 --- a/pkg/tbtc/covenant_signer_test.go +++ b/pkg/tbtc/covenant_signer_test.go @@ -1889,3 +1889,36 @@ func TestCovenantSignerEngine_VerifySignerApprovalRejectsNonLiveWallet(t *testin t.Fatal("expected VerifySignerApproval to reject a closed wallet") } } + +// TestEnsureActiveOutpointFinalityPropagatesProviderError asserts the finality +// guard fails closed when the confirmation provider cannot answer. The existing +// finality tests only exercise known confirmation counts above and below the +// threshold; this covers the error path, where refusing to sign (rather than +// treating an unknown count as sufficient) is the reorg-safety behavior we rely +// on. +func TestEnsureActiveOutpointFinalityPropagatesProviderError(t *testing.T) { + node, bitcoinChain, _ := setupCovenantSignerTestNode(t) + + // A transaction the fake chain has never seen: it is neither in the + // confirmations map nor among the known transactions, so + // GetTransactionConfirmations returns "transaction not found". Version 99 + // makes an accidental hash collision with any seeded transaction + // vanishingly unlikely. + unknownTransactionHash := (&bitcoin.Transaction{Version: 99}).Hash() + if _, err := bitcoinChain.GetTransactionConfirmations(unknownTransactionHash); err == nil { + t.Fatal("test setup: expected the fabricated transaction hash to be unknown to the chain") + } + + cse := &covenantSignerEngine{ + node: node, + minimumActiveOutpointConfirmations: 6, + } + + err := cse.ensureActiveOutpointFinality(unknownTransactionHash) + if err == nil { + t.Fatal("expected finality check to fail when confirmations cannot be determined") + } + if !strings.Contains(err.Error(), "cannot determine active outpoint transaction confirmations") { + t.Fatalf("unexpected error message: %v", err) + } +} diff --git a/pkg/tbtc/signer_approval_certificate_test.go b/pkg/tbtc/signer_approval_certificate_test.go index d75f2097b9..dd0d2bdb24 100644 --- a/pkg/tbtc/signer_approval_certificate_test.go +++ b/pkg/tbtc/signer_approval_certificate_test.go @@ -1031,3 +1031,99 @@ func TestSignerApprovalCertificateSigningDigestMatchesCrossLanguageVectorAboveUi ) } } + +// TestVerifySignerApprovalCertificateRejectsUnsupportedVersion asserts the +// certificate version is a hard gate: only version 2 is accepted, and any other +// value (an unset zero, the superseded version 1, or a future version 3) is +// rejected before the signature is checked. This pins the version-negotiation +// contract so a future format bump cannot be silently accepted by an unupgraded +// verifier. +func TestVerifySignerApprovalCertificateRejectsUnsupportedVersion(t *testing.T) { + node, _, walletPublicKey := setupCovenantSignerTestNode(t) + request := validStructuredSignerApprovalVerificationRequest( + t, + node, + walletPublicKey, + covenantsigner.TemplateSelfV1, + ) + + expectedSignerSetHash := expectedSignerSetHashForWallet(t, node, walletPublicKey) + + // An unset (0), the superseded (1), and a future (3) version must all be + // rejected; only 2 is supported. + for _, unsupportedVersion := range []uint32{0, 1, 3} { + certificate := *request.SignerApproval + certificate.CertificateVersion = unsupportedVersion + + err := verifySignerApprovalCertificate(&certificate, expectedSignerSetHash) + if err == nil || !strings.Contains(err.Error(), "unsupported certificate version") { + t.Fatalf( + "expected unsupported certificate version error for version %d, got %v", + unsupportedVersion, + err, + ) + } + } +} + +// TestVerifySignerApprovalCertificateRejectsUnsupportedSignatureAlgorithm +// asserts the signature-algorithm field is a hard gate: only the tECDSA +// secp256k1 algorithm is accepted. This prevents a certificate from claiming a +// different (potentially weaker or unverifiable) algorithm than the one the +// verifier actually checks. +func TestVerifySignerApprovalCertificateRejectsUnsupportedSignatureAlgorithm(t *testing.T) { + node, _, walletPublicKey := setupCovenantSignerTestNode(t) + request := validStructuredSignerApprovalVerificationRequest( + t, + node, + walletPublicKey, + covenantsigner.TemplateSelfV1, + ) + + expectedSignerSetHash := expectedSignerSetHashForWallet(t, node, walletPublicKey) + + certificate := *request.SignerApproval + certificate.SignatureAlgorithm = "ed25519" + + err := verifySignerApprovalCertificate(&certificate, expectedSignerSetHash) + if err == nil || !strings.Contains(err.Error(), "unsupported signature algorithm") { + t.Fatalf("expected unsupported signature algorithm error, got %v", err) + } +} + +// expectedSignerSetHashForWallet recomputes the signer-set hash the verifier +// expects for the wallet the test node controls, mirroring the inline block used +// by the other verifySignerApprovalCertificate tests. +func expectedSignerSetHashForWallet( + t *testing.T, + node *node, + walletPublicKey *ecdsa.PublicKey, +) string { + t.Helper() + + walletExecutor, ok, err := node.getSigningExecutor(walletPublicKey) + if err != nil { + t.Fatal(err) + } + if !ok { + t.Fatal("node is supposed to control wallet signers") + } + + walletChainData, err := walletExecutor.chain.GetWallet( + bitcoin.PublicKeyHash(walletPublicKey), + ) + if err != nil { + t.Fatal(err) + } + + expectedSignerSetHash, err := computeSignerApprovalCertificateSignerSetHash( + walletPublicKey, + walletChainData, + walletExecutor.groupParameters, + ) + if err != nil { + t.Fatal(err) + } + + return expectedSignerSetHash +} From 5561ea425aa11f5a6890f2fde93c2e52ba483711 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Ros=C5=82aniec?= Date: Mon, 20 Jul 2026 09:34:44 +0000 Subject: [PATCH 2/3] test(covenantsigner): cover body-limit boundary and init-failure lock release --- pkg/covenantsigner/server_test.go | 61 +++++++++++++++++++++++++++ pkg/covenantsigner/store_lock_test.go | 50 ++++++++++++++++++++++ 2 files changed, 111 insertions(+) diff --git a/pkg/covenantsigner/server_test.go b/pkg/covenantsigner/server_test.go index 246f103bed..46678c2d65 100644 --- a/pkg/covenantsigner/server_test.go +++ b/pkg/covenantsigner/server_test.go @@ -1080,3 +1080,64 @@ func TestSubmitHandlerPreservesServiceContextValues(t *testing.T) { ) } } + +// TestServerAcceptsBodyExactlyAtLimit asserts the request-body size guard is an +// inclusive cap: a body of exactly maxRequestBodyBytes must pass, since +// http.MaxBytesReader only rejects bodies strictly larger than the limit. This +// complements the maxRequestBodyBytes+1 rejection in TestServerBoundaryErrorMatrix +// and pins the exact boundary so an off-by-one in the cap would be caught. +func TestServerAcceptsBodyExactlyAtLimit(t *testing.T) { + handle := newMemoryHandle() + service, err := NewService(handle, &scriptedEngine{ + submit: func(*Job) (*Transition, error) { + return &Transition{State: JobStatePending, Detail: "queued"}, nil + }, + }) + if err != nil { + t.Fatal(err) + } + + server := httptest.NewServer(newHandler(service, context.Background(), "test-token", true)) + defer server.Close() + + // Build a well-formed submit envelope padded to exactly maxRequestBodyBytes + // by stretching the facadeRequestId string field. + prefix := `{"routeRequestId":"ors_exact","stage":"SIGNER_COORDINATION","request":{"facadeRequestId":"` + suffix := `"}}` + pad := maxRequestBodyBytes - len(prefix) - len(suffix) + if pad <= 0 { + t.Fatalf("test assumption broken: prefix+suffix already exceed the body limit") + } + body := []byte(prefix + strings.Repeat("a", pad) + suffix) + if len(body) != maxRequestBodyBytes { + t.Fatalf("expected body of exactly %d bytes, got %d", maxRequestBodyBytes, len(body)) + } + + request, err := http.NewRequest( + http.MethodPost, + server.URL+"/v1/self_v1/signer/requests", + bytes.NewReader(body), + ) + if err != nil { + t.Fatal(err) + } + request.Header.Set("Content-Type", "application/json") + request.Header.Set("Authorization", "Bearer test-token") + + response, err := http.DefaultClient.Do(request) + if err != nil { + t.Fatal(err) + } + defer response.Body.Close() + + // The body may still be rejected on its contents, but it must not be + // rejected by the size guard, which surfaces as "malformed request body". + responseBody, _ := io.ReadAll(response.Body) + if strings.Contains(string(responseBody), "malformed request body") { + t.Fatalf( + "body exactly at the limit was rejected as malformed (size guard fired): %d %s", + response.StatusCode, + string(responseBody), + ) + } +} diff --git a/pkg/covenantsigner/store_lock_test.go b/pkg/covenantsigner/store_lock_test.go index b763352c8b..dc337ed7fe 100644 --- a/pkg/covenantsigner/store_lock_test.go +++ b/pkg/covenantsigner/store_lock_test.go @@ -146,3 +146,53 @@ func TestNewStore_SequentialOpenCloseOpen(t *testing.T) { } } } + +// TestNewServiceReleasesLockOnInitFailure asserts that when NewService acquires +// the store's file lock and then fails a later initialization step, it releases +// the lock rather than leaking it. Without the release, a signer that failed to +// start once (e.g. on a misconfigured trust root) could never restart against +// the same data directory. The failure is triggered with an invalid custodian +// trust-root public key, which is normalized after the store (and its lock) is +// created. +func TestNewServiceReleasesLockOnInitFailure(t *testing.T) { + tempDir := t.TempDir() + + firstHandle, err := persistence.NewBasicDiskHandle(tempDir) + if err != nil { + t.Fatal(err) + } + + _, err = NewService( + firstHandle, + &scriptedEngine{}, + WithDataDir(tempDir), + WithCustodianTrustRoots([]CustodianTrustRoot{ + { + Route: TemplateQcV1, + Reserve: validMigrationDestination().Reserve, + Network: validMigrationDestination().Network, + PublicKey: "0x1234", + }, + }), + ) + if err == nil { + t.Fatal("expected NewService to fail on the invalid custodian trust root") + } + + // A second NewService on the same data directory must be able to acquire the + // lock, proving the failed first attempt released it. + secondHandle, err := persistence.NewBasicDiskHandle(tempDir) + if err != nil { + t.Fatal(err) + } + + service, err := NewService(secondHandle, &scriptedEngine{}, WithDataDir(tempDir)) + if err != nil { + t.Fatalf( + "expected second NewService to succeed; the lock should have been "+ + "released after the first init failure, got: %v", + err, + ) + } + t.Cleanup(func() { _ = service.Close() }) +} From eb5aab5ec8f7d9a4519e7567d1a614b9b05f260c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Ros=C5=82aniec?= Date: Mon, 20 Jul 2026 10:21:48 +0000 Subject: [PATCH 3/3] test(tbtc): drop dead signer-set-hash input from cert gate tests The version and signature-algorithm gates in verifySignerApprovalCertificate return before the expected signer-set hash is examined, so the hash computed via expectedSignerSetHashForWallet was never evaluated on those paths. Pass an arbitrary non-empty placeholder instead and document that a reordering of the gate after the hash comparison would now surface as a mismatch, catching the regression. Removes the helper, whose only two call sites are gone. --- pkg/tbtc/signer_approval_certificate_test.go | 57 ++++++-------------- 1 file changed, 16 insertions(+), 41 deletions(-) diff --git a/pkg/tbtc/signer_approval_certificate_test.go b/pkg/tbtc/signer_approval_certificate_test.go index dd0d2bdb24..f748e7a936 100644 --- a/pkg/tbtc/signer_approval_certificate_test.go +++ b/pkg/tbtc/signer_approval_certificate_test.go @@ -1047,7 +1047,13 @@ func TestVerifySignerApprovalCertificateRejectsUnsupportedVersion(t *testing.T) covenantsigner.TemplateSelfV1, ) - expectedSignerSetHash := expectedSignerSetHashForWallet(t, node, walletPublicKey) + // The version gate is checked before the signer-set hash is examined, so the + // expected hash passed here is a deliberately arbitrary, non-empty + // placeholder: its value must not affect the outcome. Were the version check + // ever reordered after the hash comparison, this mismatching hash would + // surface as a "signer set hash does not match" error and fail the assertion + // below, catching the reordering. + arbitrarySignerSetHash := "0x" + strings.Repeat("ab", 32) // An unset (0), the superseded (1), and a future (3) version must all be // rejected; only 2 is supported. @@ -1055,7 +1061,7 @@ func TestVerifySignerApprovalCertificateRejectsUnsupportedVersion(t *testing.T) certificate := *request.SignerApproval certificate.CertificateVersion = unsupportedVersion - err := verifySignerApprovalCertificate(&certificate, expectedSignerSetHash) + err := verifySignerApprovalCertificate(&certificate, arbitrarySignerSetHash) if err == nil || !strings.Contains(err.Error(), "unsupported certificate version") { t.Fatalf( "expected unsupported certificate version error for version %d, got %v", @@ -1080,50 +1086,19 @@ func TestVerifySignerApprovalCertificateRejectsUnsupportedSignatureAlgorithm(t * covenantsigner.TemplateSelfV1, ) - expectedSignerSetHash := expectedSignerSetHashForWallet(t, node, walletPublicKey) + // The algorithm gate is checked before the signer-set hash is examined, so + // the expected hash passed here is a deliberately arbitrary, non-empty + // placeholder: its value must not affect the outcome. Were the algorithm + // check ever reordered after the hash comparison, this mismatching hash would + // surface as a "signer set hash does not match" error and fail the assertion + // below, catching the reordering. + arbitrarySignerSetHash := "0x" + strings.Repeat("ab", 32) certificate := *request.SignerApproval certificate.SignatureAlgorithm = "ed25519" - err := verifySignerApprovalCertificate(&certificate, expectedSignerSetHash) + err := verifySignerApprovalCertificate(&certificate, arbitrarySignerSetHash) if err == nil || !strings.Contains(err.Error(), "unsupported signature algorithm") { t.Fatalf("expected unsupported signature algorithm error, got %v", err) } } - -// expectedSignerSetHashForWallet recomputes the signer-set hash the verifier -// expects for the wallet the test node controls, mirroring the inline block used -// by the other verifySignerApprovalCertificate tests. -func expectedSignerSetHashForWallet( - t *testing.T, - node *node, - walletPublicKey *ecdsa.PublicKey, -) string { - t.Helper() - - walletExecutor, ok, err := node.getSigningExecutor(walletPublicKey) - if err != nil { - t.Fatal(err) - } - if !ok { - t.Fatal("node is supposed to control wallet signers") - } - - walletChainData, err := walletExecutor.chain.GetWallet( - bitcoin.PublicKeyHash(walletPublicKey), - ) - if err != nil { - t.Fatal(err) - } - - expectedSignerSetHash, err := computeSignerApprovalCertificateSignerSetHash( - walletPublicKey, - walletChainData, - walletExecutor.groupParameters, - ) - if err != nil { - t.Fatal(err) - } - - return expectedSignerSetHash -}