Closed
Bug 1398692
Opened 7 years ago
Closed 7 years ago
Potentially allow navigations to toplevel data: PDFs
Categories
(Core :: DOM: Security, enhancement, P2)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: ckerschb, Assigned: ckerschb)
References
Details
(Whiteboard: [domsecurity-active])
Attachments
(2 files, 3 obsolete files)
1.24 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
2.61 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ckerschb
Blocks: 1380959
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Assignee | ||
Comment 1•7 years ago
|
||
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)
Comment 2•7 years ago
|
||
> 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)
Assignee | ||
Comment 3•7 years ago
|
||
(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.
Comment 4•7 years ago
|
||
> Well, what testcase are you using?
<a href="data:application/pdf,The data doesn't matter">Click me</a>
Assignee | ||
Comment 5•7 years ago
|
||
(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 :-(
Assignee | ||
Comment 6•7 years ago
|
||
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)
Assignee | ||
Comment 7•7 years ago
|
||
Comment 8•7 years ago
|
||
Comment on attachment 8907175 [details] [diff] [review] bug_1398692_allow_data_pdfs.patch r=me
Flags: needinfo?(bzbarsky)
Attachment #8907175 -
Flags: review+
Comment 9•7 years ago
|
||
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?
Assignee | ||
Comment 10•7 years ago
|
||
(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 11•7 years ago
|
||
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)
Assignee | ||
Comment 12•7 years ago
|
||
(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 13•7 years ago
|
||
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+
Comment 14•7 years ago
|
||
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
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0b67372c4de0 https://hg.mozilla.org/mozilla-central/rev/54e49813d0c5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•