fix: allow local-ab deep-link param for loopback realms, deriving the assets base from the realm - #9519
Conversation
…r loopback realms Adds local-ab and a new local-ab-port app arg to the loopback-realm deep-link allowlist so Creator Hub / sdk-commands preview links can enable local asset bundles. The port variant keeps the host pinned to 127.0.0.1 (int-parsed, 1024-65535, falling back to abgen's default 5147), so unlike optimized-assets-url — which stays never-permitted — a crafted link cannot point the AB/LOD/registry endpoints at remote infrastructure. local-ab-port implies local-ab, mirroring mcp-port. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review: fix: allow local-ab and host-pinned local-ab-port deep-link params for loopback realms
STEP 2 — Root-cause check ✅
Creator Hub / sdk-commands preview deep links need to enable local asset bundles, but local-ab was not allowlisted and the full-URL override (optimized-assets-url) is deliberately never-permitted — it rewrites AssetBundlesCDN, LodGeneratorCDN, and AssetBundleRegistry endpoints globally for the whole session, letting a crafted link point asset loading at arbitrary remote infrastructure.
The PR takes the correct safe variant instead: a host-pinned port param (local-ab-port) whose URL construction hardcodes 127.0.0.1 and only allows a validated integer port. This addresses the actual need (local-dev AB loading) without introducing the attack surface of optimized-assets-url.
Verdict: PASS — the fix addresses the root cause, not a symptom.
STEP 3 — Design & integration ✅
No new long-lived units introduced. This PR adds:
- A
const stringinAppArgsFlags(leaf declaration) - Two entries in the
DeepLinkAllowlistset (configuration) - A static helper
ResolveLocalAssetBundlesUrlonRealmLaunchSettings - Implication logic in
ParseRealmAppParameter
Owner search:
- Local AB configuration is owned by
RealmLaunchSettings— it holdsuseLocalAssetBundles,DEFAULT_LOCAL_ASSET_BUNDLES_URL, and theParseRealmAppParameterlogic that interpretsLOCAL_AB. The newResolveLocalAssetBundlesUrlstatic method and theLOCAL_AB_PORTimplication are both placed in this class. ✅ - The deep-link allowlist is owned by
DeepLinkAllowlist— the two new entries follow the existing tier-2 pattern (per-key security rationale comment). ✅ - The consumer (
MainSceneLoader.InitializeFlowAsync) already calledRealmLaunchSettings.DEFAULT_LOCAL_ASSET_BUNDLES_URLat this point — the change toResolveLocalAssetBundlesUrlis a drop-in replacement at initialization time (not per-frame). ✅
Pattern consistency:
- The
local-ab/local-ab-portpair mirrors the existingmcp/mcp-portpattern faithfully:- MCP port resolution:
McpServerPlugin.cs:79-83—TryGetValue+int.TryParse+ range clamp - Local AB port resolution:
RealmLaunchSettings.cs:108-113— identical shape - MCP implication:
FeaturesRegistry.cs:75—HasFlag(MCP) || HasFlag(MCP_PORT) - Local AB implication:
RealmLaunchSettings.cs:140—HasFlag(LOCAL_AB) || HasFlag(LOCAL_AB_PORT)
- MCP port resolution:
Verdict: PASS — no design issues. Code is in the right place.
STEP 4 — Member audit ✅
| Member | Consumers | Assessment |
|---|---|---|
AppArgsFlags.LOCAL_AB_PORT |
DeepLinkAllowlist, RealmLaunchSettings (×2: implication + resolve), tests |
Multi-use constant, appropriate |
ResolveLocalAssetBundlesUrl(IAppArgs) |
MainSceneLoader.cs:236, RealmLaunchSettingsShould tests |
2 production+test consumers; encapsulates port validation + URL construction; static with no hidden state |
MIN_LOCAL_AB_PORT / MAX_LOCAL_AB_PORT |
ResolveLocalAssetBundlesUrl |
Private constants at top of type per convention |
No single-use extraction, no absent≠false conflation, no redundant guards.
STEP 5 — Line-level review
Security audit — no issues found:
- Port validation (
int.TryParse+ 1024-65535 range) is strict — non-numeric values, path suffixes (5147/path), and out-of-range ports all fall back to the default. The parsedintis interpolated into a hardcodedhttp://127.0.0.1:{port}template, so no host/path injection is possible. optimized-assets-urlis correctly kept in the never-permitted tier and now has an explicit test assertion for loopback realms (AppArgsTests.cs:130).- The loopback gate (
Uri.IsLoopback) is sound — DNS rebinding and userinfo tricks don't bypass it in .NET'sUriimplementation. - Defense in depth: loopback gate → hardcoded host → strict int parsing → range clamp.
One P2 finding — see inline comment.
STEP 6 — Complexity
SIMPLE. Changes are a straightforward allowlist addition + port validator + tests. No ECS systems, components, queries, async/UniTask, plugin registration, or assembly definition changes.
STEP 7 — QA assessment
QA_REQUIRED: YES. The PR modifies runtime code (RealmLaunchSettings.cs, MainSceneLoader.cs) that affects asset bundle loading behavior during local scene development. Manual verification with a local scene server and abgen is needed to confirm the deep-link flow works end-to-end.
STEP 8 — Non-blocking warnings
None. Main.unity is not modified.
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Straightforward allowlist entries, a static port-validation helper, and implication logic in the existing RealmLaunchSettings — no ECS, async, or system-level changes.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Reworks the approach per decentraland/js-sdk-toolchain#1504: the preview server now proxies its abgen sidecar at {realm}/optimized-assets, so the client derives the assets base from the single origin it already has. local-ab becomes a pure boolean deep-link param (the base derives from the realm the loopback gate already validated), local-ab-port is removed before ever shipping, and the 5147 default-port convention retires along with it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warnings not reduced: 14075 => 14087 — remove at least one warning to merge. Warnings/errors in files changed by this PR (30) |
|
Tests: 24382 passed, 0 failed ✅ |
dalkia
changed the title
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — #9519 fix: allow local-ab deep-link param for loopback realms, deriving the assets base from the realm
STEP 2 — Root-cause check: ✅ PASS
The problem: Creator Hub / sdk-commands preview deep links need to enable local asset bundles, but local-ab was not allowlisted for deep links, and the full-URL override (optimized-assets-url) is deliberately never-permitted because it rewrites AB/LOD/registry endpoints globally with an attacker-controllable URL. The previous fallback also hardcoded port 5147, coupling the client to abgen's default port.
The fix addresses the root cause correctly:
- Adds the boolean
local-abto the loopback-only tier (no URL data flows through it). - Derives the optimized-assets base from the realm itself (
{realm}/optimized-assets), so the preview server proxies its abgen sidecar and the client needs no second origin. - Keeps
optimized-assets-urlin the never-permitted tier, now pinned by an explicit test.
This is a cause fix, not a symptom workaround.
STEP 3 — Design & integration: ✅ PASS
No new long-lived units introduced. The only new member is LocalAssetBundlesBaseUrl() — a pure derived method on RealmLaunchSettings that reads existing fields (customRealm, isLocalSceneDevelopmentRealm, initialRealm). No persistent state, no lifecycle management.
Owner search:
customRealmis owned byRealmLaunchSettings.SetLocalSceneDevelopmentRealm()/SetCustomRealm()— the same class.useLocalAssetBundlesis set inParseRealmAppParameter()— the same class.- The URL derivation logically belongs here:
RealmLaunchSettingsowns both the realm and the local-AB flag, so deriving the URL from them is cohesive. MainSceneLoader.InitializeFlowAsync()is the existing wiring point wherecliOptimizedAssetsUrlis resolved and passed toGatewayUrlsSource— the one-line change (line 236) correctly calls the settings object rather than inlining the derivation.
Precedence chain: CLI --optimized-assets-url > LocalAssetBundlesBaseUrl() > null (falls through to feature flags / default CDN). Verified correct — matches the established cliGatekeeperUrl pattern on the adjacent line.
Allowlist tier placement: local-ab fits the loopback-only tier cleanly. It's a pure boolean whose effect (enable AB loading from {realm}/optimized-assets) is gated on the same loopback validation the tier already enforces. The dangerous full-URL variant stays never-permitted.
OPTIMIZED_ASSETS_PATH constant: Placed on RealmLaunchSettings where its sole consumer (LocalAssetBundlesBaseUrl()) lives — correct. If other subsystems needed it, extraction would be warranted, but currently unnecessary.
Teardown / consumption trace: No subscriptions, callbacks, connections, or disposables are added. N/A.
STEP 4 — Member audit: ✅ PASS
LocalAssetBundlesBaseUrl() (RealmLaunchSettings.cs:107) — 2 consumers: MainSceneLoader.InitializeFlowAsync() (runtime) and RealmLaunchSettingsShould tests (2 test methods). Not a single-use merge candidate — it encapsulates URL derivation from data owned by RealmLaunchSettings; the caller (MainSceneLoader) should not contain this logic. Returns string? — null for the non-applicable case (not local-scene-dev), which the caller handles correctly (falls through to downstream defaults). No absent ≠ false/null concern.
OPTIMIZED_ASSETS_PATH (RealmLaunchSettings.cs:28) — Used in LocalAssetBundlesBaseUrl() and in the serialized [Tooltip]. Appropriate as a named constant replacing the previous magic port.
STEP 5 — Line-level review: ✅ No issues found
Pass A (blocking issues): None found.
- No CLAUDE.md violations, no bugs, no security vulnerabilities, no performance issues, no missing error handling, no resource leaks, no detached async, no nullability violations, no false-intent conditions.
TrimEnd('/')inLocalAssetBundlesBaseUrl()correctly prevents double-slash; tested with trailing-slash case.IRealmNavigator.LOCALHOST("http://127.0.0.1:8000") has no trailing slash — no issue.- Path-prefix audit confirmed: downstream consumers (
URLBuilder.AppendDomain,URLDomain.Append,ComposeRegistryUrlstring interpolation) all compose paths verbatim onto the base.AppendDomainWithReplacedPathis only used with the Lambdas URL, never with optimized-assets endpoints.
Pass B (design/encapsulation smells): None found.
- No construction smell, no naming issues, no over-explained comments (the extensive security rationale is justified for an allowlist boundary), no magic values, no resource lifecycle concerns.
Security Review: ✅ No security issues found
- Loopback gate:
Uri.TryCreate+Uri.IsLoopbackcovers 127.0.0.1, localhost, [::1]. Remote-realm deep links correctly droplocal-ab. - Boolean-only consumption:
local-abis consumed viaHasFlag()— a presence check, not a value read. No path to inject a URL. - URL derivation:
LocalAssetBundlesBaseUrl()constructscustomRealm.TrimEnd('/') + "/optimized-assets"— the only input is the realm that the loopback gate already validated. No attacker-controllable data. @URI bypass check: .NETUriparseshttp://127.0.0.1:8000@evil.examplewithevil.exampleas host (IsLoopback = false) — not a bypass vector.optimized-assets-urlstays never-permitted: Confirmed absent from bothPERMITTED_KEYSandLOOPBACK_REALM_PERMITTED_KEYS; pinned byDeepLinkDropsExecAndInfraParamsEvenForLoopbackRealmtest.- No hardcoded secrets, credentials, or injection vectors.
- Known residual (documented): The override outlives realm changes in-session — pre-existing behavior, and strictly narrower exposure than the previously-accepted
mcp-portsince it can only point at the loopback realm the link targeted.
STEP 6 — Complexity
SIMPLE — The changes are straightforward: one boolean added to an existing allowlist set, one URL derivation method replacing a hardcoded constant, and corresponding tests. No ECS systems, async flows, plugin wiring, or cross-assembly interface changes.
STEP 7 — QA assessment
QA_REQUIRED: YES — Changes affect how the client derives optimized-assets URLs during local scene development (runtime code under Explorer/).
STEP 8 — Non-blocking warnings
main instead of dev. The standard branch model for unity-explorer targets dev. The hotfix/ prefix suggests this is intentional — verify with the team.
Test Coverage
Comprehensive:
DeepLinkKeepsLocalAbForLoopbackRealm— boolean survives for loopback realmDeepLinkDropsLocalAbForRemoteRealm— boolean dropped for remote realmDeepLinkDropsExecAndInfraParamsEvenForLoopbackRealm—optimized-assets-urlpinned as never-permittedDeriveLocalAssetBundlesBaseUrlFromRealm— parametrized with trailing-slash and different loopback formsNotDeriveLocalAssetBundlesBaseUrlOutsideLocalSceneDevelopment— null for non-local-dev mode
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Adds a boolean to an existing deep-link allowlist set and replaces a hardcoded port constant with a realm-derived URL method — straightforward config/infrastructure change.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni (<@U03JSUQ5Z7U>) via Slack
…ronment dclenv sat in the loopback-realm tier of the deep-link allowlist. The login callback and jump-in links that carry it have no realm at all, so that condition could never pass and the param was always dropped — a client launched from such a link silently fell back to the default environment (org) even when the flow started on zone, taking its auth-api, signature web app and identity storage with it. Move it to the always-permitted tier. It is safe on its own: a closed Decentraland-owned enum, parsed with Enum.TryParse where it is consumed and ignored otherwise, never a URL, so it cannot point the client at attacker infrastructure. Its worst case, a session in a Decentraland-owned test environment, is strictly weaker than realm, which is already always-permitted. The local-development params (local-scene, hub, skip-auth-screen, landscape-terrain-enabled, multi-instance, mcp, mcp-port, local-ab) stay loopback-gated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
DafGreco
left a comment
DafGreco
left a comment
There was a problem hiding this comment.
✔️ PR reviewed and approved by QA on both platforms following instructions playing both happy and un-happy path
Regressions for this ticket had been performed in order to verify that the normal flow is working as expected:
- [ ✔️] Backpack and wearables in world
- [✔️ ] Emotes in world and in backpack
- [✔️ ] Teleport with map/coordinates/Jump In
- [✔️ ] Chat and multiplayer
- [✔️ ] Profile card
Evidence with --asset-bundles and without the flag
more evidence in the following thread
Ludmilafantaniella
left a comment
Ludmilafantaniella
left a comment
There was a problem hiding this comment.
✅ Approved. QA'd on Windows and Mac.
Test Steps completed:
- ✅ Build launched via deep-link flow, branch confirmed (
local-ab-allowlistshown in-game / correct branch on both platforms). - ✅ Bundles conversion confirmed (sidecar logs showing manifest + per-asset conversion + bundle serving).
- ✅ Memory comparison in Genesis Plaza:
- Mac: 7026 MB without
--asset-bundles→ 6931 MB with (lower, as expected) - Windows: 3340 MB without
--asset-bundles→ 2357 MB with (lower, as expected)
- Mac: 7026 MB without
- ✅ Degrade path: killed
abgen.exemid-session (Task Manager) — scene kept rendering fine visually, reload (button + chat command) worked without issues. - ✅ Hot reload: verified the sidecar picks up
.glbchanges and reconverts only the modified asset. - ✅ Wearable caching: wearables/emotes loaded correctly and stayed cached across scene re-entry.
Notes / minor findings (non-blocking):
- When
abgenwas killed mid-session, the server terminal logged repeatedECONNREFUSEDerrors to port 8001 for as long as the sidecar stayed down. Visual degrade path works fine, but wanted to flag the log spam in case it's worth a follow-up. - On one Mac run with
--asset-bundlesactive, the avatar's wearables/hair didn't load (bare body) — didn't reproduce consistently, flagging in case it's relevant.
Windows With Asset Bundles:
Windows Without Asset Bundles:
Mac With Asset Bundles:
Mac Without Asset Bundles:
|
✅ PR reviewed and approved by QA following the PR test instructions. Build: Test results:
Unrelated errors noted (do not affect verdict):
Verdict: PASS ✅ Remaining test steps (degrade path, wearable caching) verified by the rest of the QA team — overall team consensus is PASS. The two items above are tracked separately and do not block this PR. Player asset bundle args 9519.log replace.GLB.mp4 |
7 participants
Pull Request Description
What does this PR change?
Creator Hub /
sdk-commandspreview deep links need to enable local asset bundles, butlocal-abwas not yet allowlisted and the full-URL override (optimized-assets-url) is deliberately never-permitted for deep links: it rewrites theAssetBundlesCDN,LodGeneratorCDN, andAssetBundleRegistryendpoints globally for the whole session, and the loopback-realm gate only checks therealmparam — which an attacker also controls — so allowlisting it would let a crafted link point asset loading at arbitrary remote infrastructure.This PR implements the explorer side of decentraland/js-sdk-toolchain#1504: the preview server proxies its abgen sidecar at
{realm}/optimized-assets, so the client derives the assets base from the single origin it already has — no URL, port, or any other attacker-controllable value travels in the link.local-abtoDeepLinkAllowlist.LOOPBACK_REALM_PERMITTED_KEYS(with the per-key security rationale the set requires). It is a pure boolean: the optimized-assets base becomes{realm}/optimized-assets(RealmLaunchSettings.LocalAssetBundlesBaseUrl), the same realm the loopback gate already validated, so the flag can only point asset loading at the realm the link already targets.InitialRealm.Localhostpath derives fromIRealmNavigator.LOCALHOSTthe same way, so the deeplink-less Editor dev loop keeps working.--optimized-assets-urlCLI arg still takes precedence (CLI args are trusted and not allowlist-filtered); the param stays never-permitted for deep links, now pinned by a test.http://127.0.0.1:5147default-port convention — with the proxy, the sidecar's port is private to sdk-commands (it moves back togetPort(0)), so there is nothing to guess./optimized-assets): all consumers compose verbatim —URLBuilder.AppendDomain(manifest loads),URLDomain.Append/URLBuilder.Combine(bundle URLs), andComposeRegistryUrlstring interpolation (registry endpoints). The path-strippingAppendDomainWithReplacedPathis only used with the Lambdas URL, never with the optimized-assets endpoints.Note: per
DeepLinkAllowlist's class doc, changes to the permitted sets are a SEC-019/020 product decision — flagging for that sign-off in review.local-abflow requires decentraland/js-sdk-toolchain#1504 (the/optimized-assetsproxy route) to be released.Test Instructions
Easiest way to run this build: swap it into the launcher's
latestfolderThe SDK/Creator Hub deeplink flow always launches whatever build sits in the launcher's
latestfolder, so the most straightforward way to test end-to-end is to put this PR's build there:~/Library/Application Support/DecentralandLauncherLight/latest/Decentraland.app%APPDATA%\DecentralandLauncherLight\latest\Decentraland.app→Decentraland.original.app) so the released build is not lost.Decentraland.app) — the deeplink flow now opens it transparently, no App Parameters needed.latest.Prerequisites
No abgen installation or
ABGEN_*configuration is needed — the SDK downloads a pinned, sha256-verified abgen binary (v0.11.7) on first run and wires it automatically. ETest Steps
latestfolder (see above) and just run the scene — the deeplink flow opens it with all parameters set.feat/lsd-local-ab-allowlist.--asset-bundlesshould sit noticeably lower than without it (Genesis Plaza used as the reference scene). Please report any inconsistency between flag on and off.--asset-bundles(or kill the sidecar and reload) → the scene still renders via raw GLTFs, no exception spam..glbin the scene → the scene reloads, only that asset reconverts, and the updated model appears.Additional Testing Notes
.dcl-optimized-assets/, so any subsequent run is fine. Known issue, still to be addressed.Quality Checklist
🤖 Generated with Claude Code