fix(covenantsigner): apply audit findings from #3935 rebased onto current base (#3940)

## Problem

PR #3935 was merged into `fix/review-findings`, an intermediate base
branch that was never integrated into
`feat/psbt-covenant-final-project-pr`. This left
`feat/psbt-covenant-final-project-pr` without the audit-finding fixes
that #3935 implemented (createOrDedup mutex extraction, healthz auth
bypass restriction, canonical JSON for handoff payload, error message
improvements, gosec hardening, and more).

## Solution

This PR cherry-picks the squash commit of #3935 onto the current state
of `feat/psbt-covenant-final-project-pr`, which already contains PR
#3933 (Maclane's fault-isolation work plus his subsequent fixes for the
review comments raised on that PR). Three files required manual conflict
resolution where #3935 and #3933 made overlapping changes addressing the
same concerns. In `pkg/tbtc/signer_approval_certificate.go`, the
`ErrMissingWalletID` and `ErrMissingMembersIDsHash` sentinel errors from
#3935 were dropped because Maclane's `ensureWalletRegistryDataAvailable`
function already covers the same case and is in active use; keeping both
would have left the sentinels as dead code. In
`pkg/covenantsigner/server_test.go`, the test rewrite from #3935 (which
depended on a `serviceCtx` parameter to `newHandler`) was reverted in
favor of the existing middleware-based test, which already exercises the
`context.WithoutCancel(r.Context())` detachment path. In
`pkg/covenantsigner/store_test.go`, the test rename collision was
resolved by taking #3935's name
(`TestStoreLoadResolvesInvalidUpdatedAtForDuplicateRouteKeys`).

Three additional cleanups followed from the merge mechanics: the godoc
on `WalletChainData.MembersIDsHash` was updated to reference
`ensureWalletRegistryDataAvailable` instead of the dropped sentinels, an
orphaned `cancelService()` call was removed from `server.go` (its
`serviceCtx` infrastructure did not auto-merge), and a duplicate
`delete(s.byRequestID, existingID)` was deduplicated in `store.go`'s
load path.

## Tests

All tests across `pkg/covenantsigner`, `pkg/tbtc`, and
`pkg/chain/ethereum` pass. A behavioral diff against the base branch
shows identical build, vet, and file-listing outputs, with all `ok`
lines matching except for timing variation. The two adapter tests added
by Maclane (`TestMakeWalletChainData...`) and the two
registry-unavailable tests for the issue-and-verify signer approval
paths are preserved and pass.

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
 	}