Add URL argument to Scratch.canDownload

This lets us filter out javascript: URLs
TurboWarp · Dec 22, 2024 · fdd954c · fdd954c
1 parent 1a578aa
commit fdd954c
Show file tree

Hide file tree

Showing 5 changed files with 62 additions and 16 deletions.
diff --git a/src/extension-support/tw-security-manager.js b/src/extension-support/tw-security-manager.js
@@ -154,11 +154,12 @@ class SecurityManager {
     }
 
     /**
-     * Determine whether an extension is allowed to download a file with a given name.
-     * @param {string} filename The name of the file
+     * Determine whether an extension is allowed to download a URL with a given name.
+     * @param {string} resourceURL The URL to download
+     * @param {string} name The name of the file
      * @returns {Promise<boolean>|boolean}
      */
-    canDownload (filename) {
+    canDownload (resourceURL, name) {
         return Promise.resolve(true);
     }
 }

diff --git a/src/extension-support/tw-unsandboxed-extension-runner.js b/src/extension-support/tw-unsandboxed-extension-runner.js
@@ -96,7 +96,18 @@ const setupUnsandboxedExtensionAPI = vm => new Promise(resolve => {
         return vm.securityManager.canEmbed(parsed.href);
     };
 
-    Scratch.canDownload = async name => vm.securityManager.canDownload(name);
+    Scratch.canDownload = async (url, name) => {
+        const parsed = parseURL(url);
+        if (!parsed) {
+            return false;
+        }
+        // Always reject protocols that would allow code execution.
+        // eslint-disable-next-line no-script-url
+        if (parsed.protocol === 'javascript:') {
+            return false;
+        }
+        return vm.securityManager.canDownload(url, name);
+    };
 
     Scratch.fetch = async (url, options) => {
         const actualURL = url instanceof Request ? url.url : url;

diff --git a/test/integration/tw_security_manager.js b/test/integration/tw_security_manager.js
@@ -271,17 +271,21 @@ test('canDownload', async t => {
     const vm = new VirtualMachine();
     setupUnsandboxedExtensionAPI(vm);
 
-    const calledWithNames = [];
-    vm.securityManager.canDownload = async name => {
-        calledWithNames.push(name);
+    const calledWithArguments = [];
+    vm.securityManager.canDownload = async (url, name) => {
+        calledWithArguments.push([url, name]);
         return name.includes('safe');
     };
 
-    t.equal(await global.Scratch.canDownload('safe.html'), true);
-    t.equal(await global.Scratch.canDownload('dangerous.html'), false);
-    t.same(calledWithNames, [
-        'safe.html',
-        'dangerous.html'
+    t.equal(await global.Scratch.canDownload('http://example.com/', 'safe.html'), true);
+    t.equal(await global.Scratch.canDownload('http://example.com/', 'dangerous.html'), false);
+
+    // should not even call security manager
+    t.equal(await global.Scratch.canDownload('javascript:alert(1)', 'safe.html'), false);
+
+    t.same(calledWithArguments, [
+        ['http://example.com/', 'safe.html'],
+        ['http://example.com/', 'dangerous.html']
     ]);
 
     t.end();

diff --git a/test/unit/tw_sandboxed_extensions.js b/test/unit/tw_sandboxed_extensions.js
@@ -143,6 +143,6 @@ test('canEmbed', async t => {
 });
 
 test('canDownload', async t => {
-    t.equal(await global.Scratch.canDownload('test.sb3'), false);
+    t.equal(await global.Scratch.canDownload('https://example.com/test.sb3', 'test.sb3'), false);
     t.end();
 });
diff --git a/test/unit/tw_unsandboxed_extensions.js b/test/unit/tw_unsandboxed_extensions.js
@@ -422,9 +422,39 @@ test('canDownload', async t => {
     const vm = new VirtualMachine();
     UnsandboxedExtensionRunner.setupUnsandboxedExtensionAPI(vm);
 
-    vm.securityManager.canDownload = name => name === 'safe.txt';
-    t.ok(await global.Scratch.canDownload('safe.txt'));
-    t.notOk(await global.Scratch.canDownload('unsafe.txt'));
+    const canDownloadChecks = [];
+    vm.securityManager.canDownload = (url, name) => {
+        canDownloadChecks.push([url, name]);
+        return url === 'https://example.com/safe.txt' && name === 'safe.txt';
+    };
+
+    t.ok(await global.Scratch.canDownload('https://example.com/safe.txt', 'safe.txt'));
+    t.notOk(await global.Scratch.canDownload('https://example.com/unsafe.txt', 'safe.txt'));
+    t.notOk(await global.Scratch.canDownload('https://example.com/safe.txt', 'unsafe.txt'));
+    t.notOk(await global.Scratch.canDownload('https://example.com/unsafe.txt', 'unsafe.txt'));
+
+    // Should be rejected without even calling user-provided canDownload.
+    // eslint-disable-next-line no-script-url
+    t.notOk(await global.Scratch.canDownload('javascript:alert(1)', 'index.html'));
+
+    t.same(canDownloadChecks, [
+        [
+            'https://example.com/safe.txt',
+            'safe.txt'
+        ],
+        [
+            'https://example.com/unsafe.txt',
+            'safe.txt'
+        ],
+        [
+            'https://example.com/safe.txt',
+            'unsafe.txt'
+        ],
+        [
+            'https://example.com/unsafe.txt',
+            'unsafe.txt'
+        ]
+    ]);
 
     t.end();
 });