[Navigation Timing] unload event test failures
Categories
(Core :: DOM: Core & HTML, defect, P2)
Tracking
()
People
(Reporter: yoav, Assigned: whawkins)
References
(Depends on 1 open bug)
Details
(Keywords: csectype-sop, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main69+][adv-esr68.1+][adv-esr60.9+])
Attachments
(2 files)
|
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr68+
|
Details | Review |
|
2.52 KB,
patch
|
RyanVM
:
approval-mozilla-esr60+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/75.0.3770.90 Safari/537.36
Steps to reproduce:
http://w3c-test.org/navigation-timing/unload-event-same-origin-check.html
Actual results:
3 of the test cases are failing
Expected results:
The spec was recently changed as TAO checks in the context of Navigation Timing made little sense: https://github.com/w3c/navigation-timing/pull/108
These failures can have security implications as exposing the unload event's timing can contain sensitive information about the previous page
| Assignee | ||
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
| Assignee | ||
Comment 1•6 years ago
|
||
| Assignee | ||
Updated•6 years ago
|
| Assignee | ||
Updated•6 years ago
|
| Assignee | ||
Comment 2•6 years ago
•
|
||
I'm not sure if the checkin-needed tag will work because this is a Security Issue. Can you advise, :baku? If that tag does not work, would you mind landing it?
Comment 3•6 years ago
|
||
https://hg.mozilla.org/integration/autoland/rev/65f2110d9bb9a28c621117c24526c1613c261192
checkin-needed for security bugs can be slower because fewer people can access that, but eventually the bugs shall land.
Comment 4•6 years ago
|
||
Backed out for wpt failures at navigation-timing/nav2_test_unloadEvents_with_cross_origin_redirect_opt_in.html:
https://hg.mozilla.org/integration/autoland/rev/31d23e8d5dfcc1e307bc14143b75858e85bc73ef
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=65f2110d9bb9a28c621117c24526c1613c261192
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=254389939&repo=autoland
TEST-UNEXPECTED-FAIL | /navigation-timing/nav2_test_unloadEvents_with_cross_origin_redirect_opt_in.html | Navigation Timing 2 WPT - uncaught exception: Error: assert_not_equals: Expected unloadEventStart to not be 0. got disallowed value 0
TEST-UNEXPECTED-PASS | /navigation-timing/nav2_test_unloadEvents_with_cross_origin_redirects.html | Navigation Timing 2 WPT - expected FAIL
| Assignee | ||
Comment 5•6 years ago
|
||
Ugh. The revert came because the tests in our tree are not up to date. I have an updated patch that disables the tests temporarily which, hopefully, means that this can land. I asked for a sanity check of the new patch and then we'll be able to move forward, I hope.
Will
Comment 6•6 years ago
|
||
(You can ask jgraham about the next wpt merge)
| Assignee | ||
Comment 7•6 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #6)
(You can ask jgraham about the next wpt merge)
I did, thanks :-) He's on PTO. Back tomorrow and he will do a wpt merge later this week, according to those in #automation. Thanks for the pointer :-)
Comment 8•6 years ago
|
||
You could also just locally apply the upstream test changes and land them, fwiw.
| Assignee | ||
Comment 9•6 years ago
|
||
I did that. They were auto reverted for causing failure in wpt tests.
Comment 10•6 years ago
|
||
Just to make sure we're on the same page, https://hg.mozilla.org/integration/autoland/rev/65f2110d9bb9a28c621117c24526c1613c261192 doesn't have wpt changes other than failure annotation removal, right?. What I meant is that you could apply the changes from https://github.com/web-platform-tests/wpt/commit/94f51553f1413cf2118b9bf89762bfad16d8a18a locally as part of your changeset and push them to our tree; presumably that should fix the test issues.
| Assignee | ||
Comment 11•6 years ago
|
||
Ah. Yes, I could do that. Sorry, I now understand. I am not 100% confident in doing that at this point. I don't want to mess up things that the automation team normally handles. This workaround is ok for now. Thanks for the suggestion -- it is definitely something that I will do as I get more confident working with the tree, etc.
Thanks again, :bzbarsky!
| Assignee | ||
Updated•6 years ago
|
Comment 12•6 years ago
|
||
Comment 13•6 years ago
|
||
Updated•6 years ago
|
Comment 14•6 years ago
|
||
Do we need to backport this to Beta/ESR68/ESR60?
| Assignee | ||
Comment 15•6 years ago
|
||
I honestly don't know. There are a few things to consider:
-
This change is the result of a recent change to a version of the spec that is not yet even official.
-
This is part of the Level 2 of the Navigation Timing API. I'm not sure there is an official stance on which version of Firefox supports which level of the API. If Beta/ESR68/ESR60 are advertised as supporting this level of the spec, then I think we have to backport. If they do not support Level 2, then there's no reason to backport.
I hope that helps you make a decision. I'm sorry that I don't have more information or an opinion.
Will
Comment 16•6 years ago
|
||
Andrea, maybe you can help shed some light?
Comment 17•6 years ago
|
||
PerformanceNavigationTiming::UnloadEventStart() and UnloadEventEnd() are in firefox since version 58.
I think it's good and important to uplift the patch to beta/esr68/esr60: same-origin check has been a problem elsewhere recently (canvas API, for instance) and we should land similar fixes as soon as we can. The patch doesn't introduce extra complexity, and it's safe to uplift it.
Comment 18•6 years ago
|
||
Thanks for the feedback. Looks like this patch grafts cleanly to Beta and ESR68 but will need some minor rebasing for ESR60. Can you please attach that rebased patch and then request Beta/ESR68/ESR60 approval where applicable, Will? Thanks!
| Assignee | ||
Comment 19•6 years ago
|
||
Thanks for the feedback, baku!
I'd be happy to do that, but it'll be the first time that I am doing such an uplift to an ESR. Is there documentation on where those branches live in the tree and how to make the request for uplift? Thanks Ryan!
Comment 20•6 years ago
|
||
Assuming you're using a unified repository, you can just run |hg update -C esr60|. Otherwise, if you need to clone it for some reason, you can pull it from https://hg.mozilla.org/releases/mozilla-esr60
To request approval, click the Details link on the attachment, select '?' from the appropriate approval-mozilla-<whatever> dropdown and answer the questions which pop up.
Comment 21•6 years ago
|
||
FWIW, you don't have to let the ESR60 rebase block the Beta and ESR68 uplifts. Especially for Beta, it would be good to get the patch in front of a wider audience sooner.
| Assignee | ||
Comment 22•6 years ago
|
||
Comment on attachment 9075222 [details]
Bug 1560495: Update unload event security information for Navigation-Timing Level 2 specification.
Beta/Release Uplift Approval Request
- User impact if declined: This is a security bug and could expose users to expose information about their browsing history.
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): This patch adds no increased complexity to the code base and shields users from potential information leakage.
- String changes made/needed:
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a security bug and could expose users to expose information about their browsing history.
- User impact if declined: This is a security bug and could expose users to expose information about their browsing history.
- Fix Landed on Version: Nightly
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): This patch adds no increased complexity to the code base and shields users from potential information leakage.
- String or UUID changes made by this patch:
| Assignee | ||
Comment 23•6 years ago
|
||
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: If this is not addressed in this ESR, it is possible that user browsing history will be exposed through timing side-channels.
- User impact if declined: If this is not addressed in this ESR, it is possible that user browsing history will be exposed through timing side-channels.
- Fix Landed on Version: Nightly
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): The change does not add any additional functionality or complexity.
- String or UUID changes made by this patch:
| Assignee | ||
Comment 24•6 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #21)
FWIW, you don't have to let the ESR60 rebase block the Beta and ESR68 uplifts. Especially for Beta, it would be good to get the patch in front of a wider audience sooner.
Thanks, Ryan! All flags are updated appropriately (I think) and the rebased patch for ESR60 uplift has been attached. I hope that I handled adding all the flags, etc correctly. Please let me know if I did something wrong.
Comment 25•6 years ago
|
||
Comment on attachment 9075222 [details]
Bug 1560495: Update unload event security information for Navigation-Timing Level 2 specification.
Fixes a security issue which can cause browsing history leakage. Approved for 68.0b7.
Comment 26•6 years ago
|
||
| uplift | ||
Comment 27•6 years ago
|
||
Comment on attachment 9075222 [details]
Bug 1560495: Update unload event security information for Navigation-Timing Level 2 specification.
This has baked on Nightly and Beta without issue for awhile now. Approved for 68.1esr and 60.9esr.
Updated•6 years ago
|
Comment 28•6 years ago
|
||
| uplift | ||
Comment 29•6 years ago
|
||
| uplift | ||
Comment 30•6 years ago
|
||
Confirmed issue with: 69.0a1 (2019-06-21), 70.0a1 (2019-08-04) on Windows 10.
Fix verified with 69.0b10, esr68.1.0, 70.0a1 (2019-08-04), on Windows 10, macOS 10.14,
With 60.9.0esr - 20190801163606 (treeherder build) there is a test Failing:
Fail No previous document with same origin redirect assert_equals: Expected redirectCount to be 1 expected 1 but got 0
run_test@http://w3c-test.org/navigation-timing/unload-event-same-origin-check.html:101:17
create_test/<@http://w3c-test.org/navigation-timing/unload-event-same-origin-check.html:108:25
Comment 31•6 years ago
|
||
@Will, mind taking a look and confirming if that test was indeed supposed to pass as well?
Updated•6 years ago
|
| Assignee | ||
Comment 33•6 years ago
|
||
I rolled back to before the fix for this issue landed in esr60 and found that this test is not something that we ever passed in esr60.
We now pass this test in later releases. I believe that if we want to resolve this issue for esr60 we should track it in a separate, non-security-sensitive issue.
Thoughts?
Comment 34•6 years ago
|
||
Sounds ok, assuming it wouldn't still cause any vulnerability to pop up.
Comment 35•6 years ago
|
||
Ok from my side. Just link the new bug for esr60 here.
Comment 36•6 years ago
|
||
Marking bug 1571990 for the follow-up.
Updated•6 years ago
|
Updated•6 years ago
|
Updated•5 years ago
|
Description
•