Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2744413002:
Set view source without creating a unique origin in XMLDocumentParser (Closed)
DescriptionSet view source without creating a unique origin in XMLDocumentParser
BUG=697830
Review-Url: https://codereview.chromium.org/2744413002
Cr-Commit-Position: refs/heads/master@{#457168}
Committed: https://chromium.googlesource.com/chromium/src/+/3de53e557c2b84aed1cb4ba9ed1133ac361da46c
Patch Set 1 #Patch Set 2 : Add test #
Total comments: 2
Patch Set 3 : Remove unique security origin #
Messages
Total messages: 30 (17 generated)
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
adithyas@chromium.org changed reviewers: + dcheng@chromium.org, haraken@chromium.org, jochen@chromium.org
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM but I want to have jochen@ confirm this.
what kind of scripts do we run in the xml document? Similar to how we set the onload handler in the layout test, a site could also overwrite all kinds of prototypes that will make those scripts behave in unexpected ways. Maybe we should check whether the XML document was touched by script before, and if yes, we just don't render it at all instead of showing it via the XML tree viewer. If the window wasn't touched by script before, we can safely put it into a unique origin.. https://codereview.chromium.org/2744413002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp (right): https://codereview.chromium.org/2744413002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp:1551: if (document()->canExecuteScripts(NotAboutToExecuteScript)) why this if ()?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/14 at 20:13:53, jochen wrote: > what kind of scripts do we run in the xml document? Similar to how we set the onload handler in the layout test, a site could also overwrite all kinds of prototypes that will make those scripts behave in unexpected ways. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/xml/Docum... is the script that's run, and it's run in an isolated world, so changing prototypes shouldn't affect the script I think. https://codereview.chromium.org/2744413002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp (right): https://codereview.chromium.org/2744413002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp:1551: if (document()->canExecuteScripts(NotAboutToExecuteScript)) On 2017/03/14 at 20:13:53, jochen wrote: > why this if ()? I thought we still need the unique origin when script is disabled (which what this patch tried to do I think: https://chromium.googlesource.com/chromium/src/+/c92a57f13f7b09b68e5c0534129f...).
Actually, I tried looking into which document types use setViewSource, and its just the XML tree viewer and HTMLViewSourceDocument. By the looks of it, HTMLViewSourceDocument does not actually execute any script. So, enabling script for viewSource and the setting of a unique origin was just added for XML tree viewer in order to enable it to run a custom script when script was disabled. But considering we now use an isolated world to run a custom script for XMLTreeViewer, and isolated worlds don't really check if script is disabled, I don't think we need to enable script for viewSources. So I setting m_isViewSource should really just make the document use the view source stylesheet (which is what it used to do before https://chromium.googlesource.com/chromium/src/+/c92a57f13f7b09b68e5c0534129f...) and should not cause Document::canExecuteScript to return true. What do you think?
yeah, if we inject in an isolated world, we should be able to leave canExecuteScript alone
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I removed the creation of a unique origin & the viewSource check in canExecuteScripts, PTAL.
lgtm
LGTM based on the fact that https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/xml/Docum... seems to be using isolated worlds. However, I am a bit nervous about this change overall. I'm wondering if two followup changes would make sense: - Change view source to be a const field and set at construction time. This will be harder for the XML tree viewer, but we do have some precedent for switching documents during XML processing already: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/xml/XSLTP... - Restrict script execution on view source docs to only be allowed in the view-source isolated world. WDYT?
On 2017/03/15 at 18:48:05, dcheng wrote: > LGTM based on the fact that https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/xml/Docum... seems to be using isolated worlds. > > However, I am a bit nervous about this change overall. I'm wondering if two followup changes would make sense: > - Change view source to be a const field and set at construction time. This will be harder for the XML tree viewer, but we do have some precedent for switching documents during XML processing already: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/xml/XSLTP... > - Restrict script execution on view source docs to only be allowed in the view-source isolated world. > > WDYT? Hmm, currently only XMLTreeViewer seems to be running any script, so I don't know if we really need to be generalizing this right now for future cases. Right now, does being a view source document have any special significance right now other than the fact it loads and uses a common stylesheet (i.e. view-source.css)? Calling setViewSource() right now just affects style and I don't know if that really needs to be set at construction time.
The CQ bit was checked by adithyas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2744413002/#ps40001 (title: "Remove unique security origin")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489605971308480,
"parent_rev": "5e99bba0da407e116176cec28f370a84ca10d658", "commit_rev":
"3de53e557c2b84aed1cb4ba9ed1133ac361da46c"}
Message was sent while issue was closed.
Description was changed from ========== Set view source without creating a unique origin in XMLDocumentParser BUG=697830 ========== to ========== Set view source without creating a unique origin in XMLDocumentParser BUG=697830 Review-Url: https://codereview.chromium.org/2744413002 Cr-Commit-Position: refs/heads/master@{#457168} Committed: https://chromium.googlesource.com/chromium/src/+/3de53e557c2b84aed1cb4ba9ed11... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/3de53e557c2b84aed1cb4ba9ed11...
Message was sent while issue was closed.
On 2017/03/15 19:26:07, adithyas wrote: > On 2017/03/15 at 18:48:05, dcheng wrote: > > LGTM based on the fact that > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/xml/Docum... > seems to be using isolated worlds. > > > > However, I am a bit nervous about this change overall. I'm wondering if two > followup changes would make sense: > > - Change view source to be a const field and set at construction time. This > will be harder for the XML tree viewer, but we do have some precedent for > switching documents during XML processing already: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/xml/XSLTP... > > - Restrict script execution on view source docs to only be allowed in the > view-source isolated world. > > > > WDYT? > > Hmm, currently only XMLTreeViewer seems to be running any script, so I don't > know if we really need to be generalizing this right now for future cases. > Right now, does being a view source document have any special significance right > now other than the fact it loads and uses a common stylesheet (i.e. > view-source.css)? Calling setViewSource() right now just affects style and I > don't know if that really needs to be set at construction time. View source docs are kind of weird, and I want to make it less likely for weird bugs to creep in. For example, we used to have this weird bug where content scripts would be injected into view-source URLs: https://crbug.com/43384 -- so if we could strongly enforce invariants we know should be true, that would help preventatively. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
