Closed Bug 1397652 Opened 2 years ago Closed 2 years ago

Update tests within browser/ to comply with new toplevel data: URI navigation policy


(Core :: DOM: Security, enhancement, P2)




Tracking Status
firefox57 --- fixed


(Reporter: ckerschb, Assigned: ckerschb)



(Whiteboard: [domsecurity-active])


(1 file, 1 obsolete file)

No description provided.
Blocks: 1380959
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
Assignee: nobody → ckerschb
Priority: P3 → P2
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]
Attached patch bug_1397652_browser_tests.patch (obsolete) — Splinter Review
Gijs, we are about to block toplevel data: URI navigations within Firefox (because those navigations are mostly only used for phishing attempts). Flipping the pref |security.data_uri.block_toplevel_data_uri_navigations| causes the following tests (see undernath) within browser/ to fail. Please note that top level data: URI navigations triggered by a SystemPrincipal are allowed. For example, that allows data: URIs typed into the address bar to load, because we figured that web developers are using that feature quite often and there is no harm in allowing those loads.

Anyway, please find a summary of all the failing tests with a comment why and how I updated that test underneath. Thanks for your help!

(a) browser/base/content/test/general/browser_bug575561.js
I guess we want to keep test coverage so that pinned links to a data: URI should not open a new tab, hence I suggest we explicitly push the pref for this test.

(b) browser/base/content/test/general/browser_bug676619.js
Loading data: URI from external file.

(c) browser/base/content/test/general/browser_bug734076.js
Since this was an XSS attack (See Bug 734076), I would rather keep test coverage by explicitly pushing the pref.

(d) browser/base/content/test/urlbar/browser_bug562649.js
Loading the data: URI from an external dummy file. I personally prefer a separate file for each test, but jfyi within test/urlbar/ there is also dummy_page.html which we could reuse for that test. Happy to change if you prefer that.

(e) browser/components/extensions/test/browser/browser_ext_tabs_executeScript_bad.js
This test was added for Bug 1319070. I suppose it's best if we explicitly push the pref for that test, otherwise the line |  target.sendMessage("navigate", data);| causes window.location.href = url; to be naviagted to a data: URI and would fail.

(f) browser/components/originattributes/test/browser/browser_firstPartyIsolation_aboutPages.js
I guess we want to keep test coverage for first party isolation inheritance, hence I suppose it makes sense to push the pref explicitly.

(g) browser/components/privatebrowsing/test/browser/browser_privatebrowsing_newtab_from_popup.js
To be honest, it feels it's not worth the trouble to rewrite that test to comply with the new toplevel data: URI navigation policy. It seems quite some effort, for not much profit. If you prefer however, I would go through that trouble and update that test to load the data: URI from an external file.

(h) browser/components/sessionstore/test/browser_911547.js
We should keep test coverage for data: URI inheritance, hence explicitly pushing the pref

(i) browser/components/sessionstore/test/browser_async_duplicate_tab.js
Just loading the data: URI from an external file.

(j) browser/components/sessionstore/test/browser_async_flushes.js
The content of file_async_flushes.html is identical with file_async_duplicate.html. Do you prefer two files, or one file that is shared between the two tests?

(k) browser/components/sessionstore/test/browser_dynamic_frames.js
Rewriting this test would require quite some effort. Would you agree to just pushing the pref for that test?

(l) browser/components/sessionstore/test/browser_sessionHistory.js
Just loading the one data: URI that performs a reload from an external file.
Attachment #8906621 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8906621 [details] [diff] [review]

As discussed on IRC, there are a few more tests that I missed within this patch. Reason is simple. I just pushed to TRY running tests for Linux. Turns out some of the failing tests were disabled on Linux. Having a full TRY run revealed those other failing tests. Let me incorporate those changes and flag you for review again. Thanks!
Attachment #8906621 - Flags: review?(gijskruitbosch+bugs)
In addition to the tests that needed to be updated from comment 1, here are a few more tests and explanations of what I updated:

* browser/base/content/test/general/browser_fullscreen-window-open.js
Pushing the pref because otherwise setting the 'title' of a new document would require refactoring of the test.

* browser/base/content/test/popups/browser_popup_blocker.js
* browser/base/content/test/popups/browser_popup_frames.js
Loading the data: URIs from an external file.
Attachment #8906621 - Attachment is obsolete: true
Attachment #8907016 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8907016 [details] [diff] [review]

Review of attachment 8907016 [details] [diff] [review]:

These changes look good to me, besides the issue I noted below, which is a bit confusing...

::: browser/base/content/test/general/download_page.html
@@ +16,5 @@
>          <li><a href="video.ogg"
>                  download id="link2">Download "video.ogg"</a></li>
>          <li><a href="video.ogg"
>                  download="just some video" id="link3">Download "just some video"</a></li>
> +        <li><a href="download_page_1.txt"

Uh, this should be _2, right? How does the test still pass with the alternative URL? :-\
Attachment #8907016 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #4)
> ::: browser/base/content/test/general/download_page.html
> Uh, this should be _2, right? How does the test still pass with the
> alternative URL? :-\

You are right. The reason it works is because browser_bug676619.js does not verify the contents. Not the best behavior for a test, but orthogonal to the changes we are performing for this bug.
Pushed by
Update tests within browser/ to comply with new toplevel data: URI navigation policy. r=gijs
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.