Merge upstream

.github/workflows/deploy.yml

@@ -13,9 +13,11 @@ jobs:
   build:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v4
+      - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683
+        with:
+          persist-credentials: false
       - name: Install Node.js
-        uses: actions/setup-node@v4
+        uses: actions/setup-node@39370e3970a6d050c480ffad4ff0ed4d3fdee5af
         with:
           node-version: 20
           cache: npm
@@ -29,7 +31,7 @@ jobs:
         # It will still generate what it can, so it's safe to ignore the error
         continue-on-error: true
       - name: Upload artifact
-        uses: actions/upload-pages-artifact@v3
+        uses: actions/upload-pages-artifact@56afc609e74202658d3ffba0e8f6dda462b719fa
         with:
           path: ./playground/
 
@@ -45,4 +47,4 @@ jobs:
     steps:
     - name: Deploy to GitHub Pages
       id: deployment
-      uses: actions/deploy-pages@v4
+      uses: actions/deploy-pages@d6db90164ac5ed86f2b6aed7e0febac5b3c0c03e

.github/workflows/node.js.yml

@@ -8,9 +8,11 @@ jobs:
   build:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v4
+      - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683
+        with:
+          persist-credentials: false
       - name: Install Node.js
-        uses: actions/setup-node@v4
+        uses: actions/setup-node@39370e3970a6d050c480ffad4ff0ed4d3fdee5af
         with:
           node-version: 20
           cache: npm

src/compiler/irgen.js

@@ -19,17 +19,12 @@ const LIST_TYPE = 'list';
  */
 
 /**
- * Create a variable codegen object.
- * @param {'target'|'stage'} scope The scope of this variable -- which object owns it.
- * @param {import('../engine/variable.js')} varObj The Scratch Variable
- * @returns {*} A variable codegen object.
+ * @typedef DescendedVariable
+ * @property {'target'|'stage'} scope
+ * @property {string} id
+ * @property {string} name
+ * @property {boolean} isCloud
  */
-const createVariableData = (scope, varObj) => ({
-    scope,
-    id: varObj.id,
-    name: varObj.name,
-    isCloud: varObj.isCloud
-});
 
 /**
  * @param {string} code
@@ -1229,35 +1224,49 @@ class ScriptTreeGenerator {
         const variable = block.fields[fieldName];
         const id = variable.id;
 
-        if (Object.prototype.hasOwnProperty.call(this.variableCache, id)) {
+        if (id && Object.prototype.hasOwnProperty.call(this.variableCache, id)) {
             return this.variableCache[id];
         }
 
         const data = this._descendVariable(id, variable.value, type);
-        this.variableCache[id] = data;
+        // If variable ID was null, this might do some unnecessary updates, but that is a rare
+        // edge case and it won't have any adverse effects anyways.
+        this.variableCache[data.id] = data;
         return data;
     }
 
     /**
-     * @param {string} id The ID of the variable.
+     * @param {string|null} id The ID of the variable.
      * @param {string} name The name of the variable.
      * @param {''|'list'} type The variable type.
      * @private
-     * @returns {*} A parsed variable object.
+     * @returns {DescendedVariable} A parsed variable object.
      */
     _descendVariable (id, name, type) {
         const target = this.target;
         const stage = this.stage;
 
         // Look for by ID in target...
         if (Object.prototype.hasOwnProperty.call(target.variables, id)) {
-            return createVariableData('target', target.variables[id]);
+            const currVar = target.variables[id];
+            return {
+                scope: 'target',
+                id: currVar.id,
+                name: currVar.name,
+                isCloud: currVar.isCloud
+            };
         }
 
         // Look for by ID in stage...
         if (!target.isStage) {
             if (stage && Object.prototype.hasOwnProperty.call(stage.variables, id)) {
-                return createVariableData('stage', stage.variables[id]);
+                const currVar = stage.variables[id];
+                return {
+                    scope: 'stage',
+                    id: currVar.id,
+                    name: currVar.name,
+                    isCloud: currVar.isCloud
+                };
             }
         }
 
@@ -1266,7 +1275,12 @@ class ScriptTreeGenerator {
             if (Object.prototype.hasOwnProperty.call(target.variables, varId)) {
                 const currVar = target.variables[varId];
                 if (currVar.name === name && currVar.type === type) {
-                    return createVariableData('target', currVar);
+                    return {
+                        scope: 'target',
+                        id: currVar.id,
+                        name: currVar.name,
+                        isCloud: currVar.isCloud
+                    };
                 }
             }
         }
@@ -1277,14 +1291,22 @@ class ScriptTreeGenerator {
                 if (Object.prototype.hasOwnProperty.call(stage.variables, varId)) {
                     const currVar = stage.variables[varId];
                     if (currVar.name === name && currVar.type === type) {
-                        return createVariableData('stage', currVar);
+                        return {
+                            scope: 'stage',
+                            id: currVar.id,
+                            name: currVar.name,
+                            isCloud: currVar.isCloud
+                        };
                     }
                 }
             }
         }
 
         // Create it locally...
         const newVariable = new Variable(id, name, type, false);
+
+        // Intentionally not using newVariable.id so that this matches vanilla Scratch quirks regarding
+        // handling of null variable IDs.
         target.variables[id] = newVariable;
 
         if (target.sprite) {
@@ -1298,7 +1320,14 @@ class ScriptTreeGenerator {
             }
         }
 
-        return createVariableData('target', newVariable);
+        return {
+            scope: 'target',
+            // If the given ID was null, this won't match the .id property of the Variable object.
+            // This is intentional to match vanilla Scratch quirks.
+            id,
+            name: newVariable.name,
+            isCloud: newVariable.isCloud
+        };
     }
 
     descendProcedure (block) {

src/extension-support/extension-manager.js

@@ -277,19 +277,25 @@ class ExtensionManager {
 
     /**
      * Regenerate blockinfo for any loaded extensions
+     * @param {string} [optExtensionId] Optional extension ID for refreshing
      * @returns {Promise} resolved once all the extensions have been reinitialized
      */
-    refreshBlocks () {
-        const allPromises = Array.from(this._loadedExtensions.values()).map(serviceName =>
-            dispatch.call(serviceName, 'getInfo')
-                .then(info => {
-                    info = this._prepareExtensionInfo(serviceName, info);
-                    dispatch.call('runtime', '_refreshExtensionPrimitives', info);
-                })
-                .catch(e => {
-                    log.error('Failed to refresh built-in extension primitives', e);
-                })
-        );
+    refreshBlocks (optExtensionId) {
+        const refresh = serviceName => dispatch.call(serviceName, 'getInfo')
+            .then(info => {
+                info = this._prepareExtensionInfo(serviceName, info);
+                dispatch.call('runtime', '_refreshExtensionPrimitives', info);
+            })
+            .catch(e => {
+                log.error('Failed to refresh built-in extension primitives', e);
+            });
+        if (optExtensionId) {
+            if (!this._loadedExtensions.has(optExtensionId)) {
+                return Promise.reject(new Error(`Unknown extension: ${optExtensionId}`));
+            }
+            return refresh(this._loadedExtensions.get(optExtensionId));
+        }
+        const allPromises = Array.from(this._loadedExtensions.values()).map(refresh);
         return Promise.all(allPromises);
     }
 

src/extension-support/extension-worker.js

@@ -86,6 +86,8 @@ Object.assign(global.Scratch, ScratchCommon, {
     canNotify: () => Promise.resolve(false),
     canGeolocate: () => Promise.resolve(false),
     canEmbed: () => Promise.resolve(false),
+    canDownload: () => Promise.resolve(false),
+    download: () => Promise.reject(new Error('Scratch.download not supported in sandboxed extensions')),
     translate
 });
 

src/extension-support/tw-security-manager.js

@@ -152,6 +152,16 @@ class SecurityManager {
     canEmbed (documentURL) {
         return Promise.resolve(true);
     }
+
+    /**
+     * 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 (resourceURL, name) {
+        return Promise.resolve(true);
+    }
 }
 
 module.exports = SecurityManager;

src/extension-support/tw-unsandboxed-extension-runner.js

@@ -96,6 +96,19 @@ const setupUnsandboxedExtensionAPI = vm => new Promise(resolve => {
         return vm.securityManager.canEmbed(parsed.href);
     };
 
+    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;
 
@@ -127,6 +140,18 @@ const setupUnsandboxedExtensionAPI = vm => new Promise(resolve => {
         location.href = url;
     };
 
+    Scratch.download = async (url, name) => {
+        if (!await Scratch.canDownload(url, name)) {
+            throw new Error(`Permission to download ${name} rejected.`);
+        }
+        const link = document.createElement('a');
+        link.href = url;
+        link.download = name;
+        document.body.appendChild(link);
+        link.click();
+        link.remove();
+    };
+
     Scratch.translate = createTranslate(vm);
 
     global.Scratch = Scratch;

src/virtual-machine.js

@@ -221,6 +221,7 @@ class VirtualMachine extends EventEmitter {
             Sprite,
             RenderedTarget,
             JSZip,
+            Variable,
 
             i_will_not_ask_for_help_when_these_break: () => {
                 console.warn('You are using unsupported APIs. WHEN your code breaks, do not expect help.');

test/integration/tw_automatic_variable_creation.js

@@ -0,0 +1,43 @@
+const fs = require('fs');
+const path = require('path');
+const {test} = require('tap');
+const VM = require('../../src/virtual-machine');
+
+for (const compilerEnabled of [false, true]) {
+    const prefix = compilerEnabled ? 'compiler' : 'interpreter';
+    test(`${prefix} - quirks when block field has literal null for variable ID`, t => {
+        const vm = new VM();
+        vm.setCompilerOptions({
+            enabled: compilerEnabled
+        });
+        t.equal(vm.runtime.compilerOptions.enabled, compilerEnabled, 'compiler options sanity check');
+
+        // The execute tests ensure that this fixture compiles and runs fine and the snapshot test ensures
+        // it compiles correctly. This additional test will ensure that the internal variable objects are
+        // being created with the expected properties.
+        const fixturePath = path.join(
+            __dirname,
+            '../fixtures/execute/tw-automatic-variable-creation-literal-null-id.sb3'
+        );
+
+        vm.loadProject(fs.readFileSync(fixturePath)).then(() => {
+            vm.greenFlag();
+            vm.runtime._step();
+
+            // Variable does not exist, should get made as local variable in sprite
+            const variables = vm.runtime.targets[1].variables;
+            t.equal(Object.keys(variables).length, 1, 'created 1 new variable');
+
+            // Scratch quirk - the entry in .variables should have key "null"
+            const newVariableKey = Object.keys(variables)[0];
+            t.equal(newVariableKey, 'null', 'key is "null"');
+
+            // Scratch quirk - the actual variable.id should be the random string
+            const newVariable = Object.values(variables)[0];
+            t.notEqual(newVariable.id, 'null', 'variable.id is not "null"');
+            t.type(newVariable.id, 'string', 'variable.id is a string');
+
+            t.end();
+        });
+    });
+}

test/integration/tw_security_manager.js

@@ -266,3 +266,27 @@ test('canEmbed', async t => {
 
     t.end();
 });
+
+test('canDownload', async t => {
+    const vm = new VirtualMachine();
+    setupUnsandboxedExtensionAPI(vm);
+
+    const calledWithArguments = [];
+    vm.securityManager.canDownload = async (url, name) => {
+        calledWithArguments.push([url, name]);
+        return name.includes('safe');
+    };
+
+    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();
+});

test/snapshot/__snapshots__/tw-automatic-variable-creation-literal-null-id.sb3.tw-snapshot

@@ -0,0 +1,13 @@
+// TW Snapshot
+// Input SHA-256: 4764ae15e39b22b1a071c9ac79f8758f24ef41855802db8674e200fd26139ed0
+
+// Sprite1 script
+(function factoryXYZ(thread) { const target = thread.target; const runtime = target.runtime; const stage = runtime.getTargetForStage();
+const b0 = runtime.getOpcodeFunction("looks_say");
+const b1 = target.variables["null"];
+return function* genXYZ () {
+yield* executeInCompatibilityLayer({"MESSAGE":"plan 0",}, b0, false, false, "a", null);
+b1.value = 5;
+yield* executeInCompatibilityLayer({"MESSAGE":"end",}, b0, false, false, "d", null);
+retire(); return;
+}; })