From bb0a58ceb5c35a38b7e4172fe0e9d7cda263ccdb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 13 Jan 2022 12:47:11 +0100 Subject: [PATCH 1/5] update IsRevisionFormat to disallow newlines before the - --- modules/core/02-client/types/height.go | 5 +++-- modules/core/02-client/types/height_test.go | 6 ++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/modules/core/02-client/types/height.go b/modules/core/02-client/types/height.go index 88058674539..4241f144ef8 100644 --- a/modules/core/02-client/types/height.go +++ b/modules/core/02-client/types/height.go @@ -16,9 +16,10 @@ import ( var _ exported.Height = (*Height)(nil) // IsRevisionFormat checks if a chainID is in the format required for parsing revisions -// The chainID must be in the form: `{chainID}-{revision} +// The chainID must be in the form: `{chainID}-{revision}`. // 24-host may enforce stricter checks on chainID -var IsRevisionFormat = regexp.MustCompile(`^.*[^-]-{1}[1-9][0-9]*$`).MatchString +// Newlines are allowed in the chainID. +var IsRevisionFormat = regexp.MustCompile(`^.*[^\n-]-{1}[1-9][0-9]*$`).MatchString // ZeroHeight is a helper function which returns an uninitialized height. func ZeroHeight() Height { diff --git a/modules/core/02-client/types/height_test.go b/modules/core/02-client/types/height_test.go index b782ae9f533..95cea18557a 100644 --- a/modules/core/02-client/types/height_test.go +++ b/modules/core/02-client/types/height_test.go @@ -112,16 +112,18 @@ func TestParseChainID(t *testing.T) { {"gaiamainnet--4", 0, false}, {"gaiamainnet-3.4", 0, false}, {"gaiamainnet", 0, false}, + {"gaiamainnet-1\n", 0, false}, // newlines not allowed after dash + {"gaiamainnet\n-3", 0, false}, // newlines not allowed before revision number {"a--1", 0, false}, {"-1", 0, false}, {"--1", 0, false}, } - for i, tc := range cases { + for _, tc := range cases { require.Equal(t, tc.formatted, types.IsRevisionFormat(tc.chainID), "id %s does not match expected format", tc.chainID) revision := types.ParseChainID(tc.chainID) - require.Equal(t, tc.revision, revision, "case %d returns incorrect revision", i) + require.Equal(t, tc.revision, revision, "chainID %s returns incorrect revision", tc.chainID) } } From ee536689a73c68fa34b7c76bbe36f9dbebb66ed3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 13 Jan 2022 12:49:48 +0100 Subject: [PATCH 2/5] update IsClientIDFormat to account for newlines before the dash --- modules/core/02-client/types/keys.go | 2 +- modules/core/02-client/types/keys_test.go | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/modules/core/02-client/types/keys.go b/modules/core/02-client/types/keys.go index 643c3e639be..426747b0f6c 100644 --- a/modules/core/02-client/types/keys.go +++ b/modules/core/02-client/types/keys.go @@ -34,7 +34,7 @@ func FormatClientIdentifier(clientType string, sequence uint64) string { // IsClientIDFormat checks if a clientID is in the format required on the SDK for // parsing client identifiers. The client identifier must be in the form: `{client-type}-{N} -var IsClientIDFormat = regexp.MustCompile(`^.*[^-]-[0-9]{1,20}$`).MatchString +var IsClientIDFormat = regexp.MustCompile(`^.*[^\n-]-[0-9]{1,20}$`).MatchString // IsValidClientID checks if the clientID is valid and can be parsed into the client // identifier format. diff --git a/modules/core/02-client/types/keys_test.go b/modules/core/02-client/types/keys_test.go index 52d4e6a4c13..da722040e1c 100644 --- a/modules/core/02-client/types/keys_test.go +++ b/modules/core/02-client/types/keys_test.go @@ -27,6 +27,8 @@ func TestParseClientIdentifier(t *testing.T) { {"invalid uint64", "tendermint-18446744073709551616", "tendermint", 0, false}, // uint64 == 20 characters {"invalid large sequence", "tendermint-2345682193567182931243", "tendermint", 0, false}, + {"invalid newline in clientID", "tendermin\nt-1", "tendermin\nt", 0, false}, + {"invalid newline character before dash", "tendermint\n-1", "tendermint", 0, false}, {"missing dash", "tendermint0", "tendermint", 0, false}, {"blank id", " ", " ", 0, false}, {"empty id", "", "", 0, false}, From 54c86f16e9b55750e459b1824d6325b772ca8223 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 13 Jan 2022 12:52:26 +0100 Subject: [PATCH 3/5] add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d2f9fd72dc..ad42697c839 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (modules/core/05-port) [\#288](https://github.com/cosmos/ibc-go/issues/288) Making the 05-port keeper function IsBound public. The IsBound function checks if the provided portID is already binded to a module. * (channel) [\#644](https://github.com/cosmos/ibc-go/pull/644) Adds `GetChannelConnection` to the ChannelKeeper. This function returns the connectionID and connection state associated with a channel. * (channel) [\647](https://github.com/cosmos/ibc-go/pull/647) Reorganizes channel handshake handling to set channel state after IBC application callbacks. +* (client) [\#724](https://github.com/cosmos/ibc-go/pull/724) `IsRevisionFormat` and `IsClientIDFormat` now disallow newlines before the dash used to separate the chainID and revision number, and the client type and client sequence. ### Features From 7beafddb1c6291ff1cba7237ea48966be6adcfca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 13 Jan 2022 12:53:45 +0100 Subject: [PATCH 4/5] Update modules/core/02-client/types/height.go --- modules/core/02-client/types/height.go | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/core/02-client/types/height.go b/modules/core/02-client/types/height.go index 4241f144ef8..40125b23f2e 100644 --- a/modules/core/02-client/types/height.go +++ b/modules/core/02-client/types/height.go @@ -18,7 +18,6 @@ var _ exported.Height = (*Height)(nil) // IsRevisionFormat checks if a chainID is in the format required for parsing revisions // The chainID must be in the form: `{chainID}-{revision}`. // 24-host may enforce stricter checks on chainID -// Newlines are allowed in the chainID. var IsRevisionFormat = regexp.MustCompile(`^.*[^\n-]-{1}[1-9][0-9]*$`).MatchString // ZeroHeight is a helper function which returns an uninitialized height. From 3de13d43c38b2b061ee0e2e45a6cc2d891e73cab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 13 Jan 2022 12:55:11 +0100 Subject: [PATCH 5/5] add test case and update CHANGELOG --- CHANGELOG.md | 2 +- modules/core/02-client/types/height_test.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad42697c839..399519e6a24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,7 +58,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (modules/core/05-port) [\#288](https://github.com/cosmos/ibc-go/issues/288) Making the 05-port keeper function IsBound public. The IsBound function checks if the provided portID is already binded to a module. * (channel) [\#644](https://github.com/cosmos/ibc-go/pull/644) Adds `GetChannelConnection` to the ChannelKeeper. This function returns the connectionID and connection state associated with a channel. * (channel) [\647](https://github.com/cosmos/ibc-go/pull/647) Reorganizes channel handshake handling to set channel state after IBC application callbacks. -* (client) [\#724](https://github.com/cosmos/ibc-go/pull/724) `IsRevisionFormat` and `IsClientIDFormat` now disallow newlines before the dash used to separate the chainID and revision number, and the client type and client sequence. +* (client) [\#724](https://github.com/cosmos/ibc-go/pull/724) `IsRevisionFormat` and `IsClientIDFormat` have been updated to disallow newlines before the dash used to separate the chainID and revision number, and the client type and client sequence. ### Features diff --git a/modules/core/02-client/types/height_test.go b/modules/core/02-client/types/height_test.go index 95cea18557a..c31bbaabf21 100644 --- a/modules/core/02-client/types/height_test.go +++ b/modules/core/02-client/types/height_test.go @@ -112,6 +112,7 @@ func TestParseChainID(t *testing.T) { {"gaiamainnet--4", 0, false}, {"gaiamainnet-3.4", 0, false}, {"gaiamainnet", 0, false}, + {"gaiamain\nnet-1", 0, false}, // newlines not allowed in chainID {"gaiamainnet-1\n", 0, false}, // newlines not allowed after dash {"gaiamainnet\n-3", 0, false}, // newlines not allowed before revision number {"a--1", 0, false},