Browse Source

fix: apply csp correctly when contextIsolation: false (#37756)

* fix: apply csp correctly when contextIsolation: false

* better comments
Jeremy Rose 2 years ago
parent
commit
e9d5c3517c
2 changed files with 86 additions and 23 deletions
  1. 19 2
      shell/common/node_bindings.cc
  2. 67 21
      spec/chromium-spec.ts

+ 19 - 2
shell/common/node_bindings.cc

@@ -184,9 +184,10 @@ v8::ModifyCodeGenerationFromStringsResult ModifyCodeGenerationFromStrings(
     v8::Local<v8::Context> context,
     v8::Local<v8::Value> source,
     bool is_code_like) {
-  // If we're running with contextIsolation enabled in the renderer process,
-  // fall back to Blink's logic.
   if (node::Environment::GetCurrent(context) == nullptr) {
+    // No node environment means we're in the renderer process, either in a
+    // sandboxed renderer or in an unsandboxed renderer with context isolation
+    // enabled.
     if (gin_helper::Locker::IsBrowserProcess()) {
       NOTREACHED();
       return {false, {}};
@@ -195,6 +196,22 @@ v8::ModifyCodeGenerationFromStringsResult ModifyCodeGenerationFromStrings(
         context, source, is_code_like);
   }
 
+  // If we get here then we have a node environment, so either a) we're in the
+  // main process, or b) we're in the renderer process in a context that has
+  // both node and blink, i.e. contextIsolation disabled.
+
+  // If we're in the main process, delegate to node.
+  if (gin_helper::Locker::IsBrowserProcess()) {
+    return node::ModifyCodeGenerationFromStrings(context, source, is_code_like);
+  }
+
+  // If we're in the renderer with contextIsolation disabled, ask blink first
+  // (for CSP), and iff that allows codegen, delegate to node.
+  v8::ModifyCodeGenerationFromStringsResult result =
+      blink::V8Initializer::CodeGenerationCheckCallbackInMainThread(
+          context, source, is_code_like);
+  if (!result.codegen_allowed)
+    return result;
   return node::ModifyCodeGenerationFromStrings(context, source, is_code_like);
 }
 

+ 67 - 21
spec/chromium-spec.ts

@@ -352,28 +352,74 @@ describe('web security', () => {
     });
   });
 
-  describe('csp in sandbox: false', () => {
-    it('is correctly applied', async () => {
-      const w = new BrowserWindow({
-        show: false,
-        webPreferences: { sandbox: false }
+  describe('csp', () => {
+    for (const sandbox of [true, false]) {
+      describe(`when sandbox: ${sandbox}`, () => {
+        for (const contextIsolation of [true, false]) {
+          describe(`when contextIsolation: ${contextIsolation}`, () => {
+            it('prevents eval from running in an inline script', async () => {
+              const w = new BrowserWindow({
+                show: false,
+                webPreferences: { sandbox, contextIsolation }
+              });
+              w.loadURL(`data:text/html,<head>
+              <meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self' 'unsafe-inline'">
+            </head>
+            <script>
+              try {
+                // We use console.log here because it is easier than making a
+                // preload script, and the behavior under test changes when
+                // contextIsolation: false
+                console.log(eval('true'))
+              } catch (e) {
+                console.log(e.message)
+              }
+            </script>`);
+              const [,, message] = await once(w.webContents, 'console-message');
+              expect(message).to.match(/Refused to evaluate a string/);
+            });
+
+            it('does not prevent eval from running in an inline script when there is no csp', async () => {
+              const w = new BrowserWindow({
+                show: false,
+                webPreferences: { sandbox, contextIsolation }
+              });
+              w.loadURL(`data:text/html,
+            <script>
+              try {
+                // We use console.log here because it is easier than making a
+                // preload script, and the behavior under test changes when
+                // contextIsolation: false
+                console.log(eval('true'))
+              } catch (e) {
+                console.log(e.message)
+              }
+            </script>`);
+              const [,, message] = await once(w.webContents, 'console-message');
+              expect(message).to.equal('true');
+            });
+
+            it('prevents eval from running in executeJavaScript', async () => {
+              const w = new BrowserWindow({
+                show: false,
+                webPreferences: { sandbox, contextIsolation }
+              });
+              w.loadURL('data:text/html,<head><meta http-equiv="Content-Security-Policy" content="default-src \'self\'; script-src \'self\' \'unsafe-inline\'"></meta></head>');
+              await expect(w.webContents.executeJavaScript('eval("true")')).to.be.rejected();
+            });
+
+            it('does not prevent eval from running in executeJavaScript when there is no csp', async () => {
+              const w = new BrowserWindow({
+                show: false,
+                webPreferences: { sandbox, contextIsolation }
+              });
+              w.loadURL('data:text/html,');
+              expect(await w.webContents.executeJavaScript('eval("true")')).to.be.true();
+            });
+          });
+        }
       });
-      w.loadURL(`data:text/html,<head>
-          <meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self' 'unsafe-inline'">
-        </head>
-        <script>
-          try {
-            // We use console.log here because it is easier than making a
-            // preload script, and the behavior under test changes when
-            // contextIsolation: false
-            console.log(eval('failure'))
-          } catch (e) {
-            console.log('success')
-          }
-        </script>`);
-      const [,, message] = await once(w.webContents, 'console-message');
-      expect(message).to.equal('success');
-    });
+    }
   });
 
   it('does not crash when multiple WebContent are created with web security disabled', () => {