fix(vstorage)!: Enforce path validation #8348
Conversation
| var pathPattern = fmt.Sprintf(`^$|^%[1]s(%[2]s%[1]s)*$`, pathSegmentPattern, pathSeparatorPattern) | ||
| var pathMatcher = regexp.MustCompile(pathPattern) |
There was a problem hiding this comment.
Have we considered instead of building this complex RegExp to split the path and validate each segment with an anchored pathSegmentPattern ? I'm not sure how efficient golang is at creating string slices.
There was a problem hiding this comment.
It's not bad, but I prefer this single-pass approach.
| return func(ctx sdk.Context, pathSegments []string, req abci.RequestQuery) (res []byte, err error) { | ||
| switch pathSegments[0] { |
There was a problem hiding this comment.
Ok this is confusing. I see pathSegment is a / delimited part of the URL's path. At first I thought it was a . delimited vstorage path segment.
I'm not sure how to make this more clear, but right now it isn't sufficiently different. At the very least maybe we should use the "component" nomenclature for vstorage paths? But maybe we could prefix the variables with route/storage as appropriate here ?
There was a problem hiding this comment.
I've clarified by renaming to better distinguish URL path vs. vstorage entry path.
gibson042
force-pushed
the
gibson-8337-vstorage-path-validation
branch
from
4214f34
to
1d12459
Compare
…tion fix(vstorage)!: Enforce path validation
…tion fix(vstorage)!: Enforce path validation
Fixes #8337
Description
Improves the cross-referenced Go and JS documentation of vstorage path structure, and updates the Go code to correctly enforce it.
Security Considerations
This change means that vat storage keys are no longer generally valid in vstorage paths, but that is acceptable because they are now stored in the swingset module rather than in vstorage.
Scaling Considerations
There should be a small but largely irrelevant improvement in the performance of validating paths, which are now scanned only once if valid.
Documentation Considerations
n/a
Testing Considerations
Nothing should be needed beyond the included unit tests.
Upgrade Considerations
Existing tests should validate the assumption that vat storage keys are not used in vstorage paths.