client: parseQueryPath, isQueryStoreWithProof are untested, duplicate logic and brittle/rigid in format

[odeke-em]

Coming here from an audit, client.parseQueryStorePath and client.isQueryStoreWithProof are untested, yet is in the path of querying the ABCI and verifying proofs.

The source code to both looks like this

isQueryStoreWithProof

cosmos-sdk/client/query.go

Lines 198 to 215 in 13b5a8d

	func isQueryStoreWithProof(path string) bool {
	if !strings.HasPrefix(path, "/") {
	return false
	}
	paths := strings.SplitN(path[1:], "/", 3)
	switch {
	case len(paths) != 3:
	return false
	case paths[0] != "store":
	return false
	case rootmulti.RequireProof("/" + paths[2]):
	return true
	}
	return false
	}

parseQueryStorePath

cosmos-sdk/client/query.go

Lines 218 to 235 in 13b5a8d

	func parseQueryStorePath(path string) (storeName string, err error) {
	if !strings.HasPrefix(path, "/") {
	return "", errors.New("expected path to start with /")
	}
	paths := strings.SplitN(path[1:], "/", 3)
	switch {
	case len(paths) != 3:
	return "", errors.New("expected format like /store/<storeName>/key")
	case paths[0] != "store":
	return "", errors.New("expected format like /store/<storeName>/key")
	case paths[2] != "key":
	return "", errors.New("expected format like /store/<storeName>/key")
	}
	return paths[1], nil
	}

Problems

Code duplication

Notice that they have pretty much the same logic except one returns a boolean, the other returns (string, error). We can fold isQueryStoreWithProof to simply be

func isQueryStoreWithProof(path string) bool {
        storeName, err := parseQueryStorePath(path)
        return err == nil && storeName != ""
}

Rigid path requirement for a mistake that most users make aka a slash affixed at the end of a path

Passing in a path such as /store/foo/key/ will fail, but /store/foo/key will pass. Think about how common it is to use tab completion on the command-line and what your paths look like after tab completion, usually in the form /store/foo/key/.

We can fix this code by a special ingredient strings.Trim(path, "/") aka

        // Trim off the first and last slashes.
        path = strings.Trim(path, "/")
        paths := strings.SplitN(path, "/", 3)

and finally the code will look like this

// parseQueryStorePath expects a format like /store/<storeName>/key.
func parseQueryStorePath(path string) (storeName string, err error) {
        if !strings.HasPrefix(path, "/") {
                return "", errors.New("expected path to start with /")
        }

        // Trim off the first and last slashes.
        path = strings.Trim(path, "/")
        paths := strings.SplitN(path, "/", 3)

        switch {
        case len(paths) != 3:
                return "", errors.New("expected format like /store/<storeName>/key")
        case paths[0] != "store":
                return "", errors.New("expected format like /store/<storeName>/key")
        case paths[2] != "key":
                return "", errors.New("expected format like /store/<storeName>/key")
        }

        return paths[1], nil
}

and here is a full test that'll cover bases

package client

import (
	"testing"
)

func TestQueryStorePath(t *testing.T) {
	tests := []struct {
		in      string
		want    string
		wantErr string
	}{
		{
			in:   "/store/foo/key",
			want: "foo",
		},
		{
			in:   "/store/foo/key/",
			want: "foo",
		},
		{
			in:   "/store/foo/key//",
			want: "foo",
		},
	}

	for _, tt := range tests {
		tt := tt
		t.Run(tt.in, func(t *testing.T) {
			got, err := parseQueryStorePath(tt.in)
			if tt.wantErr != "" {
				if err == nil {
					t.Fatalf("Expected a non-nil error: %q", tt.wantErr)
				}
				if g, w := err.Error(), tt.wantErr; g != w {
					t.Fatalf("Error mismatch\nGot:  %q\nWant: %q", g, w)
				}
				return
			}
			if err != nil {
				t.Fatalf("Unexpected error: %q\n", err)
			}

			if g, w := got, tt.want; g != w {
				t.Fatalf("StoreName mismatch\nGot:  %q\nWant: %q", g, w)
			}
		})
	}
}

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[odeke-em]

PR #6805 by @marbar3778 removed parseQueryPath, so case closed.