Issue 698 - Added tests for $sitekey option

Issue 698 - Added tests for $sitekey option

[Wladimir Palant, 2014-09-10 21:55:06]

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
File chrome/content/tests/filterClasses.js (right):

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
chrome/content/tests/filterClasses.js:42: result.push("sitekeys=" +
sitekeys.sort().join("|"));
sitekeys.sort() will have side-effects - it will actually sort filter.sitekeys.
You should create a copy of the original array: sitekeys.slice().sort()

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
chrome/content/tests/filterClasses.js:222:
compareFilter("@@bla$match-case,script,other,third-party,sitekey=foo|bar",
["type=whitelist",
"text=@@bla$match-case,script,other,third-party,sitekey=foo|bar", "regexp=bla",
"matchCase=true", "contentType=" + (t.SCRIPT | t.OTHER), "thirdParty=true",
"sitekeys=BAR|FOO"]);
It's ok to add the sitekey option to the tests above - the idea there was to
test all kinds of filter options at the same time. The second compareFilter()
call can be made without a sitekey option, then this is covered as well.

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
File chrome/content/tests/matcher.js (right):

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
chrome/content/tests/matcher.js:30: };
Given that no signatures are being verified here and the keys are being used
merely as opaque strings: why not actually use "foo+publickey" and
"bar+publickey" instead and drop the beautification? That will simplify the
tests as well, these can simply hardcode the keys instead of referring to the
global object.

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
chrome/content/tests/matcher.js:194: checkMatch(["abc$sitekey=" +
publickeys.foo, "cba$sitekey=" + publickeys.bar], "http://abc/def", "IMAGE",
"baz.com", false, null, null);
Please test the scenarios where the filter combines sitekey and domain options.

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
File chrome/content/tests/policy.js (right):

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
chrome/content/tests/policy.js:59: let publickey =
"MFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBANLNgMGCl6kc3QE/73/ESBsuhzCNQWApqWS1nKVWf+RjDTiF80LD2TiTdQhlW9mlWsDDbttsDlEcCmPVz6H3OUMCAwEAAQ";
There are some RSA keys in
https://hg.adblockplus.org/adblockpluschrome/file/b8f283613e94/qunit/tests/si...
- how about using the 512 bit key from there instead?

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
chrome/content/tests/policy.js:65: let sitekey =
"MFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBANLNgMGCl6kc3QE/73/ESBsuhzCNQWApqWS1nKVWf+RjDTiF80LD2TiTdQhlW9mlWsDDbttsDlEcCmPVz6H3OUMCAwEAAQ==_peQ1CvuHieDHAXURwy9K8VQFaPWHHU0oDGT3pnIoQ6Nm7DUvCPw6FsnqfpdoIcq+kwqtkgWIcxM95gytuphvzQ==";
Call this variable "signature"?

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
File chrome/content/tests/regexpFilters_matching.js (right):

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
chrome/content/tests/regexpFilters_matching.js:29: bar:
"MFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBALlKJGIBHYU3rIWyGmiwUgK1BUUomSBgLzSngoE7gBc7mqgCsEbjf9EJ5AvQIM1x7MhVsD5djpqHLcjiQlZFXfECAwEAAQ"
Here as well: given that the keys aren't actually being used, how about using
"foo+publickey" and "bar+publickey" instead?

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
chrome/content/tests/regexpFilters_matching.js:317: testMatch("abc$sitekey=" +
publickeys.foo, "http://abc/def", "IMAGE", "foo.com", true, null, false);
What about the scenario where the sitekey is present but doesn't match the one
specified in the filter?

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
chrome/content/tests/regexpFilters_matching.js:329: testMatch("abc$sitekey=" +
publickeys.foo + ",~image", "http://abc/def", "OBJECT", "foo.com", true,
publickeys.foo, true);
This should test how the sitekeys get along with domain options rather than type
options.

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
File chrome/content/tests/signatures.js (right):

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
chrome/content/tests/signatures.js:2: * This file is part of Adblock Plus
<http://adblockplus.org/>,
Just in case: Please make sure that this file is removed from the
adblockpluschrome repository and imported from adblockplustests instead.

[Wladimir Palant, 2014-12-09 21:37:29]

May I remind you that this issue is still outstanding?

[Thomas Greiner, 2014-12-11 11:26:47]

On 2014/12/09 21:37:29, Wladimir Palant wrote:
> May I remind you that this issue is still outstanding?

Right, I'll continue working on it this week.

[Thomas Greiner, 2014-12-11 14:33:55]

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
File chrome/content/tests/filterClasses.js (right):

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
chrome/content/tests/filterClasses.js:42: result.push("sitekeys=" +
sitekeys.sort().join("|"));
On 2014/09/10 21:55:06, Wladimir Palant wrote:
> sitekeys.sort() will have side-effects - it will actually sort
filter.sitekeys.
> You should create a copy of the original array: sitekeys.slice().sort()

Done.

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
chrome/content/tests/filterClasses.js:222:
compareFilter("@@bla$match-case,script,other,third-party,sitekey=foo|bar",
["type=whitelist",
"text=@@bla$match-case,script,other,third-party,sitekey=foo|bar", "regexp=bla",
"matchCase=true", "contentType=" + (t.SCRIPT | t.OTHER), "thirdParty=true",
"sitekeys=BAR|FOO"]);
On 2014/09/10 21:55:06, Wladimir Palant wrote:
> It's ok to add the sitekey option to the tests above - the idea there was to
> test all kinds of filter options at the same time. The second compareFilter()
> call can be made without a sitekey option, then this is covered as well.

Done.

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
File chrome/content/tests/matcher.js (right):

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
chrome/content/tests/matcher.js:30: };
On 2014/09/10 21:55:06, Wladimir Palant wrote:
> Given that no signatures are being verified here and the keys are being used
> merely as opaque strings: why not actually use "foo+publickey" and
> "bar+publickey" instead and drop the beautification? That will simplify the
> tests as well, these can simply hardcode the keys instead of referring to the
> global object.

Done. That was a leftover from my previous approach in which the verification
was in the matcher.

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
chrome/content/tests/matcher.js:194: checkMatch(["abc$sitekey=" +
publickeys.foo, "cba$sitekey=" + publickeys.bar], "http://abc/def", "IMAGE",
"baz.com", false, null, null);
On 2014/09/10 21:55:06, Wladimir Palant wrote:
> Please test the scenarios where the filter combines sitekey and domain
options.

Done.

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
File chrome/content/tests/policy.js (right):

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
chrome/content/tests/policy.js:59: let publickey =
"MFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBANLNgMGCl6kc3QE/73/ESBsuhzCNQWApqWS1nKVWf+RjDTiF80LD2TiTdQhlW9mlWsDDbttsDlEcCmPVz6H3OUMCAwEAAQ";
On 2014/09/10 21:55:06, Wladimir Palant wrote:
> There are some RSA keys in
>
https://hg.adblockplus.org/adblockpluschrome/file/b8f283613e94/qunit/tests/si...
> - how about using the 512 bit key from there instead?

Done.

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
chrome/content/tests/policy.js:65: let sitekey =
"MFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBANLNgMGCl6kc3QE/73/ESBsuhzCNQWApqWS1nKVWf+RjDTiF80LD2TiTdQhlW9mlWsDDbttsDlEcCmPVz6H3OUMCAwEAAQ==_peQ1CvuHieDHAXURwy9K8VQFaPWHHU0oDGT3pnIoQ6Nm7DUvCPw6FsnqfpdoIcq+kwqtkgWIcxM95gytuphvzQ==";
On 2014/09/10 21:55:06, Wladimir Palant wrote:
> Call this variable "signature"?

Technically, it's a concatentation of public key and signature. Therefore I
called it adblockkey now to be more consistent with how it's called in the
site's HTML code and HTTP header.

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
File chrome/content/tests/regexpFilters_matching.js (right):

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
chrome/content/tests/regexpFilters_matching.js:29: bar:
"MFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBALlKJGIBHYU3rIWyGmiwUgK1BUUomSBgLzSngoE7gBc7mqgCsEbjf9EJ5AvQIM1x7MhVsD5djpqHLcjiQlZFXfECAwEAAQ"
On 2014/09/10 21:55:06, Wladimir Palant wrote:
> Here as well: given that the keys aren't actually being used, how about using
> "foo+publickey" and "bar+publickey" instead?

Done.

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
chrome/content/tests/regexpFilters_matching.js:317: testMatch("abc$sitekey=" +
publickeys.foo, "http://abc/def", "IMAGE", "foo.com", true, null, false);
On 2014/09/10 21:55:06, Wladimir Palant wrote:
> What about the scenario where the sitekey is present but doesn't match the one
> specified in the filter?

Done.

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
chrome/content/tests/regexpFilters_matching.js:329: testMatch("abc$sitekey=" +
publickeys.foo + ",~image", "http://abc/def", "OBJECT", "foo.com", true,
publickeys.foo, true);
On 2014/09/10 21:55:06, Wladimir Palant wrote:
> This should test how the sitekeys get along with domain options rather than
type
> options.

Done. Let me know if it makes sense to also include the tests with swapped
filter options because I left them out for now.

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
File chrome/content/tests/signatures.js (right):

http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chro...
chrome/content/tests/signatures.js:2: * This file is part of Adblock Plus
<http://adblockplus.org/>,
On 2014/09/10 21:55:06, Wladimir Palant wrote:
> Just in case: Please make sure that this file is removed from the
> adblockpluschrome repository and imported from adblockplustests instead.

I created a ticket for that:
https://issues.adblockplus.org/ticket/1687

[Wladimir Palant, 2014-12-11 15:57:47]

LGTM but feel free to address the nit below.

http://codereview.adblockplus.org/6612283790721024/diff/5700735861784576/chro...
File chrome/content/tests/regexpFilters_matching.js (right):

http://codereview.adblockplus.org/6612283790721024/diff/5700735861784576/chro...
chrome/content/tests/regexpFilters_matching.js:292:
testMatch("abc$domain=bar.com,sitekey=foo-publickey", "http://abc/def", "IMAGE",
"bar.com", true, "foo-publickey", true);
Nit: the last two tests are redundant, they test the scenario "Domain specified
in filter along with sitekey and matches/doesn't match document domain" - this
was tested already, merely with foo.com and bar.com reversed.