i/p/patterns: disallow /./ and /../ in path patterns #14774
+45
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since patterns like /foo/./bar don't match paths paths like /foo/bar,
throw an error if the client tries to reply or create a rule with such a
construction clearly in the pattern. That is, if the pattern contains
`/./` or `/../`, or if it ends with `/.` or `/..`.
Notably, we do this on the raw pattern string, similarly to how we
validate that the pattern begins with a `/`: we don't consider whether
all alts in a leading group happen to start with `/`, we simply throw an
error if the first character is not `/`.
In this case, we take a more lenient (but similarly lazy) approach: if
there's a literal `/./` or `/../`, or trailing `/.` or `/..`, then an
error is thrown. As long as there's something interrupting the `/` or
end of the pattern string (e.g. a group), we consider that fine. For
example, we allow `/foo/.{,bar}` even though technically this renders to
the variants `/foo/.` and `/foo/.bar`, where the former is undesirable.
We may reconsider this in the future, but checking whether any rendered
variant contains `/./` or `/../` in general requires fully rendering
each variant and checking each one, which we do not do at parse time,
and one nearly always has at least one pattern which is capable of
matching something valid (an exception being the pattern `/foo/.{,}`,
for example). But the user must fairly deliberately shoot themself in
the foot to end up in that situation.
Again, the worst case if a pattern which is "bad" in this way gets past
validation is that we end up with a path pattern which is incapable of
matching any paths. This is undesirable, but not problematic.
Signed-off-by: Oliver Calder <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #14774 +/- ##
==========================================
+ Coverage 78.95% 79.03% +0.08%
==========================================
Files 1084 1087 +3
Lines 146638 147944 +1306
==========================================
+ Hits 115773 116927 +1154
- Misses 23667 23775 +108
- Partials 7198 7242 +44
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Co-authored-by: Zeyad Yasser <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since patterns like /foo/./bar don't match paths paths like /foo/bar, throw an error if the client tries to reply or create a rule with such a construction clearly in the pattern. That is, if the pattern contains
/./or/../, or if it ends with/.or/...Notably, we do this on the raw pattern string, similarly to how we validate that the pattern begins with a
/: we don't consider whether all alts in a leading group happen to start with/, we simply throw an error if the first character is not/.In this case, we take a more lenient (but similarly lazy) approach: if there's a literal
/./or/../, or trailing/.or/.., then an error is thrown. As long as there's something interrupting the/or end of the pattern string (e.g. a group), we consider that fine. For example, we allow/foo/.{,bar}even though technically this renders to the variants/foo/.and/foo/.bar, where the former is undesirable.We may reconsider this in the future, but checking whether any rendered variant contains
/./or/../in general requires fully rendering each variant and checking each one, which we do not do at parse time, and one nearly always has at least one pattern which is capable of matching something valid (an exception being the pattern/foo/.{,}, for example). But the user must fairly deliberately shoot themself in the foot to end up in that situation.Again, the worst case if a pattern which is "bad" in this way gets past validation is that we end up with a path pattern which is incapable of matching any paths. This is undesirable, but not problematic.
This is a successor to #14502
CC @sminez
This is tracked internally by https://warthogs.atlassian.net/browse/SNAPDENG-32063