Skip to content

feat: webgl ci cd#8744

Open
NickKhalow wants to merge 40 commits into
devfrom
feat/webgl-ci-cd
Open

feat: webgl ci cd#8744
NickKhalow wants to merge 40 commits into
devfrom
feat/webgl-ci-cd

Conversation

@NickKhalow
Copy link
Copy Markdown
Contributor

TODO

Pull Request Description

What does this PR change?

Test Instructions

Steps (standard run):

metaforge explorer run XXXX  # ← replace with this PR number

Expected result:

Steps (fresh account):

metaforge account create --clear
metaforge explorer run XXXX  # ← replace with this PR number

Expected result:

Automation (if applicable):

metaforge explorer test XXXX

Prerequisites

  • List any required setup steps
  • Include environment/configuration requirements

Test Steps

  1. First step
  2. Second step
  3. Expected result after step 2
  4. ...

Additional Testing Notes

  • Note any edge cases to verify
  • Mention specific areas that need careful testing
  • List known limitations or potential issues

Quality Checklist

  • Changes have been tested locally
  • Documentation has been updated (if required)
  • Performance impact has been considered
  • For SDK features: Test scene is included

Code Review Reference

Please review our Branch & PR Standards before submitting. It explains the automated review flow, QA/DEV approval requirements, and what each label does — especially useful for first-time contributors.

@NickKhalow NickKhalow self-assigned this May 12, 2026
@NickKhalow NickKhalow requested review from a team as code owners May 12, 2026 11:40
# Conflicts:
#	Explorer/Assets/DCL/Infrastructure/SceneRunner/SceneFactory.cs
#	Explorer/Assets/DCL/Infrastructure/SceneRunner/SceneInstanceDeps.cs
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 12, 2026

badge

Build failed! Check the logs to see what went wrong

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 12, 2026

Claude finished @NickKhalow's task in 3m 8s —— View job


Dependency / Supply-Chain Security Review — PR #8744

Scope: this review focuses on external dependencies and supply-chain surface introduced by feat/webgl-ci-cd against origin/dev. Behavioral changes (system logic, UI) are out of scope.

Summary

Item Type Source / Version Scope Risk Confidence
com.atteneder.draco removed (was 4.0.2 via OpenUPM) Managed package OpenUPM runtime LOW HIGH
org.decentraland.clearscript ref changed #7.5.1.a#v2 (resolved 5f385df…ae0c51ae…) Git package (DCL fork of Microsoft ClearScript / V8) github.com/decentraland/ClearScript.git runtime — JS engine MEDIUM MEDIUM
Utility.asmdef adds ref to JsWebSockets asmdef (GUID 7b6e9563…) Assembly reference local plugin runtime (WebGL) LOW HIGH
WebGLEditor.cs (new) — launches Chrome with --disable-web-security, spawns python3 -m http.server Editor C# script local editor-only (#if UNITY_EDITOR) LOW (caveat) HIGH
LocalStorageDCLPlayerPrefs.cs (new) — [DllImport("__Internal")] for DCLPrefs_Save/DCLPrefs_Load C# / WebGL native interop local runtime (WebGL) LOW (correctness issue, not security) HIGH
.github/workflows/build-unitycloud.yml, pr-comment-artifact-url.yml CI workflows local CI LOW HIGH
ProjectSettings.asset — WebGL scripting-defines now match Standalone (incl. ALTTESTER) Build config local runtime (WebGL) LOW (parity, see note) HIGH

No new native binaries, .dll/.so/.dylib/.bundle, or precompiled assemblies were added in this PR.


MEDIUM-risk item: ClearScript ref changed to #v2

[PR] Explorer/Packages/manifest.json:

-    "org.decentraland.clearscript": "https://github.com/decentraland/ClearScript.git?path=/Unity/Package#7.5.1.a",
+    "org.decentraland.clearscript": "https://github.com/decentraland/ClearScript.git?path=/Unity/Package#v2",

[PR] packages-lock.json records the new resolved commit ae0c51ae4a6f2325f9757e9a0253ddbef8713a9b (previously 5f385df93c2d1e4a9aed3dd0aad62c247c684a4b).

Concerns:

  1. Mutability of the git ref. 7.5.1.a reads as a version tag; v2 reads as a branch (no semver/patch suffix). I could not verify whether v2 is a tag, branch, or annotated tag from this environment [UNKNOWN]. If it is a branch, any future re-resolve without the existing packages-lock.json (clean clone, manual Unity package cache clear, CI cache wipe) will pull whatever the branch tip happens to be at that moment. UPM does honor the lockfile day-to-day, so this is not an immediate runtime concern — it is a supply-chain hygiene concern.
  2. Impact surface is high. ClearScript ships the V8 engine that executes scene scripts (untrusted JS) inside the desktop/WebGL client. A bump from 7.5.1.a to a new major (v2) is not a small change in scope and is not reviewable from this PR alone.
  3. DCL-controlled fork. Provenance trust is reasonable (Decentraland org), so this isn't a third-party risk; the concern is reproducibility/immutability and the size of the upgrade.

Recommendation:

  • Pin to a tag or an immutable commit SHA, e.g. #ae0c51ae4a6f2325f9757e9a0253ddbef8713a9b (or #<tag>), and document what changed between 7.5.1.a and this commit. Fix this →
  • Confirm separately (review of decentraland/ClearScript commits between the two SHAs) what behavioral/security-relevant changes the V8 bump includes — particularly anything that touches host bindings, sandbox boundaries, or module resolution.

Other notes

  • com.atteneder.draco removal (+ scoped-registries entry removed): shrinking attack surface is fine. Verify WebGL/Standalone scenes that depend on Draco-compressed meshes still load — com.atteneder.gltfast (via the Decentraland fork at github.com/decentraland/unity-gltf.git) is now the sole path for .gltf/.glb. Note that com.atteneder.gltfast itself remains an unpinned git URL (no #ref/#commit) in manifest.json, but that's a pre-existing condition, not introduced by this PR.

  • Utility.asmdef now references JsWebSockets (Explorer/Assets/DCL/Infrastructure/Utility/Utility.asmdef:17). This means any code under Utility can now import the WebGL JS-side websocket binding (JsWebSocket.cs + JsWebSocket.jslib). No new capability is added to the build — same .jslib, same __Internal imports — only the C# call-site set widens. Low concern.

  • WebGLEditor.cs (Explorer/Assets/DCL/Infrastructure/WebGLEditor.cs:33):

    • Launches Chrome with --disable-web-security --user-data-dir=<temp> for developer testing. Guarded by #if UNITY_EDITOR, menu-driven, so it cannot ship to end users. Acceptable.
    • Caveats worth knowing: (a) the --user-data-dir paths (C:/tmp, /tmp/chrome-no-cors) are predictable and world-writable on some setups — another local user could pre-stage that dir; (b) PID files (chrome_pid.txt, server_pid.txt) are written to Unity's working directory and then Process.Kill'd without verifying the PID still belongs to chrome/python — a stale PID could be reused by an unrelated process. Both are minor developer hygiene issues, not shipped risk.
    • The reflection call to internal Unity API BuildPlayerWindow.GetBuildPlayerOptionsInternal (WebGLEditor.cs:107) is fragile across Unity upgrades, but again editor-only.
  • LocalStorageDCLPlayerPrefs.cs: declares [DllImport("__Internal")] DCLPrefs_Save/DCLPrefs_Load (LocalStorageDCLPlayerPrefs.cs:159-163). I did not find a .jslib that provides these symbols (grep -r "DCLPrefs_Save\|DCLPrefs_Load" --include="*.jslib" returns no matches). This is a functional concern — the WebGL build will fail to link these externs — not a security one, but worth flagging since it's directly on the WebGL path this PR enables. Fix this →

  • ProjectSettings.asset WebGL scripting-defines now equal Standalone, including ALTTESTER. AltTester (com.alttester.sdk 2.3.0) is an instrumentation SDK that exposes a control surface for UI automation. It already ships in Standalone builds, so this is parity — not a new exposure for the product as a whole, but it's worth confirming with the team whether shipping AltTester in production WebGL builds is intentional. If production WebGL builds should not embed AltTester, gate the define via a build-flag rather than putting it in WebGL: …. [Inference], not a finding against this PR specifically.

  • Workflows: Both modified workflows use only previously-used actions (actions/checkout@v4, actions/upload-artifact@v4, peter-evans/create-or-update-comment@v3) and existing secrets. No new secret references, no new third-party action publishers, no new pull_request_target triggers. Low concern.


Outcome

DEPENDENCY_REVIEW: NEEDS_ATTENTION

Main reason: the org.decentraland.clearscript git ref was changed from a tag-shaped string (7.5.1.a) to a branch-shaped string (v2) for the V8/JS engine the client embeds. Even though the lockfile pins the resolved commit, a branch-style ref undermines reproducibility on clean clones and is the kind of ref that gets quietly retargeted. Recommend pinning to an immutable ref before merging, and verifying the diff between 5f385df… and ae0c51ae… in decentraland/ClearScript separately.


  • Identify all dependency changes (manifest.json, packages-lock.json, plugins, binaries, .asmdef)
  • Inspect new/added files (WebGLEditor.cs, LocalStorageDCLPlayerPrefs.cs)
  • Review workflow changes in .github/workflows/
  • Look up registry metadata for any new packages — no external lookup possible from this environment; git ls-remote/gh api against decentraland/ClearScript were not permitted, so #v2 ref type is recorded as UNKNOWN. Please verify manually.
  • Assess risk classification
  • Post final review
    · branch feat/webgl-ci-cd

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 12, 2026

Claude encountered an error —— View job


Dependency / Supply-Chain Security Review — PR #8744 (re-review)

Reviewing latest commits on feat/webgl-ci-cd

  • Re-check current dependency changes (manifest.json, packages-lock.json)
  • Inspect new/changed plugin & binary surface (Plugins/, native libs, .asmdef)
  • Look up registry metadata for any newly added/changed packages
  • Review CI workflow changes
  • Re-assess risk for ClearScript #v2 ref + V8 bump
  • Re-assess LocalStorageDCLPlayerPrefs .jslib linkage
  • Publish final classified review
    · branch feat/webgl-ci-cd

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 12, 2026

Claude finished @NickKhalow's task in 3m 20s —— View job


Dependency / Supply-Chain Security Review — PR #8744 (re-review)

Scope: this re-review focuses on supply-chain and external-dependency surface introduced by feat/webgl-ci-cd against the current origin/dev (c8df85ffb). Behavioral / non-dependency changes are out of scope.

This review supersedes the earlier one — it corrects a finding about the ClearScript ref (it's a tag, not a branch) and adds context about a version-drift issue with origin/dev that the previous review missed.

Summary

Item Type Source / Version Scope Risk Confidence
com.atteneder.draco removed (was 4.0.2 via OpenUPM) Managed package OpenUPM runtime LOW HIGH
org.decentraland.clearscript ref changed vs. dev: dev #7.5.1.a (5f385df…) → PR #v2 (ae0c51ae…) Git package — DCL fork of MS ClearScript / V8 github.com/decentraland/ClearScript.git runtime — JS engine MEDIUM HIGH
Utility.asmdef adds ref to DCL.WebSockets.JS asmdef (GUID 7b6e9563…Assets/Plugins/WebGLSockets/) Assembly reference local plugin runtime (WebGL) LOW HIGH
SceneRuntime.asmdef sets allowUnsafeCode: true Build flag local runtime LOW (context-appropriate) HIGH
WebGLEditor.cs (new) — launches Chrome --disable-web-security + python3 -m http.server Editor C# script local editor-only (#if UNITY_EDITOR) LOW (caveats) HIGH
LocalStorageDCLPlayerPrefs.cs (new) — [DllImport("__Internal")] for DCLPrefs_Save/DCLPrefs_Load C# / WebGL native interop local runtime (WebGL) LOW (correctness issue, still no .jslib) HIGH
.github/workflows/build-unitycloud.yml, pr-comment-artifact-url.yml CI workflows local CI LOW HIGH
ProjectSettings.asset — WebGL scripting-defines now equal Standalone (incl. ALTTESTER) Build config local runtime (WebGL) LOW (parity, see note) HIGH

No new native binaries, .dll/.so/.dylib/.bundle, or precompiled assemblies were added in this PR. No .jslib files were added either.


MEDIUM-risk item: ClearScript #v2 lags behind origin/dev's #7.5.1.a

[PR] Current diff against origin/dev:

-    "org.decentraland.clearscript": "...ClearScript.git?path=/Unity/Package#7.5.1.a",  // dev: 5f385df93c2d1e4a9aed3dd0aad62c247c684a4b
+    "org.decentraland.clearscript": "...ClearScript.git?path=/Unity/Package#v2",       // PR:  ae0c51ae4a6f2325f9757e9a0253ddbef8713a9b

(origin/dev recently merged #8742 "Upgrade ClearScript to 7.5.1" — c8df85ffb.)

[Registry metadata]decentraland/ClearScript:

  • v2 exists as a tag (dated 2026-03-07), not a branch — correcting my previous review which guessed it was a branch.
  • 7.5.1.a is the newer tag, published 2026-05-12.
  • This is a DCL-owned fork of Microsoft ClearScript; modifications include: removed VBScript/JScript/COM/x86, custom Unity packaging, Linux native lib built with glibc 2.31, "additional unsafe APIs unfit for upstream."

Concern: this PR ships a JS engine version that is older than what dev already has. Combined with the new merge into dev (commit 8adb7cbf9 Merge remote-tracking branch 'origin/dev' into feat/webgl-ci-cd), it looks like the PR branch was created against the old ref (v2) and a later merge did not bring manifest.json forward. [Inference] — likely accidental stale ref rather than an intentional downgrade.

The supply-chain footprint of this change touches the host of all scene JS execution (V8 + ClearScript hosting), so divergence between branches matters for review-ability.

Recommendation:

  • Pull manifest.json + packages-lock.json from dev to align on #7.5.1.a (5f385df…) — unless the WebGL build genuinely requires v2, in which case document why and pin to the immutable commit SHA #ae0c51ae4a6f2325f9757e9a0253ddbef8713a9b rather than the tag. Fix this →

Other notes

  • com.atteneder.draco removal (+ scoped-registries entry removed) — fine, reduces attack surface. Verify scenes that depend on Draco-compressed meshes still load via com.atteneder.gltfast (DCL fork github.com/decentraland/unity-gltf.git, which remains git-URL-pinned without a #ref — pre-existing).

  • Utility.asmdef adds ref to DCL.WebSockets.JS (Utility.asmdef:17 → GUID 7b6e9563… maps to Explorer/Assets/Plugins/WebGLSockets/DCL.WebSockets.JS.asmdef). No new native code — JsWebSocket.jslib was already in the tree. Only the C# call-site set widens. Note: Utility is a broad foundational assembly; granting it visibility into WebSocket bindings means a lot of internal call sites can now reach raw WebSocket interop. Suggest exposing via a narrower wrapper rather than expanding Utility's ref list if possible, though risk is low.

  • SceneRuntime.asmdef sets allowUnsafeCode: true — needed by the new V8ArrayBufferAdapter / V8TypedArrayAdapter for ArrayBuffer/typed-array pointer access. Scoped to a single asmdef. Acceptable.

  • WebGLEditor.cs (Explorer/Assets/DCL/Infrastructure/WebGLEditor.cs:33):

    • --disable-web-security --user-data-dir=<temp> Chrome flag is dev-only behavior, guarded by #if UNITY_EDITOR. Acceptable for editor use.
    • Hygiene caveats unchanged from previous review:
      • --user-data-dir paths (C:/tmp, /tmp/chrome-no-cors) are predictable and world-writable on shared dev hosts. Another local user could pre-stage the directory.
      • PID files (chrome_pid.txt, server_pid.txt) live in Unity's working dir and Process.Kill is called by PID without verifying the process name. A stale PID reused by an unrelated process would be killed silently.
      • Reflection into internal BuildPlayerWindow.GetBuildPlayerOptionsInternal (WebGLEditor.cs:107) is fragile across Unity upgrades, but editor-only.
    • These are minor developer hygiene issues, not shipped risk.
  • LocalStorageDCLPlayerPrefs.cs (Explorer/Assets/DCL/Prefs/LocalStorageDCLPlayerPrefs.cs:159-163): declares [DllImport("__Internal")] DCLPrefs_Save/DCLPrefs_Load. Still no .jslib provides these symbols — confirmed via grep -rn "DCLPrefs_Save\|DCLPrefs_Load" --include="*.jslib" against the current tree. This is a functional concern (the WebGL Emscripten link step will fail or the runtime will throw Unable to resolve external function) — not a security one, but blocks the WebGL build this PR is meant to enable. The previous review flagged this and it has not been addressed in subsequent commits.

    Fix this →

  • ProjectSettings.asset — WebGL scripting-defines now equal Standalone, including ALTTESTER. AltTester (com.alttester.sdk 2.3.0) is a UI-automation instrumentation SDK that already ships in Standalone builds, so this is parity. [Inference] — worth confirming with the team that shipping AltTester to production WebGL is intentional. If WebGL production builds should not embed AltTester, gate the define via a build flag instead of putting it directly in WebGL: …. Not a finding against this PR specifically.

  • Workflows: .github/workflows/build-unitycloud.yml adds a web target to the existing build matrix, and pr-comment-artifact-url.yml adds a Web artifact link in the PR comment. Actions used (actions/checkout@v4, actions/upload-artifact@v4, peter-evans/create-or-update-comment@v3) and secrets (EXPLORER_TEAM_AWS_*, EXPLORER_TEAM_S3_BUCKET, SENTRY_CLI_AUTH_TOKEN) are all previously used. No new third-party action publishers, no new pull_request_target triggers, no new secret references. Low concern.

    • Minor: the new shell block uses set -euo pipefail, which is good. The if [ "${GITHUB_REF_NAME}" == "dev" ] gate around webgl-latest upload is safe because it requires the build matrix to run on dev itself, not on a PR head.

Outcome

DEPENDENCY_REVIEW: NEEDS_ATTENTION

Two unresolved items, neither of them strictly security blockers, but both worth addressing before merge:

  1. ClearScript version drift vs. dev. dev is on #7.5.1.a (newer), the PR is on #v2 (older tag). This appears to be stale-branch state, not an intentional downgrade. Bring the manifest forward with dev, or — if v2 is genuinely required — pin to the immutable commit SHA and document the reason. Tags can be force-moved, so the tag string alone is not sufficient if reproducibility matters.
  2. Missing DCLPrefs.jslib. WebGL build will fail to link without it. Flagged in the previous review and not yet addressed.

The AltTester-on-WebGL parity question is a separate product/security decision worth raising with the team but is not a blocker on this PR.


  • Re-check current dependency changes (manifest.json, packages-lock.json)
  • Inspect new/changed plugin & binary surface (Plugins/, native libs, .asmdef)
  • Look up registry metadata for changed packages (ClearScript v2 confirmed as tag, not branch)
  • Review CI workflow changes
  • Re-assess risk for ClearScript #v2 ref + V8 bump (downgraded to MEDIUM with new version-drift context)
  • Re-assess LocalStorageDCLPlayerPrefs .jslib linkage (still missing)
  • Publish final classified review
    · branch feat/webgl-ci-cd ·

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 12, 2026

Claude finished @NickKhalow's task in 2m 31s —— View job


Dependency / Supply-Chain Security Review — PR #8744 (re-review)

Scope: supply-chain and external-dependency surface introduced by feat/webgl-ci-cd against origin/dev (c8df85ffb). Behavioral / non-dependency changes are out of scope. This re-review covers the 4 new commits since my last pass (V8SceneRuntimeImpl, asmdef unsafe allow, webgl impl for poolable array, guard LivekitPlayer on WebGL).

TL;DR — findings are unchanged from the previous re-review. No new dependencies, no new binaries, and no new .jslib were added in the latest commits. The two outstanding items (ClearScript ref drift vs dev, and the missing DCLPrefs.jslib) still need to be addressed before merge.

Summary

Item Type Source / Version Scope Risk Confidence
com.atteneder.draco removed (was 4.0.2 via OpenUPM) Managed package OpenUPM runtime LOW HIGH
org.decentraland.clearscript ref changed vs. dev: dev #7.5.1.a (5f385df…) → PR #v2 (ae0c51ae…) Git package — DCL fork of MS ClearScript / V8 github.com/decentraland/ClearScript.git runtime — JS engine MEDIUM HIGH
Utility.asmdef adds ref to DCL.WebSockets.JS asmdef (GUID 7b6e9563…Explorer/Assets/Plugins/WebGLSockets/) Assembly reference local plugin runtime (WebGL) LOW HIGH
SceneRuntime.asmdef sets allowUnsafeCode: true Build flag local runtime LOW (context-appropriate) HIGH
WebGLEditor.cs (new) — launches Chrome --disable-web-security + python3 -m http.server Editor C# script local editor-only (#if UNITY_EDITOR) LOW (caveats) HIGH
LocalStorageDCLPlayerPrefs.cs (new) — [DllImport("__Internal")] for DCLPrefs_Save/DCLPrefs_Load C# / WebGL native interop local runtime (WebGL) LOW (correctness issue, still no .jslib) HIGH
.github/workflows/build-unitycloud.yml, pr-comment-artifact-url.yml CI workflows local CI LOW HIGH
ProjectSettings.asset — WebGL scripting-defines now equal Standalone (incl. ALTTESTER) Build config local runtime (WebGL) LOW (parity, see note) HIGH

com.renderheads.avpro.video-ultra appears in the manifest diff but is a pure reorder — same URL and same lockfile hash (f79125ef…) as dev. Not a real change.

No new native binaries (.dll/.so/.dylib/.bundle/.a/.lib/.exe) or .jslib files were added by this PR — verified with git diff origin/dev HEAD --stat -- '**/*.dll' '**/*.so' '**/*.dylib' '**/*.bundle' '**/*.a' '**/*.lib' '**/*.exe' '**/*.jslib'.


MEDIUM-risk: ClearScript #v2 lags behind origin/dev's #7.5.1.a

[PR] Current diff vs. origin/dev:

-    "org.decentraland.clearscript": "...ClearScript.git?path=/Unity/Package#7.5.1.a",  // dev: 5f385df93c2d1e4a9aed3dd0aad62c247c684a4b
+    "org.decentraland.clearscript": "...ClearScript.git?path=/Unity/Package#v2",       // PR:  ae0c51ae4a6f2325f9757e9a0253ddbef8713a9b

(origin/dev recently merged #8742 "Upgrade ClearScript to 7.5.1" — c8df85ffb.)

[Registry metadata]decentraland/ClearScript:

  • v2 is a tag (dated 2026-03-07), not a branch.
  • 7.5.1.a is the newer tag, published 2026-05-12.
  • DCL-owned fork of Microsoft ClearScript; modifications include: removed VBScript/JScript/COM/x86, custom Unity packaging, Linux native lib built with glibc 2.31, "additional unsafe APIs unfit for upstream."

Concern: this PR ships a JS engine version that is older than what dev already has. Combined with the merge commit 8adb7cbf9 "Merge origin/dev into feat/webgl-ci-cd", it looks like the PR branch was created against the old ref (v2) and a later merge did not carry manifest.json forward. [Inference] — likely accidental stale ref, not an intentional downgrade. The host of all scene JS execution (V8 + ClearScript hosting) is too high-impact a surface to leave with branch drift.

Recommendation: pull manifest.json + packages-lock.json from dev to align on #7.5.1.a (5f385df…) — unless the WebGL build genuinely requires v2, in which case document why and pin to the immutable commit SHA #ae0c51ae4a6f2325f9757e9a0253ddbef8713a9b rather than the tag (tags can be force-moved). Fix this →


Other notes

  • com.atteneder.draco removal (+ scoped-registries entry removed) — fine, reduces attack surface. Verify scenes that depend on Draco-compressed meshes still load via com.atteneder.gltfast (DCL fork github.com/decentraland/unity-gltf.git, which remains unpinned by git ref — pre-existing).

  • Utility.asmdef adds ref to DCL.WebSockets.JS (Utility.asmdef:17 → GUID 7b6e9563… maps to Explorer/Assets/Plugins/WebGLSockets/DCL.WebSockets.JS.asmdef). No new native code — JsWebSocket.jslib was already in the tree. Only the C# call-site set widens. Note: Utility is a broad foundational assembly; granting it visibility into WebSocket bindings means a lot of internal call sites can now reach raw WebSocket interop. Consider exposing via a narrower wrapper rather than expanding Utility's ref list. Risk is low.

  • SceneRuntime.asmdef sets allowUnsafeCode: true — needed by the new V8ArrayBufferAdapter / V8TypedArrayAdapter for ArrayBuffer/typed-array pointer access. Scoped to a single asmdef. Acceptable.

  • WebGLEditor.cs (Explorer/Assets/DCL/Infrastructure/WebGLEditor.cs):

    • --disable-web-security --user-data-dir=<temp> Chrome flag is dev-only behavior, guarded by #if UNITY_EDITOR. Acceptable for editor use.
    • Hygiene caveats (unchanged from previous review):
      • --user-data-dir paths (C:/tmp, /tmp/chrome-no-cors) are predictable and world-writable on shared dev hosts. Another local user could pre-stage the directory.
      • PID files (chrome_pid.txt, server_pid.txt) live in Unity's working dir and Process.Kill is called by PID without verifying the process name. A stale PID reused by an unrelated process would be killed silently.
      • Reflection into internal BuildPlayerWindow.GetBuildPlayerOptionsInternal (WebGLEditor.cs:107) is fragile across Unity upgrades, but editor-only.
    • Minor developer hygiene issues, not shipped risk.
  • LocalStorageDCLPlayerPrefs.cs (Explorer/Assets/DCL/Prefs/LocalStorageDCLPlayerPrefs.cs:159-163): declares [DllImport("__Internal")] DCLPrefs_Save/DCLPrefs_Load. Still no .jslib provides these symbols — confirmed against the current tree:

    $ find Explorer -name "*.jslib"
    Explorer/Assets/Plugins/WebGLSockets/JsWebSocket.jslib
    Explorer/Assets/Plugins/Nethereum/net472UnityCommonAOT/NethereumMetamask.jslib
    $ grep -rn "DCLPrefs_Save\|DCLPrefs_Load" --include="*.jslib" Explorer/
    (no output)
    

    This is a functional concern (the WebGL Emscripten link step will fail or the runtime will throw Unable to resolve external function) — not a security one, but blocks the WebGL build this PR is meant to enable. Flagged twice in prior reviews; not yet addressed.

    Fix this →

  • ProjectSettings.asset — WebGL scripting-defines now equal Standalone, including ALTTESTER. AltTester (com.alttester.sdk 2.3.0) is a UI-automation instrumentation SDK that already ships in Standalone builds, so this is parity. [Inference] — worth confirming with the team that shipping AltTester to production WebGL is intentional. If WebGL production builds should not embed AltTester, gate the define via a build flag instead of putting it directly in WebGL: …. Not a finding against this PR specifically.

  • Workflows: .github/workflows/build-unitycloud.yml adds a web target to the existing build matrix, and pr-comment-artifact-url.yml adds a Web artifact link in the PR comment. Actions used (actions/checkout@v4, actions/upload-artifact@v4, peter-evans/create-or-update-comment@v3) and secrets (EXPLORER_TEAM_AWS_*, EXPLORER_TEAM_S3_BUCKET, SENTRY_CLI_AUTH_TOKEN) are all previously used. No new third-party action publishers, no new pull_request_target triggers, no new secret references. The new shell block uses set -euo pipefail (good). The if [ "${GITHUB_REF_NAME}" == "dev" ] gate around webgl-latest upload is safe because it requires the build matrix to run on dev itself, not on a PR head. Low concern.


Outcome

DEPENDENCY_REVIEW: NEEDS_ATTENTION

Two unresolved items, neither a strict security blocker, but both worth addressing before merge:

  1. ClearScript version drift vs. dev. dev is on #7.5.1.a (newer), the PR is on #v2 (older tag). Looks like stale-branch state, not an intentional downgrade. Bring manifest.json + packages-lock.json forward with dev, or — if v2 is genuinely required for WebGL — pin to the immutable commit SHA and document why. Tags can be force-moved, so the tag string alone is not sufficient if reproducibility matters.
  2. Missing DCLPrefs.jslib. WebGL build will fail to link without it. Flagged in two prior reviews and not yet addressed.

The AltTester-on-WebGL parity question is a separate product/security decision worth raising with the team but is not a blocker on this PR.


  • Re-check current dependency changes (manifest.json, packages-lock.json)
  • Inspect new/changed plugin & binary surface (Plugins/, native libs, .asmdef)
  • Look up registry metadata for changed packages (ClearScript v2 confirmed as tag)
  • Review CI workflow changes
  • Re-assess risk for ClearScript ref + V8 bump (still MEDIUM, version-drift)
  • Re-assess LocalStorageDCLPlayerPrefs .jslib linkage (still missing)
  • Publish final classified review

· branch feat/webgl-ci-cd ·

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 12, 2026

Claude finished @NickKhalow's task in 2m 20s —— View job


Dependency / Supply-Chain Security Review — PR #8744 (re-review)

Scope: supply-chain and external-dependency surface introduced by feat/webgl-ci-cd against origin/dev (c8df85ffb). Behavioral / non-dependency changes are out of scope. This pass covers the latest tip (5736eae20 handle LivekitAddress on WebGL).

TL;DR — findings are unchanged from the previous re-review. No new external dependencies, no new native binaries, and no new .jslib files were added since the last pass. The two outstanding items (ClearScript ref drift vs dev, and the missing DCLPrefs.jslib) remain.

Summary

Item Type Source / Version Scope Risk Confidence
com.atteneder.draco removed (was 4.0.2 via OpenUPM) Managed package OpenUPM runtime LOW HIGH
org.decentraland.clearscript ref changed vs. dev: dev #7.5.1.a (5f385df…) → PR #v2 (ae0c51ae…) Git package — DCL fork of MS ClearScript / V8 github.com/decentraland/ClearScript.git runtime — JS engine MEDIUM HIGH
Utility.asmdef adds ref to DCL.WebSockets.JS asmdef (GUID 7b6e9563…Explorer/Assets/Plugins/WebGLSockets/) Assembly reference local plugin runtime (WebGL) LOW HIGH
SceneRuntime.asmdef sets allowUnsafeCode: true Build flag local runtime LOW (context-appropriate) HIGH
WebGLEditor.cs (new) — launches Chrome --disable-web-security + python3 -m http.server Editor C# script local editor-only (#if UNITY_EDITOR) LOW (caveats) HIGH
LocalStorageDCLPlayerPrefs.cs (new) — [DllImport("__Internal")] for DCLPrefs_Save/DCLPrefs_Load C# / WebGL native interop local runtime (WebGL) LOW (correctness issue, still no .jslib) HIGH
.github/workflows/build-unitycloud.yml, pr-comment-artifact-url.yml CI workflows local CI LOW HIGH
ProjectSettings.asset — WebGL scripting-defines now equal Standalone (incl. ALTTESTER) Build config local runtime (WebGL) LOW (parity, see note) HIGH

com.renderheads.avpro.video-ultra appears in the manifest diff but is a pure reorder — same URL and same lockfile hash as dev. Not a real change.

No new native binaries (.dll/.so/.dylib/.bundle/.a/.lib/.exe) or .jslib files were added by this PR — verified with git diff origin/dev HEAD --stat -- '**/*.dll' '**/*.so' '**/*.dylib' '**/*.bundle' '**/*.a' '**/*.lib' '**/*.exe' '**/*.jslib' (no output).


MEDIUM-risk: ClearScript #v2 lags behind origin/dev's #7.5.1.a

[PR] Current diff vs. origin/dev:

-    "org.decentraland.clearscript": "...ClearScript.git?path=/Unity/Package#7.5.1.a",  // dev: 5f385df93c2d1e4a9aed3dd0aad62c247c684a4b
+    "org.decentraland.clearscript": "...ClearScript.git?path=/Unity/Package#v2",       // PR:  ae0c51ae4a6f2325f9757e9a0253ddbef8713a9b

(origin/dev recently merged #8742 "Upgrade ClearScript to 7.5.1" — c8df85ffb.)

[Registry metadata]decentraland/ClearScript:

  • v2 is a tag (dated 2026-03-07), not a branch.
  • 7.5.1.a is the newer tag, published 2026-05-12.
  • DCL-owned fork of Microsoft ClearScript; modifications include: removed VBScript/JScript/COM/x86, custom Unity packaging, Linux native lib built with glibc 2.31, "additional unsafe APIs unfit for upstream."

Concern: this PR ships a JS engine version that is older than what dev already has. Combined with the merge commit 8adb7cbf9 "Merge origin/dev into feat/webgl-ci-cd", it looks like the PR branch was created against the old ref (v2) and a later merge did not carry manifest.json forward. [Inference] — likely accidental stale ref, not an intentional downgrade. The host of all scene JS execution (V8 + ClearScript hosting) is too high-impact a surface to leave with branch drift.

Recommendation: pull manifest.json + packages-lock.json from dev to align on #7.5.1.a (5f385df…) — unless the WebGL build genuinely requires v2, in which case document why and pin to the immutable commit SHA #ae0c51ae4a6f2325f9757e9a0253ddbef8713a9b rather than the tag (tags can be force-moved). Fix this →


Other notes

  • com.atteneder.draco removal (+ scoped-registries entry removed) — fine, reduces attack surface. Verify scenes that depend on Draco-compressed meshes still load via com.atteneder.gltfast (DCL fork github.com/decentraland/unity-gltf.git, which remains unpinned by git ref — pre-existing).

  • Utility.asmdef adds ref to DCL.WebSockets.JS (Explorer/Assets/DCL/Infrastructure/Utility/Utility.asmdef:17 → GUID 7b6e9563… maps to Explorer/Assets/Plugins/WebGLSockets/DCL.WebSockets.JS.asmdef). No new native code — JsWebSocket.jslib was already in the tree. Only the C# call-site set widens. Note: Utility is a broad foundational assembly; granting it visibility into WebSocket bindings means a lot of internal call sites can now reach raw WebSocket interop. Consider exposing via a narrower wrapper rather than expanding Utility's ref list. Risk is low.

  • SceneRuntime.asmdef sets allowUnsafeCode: true — needed by the new V8ArrayBufferAdapter / V8TypedArrayAdapter for ArrayBuffer/typed-array pointer access. Scoped to a single asmdef. Acceptable.

  • WebGLEditor.cs (Explorer/Assets/DCL/Infrastructure/WebGLEditor.cs):

    • --disable-web-security --user-data-dir=<temp> Chrome flag is dev-only behavior, guarded by #if UNITY_EDITOR. Acceptable for editor use.
    • Hygiene caveats (unchanged from previous review):
      • --user-data-dir paths (C:/tmp, /tmp/chrome-no-cors) are predictable and world-writable on shared dev hosts. Another local user could pre-stage the directory.
      • PID files (chrome_pid.txt, server_pid.txt) live in Unity's working dir and Process.Kill is called by PID without verifying the process name. A stale PID reused by an unrelated process would be killed silently.
      • Reflection into internal BuildPlayerWindow.GetBuildPlayerOptionsInternal (WebGLEditor.cs:107) is fragile across Unity upgrades, but editor-only.
    • Minor developer hygiene issues, not shipped risk.
  • LocalStorageDCLPlayerPrefs.cs (Explorer/Assets/DCL/Prefs/LocalStorageDCLPlayerPrefs.cs:159-163): declares [DllImport("__Internal")] DCLPrefs_Save/DCLPrefs_Load. Still no .jslib provides these symbols — confirmed against the current tree:

    $ find Explorer -name "*.jslib"
    Explorer/Assets/Plugins/WebGLSockets/JsWebSocket.jslib
    Explorer/Assets/Plugins/Nethereum/net472UnityCommonAOT/NethereumMetamask.jslib
    $ grep -rn "DCLPrefs_Save\|DCLPrefs_Load" --include="*.jslib" Explorer/
    (no output)
    

    This is a functional concern (the WebGL Emscripten link step will fail or the runtime will throw Unable to resolve external function) — not a security one, but blocks the WebGL build this PR is meant to enable. Flagged in three prior reviews; not yet addressed.

    Fix this →

  • ProjectSettings.asset — WebGL scripting-defines now equal Standalone, including ALTTESTER. AltTester (com.alttester.sdk 2.3.0) is a UI-automation instrumentation SDK that already ships in Standalone builds, so this is parity. [Inference] — worth confirming with the team that shipping AltTester to production WebGL is intentional. If WebGL production builds should not embed AltTester, gate the define via a build flag instead of putting it directly in WebGL: …. Not a finding against this PR specifically.

  • Workflows: .github/workflows/build-unitycloud.yml adds a web target to the existing build matrix, and pr-comment-artifact-url.yml adds a Web artifact link in the PR comment. Actions used (actions/checkout@v4, actions/upload-artifact@v4, peter-evans/create-or-update-comment@v3) and secrets (EXPLORER_TEAM_AWS_*, EXPLORER_TEAM_S3_BUCKET, SENTRY_CLI_AUTH_TOKEN) are all previously used. No new third-party action publishers, no new pull_request_target triggers, no new secret references. New shell block uses set -euo pipefail (good). The if [ "${GITHUB_REF_NAME}" == "dev" ] gate around webgl-latest upload is safe — it requires the build matrix to run on dev itself, not a PR head. Low concern.


Outcome

DEPENDENCY_REVIEW: NEEDS_ATTENTION

Two unresolved items, neither a strict security blocker, but both worth addressing before merge:

  1. ClearScript version drift vs. dev. dev is on #7.5.1.a (newer), the PR is on #v2 (older tag). Looks like stale-branch state, not an intentional downgrade. Bring manifest.json + packages-lock.json forward with dev, or — if v2 is genuinely required for WebGL — pin to the immutable commit SHA and document why. Tags can be force-moved, so the tag string alone is not sufficient if reproducibility matters.
  2. Missing DCLPrefs.jslib. WebGL build will fail to link without it. Flagged in three prior reviews and not yet addressed.

The AltTester-on-WebGL parity question is a separate product/security decision worth raising with the team but is not a blocker on this PR.


  • Re-check current dependency changes (manifest.json, packages-lock.json)
  • Inspect new/changed plugin & binary surface (Plugins/, native libs, .asmdef)
  • Review CI workflow changes
  • Re-assess risk for ClearScript ref + V8 bump (still MEDIUM, version-drift)
  • Re-assess LocalStorageDCLPlayerPrefs .jslib linkage (still missing)
  • Publish final classified review

· branch feat/webgl-ci-cd ·

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 12, 2026

Claude finished @NickKhalow's task in 2m 39s —— View job


Dependency / Supply-Chain Security Review — PR #8744 (re-review)

Scope: supply-chain and external-dependency surface introduced by feat/webgl-ci-cd against origin/dev (c8df85ffb). Behavioral / non-dependency changes are out of scope. This pass covers the two new commits since the last review (5736eae20 handle LivekitAddress on WebGL, c1a1cab0e apply audio and video stream guards).

TL;DR — findings are unchanged from the previous re-review. The two latest commits only add #if !UNITY_WEBGL guards around media/livekit code paths. No new external dependencies, no new native binaries, and no new .jslib files. The two outstanding items (ClearScript ref drift vs dev, and the missing DCLPrefs.jslib) still need to be addressed before merge.

Summary

Item Type Source / Version Scope Risk Confidence
com.atteneder.draco removed (was 4.0.2 via OpenUPM) Managed package OpenUPM runtime LOW HIGH
org.decentraland.clearscript ref changed vs. dev: dev #7.5.1.a (5f385df…) → PR #v2 (ae0c51ae…) Git package — DCL fork of MS ClearScript / V8 github.com/decentraland/ClearScript.git runtime — JS engine MEDIUM HIGH
Utility.asmdef adds ref to DCL.WebSockets.JS asmdef (GUID 7b6e9563…Explorer/Assets/Plugins/WebGLSockets/) Assembly reference local plugin runtime (WebGL) LOW HIGH
SceneRuntime.asmdef sets allowUnsafeCode: true Build flag local runtime LOW (context-appropriate) HIGH
WebGLEditor.cs (new) — launches Chrome --disable-web-security + python3 -m http.server Editor C# script local editor-only (#if UNITY_EDITOR) LOW (caveats) HIGH
LocalStorageDCLPlayerPrefs.cs (new) — [DllImport("__Internal")] for DCLPrefs_Save/DCLPrefs_Load C# / WebGL native interop local runtime (WebGL) LOW (correctness issue, still no .jslib) HIGH
.github/workflows/build-unitycloud.yml, pr-comment-artifact-url.yml CI workflows local CI LOW HIGH
ProjectSettings.asset — WebGL scripting-defines now equal Standalone (incl. ALTTESTER) Build config local runtime (WebGL) LOW (parity, see note) HIGH

com.renderheads.avpro.video-ultra appears in the manifest diff but is a pure reorder — same URL and same lockfile hash as dev. Not a real change.

No new native binaries (.dll/.so/.dylib/.bundle/.a/.lib/.exe) or .jslib files were added by this PR.


MEDIUM-risk: ClearScript #v2 lags behind origin/dev's #7.5.1.a

[PR] Confirmed against current origin/dev (c8df85ffb):

-    "org.decentraland.clearscript": "...ClearScript.git?path=/Unity/Package#7.5.1.a",  // dev: 5f385df93c2d1e4a9aed3dd0aad62c247c684a4b
+    "org.decentraland.clearscript": "...ClearScript.git?path=/Unity/Package#v2",       // PR:  ae0c51ae4a6f2325f9757e9a0253ddbef8713a9b

(origin/dev recently merged #8742 "Upgrade ClearScript to 7.5.1" — c8df85ffb.)

[Registry metadata]decentraland/ClearScript:

  • v2 is a tag (dated 2026-03-07).
  • 7.5.1.a is the newer tag, published 2026-05-12.
  • DCL-owned fork of Microsoft ClearScript; modifications include: removed VBScript/JScript/COM/x86, custom Unity packaging, Linux native lib built with glibc 2.31, "additional unsafe APIs unfit for upstream."

Concern: this PR ships a JS engine version that is older than what dev already has. Combined with the merge commit 8adb7cbf9 "Merge origin/dev into feat/webgl-ci-cd", this looks like the PR branch was created against the old ref (v2) and a later merge from dev did not carry manifest.json forward. [Inference] — likely accidental stale ref, not an intentional downgrade. The host of all scene JS execution (V8 + ClearScript hosting) is too high-impact a surface to leave with branch drift.

Recommendation: pull manifest.json + packages-lock.json from dev to align on #7.5.1.a (5f385df…) — unless the WebGL build genuinely requires v2, in which case pin to the immutable commit SHA #ae0c51ae4a6f2325f9757e9a0253ddbef8713a9b rather than the tag (tags can be force-moved) and document why. Fix this →


Other notes

  • com.atteneder.draco removal (+ scoped-registries entry removed) — fine, reduces attack surface. Verify scenes that depend on Draco-compressed meshes still load via com.atteneder.gltfast (DCL fork github.com/decentraland/unity-gltf.git, which remains unpinned by git ref — pre-existing).

  • Utility.asmdef adds ref to DCL.WebSockets.JS (Utility.asmdef:17 → GUID 7b6e9563… maps to Explorer/Assets/Plugins/WebGLSockets/DCL.WebSockets.JS.asmdef). No new native code — JsWebSocket.jslib was already in the tree. Only the C# call-site set widens. Note: Utility is a broad foundational assembly; granting it visibility into WebSocket bindings means a lot of internal call sites can now reach raw WebSocket interop. Consider exposing via a narrower wrapper rather than expanding Utility's ref list. Risk is low.

  • SceneRuntime.asmdef sets allowUnsafeCode: true — needed by the new V8ArrayBufferAdapter / V8TypedArrayAdapter for ArrayBuffer/typed-array pointer access. Scoped to a single asmdef. Acceptable.

  • WebGLEditor.cs (Explorer/Assets/DCL/Infrastructure/WebGLEditor.cs):

    • --disable-web-security --user-data-dir=<temp> Chrome flag is dev-only behavior, guarded by #if UNITY_EDITOR. Acceptable for editor use.
    • Hygiene caveats (unchanged from previous reviews):
      • --user-data-dir paths (C:/tmp, /tmp/chrome-no-cors) are predictable and world-writable on shared dev hosts. Another local user could pre-stage the directory.
      • PID files (chrome_pid.txt, server_pid.txt) live in Unity's working dir and Process.Kill is called by PID without verifying the process name. A stale PID reused by an unrelated process would be killed silently.
      • Reflection into internal BuildPlayerWindow.GetBuildPlayerOptionsInternal (WebGLEditor.cs:107) is fragile across Unity upgrades, but editor-only.
    • Minor developer hygiene issues, not shipped risk.
  • LocalStorageDCLPlayerPrefs.cs (Explorer/Assets/DCL/Prefs/LocalStorageDCLPlayerPrefs.cs:159-163): declares [DllImport("__Internal")] DCLPrefs_Save/DCLPrefs_Load. Still no .jslib provides these symbols — confirmed against the current tree:

    $ find Explorer -name "*.jslib"
    Explorer/Assets/Plugins/WebGLSockets/JsWebSocket.jslib
    Explorer/Assets/Plugins/Nethereum/net472UnityCommonAOT/NethereumMetamask.jslib
    $ grep -rn "DCLPrefs_Save\|DCLPrefs_Load" --include="*.jslib" Explorer/
    (no output)
    

    This is a functional concern (the WebGL Emscripten link step will fail or the runtime will throw Unable to resolve external function) — not a security one, but blocks the WebGL build this PR is meant to enable. Flagged in four prior reviews; not yet addressed.

    Fix this →

  • ProjectSettings.asset — WebGL scripting-defines now equal Standalone, including ALTTESTER. AltTester (com.alttester.sdk 2.3.0) is a UI-automation instrumentation SDK that already ships in Standalone builds, so this is parity. [Inference] — worth confirming with the team that shipping AltTester to production WebGL is intentional. If WebGL production builds should not embed AltTester, gate the define via a build flag instead of putting it directly in WebGL: …. Not a finding against this PR specifically.

  • Workflows: .github/workflows/build-unitycloud.yml adds a web target to the existing build matrix, and pr-comment-artifact-url.yml adds a Web artifact link in the PR comment. Actions used (actions/checkout@v4, actions/upload-artifact@v4, peter-evans/create-or-update-comment@v3) and secrets (EXPLORER_TEAM_AWS_*, EXPLORER_TEAM_S3_BUCKET, SENTRY_CLI_AUTH_TOKEN) are all previously used. No new third-party action publishers, no new pull_request_target triggers, no new secret references. New shell block uses set -euo pipefail (good). The if [ "${GITHUB_REF_NAME}" == "dev" ] gate around webgl-latest upload is safe — it requires the build matrix to run on dev itself, not a PR head. Low concern.

  • Latest commits (5736eae20, c1a1cab0e) — add #if !UNITY_WEBGL guards around media/livekit/audio code paths (LivekitAddress, MediaAddress, MultiMediaPlayer, ConnectiveRoom, InteriorLocalTracks, LogVideoStreams, etc.). No dependency, plugin, binary, or .jslib changes. No new supply-chain surface introduced.


Outcome

DEPENDENCY_REVIEW: NEEDS_ATTENTION

Two unresolved items, neither a strict security blocker, but both worth addressing before merge:

  1. ClearScript version drift vs. dev. dev is on #7.5.1.a (newer), the PR is on #v2 (older tag). Looks like stale-branch state, not an intentional downgrade. Bring manifest.json + packages-lock.json forward with dev, or — if v2 is genuinely required for WebGL — pin to the immutable commit SHA and document why. Tags can be force-moved, so the tag string alone is not sufficient if reproducibility matters.
  2. Missing DCLPrefs.jslib. WebGL build will fail to link without it. Flagged in four prior reviews and not yet addressed.

The AltTester-on-WebGL parity question is a separate product/security decision worth raising with the team but is not a blocker on this PR.


  • Re-check current dependency changes (manifest.json, packages-lock.json)
  • Inspect new/changed plugin & binary surface
  • Re-verify status of previously-flagged items (ClearScript ref, DCLPrefs.jslib)
  • Check CI workflow changes
  • Publish final classified review

· branch feat/webgl-ci-cd ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant