1695658 - Land tests for SSL spoofing in bug 1667456

Status: RESOLVED FIXED

(Firefox :: Address Bar, task)

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1695658 - tests, r?johannh

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:Gijs, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 3

[:Gijs (he/him)]

This can't land until we ship the fix.

Comment 4

[Julien Cristau [:jcristau]]

Landed:
tests, r=johannh
https://hg.mozilla.org/integration/autoland/rev/e202be0555579d96898c6a2a7330a038622f50c0

And backed out for causing failures at browser_identityBlock_focus.js:
https://hg.mozilla.org/integration/autoland/rev/dbb126a8c73310a7d8060609e5016e2bd30e1e7b

e.g. https://treeherder.mozilla.org/jobs?repo=autoland&revision=e202be0555579d96898c6a2a7330a038622f50c0&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedTaskRun=ff_Dy52HS4uMz6t_sEr51Q.0

Comment 5

[:Gijs (he/him)]

Well, this was tedious to debug. It would appear that the opening of the subframe adds a 3rdPartyStorage permission for example.org on example.com, which then causes an additional icon to show up in the identityBlock_focus test, which breaks that test because it expects that right-arrowing from the identity block focuses the anchor directly, without showing the permissions icon as well.

Johann, is it worth filing a follow-up to reset the permissions database after each test (from a cleanup function in head.js) for the identity block and permissions panel tests, or is that overkill?

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

tests, r=johannh
https://hg.mozilla.org/integration/autoland/rev/3748ad25c134e5bbccea2fdf7f07dafee41eb5cd
https://hg.mozilla.org/mozilla-central/rev/3748ad25c134

Comment 7

[Johann Hofmann [:johannh]]

Ugh sorry that's the anti-tracking heuristics and it comes from opening the popup. I'm a bit confused that the permission is set in this test because it should only be granted when both parties have user interaction. Tim, do you know why the permission is granted in this test?

Comment 8

[Tim Huang[:timhuang]]

The user interaction is only required if the third-party is on the pref list privacy.restrict3rdpartystorage.userInteractionRequiredForHosts. If it's not on the list, we will grant the permission when doing window.open(). So, the permission will be set in this case because example.org is not on the list.

Comment 9

[Johann Hofmann [:johannh]]

Ok, thanks! I guess this is a somewhat unexpected side-effect of all popup opening tests. It's probably helpful to clean up permissions regularly in the site identity tests but I don't want to take too much of your time on that, Gijs.

Comment 10

[Johann Hofmann [:johannh]]

I also wonder if we should put mochitest proxied hosts like example.org on this list in automation to avoid setting the permission too often, we can still remove them from the list for tests that test the heuristics.

Comment 11

[:Gijs (he/him)]

(In reply to Johann Hofmann [:johannh] from comment #9)

Ok, thanks! I guess this is a somewhat unexpected side-effect of all popup opening tests. It's probably helpful to clean up permissions regularly in the site identity tests but I don't want to take too much of your time on that, Gijs.

How hard could it be? (Ha, ha)

I filed bug 1719289.

Comment 12

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/d014495881f6

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out from esr78 for causing failures in browser_about_blank_same_document_tabswitch.js:

https://hg.mozilla.org/releases/mozilla-esr78/rev/a531fa5534b23a5eeaf71ea3084c95b9ca94d0a7

Push with failures: https://treeherder.mozilla.org/jobs?repo=mozilla-esr78&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=0d35cd18cdd357fb07e511ea5b922d95bdd3f812&selectedTaskRun=NtMS6I00Rc2AWZVIko2nXA.0
Failure log: https://treeherder.mozilla.org/logviewer?job_id=346028820&repo=mozilla-esr78

[task 2021-07-22T14:18:14.043Z] 14:18:14 INFO - TEST-PASS | browser/base/content/test/siteIdentity/browser_about_blank_same_document_tabswitch.js | URL bar value should be correct, was example.org/browser/browser/base/content/test/siteIdentity/open-self-from-frame.html -
[task 2021-07-22T14:18:14.043Z] 14:18:14 INFO - Buffered messages finished
[task 2021-07-22T14:18:14.050Z] 14:18:14 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/siteIdentity/browser_about_blank_same_document_tabswitch.js | Identity box should have been updated. - Got unknownIdentity, expected notSecure
[task 2021-07-22T14:18:14.051Z] 14:18:14 INFO - Stack trace:
[task 2021-07-22T14:18:14.052Z] 14:18:14 INFO - chrome://mochikit/content/browser-test.js:test_is:1327
[task 2021-07-22T14:18:14.053Z] 14:18:14 INFO - chrome://mochitests/content/browser/browser/base/content/test/siteIdentity/browser_about_blank_same_document_tabswitch.js:test_identityBlock_inherited_blank/<:62
[task 2021-07-22T14:18:14.054Z] 14:18:14 INFO - Leaving test bound test_identityBlock_inherited_blank

Comment 14

[:Gijs (he/him)]

I'm kind of confused why this landed on esr78 without an uplift request ever having been filed...

though to be fair, I would probably not have predicted the issue here.

I think this can reland with https://hg.mozilla.org/releases/mozilla-esr78/rev/d014495881f6#l2.68 updated to check for unknownIdentity instead of notSecure, as I believe that was a change that was made after esr78, but I don't have time to check this myself right now.

How important is this test-only patch landing on esr78? I would have thought that it doesn't really matter, especially given how close we are to 91esr at this point.

Comment 15

[Julien Cristau [:jcristau]]

(In reply to :Gijs (he/him) from comment #14)

I'm kind of confused why this landed on esr78 without an uplift request ever having been filed...

FWIW I uplifted this based on the tracking flag for 78.13 being set (and as a test-only change we don't normally require a formal uplift request).

though to be fair, I would probably not have predicted the issue here.

I think this can reland with https://hg.mozilla.org/releases/mozilla-esr78/rev/d014495881f6#l2.68 updated to check for unknownIdentity instead of notSecure, as I believe that was a change that was made after esr78, but I don't have time to check this myself right now.

How important is this test-only patch landing on esr78? I would have thought that it doesn't really matter, especially given how close we are to 91esr at this point.

It's probably not terribly important...