Merge pull request #196 from dashevo/lklimek/fix-light-client-panic

fix(light client): panic when empty commit is received from server

light/provider/http/http.go

@@ -80,12 +80,23 @@ func (p *http) LightBlock(ctx context.Context, height int64) (*types.LightBlock,
 		return nil, err
 	}
 
+	if sh.Commit == nil {
+		return nil, provider.ErrBadLightBlock{
+			Reason: fmt.Errorf("height %d responded with nil commit: %+v", height, sh),
+		}
+	}
+
+	if sh.Header == nil {
+		return nil, provider.ErrBadLightBlock{
+			Reason: fmt.Errorf("height %d responded with nil header: %+v", height, sh),
+		}
+	}
+
 	if height != 0 && sh.Height != height {
 		return nil, provider.ErrBadLightBlock{
 			Reason: fmt.Errorf("height %d responded doesn't match height %d requested", sh.Height, height),
 		}
 	}
-
 	vs, err := p.validatorSet(ctx, &sh.Height)
 	if err != nil {
 		return nil, err
@@ -196,10 +207,6 @@ func (p *http) signedHeader(ctx context.Context, height *int64) (*types.SignedHe
 			time.Sleep(backoffTimeout(uint16(attempt)))
 			continue
 		}
-		if commit == nil {
-			time.Sleep(backoffTimeout(uint16(attempt)))
-			continue
-		}
 		return &commit.SignedHeader, nil
 	}
 	return nil, provider.ErrNoResponse

light/provider/http/http_test.go

@@ -13,7 +13,11 @@ import (
 	"github.com/tendermint/tendermint/abci/example/kvstore"
 	"github.com/tendermint/tendermint/light/provider"
 	rpcclient "github.com/tendermint/tendermint/rpc/client"
+	rpcmock "github.com/tendermint/tendermint/rpc/client/mocks"
+
+	testify "github.com/stretchr/testify/mock"
 	rpchttp "github.com/tendermint/tendermint/rpc/client/http"
+	coretypes "github.com/tendermint/tendermint/rpc/core/types"
 	rpctest "github.com/tendermint/tendermint/rpc/test"
 	"github.com/tendermint/tendermint/types"
 )
@@ -89,3 +93,41 @@ func TestProvider(t *testing.T) {
 	require.Error(t, err)
 	assert.Equal(t, provider.ErrLightBlockNotFound, err)
 }
+
+// TestLightClient_NilCommit ensures correct handling of a case where commit returned by http client is nil
+func TestLightClient_NilCommit(t *testing.T) {
+	chainID := "none"
+	c := &rpcmock.RemoteClient{}
+	p := lighthttp.NewWithClient(chainID, c)
+	require.NotNil(t, p)
+
+	c.On("Commit", testify.Anything, testify.Anything).Return(
+		&coretypes.ResultCommit{
+			SignedHeader: types.SignedHeader{
+				Header: &types.Header{},
+				Commit: nil,
+			}}, nil)
+
+	sh, err := p.LightBlock(context.Background(), 0)
+	require.Error(t, err)
+	require.Nil(t, sh)
+}
+
+// TestLightClient_NilCommit ensures correct handling of a case where header returned by http client is nil
+func TestLightClient_NilHeader(t *testing.T) {
+	chainID := "none"
+	c := &rpcmock.RemoteClient{}
+	p := lighthttp.NewWithClient(chainID, c)
+	require.NotNil(t, p)
+
+	c.On("Commit", testify.Anything, testify.Anything).Return(
+		&coretypes.ResultCommit{
+			SignedHeader: types.SignedHeader{
+				Header: nil,
+				Commit: &types.Commit{},
+			}}, nil)
+
+	sh, err := p.LightBlock(context.Background(), 0)
+	require.Error(t, err)
+	require.Nil(t, sh)
+}

rpc/client/http/http.go

@@ -3,6 +3,7 @@ package http
 import (
 	"context"
 	"errors"
+	"fmt"
 	"net/http"
 	"strings"
 	"time"
@@ -445,6 +446,12 @@ func (c *baseRPCClient) Commit(ctx context.Context, height *int64) (*ctypes.Resu
 	if err != nil {
 		return nil, err
 	}
+	if result.Commit == nil {
+		return nil, fmt.Errorf("cannot fetch commit: commit is nil: %+v", result)
+	}
+	if result.Header == nil {
+		return nil, fmt.Errorf("cannot fetch commit: header is nil: %+v", result)
+	}
 	return result, nil
 }