This repository has been archived by the owner on Jan 26, 2022. It is now read-only.
Add examples to clarify Host and Regular expression URI match behavior #41
Merged
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
Add an example to the Host section illustrating that Host not only doesn't match parent domains but also doesn't match subdomains Add an example to Regular expression illustrating that the example regex matches substrings of the URI (because there is no end of string character)
Can we change the regex example to include an end of string character? That was the intent of the example. |
@kspearrin Ah even better, I'd assumed it was just a somewhat confusing set of examples (all of which happen to end in google.com but didn't need to). A typo in the regex makes a lot more sense. I've updated the PR have a look. |
Instead of adding an example to the Regular expression section to clarify that the regex shown doesn't require the string end in google.com, let's instead change the regex to end in a `$` and all the existing examples make a lot more sense.
ypid
added a commit
to ypid/help
that referenced
this pull request
Aug 14, 2020
I find it important that examples actually follow best practices. The current regex one is not for two reasons: 1. Not escaping dots in URIs when writing regex is a mistake that is often made, even by advanced regex people. I think this comes from the way we humans look at URIs. Dots are so common in their, aren’t they? Yes, but we write proper regular expressions here, not something in between. 2. `^https://.*google.com$` looks very artificial to me to point out the weaknesses of regular expressions for this use case. I find such an example very good for this purpose but there should also be a "good example" that complements it. Updates: bitwarden#41
ypid
added a commit
to ypid/help
that referenced
this pull request
Aug 14, 2020
I find it important that examples actually follow best practices. The current regex one is not for two reasons: 1. Not escaping dots in URIs when writing regex is a mistake that is often made, even by advanced regex people. I think this comes from the way we humans look at URIs. Dots are so common in their, aren’t they? Yes, but we write proper regular expressions here, not something in between. 2. `^https://.*google\.com$` is an improper regex (as already pointed out because it also matches `malicious-site.com`) that is only there to show the weaknesses of regular expressions for this use case. I find such an example very good for this purpose but there should also be a "good example" that complements it. I found such a "good example" that I hope is more useful and has no unwanted loopholes. Updates: bitwarden#41
ypid
added a commit
to ypid/help
that referenced
this pull request
Aug 14, 2020
I find it important that examples actually follow best practices. The current regex one is not for two reasons: 1. Not escaping dots in URIs when writing regex is a mistake that is often made, even by advanced regex people. I think this comes from the way we humans look at URIs. Dots are so common in there, aren’t they? Yes, but we write proper regular expressions here, not something in between. 2. `^https://.*google\.com$` is an improper regex (as already pointed out because it also matches `malicious-site.com`) that is only there to show the weaknesses of regular expressions for this use case. I find such an example very good for this purpose but there should also be a "good example" that complements it. I found such a "good example" that I hope is more useful and has no unwanted loopholes. Updates: bitwarden#41
gene1wood
commented
Aug 14, 2020
@@ -75,7 +75,7 @@ The regular expression option allows you to write any simple or complex [regular | |||
|
|||
Example: | |||
|
|||
- URI regex value: `^https://.*google.com` | |||
- URI regex value: `^https://.*google.com$` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gene1wood
added a commit
to gene1wood/help
that referenced
this pull request
Aug 14, 2020
This is an alternative solution the problem identified in bitwarden#158 that only corrects the regex mistake but doesn't add a good example and a bad example. The original mistake was introduced in d40a73f and then I overlooked it in bitwarden#41.
ypid
added a commit
to ypid/help
that referenced
this pull request
Feb 10, 2021
I find it important that examples actually follow best practices. The current regex one is not for two reasons: 1. Not escaping dots in URIs when writing regex is a mistake that is often made, even by advanced regex people. I think this comes from the way we humans look at URIs. Dots are so common in there, aren’t they? Yes, but we write proper regular expressions here, not something in between. 2. `^https://.*google\.com$` is an improper regex (as already pointed out because it also matches `malicious-site.com`) that is only there to show the weaknesses of regular expressions for this use case. I find such an example very good for this purpose but there should also be a "good example" that complements it. I found such a "good example" that I hope is more useful and has no unwanted loopholes. Updates: bitwarden#41
ypid
added a commit
to ypid/help
that referenced
this pull request
Feb 10, 2021
I find it important that examples actually follow best practices. The current regex one is not for two reasons: 1. Not escaping dots in URIs when writing regex is a mistake that is often made, even by advanced regex people. I think this comes from the way we humans look at URIs. Dots are so common in there, aren’t they? Yes, but we write proper regular expressions here, not something in between. 2. `^https://.*google\.com$` is an improper regex (as already pointed out because it also matches `malicious-site.com`) that is only there to show the weaknesses of regular expressions for this use case. I find such an example very good for this purpose but there should also be a "good example" that complements it. I found such a "good example" that I hope is more useful and has no unwanted loopholes. Updates: bitwarden#41 Replaces: bitwarden#160
ypid
added a commit
to ypid/help
that referenced
this pull request
Feb 10, 2021
I find it important that examples actually follow best practices. The current regex one is not for two reasons: 1. Not escaping dots in URIs when writing regex is a mistake that is often made, even by advanced regex people. I think this comes from the way we humans look at URIs. Dots are so common in there, aren’t they? Yes, but we write proper regular expressions here, not something in between. 2. `^https://.*google\.com$` is an improper regex (as already pointed out because it also matches `malicious-site.com`) that is only there to show the weaknesses of regular expressions for this use case. I find such an example very good for this purpose but there should also be a "good example" that complements it. I found such a "good example" that I hope is more useful and has no unwanted loopholes. Updates: bitwarden#41 Alternative: bitwarden#158 Replaces: bitwarden#160
ypid
added a commit
to ypid/help
that referenced
this pull request
Feb 10, 2021
I find it important that examples actually follow best practices. The current regex does not escaping dots in URIs when writing regex is a mistake that is often made, even by advanced regex people. I think this comes from the way we humans look at URIs. Dots are so common in there, aren’t they? Yes, but we write proper regular expressions here, not something in between. Updates: bitwarden#41 Alternative: bitwarden#158 Replaces: bitwarden#160
ypid
added a commit
to ypid/help
that referenced
this pull request
Feb 10, 2021
I find it important that examples actually follow best practices. The current regex does not escaping dots in URIs when writing regex is a mistake that is often made, even by advanced regex people. I think this comes from the way we humans look at URIs. Dots are so common in there, aren’t they? Yes, but we write proper regular expressions here, not something in between. Updates: bitwarden#41 Alternative: bitwarden#158 Replaces: bitwarden#160
fschillingeriv
pushed a commit
that referenced
this pull request
Feb 10, 2021
I find it important that examples actually follow best practices. The current regex does not escaping dots in URIs when writing regex is a mistake that is often made, even by advanced regex people. I think this comes from the way we humans look at URIs. Dots are so common in there, aren’t they? Yes, but we write proper regular expressions here, not something in between. Updates: #41 Alternative: #158 Replaces: #160
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Host
section illustrating thatHost
not only doesn't match parent domains but also doesn't match subdomainsRegular expression
illustrating that the example regex matches substrings of the URI (because there is no end of string character)