certusone
/
cosmos-sdk
mirror of https://github.com/certusone/cosmos-sdk.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

x/ibc: alter id requirements (#6227) 

* x/ibc: alter id requirements

* add regex to validations

* comment

* update error

* test all validators:

* fix tests

* check for only separators
This commit is contained in:
Federico Kunze 2020-05-15 13:24:31 -04:00 committed by GitHub
parent 34d9ed52b3
commit b2ad4d2a23
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
18 changed files with 196 additions and 58 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
baseapp/helpers.go
View File

@ -1,15 +1,11 @@
package baseapp
import (
"regexp"
abci "github.com/tendermint/tendermint/abci/types"
sdk "github.com/cosmos/cosmos-sdk/types"
)
var isAlphaNumeric = regexp.MustCompile(`^[a-zA-Z0-9]+$`).MatchString
func (app *BaseApp) Check(tx sdk.Tx) (sdk.GasInfo, *sdk.Result, error) {
return app.runTx(runTxModeCheck, nil, tx)
}

2
baseapp/queryrouter.go
View File

@ -22,7 +22,7 @@ func NewQueryRouter() *QueryRouter {
// AddRoute adds a query path to the router with a given Querier. It will panic
// if a duplicate route is given. The route must be alphanumeric.
func (qrt *QueryRouter) AddRoute(path string, q sdk.Querier) sdk.QueryRouter {
if !isAlphaNumeric(path) {
if !sdk.IsAlphaNumeric(path) {
panic("route expressions can only contain alphanumeric characters")
}

2
baseapp/router.go
View File

@ -22,7 +22,7 @@ func NewRouter() *Router {
// AddRoute adds a route path to the router with a given handler. The route must
// be alphanumeric.
func (rtr *Router) AddRoute(path string, h sdk.Handler) sdk.Router {
if !isAlphaNumeric(path) {
if !sdk.IsAlphaNumeric(path) {
panic("route expressions can only contain alphanumeric characters")
}
if rtr.routes[path] != nil {

16
x/ibc/03-connection/types/connection.go
View File

@ -98,23 +98,13 @@ func (c Counterparty) GetPrefix() commitmentexported.Prefix {
// ValidateBasic performs a basic validation check of the identifiers and prefix
func (c Counterparty) ValidateBasic() error {
if err := host.ConnectionIdentifierValidator(c.ConnectionID); err != nil {
return sdkerrors.Wrap(err,
sdkerrors.Wrapf(
ErrInvalidCounterparty,
"invalid counterparty connection ID %s", c.ConnectionID,
).Error(),
)
return sdkerrors.Wrap(err, "invalid counterparty connection ID")
}
if err := host.ClientIdentifierValidator(c.ClientID); err != nil {
return sdkerrors.Wrap(err,
sdkerrors.Wrapf(
ErrInvalidCounterparty,
"invalid counterparty client ID %s", c.ClientID,
).Error(),
)
return sdkerrors.Wrap(err, "invalid counterparty client ID")
}
if c.Prefix.IsEmpty() {
return sdkerrors.Wrap(ErrInvalidCounterparty, "invalid counterparty prefix")
return sdkerrors.Wrap(ErrInvalidCounterparty, "counterparty prefix cannot be empty")
}
return nil
}

8
x/ibc/03-connection/types/connection_test.go
View File

@ -28,12 +28,12 @@ func TestConnectionValidateBasic(t *testing.T) {
},
{
"invalid connection id",
ConnectionEnd{"connectionIDONE", clientID, []string{"1.0.0"}, INIT, Counterparty{clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}},
ConnectionEnd{"(connectionIDONE)", clientID, []string{"1.0.0"}, INIT, Counterparty{clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}},
false,
},
{
"invalid client id",
ConnectionEnd{connectionID, "clientID1", []string{"1.0.0"}, INIT, Counterparty{clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}},
ConnectionEnd{connectionID, "(clientID1)", []string{"1.0.0"}, INIT, Counterparty{clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}},
false,
},
{
@ -72,8 +72,8 @@ func TestCounterpartyValidateBasic(t *testing.T) {
expPass bool
}{
{"valid counterparty", Counterparty{clientID, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}, true},
{"invalid client id", Counterparty{"InvalidClient", "channelidone", commitmenttypes.NewMerklePrefix([]byte("prefix"))}, false},
{"invalid connection id", Counterparty{clientID, "InvalidConnection", commitmenttypes.NewMerklePrefix([]byte("prefix"))}, false},
{"invalid client id", Counterparty{"(InvalidClient)", connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}, false},
{"invalid connection id", Counterparty{clientID, "(InvalidConnection)", commitmenttypes.NewMerklePrefix([]byte("prefix"))}, false},
{"invalid prefix", Counterparty{clientID, connectionID2, emptyPrefix}, false},
}

4
x/ibc/03-connection/types/genesis_test.go
View File

@ -37,7 +37,7 @@ func TestValidateGenesis(t *testing.T) {
name: "invalid connection",
genState: NewGenesisState(
[]ConnectionEnd{
NewConnectionEnd(INIT, connectionID, "CLIENTIDONE", Counterparty{clientID, connectionID, commitmenttypes.NewMerklePrefix([]byte("prefix"))}, []string{"1.0.0"}),
NewConnectionEnd(INIT, connectionID, "(CLIENTIDONE)", Counterparty{clientID, connectionID, commitmenttypes.NewMerklePrefix([]byte("prefix"))}, []string{"1.0.0"}),
},
[]ConnectionPaths{
{clientID, []string{host.ConnectionPath(connectionID)}},
@ -52,7 +52,7 @@ func TestValidateGenesis(t *testing.T) {
NewConnectionEnd(INIT, connectionID, clientID, Counterparty{clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}, []string{"1.0.0"}),
},
[]ConnectionPaths{
{"CLIENTIDONE", []string{host.ConnectionPath(connectionID)}},
{"(CLIENTIDONE)", []string{host.ConnectionPath(connectionID)}},
},
),
expPass: false,

4
x/ibc/04-channel/types/channel_test.go
View File

@ -13,8 +13,8 @@ func TestCounterpartyValidateBasic(t *testing.T) {
expPass bool
}{
{"valid counterparty", Counterparty{"portidone", "channelidone"}, true},
{"invalid port id", Counterparty{"InvalidPort", "channelidone"}, false},
{"invalid channel id", Counterparty{"portidone", "InvalidChannel"}, false},
{"invalid port id", Counterparty{"(InvalidPort)", "channelidone"}, false},
{"invalid channel id", Counterparty{"portidone", "(InvalidChannel)"}, false},
}
for i, tc := range testCases {

6
x/ibc/04-channel/types/genesis_test.go
View File

@ -66,7 +66,7 @@ func TestValidateGenesis(t *testing.T) {
genState: GenesisState{
Channels: []IdentifiedChannel{
NewIdentifiedChannel(
testPort1, "testChannel1", NewChannel(
testPort1, "(testChannel1)", NewChannel(
INIT, testChannelOrder, counterparty2, []string{testConnectionIDA}, testChannelVersion,
),
),
@ -105,7 +105,7 @@ func TestValidateGenesis(t *testing.T) {
name: "invalid recv seq",
genState: GenesisState{
RecvSequences: []PacketSequence{
NewPacketSequence(testPort1, "testChannel1", 1),
NewPacketSequence(testPort1, "(testChannel1)", 1),
},
},
expPass: false,
@ -114,7 +114,7 @@ func TestValidateGenesis(t *testing.T) {
name: "invalid recv seq 2",
genState: GenesisState{
RecvSequences: []PacketSequence{
NewPacketSequence("testPort1", testChannel1, 1),
NewPacketSequence("(testPort1)", testChannel1, 1),
},
},
expPass: false,

6
x/ibc/04-channel/types/msgs_test.go
View File

@ -21,15 +21,15 @@ import (
// define constants used for testing
const (
invalidPort = "invalidport1"
invalidPort = "(invalidport1)"
invalidShortPort = "p"
invalidLongPort = "invalidlongportinvalidlongport"
invalidChannel = "invalidchannel1"
invalidChannel = "(invalidchannel1)"
invalidShortChannel = "invalidch"
invalidLongChannel = "invalidlongchannelinvalidlongchannel"
invalidConnection = "invalidconnection1"
invalidConnection = "(invalidconnection1)"
invalidShortConnection = "invalidcn"
invalidLongConnection = "invalidlongconnection"
)

2
x/ibc/05-port/keeper/keeper_test.go
View File

@ -15,7 +15,7 @@ import (
var (
validPort = "validportid"
invalidPort = "invalidPortID"
invalidPort = "(invalidPortID)"
)
type KeeperTestSuite struct {

2
x/ibc/07-tendermint/types/client_state_test.go
View File

@ -32,7 +32,7 @@ func (suite *TendermintTestSuite) TestValidate() {
},
{
name: "invalid client id",
clientState: ibctmtypes.NewClientState("testClientID", lite.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, suite.header),
clientState: ibctmtypes.NewClientState("(testClientID)", lite.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, suite.header),
expPass: false,
},
{

4
x/ibc/07-tendermint/types/msgs_test.go
View File

@ -23,7 +23,7 @@ func (suite *TendermintTestSuite) TestMsgCreateClientValidateBasic() {
errMsg string
}{
{ibctmtypes.NewMsgCreateClient(exported.ClientTypeTendermint, suite.header, lite.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, signer), true, "success msg should pass"},
{ibctmtypes.NewMsgCreateClient("BADCHAIN", suite.header, lite.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, signer), false, "invalid client id passed"},
{ibctmtypes.NewMsgCreateClient("(BADCHAIN)", suite.header, lite.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, signer), false, "invalid client id passed"},
{ibctmtypes.NewMsgCreateClient(exported.ClientTypeTendermint, suite.header, math.Fraction{Numerator: 0, Denominator: 1}, trustingPeriod, ubdPeriod, maxClockDrift, signer), false, "invalid trust level"},
{ibctmtypes.NewMsgCreateClient(exported.ClientTypeTendermint, suite.header, lite.DefaultTrustLevel, 0, ubdPeriod, maxClockDrift, signer), false, "zero trusting period passed"},
{ibctmtypes.NewMsgCreateClient(exported.ClientTypeTendermint, suite.header, lite.DefaultTrustLevel, trustingPeriod, 0, maxClockDrift, signer), false, "zero unbonding period passed"},
@ -52,7 +52,7 @@ func (suite *TendermintTestSuite) TestMsgUpdateClient() {
errMsg string
}{
{ibctmtypes.NewMsgUpdateClient(exported.ClientTypeTendermint, ibctmtypes.Header{}, signer), true, "success msg should pass"},
{ibctmtypes.NewMsgUpdateClient("badClient", ibctmtypes.Header{}, signer), false, "invalid client id passed"},
{ibctmtypes.NewMsgUpdateClient("(badClient)", ibctmtypes.Header{}, signer), false, "invalid client id passed"},
{ibctmtypes.NewMsgUpdateClient(exported.ClientTypeTendermint, ibctmtypes.Header{}, nil), false, "Empty address passed"},
}

2
x/ibc/20-transfer/types/genesis_test.go
View File

@ -29,7 +29,7 @@ func TestValidateGenesis(t *testing.T) {
{
"invalid client",
types.GenesisState{
PortID: "INVALIDPORT",
PortID: "(INVALIDPORT)",
},
false,
},

4
x/ibc/20-transfer/types/msgs_test.go
View File

@ -12,12 +12,12 @@ import (
// define constants used for testing
const (
validPort = "testportid"
invalidPort = "invalidport1"
invalidPort = "(invalidport1)"
invalidShortPort = "p"
invalidLongPort = "invalidlongportinvalidlongport"
validChannel = "testchannel"
invalidChannel = "invalidchannel1"
invalidChannel = "(invalidchannel1)"
invalidShortChannel = "invalidch"
invalidLongChannel = "invalidlongchannelinvalidlongchannel"
)

6
x/ibc/23-commitment/types/merkle_test.go
View File

@ -125,13 +125,13 @@ func (suite *MerkleTestSuite) TestVerifyNonMembership() {
func TestApplyPrefix(t *testing.T) {
prefix := types.NewMerklePrefix([]byte("storePrefixKey"))
pathStr := "path1/path2/path3/key"
pathStr := "pathone/pathtwo/paththree/key"
prefixedPath, err := types.ApplyPrefix(prefix, pathStr)
require.Nil(t, err, "valid prefix returns error")
require.Equal(t, "/storePrefixKey/path1/path2/path3/key", prefixedPath.Pretty(), "Prefixed path incorrect")
require.Equal(t, "/storePrefixKey/path1%2Fpath2%2Fpath3%2Fkey", prefixedPath.String(), "Prefixed scaped path incorrect")
require.Equal(t, "/storePrefixKey/"+pathStr, prefixedPath.Pretty(), "Prefixed path incorrect")
require.Equal(t, "/storePrefixKey/pathone%2Fpathtwo%2Fpaththree%2Fkey", prefixedPath.String(), "Prefixed scaped path incorrect")
// invalid prefix contains non-alphanumeric character
invalidPathStr := "invalid-path/doesitfail?/hopefully"

70
x/ibc/24-host/validate.go
View File

@ -1,12 +1,20 @@
package host
import (
"fmt"
"regexp"
"strings"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
)
// IsValidID defines regular expression to check if the string consist of
// characters in one of the following categories only:
// - Alphanumeric
// - `.`, `_`, `+`, `-`, `#`
// - `[`, `]`, `<`, `>`
var IsValidID = regexp.MustCompile(`^[a-zA-Z0-9\.\_\+\-\#\[\]\<\>]+$`).MatchString
// ICS 024 Identifier and Path Validation Implementation
//
// This file defines ValidateFn to validate identifier and path strings
@ -17,6 +25,9 @@ import (
type ValidateFn func(string) error
func defaultIdentifierValidator(id string, min, max int) error { //nolint:unparam
if strings.TrimSpace(id) == "" {
return sdkerrors.Wrap(ErrInvalidID, "identifier cannot be blank")
}
// valid id MUST NOT contain "/" separator
if strings.Contains(id, "/") {
return sdkerrors.Wrapf(ErrInvalidID, "identifier %s cannot contain separator '/'", id)
@ -26,8 +37,12 @@ func defaultIdentifierValidator(id string, min, max int) error { //nolint:unpara
return sdkerrors.Wrapf(ErrInvalidID, "identifier %s has invalid length: %d, must be between %d-%d characters", id, len(id), min, max)
}
// valid id must contain only lower alphabetic characters
if !sdk.IsAlphaLower(id) {
return sdkerrors.Wrapf(ErrInvalidID, "identifier %s must contain only lowercase alphabetic characters", id)
if !IsValidID(id) {
return sdkerrors.Wrapf(
ErrInvalidID,
"identifier %s must contain only alphanumeric or the following characters: '.', '_', '+', '-', '#', '[', ']', '<', '>'",
id,
)
}
return nil
}
@ -66,30 +81,61 @@ func PortIdentifierValidator(id string) error {
func NewPathValidator(idValidator ValidateFn) ValidateFn {
return func(path string) error {
pathArr := strings.Split(path, "/")
if len(pathArr) > 0 && pathArr[0] == path {
return sdkerrors.Wrapf(ErrInvalidPath, "path %s doesn't contain any separator '/'", path)
}
allEmptyItems := true
for _, p := range pathArr {
// Each path element must either be valid identifier or alphanumeric
err := idValidator(p)
if err != nil && !sdk.IsAlphaNumeric(p) {
return sdkerrors.Wrapf(ErrInvalidPath, "path %s contains invalid identifier or non-alphanumeric path element: %s", path, p)
// NOTE: for some reason an empty string is added after Split
if p == "" {
continue
}
allEmptyItems = false
if err := idValidator(p); err != nil {
return err
}
// Each path element must either be a valid identifier or constant number
if err := defaultIdentifierValidator(p, 1, 20); err != nil {
return sdkerrors.Wrapf(err, "path %s contains an invalid identifier: '%s'", path, p)
}
}
if allEmptyItems {
return fmt.Errorf("path %s must contain at least one identifier", path)
}
return nil
}
}
// PathValidator takes in path string and validateswith def ault identifier rules.
// PathValidator takes in path string and validates with default identifier rules.
// This is optimized by simply checking that all path elements are alphanumeric.
func PathValidator(path string) error {
pathArr := strings.Split(path, "/")
if pathArr[0] == path {
if len(pathArr) > 0 && pathArr[0] == path {
return sdkerrors.Wrapf(ErrInvalidPath, "path %s doesn't contain any separator '/'", path)
}
allEmptyItems := true
for _, p := range pathArr {
// Each path element must be alphanumeric and non-blank
if strings.TrimSpace(p) == "" || !sdk.IsAlphaNumeric(p) {
return sdkerrors.Wrapf(ErrInvalidPath, "path %s contains an invalid non-alphanumeric character: '%s'", path, p)
// NOTE: for some reason an empty string is added after Split
if p == "" {
continue
}
allEmptyItems = false
// Each path element must be a valid identifier or constant number
if err := defaultIdentifierValidator(p, 1, 20); err != nil {
return sdkerrors.Wrapf(err, "path %s contains an invalid identifier: '%s'", path, p)
}
}
if allEmptyItems {
return fmt.Errorf("path %s must contain at least one identifier", path)
}
return nil
}

106
x/ibc/24-host/validate_test.go Normal file
View File

@ -0,0 +1,106 @@
package host
import (
"fmt"
"strings"
"testing"
"github.com/stretchr/testify/require"
)
type testCase struct {
msg string
id string
expPass bool
}
func TestDefaultIdentifierValidator(t *testing.T) {
testCases := []testCase{
{"valid lowercase", "lowercaseid", true},
{"valid id special chars", "._+-#[]<>._+-#[]<>", true},
{"valid id lower and special chars", "lower._+-#[]<>", true},
{"numeric id", "1234567890", true},
{"uppercase id", "NOTLOWERCASE", true},
{"numeric id", "1234567890", true},
{"blank id", " ", false},
{"id length out of range", "1", false},
{"path-like id", "lower/case/id", false},
{"invalid id", "(clientid)", false},
{"empty string", "", false},
}
for _, tc := range testCases {
err := ClientIdentifierValidator(tc.id)
err1 := ConnectionIdentifierValidator(tc.id)
err2 := ChannelIdentifierValidator(tc.id)
err3 := PortIdentifierValidator(tc.id)
if tc.expPass {
require.NoError(t, err, tc.msg)
require.NoError(t, err1, tc.msg)
require.NoError(t, err2, tc.msg)
require.NoError(t, err3, tc.msg)
} else {
require.Error(t, err, tc.msg)
require.Error(t, err1, tc.msg)
require.Error(t, err2, tc.msg)
require.Error(t, err3, tc.msg)
}
}
}
func TestPathValidator(t *testing.T) {
testCases := []testCase{
{"valid lowercase", "/lowercaseid", true},
{"numeric path", "/239123", true},
{"valid id special chars", "/._+-#[]<>._+-#[]<>", true},
{"valid id lower and special chars", "lower/._+-#[]<>", true},
{"id length out of range", "/l", true},
{"uppercase id", "/NOTLOWERCASE", true},
{"invalid path", "lowercaseid", false},
{"blank id", "/ ", false},
{"id length out of range", "/123456789012345678901", false},
{"invalid id", "/(clientid)", false},
{"empty string", "", false},
{"separators only", "////", false},
{"just separator", "/", false},
}
for _, tc := range testCases {
err := PathValidator(tc.id)
if tc.expPass {
seps := strings.Count(tc.id, "/")
require.Equal(t, 1, seps)
require.NoError(t, err, tc.msg)
} else {
require.Error(t, err, tc.msg)
}
}
}
func TestCustomPathValidator(t *testing.T) {
validateFn := NewPathValidator(func(path string) error {
if !strings.HasPrefix(path, "id_") {
return fmt.Errorf("identifier %s must start with 'id_", path)
}
return nil
})
testCases := []testCase{
{"valid custom path", "/id_client/id_one", true},
{"invalid path", "client", false},
{"invalid custom path", "/client", false},
{"invalid identifier", "/id_client/id_123456789012345678901", false},
{"separators only", "////", false},
{"just separator", "/", false},
}
for _, tc := range testCases {
err := validateFn(tc.id)
if tc.expPass {
require.NoError(t, err, tc.msg)
} else {
require.Error(t, err, tc.msg)
}
}
}

6
x/ibc/genesis_test.go
View File

@ -84,7 +84,7 @@ func (suite *IBCTestSuite) TestValidateGenesis() {
ClientGenesis: client.NewGenesisState(
[]exported.ClientState{
ibctmtypes.NewClientState(clientID, lite.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, suite.header),
localhosttypes.NewClientState("chaindID", 0),
localhosttypes.NewClientState("(chaindID)", 0),
},
nil,
false,
@ -99,7 +99,7 @@ func (suite *IBCTestSuite) TestValidateGenesis() {
ClientGenesis: client.DefaultGenesisState(),
ConnectionGenesis: connection.NewGenesisState(
[]connection.End{
connection.NewConnectionEnd(connection.INIT, connectionID, "CLIENTIDONE", connection.NewCounterparty(clientID, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))), []string{"1.0.0"}),
connection.NewConnectionEnd(connection.INIT, connectionID, "(CLIENTIDONE)", connection.NewCounterparty(clientID, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))), []string{"1.0.0"}),
},
[]connection.Paths{
connection.NewConnectionPaths(clientID, []string{host.ConnectionPath(connectionID)}),
@ -115,7 +115,7 @@ func (suite *IBCTestSuite) TestValidateGenesis() {
ConnectionGenesis: connection.DefaultGenesisState(),
ChannelGenesis: channel.GenesisState{
Acknowledgements: []channel.PacketAckCommitment{
channel.NewPacketAckCommitment("portID", channel1, 1, []byte("ack")),
channel.NewPacketAckCommitment("(portID)", channel1, 1, []byte("ack")),
},
},
},