hyperledger / hyperledger/fabric-x

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

Open
#241 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
64
Forks
80
Avg merge
1d 22h
Merged PRs (30d)
15

Description

### 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`:

```go
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:

```go
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()`.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.