Issue 29421712: Issue 5184 - Support Firefox-specific webRequest types (Closed)
|
Created:
April 25, 2017, 4:25 p.m. by Manish Jethani Modified:
May 21, 2017, 9:04 p.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionIssue 5184 - Support Firefox-specific webRequest types
Patch Set 1 #Patch Set 2 : Move logic to requestBlocker.js #
Total comments: 8
Patch Set 3 : Address comments to Patch Set 2 #
Total comments: 4
Patch Set 4 : Report unmapped types as OTHER #
Total comments: 2
Patch Set 5 : Simplify logic a little bit #
Total comments: 4
Patch Set 6 : Use generator to populate map #
Total comments: 3
Patch Set 7 : Use for..in #MessagesTotal messages: 20
Patch Set 1
I'd rather handle this mapping within RegExpFilter.typeMap (see lib/requestBlocker.js). This would simplify this change, but also keeping in mind that the ext namespace is legacy, lib/requestBlocker.js seems to be the place where this logic rather belongs.
On 2017/04/29 21:49:09, Sebastian Noack wrote: > I'd rather handle this mapping within RegExpFilter.typeMap (see > lib/requestBlocker.js). This would simplify this change, but also keeping in > mind that the ext namespace is legacy, lib/requestBlocker.js seems to be the > place where this logic rather belongs. You mean add these into RegExpFilter.typeMap directly? I was doing that at first, but if we do that we end up supporting filters like abc$imageset and abc$beacon, which is not our intention here (only abc$xbl is supported for legacy reasons). We only want to map these types reported by the browser to types we recognize. Is that correct?
On 2017/05/01 23:25:18, Manish Jethani wrote:
> On 2017/04/29 21:49:09, Sebastian Noack wrote:
> > I'd rather handle this mapping within RegExpFilter.typeMap (see
> > lib/requestBlocker.js). This would simplify this change, but also keeping in
> > mind that the ext namespace is legacy, lib/requestBlocker.js seems to be the
> > place where this logic rather belongs.
>
> You mean add these into RegExpFilter.typeMap directly?
>
> I was doing that at first, but if we do that we end up supporting filters like
> abc$imageset and abc$beacon, which is not our intention here (only abc$xbl is
> supported for legacy reasons). We only want to map these types reported by the
> browser to types we recognize. Is that correct?
I see, makes sense. How about removing any mapping from ext/background.js, and
using following map in lib/requestBlocker.js?
let typeMap = new Map();
for (let type in RegExpFilter.typeMap)
if (type in chrome.webRequest.ResourceType)
typeMap[type.toLowerCase()] = RegExpFilter.typeMap[type];
typeMap.set("sub_frame", RegExpFilter.typeMap.SUBDOCUMENT);
typeMap.set("imageset", RegExpFilter.typeMap.IMAGE);
typeMap.set("beacon", RegExpFilter.typeMap.PING);
This way, we unify the logic, and only need one lookup (as opposed to two
lookups and a potential conversion to uppercase) at runtime.
Patch Set 2: Move logic to requestBlocker.js I've moved the entire mapping logic into requestBlocker.js now. We still need to do an uppercase and a second lookup, but only for devtools.
https://codereview.adblockplus.org/29421712/diff/29440609/lib/requestBlocker.js File lib/requestBlocker.js (left): https://codereview.adblockplus.org/29421712/diff/29440609/lib/requestBlocker.... lib/requestBlocker.js:33: RegExpFilter.typeMap.OBJECT_SUBREQUEST = RegExpFilter.typeMap.OBJECT; Unrelated, but note that this is also true for Firefox WebExtensions. Mind updating the comment above while doing changes here? Eventually, these types should be aliased in the definition in adblockpluscore as there is no platform that has this distinction anymore. https://codereview.adblockplus.org/29421712/diff/29440609/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29421712/diff/29440609/lib/requestBlocker.... lib/requestBlocker.js:35: let resourceTypeMappings = new Map([ Nit: This is a mapping of resource types, so IMHO the variable name should be resourceTypes or resourceTypeMapping. The former would also be consistent with the mapping below. https://codereview.adblockplus.org/29421712/diff/29440609/lib/requestBlocker.... lib/requestBlocker.js:39: ["web_manifest", "OTHER"], I think we shouldn't hard-code the types we treat as OTHER, but rather handle each unknown type as OTHER, as these might vary on different platforms, and new types might be introduced in the future. https://codereview.adblockplus.org/29421712/diff/29440609/lib/requestBlocker.... lib/requestBlocker.js:48: RegExpFilter.typeMap[type.toUpperCase()] || 0]) Please use RegExpFilter.typeMap.OTHER, rather than hard-coding 0.
Patch Set 3: Address comments to Patch Set 2 Change variable name, map unmapped types to "other" Comments inline. https://codereview.adblockplus.org/29421712/diff/29440609/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29421712/diff/29440609/lib/requestBlocker.... lib/requestBlocker.js:35: let resourceTypeMappings = new Map([ On 2017/05/18 12:44:24, Sebastian Noack wrote: > Nit: This is a mapping of resource types, so IMHO the variable name should be > resourceTypes or resourceTypeMapping. The former would also be consistent with > the mapping below. resourceTypes implies all resource types, but I've made it resourceTypeMapping now. https://codereview.adblockplus.org/29421712/diff/29440609/lib/requestBlocker.... lib/requestBlocker.js:39: ["web_manifest", "OTHER"], On 2017/05/18 12:44:24, Sebastian Noack wrote: > I think we shouldn't hard-code the types we treat as OTHER, but rather handle > each unknown type as OTHER, as these might vary on different platforms, and new > types might be introduced in the future. My reasoning was that we shouldn't block any resource types that we don't understand, but I see your point. I've made the change. I should say that Firefox has a resource type called "csp_report", which is a request the browser sends to a URL of your choice when there's a content security policy violation on your page. I don't know if the "other" type of filters should be able to block these too. Background (see "report-uri"): https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP https://codereview.adblockplus.org/29421712/diff/29440609/lib/requestBlocker.... lib/requestBlocker.js:48: RegExpFilter.typeMap[type.toUpperCase()] || 0]) On 2017/05/18 12:44:24, Sebastian Noack wrote: > Please use RegExpFilter.typeMap.OTHER, rather than hard-coding 0. Done.
https://codereview.adblockplus.org/29421712/diff/29441573/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29421712/diff/29441573/lib/requestBlocker.... lib/requestBlocker.js:32: // Chrome and Firefox can't distinguish between OBJECT_SUBREQUEST and OBJECT Nit: Thanks for updating the comment, but it's still a little misleading as Firefox before they moved to WebExtensions actually distinguished these types. So I'd either refer to "Chrome and Firefox (WebExtensions) ..." or "Browsers targeted by this code ...". https://codereview.adblockplus.org/29421712/diff/29441573/lib/requestBlocker.... lib/requestBlocker.js:61: resourceTypeMapping.get(type) || type.toUpperCase(), docDomain, In case of the resource types we handle as OTHER, this would cause the original type (as reported by the webRequest API), merely in upper case, to show up in the devtools panel, right? This will be a change in behavior, and also confusing because filter lists don't know anything about the webRequest API but only know our filter syntax which identifies these requests as OTHER.
Patch Set 4: Report unmapped types as OTHER https://codereview.adblockplus.org/29421712/diff/29441573/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29421712/diff/29441573/lib/requestBlocker.... lib/requestBlocker.js:32: // Chrome and Firefox can't distinguish between OBJECT_SUBREQUEST and OBJECT On 2017/05/18 21:33:41, Sebastian Noack wrote: > Nit: Thanks for updating the comment, but it's still a little misleading as > Firefox before they moved to WebExtensions actually distinguished these types. > So I'd either refer to "Chrome and Firefox (WebExtensions) ..." or "Browsers > targeted by this code ...". Done. https://codereview.adblockplus.org/29421712/diff/29441573/lib/requestBlocker.... lib/requestBlocker.js:61: resourceTypeMapping.get(type) || type.toUpperCase(), docDomain, On 2017/05/18 21:33:42, Sebastian Noack wrote: > In case of the resource types we handle as OTHER, this would cause the original > type (as reported by the webRequest API), merely in upper case, to show up in > the devtools panel, right? This will be a change in behavior, and also confusing > because filter lists don't know anything about the webRequest API but only know > our filter syntax which identifies these requests as OTHER. Oops, somehow I missed this. Fixed.
https://codereview.adblockplus.org/29421712/diff/29440609/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29421712/diff/29440609/lib/requestBlocker.... lib/requestBlocker.js:39: ["web_manifest", "OTHER"], On 2017/05/18 20:52:04, Manish Jethani wrote: > On 2017/05/18 12:44:24, Sebastian Noack wrote: > > I think we shouldn't hard-code the types we treat as OTHER, but rather handle > > each unknown type as OTHER, as these might vary on different platforms, and > new > > types might be introduced in the future. > > My reasoning was that we shouldn't block any resource types that we don't > understand, but I see your point. I've made the change. > > I should say that Firefox has a resource type called "csp_report", which is a > request the browser sends to a URL of your choice when there's a content > security policy violation on your page. I don't know if the "other" type of > filters should be able to block these too. > > Background (see "report-uri"): > > https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP Sorry, I forgot to reply to this, since you addressed my comment anyway. I think it is preferable to block request types we don't know (if they match any filter, when considered of the type OTHER). Usually, new request types existed before but were originally reported as OTHER, or are still in some browsers. In the past there hasn't been a single instance of new request type that would have made sense to completely ignore. The csp_report request type might be a little special though. After all this is a security features, and if websites are no longer notified about CSP violations that might be bad. But then again, this feature could potentially still be misused to track users, so being able to block it might be a good thing. However, there is already one request type which we deliberatly ignore, i.e. "main_frame". If we decide to do the same for "csp_report", we should do so in the same way, i.e. by bailing out in webRequest.onBeforeRequest handler. https://codereview.adblockplus.org/29421712/diff/29441576/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29421712/diff/29441576/lib/requestBlocker.... lib/requestBlocker.js:40: ]); How about using a mapping like below? let resourceTypes = new Map(); for (let type in RegExpFilter.typeMap) resourceTypes.set(type.toLowerCase(), type); resourceTypes.set("beacon", "PING"); resourceTypes.set("imageset", "IMAGE"); resourceTypes.set("sub_frame", "SUBDOCUMENT"); Then in the onBeforeRequest handler: let mappedType = resourceTypes.get(type) || "OTHER"; ... let filter = defaultMatcher.matchesAny( ..., RegExpFilter.typeMap[mappedType], ... ); setTimeout(onBeforeRequestAsync, 0, ..., mappedType, ...); This way we have two lookups again though, but: * It seems to be overall less code. * We don't have any duplicated logic (otherwise the code generating the typeMasks mapping and the mapping for the devtools panel use redundant logic). * We don't rely on chrome.webRequest.ResourceType. (Microsoft Edge actually doesn't have ResourceType. It also seems to be generally more robust.) What do you think?
Patch Set 5: Simplify logic a little bit https://codereview.adblockplus.org/29421712/diff/29441576/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29421712/diff/29441576/lib/requestBlocker.... lib/requestBlocker.js:40: ]); On 2017/05/19 11:30:46, Sebastian Noack wrote: > How about using a mapping like below? > > let resourceTypes = new Map(); > for (let type in RegExpFilter.typeMap) > resourceTypes.set(type.toLowerCase(), type); > resourceTypes.set("beacon", "PING"); > resourceTypes.set("imageset", "IMAGE"); > resourceTypes.set("sub_frame", "SUBDOCUMENT"); > > [...] Yes, that's two lookups now instead of one for the normal case, but it does simplify the logic a bit. I've made the change.
On 2017/05/19 11:30:46, Sebastian Noack wrote: > I think it is preferable to block request types we don't know (if they match any > filter, when considered of the type OTHER). Usually, new request types existed > before but were originally reported as OTHER, or are still in some browsers. In > the past there hasn't been a single instance of new request type that would have > made sense to completely ignore. That's good to know, thanks. > The csp_report request type might be a little special though. After all this is > a security features, and if websites are no longer notified about CSP violations > that might be bad. But then again, this feature could potentially still be > misused to track users, so being able to block it might be a good thing. > > [...] By the way, to test csp_report in Firefox you just have to write the filter "$other,domain=report-uri.io" and go to https://report-uri.io/ It's getting correctly mapped to OTHER so it's looking good now. Yeah I agree it could be abused to track users, it's probably a good idea to cover it under OTHER.
https://codereview.adblockplus.org/29421712/diff/29442629/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29421712/diff/29442629/lib/requestBlocker.... lib/requestBlocker.js:39: // Map unsupported resource types to types we support. Anything that isn't I wouldn't consider these resource types "unsupported", but rather translated into a more abstract model. For reference, this is the original mapping in the legacy Gecko extension (which has more comprehensive comments for the mapped types): https://hg.adblockplus.org/adblockplus/file/ce08e8e0aabf/lib/contentPolicy.js... Also I'd wonder what you think about the approach, there, creating the Map from a generator? I don't have a strong opinion. But if you think it's more readable you might want to adopt it here.
Patch Set 6: Use generator to populate map https://codereview.adblockplus.org/29421712/diff/29442629/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29421712/diff/29442629/lib/requestBlocker.... lib/requestBlocker.js:39: // Map unsupported resource types to types we support. Anything that isn't On 2017/05/19 17:47:41, Sebastian Noack wrote: > I wouldn't consider these resource types "unsupported", but rather translated > into a more abstract model. > > For reference, this is the original mapping in the legacy Gecko extension (which > has more comprehensive comments for the mapped types): > https://hg.adblockplus.org/adblockplus/file/ce08e8e0aabf/lib/contentPolicy.js... OK, I've just copied that comment over and changed "Firefox" to "the browser". > Also I'd wonder what you think about the approach, there, creating the Map from > a generator? I don't have a strong opinion. But if you think it's more readable > you might want to adopt it here. Feels like an unnecessary use of a generator, but doesn't hurt and probably not a bad idea to go for it for consistency. I've made the change. https://codereview.adblockplus.org/29421712/diff/29442758/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29421712/diff/29442758/lib/requestBlocker.... lib/requestBlocker.js:40: for (let type of Object.keys(RegExpFilter.typeMap)) This by the way could just be: yield* Object.keys(RegExpFilter.typeMap).map(type => [type.toLowerCase(), type]); I wasn't sure if Chrome 49 supports that.
https://codereview.adblockplus.org/29421712/diff/29442629/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29421712/diff/29442629/lib/requestBlocker.... lib/requestBlocker.js:39: // Map unsupported resource types to types we support. Anything that isn't On 2017/05/19 23:44:15, Manish Jethani wrote: > On 2017/05/19 17:47:41, Sebastian Noack wrote: > > Also I'd wonder what you think about the approach, there, creating the Map > from > > a generator? I don't have a strong opinion. But if you think it's more > readable > > you might want to adopt it here. > > Feels like an unnecessary use of a generator, but doesn't hurt and probably not > a bad idea to go for it for consistency. I've made the change. As I said, I don't have a strong opinion. So if you'd rather not use a generator here that is fine too. Though I personally don't like chains of .map() and .concat() either. And the generator syntax seems to be the approach with the least boilerplate. But up to you. https://codereview.adblockplus.org/29421712/diff/29442758/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29421712/diff/29442758/lib/requestBlocker.... lib/requestBlocker.js:40: for (let type of Object.keys(RegExpFilter.typeMap)) On 2017/05/19 23:44:15, Manish Jethani wrote: > This by the way could just be: > > yield* Object.keys(RegExpFilter.typeMap).map(type => [type.toLowerCase(), > type]); > > I wasn't sure if Chrome 49 supports that. It seems all versions of Chrome that support generators, also support yield*. I just confirmed that yield* works in Chrome 47. However, I'm not sure if thismakes the code any more readable here. But it seems to be simpler to just use `for (let type in RegExpFilter.typeMap)` here, rather than Object.keys().
Patch Set 7: Use for..in https://codereview.adblockplus.org/29421712/diff/29442629/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29421712/diff/29442629/lib/requestBlocker.... lib/requestBlocker.js:39: // Map unsupported resource types to types we support. Anything that isn't On 2017/05/20 06:42:37, Sebastian Noack wrote: > On 2017/05/19 23:44:15, Manish Jethani wrote: > > On 2017/05/19 17:47:41, Sebastian Noack wrote: > > > Also I'd wonder what you think about the approach, there, creating the Map > > from > > > a generator? I don't have a strong opinion. But if you think it's more > > readable > > > you might want to adopt it here. > > > > Feels like an unnecessary use of a generator, but doesn't hurt and probably > not > > a bad idea to go for it for consistency. I've made the change. > > As I said, I don't have a strong opinion. So if you'd rather not use a generator > here that is fine too. Though I personally don't like chains of .map() and > .concat() either. And the generator syntax seems to be the approach with the > least boilerplate. But up to you. Acknowledged. https://codereview.adblockplus.org/29421712/diff/29442758/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29421712/diff/29442758/lib/requestBlocker.... lib/requestBlocker.js:40: for (let type of Object.keys(RegExpFilter.typeMap)) On 2017/05/20 06:42:37, Sebastian Noack wrote: > On 2017/05/19 23:44:15, Manish Jethani wrote: > > This by the way could just be: > > > > yield* Object.keys(RegExpFilter.typeMap).map(type => [type.toLowerCase(), > > type]); > > > > I wasn't sure if Chrome 49 supports that. > > It seems all versions of Chrome that support generators, also support yield*. I > just confirmed that yield* works in Chrome 47. However, I'm not sure if > thismakes the code any more readable here. > > But it seems to be simpler to just use `for (let type in RegExpFilter.typeMap)` > here, rather than Object.keys(). Done.
On 2017/05/20 18:50:47, Manish Jethani wrote: > Patch Set 7: Use for..in Sorry, please ignore.
On 2017/05/20 18:51:37, Manish Jethani wrote: > On 2017/05/20 18:50:47, Manish Jethani wrote: > > Patch Set 7: Use for..in > > Sorry, please ignore. OK, fixed it now. I had diffed against the wrong rev. Please review.
LGTM 
|
