R4R: validate sign tx request's body (#3179)

* validate sign tx request's body

Closes: #3176

* Introduce CodeNoSignatures

* Update swagger.yaml

* Fix tests

* Update x/auth/client/rest/sign.go

Co-Authored-By: alessio <quadrispro@ubuntu.com>

PENDING.md

@@ -17,6 +17,9 @@ BREAKING CHANGES
   * [\#3162](https://github.com/cosmos/cosmos-sdk/issues/3162) The `--gas` flag now takes `auto` instead of `simulate`
     in order to trigger a simulation of the tx before the actual execution.
 
+* Gaia REST API
+  * [\#3176](https://github.com/cosmos/cosmos-sdk/issues/3176) `tx/sign` endpoint now expects `BaseReq` fields as nested object.
+
 * SDK
   * [stake] \#2513 Validator power type from Dec -> Int 
   * [\#3064](https://github.com/cosmos/cosmos-sdk/issues/3064) Sanitize `sdk.Coin` denom. Coins denoms are now case insensitive, i.e. 100fooToken equals to 100FOOTOKEN.
@@ -44,13 +47,16 @@ FEATURES
 * SDK
   * \#2996 Update the `AccountKeeper` to contain params used in the context of
   the ante handler.
+  * [\#3179](https://github.com/cosmos/cosmos-sdk/pull/3179) New CodeNoSignatures error code.
+
 
 * Tendermint
 
 
 IMPROVEMENTS
 
-* Gaia REST API (`gaiacli advanced rest-server`)
+* Gaia REST API
+  * [\#3176](https://github.com/cosmos/cosmos-sdk/issues/3176) Validate tx/sign endpoint POST body.
 
 * Gaia CLI  (`gaiacli`)
 
@@ -74,7 +80,7 @@ IMPROVEMENTS
 
 BUG FIXES
 
-* Gaia REST API (`gaiacli advanced rest-server`)
+* Gaia REST API
 
 * Gaia CLI  (`gaiacli`)
   * \#3141 Fix the bug in GetAccount when `len(res) == 0` and `err == nil`

client/lcd/lcd_test.go

@@ -17,6 +17,7 @@ import (
 
 	client "github.com/cosmos/cosmos-sdk/client"
 	"github.com/cosmos/cosmos-sdk/client/tx"
+	"github.com/cosmos/cosmos-sdk/client/utils"
 	"github.com/cosmos/cosmos-sdk/cmd/gaia/app"
 
 	"github.com/cosmos/cosmos-sdk/crypto/keys/mintkey"
@@ -259,12 +260,10 @@ func TestCoinSendGenerateSignAndBroadcast(t *testing.T) {
 	var signedMsg auth.StdTx
 
 	payload := authrest.SignBody{
-		Tx:               msg,
-		LocalAccountName: name1,
-		Password:         pw,
-		ChainID:          viper.GetString(client.FlagChainID),
-		AccountNumber:    accnum,
-		Sequence:         sequence,
+		Tx: msg,
+		BaseReq: utils.NewBaseReq(
+			name1, pw, "", viper.GetString(client.FlagChainID), "", "", accnum, sequence, nil, false, false,
+		),
 	}
 	json, err := cdc.MarshalJSON(payload)
 	require.Nil(t, err)

client/lcd/swagger-ui/swagger.yaml

@@ -286,20 +286,10 @@ paths:
         schema:
           type: object
           properties:
+            base_req:
+              $ref: "#/definitions/BaseReq"
             tx:
               $ref: "#/definitions/StdTx"
-            name:
-              type: string
-            password:
-              type: string
-            chain_id:
-              type: string
-            account_number:
-              type: string
-              example: "0"
-            sequence:
-              type: string
-              example: "0"
             append_sig:
               type: boolean
               example: true

client/lcd/test_helpers.go

@@ -645,12 +645,10 @@ func getAccount(t *testing.T, port string, addr sdk.AccAddress) auth.Account {
 func doSign(t *testing.T, port, name, password, chainID string, accnum, sequence uint64, msg auth.StdTx) auth.StdTx {
 	var signedMsg auth.StdTx
 	payload := authrest.SignBody{
-		Tx:               msg,
-		LocalAccountName: name,
-		Password:         password,
-		ChainID:          chainID,
-		AccountNumber:    accnum,
-		Sequence:         sequence,
+		Tx: msg,
+		BaseReq: utils.NewBaseReq(
+			name, password, "", chainID, "", "", accnum, sequence, nil, false, false,
+		),
 	}
 	json, err := cdc.MarshalJSON(payload)
 	require.Nil(t, err)

client/utils/rest.go

@@ -189,7 +189,7 @@ func ReadRESTReq(w http.ResponseWriter, r *http.Request, cdc *codec.Codec, req i
 
 	err = cdc.UnmarshalJSON(body, req)
 	if err != nil {
-		WriteErrorResponse(w, http.StatusBadRequest, err.Error())
+		WriteErrorResponse(w, http.StatusBadRequest, fmt.Sprintf("failed to decode JSON payload: %s", err))
 		return err
 	}
 

types/errors.go

@@ -45,6 +45,7 @@ const (
 	CodeInsufficientFee   CodeType = 14
 	CodeTooManySignatures CodeType = 15
 	CodeGasOverflow       CodeType = 16
+	CodeNoSignatures      CodeType = 17
 
 	// CodespaceRoot is a codespace for error codes in this file only.
 	// Notice that 0 is an "unset" codespace, which can be overridden with
@@ -90,6 +91,8 @@ func CodeToDefaultMsg(code CodeType) string {
 		return "insufficient fee"
 	case CodeTooManySignatures:
 		return "maximum numer of signatures exceeded"
+	case CodeNoSignatures:
+		return "no signatures supplied"
 	default:
 		return unknownCodeMsg(code)
 	}
@@ -145,6 +148,9 @@ func ErrInsufficientFee(msg string) Error {
 func ErrTooManySignatures(msg string) Error {
 	return newErrorWithRootCodespace(CodeTooManySignatures, msg)
 }
+func ErrNoSignatures(msg string) Error {
+	return newErrorWithRootCodespace(CodeNoSignatures, msg)
+}
 func ErrGasOverflow(msg string) Error {
 	return newErrorWithRootCodespace(CodeGasOverflow, msg)
 }

x/auth/ante_test.go

@@ -71,7 +71,7 @@ func TestAnteHandlerSigErrors(t *testing.T) {
 	require.Equal(t, expectedSigners, stdTx.GetSigners())
 
 	// Check no signatures fails
-	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeUnauthorized)
+	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeNoSignatures)
 
 	// test num sigs dont match GetSigners
 	privs, accNums, seqs = []crypto.PrivKey{priv1}, []uint64{0}, []uint64{0}

x/auth/client/rest/sign.go

@@ -1,26 +1,22 @@
 package rest
 
 import (
-	"io/ioutil"
 	"net/http"
 
 	"github.com/cosmos/cosmos-sdk/client/context"
 	"github.com/cosmos/cosmos-sdk/client/utils"
 	"github.com/cosmos/cosmos-sdk/codec"
 	"github.com/cosmos/cosmos-sdk/crypto/keys/keyerror"
+	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/x/auth"
 	authtxb "github.com/cosmos/cosmos-sdk/x/auth/client/txbuilder"
 )
 
 // SignBody defines the properties of a sign request's body.
 type SignBody struct {
-	Tx               auth.StdTx `json:"tx"`
-	LocalAccountName string     `json:"name"`
-	Password         string     `json:"password"`
-	ChainID          string     `json:"chain_id"`
-	AccountNumber    uint64     `json:"account_number"`
-	Sequence         uint64     `json:"sequence"`
-	AppendSig        bool       `json:"append_sig"`
+	Tx        auth.StdTx    `json:"tx"`
+	AppendSig bool          `json:"append_sig"`
+	BaseReq   utils.BaseReq `json:"base_req"`
 }
 
 // nolint: unparam
@@ -30,21 +26,34 @@ func SignTxRequestHandlerFn(cdc *codec.Codec, cliCtx context.CLIContext) http.Ha
 	return func(w http.ResponseWriter, r *http.Request) {
 		var m SignBody
 
-		body, err := ioutil.ReadAll(r.Body)
-		if err != nil {
+		if err := utils.ReadRESTReq(w, r, cdc, &m); err != nil {
 			utils.WriteErrorResponse(w, http.StatusBadRequest, err.Error())
 			return
 		}
-		err = cdc.UnmarshalJSON(body, &m)
-		if err != nil {
+
+		if !m.BaseReq.ValidateBasic(w) {
+			return
+		}
+
+		// validate tx
+ 		// discard error if it's CodeNoSignatures as the tx comes with no signatures
+		if err := m.Tx.ValidateBasic(); err != nil && err.Code() != sdk.CodeNoSignatures {
 			utils.WriteErrorResponse(w, http.StatusBadRequest, err.Error())
 			return
 		}
 
-		txBldr := authtxb.NewTxBuilder(utils.GetTxEncoder(cdc), m.AccountNumber,
-			m.Sequence, m.Tx.Fee.Gas, 1.0, false, m.ChainID, m.Tx.GetMemo(), m.Tx.Fee.Amount)
+		txBldr := authtxb.NewTxBuilder(
+			utils.GetTxEncoder(cdc),
+			m.BaseReq.AccountNumber,
+			m.BaseReq.Sequence,
+			m.Tx.Fee.Gas,
+			1.0,
+			false,
+			m.BaseReq.ChainID,
+			m.Tx.GetMemo(),
+			m.Tx.Fee.Amount)
 
-		signedTx, err := txBldr.SignStdTx(m.LocalAccountName, m.Password, m.Tx, m.AppendSig)
+		signedTx, err := txBldr.SignStdTx(m.BaseReq.Name, m.BaseReq.Password, m.Tx, m.AppendSig)
 		if keyerror.IsErrKeyNotFound(err) {
 			utils.WriteErrorResponse(w, http.StatusBadRequest, err.Error())
 			return

x/auth/stdtx.go

@@ -50,7 +50,7 @@ func (tx StdTx) ValidateBasic() sdk.Error {
 		return sdk.ErrInsufficientFee(fmt.Sprintf("invalid fee %s amount provided", tx.Fee.Amount))
 	}
 	if len(stdSigs) == 0 {
-		return sdk.ErrUnauthorized("no signers")
+		return sdk.ErrNoSignatures("no signers")
 	}
 	if len(stdSigs) != len(tx.GetSigners()) {
 		return sdk.ErrUnauthorized("wrong number of signers")

x/auth/stdtx_test.go

@@ -91,7 +91,7 @@ func TestTxValidateBasic(t *testing.T) {
 
 	err = tx.ValidateBasic()
 	require.Error(t, err)
-	require.Equal(t, sdk.CodeUnauthorized, err.Result().Code)
+	require.Equal(t, sdk.CodeNoSignatures, err.Result().Code)
 
 	// require to fail validation when signatures do not match expected signers
 	privs, accNums, seqs = []crypto.PrivKey{priv1}, []uint64{0, 1}, []uint64{0, 0}