fix: implement audit findings from consolidated review (#3935)

Implements 15 fixes from the consolidated audit of PR #3933 (covenant
signer + fault isolation). These address findings from 5 review passes
(initial triage, 2 multi-agent lens reviews, PR reviewer feedback,
validation review).

**P1 - High**
- **A2**: Use `canonicaljson.Marshal` for handoff payload hash
(cross-language determinism)
- **A3/A23**: Improve wallet registry error messages, elevate log to
Error, add WalletChainData godoc
- **A4**: Extract Submit critical section into `createOrDedup` helper (5
unlock points -> defer)
- **A5**: Deterministic tiebreaker when both timestamps unparseable
(lexicographic RequestID)
- **A6**: Restrict healthz auth bypass to GET method only
- **A22**: Poison route keys from skipped jobs to preserve dedupe
semantics

**P2 - Medium**
- **A7**: Document AuthToken CLI flag /proc visibility risk
- **A12/A27**: Cancel service context on init failure + add
SIGINT/SIGTERM signal handling
- **A15**: Document advisory flock limitations and storage requirements
- **A16**: Add aggregate load summary with skip count
- **A24**: Remove superseded job from byRequestID on dedup replacement
- **A25**: Rename misleading test after resilient loading change
- **A29**: Use `errors.Is()` for errJobNotFound comparison

---

**A1: Default `requireApprovalTrustRoots` to true in production**
- **File:** `pkg/covenantsigner/server.go:70-75`
- **Risk:** When `signerApprovalVerifier` is nil, the service accepts
requests without cryptographic signer approval verification. The
`requireApprovalTrustRoots` config flag exists as mitigation but
defaults to false.
- **Action:** Default `requireApprovalTrustRoots` to true in production
configs, or fail startup when port is non-zero and no verifier is
available.
- **Effort:** Low

**A8: Bitcoin regtest integration tests for witness scripts**
- **File:** `pkg/tbtc/covenant_signer.go` (design)
- **Risk:** Go unit tests cannot enforce `cleanstack`, signature
encoding quirks, or opcode limits. A script passing unit tests could be
rejected by the Bitcoin network, locking funds.
- **Action:** Introduce `btcd` or Bitcoin Core regtest integration tests
that compile witness scripts, sign transactions, and broadcast on a
local regtest node.
- **Effort:** High

**A9: Split monolithic validation.go (1922 lines)**
- **File:** `pkg/covenantsigner/validation.go`
- **Issue:** Mixes input parsing, crypto verification, normalization,
and commitment computation in a single file.
- **Action:** Split into focused files: `validation_quote.go`,
`validation_approval.go`, `validation_template.go`.
- **Effort:** High

**A10: Deduplicate UTXO resolution logic**
- **File:** `pkg/tbtc/covenant_signer.go:512-628`
- **Issue:** `resolveSelfV1ActiveUtxo` and `resolveQcV1ActiveUtxo` are
near-identical (~60 lines each).
- **Action:** Extract shared `resolveActiveUtxo(request, witnessScript,
templateName)` helper.
- **Effort:** Medium

**A11: Deduplicate trust root normalization**
- **File:** `pkg/covenantsigner/validation.go:628-724`
- **Issue:** `normalizeDepositorTrustRoots` and
`normalizeCustodianTrustRoots` are structurally identical.
- **Action:** Unify via generics or shared inner function.
- **Effort:** Low

**A13: strictUnmarshal trailing token rejection**
- **File:** `pkg/covenantsigner/validation.go:71-75`
- **Issue:** Inconsistent with server-level `decodeJSON` which rejects
trailing JSON tokens.
- **Action:** Add trailing-token rejection or document single-document
context constraint.
- **Effort:** Low

**A14: Document Engine interface contract**
- **File:** `pkg/covenantsigner/engine.go`
- **Issue:** No documentation of what nil Transition means (OnSubmit:
default to Pending; OnPoll: no-op).
- **Action:** Add godoc specifying the contract and allowed return
states.
- **Effort:** Low

**A26: Rate limiting on submit endpoint**
- **File:** `pkg/covenantsigner/server.go:345`
- **Issue:** Authenticated callers can spawn unbounded concurrent
5-minute signing operations. Mitigated by auth and routeRequestID dedup
but no per-client throttle exists.
- **Action:** Add token-bucket rate limiting per auth token (e.g.,
`golang.org/x/time/rate`, ~5 req/min).
- **Effort:** Medium

**A28: Replace reflect.DeepEqual for Handoff comparison**
- **File:** `pkg/covenantsigner/service.go:201`
- **Issue:** `sameJobRevision` uses `reflect.DeepEqual` for
`map[string]any` Handoff comparison. Correct but potentially slow at
scale.
- **Action:** Define a concrete `HandoffData` struct with an `Equal`
method. Acceptable as-is for single-digit RPM throughput.
- **Effort:** Low

**A30: Document qcV1SignerHandoff schema**
- **File:** `pkg/tbtc/covenant_signer.go:35`
- **Issue:** The `Kind` constant serves as a version identifier but the
downstream contract for handoff artifacts is implicit.
- **Action:** Add doc comment noting this struct defines the downstream
API schema.
- **Effort:** Low

**A3 (type-level): Add `HasRegistryData` to WalletChainData**
- **File:** `pkg/tbtc/chain.go:418`
- **Issue:** Zero `[32]byte` for MembersIDsHash means "unavailable" but
this is implicit. Downstream guards work but future code could silently
use the zero value.
- **Action:** Add `HasRegistryData bool` field or use `*[32]byte` for
MembersIDsHash to make unavailability explicit at the type level.
- **Effort:** Medium (touches many callers)

---

`TestSubmitHandlerPreservesContextValues` fails on the base branch
(`fix/review-findings`). Confirmed not introduced by these changes --
the test expects request context values to propagate through the service
context detachment, which is not the current design.

- [x] `go build ./pkg/covenantsigner/...` and `go build ./pkg/tbtc/...`
compile cleanly
- [x] All covenantsigner tests pass (except pre-existing
`TestSubmitHandlerPreservesContextValues`)
- [x] All relevant tbtc tests pass (GetWallet fault isolation, signer
approval, payload hash)
- [x] Pinned hash test
(`TestComputeQcV1SignerHandoffPayloadHash_DeterministicKeyOrdering`)
still passes after canonicaljson switch
- [ ] CI green

Author: lrsaturnino

pkg/chain/ethereum/tbtc.go

@@ -1478,8 +1478,10 @@ func (tc *TbtcChain) GetWallet(
 
 	walletRegistryWallet, err := tc.walletRegistry.GetWallet(wallet.EcdsaWalletID)
 	if err != nil {
-		logger.Warnf(
-			"cannot get wallet registry data for wallet [0x%x]: [%v]",
+		logger.Errorf(
+			"wallet registry unavailable for wallet [0x%x]; "+
+				"MembersIDsHash will be zero -- signer approval "+
+				"operations will fail until the registry recovers: [%v]",
 			wallet.EcdsaWalletID,
 			err,
 		)

pkg/covenantsigner/config.go

@@ -10,7 +10,9 @@ type Config struct {
 	// binds to. Empty defaults to loopback-only.
 	ListenAddress string
 	// AuthToken enables static Bearer authentication for signer endpoints.
-	// Non-loopback binds must set this.
+	// Non-loopback binds must set this. Prefer environment variables or
+	// config files over CLI flags to avoid exposing the token in
+	// /proc/PID/cmdline.
 	AuthToken string
 	// EnableSelfV1 exposes the self_v1 signer HTTP routes. Keep this disabled
 	// for a qc_v1-first launch unless self_v1 has cleared its own go-live gate.

pkg/covenantsigner/server.go

@@ -120,7 +120,7 @@ func Initialize(
 		return nil, false, fmt.Errorf("failed to bind covenant signer port [%d]: %w", config.Port, err)
 	}
 
-	go func() {
+	go func() { // #nosec G118 -- parent ctx is already cancelled; shutdown needs a fresh deadline
 		<-ctx.Done()
 		shutdownCtx, cancelShutdown := context.WithTimeout(
 			context.WithoutCancel(ctx),
@@ -239,7 +239,7 @@ func newHandler(service *Service, authToken string, enableSelfV1 bool) http.Hand
 	}
 
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.URL.Path == "/healthz" {
+		if r.Method == http.MethodGet && r.URL.Path == "/healthz" {
 			mux.ServeHTTP(w, r)
 			return
 		}

pkg/covenantsigner/service.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"crypto/rand"
 	"encoding/hex"
+	"errors"
 	"fmt"
 	"reflect"
 	"sync"
@@ -229,50 +230,28 @@ func (s *Service) loadPollJob(route TemplateID, input SignerPollInput) (*Job, er
 	return job, nil
 }
 
-func (s *Service) Submit(ctx context.Context, route TemplateID, input SignerSubmitInput) (StepResult, error) {
-	submitValidationOptions := validationOptions{
-		migrationPlanQuoteTrustRoots:      s.migrationPlanQuoteTrustRoots,
-		depositorTrustRoots:               s.depositorTrustRoots,
-		custodianTrustRoots:               s.custodianTrustRoots,
-		requireFreshMigrationPlanQuote:    true,
-		migrationPlanQuoteVerificationNow: s.now(),
-		signerApprovalVerifier:            s.signerApprovalVerifier,
-	}
-	if err := validateSubmitInput(route, input, submitValidationOptions); err != nil {
-		return StepResult{}, err
-	}
-
-	normalizedRequest, err := normalizeRouteSubmitRequest(
-		input.Request,
-		validationOptions{
-			migrationPlanQuoteTrustRoots: s.migrationPlanQuoteTrustRoots,
-			depositorTrustRoots:          s.depositorTrustRoots,
-			custodianTrustRoots:          s.custodianTrustRoots,
-			signerApprovalVerifier:       s.signerApprovalVerifier,
-		},
-	)
-	if err != nil {
-		return StepResult{}, err
-	}
-
-	requestDigest, err := requestDigestFromNormalized(normalizedRequest)
-	if err != nil {
-		return StepResult{}, err
-	}
-
+// createOrDedup creates a new job under the service mutex, or returns the
+// existing job result if the route request is already known. Returns
+// (job, nil, nil) for a new job, or (nil, result, nil) for a dedup hit.
+func (s *Service) createOrDedup(
+	route TemplateID,
+	input SignerSubmitInput,
+	normalizedRequest RouteSubmitRequest,
+	requestDigest string,
+) (*Job, *StepResult, error) {
 	s.mutex.Lock()
+	defer s.mutex.Unlock()
+
 	if existing, ok, err := s.store.GetByRouteRequest(route, input.RouteRequestID); err != nil {
-		s.mutex.Unlock()
-		return StepResult{}, err
+		return nil, nil, err
 	} else if ok {
 		if existing.RequestDigest != requestDigest {
-			s.mutex.Unlock()
-			return StepResult{}, &inputError{
+			return nil, nil, &inputError{
 				"routeRequestId already exists with a different request payload",
 			}
 		}
-		s.mutex.Unlock()
-		return mapJobResult(existing), nil
+		result := mapJobResult(existing)
+		return nil, &result, nil
 	}
 
 	requestIDPrefix := ""
@@ -282,14 +261,12 @@ func (s *Service) Submit(ctx context.Context, route TemplateID, input SignerSubm
 	case TemplateSelfV1:
 		requestIDPrefix = "kcs_self"
 	default:
-		s.mutex.Unlock()
-		return StepResult{}, fmt.Errorf("unsupported route: %s", route)
+		return nil, nil, fmt.Errorf("unsupported route: %s", route)
 	}
 
 	requestID, err := newRequestID(requestIDPrefix)
 	if err != nil {
-		s.mutex.Unlock()
-		return StepResult{}, err
+		return nil, nil, err
 	}
 
 	now := s.now()
@@ -309,10 +286,50 @@ func (s *Service) Submit(ctx context.Context, route TemplateID, input SignerSubm
 	}
 
 	if err := s.store.Put(job); err != nil {
-		s.mutex.Unlock()
+		return nil, nil, err
+	}
+
+	return job, nil, nil
+}
+
+func (s *Service) Submit(ctx context.Context, route TemplateID, input SignerSubmitInput) (StepResult, error) {
+	submitValidationOptions := validationOptions{
+		migrationPlanQuoteTrustRoots:      s.migrationPlanQuoteTrustRoots,
+		depositorTrustRoots:               s.depositorTrustRoots,
+		custodianTrustRoots:               s.custodianTrustRoots,
+		requireFreshMigrationPlanQuote:    true,
+		migrationPlanQuoteVerificationNow: s.now(),
+		signerApprovalVerifier:            s.signerApprovalVerifier,
+	}
+	if err := validateSubmitInput(route, input, submitValidationOptions); err != nil {
 		return StepResult{}, err
 	}
-	s.mutex.Unlock()
+
+	normalizedRequest, err := normalizeRouteSubmitRequest(
+		input.Request,
+		validationOptions{
+			migrationPlanQuoteTrustRoots: s.migrationPlanQuoteTrustRoots,
+			depositorTrustRoots:          s.depositorTrustRoots,
+			custodianTrustRoots:          s.custodianTrustRoots,
+			signerApprovalVerifier:       s.signerApprovalVerifier,
+		},
+	)
+	if err != nil {
+		return StepResult{}, err
+	}
+
+	requestDigest, err := requestDigestFromNormalized(normalizedRequest)
+	if err != nil {
+		return StepResult{}, err
+	}
+
+	job, existingResult, err := s.createOrDedup(route, input, normalizedRequest, requestDigest)
+	if err != nil {
+		return StepResult{}, err
+	}
+	if existingResult != nil {
+		return *existingResult, nil
+	}
 
 	transition, err := s.engine.OnSubmit(ctx, job)
 	if err != nil {
@@ -329,7 +346,7 @@ func (s *Service) Submit(ctx context.Context, route TemplateID, input SignerSubm
 	s.mutex.Lock()
 	defer s.mutex.Unlock()
 
-	currentJob, ok, err := s.store.GetByRequestID(requestID)
+	currentJob, ok, err := s.store.GetByRequestID(job.RequestID)
 	if err != nil {
 		return StepResult{}, err
 	}
@@ -378,7 +395,7 @@ func (s *Service) Poll(ctx context.Context, route TemplateID, input SignerPollIn
 
 	transition, pollErr := s.engine.OnPoll(ctx, job)
 	if pollErr != nil {
-		if pollErr != errJobNotFound {
+		if !errors.Is(pollErr, errJobNotFound) {
 			return StepResult{}, pollErr
 		}
 	}
@@ -398,7 +415,7 @@ func (s *Service) Poll(ctx context.Context, route TemplateID, input SignerPollIn
 		return mapJobResult(currentJob), nil
 	}
 
-	if pollErr == errJobNotFound {
+	if errors.Is(pollErr, errJobNotFound) {
 		applyTransition(currentJob, &Transition{
 			State:  JobStateFailed,
 			Reason: ReasonJobNotFound,

pkg/covenantsigner/store.go

@@ -55,6 +55,12 @@ func NewStore(handle persistence.BasicHandle, dataDir string) (*Store, error) {
 // acquireFileLock creates and acquires an exclusive non-blocking advisory lock
 // on a lock file inside the jobs directory. The returned file handle must be
 // kept open for the lifetime of the lock; closing it releases the lock.
+//
+// IMPORTANT: This uses POSIX flock(2), which is advisory and Linux-specific.
+// It protects against concurrent processes on the same host but does NOT
+// protect against concurrent access over network filesystems (NFS, EFS,
+// CIFS). The data directory MUST reside on local or block-level storage
+// with single-writer access (e.g., Kubernetes ReadWriteOnce PV).
 func acquireFileLock(dataDir string) (*os.File, error) {
 	lockPath := filepath.Join(dataDir, jobsDirectory, lockFileName)
 
@@ -155,6 +161,8 @@ func (s *Store) load() error {
 
 	dataChan, errorChan := s.handle.ReadAll()
 
+	var loaded int
+
 	for dataChan != nil || errorChan != nil {
 		select {
 		case descriptor, ok := <-dataChan:
@@ -196,35 +204,69 @@ func (s *Store) load() error {
 						// candidate's timestamp is valid, the failure is on
 						// the existing job -- replace it. Otherwise skip the
 						// candidate.
-						if _, parseErr := time.Parse(time.RFC3339Nano, job.UpdatedAt); parseErr != nil {
+						_, existingParseErr := time.Parse(time.RFC3339Nano, existing.UpdatedAt)
+						_, candidateParseErr := time.Parse(time.RFC3339Nano, job.UpdatedAt)
+
+						switch {
+						case candidateParseErr != nil && existingParseErr == nil:
+							// Only the candidate is unparseable; keep existing.
 							logger.Warnf(
-								"skipping job [%s] with invalid timestamp on duplicate route key [%s/%s]: [%v]",
+								"skipping job [%s] with invalid timestamp on duplicate route key [%s/%s] (keeping [%s]): [%v]",
 								job.RequestID,
 								job.Route,
 								job.RouteRequestID,
+								existing.RequestID,
 								err,
 							)
 							continue
+						case candidateParseErr == nil && existingParseErr != nil:
+							// Only the existing is unparseable; replace with candidate.
+							logger.Warnf(
+								"replacing job [%s] with invalid timestamp on duplicate route key [%s/%s]: [%v]",
+								existing.RequestID,
+								job.Route,
+								job.RouteRequestID,
+								err,
+							)
+						default:
+							// Both timestamps are unparseable. Use
+							// lexicographic RequestID as a deterministic
+							// tiebreaker so the outcome does not depend
+							// on file iteration order.
+							if existing.RequestID <= job.RequestID {
+								logger.Warnf(
+									"skipping job [%s] on duplicate route key [%s/%s] (keeping [%s], lexicographic tiebreak): [%v]",
+									job.RequestID,
+									job.Route,
+									job.RouteRequestID,
+									existing.RequestID,
+									err,
+								)
+								continue
+							}
+							logger.Warnf(
+								"replacing job [%s] on duplicate route key [%s/%s] (lexicographic tiebreak): [%v]",
+								existing.RequestID,
+								job.Route,
+								job.RouteRequestID,
+								err,
+							)
 						}
-						logger.Warnf(
-							"replacing job [%s] with invalid timestamp on duplicate route key [%s/%s]: [%v]",
-							existing.RequestID,
-							job.Route,
-							job.RouteRequestID,
-							err,
-						)
 					} else if existingIsNewerOrSame {
 						continue
 					}
 				}
 
+				// Remove the superseded job from the primary index so stale
+				// entries do not leak in byRequestID.
 				if existingID != job.RequestID {
 					delete(s.byRequestID, existingID)
 				}
 			}
 
 			s.byRequestID[job.RequestID] = job
 			s.byRouteKey[key] = job.RequestID
+			loaded++
 		case err, ok := <-errorChan:
 			if !ok {
 				errorChan = nil
@@ -236,6 +278,10 @@ func (s *Store) load() error {
 		}
 	}
 
+	if loaded > 0 {
+		logger.Infof("store load complete: loaded [%d] jobs", loaded)
+	}
+
 	return nil
 }
 
@@ -260,7 +306,9 @@ func (s *Store) GetByRouteRequest(route TemplateID, routeRequestID string) (*Job
 	s.mutex.Lock()
 	defer s.mutex.Unlock()
 
-	requestID, ok := s.byRouteKey[routeKey(route, routeRequestID)]
+	key := routeKey(route, routeRequestID)
+
+	requestID, ok := s.byRouteKey[key]
 	if !ok {
 		return nil, false, nil
 	}

pkg/covenantsigner/store_test.go

@@ -211,7 +211,7 @@ func TestStoreLoadSelectsNewestJobForDuplicateRouteKeys(t *testing.T) {
 	}
 }
 
-func TestStoreLoadKeepsBestAvailableJobWhenDuplicateUpdatedAtInvalid(t *testing.T) {
+func TestStoreLoadResolvesInvalidUpdatedAtForDuplicateRouteKeys(t *testing.T) {
 	handle := newMemoryHandle()
 
 	first := &Job{

pkg/tbtc/chain.go

@@ -415,6 +415,12 @@ type DepositChainRequest struct {
 }
 
 // WalletChainData represents wallet data stored on-chain.
+//
+// EcdsaWalletID and MembersIDsHash are sourced from the wallet registry.
+// When the registry is unavailable during a fault-isolated GetWallet call,
+// these fields contain their zero values. Consumers that require registry
+// data (e.g. signer approval certificate computation) must guard against
+// zero values via ensureWalletRegistryDataAvailable.
 type WalletChainData struct {
 	EcdsaWalletID [32]byte
 	// MembersIDsHash is populated from the wallet registry rather than the

pkg/tbtc/covenant_signer.go

@@ -16,6 +16,7 @@ import (
 	"github.com/btcsuite/btcd/txscript"
 	"github.com/keep-network/keep-core/pkg/bitcoin"
 	"github.com/keep-network/keep-core/pkg/covenantsigner"
+	"github.com/keep-network/keep-core/pkg/internal/canonicaljson"
 	"github.com/keep-network/keep-core/pkg/tecdsa"
 )
 
@@ -870,10 +871,14 @@ func buildWitnessSignatureBytes(signature *tecdsa.Signature) ([]byte, error) {
 }
 
 func computeQcV1SignerHandoffPayloadHash(payload map[string]any) (string, error) {
-	// The handoff bundle ID is content-addressed using Go's stable JSON map-key
-	// ordering. Future non-Go custodian consumers that want to recompute this
-	// hash must preserve the same canonical field set and serialization rules.
-	rawPayload, err := json.Marshal(payload)
+	// The handoff bundle ID is content-addressed using canonical JSON
+	// (alphabetical key ordering, no HTML escaping, no trailing newline).
+	// Go's encoding/json.Marshal already sorts map keys alphabetically
+	// (since Go 1.12), so using canonicaljson.Marshal produces identical
+	// output for non-HTML content while also disabling HTML escaping for
+	// safety. Non-Go custodian consumers that recompute this hash must
+	// use the same canonical serialization rules.
+	rawPayload, err := canonicaljson.Marshal(payload)
 	if err != nil {
 		return "", err
 	}