Skip to content

Commit

Permalink
Fix false positives for multi-line url() in max-line-length (#4169)
Browse files Browse the repository at this point in the history
  • Loading branch information
@vankop @hudochenkov
vankop authored and hudochenkov committed Jul 31, 2019
1 parent 447bae9 commit b9c6458
Show file tree
Hide file tree
Showing 2 changed files with 119 additions and 34 deletions.
93 changes: 74 additions & 19 deletions lib/rules/max-line-length/__tests__/index.js
View file
Expand Up @@ -2,6 +2,8 @@

const rule = require("..");
const { messages, ruleName } = rule;
const testUrl = "somethingsomething something\tsomething";
const _21whitespaces = new Array(21).fill("\u0020").join("");

testRule(rule, {
ruleName,
Expand All @@ -21,25 +23,25 @@ testRule(rule, {
code: "@media print {\n a {\n color: pink;\n }\n}"
},
{
code: "a {\n background: url(somethingsomethingsomethingsomething);\n}"
code: `a {\n background: url("${testUrl}");\n}`
},
{
code: "a {\n background: uRl(somethingsomethingsomethingsomething);\n}"
code: `a {\n background: uRl("${testUrl}");\n}`
},
{
code: "a {\n background: URL(somethingsomethingsomethingsomething);\n}"
code: `a {\n background: URL("${testUrl}");\n}`
},
{
code:
"a {\n background: url(\n somethingsomethingsomethingsomething\n );\n}"
code: `a {\n background: url(\n "${testUrl}"\n );\n}`
},
{
code:
"a {\n background: uRl(\n somethingsomethingsomethingsomething\n );\n}"
code: `a {\n background: uRl(\n "${testUrl}"\n );\n}`
},
{
code:
"a {\n background: URL(\n somethingsomethingsomethingsomething\n );\n}"
code: `a {\n background: URL(\n "${testUrl}"\n );\n}`
},
{
code: `a {\n background: URL(\n "${testUrl}"\n );\nbackground: url(\n "${testUrl}"\n);\n}`
},
{
code: "a { margin: 0 2px; }\r\n"
Expand All @@ -48,13 +50,32 @@ testRule(rule, {
code: "a { margin: 0 2px; }\r\na { margin: 4px 0; }\n"
},
{
code: '@import url("somethingsomethingsomethingsomething.css") print;'
code: `@import url("${testUrl}") print;`
},
{
code: `@import '${testUrl}';`
},
{
code: "@import 'somethingsomethingsomethingsomething.css';"
code: `@import "${testUrl}";`
},
{
code: "@import 'svg-something<id=\"horse\">' projection;"
},
{
// exactly 20 whitespaces
code:
"a {\n background-image:\nurl(\n" +
_21whitespaces.substr(0, 20) +
`"${testUrl}"` +
"\n); }"
},
{
// exactly 20 whitespaces
code:
"a {\n background-image:\nurl(\n" +
`"${testUrl}"` +
_21whitespaces.substr(0, 20) +
"\n); }"
}
],

Expand Down Expand Up @@ -83,6 +104,12 @@ testRule(rule, {
line: 3,
column: 33
},
{
code: `a {\n background: URL(\n "${testUrl}"\n );\n background: url(\n "${testUrl}"\n);\n}`,
message: messages.expected(20),
line: 5,
column: 27
},
{
code:
"@media print {\n a {\n color: pink; background: orange;\n }\n}",
Expand All @@ -103,18 +130,22 @@ testRule(rule, {
column: 21
},
{
code:
'@import url("somethingsomethingsomethingsomething.css") projection, tv;',
code: `@import url("${testUrl}") projection, tv;`,
message: messages.expected(20),
line: 1,
column: 71
column: 69
},
{
code:
"@import 'somethingsomethingsomethingsomething.css' projection, tv;",
code: `@import '${testUrl}' projection, tv;`,
message: messages.expected(20),
line: 1,
column: 64
},
{
code: `@import "${testUrl}" projection, tv;`,
message: messages.expected(20),
line: 1,
column: 66
column: 64
},
{
code: "@import 'svg-something<id=\"horse\">' screens, tv;",
Expand All @@ -123,17 +154,41 @@ testRule(rule, {
column: 48
},
{
code: "a { background-image: url('some ignored url'); }",
code: `a { background-image: url("${testUrl}"); }`,
message: messages.expected(20),
line: 1,
column: 48
column: 70
},
{
code:
"a {\n /* Lorem ipsum dolor sit amet. The comment Lorem ipsum dolor sit amet, consectetur adipisicing elit. Praesentium officia fugiat unde deserunt sit, tenetur! Incidunt similique blanditiis placeat ad quia possimus libero, reiciendis excepturi non esse deserunt a odit. */\n}",
message: messages.expected(20),
line: 2,
column: 272
},
{
// more than 20 whitespaces
code: `a {
background-image:
url(
${_21whitespaces}"${testUrl}"
);
}`,
message: messages.expected(20),
line: 4,
column: 69
},
{
// more than 20 whitespaces
code: `a {
background-image:
url(
"${testUrl}"${_21whitespaces}
);
}`,
message: messages.expected(20),
line: 4,
column: 69
}
]
});
Expand Down
60 changes: 45 additions & 15 deletions lib/rules/max-line-length/index.js
View file
Expand Up @@ -9,6 +9,10 @@ const styleSearch = require("style-search");
const validateOptions = require("../../utils/validateOptions");

const ruleName = "max-line-length";
const EXCLUDED_PATTERNS = [
/url\(\s*((.|[\t\v])*[^\s])\s*\)/gi, // allow tab, whitespace in url content
/@import\s+(['"](.|[\t\v])*['"])/gi

This comment has been minimized.

Sign in to view

Copy link
@aladdin-add

aladdin-add Aug 15, 2019

Contributor

please do not use this kind of regex -- it can cause potential redos!
refs: https://en.wikipedia.org/wiki/ReDoS

This comment has been minimized.

Sign in to view

Copy link
@ntwb

ntwb Aug 23, 2019

Member

Thanks, we're working on a solution to this internally with the team 👍🏼
];

const messages = ruleMessages(ruleName, {
expected: max =>
Expand Down Expand Up @@ -43,6 +47,21 @@ const rule = function(maxLength, options, context) {
const ignoreNonComments = optionsMatches(options, "ignore", "non-comments");
const ignoreComments = optionsMatches(options, "ignore", "comments");
const rootString = context.fix ? root.toString() : root.source.input.css;
// Array of skipped sub strings, i.e `url(...)`, `@import "..."`
const skippedSubStrings = [];
let skippedSubStringsIndex = 0;

EXCLUDED_PATTERNS.forEach(pattern =>
execall(pattern, rootString).forEach(match => {
const startOfSubString =
match.index + match.match.indexOf(_.get(match, "subMatches[0]", ""));

return skippedSubStrings.push([
startOfSubString,
startOfSubString + _.get(match, "subMatches[0].length", 0)
]);
})
);

// Check first line
checkNewline(rootString, { endIndex: 0 }, root);
Expand All @@ -62,6 +81,28 @@ const rule = function(maxLength, options, context) {
});
}

function tryToPopSubString(start, end) {
const [startSubString, endSubString] = skippedSubStrings[
skippedSubStringsIndex
];

// Excluded substring does not presented in current line
if (end < startSubString) {
return 0;
}

// Compute excluded substring size regarding to current line indexes
const excluded =
Math.min(end, endSubString) - Math.max(start, startSubString);

// Current substring is out of range for next lines
if (endSubString <= end) {
skippedSubStringsIndex++;
}

return excluded;
}

function checkNewline(rootString, match, root) {
let nextNewlineIndex = rootString.indexOf("\n", match.endIndex);

Expand All @@ -75,32 +116,21 @@ const rule = function(maxLength, options, context) {
}

const rawLineLength = nextNewlineIndex - match.endIndex;
const excludedLength = skippedSubStrings[skippedSubStringsIndex]
? tryToPopSubString(match.endIndex, nextNewlineIndex)
: 0;
const lineText = rootString.slice(match.endIndex, nextNewlineIndex);

// Case sensitive ignorePattern match
if (optionsMatches(options, "ignorePattern", lineText)) {
return;
}

const urlArgumentsLength = execall(/url\((.*)\)/gi, lineText).reduce(
(result, match) => {
return result + _.get(match, "subMatches[0].length", 0);
},
0
);

const importUrlsLength = execall(
/@import\s+(['"].*['"])/gi,
lineText
).reduce((result, match) => {
return result + _.get(match, "subMatches[0].length", 0);
}, 0);

// If the line's length is less than or equal to the specified
// max, ignore it ... So anything below is liable to be complained about.
// **Note that the length of any url arguments or import urls
// are excluded from the calculation.**
if (rawLineLength - urlArgumentsLength - importUrlsLength <= maxLength) {
if (rawLineLength - excludedLength <= maxLength) {
return;
}

Expand Down

0 comments on commit b9c6458

Please sign in to comment.