Merge pull request #8348 from Agoric/gibson-8337-vstorage-path-valida…

…tion

fix(vstorage)!: Enforce path validation

golang/cosmos/x/vstorage/keeper/querier.go

@@ -1,8 +1,6 @@
 package keeper
 
 import (
-	"strings"
-
 	abci "github.com/tendermint/tendermint/abci/types"
 
 	"github.com/cosmos/cosmos-sdk/codec"
@@ -18,14 +16,36 @@ const (
 	QueryChildren = "children"
 )
 
-// NewQuerier is the module level router for state queries
+// getVstorageEntryPath validates that a request URL path represents a valid
+// entry path with no extra data, and returns the path of that vstorage entry.
+func getVstorageEntryPath(urlPathSegments []string) (string, error) {
+	if len(urlPathSegments) != 1 || types.ValidatePath(urlPathSegments[0]) != nil {
+		return "", sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "invalid vstorage entry path")
+	}
+	return urlPathSegments[0], nil
+}
+
+// NewQuerier returns the function for handling queries routed to this module.
+// It performs its own routing based on the first slash-separated URL path
+// segment (e.g., URL path `/data/foo.bar` is a request for the value associated
+// with vstorage path "foo.bar", and `/children/foo.bar` is a request for the
+// child path segments immediately underneath vstorage path "foo.bar" which may
+// be used to extend it to a vstorage path such as "foo.bar.baz").
 func NewQuerier(keeper Keeper, legacyQuerierCdc *codec.LegacyAmino) sdk.Querier {
-	return func(ctx sdk.Context, path []string, req abci.RequestQuery) (res []byte, err error) {
-		switch path[0] {
+	return func(ctx sdk.Context, urlPathSegments []string, req abci.RequestQuery) (res []byte, err error) {
+		switch urlPathSegments[0] {
 		case QueryData:
-			return queryData(ctx, strings.Join(path[1:], "/"), req, keeper, legacyQuerierCdc)
+			entryPath, entryPathErr := getVstorageEntryPath(urlPathSegments[1:])
+			if entryPathErr != nil {
+				return nil, entryPathErr
+			}
+			return queryData(ctx, entryPath, req, keeper, legacyQuerierCdc)
 		case QueryChildren:
-			return queryChildren(ctx, strings.Join(path[1:], "/"), req, keeper, legacyQuerierCdc)
+			entryPath, entryPathErr := getVstorageEntryPath(urlPathSegments[1:])
+			if entryPathErr != nil {
+				return nil, entryPathErr
+			}
+			return queryChildren(ctx, entryPath, req, keeper, legacyQuerierCdc)
 		default:
 			return nil, sdkerrors.Wrap(sdkerrors.ErrUnknownRequest, "unknown vstorage query endpoint")
 		}
@@ -36,12 +56,12 @@ func NewQuerier(keeper Keeper, legacyQuerierCdc *codec.LegacyAmino) sdk.Querier
 func queryData(ctx sdk.Context, path string, req abci.RequestQuery, keeper Keeper, legacyQuerierCdc *codec.LegacyAmino) (res []byte, err error) {
 	entry := keeper.GetEntry(ctx, path)
 	if !entry.HasValue() {
-		return nil, sdkerrors.Wrap(sdkerrors.ErrUnknownRequest, "could not get vstorage path")
+		return nil, sdkerrors.Wrap(sdkerrors.ErrNotFound, "no data for vstorage path")
 	}
 
-	bz, err2 := codec.MarshalJSONIndent(legacyQuerierCdc, types.Data{Value: entry.StringValue()})
-	if err2 != nil {
-		return nil, sdkerrors.Wrap(sdkerrors.ErrJSONMarshal, err2.Error())
+	bz, marshalErr := codec.MarshalJSONIndent(legacyQuerierCdc, types.Data{Value: entry.StringValue()})
+	if marshalErr != nil {
+		return nil, sdkerrors.Wrap(sdkerrors.ErrJSONMarshal, marshalErr.Error())
 	}
 
 	return bz, nil

golang/cosmos/x/vstorage/types/path_keys.go

@@ -7,9 +7,17 @@ import (
 	"strings"
 )
 
-// - A "path" is a sequence of zero or more dot-separated nonempty strings of
-// 7-bit non-nul, non-dot ASCII characters. So `""`, `"foo"`, and
-// `"foo.bar.baz"` are paths but `"."`, "foo.", and "fo\0o" are not.
+// - A "path" is a sequence of zero or more dot-separated nonempty segments
+// using a restricted alphabet of ASCII alphanumerics plus underscore and dash,
+// consistent with packages/internal/src/lib-chainStorage.js but not currently
+// enforcing a length restriction on path segments.
+// So `""`, `"foo"`, and `"foo.bar__baz.qux--quux"` are paths but `"."`,
+// `"foo/bar"`, `"fo\to"`, and `"foö"` are not.
+// This alphabet might be expanded in the future, but such expansion SHOULD NOT
+// include control characters (including those that are not ASCII, such as
+// U+202E RIGHT-TO-LEFT OVERRIDE), slash `/` (which separates ABCI request path
+// segments in e.g. `custom/vstorage/data/foo`), or backslash `\` (which should
+// be reserved for adding escape sequences).
 //
 // - An encoded key for a path is the path prefixed with its length (in ASCII
 // digits), separated by nul, followed by the path with dots replaced with nul.
@@ -42,22 +50,26 @@ func EncodedKeyToPath(key []byte) string {
 	return string(pathBytes)
 }
 
-var pathPattern = regexp.MustCompile(`[-a-zA-Z0-9_` + PathSeparator + `]*`)
+var pathSegmentPattern = `[a-zA-Z0-9_-]+`
+var pathSeparatorPattern = `\` + PathSeparator
+var pathPattern = fmt.Sprintf(`^$|^%[1]s(%[2]s%[1]s)*$`, pathSegmentPattern, pathSeparatorPattern)
+var pathMatcher = regexp.MustCompile(pathPattern)
 
 func ValidatePath(path string) error {
-	if !pathPattern.MatchString(path) {
-		return fmt.Errorf("path %q contains invalid characters", path)
-	}
-	if strings.Contains(path, PathSeparator+PathSeparator) {
-		return fmt.Errorf("path %q contains doubled separators", path)
+	if pathMatcher.MatchString(path) {
+		return nil
 	}
+	// Rescan the string to give a useful error message.
 	if strings.HasPrefix(path, PathSeparator) {
 		return fmt.Errorf("path %q starts with separator", path)
 	}
 	if strings.HasSuffix(path, PathSeparator) {
 		return fmt.Errorf("path %q ends with separator", path)
 	}
-	return nil
+	if strings.Contains(path, PathSeparator+PathSeparator) {
+		return fmt.Errorf("path %q contains doubled separators", path)
+	}
+	return fmt.Errorf("path %q contains invalid characters", path)
 }
 
 // PathToEncodedKey converts a path to a byte slice key

golang/cosmos/x/vstorage/types/path_keys_test.go

@@ -2,40 +2,106 @@ package types
 
 import (
 	"bytes"
+	"fmt"
+	"strings"
 	"testing"
 )
 
 func Test_Key_Encoding(t *testing.T) {
 	tests := []struct {
-		name     string
-		childStr string
-		key      []byte
+		name        string
+		path        string
+		key         []byte
+		errContains string
 	}{
 		{
-			name:     "empty key is prefixed",
-			childStr: "",
-			key:      []byte("0\x00"),
+			name: "empty path",
+			path: "",
+			key:  []byte("0\x00"),
 		},
 		{
-			name:     "some key string",
-			childStr: "some",
-			key:      []byte("1\x00some"),
+			name: "single-segment path",
+			path: "some",
+			key:  []byte("1\x00some"),
 		},
 		{
-			name:     "dot-separated",
-			childStr: "some.child.grandchild",
-			key:      []byte("3\x00some\x00child\x00grandchild"),
+			name: "multi-segment path",
+			path: "some.child.grandchild",
+			key:  []byte("3\x00some\x00child\x00grandchild"),
+		},
+		{
+			name: "non-letters",
+			path: "-_0_-",
+			key:  []byte("1\x00-_0_-"),
+		},
+		{
+			name:        "lone dot",
+			path:        ".",
+			errContains: "starts with separator",
+		},
+		{
+			name:        "starts with dot",
+			path:        ".foo",
+			errContains: "starts with separator",
+		},
+		{
+			name:        "ends with dot",
+			path:        "foo.",
+			errContains: "ends with separator",
+		},
+		{
+			name:        "empty path segment",
+			path:        "foo..bar",
+			errContains: "doubled separators",
+		},
+		{
+			name:        "invalid path character U+0000 NUL",
+			path:        "foo\x00bar",
+			errContains: "invalid character",
+		},
+		{
+			name:        "invalid path character U+002F SOLIDUS",
+			path:        "foo/bar",
+			errContains: "invalid character",
+		},
+		{
+			name:        "invalid path character U+005C REVERSE SOLIDUS",
+			path:        "foo\\bar",
+			errContains: "invalid character",
+		},
+		{
+			name:        "invalid path character U+007C VERTICAL LINE",
+			path:        "foo|bar",
+			errContains: "invalid character",
 		},
 	}
 
 	for _, tt := range tests {
+		if tt.key != nil {
+			t.Run(tt.name, func(t *testing.T) {
+				if key := PathToEncodedKey(tt.path); !bytes.Equal(key, tt.key) {
+					t.Errorf("pathToKey(%q) = []byte(%q), want []byte(%q)", tt.path, key, tt.key)
+				}
+				if path := EncodedKeyToPath(tt.key); path != tt.path {
+					t.Errorf("keyToPath([]byte(%q)) = %q, want %q", tt.key, path, tt.path)
+				}
+			})
+			continue
+		}
+		expect := tt.errContains
 		t.Run(tt.name, func(t *testing.T) {
-			if key := PathToEncodedKey(tt.childStr); !bytes.Equal(key, tt.key) {
-				t.Errorf("pathToKey(%q) = %v, want %v", tt.childStr, key, tt.key)
-			}
-			if childStr := EncodedKeyToPath(tt.key); childStr != tt.childStr {
-				t.Errorf("keyToString(%v) = %q, want %q", tt.key, childStr, tt.childStr)
-			}
+			var key []byte
+			defer func() {
+				if err := recover(); err != nil {
+					errStr := fmt.Sprintf("%v", err)
+					if !strings.Contains(errStr, expect) {
+						t.Errorf("pathToKey(%q) = error %q, want error %v", tt.path, errStr, expect)
+					}
+				} else {
+					t.Errorf("pathToKey(%q) = []byte(%q), want error %v", tt.path, key, expect)
+				}
+			}()
+			key = PathToEncodedKey(tt.path)
 		})
 	}
 }

packages/internal/src/lib-chainStorage.js

@@ -95,6 +95,7 @@ harden(assertCapData);
 // Must be nonempty and disallow (unescaped) `.`, and for simplicity
 // (and future possibility of e.g. escaping) we currently limit to
 // ASCII alphanumeric plus underscore and dash.
+// Should remain consistent with golang/cosmos/x/vstorage/types/path_keys.go
 const pathSegmentPattern = /^[a-zA-Z0-9_-]{1,100}$/;
 
 /** @type {(name: string) => void} */