Issue 29760707: Issue 6622 - Implement $rewrite filter option (Closed)
|
Created:
April 24, 2018, 8:33 p.m. by hub Modified:
May 18, 2018, 4:38 p.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionIssue 6622 - Implement $rewrite filter option
Patch Set 1 #
Total comments: 3
Patch Set 2 : Filter rewrite is in core. Some cleanup #Patch Set 3 : Refactor the notification code. #
Total comments: 16
Patch Set 4 : Simplified the redirect logic. #
Total comments: 2
Patch Set 5 : Remove uneeded check #
Total comments: 5
Patch Set 6 : No longer have a "REWRITE" type in devtools. Log the rewritten to URL. #
Total comments: 1
Patch Set 7 : Fix a linting error. #
Total comments: 10
Patch Set 8 : Log all request, even failed rewrites. + comment #
Total comments: 9
Patch Set 9 : Some comments adjustments. #Patch Set 10 : Rebased #
Total comments: 2
Patch Set 11 : rename rewritten to rewrittenTo for consistency? #
Total comments: 2
Patch Set 12 : comment fixes #
Total comments: 6
Patch Set 13 : Rename rewrittenTo to rewrittenUrl. Adjust some comments. #
Total comments: 4
Patch Set 14 : Updated proper revision for adblockpluscore. No dependency change for ui #MessagesTotal messages: 43
https://codereview.adblockplus.org/29760707/diff/29760708/dependencies File dependencies (right): https://codereview.adblockplus.org/29760707/diff/29760708/dependencies#newcode4 dependencies:4: adblockpluscore = adblockpluscore hg:10e78c931816 git:8508af0 This need to be synchronized with issue 6592.
https://codereview.adblockplus.org/29760707/diff/29760708/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29760708/lib/requestBlocker.... lib/requestBlocker.js:203: let rewritten = filter.rewrite.replace("%1", matches[0]); I actually wonder if we should move the rewrite part to adblockpluscore in the filter class to totally self contain it in adblockpluscore.
The substitution pattern is different now. It is $1 (but this is only visible in core) https://codereview.adblockplus.org/29760707/diff/29760708/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29760708/lib/requestBlocker.... lib/requestBlocker.js:203: let rewritten = filter.rewrite.replace("%1", matches[0]); On 2018/04/24 20:39:05, hub wrote: > I actually wonder if we should move the rewrite part to adblockpluscore in the > filter class to totally self contain it in adblockpluscore. moved this to core in the end.
https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.... lib/requestBlocker.js:204: if (filter instanceof BlockingFilter && filter.rewrite) I think maybe rewrite should be a separate module in the extension, similar to lib/csp.js. REWRITE filters should be looked up specifically, and only if the URL isn't already blocked (so the onBeforeRequest handler for rewrite filters should be added afterwards).
https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.... lib/requestBlocker.js:204: if (filter instanceof BlockingFilter && filter.rewrite) On 2018/04/30 20:24:58, Manish Jethani wrote: > I think maybe rewrite should be a separate module in the extension, similar to > lib/csp.js. REWRITE filters should be looked up specifically, and only if the > URL isn't already blocked (so the onBeforeRequest handler for rewrite filters > should be added afterwards). This is a blocking filter with the rewrite option. Doing as you suggest would require another call to matchRequest() which is IMHO costly (but necessary).
https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.... lib/requestBlocker.js:73: // POPUP, CSP, REWRITE and ELEMHIDE filters aren't mapped to resource types. Nit: As this list growth, listing those filter options redundantly in the comment doesn't seem to add much value. How about changing the comment to "These filter types aren't mapped to resource types."? https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.... lib/requestBlocker.js:204: if (filter instanceof BlockingFilter && filter.rewrite) On 2018/05/01 18:32:36, hub wrote: > On 2018/04/30 20:24:58, Manish Jethani wrote: > > I think maybe rewrite should be a separate module in the extension, similar to > > lib/csp.js. REWRITE filters should be looked up specifically, and only if the > > URL isn't already blocked (so the onBeforeRequest handler for rewrite filters > > should be added afterwards). > > This is a blocking filter with the rewrite option. Doing as you suggest would > require another call to matchRequest() which is IMHO costly (but necessary). I agree that having the logic here is fine. Otherwise, we have to run some logic (whitelisting checks, etc.) twice on every request. In case of CSP, this is less an issue since the onBeforeRequest handler there is only registered for requests of the type main_frame and sub_frame, besides the logic there diverges even more from what we are doing here. https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.... lib/requestBlocker.js:217: log(type); I'd rather move the logic around so that we don't need that log() function. getRelatedTabIds(details).then(tabIds => { logRequest(tabIds, urlString, type, docDomain, thirdParty, sitekey, specificOnly, filter); }); if (filter instanceof BlockingFilter) { if (filter.rewrite) { ... } return {cancel: true}; }
https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.... lib/requestBlocker.js:204: if (filter instanceof BlockingFilter && filter.rewrite) On 2018/05/02 15:04:59, Sebastian Noack wrote: > On 2018/05/01 18:32:36, hub wrote: > > On 2018/04/30 20:24:58, Manish Jethani wrote: > > > I think maybe rewrite should be a separate module in the extension, similar > to > > > lib/csp.js. REWRITE filters should be looked up specifically, and only if > the > > > URL isn't already blocked (so the onBeforeRequest handler for rewrite > filters > > > should be added afterwards). > > > > This is a blocking filter with the rewrite option. Doing as you suggest would > > require another call to matchRequest() which is IMHO costly (but necessary). > > I agree that having the logic here is fine. Otherwise, we have to run some logic > (whitelisting checks, etc.) twice on every request. In case of CSP, this is less > an issue since the onBeforeRequest handler there is only registered for requests > of the type main_frame and sub_frame, besides the logic there diverges even more > from what we are doing here. Suppose there are two filters: /([^/]+\/)?bar$/$rewrite=lambda /foo/ The URL https://example.com/foo/bar would get rewritten to https://example.com/lambda instead of getting blocked. Is this OK? There's no way to add an exception based on the URL itself. Furthermore, because filters are sorted in random order (see lib/filterListener.js), it will not be possible to predict whether the URL will get rewritten or blocked. This is not going to be fun.
https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.... lib/requestBlocker.js:204: if (filter instanceof BlockingFilter && filter.rewrite) On 2018/05/02 16:13:45, Manish Jethani wrote: > On 2018/05/02 15:04:59, Sebastian Noack wrote: > > On 2018/05/01 18:32:36, hub wrote: > > > On 2018/04/30 20:24:58, Manish Jethani wrote: > > > > I think maybe rewrite should be a separate module in the extension, > similar > > to > > > > lib/csp.js. REWRITE filters should be looked up specifically, and only if > > the > > > > URL isn't already blocked (so the onBeforeRequest handler for rewrite > > filters > > > > should be added afterwards). > > > > > > This is a blocking filter with the rewrite option. Doing as you suggest > would > > > require another call to matchRequest() which is IMHO costly (but necessary). > > > > I agree that having the logic here is fine. Otherwise, we have to run some > logic > > (whitelisting checks, etc.) twice on every request. In case of CSP, this is > less > > an issue since the onBeforeRequest handler there is only registered for > requests > > of the type main_frame and sub_frame, besides the logic there diverges even > more > > from what we are doing here. > > Suppose there are two filters: > > /([^/]+\/)?bar$/$rewrite=lambda > /foo/ > > The URL https://example.com/foo/bar would get rewritten to > https://example.com/lambda instead of getting blocked. Is this OK? There's no > way to add an exception based on the URL itself. > > Furthermore, because filters are sorted in random order (see > lib/filterListener.js), it will not be possible to predict whether the URL will > get rewritten or blocked. This is not going to be fun. If we want to give precedence to blocking filters over $rewrite filters, we could do another filter match without REWRITE in the type mask if we found a $rewrite filter here. Still better than running the whole logic again in another onBeforeRequest listener. But I'm inclined to just accept undefined behavior in case of conflicting $rewrite and blocking filters, for now (in favor of a simpler and more efficient implementation). I wouldn't insist though. Dave, what do you think?
https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.... lib/requestBlocker.js:204: if (filter instanceof BlockingFilter && filter.rewrite) On 2018/05/02 16:40:09, Sebastian Noack wrote: > On 2018/05/02 16:13:45, Manish Jethani wrote: > > On 2018/05/02 15:04:59, Sebastian Noack wrote: > > > On 2018/05/01 18:32:36, hub wrote: > > > > On 2018/04/30 20:24:58, Manish Jethani wrote: > > > > > I think maybe rewrite should be a separate module in the extension, > > similar > > > to > > > > > lib/csp.js. REWRITE filters should be looked up specifically, and only > if > > > the > > > > > URL isn't already blocked (so the onBeforeRequest handler for rewrite > > > filters > > > > > should be added afterwards). > > > > > > > > This is a blocking filter with the rewrite option. Doing as you suggest > > would > > > > require another call to matchRequest() which is IMHO costly (but > necessary). > > > > > > I agree that having the logic here is fine. Otherwise, we have to run some > > logic > > > (whitelisting checks, etc.) twice on every request. In case of CSP, this is > > less > > > an issue since the onBeforeRequest handler there is only registered for > > requests > > > of the type main_frame and sub_frame, besides the logic there diverges even > > more > > > from what we are doing here. > > > > Suppose there are two filters: > > > > /([^/]+\/)?bar$/$rewrite=lambda > > /foo/ > > > > The URL https://example.com/foo/bar would get rewritten to > > https://example.com/lambda instead of getting blocked. Is this OK? There's no > > way to add an exception based on the URL itself. > > > > Furthermore, because filters are sorted in random order (see > > lib/filterListener.js), it will not be possible to predict whether the URL > will > > get rewritten or blocked. This is not going to be fun. > > If we want to give precedence to blocking filters over $rewrite filters, we > could do another filter match without REWRITE in the type mask if we found a > $rewrite filter here. Still better than running the whole logic again in another > onBeforeRequest listener. Yeah, I think that's fair. I think defaultMatcher.matchesAny will have to take another argument called "rewrites": if it's true, the matcher will return a rewrite filter or null, otherwise a non-rewrite filter or null. Like in the _checkEntryMatch function: _checkEntryMatch(keyword, location, typeMask, docDomain, thirdParty, sitekey, specificOnly, rewrites = false) { ... if (!!filter.rewrite == rewrites && filter.matches(location, typeMask, docDomain, thirdParty, sitekey)) { return filter; } ... } Maybe the values in filterByKeyword should be two lists (rewrite and non-rewrite) instead of a single list.
https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.... lib/requestBlocker.js:73: // POPUP, CSP, REWRITE and ELEMHIDE filters aren't mapped to resource types. On 2018/05/02 15:04:59, Sebastian Noack wrote: > Nit: As this list growth, listing those filter options redundantly in the > comment doesn't seem to add much value. How about changing the comment to "These > filter types aren't mapped to resource types."? Done. https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.... lib/requestBlocker.js:204: if (filter instanceof BlockingFilter && filter.rewrite) On 2018/05/02 17:45:50, Manish Jethani wrote: > On 2018/05/02 16:40:09, Sebastian Noack wrote: > > On 2018/05/02 16:13:45, Manish Jethani wrote: > > > On 2018/05/02 15:04:59, Sebastian Noack wrote: > > > > On 2018/05/01 18:32:36, hub wrote: > > > > > On 2018/04/30 20:24:58, Manish Jethani wrote: > > > > > > I think maybe rewrite should be a separate module in the extension, > > > similar > > > > to > > > > > > lib/csp.js. REWRITE filters should be looked up specifically, and only > > if > > > > the > > > > > > URL isn't already blocked (so the onBeforeRequest handler for rewrite > > > > filters > > > > > > should be added afterwards). > > > > > > > > > > This is a blocking filter with the rewrite option. Doing as you suggest > > > would > > > > > require another call to matchRequest() which is IMHO costly (but > > necessary). > > > > > > > > I agree that having the logic here is fine. Otherwise, we have to run some > > > logic > > > > (whitelisting checks, etc.) twice on every request. In case of CSP, this > is > > > less > > > > an issue since the onBeforeRequest handler there is only registered for > > > requests > > > > of the type main_frame and sub_frame, besides the logic there diverges > even > > > more > > > > from what we are doing here. > > > > > > Suppose there are two filters: > > > > > > /([^/]+\/)?bar$/$rewrite=lambda > > > /foo/ > > > > > > The URL https://example.com/foo/bar would get rewritten to > > > https://example.com/lambda instead of getting blocked. Is this OK? There's > no > > > way to add an exception based on the URL itself. > > > > > > Furthermore, because filters are sorted in random order (see > > > lib/filterListener.js), it will not be possible to predict whether the URL > > will > > > get rewritten or blocked. This is not going to be fun. > > > > If we want to give precedence to blocking filters over $rewrite filters, we > > could do another filter match without REWRITE in the type mask if we found a > > $rewrite filter here. Still better than running the whole logic again in > another > > onBeforeRequest listener. > > Yeah, I think that's fair. > > I think defaultMatcher.matchesAny will have to take another argument called > "rewrites": if it's true, the matcher will return a rewrite filter or null, > otherwise a non-rewrite filter or null. > > Like in the _checkEntryMatch function: > > _checkEntryMatch(keyword, location, typeMask, docDomain, thirdParty, sitekey, > specificOnly, rewrites = false) > { > ... > > if (!!filter.rewrite == rewrites && > filter.matches(location, typeMask, docDomain, thirdParty, sitekey)) > { > return filter; > } > > ... > } > > Maybe the values in filterByKeyword should be two lists (rewrite and > non-rewrite) instead of a single list. I'm trying to actually keep it simple here. https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.... lib/requestBlocker.js:217: log(type); On 2018/05/02 15:04:59, Sebastian Noack wrote: > I'd rather move the logic around so that we don't need that log() function. > > getRelatedTabIds(details).then(tabIds => > { > logRequest(tabIds, urlString, type, docDomain, > thirdParty, sitekey, specificOnly, filter); > }); > > if (filter instanceof BlockingFilter) > { > if (filter.rewrite) > { > ... > } > > return {cancel: true}; > } Done in a different way.
https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.... lib/requestBlocker.js:204: if (filter instanceof BlockingFilter && filter.rewrite) On 2018/05/03 01:18:02, hub wrote: > On 2018/05/02 17:45:50, Manish Jethani wrote: > > On 2018/05/02 16:40:09, Sebastian Noack wrote: > > > On 2018/05/02 16:13:45, Manish Jethani wrote: > > > > On 2018/05/02 15:04:59, Sebastian Noack wrote: > > > > > On 2018/05/01 18:32:36, hub wrote: > > > > > > On 2018/04/30 20:24:58, Manish Jethani wrote: > > > > > > > I think maybe rewrite should be a separate module in the extension, > > > > similar > > > > > to > > > > > > > lib/csp.js. REWRITE filters should be looked up specifically, and > only > > > if > > > > > the > > > > > > > URL isn't already blocked (so the onBeforeRequest handler for > rewrite > > > > > filters > > > > > > > should be added afterwards). > > > > > > > > > > > > This is a blocking filter with the rewrite option. Doing as you > suggest > > > > would > > > > > > require another call to matchRequest() which is IMHO costly (but > > > necessary). > > > > > > > > > > I agree that having the logic here is fine. Otherwise, we have to run > some > > > > logic > > > > > (whitelisting checks, etc.) twice on every request. In case of CSP, this > > is > > > > less > > > > > an issue since the onBeforeRequest handler there is only registered for > > > > requests > > > > > of the type main_frame and sub_frame, besides the logic there diverges > > even > > > > more > > > > > from what we are doing here. > > > > > > > > Suppose there are two filters: > > > > > > > > /([^/]+\/)?bar$/$rewrite=lambda > > > > /foo/ > > > > > > > > The URL https://example.com/foo/bar would get rewritten to > > > > https://example.com/lambda instead of getting blocked. Is this OK? There's > > no > > > > way to add an exception based on the URL itself. > > > > > > > > Furthermore, because filters are sorted in random order (see > > > > lib/filterListener.js), it will not be possible to predict whether the URL > > > will > > > > get rewritten or blocked. This is not going to be fun. > > > > > > If we want to give precedence to blocking filters over $rewrite filters, we > > > could do another filter match without REWRITE in the type mask if we found a > > > $rewrite filter here. Still better than running the whole logic again in > > another > > > onBeforeRequest listener. > > > > Yeah, I think that's fair. > > > > I think defaultMatcher.matchesAny will have to take another argument called > > "rewrites": if it's true, the matcher will return a rewrite filter or null, > > otherwise a non-rewrite filter or null. > > > > Like in the _checkEntryMatch function: > > > > _checkEntryMatch(keyword, location, typeMask, docDomain, thirdParty, > sitekey, > > specificOnly, rewrites = false) > > { > > ... > > > > if (!!filter.rewrite == rewrites && > > filter.matches(location, typeMask, docDomain, thirdParty, sitekey)) > > { > > return filter; > > } > > > > ... > > } > > > > Maybe the values in filterByKeyword should be two lists (rewrite and > > non-rewrite) instead of a single list. > > I'm trying to actually keep it simple here. I'm OK with letting the filters with a rewrite option compete with other filters, but it will be difficult to debug (especially for filter authors) when a rewrite filter doesn't work sometimes (and only sometimes). But yeah in the interest of keeping things simple I'm OK with this. https://codereview.adblockplus.org/29760707/diff/29768693/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29768693/lib/requestBlocker.... lib/requestBlocker.js:205: if (!rewritten || rewritten == urlString) This should probably be updated now, since rewriteUrl never returns a falsey value. Actually we know at this point that filter.regexp matches, this means the rewrite is happening even if the result is the same as the original URL. That's OK, we can just remove this check here entirely.
https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.... lib/requestBlocker.js:204: if (filter instanceof BlockingFilter && filter.rewrite) On 2018/05/02 17:45:50, Manish Jethani wrote: > I think defaultMatcher.matchesAny will have to take another argument called > "rewrites": if it's true, the matcher will return a rewrite filter or null, > otherwise a non-rewrite filter or null. I don't quite get what we need another argument for. Can't we control through the typeMask whether $rewrite filters should be considered or not?
Updated patch https://codereview.adblockplus.org/29760707/diff/29768693/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29768693/lib/requestBlocker.... lib/requestBlocker.js:205: if (!rewritten || rewritten == urlString) On 2018/05/03 02:51:16, Manish Jethani wrote: > This should probably be updated now, since rewriteUrl never returns a falsey > value. Actually we know at this point that filter.regexp matches, this means the > rewrite is happening even if the result is the same as the original URL. That's > OK, we can just remove this check here entirely. Done.
https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.... lib/requestBlocker.js:204: if (filter instanceof BlockingFilter && filter.rewrite) On 2018/05/03 15:55:47, Sebastian Noack wrote: > On 2018/05/02 17:45:50, Manish Jethani wrote: > > I think defaultMatcher.matchesAny will have to take another argument called > > "rewrites": if it's true, the matcher will return a rewrite filter or null, > > otherwise a non-rewrite filter or null. > > I don't quite get what we need another argument for. Can't we control through > the typeMask whether $rewrite filters should be considered or not? If the typeMask doesn't contain REWRITE it'll still match on other bases, like SCRIPT or STYLESHEET. If we want to say explicitly that it shouldn't match rewrite filters we need an additional parameter. For example a filter may be $script,rewrite=$1, it'll match any script requests.
https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.... lib/requestBlocker.js:204: if (filter instanceof BlockingFilter && filter.rewrite) On 2018/05/04 10:01:59, Manish Jethani wrote: > On 2018/05/03 15:55:47, Sebastian Noack wrote: > > On 2018/05/02 17:45:50, Manish Jethani wrote: > > > I think defaultMatcher.matchesAny will have to take another argument called > > > "rewrites": if it's true, the matcher will return a rewrite filter or null, > > > otherwise a non-rewrite filter or null. > > > > I don't quite get what we need another argument for. Can't we control through > > the typeMask whether $rewrite filters should be considered or not? > > If the typeMask doesn't contain REWRITE it'll still match on other bases, like > SCRIPT or STYLESHEET. If we want to say explicitly that it shouldn't match > rewrite filters we need an additional parameter. > > For example a filter may be $script,rewrite=$1, it'll match any script requests. Gotya. In fact REWRITE isn't even a flag in the type mask which makes sense. So yeah, if we want to enable filter matches ignoring $rewrite filters, in order to give precedence to blocking filters, we'd need a new argument in matchesAny(). But I'm still inclined to keep things simple for now, and accept/document undefined behavior in case of conflicting filters. If this should become a problem in the future we can revisit it then.
https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29762749/lib/requestBlocker.... lib/requestBlocker.js:204: if (filter instanceof BlockingFilter && filter.rewrite) On 2018/05/04 15:17:16, Sebastian Noack wrote: > On 2018/05/04 10:01:59, Manish Jethani wrote: > > On 2018/05/03 15:55:47, Sebastian Noack wrote: > > > On 2018/05/02 17:45:50, Manish Jethani wrote: > > > > I think defaultMatcher.matchesAny will have to take another argument > called > > > > "rewrites": if it's true, the matcher will return a rewrite filter or > null, > > > > otherwise a non-rewrite filter or null. > > > > > > I don't quite get what we need another argument for. Can't we control > through > > > the typeMask whether $rewrite filters should be considered or not? > > > > If the typeMask doesn't contain REWRITE it'll still match on other bases, like > > SCRIPT or STYLESHEET. If we want to say explicitly that it shouldn't match > > rewrite filters we need an additional parameter. > > > > For example a filter may be $script,rewrite=$1, it'll match any script > requests. > > Gotya. In fact REWRITE isn't even a flag in the type mask which makes sense. So > yeah, if we want to enable filter matches ignoring $rewrite filters, in order to > give precedence to blocking filters, we'd need a new argument in matchesAny(). > But I'm still inclined to keep things simple for now, and accept/document > undefined behavior in case of conflicting filters. If this should become a > problem in the future we can revisit it then. Makes sense. https://codereview.adblockplus.org/29760707/diff/29769599/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29769599/lib/requestBlocker.... lib/requestBlocker.js:205: if (rewritten == urlString) Do we still need this check? If the filter matched there was a rewrite, even if the result was the same. https://codereview.adblockplus.org/29760707/diff/29769599/lib/requestBlocker.... lib/requestBlocker.js:209: type = "REWRITE"; Also I'm wondering if it makes sense to set the type here. The original type may be "SCRIPT" and now we're overwriting it with "REWRITE", but we don't provide information on what it was rewritten to. Technically we did block the original script, we just redirected it to a new resource. I'm wondering if I would prefer to see "SCRIPT" rather than "REWRITE" in the DevTools panel in this case.
https://codereview.adblockplus.org/29760707/diff/29769599/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29769599/lib/requestBlocker.... lib/requestBlocker.js:209: type = "REWRITE"; On 2018/05/04 15:37:47, Manish Jethani wrote: > Also I'm wondering if it makes sense to set the type here. The original type may > be "SCRIPT" and now we're overwriting it with "REWRITE", but we don't provide > information on what it was rewritten to. Technically we did block the original > script, we just redirected it to a new resource. I'm wondering if I would prefer > to see "SCRIPT" rather than "REWRITE" in the DevTools panel in this case. I already thought the same, and tend to agree to not indicate requests matching a $rewrite filter with a type of REWRITE. This would be a leaky abstraction. But then we need a different way to indicate $rewrite filters in the devtools panel.
Updated patch. https://codereview.adblockplus.org/29760707/diff/29769599/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29769599/lib/requestBlocker.... lib/requestBlocker.js:205: if (rewritten == urlString) On 2018/05/04 15:37:47, Manish Jethani wrote: > Do we still need this check? If the filter matched there was a rewrite, even if > the result was the same. definitely. a redirectUrl will cause onBeforeRequest to be called again - which will likely be an infinite loop until the browser cancel it. By just doing a return we say "let's go". https://codereview.adblockplus.org/29760707/diff/29769599/lib/requestBlocker.... lib/requestBlocker.js:209: type = "REWRITE"; On 2018/05/04 15:46:49, Sebastian Noack wrote: > On 2018/05/04 15:37:47, Manish Jethani wrote: > > Also I'm wondering if it makes sense to set the type here. The original type > may > > be "SCRIPT" and now we're overwriting it with "REWRITE", but we don't provide > > information on what it was rewritten to. Technically we did block the original > > script, we just redirected it to a new resource. I'm wondering if I would > prefer > > to see "SCRIPT" rather than "REWRITE" in the DevTools panel in this case. > > I already thought the same, and tend to agree to not indicate requests matching > a $rewrite filter with a type of REWRITE. This would be a leaky abstraction. But > then we need a different way to indicate $rewrite filters in the devtools panel. I have a solution to indicate the rewrite in the devtools panel. It requires a change in adblockplusui (for which I have a patch) to actually display it. But then I wanted to be able to filter REWRITE explicitly. Can't have both the type and REWRITE in the filter though. Not sure there is an easy way without an overhaul of the logging. If you think we don't need to filter REWRITE, then I'm fine. It will simplify this part. https://codereview.adblockplus.org/29760707/diff/29770578/dependencies File dependencies (right): https://codereview.adblockplus.org/29760707/diff/29770578/dependencies#newcode5 dependencies:5: adblockplusui = adblockplusui hg:31100977a122 git:dcb681f not mandatory (blocking) to run, but nice to have.
UI code changes are at https://codereview.adblockplus.org/29770586/
https://codereview.adblockplus.org/29760707/diff/29770582/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29770582/lib/requestBlocker.... lib/requestBlocker.js:206: // we couldn't do the rewrite, so just let it through. The request should still be logged. Just for clarification, bailing here is necessary because the onBeforeRequest listener would be called infinite times if we keep redirecting to the same URL?
https://codereview.adblockplus.org/29760707/diff/29770582/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29770582/lib/requestBlocker.... lib/requestBlocker.js:206: // we couldn't do the rewrite, so just let it through. On 2018/05/04 19:46:36, Sebastian Noack wrote: > The request should still be logged. Agreed. > Just for clarification, bailing here is necessary because the onBeforeRequest > listener would be called infinite times if we keep redirecting to the same URL? I checked on both the latest versions of Chrome and Firefox. If the redirect URL is the same as the current URL, Chrome will do nothing (simply let the URL through) while Firefox will actually do a redirection to the same URL, but only a fixed number of times. Naturally the number is 42.
https://codereview.adblockplus.org/29760707/diff/29770582/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29770582/lib/requestBlocker.... lib/requestBlocker.js:206: // we couldn't do the rewrite, so just let it through. On 2018/05/04 20:06:12, Manish Jethani wrote: > On 2018/05/04 19:46:36, Sebastian Noack wrote: > > The request should still be logged. > > Agreed. > > > Just for clarification, bailing here is necessary because the onBeforeRequest > > listener would be called infinite times if we keep redirecting to the same > URL? > > I checked on both the latest versions of Chrome and Firefox. If the redirect URL > is the same as the current URL, Chrome will do nothing (simply let the URL > through) while Firefox will actually do a redirection to the same URL, but only > a fixed number of times. > > Naturally the number is 42. To be clear, I'm OK with the check here even though it would be unusual in practice to come up with a rewrite that doesn't change anything. I could see it happening with, for example, "/(?foo=)\d+/$rewrite=$10" if the matched substring is already "?foo=0" (which it will be after the first rewrite). If we're going to add the check then I think we should also not log the hit, because otherwise in the preceding example we would have two entries, one for "?foo=123" and one for "?foo=0". Also in some cases the rewrite will fail because of the same-origin policy we have. We could update the comment to something like: // Do not redirect to the same URL, lest we end up in a potentially infinite redirect loop.
https://codereview.adblockplus.org/29760707/diff/29770582/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29770582/lib/requestBlocker.... lib/requestBlocker.js:206: // we couldn't do the rewrite, so just let it through. On 2018/05/04 20:06:12, Manish Jethani wrote: > On 2018/05/04 19:46:36, Sebastian Noack wrote: > > The request should still be logged. > > Agreed. > > > Just for clarification, bailing here is necessary because the onBeforeRequest > > listener would be called infinite times if we keep redirecting to the same > URL? > > I checked on both the latest versions of Chrome and Firefox. If the redirect URL > is the same as the current URL, Chrome will do nothing (simply let the URL > through) while Firefox will actually do a redirection to the same URL, but only > a fixed number of times. > > Naturally the number is 42. Last time I checked, Chrome (66) did that too. https://codereview.adblockplus.org/29760707/diff/29770582/lib/requestBlocker.... lib/requestBlocker.js:206: // we couldn't do the rewrite, so just let it through. On 2018/05/04 19:46:36, Sebastian Noack wrote: > The request should still be logged. > > Just for clarification, bailing here is necessary because the onBeforeRequest > listener would be called infinite times if we keep redirecting to the same URL? Ok, I'll log. https://codereview.adblockplus.org/29760707/diff/29770582/lib/requestBlocker.... lib/requestBlocker.js:206: // we couldn't do the rewrite, so just let it through. > // Do not redirect to the same URL, lest we end up in a potentially infinite > redirect loop. Done this.
https://codereview.adblockplus.org/29760707/diff/29770582/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29770582/lib/requestBlocker.... lib/requestBlocker.js:204: let rewritten = filter.rewriteUrl(urlString); We don't need rewritten, we can just assign rewrittenTo here. https://codereview.adblockplus.org/29760707/diff/29770582/lib/requestBlocker.... lib/requestBlocker.js:205: if (rewritten == urlString) Nit: since the if block spans multiple lines, we should add braces around it.
https://codereview.adblockplus.org/29760707/diff/29770582/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29770582/lib/requestBlocker.... lib/requestBlocker.js:204: let rewritten = filter.rewriteUrl(urlString); On 2018/05/04 20:49:47, Manish Jethani wrote: > We don't need rewritten, we can just assign rewrittenTo here. already done https://codereview.adblockplus.org/29760707/diff/29770582/lib/requestBlocker.... lib/requestBlocker.js:205: if (rewritten == urlString) On 2018/05/04 20:49:47, Manish Jethani wrote: > Nit: since the if block spans multiple lines, we should add braces around it. Acknowledged.
https://codereview.adblockplus.org/29760707/diff/29770597/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29770597/lib/requestBlocker.... lib/requestBlocker.js:217: thirdParty, sitekey, specificOnly, filter, rewritten); For what it's worth, I think we should do the logging part as a separate patch. For now we can just log it as a regular blocked request.
https://codereview.adblockplus.org/29760707/diff/29770597/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29770597/lib/requestBlocker.... lib/requestBlocker.js:198: let rewritten; Nit: Since these variables are no always assigned below, we might want to initialize them here, to make the behavior more explicit, and for consistency since we never return or pass along uninitialized variables anywhere. https://codereview.adblockplus.org/29760707/diff/29770597/lib/requestBlocker.... lib/requestBlocker.js:205: // if no rewrite happened (error, diff origin), we'll Nit: Capitalize first letter of comment (if it is a sentence). https://codereview.adblockplus.org/29760707/diff/29770597/lib/requestBlocker.... lib/requestBlocker.js:206: // return undefined avoid an "infinite" loop. Typo: ... in order to avoid ... https://codereview.adblockplus.org/29760707/diff/29770597/lib/requestBlocker.... lib/requestBlocker.js:217: thirdParty, sitekey, specificOnly, filter, rewritten); On 2018/05/05 00:54:12, Manish Jethani wrote: > For what it's worth, I think we should do the logging part as a separate patch. > For now we can just log it as a regular blocked request. Without the logging changes, I consider the resulting behavior of the devtools panel a regression. So I think we should perform those changes simultaneously.
Updated patch. https://codereview.adblockplus.org/29760707/diff/29770597/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29770597/lib/requestBlocker.... lib/requestBlocker.js:198: let rewritten; On 2018/05/05 13:46:53, Sebastian Noack wrote: > Nit: Since these variables are no always assigned below, we might want to > initialize them here, to make the behavior more explicit, and for consistency > since we never return or pass along uninitialized variables anywhere. In both cases they are undefined which is totally fine. return result that is undefined is what we want if there is nothing to rewrite and if we don't block. https://codereview.adblockplus.org/29760707/diff/29770597/lib/requestBlocker.... lib/requestBlocker.js:205: // if no rewrite happened (error, diff origin), we'll On 2018/05/05 13:46:53, Sebastian Noack wrote: > Nit: Capitalize first letter of comment (if it is a sentence). Done. https://codereview.adblockplus.org/29760707/diff/29770597/lib/requestBlocker.... lib/requestBlocker.js:206: // return undefined avoid an "infinite" loop. On 2018/05/05 13:46:53, Sebastian Noack wrote: > Typo: ... in order to avoid ... Done. https://codereview.adblockplus.org/29760707/diff/29770597/lib/requestBlocker.... lib/requestBlocker.js:217: thirdParty, sitekey, specificOnly, filter, rewritten); On 2018/05/05 13:46:53, Sebastian Noack wrote: > On 2018/05/05 00:54:12, Manish Jethani wrote: > > For what it's worth, I think we should do the logging part as a separate > patch. > > For now we can just log it as a regular blocked request. > > Without the logging changes, I consider the resulting behavior of the devtools > panel a regression. So I think we should perform those changes simultaneously. Without the UI patch, it will just be logged as a blocked request anyway.
This patch needs to be rebased now
Rebased patch.
https://codereview.adblockplus.org/29760707/diff/29777643/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29777643/lib/requestBlocker.... lib/requestBlocker.js:211: sitekey, specificOnly, rewrittenTo: rewritten Nit: How about just calling the variable rewritteTo for consistency?
updated patch https://codereview.adblockplus.org/29760707/diff/29777643/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29777643/lib/requestBlocker.... lib/requestBlocker.js:211: sitekey, specificOnly, rewrittenTo: rewritten On 2018/05/10 15:09:52, Sebastian Noack wrote: > Nit: How about just calling the variable rewritteTo for consistency? Done.
This look good to me modulo the dependency update. https://codereview.adblockplus.org/29760707/diff/29777649/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29777649/lib/requestBlocker.... lib/requestBlocker.js:196: // If no rewrite happened (error, diff origin), we'll Nit: can we spell out "different" here? :) (Sorry about being a stickler)
Patch updated https://codereview.adblockplus.org/29760707/diff/29777649/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29777649/lib/requestBlocker.... lib/requestBlocker.js:196: // If no rewrite happened (error, diff origin), we'll On 2018/05/11 14:11:19, Manish Jethani wrote: > Nit: can we spell out "different" here? :) (Sorry about being a stickler) Done.
https://codereview.adblockplus.org/29760707/diff/29778594/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29778594/lib/requestBlocker.... lib/requestBlocker.js:73: // These filter types aren't mapped to resource types. Nit: Mind removing this unrelated change? https://codereview.adblockplus.org/29760707/diff/29778594/lib/requestBlocker.... lib/requestBlocker.js:189: let rewrittenTo; Nit: I think rewrittenUrl would be a nicer name.
https://codereview.adblockplus.org/29760707/diff/29778594/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29778594/lib/requestBlocker.... lib/requestBlocker.js:189: let rewrittenTo; On 2018/05/15 14:30:29, kzar wrote: > Nit: I think rewrittenUrl would be a nicer name. I'll second that.
https://codereview.adblockplus.org/29760707/diff/29778594/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29778594/lib/requestBlocker.... lib/requestBlocker.js:189: let rewrittenTo; On 2018/05/15 14:30:29, kzar wrote: > Nit: I think rewrittenUrl would be a nicer name. Note that the varialbe name here is consistent with the property name expected by the devtools panel (and therefore also simplifies the object creation below). So if we change it we should also change the name of the property passed to the devtools panel. (However, as far as I'm concerned either name would be fine.)
Updated patch. UI patch updated as well to match. https://codereview.adblockplus.org/29760707/diff/29778594/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29760707/diff/29778594/lib/requestBlocker.... lib/requestBlocker.js:73: // These filter types aren't mapped to resource types. On 2018/05/15 14:30:29, kzar wrote: > Nit: Mind removing this unrelated change? Done. https://codereview.adblockplus.org/29760707/diff/29778594/lib/requestBlocker.... lib/requestBlocker.js:189: let rewrittenTo; On 2018/05/15 14:44:36, Sebastian Noack wrote: > On 2018/05/15 14:30:29, kzar wrote: > > Nit: I think rewrittenUrl would be a nicer name. > > Note that the varialbe name here is consistent with the property name expected > by the devtools panel (and therefore also simplifies the object creation below). > So if we change it we should also change the name of the property passed to the > devtools panel. (However, as far as I'm concerned either name would be fine.) I'll do that.
Can you please save your devbuild announcement draft in Textpattern, so that we can publish it as soon as this change lands? https://codereview.adblockplus.org/29760707/diff/29782620/dependencies File dependencies (right): https://codereview.adblockplus.org/29760707/diff/29782620/dependencies#newcode4 dependencies:4: adblockpluscore = adblockpluscore hg:89490da18380 git:8508af0 This revision seems to be wrong: https://hg.adblockplus.org/adblockpluscore/rev/e59042524857 https://codereview.adblockplus.org/29760707/diff/29782620/dependencies#newcode5 dependencies:5: adblockplusui = adblockplusui hg:ff614a2838a9 git:dcb681f I knew that I previously said otherwise, but given the new workflow in adblockplusui, I don't think anymore that we should wait for the adblockplusui dependency being ready to be updated before landing this. For reference, the related adblockplusui change already landed, not in Mercurial but in a feature branch on GitLab, which is merged back to Mercurial once all UI changes (including translations are ready), i.e. right before code freeze.
patch updated with the proper revision https://codereview.adblockplus.org/29760707/diff/29782620/dependencies File dependencies (right): https://codereview.adblockplus.org/29760707/diff/29782620/dependencies#newcode4 dependencies:4: adblockpluscore = adblockpluscore hg:89490da18380 git:8508af0 On 2018/05/18 15:13:30, Sebastian Noack wrote: > This revision seems to be wrong: > https://hg.adblockplus.org/adblockpluscore/rev/e59042524857 I had the right one locally (both mercurial and git). Will update the patch. https://codereview.adblockplus.org/29760707/diff/29782620/dependencies#newcode5 dependencies:5: adblockplusui = adblockplusui hg:ff614a2838a9 git:dcb681f On 2018/05/18 15:13:30, Sebastian Noack wrote: > I knew that I previously said otherwise, but given the new workflow in > adblockplusui, I don't think anymore that we should wait for the adblockplusui > dependency being ready to be updated before landing this. > > For reference, the related adblockplusui change already landed, not in Mercurial > but in a feature branch on GitLab, which is merged back to Mercurial once all UI > changes (including translations are ready), i.e. right before code freeze. I'll revert this dependency. We'll update it as soon as we can.
LGTM. But don't push before the devbuild announcement is ready to be published in Textpattern. 
|
