Issue 29739603: Issue 6544 - Prevent requests sent by Chrome or Adblock Plus from being blocked (Closed)
|
Created:
April 2, 2018, 3:03 a.m. by Sebastian Noack Modified:
April 12, 2018, 1:33 p.m. Reviewers:
kzar CC:
Manish Jethani Visibility:
Public. |
DescriptionIssue 6544 - Prevent requests sent by Chrome or Adblock Plus from being blocked
Patch Set 1 #
Total comments: 6
Patch Set 2 : Improved comment #Patch Set 3 : Rebased #Patch Set 4 : Also ignore requests sent by Chrome itself #Patch Set 5 : Streamlined request logging #
Total comments: 12
Patch Set 6 : Introduced ignoredOrigins Set #
Total comments: 4
Patch Set 7 : Rebased #Patch Set 8 : Made logic more verbose #MessagesTotal messages: 15
https://codereview.adblockplus.org/29739603/diff/29739604/lib/requestBlocker.js File lib/requestBlocker.js (left): https://codereview.adblockplus.org/29739603/diff/29739604/lib/requestBlocker.... lib/requestBlocker.js:142: let originUrl = null; This diff doesn't seem right, I don't see this stuff that you've removed in lib/requestBlocker.js so far in next or master. Seems it's rather in ext/background.js. Am I missing something? https://codereview.adblockplus.org/29739603/diff/29739604/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29739603/diff/29739604/lib/requestBlocker.... lib/requestBlocker.js:151: // the browser (i.e. chrome://), while Chromium only allows to interecept > Firefox allows to intercept requests sent by all extensions or the browser (i.e. chrome://) Do you mean chrome:// for Firefox? Also how come there's a special case for "chrome:" below when we already figured out the protocol the browser uses for extension pages? > {Firefox,Chromium} allows to intercept requests... Nit: This reads a little funny, perhaps add "us" before "to intercept"?
https://codereview.adblockplus.org/29739603/diff/29739604/lib/requestBlocker.js File lib/requestBlocker.js (left): https://codereview.adblockplus.org/29739603/diff/29739604/lib/requestBlocker.... lib/requestBlocker.js:142: let originUrl = null; On 2018/04/03 12:06:45, kzar wrote: > This diff doesn't seem right, I don't see this stuff that you've removed in > lib/requestBlocker.js so far in next or master. Seems it's rather in > ext/background.js. Am I missing something? Oh I see, it's based on these changes https://codereview.adblockplus.org/29739594/diff/29739595/lib/requestBlocker.js
https://codereview.adblockplus.org/29739603/diff/29739604/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29739603/diff/29739604/lib/requestBlocker.... lib/requestBlocker.js:151: // the browser (i.e. chrome://), while Chromium only allows to interecept On 2018/04/03 12:06:45, kzar wrote: > Do you mean chrome:// for Firefox? Also how come there's a special case for > "chrome:" below when we already figured out the protocol the browser uses for > extension pages? Yes, "chrome:" is the protocol for content that is part of the browser on Firefox. The protocol used for parts of an extension (i.e. extensionProtocol) is "chrome-extension:" and "moz-extension:", repsecively on Chrome and Firefox. > > {Firefox,Chromium} allows to intercept requests... > > Nit: This reads a little funny, perhaps add "us" before "to intercept"? I rephrased this sentence.
Otherwise LGTM https://codereview.adblockplus.org/29739603/diff/29739604/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29739603/diff/29739604/lib/requestBlocker.... lib/requestBlocker.js:151: // the browser (i.e. chrome://), while Chromium only allows to interecept On 2018/04/03 23:36:28, Sebastian Noack wrote: > On 2018/04/03 12:06:45, kzar wrote: > > Do you mean chrome:// for Firefox? Also how come there's a special case for > > "chrome:" below when we already figured out the protocol the browser uses for > > extension pages? > > Yes, "chrome:" is the protocol for content that is part of the browser on > Firefox. The protocol used for parts of an extension (i.e. extensionProtocol) is > "chrome-extension:" and "moz-extension:", repsecively on Chrome and Firefox. Acknowledged. > > > {Firefox,Chromium} allows to intercept requests... > > > > Nit: This reads a little funny, perhaps add "us" before "to intercept"? > > I rephrased this sentence. Nit: Thanks, but it still reads a little bit funny and now there's a typo. How about something like this? // We don't want to block requests sent by extensions (e.g. "moz-extension://") // or the browser (e.g. "chrome://").
https://codereview.adblockplus.org/29739603/diff/29739604/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29739603/diff/29739604/lib/requestBlocker.... lib/requestBlocker.js:151: // the browser (i.e. chrome://), while Chromium only allows to interecept On 2018/04/04 17:38:44, kzar wrote: > On 2018/04/03 23:36:28, Sebastian Noack wrote: > > I rephrased this sentence. > > Nit: Thanks, but it still reads a little bit funny and now there's a typo. How > about something like this? > > // We don't want to block requests sent by extensions (e.g. "moz-extension://") > // or the browser (e.g. "chrome://"). I changed the scope of this change, now also ignoring requests sent by Chrome itself (just like we do on Firefox). As a result I completely rewrote the comment here. I hope it is more clear (and free of typos) now.
https://codereview.adblockplus.org/29739603/diff/29743555/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29739603/diff/29743555/lib/devtools.js#new... lib/devtools.js:145: } I figured now where we don't log requests out of context (to all tabs) anymore, it makes sense to streamline the logic here.
https://codereview.adblockplus.org/29739603/diff/29743555/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29739603/diff/29743555/lib/devtools.js#new... lib/devtools.js:145: } On 2018/04/05 05:42:45, Sebastian Noack wrote: > I figured now where we don't log requests out of context (to all tabs) anymore, > it makes sense to streamline the logic here. For reference, the case of `tabIds.length == 0` right before this change meant almost all of the time, a request sent by Chrome or Adblock Plus themselves. The only scenario left where this condition is met are extremely rare scenarios where oringUrl/intiator cannot be mapped to an open tab.
https://codereview.adblockplus.org/29739603/diff/29743555/lib/devtools.js File lib/devtools.js (left): https://codereview.adblockplus.org/29739603/diff/29743555/lib/devtools.js#old... lib/devtools.js:137: if (panels.size == 0) How come you removed this check? I agreed with it, why bother iterate through all the tabs to find listeners if there's no listeners anyway? https://codereview.adblockplus.org/29739603/diff/29743555/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29739603/diff/29743555/lib/devtools.js#new... lib/devtools.js:142: let request = {url, type, docDomain, thirdParty, sitekey, specificOnly}; How come we're now assigning request inside the loop? I figure it made more sense before when we do it outside. https://codereview.adblockplus.org/29739603/diff/29743555/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29739603/diff/29743555/lib/requestBlocker.... lib/requestBlocker.js:136: let originUrl = details.originUrl ? new URL(details.originUrl) : (I'm kinda surprised you can put two inline conditions in a row like this, but assuming it works I think it's cool how you've done it here.) https://codereview.adblockplus.org/29739603/diff/29743555/lib/requestBlocker.... lib/requestBlocker.js:149: if (originUrl ? originUrl.protocol == extensionProtocol || This one is too terse and gets kinda confusing IMO. If I understood it right perhaps something like this would be easier to grok? if (originUrl) { if (originUrl.protocol == extensionProtocol || originUrl.protocol == "chrome:") return; } else if (details.tabId == -1) return; (That way you could split the bullet points of your big comment around the logic too.)
https://codereview.adblockplus.org/29739603/diff/29743555/lib/devtools.js File lib/devtools.js (left): https://codereview.adblockplus.org/29739603/diff/29743555/lib/devtools.js#old... lib/devtools.js:137: if (panels.size == 0) On 2018/04/05 10:56:37, kzar wrote: > How come you removed this check? I agreed with it, why bother iterate through > all the tabs to find listeners if there's no listeners anyway? Why bother checking the size if we would only do one lookup anyway? I'm thinking about the common case, in which tabIds is an array with a single element. https://codereview.adblockplus.org/29739603/diff/29743555/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29739603/diff/29743555/lib/devtools.js#new... lib/devtools.js:142: let request = {url, type, docDomain, thirdParty, sitekey, specificOnly}; On 2018/04/05 10:56:37, kzar wrote: > How come we're now assigning request inside the loop? I figure it made more > sense before when we do it outside. Most of the time we have only one tabId which most of the time doesn't even have an active panel. So by creating the object here we avoid creating it unnecessarily. Though this means if the request is sent by Service Worker controlling multiple tabs which all have an active devtools panel, we would create the object redundantly, now. But this is an extremely rare scenario, and its better to avoid creating the object at all in other (more common) scenarios. https://codereview.adblockplus.org/29739603/diff/29743555/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29739603/diff/29743555/lib/requestBlocker.... lib/requestBlocker.js:149: if (originUrl ? originUrl.protocol == extensionProtocol || On 2018/04/05 10:56:37, kzar wrote: > This one is too terse and gets kinda confusing IMO. If I understood it right > perhaps something like this would be easier to grok? > > if (originUrl) > { > if (originUrl.protocol == extensionProtocol || originUrl.protocol == > "chrome:") > return; > } > else if (details.tabId == -1) > return; > > (That way you could split the bullet points of your big comment around the logic > too.) How about this?
https://codereview.adblockplus.org/29739603/diff/29743555/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29739603/diff/29743555/lib/devtools.js#new... lib/devtools.js:142: let request = {url, type, docDomain, thirdParty, sitekey, specificOnly}; On 2018/04/05 17:38:59, Sebastian Noack wrote: > On 2018/04/05 10:56:37, kzar wrote: > > How come we're now assigning request inside the loop? ... > ... its better to avoid creating the object at all in other (more common) scenarios. Sure, that's why we had the extra check above which I commented about. https://codereview.adblockplus.org/29739603/diff/29743735/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29739603/diff/29743735/lib/requestBlocker.... lib/requestBlocker.js:150: if (originUrl ? ignoredOrigins.has(originUrl.protocol) : details.tabId == -1) Or perhaps like this? if (originUrl && ignoredOrigins.has(originUrl.protocol) || details.tabId == -1) return; Or even better IMO (so you can comment each part separately) if (originUrl && ignoredOrigins.has(originUrl.protocol)) return; if (details.tabId == -1) return;
https://codereview.adblockplus.org/29739603/diff/29743555/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29739603/diff/29743555/lib/devtools.js#new... lib/devtools.js:142: let request = {url, type, docDomain, thirdParty, sitekey, specificOnly}; On 2018/04/06 14:48:10, kzar wrote: > On 2018/04/05 17:38:59, Sebastian Noack wrote: > > On 2018/04/05 10:56:37, kzar wrote: > > > How come we're now assigning request inside the loop? ... > > ... its better to avoid creating the object at all in other (more common) > scenarios. > > Sure, that's why we had the extra check above which I commented about. The object was still created redundantly if there was any devtools panel but not for any of the given tabs. Sure, this would be acceptable, but I don't get why we should make the logic here more complex than necessary if its already slightly better optimized for the common if we leave it as simple as it is now. https://codereview.adblockplus.org/29739603/diff/29743735/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29739603/diff/29743735/lib/requestBlocker.... lib/requestBlocker.js:150: if (originUrl ? ignoredOrigins.has(originUrl.protocol) : details.tabId == -1) On 2018/04/06 14:48:10, kzar wrote: > Or perhaps like this? > > if (originUrl && ignoredOrigins.has(originUrl.protocol) || details.tabId == -1) > return; > > Or even better IMO (so you can comment each part separately) > > if (originUrl && ignoredOrigins.has(originUrl.protocol)) > return; > > if (details.tabId == -1) > return; These checks aren't equivalent. We only want to bail if we neither have a valid tabId nor an originUrl. Otherwise requests sent by Service Workers would be ignored too.
https://codereview.adblockplus.org/29739603/diff/29743555/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29739603/diff/29743555/lib/devtools.js#new... lib/devtools.js:142: let request = {url, type, docDomain, thirdParty, sitekey, specificOnly}; On 2018/04/06 17:55:46, Sebastian Noack wrote: > On 2018/04/06 14:48:10, kzar wrote: > > On 2018/04/05 17:38:59, Sebastian Noack wrote: > > > On 2018/04/05 10:56:37, kzar wrote: > > > > How come we're now assigning request inside the loop? ... > > > ... its better to avoid creating the object at all in other (more common) > > scenarios. > > > > Sure, that's why we had the extra check above which I commented about. > > The object was still created redundantly if there was any devtools panel but not > for any of the given tabs. Sure, this would be acceptable, but I don't get why > we should make the logic here more complex than necessary if its already > slightly better optimized for the common if we leave it as simple as it is now. Well I guess we disagree on this but whatever, I won't insist. https://codereview.adblockplus.org/29739603/diff/29743735/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29739603/diff/29743735/lib/requestBlocker.... lib/requestBlocker.js:150: if (originUrl ? ignoredOrigins.has(originUrl.protocol) : details.tabId == -1) On 2018/04/06 17:55:46, Sebastian Noack wrote: > On 2018/04/06 14:48:10, kzar wrote: > > Or perhaps like this? ... > These checks aren't equivalent. We only want to bail if we neither have a > valid tabId nor an originUrl. Otherwise requests sent by Service Workers > would be ignored too. Ah right I see. Thing is I think this demonstrates how this nested conditional is kind of confusing. I know it doesn't look as pretty but I think it's better to do it with if... else..., it's way more obvious that way. if (originURL) { if (ignoredOrigins.has(originURL.protocol)) return; } else if (details.tabId == -1) return;
https://codereview.adblockplus.org/29739603/diff/29743735/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29739603/diff/29743735/lib/requestBlocker.... lib/requestBlocker.js:150: if (originUrl ? ignoredOrigins.has(originUrl.protocol) : details.tabId == -1) On 2018/04/09 11:08:44, kzar wrote: > On 2018/04/06 17:55:46, Sebastian Noack wrote: > > On 2018/04/06 14:48:10, kzar wrote: > > > Or perhaps like this? ... > > These checks aren't equivalent. We only want to bail if we neither have a > > valid tabId nor an originUrl. Otherwise requests sent by Service Workers > > would be ignored too. > > Ah right I see. Thing is I think this demonstrates how this nested conditional > is kind of confusing. I know it doesn't look as pretty but I think it's better > to do it with if... else..., it's way more obvious that way. > > if (originURL) > { > if (ignoredOrigins.has(originURL.protocol)) > return; > } > else if (details.tabId == -1) > return; I don't see how the ternary operation is any more complicated to parse (for humans) than binary logical operations. But fine, here you go.
LGTM 
|
