Closed Bug 1398692 Opened 7 years ago Closed 7 years ago

Potentially allow navigations to toplevel data: PDFs

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(2 files, 3 obsolete files)

      No description provided.
Assignee: nobody → ckerschb
Blocks: 1380959
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Boris, since we were chatting about allowing data:application/pdf the other day. I suppose we could land a patch like the attached, but from my testing it seems that data:application/pdf is not even working anymore. Please note that Chrome treats data:application/pdf as other data: URIs and blocks such loads.

How do you think we should proceed?
Flags: needinfo?(bzbarsky)
> but from my testing it seems that data:application/pdf is not even working anymore

Not working in what sense?  It seems to work fine over here: offers for me to save the file (note that I have pdf.js turned off).

> Please note that Chrome treats data:application/pdf as other data: URIs and blocks such loads.

Yes, they got complaints about this on their mailing list...
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #2)
> Not working in what sense?  It seems to work fine over here: offers for me
> to save the file (note that I have pdf.js turned off).

Well, what testcase are you using? I tried to browse through our tree and couln't find any automated test, but happy to update add a specific test.

In general, I agree, if it's working, then we should allow data:application/pdf. Thanks.
> Well, what testcase are you using? 

   <a href="data:application/pdf,The data doesn't matter">Click me</a>
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #4)
>    <a href="data:application/pdf,The data doesn't matter">Click me</a>

my pdf itself was not valid, and I didn't realize because of the base64 encoding :-(
Boris, I know you are not accepting review requests at the moment, but this and the test in the following patch should be fairly straightforward. What do you think?
Attachment #8907079 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
Comment on attachment 8907175 [details] [diff] [review]
bug_1398692_allow_data_pdfs.patch

r=me
Flags: needinfo?(bzbarsky)
Attachment #8907175 - Flags: review+
Comment on attachment 8907176 [details] [diff] [review]
bug_1398692_test_allow_pdf.patch

>+SimpleTest.registerCleanupFunction(() => {

Why not just use pushPrefEnv here?  Do you really need a sync pref set?

>+    ok(wrappedWin.document.location.href.startsWith("data:application/pdf"),
>+       "data:application/pdf loaded");

This is definitely racy and will lead to random oranges... :(

If the load is blocked, what will happen with the window that was opened?  Will it get an error page?  Can we detect that?
Attached patch bug_1398692_test_allow_pdf.patch (obsolete) — Splinter Review
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #9)
> Why not just use pushPrefEnv here?
Done.

> This is definitely racy and will lead to random oranges... :(

I agree that setTimeOut is not great but unfortunately there is no onload or similar event event once the data:application/pdf is loaded into the new frame. I slightly improved by using setInterval. Even though this solution is also not great, I think it's the best option we have :-(

What do you think?
Attachment #8907176 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
Comment on attachment 8907588 [details] [diff] [review]
bug_1398692_test_allow_pdf.patch

I guess this is a bit better, as long as we unload the about:blank within 200ms... Have you run this on try a bunch of times to see how often you get random orange?

But anyway, a better option is to just poll at 200ms intervals until the location.href is correct and have the test time out on failure.  That's annoying when it actually fails, but shouldn't have randomorange more than any other test timeout issue we run into.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #11)
> But anyway, a better option is to just poll at 200ms intervals until the
> location.href is correct and have the test time out on failure.  That's
> annoying when it actually fails, but shouldn't have randomorange more than
> any other test timeout issue we run into.

Yeah, that makes sense, I'll run it through TRY a couple times anyway before landing. Thanks!
Attachment #8907588 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
Comment on attachment 8907721 [details] [diff] [review]
bug_1398692_test_allow_pdf.patch

r=me if we add a comment explaining why we're polling, etc.
Flags: needinfo?(bzbarsky)
Attachment #8907721 - Flags: review+
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b67372c4de0
Allow toplevel navigation to a data:application/pdf. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/54e49813d0c5
Test toplevel navigation to a data:application/pdf. r=bz
https://hg.mozilla.org/mozilla-central/rev/0b67372c4de0
https://hg.mozilla.org/mozilla-central/rev/54e49813d0c5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1418642
You need to log in before you can comment on or make changes to this bug.