Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow host name fuzzy matching for Compatibility URLs #680

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

@christophertino
Copy link
Member

@christophertino christophertino commented Feb 6, 2021

Updates fuzzyUrlMatcher() to allow host name fuzzy matches from the Compatibility DB. A compatibility entry for *atlassian.net should allow ghostery.atlassian.net. This will also allow host and path fuzzy matches, i.e. *atlassian.net/page*.

@christophertino christophertino requested a review from zarembsky Feb 6, 2021
@christophertino christophertino requested review from benstrumeyer, wlycdgr and ghostery/extension as code owners Feb 6, 2021
@christophertino christophertino changed the base branch from master to develop Feb 6, 2021
@jsignanini
Copy link
Member

@jsignanini jsignanini commented Feb 9, 2021

@christophertino my only suggestion here is that we make this more strict so that fuzzy matching only works before a ., because on the example above, an entry of *atlassian.net would match with www.anythingatlassian.net which would not be the desired outcome. So it should only work as *.atlassian.net.

Copy link
Contributor

@zarembsky zarembsky left a comment

Agree with @jsignanini. I'll update Midnight PR accordingly.

@fcjr
Copy link
Member

@fcjr fcjr commented Feb 9, 2021

@jsignanini if we keep the syntax as is and instead just add the rules as *.atlassian.net is there much harm? Then if for some reason we wanted to add a rule based on domain partials in the future we have the flexibility.

@jsignanini
Copy link
Member

@jsignanini jsignanini commented Feb 9, 2021

@fcjr I'd rather be safe than sorry, patterns could be incorrectly introduced quite easily, and I don't really see a valid use-case for a catch-all that doesn't get restricted to sub-domains.

@christophertino christophertino requested a review from zarembsky Feb 9, 2021
Copy link
Contributor

@zarembsky zarembsky left a comment

Looks good. Can be merged once Travis CI is over.

@christophertino christophertino added this to the 8.5.6 milestone Feb 10, 2021
@wlycdgr wlycdgr modified the milestones: 8.5.6, 8.5.7 Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants