chore: sync main to dev#9479
Open
github-actions[bot] wants to merge 3 commits into
Open
Conversation
* fix: guard GltfContainer dereference against destroyed Root (#9452) A stale StreamableLoadingResult could still reference a GltfContainerAsset whose Root GameObject was already destroyed by GltfContainerAsset.Dispose (drained by cache Unload). Re-dereferencing it invoked SetActive/SetParent on the dead Root, surfacing as a NullReferenceException in IL2CPP (ResetGltfContainerSystem -> DereferenceFinalOperation, Sentry UNITY-EXPLORER-P83). Guard the single choke point every dereference path funnels through: skip when asset.Root compares null (Unity destroyed-object check) before touching cache state, so the crash is avoided and a dead instance is never re-pooled. Add unit coverage for the pooled-live and destroyed-Root cases. * log: repro the issue * Refactor Dereference method in GltfContainerAssetsCache Removed stale promise handling and associated logging from Dereference method. Signed-off-by: alejandro-jimenez-dcl <alejandro.jimenez@decentraland.org> * fix: restore destroyed-Root guard without repro logging Re-add the asset.Root null guard in Dereference so a stale promise result referencing an already-destroyed GameObject skips re-pooling, keeping only the early return without the temporary repro logging. --------- Signed-off-by: alejandro-jimenez-dcl <alejandro.jimenez@decentraland.org> (cherry picked from commit 481ff99)
…3/004/005/008/019/028) (#9466) * fix(sec-019): deny-by-default allowlist for deep-link params Replace the copy-every-query-param behavior in ProcessDeepLinkParameters with a shared deny-by-default allowlist (DeepLinkAllowlist). Only realm, position, community, signin, authRequestId, force-open-backpack survive; attacker-controllable flags (creator-hub-bin-path, launch-cdp-monitor-on-start, local-scene, comms-adapter, ...) are dropped. Closes the SEC-019 arg-injection root shared by the cold-start argv and runtime bridge paths. SEC-020: local-scene is dropped by default, but re-permitted when the deep link's realm is a loopback host (Uri.IsLoopback) so the legitimate local-scene -development deep link keeps working while a remote-realm attacker link cannot enable LSD. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(sec-005): pin creator-hub binary to install path, drop app-arg override * fix(sec-008/028): http(s)-only external-URL scheme policy Add ExternalUrlPolicy as the single http(s) predicate for the client and enforce it at the shared external-URL sink (UnityAppWebBrowser) so no caller can hand a custom-scheme URL (decentraland://, smb://, file://, mailto://, app-protocol) to the OS handler. Blocks SEC-028 launcher re-invocation and SEC-008 NTLM/local-file abuse. IsWebScheme validates the scheme by prefix without allocating a Uri; the trust-cache path keeps using the already-parsed Uri via TryGetTrustKey. Unifies the scattered http(s) URL checks (RealmLaunchSettings.IsRealmAValidUrl, RealmUrls.CustomRealmAsync) onto this single function. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(sec-008): reject non-web schemes at external-url prompt construction * fix(sec-008): key external-url trust cache on (scheme, host); never cache empty host * fix(sec-003): changeRealm prompt always requires explicit approval Remove the empty-message auto-approve: RequestChangeRealm always renders the destination (default confirmation text when no message) and WaitForCloseIntentAsync always waits for an explicit Continue/Cancel/Close click. A scene can no longer switch realm silently by passing an empty message. Harden the consent display (SEC-003 #3): disable rich-text on the message/realm TMP fields so an attacker-controlled string cannot inject TMP markup into the prompt, and show the destination host (authority) with any misleading path/query stripped, so the user consents to the real destination. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(sec-004): host-suffix realm validation instead of substring match * fix(sec-004): route runtime realm deep links through the consent prompt A realm deep link on a running client now shows the ChangeRealm consent prompt (Cancel does not switch). The prompt callback and Params carry the target parcel as a typed Vector2Int? — a deep link with a position teleports via ChatTeleporter.TeleportToRealmAsync(realm, position) directly (works for URL realms), instead of stringifying the position into the realm and round-tripping it through the /goto text command. Realm-only changes keep the existing chat-command route. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(sec-deeplink): resolve net-new nullability warnings + document allowlist Nullability (warnings introduced by this branch): - ExternalUrlPromptController.Params.Uri is genuinely nullable (null for a non-web URL) -> declare Uri? and capture a non-null local in the consumer; clears CS8601 + the "always true/false" guards. - Drop the redundant ?. on the non-nullable changeRealmCallback field. - ChangeRealmPromptPlugin serialized field -> = null! (Unity-serialized). DeepLinkAllowlist: document the deny-by-default rationale and add a per-arg comment explaining why each permitted key is safe (navigation/share/login intents gated by consent, a matching login token, or a plain coordinate); keep PERMITTED_KEYS private behind IsPermitted. Remaining warnings in these files are pre-existing framework/lifecycle patterns (viewInstance nullability, resultCallback lifecycle) left as-is. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fix compiler warnings * Always print dropped app args * chore(sec-005): remove creator-hub-bin-path arg; editor-only path override creator-hub-bin-path was the SEC-005 RCE vector (an attacker-reachable app-arg / deep-link param that set the executable CreatorHubBrowser launches). It is already neutralized by pinning the browser to the default install path, so remove the arg entirely: the AppArgsFlags const, the Debug Settings drawer plumbing, and the docs entry. No runtime code reads an externally supplied executable path anymore. Restore the developer's ability to point CDP DevTools at a non-default Creator Hub build via an editor-only override stored in EditorPrefs (CreatorHubBrowser.BinPathOverride), assigned through the Debug Settings drawer. It is read only under #if UNITY_EDITOR and never comes from app-args or deep links, so player builds can launch only the pinned default path. - CreatorHubBrowser: ResolveBinPath() returns the EditorPrefs override in the Editor, else DEFAULT_CREATOR_HUB_BIN_PATH. - DebugSettingsDrawer: path field / Browse / "Use Default Path" now read-write EditorPrefs instead of injecting a --creator-hub-bin-path app-arg. - AppArgsTests: SEC-005 regression now asserts the literal key is dropped by deny-by-default (the const no longer exists). - docs/app-arguments.md: drop the creator-hub-bin-path entry. Verified: Unity 6000.4.0f1 batch EditMode compile clean (0 error CS), AppArgs tests 10/10. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(sec-005): allow creator-hub-bin-path via app-args only Restore the creator-hub-bin-path override (removed in the prior commit). With the SEC-019 deny-by-default deep-link allowlist in place, the param can only reach appParameters through the trusted command-line channel — a decentraland:// link carrying it is dropped (asserted by DeepLinkDropsInternalFlags). The RCE vector is therefore closed by the allowlist, so exposing the app-arg override again is safe. - AppArgsFlags: re-add CREATOR_HUB_BIN_PATH const. - CreatorHubBrowser: read the creator-hub-bin-path app-arg via IAppArgs, else the pinned DEFAULT_CREATOR_HUB_BIN_PATH. - ChromeDevToolHandler.New / BootstrapContainer: thread IAppArgs back in. - DebugSettingsDrawer: editor path field writes the app-arg (appParameters) again instead of EditorPrefs; widen two locals to string?. - AppArgsTests: DeepLinkDropsInternalFlags asserts the key stays dropped from deep links (the "app-args only" guarantee). - docs/app-arguments.md: restore the entry, noting it is app-args-only. Security note: this makes the SEC-019 allowlist the sole barrier keeping creator-hub-bin-path off the untrusted channel (A2's binary pin is gone). That holds only while the launcher never promotes received deep-link params to trusted --flag args (cross-repo invariant; the CH app-args migration spec's launcher guard test locks it). Verified: Rider solution analysis reports 0 errors on all changed files. Unity batch EditMode was not re-run this iteration (the Editor held the project lock); the only test change swaps a literal for the identical-valued const. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(sec-005): resolve Creator Hub path at runtime; drop the app-arg Remove the creator-hub-bin-path app-arg (restored in the previous commit) and resolve the Creator Hub executable at the point of use instead. The CDP bridge is the only consumer, so CreatorHubBrowser.OpenUrl now resolves lazily: 1. a path the developer previously selected (DCLPlayerPrefs), if it still exists; 2. else the pinned default install path; 3. else prompt the developer to locate it (FileBrowser PRO - Editor + Win/Mac/Linux standalone) and remember the choice in DCLPlayerPrefs. No attacker-reachable input feeds the exec sink: the path is either the known install location or a local file the developer explicitly picks, and the prompt fires only when DevTools actually needs to launch Creator Hub. - AppArgsFlags: drop CREATOR_HUB_BIN_PATH. - CreatorHubBrowser: PlayerPrefs -> default -> FileBrowser prompt; no IAppArgs. - ChromeDevToolHandler / BootstrapContainer: drop the IAppArgs plumbing. - DebugSettingsDrawer: drop the path field; keep the CDP toggle + install status. - DCLPrefKeys: add CreatorHub.BinPath. - DCL.Network.asmdef: reference filebrowserpro.runtime (the assembly CreatorHubBrowser compiles into, via the DCL.WebRequests asmref). - AppArgsTests: assert creator-hub-bin-path stays dropped from deep links. - docs/app-arguments.md: remove the entry. Verified: Unity 6000.4.0f1 batch EditMode - compile clean (0 error CS), AppArgs tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(sec-019): permit SDK/Creator Hub dev deep-link params for a loopback realm Generalize the local-scene loopback gate into a second allowlist tier: the local-development params Creator Hub and sdk-commands attach to their preview deep links are permitted only when the target realm is loopback (127.0.0.1 / localhost / [::1]). A remote-realm deep link from a web page cannot enable them, while a legitimate local-dev launch (which always targets loopback) works. Loopback-permitted set: local-scene, dclenv, hub, skip-auth-screen, landscape-terrain-enabled, multi-instance. Each is individually low-harm (env enum, analytics tag, cosmetic toggle, instance count, or a screen skip that still forces auth when no valid identity is cached); the loopback gate confines them to the dev context. The SEC-005 exec params (creator-hub-bin-path, launch-cdp-monitor-on-start) and the attacker-infrastructure params stay dropped for every realm. - DeepLinkAllowlist: add LOOPBACK_REALM_PERMITTED_KEYS + IsPermittedForLoopbackRealm; document the three tiers (always / loopback-only / never). - ApplicationParametersParser.ProcessDeepLinkParameters: two-tier resolution (base keys -> normalize realm -> loopback-gated dev keys), replacing the local-scene-only special case. - AppArgsTests: dev params kept for a loopback realm, dropped for a remote realm, and exec/infra params dropped even for a loopback realm. SEC-019/020 policy expansion (product decision). Verified: Unity 6000.4.0f1 batch EditMode - 0 compile errors, AppArgs tests 13/13. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(sec-004): strip userinfo from consent-prompt host; drop stale spawnpoint doc Addresses the decentraland-bot review of PR #9466: - P1 (security): ChangeRealmPromptController.DestinationHostFor now skips past any userinfo ('@') so the real host after the last '@' is shown on the consent prompt. Previously a realm like https://decentraland.org@evil.com displayed "decentraland.org@evil.com" while actually connecting to evil.com — a spoof. Matches the '@' handling already in ChatEnvironmentValidator.HostHasSuffix. - P2 (docs): DeepLinkAllowlist XML doc no longer lists 'spawnpoint' as permitted — it isn't in PERMITTED_KEYS and no SPAWN_POINT const exists on this branch (the dev spawn-points feature #9369 was dropped when the branch was made SEC-only). Verified: Unity 6000.4.0f1 batch EditMode - 0 compile errors, AppArgs tests 13/13. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(sec-004): close HostHasSuffix colon-before-@ host spoof + review cleanups Addresses the decentraland-bot re-review of PR #9466 (1 P1 + 5 P2): - P1 (security): HostHasSuffix broke its scan on ':' before reaching any '@', so https://decentraland.org:1234@evil.com extracted host "decentraland.org" and passed the suffix check while the real host is evil.com. Fix: don't break on ':'; reject on '@' (checked before any ':'); strip the port after the loop. + regression cases (colon-before-@, plain userinfo, legit host:port). - DestinationHostFor: made internal + new ChangeRealmPromptDestinationHostShould (7 cases: userinfo/port/query/fragment/no-scheme). UI assembly exposes internals to DCL.EditMode.Tests. - DeepLinkHandleImplementation: thread the cancellation token through SwitchToMainThread and ShowAsync. - DeepLinkAllowlist: remove the // ReSharper disable CheckNamespace suppression (CLAUDE.md §11; the namespace compiles fine — matches siblings). - ChangeRealmPromptController.Params: collapse the dual constructors into one with a default; simplify the caller. - DynamicWorldContainer: TODO documenting the position-teleport result/token gap. Verified: Unity 6000.4.0f1 batch EditMode - 0 compile errors, 30/30 tests pass (AppArgs 13, ChatEnvironmentValidatorShould 10, ChangeRealmPromptDestinationHostShould 7). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
release: 2026-07-24
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
👑 An automated PR