Skip to content

node: Consolidate borsch library usage #4892

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
djb15 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

node: Consolidate borsch library usage #4892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion node/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ require (
github.com/libp2p/go-libp2p-pubsub v0.12.0
github.com/mr-tron/base58 v1.2.0
github.com/multiformats/go-multiaddr v0.13.0
github.com/near/borsh-go v0.3.0
github.com/prometheus/client_golang v1.20.5
github.com/spf13/cobra v1.6.1
github.com/spf13/pflag v1.0.5
Expand Down
2 changes: 0 additions & 2 deletions node/go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3416,8 +3416,6 @@ github.com/nats-io/nuid v1.0.1/go.mod h1:19wcPz3Ph3q0Jbyiqsd0kePYG7A95tJPxeL+1OS
github.com/nbutton23/zxcvbn-go v0.0.0-20180912185939-ae427f1e4c1d/go.mod h1:o96djdrsSGy3AWPyBgZMAGfxZNfgntdJG+11KU4QvbU=
github.com/nbutton23/zxcvbn-go v0.0.0-20210217022336-fa2cb2858354/go.mod h1:KSVJerMDfblTH7p5MZaTt+8zaT2iEk3AkVb9PQdZuE8=
github.com/ncw/swift v1.0.47/go.mod h1:23YIA4yWVnGwv2dQlN4bB7egfYX6YLn0Yo/S6zZO/ZM=
github.com/near/borsh-go v0.3.0 h1:+DvG7eApOD3KrHIh7TwZvYzhXUF/OzMTC6aRTUEtW+8=
github.com/near/borsh-go v0.3.0/go.mod h1:NeMochZp7jN/pYFuxLkrZtmLqbADmnp/y1+/dL+AsyQ=
github.com/neelance/astrewrite v0.0.0-20160511093645-99348263ae86/go.mod h1:kHJEU3ofeGjhHklVoIGuVj85JJwZ6kWPaJwCIxgnFmo=
github.com/neelance/sourcemap v0.0.0-20151028013722-8c68805598ab/go.mod h1:Qr6/a/Q4r9LP1IltGz7tA7iOK1WonHEYhu1HRBA7ZiM=
github.com/neilotoole/errgroup v0.1.5/go.mod h1:Q2nLGf+594h0CLBs/Mbg6qOr7GtqDK7C2S41udRnToE=
Expand Down
10 changes: 7 additions & 3 deletions node/pkg/watchers/solana/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@ import (
"github.com/certusone/wormhole/node/pkg/readiness"
"github.com/certusone/wormhole/node/pkg/supervisor"
"github.com/certusone/wormhole/node/pkg/watchers"
bin "github.com/gagliardetto/binary"
"github.com/gagliardetto/solana-go"
lookup "github.com/gagliardetto/solana-go/programs/address-lookup-table"
"github.com/gagliardetto/solana-go/rpc"
"github.com/gagliardetto/solana-go/rpc/jsonrpc"
"github.com/google/uuid"
"github.com/mr-tron/base58"
"github.com/near/borsh-go"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promauto"
"github.com/wormhole-foundation/wormhole/sdk/vaa"
Expand Down Expand Up @@ -320,6 +320,10 @@ type PostMessageData struct {
ConsistencyLevel ConsistencyLevel
}

func decodeBorsh(dst interface{}, data []byte) error {
return bin.UnmarshalBorsh(dst, data)
}

func NewSolanaWatcher(
rpcUrl string,
wsUrl string,
Expand Down Expand Up @@ -956,7 +960,7 @@ func (s *SolanaWatcher) processInstruction(ctx context.Context, rpcClient *rpc.C

// Decode instruction data (UNTRUSTED)
var data PostMessageData
if err := borsh.Deserialize(&data, inst.Data[1:]); err != nil {
if err := decodeBorsh(&data, inst.Data[1:]); err != nil {
return false, fmt.Errorf("failed to deserialize instruction data: %w", err)
}

Expand Down Expand Up @@ -1308,7 +1312,7 @@ func ParseMessagePublicationAccount(
}

prop := &MessagePublicationAccount{}
if err := borsh.Deserialize(prop, data[prefixLength:]); err != nil {
if err := decodeBorsh(prop, data[prefixLength:]); err != nil {
return nil, err
}

Expand Down
95 changes: 76 additions & 19 deletions node/pkg/watchers/solana/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,10 +216,6 @@ func TestParseMessagePublicationAccount(t *testing.T) {
)

const (
// Define error string for testing. This is returned by the borsh-go library when it fails to
// deserialize a struct. In this case, we're relying on the library to fail when
// the message account data can't be deserialized into [MessagePublicationAccount].
errStringBorsh = "failed to read required bytes"
errStringParseTooShort = "message account data is too short"
)

Expand All @@ -228,6 +224,7 @@ func TestParseMessagePublicationAccount(t *testing.T) {
// Named input parameters for target function.
messageAccountData func(t *testing.T) MessageAccountData
want *MessagePublicationAccount
wantErr bool
errStr string
}{
{
Expand Down Expand Up @@ -267,7 +264,29 @@ func TestParseMessagePublicationAccount(t *testing.T) {
Sequence: 3458072,
EmitterChain: 1,
EmitterAddress: emitterAddrUnreliable,
Payload: nil, // borsh deserialization results in this being nil rather than an empty slice
Payload: nil, // Borsh deserialization results in this being nil rather than an empty slice
},
errStr: "",
},
{
name: "success -- trailing bytes are ignored",

@johnsaigle johnsaigle Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a change in behaviour, right? Previously I believe this returned an error, and that's probably safer

@johnsaigle johnsaigle Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If gagliardetto doesn't do this then I think that's arguably a bug in that library

@johnsaigle johnsaigle Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got this from an LLM FWIW:

_Borsh rejects trailing bytes. The reference implementation (borsh-rs) treats any unconsumed bytes after deserialization as an error.
The high-level entry points enforce this:

  • from_slice / try_from_slice — after T::deserialize, check if !v_mut.is_empty() and return Err(InvalidData, "Not all bytes read") (borsh-rs/borsh/src/de/mod.rs).
  • from_reader / try_from_reader — after deserializing, attempt to read 1 more byte; success (non-EOF) triggers the same "Not all bytes read" error.
    The low-level deserialize / deserialize_reader methods themselves do not check — they only consume what the type needs. So the "no trailing bytes" rule is enforced at the API boundary, not per-field. The written spec (the README pseudocode) doesn't call it out explicitly, but the reference implementation's behavior is the de facto standard, and the error constant ERROR_NOT_ALL_BYTES_READ makes the stance unambiguous: the entire input must be consumed._

@djb15 djb15 Jul 13, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes definitely a change of behaviour, although I couldn't think of a way to abuse this, and I also couldn't think of a neat way to keep the behaviour without just creating a wrapper function with the extra trailing bytes validation step. Any strong opinions (including abandoning this PR)?

@johnsaigle johnsaigle Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the PR is good and I do think it would be better to make just such a wrapper function! I'd prefer that the Guardian follows the expected behaviour of Borsh even if it's not exploitable at the moment. It'll keep things easier to reason about IMO, plus set us up to be more compatible with other libraries.

It's also possible that the solana-go behaviour has been patched in a newer version, so if we upgrade that library in the future, that might address it also.

I think it'd be best if there was a code comment on top of it where we link the Borsh standard.

This would be easy to do in a follow-up PR instead, but it would mean that we'd have to revert these changes to the test behaviour later.

messageAccountData: func(t *testing.T) MessageAccountData {
withTrailingBytes := append([]byte{}, validMessageAccountDataReliable...)
withTrailingBytes = append(withTrailingBytes, 0xff, 0xee, 0xdd)
return mustNewMessageAccountData(t, withTrailingBytes)
},
want: &MessagePublicationAccount{
VaaVersion: 0,
ConsistencyLevel: 32,
EmitterAuthority: vaa.Address{},
MessageStatus: 0,
Gap: [3]byte{0, 0, 0},
SubmissionTime: 1770212672,
Nonce: 0,
Sequence: 1367797,
EmitterChain: 1,
EmitterAddress: emitterAddrReliable,
Payload: payload,
},
errStr: "",
},
Expand All @@ -276,8 +295,9 @@ func TestParseMessagePublicationAccount(t *testing.T) {
messageAccountData: func(t *testing.T) MessageAccountData {
return MessageAccountData{}
},
want: &MessagePublicationAccount{},
errStr: errStringParseTooShort,
want: &MessagePublicationAccount{},
wantErr: true,
errStr: errStringParseTooShort,
},
{
name: "failure -- data too short",
Expand All @@ -286,47 +306,60 @@ func TestParseMessagePublicationAccount(t *testing.T) {
// defense-in-depth length check rather than the constructor's validation.
return MessageAccountData{[]byte("ms")}
},
want: &MessagePublicationAccount{},
errStr: errStringParseTooShort,
want: &MessagePublicationAccount{},
wantErr: true,
errStr: errStringParseTooShort,
},
{
name: "failure -- no data following prefix (msg)",
messageAccountData: func(t *testing.T) MessageAccountData {
return mustNewMessageAccountData(t, []byte("msg"))
},
want: &MessagePublicationAccount{},
errStr: errStringBorsh,
want: &MessagePublicationAccount{},
wantErr: true,
},
{
name: "failure -- no data following prefix (msu)",
messageAccountData: func(t *testing.T) MessageAccountData {
return mustNewMessageAccountData(t, []byte("msu"))
},
want: &MessagePublicationAccount{},
errStr: errStringBorsh,
want: &MessagePublicationAccount{},
wantErr: true,
},
{
name: "failure -- truncated data (msg)",
messageAccountData: func(t *testing.T) MessageAccountData {
return mustNewMessageAccountData(t, validMessageAccountDataReliable[:len(validMessageAccountDataReliable)-1])
},
want: &MessagePublicationAccount{},
errStr: errStringBorsh,
want: &MessagePublicationAccount{},
wantErr: true,
},
{
name: "failure -- truncated data (msu)",
messageAccountData: func(t *testing.T) MessageAccountData {
return mustNewMessageAccountData(t, validMessageAccountDataUnreliable[:len(validMessageAccountDataUnreliable)-1])
},
want: &MessagePublicationAccount{},
errStr: errStringBorsh,
want: &MessagePublicationAccount{},
wantErr: true,
},
{
name: "failure -- payload length exceeds remaining data",
messageAccountData: func(t *testing.T) MessageAccountData {
payloadLen := uint32(len(payload) + 1) // #nosec G115 -- Test data, payload length is small.
return mustNewMessageAccountData(t, corruptMessagePublicationPayloadLength(t, validMessageAccountDataReliable, payloadLen))
},
want: &MessagePublicationAccount{},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, gotErr := ParseMessagePublicationAccount(tt.messageAccountData(t))
if tt.errStr != "" {
require.ErrorContains(t, gotErr, tt.errStr)
if tt.wantErr {
require.Error(t, gotErr)
if tt.errStr != "" {
require.ErrorContains(t, gotErr, tt.errStr)
}
return
}

Expand Down Expand Up @@ -657,6 +690,7 @@ func TestProcessAccountSubscriptionData(t *testing.T) {
func TestProcessInstructionEarlyReturns(t *testing.T) {
// Scenario: instruction filtering should return early for non-matching cases.
commitmentMismatchData := encodePostMessageData(t, 42, []byte("hi"), consistencyLevelConfirmed)
payloadLenExceedsData := corruptPostMessagePayloadLength(t, encodePostMessageData(t, 7, []byte("test"), consistencyLevelFinalized), 6)

tests := []struct {
name string
Expand Down Expand Up @@ -685,6 +719,11 @@ func TestProcessInstructionEarlyReturns(t *testing.T) {
inst: solana.CompiledInstruction{ProgramIDIndex: 0, Data: []byte{postMessageInstructionID}, Accounts: make([]uint16, postMessageInstructionMinNumAccounts)},
wantErr: true,
},
{
name: "payload_length_exceeds_data",
inst: solana.CompiledInstruction{ProgramIDIndex: 0, Data: append([]byte{postMessageInstructionID}, payloadLenExceedsData...), Accounts: make([]uint16, postMessageInstructionMinNumAccounts)},
wantErr: true,
},
{
name: "unsupported_consistency_level",
inst: solana.CompiledInstruction{ProgramIDIndex: 0, Data: append([]byte{postMessageInstructionID}, encodePostMessageData(t, 7, []byte("test"), ConsistencyLevel(9))...), Accounts: make([]uint16, postMessageInstructionMinNumAccounts)},
Expand Down Expand Up @@ -1485,6 +1524,15 @@ func encodeMessagePublicationAccount(t *testing.T, prefix string, proposal Messa
return buf.Bytes()
}

func corruptMessagePublicationPayloadLength(t *testing.T, data []byte, payloadLen uint32) []byte {
t.Helper()
const payloadLenOffset = 3 + 1 + 1 + 32 + 1 + 3 + 4 + 4 + 8 + 2 + 32
ret := append([]byte{}, data...)
require.GreaterOrEqual(t, len(ret), payloadLenOffset+4)
binary.LittleEndian.PutUint32(ret[payloadLenOffset:payloadLenOffset+4], payloadLen)
return ret
}

func mustDecodeHex(t *testing.T, s string) []byte {
t.Helper()
b, err := hex.DecodeString(s)
Expand Down Expand Up @@ -1515,6 +1563,15 @@ func encodePostMessageData(t *testing.T, nonce uint32, payload []byte, consisten
return buf.Bytes()
}

func corruptPostMessagePayloadLength(t *testing.T, data []byte, payloadLen uint32) []byte {
t.Helper()
const payloadLenOffset = 4
ret := append([]byte{}, data...)
require.GreaterOrEqual(t, len(ret), payloadLenOffset+4)
binary.LittleEndian.PutUint32(ret[payloadLenOffset:payloadLenOffset+4], payloadLen)
return ret
}

func buildSubscriptionPayload(t *testing.T, owner, pubkey string, accountData []byte) []byte {
t.Helper()
payload := map[string]interface{}{
Expand Down
7 changes: 3 additions & 4 deletions node/pkg/watchers/solana/shim.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ import (
"github.com/certusone/wormhole/node/pkg/watchers"
"github.com/gagliardetto/solana-go"
"github.com/gagliardetto/solana-go/rpc"
"github.com/near/borsh-go"
"go.uber.org/zap"
"go.uber.org/zap/zapcore"
)
Expand Down Expand Up @@ -116,7 +115,7 @@ func shimParsePostMessage(shimPostMessageDiscriminator []byte, buf []byte) (*Shi
}

data := new(ShimPostMessageData)
if err := borsh.Deserialize(data, buf[len(shimPostMessageDiscriminator):]); err != nil {
if err := decodeBorsh(data, buf[len(shimPostMessageDiscriminator):]); err != nil {
return nil, fmt.Errorf("failed to deserialize shim post message: %w", err)
}

Expand All @@ -135,7 +134,7 @@ func shimVerifyCoreMessage(buf []byte) (bool, error) {
}

var data PostMessageData
if err := borsh.Deserialize(&data, buf[1:]); err != nil {
if err := decodeBorsh(&data, buf[1:]); err != nil {
return false, fmt.Errorf("failed to deserialize core instruction data: %w", err)
}

Expand All @@ -153,7 +152,7 @@ func shimParseMessageEvent(shimMessageEventDiscriminator []byte, buf []byte) (*S
}

data := new(ShimMessageEventData)
if err := borsh.Deserialize(data, buf[len(shimMessageEventDiscriminator):]); err != nil {
if err := decodeBorsh(data, buf[len(shimMessageEventDiscriminator):]); err != nil {
return nil, fmt.Errorf("failed to deserialize shim message event: %w", err)
}

Expand Down
6 changes: 3 additions & 3 deletions node/pkg/watchers/solana/shim_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2277,9 +2277,9 @@ func TestShimProcessRestWithoutShimEventShouldFail(t *testing.T) {
require.ErrorContains(t, err, "failed to find inner shim message event instruction")
}

// The core inner instruction has the unreliable PostMessage id (0x08) but its
// trailing bytes can't be borsh-deserialized, so shimVerifyCoreMessage returns
// an error and shimProcessRest wraps it.
// The core inner instruction has the unreliable PostMessage id (0x08), but the
// remaining bytes are truncated, so shimVerifyCoreMessage returns an error and
// shimProcessRest wraps it.
func TestShimProcessRestWithMalformedCoreInstructionShouldFail(t *testing.T) {
var txRpc rpc.TransactionWithMeta
require.NoError(t, json.Unmarshal([]byte(shimDirectEventJSON), &txRpc))
Expand Down
Loading