Validate URI query string encoding #183
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / validate-protos (pull_request).
|
cel/library.go
Outdated
| return false | ||
| } | ||
| } | ||
| if _, err := url.ParseQuery(uri.RawQuery); err != nil { |
There was a problem hiding this comment.
Can you add a comment here real quick why this is necessary for future contributors who might wonder why it's necessary?
There was a problem hiding this comment.
Yep, done. Let me know if it needs more detail.
Co-authored-by: Chris Roche <[email protected]>
Co-authored-by: Chris Roche <[email protected]>
There was a problem hiding this comment.
I think this is a worthwhile improvement in strictness overall, though I definitely am thinking more strongly by the day that we need to formally define the grammar allowed by each validation rule. Relying on Go's sensibilities is nice for protovalidate-go, but even not considering other languages it has some caveats, including this issue of parsing things just to validate them and throw away the parsed result. But, that's definitely out-of-scope for this PR.
| } | ||
|
|
||
| // Parse the query string to validate it is formed and encoded properly | ||
| _, err = url.ParseQuery(uri.RawQuery) |
There was a problem hiding this comment.
I understand the intent here, but this does an awful lot of work just to check the validity of the encoding. It parses out the query components and constructs a map with them, which we don't need.
It's a bit less succinct, but we can get the exact same validation while doing a lot less work:
// (Not sure if we want to disallow unescaped semicolons, but ParseQuery does.)
if strings.Contains(uri.RawQuery, ";") {
return false
}
_, err = url.QueryUnescape(uri.RawQuery)
return err == nilIt'd be even better if we could just validate without parsing and decoding everything, but I argue we at least should avoid building the map if we can.
There was a problem hiding this comment.
Sorry @jchadwick-buf i merged before i saw your comments. @rodaine wdyt? I can open up another PR to incorporate John's suggestions.
There was a problem hiding this comment.
Actually in talking to @timostamm today, we may want to discuss this deeper once his validation is done for protovalidate-ts. We can then make sure all the implementations are behaving the same way since he's following the RFC. We could then add additional conformance checks and formally define the grammar for what we validate.
This adds an additional check for validating that the query string of a URI is correctly encoded.