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

Added: `function-url-scheme-blacklist` rule. #2626

Merged
merged 5 commits into from Jun 20, 2017

Conversation

3 participants
@evilebottnawi
Member

evilebottnawi commented Jun 12, 2017

Which issue, if any, is this issue related to?

#2513

Is there anything in the PR that needs further explanation?

No, it's self explanatory.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 12, 2017

@jeddy3

Looking good.

Minor doc request. Additionally, can you update:

  • docs/user-guide/example-config.md.
  • docs/user-guide/rules.md.

If we ever add any more rules which operate on the scheme of a URL I suggest we abstract some of the code this and the whitelist rule, e.g. is schemeless url, into utilities. I think it's fine at the moment, though.

@@ -0,0 +1,67 @@
# function-url-scheme-blacklist
Specify a blacklist of allowed url schemes.

This comment has been minimized.

@jeddy3

jeddy3 Jun 12, 2017

Member

Specify a blacklist of disallowed url schemes.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 12, 2017

@jeddy3 stupid error, sorry

@evilebottnawi evilebottnawi force-pushed the function-url-scheme-blacklist branch from 76e07a7 to 9f7c21f Jun 12, 2017

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 12, 2017

/cc @jeddy3

@jeddy3

jeddy3 approved these changes Jun 12, 2017

LGTM thanks

@hudochenkov

Great work!

I have few questions, though, in comments.

Also, please, make all “url” → “URL” in readme, because “URL” is an acronym.

const ruleName = "function-url-scheme-blacklist"
const messages = ruleMessages(ruleName, {
rejected: scheme => `Unexpected url scheme "${scheme}:"`,

This comment has been minimized.

@hudochenkov

hudochenkov Jun 15, 2017

Member

Upper case “URL”.

This comment has been minimized.

@evilebottnawi

evilebottnawi Jun 20, 2017

Member

done, also in whitelist rule.

return (root, result) => {
const validOptions = validateOptions(result, ruleName, {
actual: blacklist,
possible: [_.isString],

This comment has been minimized.

@hudochenkov

hudochenkov Jun 15, 2017

Member

Readme says it accepts array|string. I'm not sure if it checks that. There're no tests if only one string was in config (not array with one string).

This comment has been minimized.

@evilebottnawi

evilebottnawi Jun 20, 2017

Member

@hudochenkov we use this in all *-blacklist/*-whitelist rules.

testRule(rule, {
ruleName,
config: [[]],

This comment has been minimized.

@hudochenkov

hudochenkov Jun 15, 2017

Member

Why this test group rejects https and data? Is it an undocumented behaviour?

This comment has been minimized.

@evilebottnawi

evilebottnawi Jun 20, 2017

Member

@hudochenkov this used if your want add all schemes to blacklist, analogy in whitelist

This comment has been minimized.

@hudochenkov

hudochenkov Jun 20, 2017

Member

But still behaviour isn't documented anywhere. We should document this.

testRule(rule, {
ruleName,
config: [""],

This comment has been minimized.

@hudochenkov

hudochenkov Jun 15, 2017

Member

Why this test group rejects https and data? Is it an undocumented behaviour?

This comment has been minimized.

@evilebottnawi

evilebottnawi Jun 20, 2017

Member

@hudochenkov we already have same tests in function-url-scheme-whitelist

This comment has been minimized.

@hudochenkov

hudochenkov Jun 20, 2017

Member

But still behaviour isn't documented anywhere. We should document this.

This comment has been minimized.

@evilebottnawi

evilebottnawi Jun 20, 2017

Member

@hudochenkov can your help me with this, I would like the current time to concentrate on the new rules (#2528) and I'm bad at documentation 😞

This comment has been minimized.

@jeddy3

jeddy3 Jun 20, 2017

Member

@hudochenkov Good catch! Sorry I missed this. This isn't the correct behaviour. Both [""] and [[]] should allow all schemes.

This is how the other -blacklist rules behave. The -whitelist rules behave differently because whitelists and blacklists are different.

It's useful to think as:

blacklist: [] = "blacklist nothing" i.e. "everything is ok"
whitelist: [] = "whitelist nothing" i.e. "nothing is ok".

@evilebottnawi evilebottnawi force-pushed the function-url-scheme-blacklist branch from 9f7c21f to 93b5a52 Jun 20, 2017

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 20, 2017

@evilebottnawi evilebottnawi force-pushed the function-url-scheme-blacklist branch from 93b5a52 to bae664d Jun 20, 2017

testRule(rule, {
ruleName,
config: [""],

This comment has been minimized.

@jeddy3

jeddy3 Jun 20, 2017

Member

@hudochenkov Good catch! Sorry I missed this. This isn't the correct behaviour. Both [""] and [[]] should allow all schemes.

This is how the other -blacklist rules behave. The -whitelist rules behave differently because whitelists and blacklists are different.

It's useful to think as:

blacklist: [] = "blacklist nothing" i.e. "everything is ok"
whitelist: [] = "whitelist nothing" i.e. "nothing is ok".

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 20, 2017

@jeddy3 can your help with documentation this?

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jun 20, 2017

@jeddy3 can your help with documentation this?

It's a behaviour issue, not a documentation one. I'm pushing up a commit to fix though.

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jun 20, 2017

  • I've added to tests to the property-*list rules to show how the whitelist and blacklist rule behaviours are correctly different when given empty strings or arrays as primary options.
  • This behaviour comes for free when using the matchesStringOrRegExp util. I've updated function-url-scheme-blacklist to use it.
  • Likewise, support for regex also comes for free when using matchesStringOrRegExp.
  • I've removed the tests for case insensitive primary options. None of the other *-*list rules have case-insensitive primary options. If we want to revisit this behaviour, so should do so across the board and in a separate issue.
@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 20, 2017

@jeddy3 After commits what are my actions here? Or just wait when @hudochenkov do review?

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jun 20, 2017

Or just wait when @hudochenkov do review?

Just wait on @hudochenkov as my commits came off the back of his original review.

@hudochenkov

RegEx addition is 🔥

And again about "" and [] configurations. We won't document them?

A [URL scheme](https://url.spec.whatwg.org/#syntax-url-scheme) consists of alphanumeric, `+`, `-`, and `.` characters. It can appear at the start of a URL and is followed by `:`.
This rule treats URL schemes as case insensitive (`https` and `HTTPS` are the same).

This comment has been minimized.

@hudochenkov

hudochenkov Jun 20, 2017

Member

I've removed the tests for case insensitive primary options. None of the other *-*list rules have case-insensitive primary options. If we want to revisit this behaviour, so should do so across the board and in a separate issue. — @jeddy3

Is this line in readme still valid?

This comment has been minimized.

@jeddy3

jeddy3 Jun 20, 2017

Member

Yes. I've added tests to show this.

Like all the other *-*list rules, this rule now only accepts lowercase primary options but is case insensitive when checking for matches.

So function-url-scheme-blacklist: "http" will disallow both http://g.co/x.jpg and HTTP://g.co/x.jpg.

#2660 is about allowing the user to specify primary options in any case e.g. function-url-scheme-blacklist: "HTTP", or function-blacklist: "translateX".

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jun 20, 2017

And again about "" and [] configurations. We won't document them?

I'm not sure there's any need to do that now because the behaviour is intuitive. We don't document this behaviour in any of the other *-blacklist rules for this reason.

scheme-blacklist: [] = "blacklist nothing" i.e. "all schemes are ok" seems pretty clear to me.

@hudochenkov

Thank you for your work, @evilebottnawi and @jeddy3!

@jeddy3

jeddy3 approved these changes Jun 20, 2017

@jeddy3 jeddy3 merged commit c3b4788 into master Jun 20, 2017

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 95.646%
Details

@jeddy3 jeddy3 deleted the function-url-scheme-blacklist branch Jun 20, 2017

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jun 20, 2017

Changelog

  • Added: function-url-scheme-blacklist rule (#2626).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment