Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Unified Diff: lib/filterClasses.js

Issue 29680684: [$csp1 adblockpluscore] Issue 6329 - Allow whitespace in filter option values (Closed)
Patch Set: Created Jan. 26, 2018, 5:32 p.m.
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « no previous file | lib/synchronizer.js » ('j') | lib/synchronizer.js » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: lib/filterClasses.js
diff --git a/lib/filterClasses.js b/lib/filterClasses.js
index 1498ad8db2da7975ef3dce4916ef843d29d51420..1cf4f48e192c33afc7d9fb3d731c5a0f4de94946 100644
--- a/lib/filterClasses.js
+++ b/lib/filterClasses.js
@@ -87,7 +87,7 @@ Filter.knownFilters = new Map();
* Regular expression that element hiding filters should match
* @type {RegExp}
*/
-Filter.elemhideRegExp = /^([^/*|@"!]*?)#([@?])?#(.+)$/;
+Filter.elemhideRegExp = /^([^/*|@"!]*?)(#([@?])?#)(.+)$/;
/**
* Regular expression that RegExp filters specified as RegExps should match
* @type {RegExp}
@@ -97,12 +97,12 @@ Filter.regexpRegExp = /^(@@)?\/.*\/(?:\$~?[\w-]+(?:=[^,\s]+)?(?:,~?[\w-]+(?:=[^,
* Regular expression that options on a RegExp filter should match
* @type {RegExp}
*/
-Filter.optionsRegExp = /\$(~?[\w-]+(?:=[^,\s]+)?(?:,~?[\w-]+(?:=[^,\s]+)?)*)$/;
+Filter.optionsRegExp = /\$(~?[\w-]+(?:=[^,]+)?(?:,~?[\w-]+(?:=[^,]+)?)*)$/;
/**
* Creates a filter of correct type from its text representation - does the
- * basic parsing and calls the right constructor then.
- *
+ * basic normalisation and parsing where possible before deferring to the right
Manish Jethani 2018/02/05 16:09:57 I noticed that the documentation is now using the
kzar 2018/02/13 11:56:46 Kind of ironic heh, Done.
+ * constructor.
* @param {string} text as in Filter()
* @return {Filter}
*/
@@ -115,20 +115,25 @@ Filter.fromText = function(text)
let match = (text.includes("#") ? Filter.elemhideRegExp.exec(text) : null);
if (match)
{
+ let [, domain, seperator, type, selector] = match;
+
+ domain = domain.replace(/\s/g, "");
+ selector = selector.trim();
+
let propsMatch;
- if (!match[2] &&
- (propsMatch = /\[-abp-properties=(["'])([^"']+)\1\]/.exec(match[3])))
+ if (!type &&
+ (propsMatch = /\[-abp-properties=(["'])([^"']+)\1\]/.exec(selector)))
{
// This is legacy CSS properties syntax, convert to current syntax
- let prefix = match[3].substr(0, propsMatch.index);
+ let prefix = selector.substr(0, propsMatch.index);
let expression = propsMatch[2];
- let suffix = match[3].substr(propsMatch.index + propsMatch[0].length);
- return Filter.fromText(`${match[1]}#?#` +
+ let suffix = selector.substr(propsMatch.index + propsMatch[0].length);
+ return Filter.fromText(`${domain}#?#` +
`${prefix}:-abp-properties(${expression})${suffix}`);
}
filter = ElemHideBase.fromText(
- text, match[1], match[2], match[3]
+ domain + seperator + selector, domain, type, selector
);
}
else if (text[0] == "!")
@@ -162,32 +167,16 @@ Filter.fromObject = function(obj)
};
/**
- * Removes unnecessary whitespaces from filter text, will only return null if
- * the input parameter is null.
+ * Strip linebreaks etc and then trim whitespace from the filter text.
+ * Will only return null if the input parameter is null.
* @param {string} text
* @return {string}
*/
-Filter.normalize = function(text)
+Filter.stripJunk = function(text)
{
if (!text)
return text;
-
- // Remove line breaks and such
- text = text.replace(/[^\S ]/g, "");
-
- if (/^\s*!/.test(text))
- {
- // Don't remove spaces inside comments
- return text.trim();
- }
- else if (Filter.elemhideRegExp.test(text))
- {
- // Special treatment for element hiding filters, right side is allowed to
- // contain spaces
- let [, domain, separator, selector] = /^(.*?)(#@?#?)(.*)$/.exec(text);
- return domain.replace(/\s/g, "") + separator + selector.trim();
- }
- return text.replace(/\s/g, "");
+ return text.replace(/[^\S ]/g, "").trim();
};
/**
@@ -713,34 +702,33 @@ Object.defineProperty(RegExpFilter.prototype, "0", {
*/
RegExpFilter.fromText = function(text)
{
- let blocking = true;
- let origText = text;
- if (text.indexOf("@@") == 0)
- {
- blocking = false;
- text = text.substr(2);
- }
-
let contentType = null;
let matchCase = null;
let domains = null;
let sitekeys = null;
let thirdParty = null;
let collapse = null;
- let options;
+ let origText;
Manish Jethani 2018/02/05 16:09:58 Why not initialize origText to null for consistenc
kzar 2018/02/13 11:56:46 Done.
+
let match = (text.indexOf("$") >= 0 ? Filter.optionsRegExp.exec(text) : null);
- if (match)
+ if (!match)
{
- options = match[1].toUpperCase().split(",");
- text = match.input.substr(0, match.index);
- for (let option of options)
+ origText = text = text.replace(/\s/g, "");
+ }
+ else
+ {
+ text = match.input.substring(0, match.index).replace(/\s/g, "");
+ let options = match[1].replace(/\s/g, "");
Manish Jethani 2018/02/05 16:09:58 I don't get this part. Doesn't this mean that "$cs
Manish Jethani 2018/02/06 05:14:39 I see, so the other patch builds on this. Got it.
kzar 2018/02/13 11:56:46 Sorry I should have left a comment on the review h
+ origText = text + "$" + options;
+
+ for (let option of options.toUpperCase().split(","))
{
let value = null;
let separatorIndex = option.indexOf("=");
if (separatorIndex >= 0)
{
value = option.substr(separatorIndex + 1);
- option = option.substr(0, separatorIndex);
+ option = option.substring(0, separatorIndex);
Manish Jethani 2018/02/05 16:09:58 Any particular reason this is now using String.sub
kzar 2018/02/13 11:56:47 Well since I noticed that `substr` was being used
Manish Jethani 2018/02/20 16:00:57 Why is it a mistake to use substr? Anyway, the ch
}
option = option.replace(/-/, "_");
if (option in RegExpFilter.typeMap)
@@ -778,13 +766,13 @@ RegExpFilter.fromText = function(text)
try
{
- if (blocking)
+ if (text.indexOf("@@") == 0)
{
- return new BlockingFilter(origText, text, contentType, matchCase, domains,
- thirdParty, sitekeys, collapse);
+ return new WhitelistFilter(origText, text.substr(2), contentType,
+ matchCase, domains, thirdParty, sitekeys);
}
- return new WhitelistFilter(origText, text, contentType, matchCase, domains,
- thirdParty, sitekeys);
+ return new BlockingFilter(origText, text, contentType, matchCase, domains,
+ thirdParty, sitekeys, collapse);
}
catch (e)
{
« no previous file with comments | « no previous file | lib/synchronizer.js » ('j') | lib/synchronizer.js » ('J')

Powered by Google App Engine
This is Rietveld