From dc96c4920a6fc597e65fc91878b5cb28f53e3559 Mon Sep 17 00:00:00 2001 From: Slyghtning Date: Tue, 25 Aug 2026 17:00:28 +0200 Subject: [PATCH] cmd/loop: fix route hint parsing Prevent urfave/cli from splitting route hint arguments on commas so that their JSON remains intact. Limit this behavior to route-hint commands while preserving comma parsing for their other slice flags. Return and strictly decode the hints instead of discarding them through a shadowed local variable. Reject malformed hints at the loopd RPC boundary before invoice creation. --- cmd/loop/loopin.go | 7 +- cmd/loop/quote.go | 11 +- cmd/loop/staticaddr.go | 9 +- cmd/loop/utils.go | 19 ++- cmd/loop/utils_test.go | 161 +++++++++++++++++++++++ docs/release-notes/release-notes-next.md | 4 + loopd/route_hints_test.go | 143 ++++++++++++++++++++ loopd/swapclient_server.go | 42 +++++- 8 files changed, 379 insertions(+), 17 deletions(-) create mode 100644 cmd/loop/utils_test.go create mode 100644 loopd/route_hints_test.go diff --git a/cmd/loop/loopin.go b/cmd/loop/loopin.go index d0512040d..863bbea55 100644 --- a/cmd/loop/loopin.go +++ b/cmd/loop/loopin.go @@ -50,9 +50,10 @@ var ( } loopInCommand = &cli.Command{ - Name: "in", - Usage: "perform an on-chain to off-chain swap (loop in)", - ArgsUsage: "amt", + Name: "in", + Usage: "perform an on-chain to off-chain swap (loop in)", + ArgsUsage: "amt", + DisableSliceFlagSeparator: true, Description: ` Send the amount in satoshis specified by the amt argument off-chain. diff --git a/cmd/loop/quote.go b/cmd/loop/quote.go index 46826eaef..d2501d87d 100644 --- a/cmd/loop/quote.go +++ b/cmd/loop/quote.go @@ -24,9 +24,10 @@ var quoteCommand = &cli.Command{ } var quoteInCommand = &cli.Command{ - Name: "in", - Usage: "get a quote for the cost of a loop in swap", - ArgsUsage: "amt", + Name: "in", + Usage: "get a quote for the cost of a loop in swap", + ArgsUsage: "amt", + DisableSliceFlagSeparator: true, Description: "Allows to determine the cost of a swap up front." + "Either specify an amount or deposit outpoints.", Flags: []cli.Flag{ @@ -86,7 +87,9 @@ func quoteIn(ctx context.Context, cmd *cli.Command) error { } if cmd.IsSet("deposit_outpoint") { - depositOutpoints = cmd.StringSlice("deposit_outpoint") + depositOutpoints = commaSeparatedStringSlice( + cmd, "deposit_outpoint", + ) depositAmt, err = depositAmount(ctx, client, depositOutpoints) if err != nil { return err diff --git a/cmd/loop/staticaddr.go b/cmd/loop/staticaddr.go index c973a0028..c379ffd2b 100644 --- a/cmd/loop/staticaddr.go +++ b/cmd/loop/staticaddr.go @@ -416,9 +416,10 @@ func summary(ctx context.Context, cmd *cli.Command) error { } var staticAddressLoopInCommand = &cli.Command{ - Name: "in", - Usage: "Loop in funds from static address deposits.", - ArgsUsage: "[amt] [--all | --utxo xxx:xx]", + Name: "in", + Usage: "Loop in funds from static address deposits.", + ArgsUsage: "[amt] [--all | --utxo xxx:xx]", + DisableSliceFlagSeparator: true, Description: ` Requests a loop-in swap based on static address deposits. After the creation of a static address funds can be sent to it. Once the funds are @@ -568,7 +569,7 @@ func staticAddressLoopIn(ctx context.Context, cmd *cli.Command) error { depositOutpoints = depositsToOutpoints(allDeposits) case isUtxoSelected: - depositOutpoints = cmd.StringSlice("utxo") + depositOutpoints = commaSeparatedStringSlice(cmd, "utxo") case selectedAmount > 0: // If only an amount is selected, we will trigger coin diff --git a/cmd/loop/utils.go b/cmd/loop/utils.go index 07a6016a6..8c9acdb89 100644 --- a/cmd/loop/utils.go +++ b/cmd/loop/utils.go @@ -2,13 +2,26 @@ package main import ( "context" - "encoding/json" "fmt" + "strings" "github.com/lightninglabs/loop/swapserverrpc" "github.com/urfave/cli/v3" + "google.golang.org/protobuf/encoding/protojson" ) +// commaSeparatedStringSlice restores comma parsing for other string slice +// flags on commands that disable it to preserve route hint JSON. +func commaSeparatedStringSlice(cmd *cli.Command, name string) []string { + values := cmd.StringSlice(name) + result := make([]string, 0, len(values)) + for _, value := range values { + result = append(result, strings.Split(value, ",")...) + } + + return result +} + // showCommandHelp prints help for the current command by delegating to the // parent command when available. This ensures help output renders even when // invoked from inside a subcommand's action. @@ -37,10 +50,10 @@ func validateRouteHints(cmd *cli.Command) ([]*swapserverrpc.RouteHint, error) { jsonHints := cmd.StringSlice(routeHintsFlag.Name) - hints := make([]*swapserverrpc.RouteHint, len(jsonHints)) + hints = make([]*swapserverrpc.RouteHint, len(jsonHints)) for i, jsonHint := range jsonHints { var h swapserverrpc.RouteHint - err := json.Unmarshal([]byte(jsonHint), &h) + err := protojson.Unmarshal([]byte(jsonHint), &h) if err != nil { return nil, fmt.Errorf("unable to parse %d-th "+ "hint json %v: %w", i, jsonHint, err) diff --git a/cmd/loop/utils_test.go b/cmd/loop/utils_test.go new file mode 100644 index 000000000..3353bd092 --- /dev/null +++ b/cmd/loop/utils_test.go @@ -0,0 +1,161 @@ +package main + +import ( + "context" + "testing" + + "github.com/lightninglabs/loop/swapserverrpc" + "github.com/stretchr/testify/require" + "github.com/urfave/cli/v3" +) + +// TestRouteHintsCLIParsing verifies that the real commands preserve repeated +// JSON route hints containing commas and return the decoded values. +func TestRouteHintsCLIParsing(t *testing.T) { + const ( + firstHint = `{"hop_hints":[{"node_id":"node-1",` + + `"chan_id":1,"fee_base_msat":1000,` + + `"fee_proportional_millionths":1,` + + `"cltv_expiry_delta":80}]}` + secondHint = `{"hopHints":[{"nodeId":"node-2",` + + `"chanId":2,"feeBaseMsat":2000,` + + `"feeProportionalMillionths":2,` + + `"cltvExpiryDelta":81}]}` + ) + + testCases := []struct { + name string + path []string + sliceFlag string + }{ + { + name: "loop in", + path: []string{"in"}, + }, + { + name: "quote in", + path: []string{"quote", "in"}, + sliceFlag: "deposit_outpoint", + }, + { + name: "static in", + path: []string{"static", "in"}, + sliceFlag: "utxo", + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + rootCommand := newRootCommandForReplay() + routeHintCommand := commandAtPath( + t, rootCommand, testCase.path, + ) + + var ( + routeHints []*swapserverrpc.RouteHint + sliceValues []string + ) + routeHintCommand.Action = func(_ context.Context, + cmd *cli.Command) error { + + if testCase.sliceFlag != "" { + sliceValues = commaSeparatedStringSlice( + cmd, + testCase.sliceFlag, + ) + } + + var err error + routeHints, err = validateRouteHints(cmd) + + return err + } + + args := append([]string{"loop"}, testCase.path...) + args = append( + args, "--route_hints", firstHint, + "--route_hints", secondHint, + ) + if testCase.sliceFlag != "" { + args = append( + args, "--"+testCase.sliceFlag, + "first:0,second:1", + ) + } + + err := rootCommand.Run(t.Context(), args) + require.NoError(t, err) + require.Len(t, routeHints, 2) + if testCase.sliceFlag != "" { + require.Equal( + t, []string{"first:0", "second:1"}, + sliceValues, + ) + } + + require.Len(t, routeHints[0].HopHints, 1) + require.Equal( + t, "node-1", routeHints[0].HopHints[0].NodeId, + ) + require.Equal( + t, uint64(1), routeHints[0].HopHints[0].ChanId, + ) + require.Equal( + t, uint32(1000), + routeHints[0].HopHints[0].FeeBaseMsat, + ) + require.Equal( + t, uint32(1), routeHints[0].HopHints[0]. + FeeProportionalMillionths, + ) + require.Equal( + t, uint32(80), + routeHints[0].HopHints[0].CltvExpiryDelta, + ) + + require.Len(t, routeHints[1].HopHints, 1) + require.Equal( + t, "node-2", routeHints[1].HopHints[0].NodeId, + ) + require.Equal( + t, uint64(2), routeHints[1].HopHints[0].ChanId, + ) + }) + } +} + +// TestRouteHintsRejectUnknownFields verifies that misspelled protobuf fields +// fail at the CLI boundary instead of producing zero-valued route hints. +func TestRouteHintsRejectUnknownFields(t *testing.T) { + rootCommand := newRootCommandForReplay() + routeHintCommand := commandAtPath(t, rootCommand, []string{"in"}) + routeHintCommand.Action = func(_ context.Context, + cmd *cli.Command) error { + + _, err := validateRouteHints(cmd) + + return err + } + + err := rootCommand.Run(t.Context(), []string{ + "loop", "in", "--route_hints", + `{"hop_hints":[{"node_id":"node-1","chan_id":1,` + + `"cltv_expiry_delat":80}]}`, + }) + require.ErrorContains(t, err, "unknown field") +} + +// commandAtPath returns a command from root by following path. +func commandAtPath(t *testing.T, root *cli.Command, + path []string) *cli.Command { + + t.Helper() + + command := root + for _, name := range path { + command = command.Command(name) + require.NotNil(t, command) + } + + return command +} diff --git a/docs/release-notes/release-notes-next.md b/docs/release-notes/release-notes-next.md index 8cea37ddd..2d7c64919 100644 --- a/docs/release-notes/release-notes-next.md +++ b/docs/release-notes/release-notes-next.md @@ -16,6 +16,10 @@ #### Bug Fixes +* Loop In commands and quotes now correctly parse, validate, and forward + explicit JSON route hints supplied by repeating `--route_hints`. Malformed + or empty hints are rejected before invoice creation. + * Loop Out requests now account for channel reserves when checking outbound capacity, preventing swaps from starting when their off-chain payment cannot be funded. diff --git a/loopd/route_hints_test.go b/loopd/route_hints_test.go new file mode 100644 index 000000000..b26aecb69 --- /dev/null +++ b/loopd/route_hints_test.go @@ -0,0 +1,143 @@ +package loopd + +import ( + "math" + "testing" + + "github.com/lightninglabs/loop/swapserverrpc" + mock_lnd "github.com/lightninglabs/loop/test" + "github.com/stretchr/testify/require" +) + +// TestUnmarshallRouteHintsValidation verifies malformed RPC route hints are +// rejected before they reach invoice creation. +func TestUnmarshallRouteHintsValidation(t *testing.T) { + validHop := func() *swapserverrpc.HopHint { + return &swapserverrpc.HopHint{ + NodeId: mock_lnd.NewMockLnd().NodePubkey, + ChanId: 1, + CltvExpiryDelta: 80, + } + } + + testCases := []struct { + name string + routeHints []*swapserverrpc.RouteHint + errContains string + }{ + { + name: "valid", + routeHints: []*swapserverrpc.RouteHint{ + { + HopHints: []*swapserverrpc.HopHint{ + validHop(), + }, + }, + }, + }, + { + name: "nil route hint", + routeHints: []*swapserverrpc.RouteHint{ + nil, + }, + errContains: "route hint 0 is nil", + }, + { + name: "empty route hint", + routeHints: []*swapserverrpc.RouteHint{ + {}, + }, + errContains: "route hint 0 has no hop hints", + }, + { + name: "nil hop hint", + routeHints: []*swapserverrpc.RouteHint{ + { + HopHints: []*swapserverrpc.HopHint{nil}, + }, + }, + errContains: "hop hint 0 in route hint 0 is nil", + }, + { + name: "invalid node ID", + routeHints: []*swapserverrpc.RouteHint{ + { + HopHints: []*swapserverrpc.HopHint{ + { + NodeId: "invalid", + ChanId: 1, + CltvExpiryDelta: 80, + }, + }, + }, + }, + errContains: "invalid hop hint 0 in route hint 0", + }, + { + name: "zero channel ID", + routeHints: []*swapserverrpc.RouteHint{ + { + HopHints: []*swapserverrpc.HopHint{ + { + NodeId: mock_lnd.NewMockLnd().NodePubkey, + CltvExpiryDelta: 80, + }, + }, + }, + }, + errContains: "hop hint 0 in route hint 0 has zero " + + "channel ID", + }, + { + name: "zero CLTV delta", + routeHints: []*swapserverrpc.RouteHint{ + { + HopHints: []*swapserverrpc.HopHint{ + { + NodeId: mock_lnd.NewMockLnd().NodePubkey, + ChanId: 1, + }, + }, + }, + }, + errContains: "hop hint 0 in route hint 0 has zero " + + "CLTV expiry delta", + }, + { + name: "CLTV delta overflow", + routeHints: []*swapserverrpc.RouteHint{ + { + HopHints: []*swapserverrpc.HopHint{ + { + NodeId: mock_lnd.NewMockLnd().NodePubkey, + ChanId: 1, + CltvExpiryDelta: math.MaxUint16 + 1, + }, + }, + }, + }, + errContains: "CLTV expiry delta exceeds 65535", + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + routeHints, err := unmarshallRouteHints( + testCase.routeHints, + ) + if testCase.errContains != "" { + require.ErrorContains(t, err, testCase.errContains) + + return + } + + require.NoError(t, err) + require.Len(t, routeHints, 1) + require.Len(t, routeHints[0], 1) + require.Equal(t, uint64(1), routeHints[0][0].ChannelID) + require.Equal( + t, uint16(80), routeHints[0][0].CLTVExpiryDelta, + ) + }) + } +} diff --git a/loopd/swapclient_server.go b/loopd/swapclient_server.go index 72c3d7dd7..d099caa11 100644 --- a/loopd/swapclient_server.go +++ b/loopd/swapclient_server.go @@ -7,6 +7,7 @@ import ( "encoding/hex" "errors" "fmt" + "math" "reflect" "slices" "sort" @@ -1244,14 +1245,49 @@ func unmarshallRouteHints(rpcRouteHints []*swapserverrpc.RouteHint) ( [][]zpay32.HopHint, error) { routeHints := make([][]zpay32.HopHint, 0, len(rpcRouteHints)) - for _, rpcRouteHint := range rpcRouteHints { + for routeIndex, rpcRouteHint := range rpcRouteHints { + if rpcRouteHint == nil { + return nil, fmt.Errorf("route hint %d is nil", routeIndex) + } + + if len(rpcRouteHint.HopHints) == 0 { + return nil, fmt.Errorf( + "route hint %d has no hop hints", routeIndex, + ) + } + routeHint := make( []zpay32.HopHint, 0, len(rpcRouteHint.HopHints), ) - for _, rpcHint := range rpcRouteHint.HopHints { + for hopIndex, rpcHint := range rpcRouteHint.HopHints { + if rpcHint == nil { + return nil, fmt.Errorf("hop hint %d in route hint "+ + "%d is nil", hopIndex, routeIndex) + } + + if rpcHint.ChanId == 0 { + return nil, fmt.Errorf("hop hint %d in route hint "+ + "%d has zero channel ID", hopIndex, + routeIndex) + } + + if rpcHint.CltvExpiryDelta == 0 { + return nil, fmt.Errorf("hop hint %d in route hint "+ + "%d has zero CLTV expiry delta", hopIndex, + routeIndex) + } + + if rpcHint.CltvExpiryDelta > math.MaxUint16 { + return nil, fmt.Errorf("hop hint %d in route hint "+ + "%d CLTV expiry delta exceeds %d", hopIndex, + routeIndex, uint32(math.MaxUint16)) + } + hint, err := unmarshallHopHint(rpcHint) if err != nil { - return nil, err + return nil, fmt.Errorf("invalid hop hint %d in "+ + "route hint %d: %w", hopIndex, + routeIndex, err) } routeHint = append(routeHint, hint)