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

GH-1477 Wildcard/Regex Whitelisting #497

Closed
wants to merge 24 commits into from
Closed

Conversation

@benstrumeyer
Copy link
Contributor

@benstrumeyer benstrumeyer commented Feb 10, 2020

  • [*] Have you followed the guidelines in CONTRIBUTING.md?
  • [*] Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • [*] Have you added an explanation of what your changes do?
  • [*] Does your submission pass tests?
  • [*] Did you lint your code prior to submission?
  • Add wildcard and regex input for whitelist/blacklist
  • Remove some validation since everything is a regex
  • Does not include paygating

Ticket: https://cliqztix.atlassian.net/browse/GH-1477

@benstrumeyer benstrumeyer requested a review from ghostery/ghostery as a code owner Feb 10, 2020
@benstrumeyer benstrumeyer requested a review from Eden12345 Feb 11, 2020
app/panel/components/Settings/TrustAndRestrict.jsx Outdated Show resolved Hide resolved
src/classes/Policy.js Outdated Show resolved Hide resolved
@Eden12345
Copy link
Contributor

@Eden12345 Eden12345 commented Feb 11, 2020

@benstrumeyer I'm also wondering if we need to add that new dependency since it's such a little amount of code. Do you want to just pull the code you're using out of it and add that code directly into the function where you used the module? Since we're validating that the pattern will be a string in the component, as it's coming in from a text input, I think all this node module is doing for you is essentially:

let regex = pattern.replace(/[|\\{}()[\]^$+*?.-]/g, '\\$&');

@benstrumeyer benstrumeyer requested a review from Eden12345 Feb 11, 2020
@Eden12345
Copy link
Contributor

@Eden12345 Eden12345 commented Feb 11, 2020

Looks good to me now, but requesting a review from @christophertino as I haven't touched this code much.

@Eden12345 Eden12345 requested a review from christophertino Feb 11, 2020
@christophertino christophertino added this to the 8.4.7 milestone Feb 14, 2020
Copy link
Member

@christophertino christophertino left a comment

We need to update the UI to let users know that wildcards/regex are supported. Let's update the settings_sites_placeholder string to example.com (wildcards/regex supported)

*/
addSite() {
// from node-validator
const isValidUrlRegex = /^(?!mailto:)(?:(?:https?|ftp):\/\/)?(?:\S+(?::\S*)?@)?(?:(?:(?:[1-9]\d?|1\d\d|2[01]\d|22[0-3])(?:\.(?:1?\d{1,2}|2[0-4]\d|25[0-5])){2}(?:\.(?:[1-9]\d?|1\d\d|2[0-4]\d|25[0-4]))|(?:(?:[a-z\u00a1-\uffff0-9]+-?)*[a-z\u00a1-\uffff0-9]+)(?:\.(?:[a-z\u00a1-\uffff0-9]+-?)*[a-z\u00a1-\uffff0-9]+)*(?:\.(?:[a-z\u00a1-\uffff]{2,})))|localhost)(?::\d{2,5})?(?:\/[^\s]*)?$/i;

This comment has been minimized.

@christophertino

christophertino Feb 14, 2020
Member

We still need front-end validation here so that we can prevent users from adding urls in unsupported formats. You should check that the url entered is either a valid url or a valid regex, and show an error if not.

This comment has been minimized.

@benstrumeyer

benstrumeyer Feb 18, 2020
Author Contributor

I checked if it's a valid URL, wildcard, or regex and updated the error messages. Let me know if you'd like to change anything else

christophertino and others added 4 commits Feb 14, 2020
…ension into regex-whitelisting
@benstrumeyer benstrumeyer requested a review from christophertino Feb 18, 2020
benstrumeyer and others added 9 commits Feb 18, 2020
* Add UTM params to Plus checkout from all locations

* Update tests

* Remove / from links with utm params

* Refactor messaging passing
@benstrumeyer benstrumeyer requested a review from zarembsky as a code owner Feb 21, 2020
@christophertino christophertino removed this from the 8.4.7 milestone Feb 25, 2020
@benstrumeyer benstrumeyer deleted the regex-whitelisting branch Feb 25, 2020
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

3 participants