Merge pull request #196 from atom/iframe-security

Fix iframe security

browser/atom_browser_client.cc

@@ -54,7 +54,7 @@ void AtomBrowserClient::OverrideWebkitPrefs(
     const GURL& url,
     WebPreferences* prefs) {
   prefs->javascript_enabled = true;
-  prefs->web_security_enabled = false;
+  prefs->web_security_enabled = true;
   prefs->javascript_can_open_windows_automatically = true;
   prefs->plugins_enabled = false;
   prefs->dom_paste_enabled = true;

browser/native_window.cc

@@ -56,7 +56,7 @@ NativeWindow::NativeWindow(content::WebContents* web_contents,
     : content::WebContentsObserver(web_contents),
       has_frame_(true),
       is_closed_(false),
-      node_integration_("all"),
+      node_integration_("except-iframe"),
       has_dialog_attached_(false),
       weak_factory_(this),
       inspectable_web_contents_(

docs/api/browser/browser-window.md

@@ -48,9 +48,9 @@ Creates a new `BrowserWindow` with native properties set by the `options`.
 Usually you only need to set the `width` and `height`, other properties will
 have decent default values.
 
-By default the `node-integration` option is `all`, which means node integration
-is available to the main page and all its iframes. You can also set it to
-`except-iframe`, which would disable node integration in all iframes, or
+By default the `node-integration` option is `except-iframe`, which means node
+integration is disabled in all iframes, . You can also set it to `all`, with
+which node integration is available to the main page and all its iframes, or
 `manual-enable-iframe`, which is like `except-iframe`, but would enable iframes
 whose name is suffixed by `-enable-node-integration`. And setting to `disable`
 would disable the node integration in both the main page and its iframes.
@@ -66,6 +66,14 @@ An example of enable node integration in iframe with `node-integration` set to
 <iframe src="http://jandan.net"></iframe>
 ```
 
+And you should also notice that the iframes can have access to parent window's
+javascript objects via `window.parent`, so in order to grant complete security
+from iframes, you should add `sandbox` attribute to the iframes:
+
+```html
+<iframe sandbox="allow-scripts" src="http://bbs.seu.edu.cn"></iframe>
+```
+
 ### Event: 'page-title-updated'
 
 * `event` Event

renderer/atom_renderer_client.cc

@@ -21,10 +21,11 @@ namespace atom {
 namespace {
 
 // Security tokens.
-const char* kExceptIframe = "except-iframe";
-const char* kManualEnableIframe = "manual-enable-iframe";
-const char* kDisable = "disable";
-const char* kEnableNodeIntegration = "enable-node-integration";
+const char* kSecurityAll = "all";
+const char* kSecurityExceptIframe = "except-iframe";
+const char* kSecurityManualEnableIframe = "manual-enable-iframe";
+const char* kSecurityDisable = "disable";
+const char* kSecurityEnableNodeIntegration = "enable-node-integration";
 
 // Scheme used by devtools
 const char* kChromeDevToolsScheme = "chrome-devtools";
@@ -32,17 +33,19 @@ const char* kChromeDevToolsScheme = "chrome-devtools";
 }  // namespace
 
 AtomRendererClient::AtomRendererClient()
-    : node_integration_(ALL),
+    : node_integration_(EXCEPT_IFRAME),
       main_frame_(NULL) {
   // Translate the token.
   std::string token = CommandLine::ForCurrentProcess()->
       GetSwitchValueASCII(switches::kNodeIntegration);
-  if (token == kExceptIframe)
+  if (token == kSecurityExceptIframe)
     node_integration_ = EXCEPT_IFRAME;
-  else if (token == kManualEnableIframe)
+  else if (token == kSecurityManualEnableIframe)
     node_integration_ = MANUAL_ENABLE_IFRAME;
-  else if (token == kDisable)
+  else if (token == kSecurityDisable)
     node_integration_ = DISABLE;
+  else if (token == kSecurityAll)
+    node_integration_ = ALL;
 
   if (IsNodeBindingEnabled()) {
     node_bindings_.reset(NodeBindings::Create(false));
@@ -164,7 +167,7 @@ bool AtomRendererClient::IsNodeBindingEnabled(WebKit::WebFrame* frame) {
     return true;
   else if (node_integration_ == MANUAL_ENABLE_IFRAME &&
            frame != NULL &&
-           frame->uniqueName().utf8().find(kEnableNodeIntegration)
+           frame->uniqueName().utf8().find(kSecurityEnableNodeIntegration)
                == std::string::npos)
     return false;
   else if (node_integration_ == EXCEPT_IFRAME && frame != NULL)

spec/chromium-spec.coffee

@@ -1,6 +1,9 @@
 assert = require 'assert'
+path = require 'path'
 
 describe 'chromium feature', ->
+  fixtures = path.resolve __dirname, 'fixtures'
+
   describe 'heap snapshot', ->
     it 'does not crash', ->
       process.atomBinding('v8_util').takeHeapSnapshot()
@@ -21,3 +24,18 @@ describe 'chromium feature', ->
       b = window.open 'about:blank', 'test', 'show=no'
       assert.equal b.constructor.name, 'BrowserWindow'
       b.destroy()
+
+  describe 'iframe with sandbox attribute', ->
+    it 'can not modify parent', (done) ->
+      page = path.join fixtures, 'pages', 'change-parent.html'
+      global.changedByIframe = false
+
+      iframe = $('<iframe sandbox="allow-scripts">')
+      iframe.hide()
+      iframe.attr 'src', "file://#{page}"
+      iframe.appendTo 'body'
+      isChanged = ->
+        iframe.remove()
+        assert.equal global.changedByIframe, false
+        done()
+      setTimeout isChanged, 30

spec/fixtures/pages/change-parent.html

@@ -0,0 +1,8 @@
+<html>
+<body>
+<script type="text/javascript" charset="utf-8">
+  console.log('ready2')
+  window.parent.changedByIframe = true;
+</script>
+</body>
+</html>