Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1217503012:
Avoid cross-origin iframe issues when loading PDF in print preview (Closed)
|
Created:
5 years, 5 months ago by raymes Modified:
4 years, 11 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, darin-cc_chromium.org, jam, arv+watch_chromium.org, extensions-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid cross-origin iframe issues when loading PDF in print preview
There are several issues that arise because we load the PDF viewer in a cross-origin iframe in print preview:
1) It doesn't get access to chrome:// resources by default (we have a hack in place, see crbug.com/444752)
2) The above hack causes its own problems (see crbug.com/461987).
3) Cross-origin policy prevents direct communication between the two frames DOMs which has made some implementation more difficult (we could simplify some code after this patch).
4) It's necessary that the two frames run in the same process because Chrome's printing needs direct access to the plugin element. With site isolation, the extension could be loaded up in a separate process, we don't want this.
This patch causes the iframe to load in the same origin as print preview and then all the resources are pulled in from the Chrome extension.
Ideally we would turn the PDF viewer into a polymer element which would encapsulate it better and make it clearer what is going on in this situation but this will require some additional work.
BUG=444752, 461987
Committed: https://crrev.com/de5f44d770c51d181f04ff3bb029dee8ac733d63
Cr-Commit-Position: refs/heads/master@{#367293}
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : #Patch Set 4 : #
Total comments: 1
Patch Set 5 : #Patch Set 6 : #
Total comments: 2
Patch Set 7 : #
Total comments: 1
Messages
Total messages: 49 (20 generated)
The CQ bit was checked by raymes@chromium.org
The CQ bit was unchecked by raymes@chromium.org
sammc@chromium.org changed reviewers: + sammc@chromium.org
lgtm
raymes@chromium.org changed reviewers: + thestig@chromium.org
+thestig@ for print preview related changes.
thestig@chromium.org changed reviewers: + tsepez@chromium.org
Deferring to Tom for the CSP bits in print_preview_ui.cc.
On 2015/07/14 23:53:56, Lei Zhang wrote: > Deferring to Tom for the CSP bits in print_preview_ui.cc. Wouldn't this allow the untrusted DOM to directly call any of the WebUI bindings in the print preview window?
tsepez@chromium.org changed reviewers: + creis@chromium.org
+charlie Would this also grant webui permissions to an ordinary renderer process?
The DOM for the PDF viewer isn't untrusted. It's in the Chrome repo, written by us - it's not really any different from the DOM running in the parent frame. All this does is load up the PDF viewer html to run in the print preview renderer process (which it does right now anyway). I don't think it would be any different if we made a copy of all the PDF viewer html code and copied it into print preview. Basically we just want to share that code.
On 2015/07/15 00:07:37, raymes wrote: > The DOM for the PDF viewer isn't untrusted. It's in the Chrome repo, written by > us - it's not really any different from the DOM running in the parent frame. > > All this does is load up the PDF viewer html to run in the print preview > renderer process (which it does right now anyway). I don't think it would be any > different if we made a copy of all the PDF viewer html code and copied it into > print preview. Basically we just want to share that code. Ah, OK. LGTM>
On 2015/07/15 16:48:06, Tom Sepez wrote: > On 2015/07/15 00:07:37, raymes wrote: > > The DOM for the PDF viewer isn't untrusted. It's in the Chrome repo, written > by > > us - it's not really any different from the DOM running in the parent frame. > > > > All this does is load up the PDF viewer html to run in the print preview > > renderer process (which it does right now anyway). I don't think it would be > any > > different if we made a copy of all the PDF viewer html code and copied it into > > print preview. Basically we just want to share that code. > > Ah, OK. LGTM> Thanks! thestig@ are you ok with this?
lgtm
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1217503012/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
I don't have a great understanding of the print preview architecture, but (4) in the CL description concerns me: > 4) It's necessary that the two frames run in the same process because Chrome's printing needs direct access to the plugin element. With site isolation, the extension could be loaded up in a separate process, we don't want this. What were the two origins before, and what origin do they share now? Is the same-process requirement only true until we get printing to work with OOPIFs? (Lei is planning to work on that soon, IIUC.) I think I'm ok with this change, but I haven't figured out the implications for which pages are getting privileged processes, etc. A few other minor questions below. https://codereview.chromium.org/1217503012/diff/20001/chrome/browser/resource... File chrome/browser/resources/pdf/manifest.json (right): https://codereview.chromium.org/1217503012/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/manifest.json:22: "*.html", Why do you need both this and index.html? https://codereview.chromium.org/1217503012/diff/20001/content/browser/webui/u... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/1217503012/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:85: std::string URLToRequestPath(const GURL& url) { Why can't we just use GURL::path()? This seems to depend on the parsed version for the offset anyway.
What were the two origins before, and what origin do they share now? Is the same-process requirement only true until we get printing to work with OOPIFs? (Lei is planning to work on that soon, IIUC.) The two origins were chrome://print (in the parent frame) and a component extension chrome-extension:// in the child frame. There isn't really a reason to load the chrome-extension directly though, it was just a way of code sharing. We could copy all of the PDF html into print preview and have it all served from chrome://print but this will increase our binary size. So instead we just basically html import it. Ideally this would be nicely componentized as a polymer element and then this would be clearer but the work hasn't been done for that yet. The same origin requirement is because our print preview code reaches directly into chrome://print DOM grabs the PDF plugin and calls print on it directly. I don't fully know the details behind why it works this way unfortunately. I'm sure there are ways to rework the code to avoid doing this directly and avoid the same-process requirement but I also wanted to make sure this didn't somehow block OOP iframes (note that this was only one of several reasons for this change, it was really just in the back of my mind). Thanks Charlie! https://codereview.chromium.org/1217503012/diff/20001/chrome/browser/resource... File chrome/browser/resources/pdf/manifest.json (right): https://codereview.chromium.org/1217503012/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/manifest.json:22: "*.html", Good point, we actually don't now. Done. https://codereview.chromium.org/1217503012/diff/20001/content/browser/webui/u... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/1217503012/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:85: std::string URLToRequestPath(const GURL& url) { On 2015/07/16 19:25:03, Charlie Reis wrote: > Why can't we just use GURL::path()? This seems to depend on the parsed version > for the offset anyway. I don't know the answer to that, I didn't write this :) Perhaps we can. Do you want me to do this in a followup patch?
Ok, thanks for the explanation. I'm happy with that. https://codereview.chromium.org/1217503012/diff/20001/content/browser/webui/u... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/1217503012/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:85: std::string URLToRequestPath(const GURL& url) { On 2015/07/17 01:58:29, raymes wrote: > On 2015/07/16 19:25:03, Charlie Reis wrote: > > Why can't we just use GURL::path()? This seems to depend on the parsed > version > > for the offset anyway. > > I don't know the answer to that, I didn't write this :) Perhaps we can. Do you > want me to do this in a followup patch? Wow, this goes back to the initial commit: https://chromium.googlesource.com/chromium/src/+blame/be7f2174ece293c452eb122... I suppose we can leave it, but it'd be good to add tests for the query case in url_data_manager_backend_unittest.cc if you can. Parsing logic is almost always worth testing.
https://codereview.chromium.org/1217503012/diff/20001/content/browser/webui/u... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/1217503012/diff/20001/content/browser/webui/u... content/browser/webui/url_data_manager_backend.cc:85: std::string URLToRequestPath(const GURL& url) { I no longer change this file in the latest patchset. The approach I had here didn't work because the dom distiller does something tricky and relies on Web UI URLs that can have ? in them. I moved this code into UrlDataSource. Does this seem ok to you? If so I can write a small test for that file.
https://codereview.chromium.org/1217503012/diff/60001/content/public/browser/... File content/public/browser/url_data_source.cc (right): https://codereview.chromium.org/1217503012/diff/60001/content/public/browser/... content/public/browser/url_data_source.cc:69: *path = path->substr(0, path->find_last_of('?')); I don't think this is a good change to make. Things in content/public should mainly be interfaces, and the other implementations in this file are small and don't have side effects. This code makes a meaningful change to the path in a way that isn't obvious, and it's easy to accidentally prevent this change (without realizing it) by overriding the method.
Charlie: could you take another look at this. I moved that functionality out of the content/public interface and into content/browser/webui/web_ui_data_source_impl.cc and added a small test. Thanks!
raymes@chromium.org changed reviewers: + estaab@chromium.org
+estade for content/browser/webui/*
content/ LGTM.
estaab@chromium.org changed reviewers: + estade@chromium.org - estaab@chromium.org
The CQ bit was checked by raymes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1217503012/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1217503012/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by raymes@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, sammc@chromium.org, tsepez@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/1217503012/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1217503012/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1217503012/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
https://codereview.chromium.org/1217503012/diff/100001/content/browser/webui/... File content/browser/webui/web_ui_data_source_impl.cc (right): https://codereview.chromium.org/1217503012/diff/100001/content/browser/webui/... content/browser/webui/web_ui_data_source_impl.cc:77: *path = path->substr(0, path->find_last_of('?')); I'm sure someone more knowledgeable than me has already taken a look at this, but why is find_last_of the right thing? If it's possible to have multiple ?s, why is path.js?q=string?r=strong being changed into path.js?q=string and not path.js
https://codereview.chromium.org/1217503012/diff/100001/content/browser/webui/... File content/browser/webui/web_ui_data_source_impl.cc (right): https://codereview.chromium.org/1217503012/diff/100001/content/browser/webui/... content/browser/webui/web_ui_data_source_impl.cc:77: *path = path->substr(0, path->find_last_of('?')); Oops good catch. Thank you and I added a test to catch this :)
The CQ bit was checked by raymes@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, sammc@chromium.org, creis@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/1217503012/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1217503012/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1217503012/120001
Message was sent while issue was closed.
Description was changed from ========== Avoid cross-origin iframe issues when loading PDF in print preview There are several issues that arise because we load the PDF viewer in a cross-origin iframe in print preview: 1) It doesn't get access to chrome:// resources by default (we have a hack in place, see crbug.com/444752) 2) The above hack causes its own problems (see crbug.com/461987). 3) Cross-origin policy prevents direct communication between the two frames DOMs which has made some implementation more difficult (we could simplify some code after this patch). 4) It's necessary that the two frames run in the same process because Chrome's printing needs direct access to the plugin element. With site isolation, the extension could be loaded up in a separate process, we don't want this. This patch causes the iframe to load in the same origin as print preview and then all the resources are pulled in from the Chrome extension. Ideally we would turn the PDF viewer into a polymer element which would encapsulate it better and make it clearer what is going on in this situation but this will require some additional work. BUG=444752,461987 ========== to ========== Avoid cross-origin iframe issues when loading PDF in print preview There are several issues that arise because we load the PDF viewer in a cross-origin iframe in print preview: 1) It doesn't get access to chrome:// resources by default (we have a hack in place, see crbug.com/444752) 2) The above hack causes its own problems (see crbug.com/461987). 3) Cross-origin policy prevents direct communication between the two frames DOMs which has made some implementation more difficult (we could simplify some code after this patch). 4) It's necessary that the two frames run in the same process because Chrome's printing needs direct access to the plugin element. With site isolation, the extension could be loaded up in a separate process, we don't want this. This patch causes the iframe to load in the same origin as print preview and then all the resources are pulled in from the Chrome extension. Ideally we would turn the PDF viewer into a polymer element which would encapsulate it better and make it clearer what is going on in this situation but this will require some additional work. BUG=444752,461987 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Avoid cross-origin iframe issues when loading PDF in print preview There are several issues that arise because we load the PDF viewer in a cross-origin iframe in print preview: 1) It doesn't get access to chrome:// resources by default (we have a hack in place, see crbug.com/444752) 2) The above hack causes its own problems (see crbug.com/461987). 3) Cross-origin policy prevents direct communication between the two frames DOMs which has made some implementation more difficult (we could simplify some code after this patch). 4) It's necessary that the two frames run in the same process because Chrome's printing needs direct access to the plugin element. With site isolation, the extension could be loaded up in a separate process, we don't want this. This patch causes the iframe to load in the same origin as print preview and then all the resources are pulled in from the Chrome extension. Ideally we would turn the PDF viewer into a polymer element which would encapsulate it better and make it clearer what is going on in this situation but this will require some additional work. BUG=444752,461987 ========== to ========== Avoid cross-origin iframe issues when loading PDF in print preview There are several issues that arise because we load the PDF viewer in a cross-origin iframe in print preview: 1) It doesn't get access to chrome:// resources by default (we have a hack in place, see crbug.com/444752) 2) The above hack causes its own problems (see crbug.com/461987). 3) Cross-origin policy prevents direct communication between the two frames DOMs which has made some implementation more difficult (we could simplify some code after this patch). 4) It's necessary that the two frames run in the same process because Chrome's printing needs direct access to the plugin element. With site isolation, the extension could be loaded up in a separate process, we don't want this. This patch causes the iframe to load in the same origin as print preview and then all the resources are pulled in from the Chrome extension. Ideally we would turn the PDF viewer into a polymer element which would encapsulate it better and make it clearer what is going on in this situation but this will require some additional work. BUG=444752,461987 Committed: https://crrev.com/de5f44d770c51d181f04ff3bb029dee8ac733d63 Cr-Commit-Position: refs/heads/master@{#367293} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/de5f44d770c51d181f04ff3bb029dee8ac733d63 Cr-Commit-Position: refs/heads/master@{#367293}
Message was sent while issue was closed.
primiano@chromium.org changed reviewers: + primiano@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1217503012/diff/120001/content/browser/webui/... File content/browser/webui/web_ui_data_source_impl.cc (right): https://codereview.chromium.org/1217503012/diff/120001/content/browser/webui/... content/browser/webui/web_ui_data_source_impl.cc:79: // We want to remove any query strings from the path. The funny thing is that [1] relies on the opposite behavior. (And sadly there doesn't seem to be any test convering the tracing UI integration :/) [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/tr... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
