Issue 4559243822759936: Issue 431/432 - Remove special handling for the $sitekey option (Closed)
|
Created:
July 28, 2014, 2:44 p.m. by Thomas Greiner Modified:
Sept. 2, 2014, 8:37 a.m. Reviewers:
Wladimir Palant Visibility:
Public. |
DescriptionThis review addresses both issues #431 and #432 due to them being somewhat intertwined. I'll create a separate review for #698 as soon as there's consent about the interfaces.
Patch Set 1 #
Total comments: 16
Patch Set 2 : #Patch Set 3 : #
Total comments: 34
Patch Set 4 : #
Total comments: 2
Patch Set 5 : Rebased and added comment #Patch Set 6 : Made changes for WebKit bug 132872 #
MessagesTotal messages: 11
http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/... File lib/contentPolicy.js (right): http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/... lib/contentPolicy.js:175: match = Policy.isWhitelisted(testWndLocation, parentWndLocation, siteKey); The logic here seems wrong to me. I've extended the description in https://issues.adblockplus.org/ticket/431 to explain what I mean - the sitekey shouldn't just apply to the current frame but also all frames below it. What this boils down IMHO: let [sitekey, sitekeyWnd] = getSitekey(wnd); This function should walk up from wnd, retrieve the sitekeys and validate them. If a valid one is found it should be returned along with the window it belongs to. The whitelisting check then should create its own copies of these variables and call getSitekey() again whenever it arrives at the frame belonging to the sitekey (only after processing this frame and only if it isn't the top frame already of course). Note that this approach also makes sure the sitekey isn't retrieved twice (currently you do it for the whitelisting check and actual request check). http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/... lib/contentPolicy.js:286: isWhitelisted: function(url, parentUrl, siteKey) If you change the parameters here you probably want to adjust the other callers: isWindowWhitelisted() here, appSupport.js, ui.js. http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/... lib/contentPolicy.js:297: parentUrl = url; If parentUrl is no longer optional then this code should be unnecessary. http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/... lib/contentPolicy.js:679: function getSiteKey(node, wnd) Why pass in node parameter that isn't used? http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/... File lib/filterClasses.js (right): http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/... lib/filterClasses.js:23: let {Utils} = require("utils"); This is a pretty self-contained module, it shouldn't have a dependency on utils.js. http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/... lib/filterClasses.js:220: * @param {String} siteKeys (optional) Public keys of websites that this filter should apply to, e.g. "foo|bar|~baz" I don't really see how exceptions are useful here, original implementation didn't have any. Keep it simple? http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/... lib/filterClasses.js:229: this.siteKeySource = siteKeys; This should be on RegExpFilter and not on ActiveFilter - sitekeys aren't considered for element hiding filters. http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/... lib/filterClasses.js:498: isActiveOnPage: function(/**String*/ docDomain, /**String*/ docLocation, /**String*/ siteKey) /**Boolean*/ I don't think that this is the right place to perform sitekey validation, by the time the matcher is called the sitekey should already be validated. This also means that you don't need to pass document URL around everywhere and no new function, you can simply extend isActiveOnDomain() in RegExpFilter to consider one more parameter.
http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/... File lib/contentPolicy.js (right): http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/... lib/contentPolicy.js:175: match = Policy.isWhitelisted(testWndLocation, parentWndLocation, siteKey); On 2014/08/01 10:50:21, Wladimir Palant wrote: > The logic here seems wrong to me. I've extended the description in > https://issues.adblockplus.org/ticket/431 to explain what I mean - the sitekey > shouldn't just apply to the current frame but also all frames below it. > > What this boils down IMHO: > > let [sitekey, sitekeyWnd] = getSitekey(wnd); > > This function should walk up from wnd, retrieve the sitekeys and validate them. > If a valid one is found it should be returned along with the window it belongs > to. > > The whitelisting check then should create its own copies of these variables and > call getSitekey() again whenever it arrives at the frame belonging to the > sitekey (only after processing this frame and only if it isn't the top frame > already of course). > > Note that this approach also makes sure the sitekey isn't retrieved twice > (currently you do it for the whitelisting check and actual request check). Done. http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/... lib/contentPolicy.js:286: isWhitelisted: function(url, parentUrl, siteKey) On 2014/08/01 10:50:21, Wladimir Palant wrote: > If you change the parameters here you probably want to adjust the other callers: > isWindowWhitelisted() here, appSupport.js, ui.js. Done. http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/... lib/contentPolicy.js:297: parentUrl = url; On 2014/08/01 10:50:21, Wladimir Palant wrote: > If parentUrl is no longer optional then this code should be unnecessary. Technically, it's still optional since it's only required when a sitekey was passed. I added the "optional" keyword to the parameter description. http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/... lib/contentPolicy.js:679: function getSiteKey(node, wnd) On 2014/08/01 10:50:21, Wladimir Palant wrote: > Why pass in node parameter that isn't used? Done. http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/... File lib/filterClasses.js (right): http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/... lib/filterClasses.js:23: let {Utils} = require("utils"); On 2014/08/01 10:50:21, Wladimir Palant wrote: > This is a pretty self-contained module, it shouldn't have a dependency on > utils.js. Done. http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/... lib/filterClasses.js:220: * @param {String} siteKeys (optional) Public keys of websites that this filter should apply to, e.g. "foo|bar|~baz" On 2014/08/01 10:50:21, Wladimir Palant wrote: > I don't really see how exceptions are useful here, original implementation > didn't have any. Keep it simple? The issue stated that $sitekey should be handled exactly the same way as the domain option. However, if you don't see any concerns in regard to backwards compatibility when implementing it at a later point if needed, I don't mind removing it. http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/... lib/filterClasses.js:229: this.siteKeySource = siteKeys; On 2014/08/01 10:50:21, Wladimir Palant wrote: > This should be on RegExpFilter and not on ActiveFilter - sitekeys aren't > considered for element hiding filters. Done. http://codereview.adblockplus.org/4559243822759936/diff/5629499534213120/lib/... lib/filterClasses.js:498: isActiveOnPage: function(/**String*/ docDomain, /**String*/ docLocation, /**String*/ siteKey) /**Boolean*/ On 2014/08/01 10:50:21, Wladimir Palant wrote: > I don't think that this is the right place to perform sitekey validation, by the > time the matcher is called the sitekey should already be validated. This also > means that you don't need to pass document URL around everywhere and no new > function, you can simply extend isActiveOnDomain() in RegExpFilter to consider > one more parameter. Done.
The changes to appSupport.js were missing in the previous patch so I uploaded them now.
http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... File lib/appSupport.js (right): http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/appSupport.js:865: if (Policy.isWindowWhitelisted(exports.getBrowser(window).contentWindow)) Please revert the changes to this file. Sitekeys should whitelist individual requests but not whitelist entire windows - that's the whole point of this change. Communicating sitekeys via whitelisting UI that is all about locations is hard to say the least, so we don't. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... File lib/contentPolicy.js (right): http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/contentPolicy.js:190: } Nit: I'd put these lines without an |else| block here, it doesn't look right with your changes. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/contentPolicy.js:246: match = defaultMatcher.matchesAny(locationText, Policy.typeDescr[contentType] || "", docDomain, thirdParty, sitekey); This is the sitekey of the top-level window, not the one of the frame making the request - you overwrote it above. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/contentPolicy.js:284: * @param {String} (optional) sitekey public key provided on the page Please see https://code.google.com/p/jsdoc-toolkit/wiki/TagParam#Optional_Parameters for the correct syntax. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/contentPolicy.js:319: return Policy.isWhitelisted(url, parentUrl, sitekey); This function is meant to be used from the UI, its result should match what we show for the "whitelisted" state - meaning no parent URLs (always a top-level window) and no sitekeys. So please revert it to the original state. Note that the JSDoc documentation can certainly be improved here. Also, the implementation of getWindowLocation() that this relies on isn't quite correct (application-specific branching should be moved into appSupport.js, missing Utils.unwrapURL() call). Might be a good idea to create follow-up issues to clean this up. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/contentPolicy.js:686: if (Utils.crypto) This is a pointless assumption about the inner workings of verifySignature(). Given that we stopped supporting Gecko 1.9.2 a long time ago, Utils.crypto should always be set unless something is badly wrong - nothing to optimize here. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/contentPolicy.js:688: while (wnd) This is misleading: we don't expect wnd to ever become null. I'd rather have |while (true)| here as in the original code, to indicate hat this isn't a real break condition. Alternatively you end the loop with: wnd = (wnd !== wnd.parent ? wnd.parent : null); http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/contentPolicy.js:693: if (foundKey && foundKey.indexOf("_") > - 1) The original code uses ">= 0" here, why change it into "> -1"? And there definitely should be no space between - and 1. Besides, why was this code changed at all? You moved it so now I have to compare all of it line by line. The changes seem pointless, I don't consider foundKey a better variable name than keydata - it sounds like a boolean flag but isn't. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/contentPolicy.js:708: sitekey = foundKey; The current code will ignore a valid key further up in the hierarchy if an invalid one is found. How about |return [key, wnd];| here insteadof breaking out of the look below. Note that only the key part should be returned, the signature is irrelevant once it has been validated. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/contentPolicy.js:713: if (wnd === wnd.top) I would compare to the parent here, as in the original code - the parent is what we care about, to avoid an endless loop. Comparing to wnd.top unnecessarily adds the assumption that wnd.parent === wnd can only be true for the top window. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... File lib/filterClasses.js (right): http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/filterClasses.js:386: * @param {String} sitekey (optional) public key provided by the document Nit: Please indicate the optional parameter with square brackets. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/filterClasses.js:392: if (!this.domains && !this.sitekeys) I think there should be a |sitekey: null| property on ActiveFilter just to make sure that you don't access an undefined property here (it will only be set properly for RegExpFilter still). http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/filterClasses.js:395: let activeDomain = true; This made the code significantly more complicated and quite frankly I don't even want to validate that it is still correct. How about you simply deal with the sitekeys at the very beginning of the function? if (this.sitekeys && (!sitekey || this.sitekeys.indexOf(sitekey) < 0) return false; And then you can check the domains exactly the same way it was done before - starting with |if (!domains)|. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/filterClasses.js:433: let [key, signature] = sitekey.split("_", 2); There should be no signature at this point. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/filterClasses.js:489: * @param {String} sitekeys (optional) Public keys of websites that this filter should apply to, e.g. "foo|bar|~baz" Please remove the exceptions from the comment here. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/filterClasses.js:636: } You don't need this complicated data structure if you don't have exceptions: sitekeys = this.sitekeySource.split("|"); http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/ui.js:1092: if (Policy.isWindowWhitelisted(getBrowser(window).contentWindow)) Please revert the changes in this file, the UI shouldn't be made aware of sitekeys.
http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... File lib/appSupport.js (right): http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/appSupport.js:865: if (Policy.isWindowWhitelisted(exports.getBrowser(window).contentWindow)) On 2014/08/14 19:58:15, Wladimir Palant wrote: > Please revert the changes to this file. Sitekeys should whitelist individual > requests but not whitelist entire windows - that's the whole point of this > change. Communicating sitekeys via whitelisting UI that is all about locations > is hard to say the least, so we don't. Done. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... File lib/contentPolicy.js (right): http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/contentPolicy.js:190: } On 2014/08/14 19:58:15, Wladimir Palant wrote: > Nit: I'd put these lines without an |else| block here, it doesn't look right > with your changes. Done. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/contentPolicy.js:246: match = defaultMatcher.matchesAny(locationText, Policy.typeDescr[contentType] || "", docDomain, thirdParty, sitekey); On 2014/08/14 19:58:15, Wladimir Palant wrote: > This is the sitekey of the top-level window, not the one of the frame making the > request - you overwrote it above. Done. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/contentPolicy.js:284: * @param {String} (optional) sitekey public key provided on the page On 2014/08/14 19:58:15, Wladimir Palant wrote: > Please see > https://code.google.com/p/jsdoc-toolkit/wiki/TagParam#Optional_Parameters for > the correct syntax. Done. I was using the syntax used in lib/filterClasses.js so I changed it there as well. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/contentPolicy.js:319: return Policy.isWhitelisted(url, parentUrl, sitekey); On 2014/08/14 19:58:15, Wladimir Palant wrote: > This function is meant to be used from the UI, its result should match what we > show for the "whitelisted" state - meaning no parent URLs (always a top-level > window) and no sitekeys. So please revert it to the original state. > > Note that the JSDoc documentation can certainly be improved here. Also, the > implementation of getWindowLocation() that this relies on isn't quite correct > (application-specific branching should be moved into appSupport.js, missing > Utils.unwrapURL() call). Might be a good idea to create follow-up issues to > clean this up. Done. I'll create an issue for that. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/contentPolicy.js:686: if (Utils.crypto) On 2014/08/14 19:58:15, Wladimir Palant wrote: > This is a pointless assumption about the inner workings of verifySignature(). > Given that we stopped supporting Gecko 1.9.2 a long time ago, Utils.crypto > should always be set unless something is badly wrong - nothing to optimize here. Done. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/contentPolicy.js:688: while (wnd) On 2014/08/14 19:58:15, Wladimir Palant wrote: > This is misleading: we don't expect wnd to ever become null. I'd rather have > |while (true)| here as in the original code, to indicate hat this isn't a real > break condition. Alternatively you end the loop with: > > wnd = (wnd !== wnd.parent ? wnd.parent : null); Done. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/contentPolicy.js:693: if (foundKey && foundKey.indexOf("_") > - 1) On 2014/08/14 19:58:15, Wladimir Palant wrote: > The original code uses ">= 0" here, why change it into "> -1"? And there > definitely should be no space between - and 1. > > Besides, why was this code changed at all? You moved it so now I have to compare > all of it line by line. The changes seem pointless, I don't consider foundKey a > better variable name than keydata - it sounds like a boolean flag but isn't. Done. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/contentPolicy.js:708: sitekey = foundKey; On 2014/08/14 19:58:15, Wladimir Palant wrote: > The current code will ignore a valid key further up in the hierarchy if an > invalid one is found. How about |return [key, wnd];| here insteadof breaking out > of the look below. > > Note that only the key part should be returned, the signature is irrelevant once > it has been validated. Done. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/contentPolicy.js:713: if (wnd === wnd.top) On 2014/08/14 19:58:15, Wladimir Palant wrote: > I would compare to the parent here, as in the original code - the parent is what > we care about, to avoid an endless loop. Comparing to wnd.top unnecessarily adds > the assumption that wnd.parent === wnd can only be true for the top window. Done. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... File lib/filterClasses.js (right): http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/filterClasses.js:386: * @param {String} sitekey (optional) public key provided by the document On 2014/08/14 19:58:15, Wladimir Palant wrote: > Nit: Please indicate the optional parameter with square brackets. Done. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/filterClasses.js:392: if (!this.domains && !this.sitekeys) On 2014/08/14 19:58:15, Wladimir Palant wrote: > I think there should be a |sitekey: null| property on ActiveFilter just to make > sure that you don't access an undefined property here (it will only be set > properly for RegExpFilter still). Done. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/filterClasses.js:395: let activeDomain = true; On 2014/08/14 19:58:15, Wladimir Palant wrote: > This made the code significantly more complicated and quite frankly I don't even > want to validate that it is still correct. How about you simply deal with the > sitekeys at the very beginning of the function? > > if (this.sitekeys && (!sitekey || this.sitekeys.indexOf(sitekey) < 0) > return false; > > And then you can check the domains exactly the same way it was done before - > starting with |if (!domains)|. Done. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/filterClasses.js:433: let [key, signature] = sitekey.split("_", 2); On 2014/08/14 19:58:15, Wladimir Palant wrote: > There should be no signature at this point. Done. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/filterClasses.js:489: * @param {String} sitekeys (optional) Public keys of websites that this filter should apply to, e.g. "foo|bar|~baz" On 2014/08/14 19:58:15, Wladimir Palant wrote: > Please remove the exceptions from the comment here. Done. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/filterClasses.js:636: } On 2014/08/14 19:58:15, Wladimir Palant wrote: > You don't need this complicated data structure if you don't have exceptions: > > sitekeys = this.sitekeySource.split("|"); Done. http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/4559243822759936/diff/5661458385862656/lib/... lib/ui.js:1092: if (Policy.isWindowWhitelisted(getBrowser(window).contentWindow)) On 2014/08/14 19:58:15, Wladimir Palant wrote: > Please revert the changes in this file, the UI shouldn't be made aware of > sitekeys. Done.
Look good, only one issue below. http://codereview.adblockplus.org/4559243822759936/diff/5121099960418304/lib/... File lib/filterClasses.js (right): http://codereview.adblockplus.org/4559243822759936/diff/5121099960418304/lib/... lib/filterClasses.js:397: if (this.sitekeys && (!sitekey || this.sitekeys.indexOf(sitekey.toUpperCase()) < 0)) Ok, we have an issue here. This code works in the first approximation, but sitekeys are actually case-sensitive. There is a very slight chance that two different sitekeys will be identical when converted to upper-case. Given that this issue was present in the original code as well, and that fixing it is non-trivial (we would need to change the way filter options are parsed) - could you just add a comment noting this problem?
http://codereview.adblockplus.org/4559243822759936/diff/5121099960418304/lib/... File lib/filterClasses.js (right): http://codereview.adblockplus.org/4559243822759936/diff/5121099960418304/lib/... lib/filterClasses.js:397: if (this.sitekeys && (!sitekey || this.sitekeys.indexOf(sitekey.toUpperCase()) < 0)) On 2014/08/29 20:13:59, Wladimir Palant wrote: > Ok, we have an issue here. This code works in the first approximation, but > sitekeys are actually case-sensitive. There is a very slight chance that two > different sitekeys will be identical when converted to upper-case. > > Given that this issue was present in the original code as well, and that fixing > it is non-trivial (we would need to change the way filter options are parsed) - > could you just add a comment noting this problem? Done.
LGTM
I noticed that WebKit bug 132872 also affects this issue so I added the necessary changes based on https://hg.adblockplus.org/adblockplus/rev/10df3dfad74f
Not sure whether this was really necessary, definitely won't hurt however. LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
