- Revision
- 99330
- Author
- [email protected]
- Date
- 2011-11-04 15:50:54 -0700 (Fri, 04 Nov 2011)
Log Message
Delete FrameLoader::isSandboxed
https://bugs.webkit.org/show_bug.cgi?id=71591
Reviewed by Eric Seidel.
Source/WebCore:
We should always use document->securityOrigin()->isSandboxed because
that picks up the sandbox bits that are frozen on the document rather
than the ones that could change on the Frame.
This patch starts preparing us to implement the CSP sandbox directive,
which will cause use to have document sandbox bits without any attribute.
* bindings/ScriptControllerBase.cpp:
(WebCore::ScriptController::canExecuteScripts):
- This call site was the only functional site left where these two
could be different. This patch causes one progression and one
regression. The progression is that we now correctly freeze the
allow-scripts bit when a document is created, but the regression
is we now allow the execution of _javascript_ URLs, as noted in
fast/frames/sandboxed-iframe-scripting.html. That's even more of
an edge case, so I think it's a win overall.
* loader/DocumentWriter.cpp:
(WebCore::DocumentWriter::begin):
- I don't think this part of the change is testable. There's no
time to execute script between when the bits get copied off the
Frame and when they're checked, so there's no time to change them.
* loader/FrameLoader.cpp:
- Update FIXME comment that is now fixed.
* loader/FrameLoader.h:
- Remove wrong API.
LayoutTests:
This patch updates this test to check a few more cases and notes a bug
in our current implementation.
* fast/frames/resources/sandboxed-iframe-script-dynamic.html:
* fast/frames/sandboxed-iframe-scripting-expected.txt:
* fast/frames/sandboxed-iframe-scripting.html:
Modified Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (99329 => 99330)
--- trunk/LayoutTests/ChangeLog 2011-11-04 22:32:13 UTC (rev 99329)
+++ trunk/LayoutTests/ChangeLog 2011-11-04 22:50:54 UTC (rev 99330)
@@ -1,5 +1,19 @@
2011-11-04 Adam Barth <[email protected]>
+ Delete FrameLoader::isSandboxed
+ https://bugs.webkit.org/show_bug.cgi?id=71591
+
+ Reviewed by Eric Seidel.
+
+ This patch updates this test to check a few more cases and notes a bug
+ in our current implementation.
+
+ * fast/frames/resources/sandboxed-iframe-script-dynamic.html:
+ * fast/frames/sandboxed-iframe-scripting-expected.txt:
+ * fast/frames/sandboxed-iframe-scripting.html:
+
+2011-11-04 Adam Barth <[email protected]>
+
When CSP blocks mixed content, we shouldn't also warn about mixed content
https://bugs.webkit.org/show_bug.cgi?id=71588
Modified: trunk/LayoutTests/fast/frames/resources/sandboxed-iframe-script-dynamic.html (99329 => 99330)
--- trunk/LayoutTests/fast/frames/resources/sandboxed-iframe-script-dynamic.html 2011-11-04 22:32:13 UTC (rev 99329)
+++ trunk/LayoutTests/fast/frames/resources/sandboxed-iframe-script-dynamic.html 2011-11-04 22:50:54 UTC (rev 99330)
@@ -4,8 +4,8 @@
frame = window.top.document.getElementById('frame');
- // setting the sandbox flag at runtime should not terminate already
- // running scripts (e.g., this one)
+ // setting the sandbox flag at runtime should not change whether scripts
+ // are allowed in this document.
frame.sandbox = 'allow-same-origin'; // NO allow-scripts
@@ -16,7 +16,7 @@
function onload_hook()
{
- ++window.top.disallowedExecuted;
+ ++window.top.allowedExecuted;
};
</script>
Modified: trunk/LayoutTests/fast/frames/sandboxed-iframe-scripting-expected.txt (99329 => 99330)
--- trunk/LayoutTests/fast/frames/sandboxed-iframe-scripting-expected.txt 2011-11-04 22:32:13 UTC (rev 99329)
+++ trunk/LayoutTests/fast/frames/sandboxed-iframe-scripting-expected.txt 2011-11-04 22:50:54 UTC (rev 99330)
@@ -1,3 +1,4 @@
+ALERT: PASS: Executed script in data URL
Verify that sandboxed frames with sandbox="allow-scripts" can execute scripts, but other sandboxed frames cannot. Also verify that removing the sandbox="allow-scripts" attribute at runtime prevents new scripts from launching, but existing ones keep running.
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
@@ -3,8 +4,8 @@
-PASS disallowedExecuted is 0
-PASS allowedExecuted is 2
+FAIL disallowedExecuted should be 0. Was 1.
+PASS allowedExecuted is 3
PASS successfullyParsed is true
TEST COMPLETE
-
+
Modified: trunk/LayoutTests/fast/frames/sandboxed-iframe-scripting.html (99329 => 99330)
--- trunk/LayoutTests/fast/frames/sandboxed-iframe-scripting.html 2011-11-04 22:32:13 UTC (rev 99329)
+++ trunk/LayoutTests/fast/frames/sandboxed-iframe-scripting.html 2011-11-04 22:50:54 UTC (rev 99330)
@@ -11,7 +11,7 @@
window._onload_ = function()
{
shouldBe("disallowedExecuted", "0");
- shouldBe("allowedExecuted", "2");
+ shouldBe("allowedExecuted", "3");
isSuccessfullyParsed();
}
@@ -23,10 +23,20 @@
<iframe sandbox="allow-same-origin allow-scripts"
src="" ++window.top.allowedExecuted;">
</iframe>
+ <iframe sandbox="allow-same-origin allow-scripts"
+ src="" alert('PASS: Executed script in data URL'); </script>">
+ </iframe>
+ <!-- This case execute script even though it shouldn't. The problem is that
+ the iframe starts out as about:blank, which inherits the parent documents
+ SecurityOrigin, along with its frozen sandbox bits. To fix this case,
+ we'll need to move the sandbox bits from SecurityOrigin to Document. --!>
<iframe sandbox="allow-same-origin"
src="" ++window.top.disallowedExecuted;">
</iframe>
-
+ <iframe sandbox="allow-same-origin"
+ src="" alert('FAIL: Executed script without allow-scripts in data URL'); </script>">
+ </iframe>
+
<iframe id="frame" src=""
</iframe>
Modified: trunk/Source/WebCore/ChangeLog (99329 => 99330)
--- trunk/Source/WebCore/ChangeLog 2011-11-04 22:32:13 UTC (rev 99329)
+++ trunk/Source/WebCore/ChangeLog 2011-11-04 22:50:54 UTC (rev 99330)
@@ -1,5 +1,38 @@
2011-11-04 Adam Barth <[email protected]>
+ Delete FrameLoader::isSandboxed
+ https://bugs.webkit.org/show_bug.cgi?id=71591
+
+ Reviewed by Eric Seidel.
+
+ We should always use document->securityOrigin()->isSandboxed because
+ that picks up the sandbox bits that are frozen on the document rather
+ than the ones that could change on the Frame.
+
+ This patch starts preparing us to implement the CSP sandbox directive,
+ which will cause use to have document sandbox bits without any attribute.
+
+ * bindings/ScriptControllerBase.cpp:
+ (WebCore::ScriptController::canExecuteScripts):
+ - This call site was the only functional site left where these two
+ could be different. This patch causes one progression and one
+ regression. The progression is that we now correctly freeze the
+ allow-scripts bit when a document is created, but the regression
+ is we now allow the execution of _javascript_ URLs, as noted in
+ fast/frames/sandboxed-iframe-scripting.html. That's even more of
+ an edge case, so I think it's a win overall.
+ * loader/DocumentWriter.cpp:
+ (WebCore::DocumentWriter::begin):
+ - I don't think this part of the change is testable. There's no
+ time to execute script between when the bits get copied off the
+ Frame and when they're checked, so there's no time to change them.
+ * loader/FrameLoader.cpp:
+ - Update FIXME comment that is now fixed.
+ * loader/FrameLoader.h:
+ - Remove wrong API.
+
+2011-11-04 Adam Barth <[email protected]>
+
When CSP blocks mixed content, we shouldn't also warn about mixed content
https://bugs.webkit.org/show_bug.cgi?id=71588
Modified: trunk/Source/WebCore/bindings/ScriptControllerBase.cpp (99329 => 99330)
--- trunk/Source/WebCore/bindings/ScriptControllerBase.cpp 2011-11-04 22:32:13 UTC (rev 99329)
+++ trunk/Source/WebCore/bindings/ScriptControllerBase.cpp 2011-11-04 22:50:54 UTC (rev 99330)
@@ -37,8 +37,7 @@
bool ScriptController::canExecuteScripts(ReasonForCallingCanExecuteScripts reason)
{
- // FIXME: We should get this information from the document instead of the frame.
- if (m_frame->loader()->isSandboxed(SandboxScripts))
+ if (m_frame->document() && m_frame->document()->securityOrigin()->isSandboxed(SandboxScripts))
return false;
if (m_frame->document() && m_frame->document()->isViewSource()) {
Modified: trunk/Source/WebCore/loader/DocumentWriter.cpp (99329 => 99330)
--- trunk/Source/WebCore/loader/DocumentWriter.cpp 2011-11-04 22:32:13 UTC (rev 99329)
+++ trunk/Source/WebCore/loader/DocumentWriter.cpp 2011-11-04 22:50:54 UTC (rev 99330)
@@ -119,7 +119,7 @@
// If the new document is for a Plugin but we're supposed to be sandboxed from Plugins,
// then replace the document with one whose parser will ignore the incoming data (bug 39323)
- if (document->isPluginDocument() && m_frame->loader()->isSandboxed(SandboxPlugins))
+ if (document->isPluginDocument() && document->securityOrigin()->isSandboxed(SandboxPlugins))
document = SinkDocument::create(m_frame, url);
// FIXME: Do we need to consult the content security policy here about blocked plug-ins?
Modified: trunk/Source/WebCore/loader/FrameLoader.cpp (99329 => 99330)
--- trunk/Source/WebCore/loader/FrameLoader.cpp 2011-11-04 22:32:13 UTC (rev 99329)
+++ trunk/Source/WebCore/loader/FrameLoader.cpp 2011-11-04 22:50:54 UTC (rev 99330)
@@ -161,7 +161,6 @@
// non-member lets us exclude it from the header file, thus keeping FrameLoader.h's
// API simpler.
//
-// FIXME: isDocumentSandboxed should eventually replace isSandboxed.
static bool isDocumentSandboxed(Frame* frame, SandboxFlags mask)
{
return frame->document() && frame->document()->securityOrigin()->isSandboxed(mask);
Modified: trunk/Source/WebCore/loader/FrameLoader.h (99329 => 99330)
--- trunk/Source/WebCore/loader/FrameLoader.h 2011-11-04 22:32:13 UTC (rev 99329)
+++ trunk/Source/WebCore/loader/FrameLoader.h 2011-11-04 22:50:54 UTC (rev 99330)
@@ -211,7 +211,6 @@
void ownerElementSandboxFlagsChanged() { updateSandboxFlags(); }
- bool isSandboxed(SandboxFlags mask) const { return m_sandboxFlags & mask; }
SandboxFlags sandboxFlags() const { return m_sandboxFlags; }
// The following sandbox flags will be forced, regardless of changes to
// the sandbox attribute of any parent frames.
