Skip to content

bug(fxconfig/transaction): Endorse() silently appends duplicate endorsements when called twice with the same signer #241

@Prachi194agrawal

Description

@Prachi194agrawal

Summary

Endorse() has a comment that reads:

// check that tx does not yet carry any endorsements

But the actual guard only checks whether tx.Endorsements is nil:

if tx.Endorsements == nil {
    tx.Endorsements = make([]*applicationpb.Endorsements, len(tx.GetNamespaces()))
}

This means if Endorse() is called a second time on a transaction that already
has endorsements, the nil check is silently skipped and a duplicate
EndorsementWithIdentity with the same MSP ID is appended for every namespace —
with no error, no warning, and no deduplication.

Affected File

tools/fxconfig/internal/transaction/endorse.go

Steps to Reproduce

  1. Create a *applicationpb.Tx with at least one namespace.
  2. Call Endorse(signer, txID, tx) — succeeds, one endorsement per namespace.
  3. Call Endorse(signer, txID, tx1) again with the same signer on the
    returned transaction.
  4. Inspect tx2.Endorsements[0].EndorsementsWithIdentity — it now contains
    two entries with the same Identity.MspId.

Repro Test (added locally)

endorse_test.go lines 212–230 reliably reproduces the duplicate append.

Expected Behaviour

Endorse() should detect that an endorsement from the same MSP ID already exists
for a given namespace and either:

  • return a descriptive error such as
    "duplicate endorsement: signer %q already endorsed namespace %d", or
  • silently skip the duplicate (no-op).

Actual Behaviour

The duplicate endorsement is appended with no error and no log warning, leaving
the transaction in an inconsistent state that may cause downstream validation
failures when the same MSP ID appears more than once for a namespace.

Inconsistency with mergeEndorsements()

mergeEndorsements() in the same package already deduplicates endorsements by
MSP ID explicitly. Endorse() applying no such check creates a trust gap — a
transaction can leave Endorse() carrying duplicates that would only be caught
(or silently broken) further downstream.

Suggested Fix

Before appending eid, scan the existing entries and bail out on a match:

for _, existing := range tx.Endorsements[nsIdx].EndorsementsWithIdentity {
    if existing.Identity.MspId == signerIdentity.MspId {
        return nil, fmt.Errorf(
            "duplicate endorsement: signer %q already endorsed namespace %d",
            signerIdentity.MspId, nsIdx,
        )
    }
}
tx.Endorsements[nsIdx].EndorsementsWithIdentity = append(
    tx.Endorsements[nsIdx].EndorsementsWithIdentity, eid,
)

This aligns Endorse() with the deduplication intent already expressed in both
the existing comment and mergeEndorsements().

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions