From 537f9fcf0e42df0f246d8a8daf2bd0b275bdc2c7 Mon Sep 17 00:00:00 2001 From: alejandro-jimenez-dcl Date: Thu, 23 Jul 2026 12:02:00 +0200 Subject: [PATCH 1/2] fix: gltf container dereference destroyed root (#9460) * 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 * 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 (cherry picked from commit 481ff99a23e368eb237e9832df47cbd2b205fcf2) --- .../Asset/Cache/GltfContainerAssetsCache.cs | 6 +++ .../Tests/GltfContainerAssetsCacheShould.cs | 48 +++++++++++++++++++ .../GltfContainerAssetsCacheShould.cs.meta | 2 + 3 files changed, 56 insertions(+) create mode 100644 Explorer/Assets/DCL/Infrastructure/ECS/Unity/GLTFContainer/Asset/Tests/GltfContainerAssetsCacheShould.cs create mode 100644 Explorer/Assets/DCL/Infrastructure/ECS/Unity/GLTFContainer/Asset/Tests/GltfContainerAssetsCacheShould.cs.meta diff --git a/Explorer/Assets/DCL/Infrastructure/ECS/Unity/GLTFContainer/Asset/Cache/GltfContainerAssetsCache.cs b/Explorer/Assets/DCL/Infrastructure/ECS/Unity/GLTFContainer/Asset/Cache/GltfContainerAssetsCache.cs index 30b9b80ee97..6c0cc3343fd 100644 --- a/Explorer/Assets/DCL/Infrastructure/ECS/Unity/GLTFContainer/Asset/Cache/GltfContainerAssetsCache.cs +++ b/Explorer/Assets/DCL/Infrastructure/ECS/Unity/GLTFContainer/Asset/Cache/GltfContainerAssetsCache.cs @@ -90,6 +90,12 @@ public bool TryGet(in string key, out GltfContainerAsset? asset) /// public void Dereference(in string key, GltfContainerAsset asset, bool putInBridge = false, bool handleAssetLoad = true) { + // A stale promise result can still reference an asset whose Root was already destroyed + // (e.g. drained by Unload). Comparing against Unity's overloaded null catches that destroyed + // GameObject; re-pooling it would crash DereferenceFinalOperation and hand a dead instance back out. + if (asset.Root == null) + return; + if (handleAssetLoad && assetLoadCache != null && assetLoadCache.ContainsGltf(key)) { assetLoadCache.ReleaseGltfInstance(key, asset); diff --git a/Explorer/Assets/DCL/Infrastructure/ECS/Unity/GLTFContainer/Asset/Tests/GltfContainerAssetsCacheShould.cs b/Explorer/Assets/DCL/Infrastructure/ECS/Unity/GLTFContainer/Asset/Tests/GltfContainerAssetsCacheShould.cs new file mode 100644 index 00000000000..b0d9ee4994d --- /dev/null +++ b/Explorer/Assets/DCL/Infrastructure/ECS/Unity/GLTFContainer/Asset/Tests/GltfContainerAssetsCacheShould.cs @@ -0,0 +1,48 @@ +using DCL.Optimization.Pools; +using ECS.Unity.GLTFContainer.Asset.Cache; +using ECS.Unity.GLTFContainer.Asset.Components; +using NSubstitute; +using NUnit.Framework; +using UnityEngine; +using Object = UnityEngine.Object; + +namespace ECS.Unity.GLTFContainer.Asset.Tests +{ + [TestFixture] + public class GltfContainerAssetsCacheShould + { + private GltfContainerAssetsCache cache; + + [SetUp] + public void SetUp() + { + cache = new GltfContainerAssetsCache(Substitute.For()); + } + + [Test] + public void PoolLiveAssetOnDereference() + { + var asset = GltfContainerAsset.Create(new GameObject(), assetData: null); + + cache.Dereference("hash", asset); + + Assert.That(cache.TryGet("hash", out GltfContainerAsset? pooled), Is.True); + Assert.That(pooled, Is.SameAs(asset)); + + Object.DestroyImmediate(asset.Root); + } + + [Test] + public void SkipDereferenceWhenRootAlreadyDestroyed() + { + // A stale promise result can reference an asset whose Root was destroyed by a prior Unload. + var asset = GltfContainerAsset.Create(new GameObject(), assetData: null); + Object.DestroyImmediate(asset.Root); + + Assert.DoesNotThrow(() => cache.Dereference("hash", asset)); + + // The dead asset must not be re-pooled, otherwise TryGet would hand a destroyed instance back out. + Assert.That(cache.TryGet("hash", out _), Is.False); + } + } +} diff --git a/Explorer/Assets/DCL/Infrastructure/ECS/Unity/GLTFContainer/Asset/Tests/GltfContainerAssetsCacheShould.cs.meta b/Explorer/Assets/DCL/Infrastructure/ECS/Unity/GLTFContainer/Asset/Tests/GltfContainerAssetsCacheShould.cs.meta new file mode 100644 index 00000000000..54562a9af33 --- /dev/null +++ b/Explorer/Assets/DCL/Infrastructure/ECS/Unity/GLTFContainer/Asset/Tests/GltfContainerAssetsCacheShould.cs.meta @@ -0,0 +1,2 @@ +fileFormatVersion: 2 +guid: 21b8cd3964c9b8642b70e255e64b11b4 \ No newline at end of file From 2eb218c3bf5eee4e5c9504f45c2654eca4bbb0a1 Mon Sep 17 00:00:00 2001 From: Mikhail Agapov <118179774+mikhail-dcl@users.noreply.github.com> Date: Fri, 24 Jul 2026 18:00:23 +0300 Subject: [PATCH 2/2] fix: harden decentraland:// deep-link & external-URL handling (SEC-003/004/005/008/019/028) (#9466) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * 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 * 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 * 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 * 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 * 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 * 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 * 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 * 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 * 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 * 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 --------- Co-authored-by: Claude Fable 5 --- .../ChangeRealmPromptController.Params.cs | 10 +- .../ChangeRealmPromptController.cs | 62 +++++--- .../Chat/Commands/ChatEnvironmentValidator.cs | 72 +++++++-- .../ExternalUrlPromptController.Params.cs | 10 +- .../ExternalUrlPromptController.cs | 35 +++-- .../Global/AppArgs/AppArgsFlags.cs | 1 - .../AppArgs/ApplicationParametersParser.cs | 30 +++- .../Global/AppArgs/DeepLinkAllowlist.cs | 99 ++++++++++++ .../Global/AppArgs/DeepLinkAllowlist.cs.meta | 2 + .../Global/AppArgs/Tests/AppArgsTests.cs | 84 +++++++++++ .../Global/Dynamic/BootstrapContainer.cs | 2 +- .../Global/Dynamic/DynamicWorldContainer.cs | 12 +- .../Global/Dynamic/RealmLaunchSettings.cs | 7 +- .../Global/Dynamic/RealmUrl/RealmUrls.cs | 3 +- .../Global/Editor/DebugSettingsDrawer.cs | 141 +----------------- .../Browser/ExternalUrlPolicy.cs | 35 +++++ .../Browser/ExternalUrlPolicy.cs.meta | 11 ++ .../Browser/Tests/ExternalUrlPolicyShould.cs | 33 ++++ .../Tests/ExternalUrlPolicyShould.cs.meta | 11 ++ .../Browser/UnityAppWebBrowser.cs | 7 + .../DCL/NetworkDefinitions/DCL.Network.asmdef | 3 +- .../Global/ChangeRealmPromptPlugin.cs | 6 +- Explorer/Assets/DCL/Prefs/DCLPrefKeys.cs | 4 + .../DeepLinkHandleImplementation.cs | 20 ++- .../ChangeRealmPromptDestinationHostShould.cs | 20 +++ ...geRealmPromptDestinationHostShould.cs.meta | 2 + .../Editor/ChatEnvironmentValidatorShould.cs | 31 ++++ .../ChatEnvironmentValidatorShould.cs.meta | 2 + Explorer/Assets/DCL/UI/AssemblyInfo.cs | 5 + Explorer/Assets/DCL/UI/AssemblyInfo.cs.meta | 2 + .../ChromeDevtool/ChromeDevToolHandler.cs | 7 +- .../ChromeDevtool/CreatorHubBrowser.cs | 73 ++++++--- docs/app-arguments.md | 11 -- 33 files changed, 610 insertions(+), 243 deletions(-) create mode 100644 Explorer/Assets/DCL/Infrastructure/Global/AppArgs/DeepLinkAllowlist.cs create mode 100644 Explorer/Assets/DCL/Infrastructure/Global/AppArgs/DeepLinkAllowlist.cs.meta create mode 100644 Explorer/Assets/DCL/NetworkDefinitions/Browser/ExternalUrlPolicy.cs create mode 100644 Explorer/Assets/DCL/NetworkDefinitions/Browser/ExternalUrlPolicy.cs.meta create mode 100644 Explorer/Assets/DCL/NetworkDefinitions/Browser/Tests/ExternalUrlPolicyShould.cs create mode 100644 Explorer/Assets/DCL/NetworkDefinitions/Browser/Tests/ExternalUrlPolicyShould.cs.meta create mode 100644 Explorer/Assets/DCL/Tests/Editor/ChangeRealmPromptDestinationHostShould.cs create mode 100644 Explorer/Assets/DCL/Tests/Editor/ChangeRealmPromptDestinationHostShould.cs.meta create mode 100644 Explorer/Assets/DCL/Tests/Editor/ChatEnvironmentValidatorShould.cs create mode 100644 Explorer/Assets/DCL/Tests/Editor/ChatEnvironmentValidatorShould.cs.meta create mode 100644 Explorer/Assets/DCL/UI/AssemblyInfo.cs create mode 100644 Explorer/Assets/DCL/UI/AssemblyInfo.cs.meta diff --git a/Explorer/Assets/DCL/ChangeRealmPrompt/ChangeRealmPromptController.Params.cs b/Explorer/Assets/DCL/ChangeRealmPrompt/ChangeRealmPromptController.Params.cs index a57ed909f85..a4ac3281630 100644 --- a/Explorer/Assets/DCL/ChangeRealmPrompt/ChangeRealmPromptController.Params.cs +++ b/Explorer/Assets/DCL/ChangeRealmPrompt/ChangeRealmPromptController.Params.cs @@ -1,4 +1,6 @@ -namespace DCL.ChangeRealmPrompt +using UnityEngine; + +namespace DCL.ChangeRealmPrompt { public partial class ChangeRealmPromptController { @@ -7,10 +9,14 @@ public struct Params public string Message { get; } public string Realm { get; } - public Params(string message, string realm) + /// Optional target parcel to land on after the realm switch. + public Vector2Int? Position { get; } + + public Params(string message, string realm, Vector2Int? position = null) { Message = message; Realm = realm; + Position = position; } } } diff --git a/Explorer/Assets/DCL/ChangeRealmPrompt/ChangeRealmPromptController.cs b/Explorer/Assets/DCL/ChangeRealmPrompt/ChangeRealmPromptController.cs index 28ed665798e..1eb81ef5c17 100644 --- a/Explorer/Assets/DCL/ChangeRealmPrompt/ChangeRealmPromptController.cs +++ b/Explorer/Assets/DCL/ChangeRealmPrompt/ChangeRealmPromptController.cs @@ -3,6 +3,7 @@ using MVC; using System; using System.Threading; +using UnityEngine; namespace DCL.ChangeRealmPrompt { @@ -10,14 +11,16 @@ public partial class ChangeRealmPromptController : ControllerBase CanvasOrdering.SortingLayer.POPUP; + private const string DEFAULT_CONFIRMATION_MESSAGE = "Are you sure you want to enter this World?"; + private readonly ICursor cursor; - private readonly Action changeRealmCallback; - private Action resultCallback; + private readonly Action changeRealmCallback; + private Action? resultCallback; public ChangeRealmPromptController( ViewFactoryMethod viewFactory, ICursor cursor, - Action changeRealmCallback) : base(viewFactory) + Action changeRealmCallback) : base(viewFactory) { this.cursor = cursor; this.changeRealmCallback = changeRealmCallback; @@ -25,9 +28,14 @@ public ChangeRealmPromptController( protected override void OnViewInstantiated() { - viewInstance.CloseButton.onClick.AddListener(Dismiss); + viewInstance!.CloseButton.onClick.AddListener(Dismiss); viewInstance.CancelButton.onClick.AddListener(Dismiss); viewInstance.ContinueButton.onClick.AddListener(Approve); + + // Message and realm are attacker-controllable (scene changeRealm / deep link). Disable rich-text + // parsing so neither can inject TMP markup into this consent prompt (SEC-003; same class as SEC-034/050). + viewInstance.MessageText.richText = false; + viewInstance.RealmText.richText = false; } protected override void OnViewShow() @@ -38,32 +46,50 @@ protected override void OnViewShow() if (result != ChangeRealmPromptResultType.Approved) return; - changeRealmCallback?.Invoke(inputData.Realm); + changeRealmCallback.Invoke(inputData.Realm, inputData.Position); }); } - protected override UniTask WaitForCloseIntentAsync(CancellationToken ct) - { - if (string.IsNullOrEmpty(inputData.Message)) - return UniTask.CompletedTask; - - return UniTask.WhenAny( - viewInstance.CloseButton.OnClickAsync(ct), + protected override UniTask WaitForCloseIntentAsync(CancellationToken ct) => + UniTask.WhenAny( + viewInstance!.CloseButton.OnClickAsync(ct), viewInstance.CancelButton.OnClickAsync(ct), viewInstance.ContinueButton.OnClickAsync(ct)); - } private void RequestChangeRealm(string message, string realm, Action result) { resultCallback = result; + viewInstance!.MessageText.text = string.IsNullOrEmpty(message) ? DEFAULT_CONFIRMATION_MESSAGE : message; + viewInstance.RealmText.text = DestinationHostFor(realm); + } - if (string.IsNullOrEmpty(message)) - resultCallback?.Invoke(ChangeRealmPromptResultType.Approved); - else + /// + /// The destination shown to the user: for a URL realm the authority (host[:port]) with any misleading + /// userinfo (https://trusted@evil.com) and path/query stripped, so the true host is shown — not a + /// spoof; a world name or realm alias is shown unchanged. This — with rich-text disabled in + /// — is what the user actually consents to. + /// + internal static string DestinationHostFor(string realm) + { + int schemeIdx = realm.IndexOf("://", StringComparison.Ordinal); + + if (schemeIdx < 0) + return realm; + + int start = schemeIdx + 3; + int end = start; + + while (end < realm.Length && realm[end] != '/' && realm[end] != '?' && realm[end] != '#') { - viewInstance.MessageText.text = message; - viewInstance.RealmText.text = realm; + // Skip past any userinfo (e.g. https://decentraland.org@evil.com) so the real host after the + // last '@' is displayed, not the trusted-looking prefix (consent-prompt spoofing, SEC-004). + if (realm[end] == '@') + start = end + 1; + + end++; } + + return end > start ? realm.Substring(start, end - start) : realm; } private void Dismiss() => diff --git a/Explorer/Assets/DCL/Chat/Commands/ChatEnvironmentValidator.cs b/Explorer/Assets/DCL/Chat/Commands/ChatEnvironmentValidator.cs index b3db2a61d9e..323441ec952 100644 --- a/Explorer/Assets/DCL/Chat/Commands/ChatEnvironmentValidator.cs +++ b/Explorer/Assets/DCL/Chat/Commands/ChatEnvironmentValidator.cs @@ -1,21 +1,19 @@ using DCL.Multiplayer.Connections.DecentralandUrls; using DCL.Utility.Types; -using UnityEngine.Networking; +using System; namespace DCL.Chat.Commands { public class ChatEnvironmentValidator { - private readonly DecentralandEnvironment dclEnvironment; + private const string ORG_HOST_SUFFIX = "decentraland.org"; + private const string ZONE_HOST_SUFFIX = "decentraland.zone"; - private readonly string zoneDescription; - private readonly string orgDescription; + private readonly DecentralandEnvironment dclEnvironment; public ChatEnvironmentValidator(DecentralandEnvironment dclEnvironment) { this.dclEnvironment = dclEnvironment; - zoneDescription = DecentralandEnvironment.Zone.ToString().ToLower(); - orgDescription = DecentralandEnvironment.Org.ToString().ToLower(); } public Result ValidateTeleport(string realmToTeleportTo) @@ -26,19 +24,63 @@ public Result ValidateTeleport(string realmToTeleportTo) return Result.ErrorResult( "🔴 Error. You cannot change realms in the Today environment. Please restart DCL with the desired environment"); case DecentralandEnvironment.Zone: - if (realmToTeleportTo.Contains(zoneDescription)) - return Result.SuccessResult(); - return Result.ErrorResult( - "🔴 Error. You cannot teleport to other realms that are not Zone in Zone environment. Please restart DCL with the desired environment"); + return HostHasSuffix(realmToTeleportTo, ZONE_HOST_SUFFIX) + ? Result.SuccessResult() + : Result.ErrorResult( + "🔴 Error. You cannot teleport to other realms that are not Zone in Zone environment. Please restart DCL with the desired environment"); case DecentralandEnvironment.Org: - if (realmToTeleportTo.Contains(orgDescription)) - return Result.SuccessResult(); - - return Result.ErrorResult( - "🔴 Error. You cannot teleport to other realms that are not Org or World in Org environment. Please restart DCL with the desired environment"); + return HostHasSuffix(realmToTeleportTo, ORG_HOST_SUFFIX) + ? Result.SuccessResult() + : Result.ErrorResult( + "🔴 Error. You cannot teleport to other realms that are not Org or World in Org environment. Please restart DCL with the desired environment"); } return Result.SuccessResult(); } + + /// + /// True if the URL's host equals or ends with "." + suffix at a domain + /// boundary. Reads the host out of the string via a span (no allocation); a URL + /// carrying userinfo ('@') is rejected so it cannot spoof the host — the '@' is checked before any ':' + /// so a colon-before-'@' form (https://decentraland.org:1234@evil.com) cannot slip past the guard. + /// + private static bool HostHasSuffix(string url, string suffix) + { + int schemeIdx = url.IndexOf("://", StringComparison.Ordinal); + + if (schemeIdx < 0) + return false; + + int hostStart = schemeIdx + 3; + int authorityEnd = hostStart; + + while (authorityEnd < url.Length) + { + char c = url[authorityEnd]; + + if (c == '/' || c == '?' || c == '#') + break; + + // userinfo is not expected in a realm URL; reject rather than risk host-confusion. Checked BEFORE + // any ':' so https://decentraland.org:1234@evil.com (port before userinfo) can't slip past. + if (c == '@') + return false; + + authorityEnd++; + } + + // Strip the port (if any) from the authority to get the bare host. + ReadOnlySpan authority = url.AsSpan(hostStart, authorityEnd - hostStart); + int portSep = authority.IndexOf(':'); + ReadOnlySpan host = portSep >= 0 ? authority.Slice(0, portSep) : authority; + + if (host.Equals(suffix, StringComparison.OrdinalIgnoreCase)) + return true; + + // subdomain boundary match: host ends with ".{suffix}" + return host.Length > suffix.Length + && host[host.Length - suffix.Length - 1] == '.' + && host.EndsWith(suffix, StringComparison.OrdinalIgnoreCase); + } } } diff --git a/Explorer/Assets/DCL/ExternalUrlPrompt/ExternalUrlPromptController.Params.cs b/Explorer/Assets/DCL/ExternalUrlPrompt/ExternalUrlPromptController.Params.cs index ef8af24a015..46a4baf0257 100644 --- a/Explorer/Assets/DCL/ExternalUrlPrompt/ExternalUrlPromptController.Params.cs +++ b/Explorer/Assets/DCL/ExternalUrlPrompt/ExternalUrlPromptController.Params.cs @@ -1,4 +1,5 @@ -using System; +using DCL.Browser; +using System; namespace DCL.ExternalUrlPrompt { @@ -6,11 +7,14 @@ public partial class ExternalUrlPromptController { public struct Params { - public Uri Uri { get; } + /// Null when url is not an absolute http/https URL — callers must null-check. + public Uri? Uri { get; } public Params(string url) { - Uri = Uri.TryCreate(url, UriKind.Absolute, out Uri uri) ? uri : null; + Uri = ExternalUrlPolicy.IsWebScheme(url) && Uri.TryCreate(url, UriKind.Absolute, out Uri uri) + ? uri + : null; } } } diff --git a/Explorer/Assets/DCL/ExternalUrlPrompt/ExternalUrlPromptController.cs b/Explorer/Assets/DCL/ExternalUrlPrompt/ExternalUrlPromptController.cs index c2b9f1304b2..3a4e59c9b1a 100644 --- a/Explorer/Assets/DCL/ExternalUrlPrompt/ExternalUrlPromptController.cs +++ b/Explorer/Assets/DCL/ExternalUrlPrompt/ExternalUrlPromptController.cs @@ -14,8 +14,8 @@ public partial class ExternalUrlPromptController : ControllerBase trustedDomains = new (); - private Action resultCallback; + private readonly HashSet trustedKeys = new (); + private Action? resultCallback; public ExternalUrlPromptController( ViewFactoryMethod viewFactory, @@ -28,7 +28,7 @@ public ExternalUrlPromptController( protected override void OnViewInstantiated() { - viewInstance.CloseButton.onClick.AddListener(Dismiss); + viewInstance!.CloseButton.onClick.AddListener(Dismiss); viewInstance.CancelButton.onClick.AddListener(Dismiss); viewInstance.ContinueButton.onClick.AddListener(Approve); } @@ -38,25 +38,28 @@ protected override void OnViewShow() if (inputData.Uri == null) return; - if (trustedDomains.Contains(inputData.Uri.Host)) + Uri uri = inputData.Uri; + + if (ExternalUrlPolicy.TryGetTrustKey(uri, out string trustKey) && trustedKeys.Contains(trustKey)) { - webBrowser.OpenUrlMainThreadOnly(inputData.Uri.OriginalString); - viewInstance.CloseButton.OnClickAsync(CancellationToken.None).Forget(); + webBrowser.OpenUrlMainThreadOnly(uri.OriginalString); + viewInstance!.CloseButton.OnClickAsync(CancellationToken.None).Forget(); return; } cursor.Unlock(); - RequestOpenUrl(inputData.Uri, result => + RequestOpenUrl(uri, result => { switch (result) { case ExternalUrlPromptResultType.ApprovedTrusted: - if (!trustedDomains.Contains(inputData.Uri.Host)) - trustedDomains.Add(inputData.Uri.Host); - webBrowser.OpenUrlMainThreadOnly(inputData.Uri.OriginalString); + // Only cache when a real (scheme, host) key exists — empty-host URIs are never trusted (SEC-008). + if (ExternalUrlPolicy.TryGetTrustKey(uri, out string key)) + trustedKeys.Add(key); + webBrowser.OpenUrlMainThreadOnly(uri.OriginalString); break; case ExternalUrlPromptResultType.Approved: - webBrowser.OpenUrlMainThreadOnly(inputData.Uri.OriginalString); + webBrowser.OpenUrlMainThreadOnly(uri.OriginalString); break; } }); @@ -64,7 +67,9 @@ protected override void OnViewShow() protected override UniTask WaitForCloseIntentAsync(CancellationToken ct) { - if (inputData.Uri != null && trustedDomains.Contains(inputData.Uri.Host)) + if (inputData.Uri != null + && ExternalUrlPolicy.TryGetTrustKey(inputData.Uri, out string trustKey) + && trustedKeys.Contains(trustKey)) return UniTask.CompletedTask; return UniTask.WhenAny( @@ -75,13 +80,13 @@ protected override UniTask WaitForCloseIntentAsync(CancellationToken ct) public override void Dispose() { - trustedDomains.Clear(); + trustedKeys.Clear(); } private void RequestOpenUrl(Uri uri, Action result) { resultCallback = result; - viewInstance.DomainText.text = uri.Host; + viewInstance!.DomainText.text = uri.Host; viewInstance.UrlText.text = uri.OriginalString; viewInstance.TrustToggle.isOn = false; } @@ -90,6 +95,6 @@ private void Dismiss() => resultCallback?.Invoke(ExternalUrlPromptResultType.Canceled); private void Approve() => - resultCallback?.Invoke(viewInstance.TrustToggle.isOn ? ExternalUrlPromptResultType.ApprovedTrusted : ExternalUrlPromptResultType.Approved); + resultCallback?.Invoke(viewInstance!.TrustToggle.isOn ? ExternalUrlPromptResultType.ApprovedTrusted : ExternalUrlPromptResultType.Approved); } } diff --git a/Explorer/Assets/DCL/Infrastructure/Global/AppArgs/AppArgsFlags.cs b/Explorer/Assets/DCL/Infrastructure/Global/AppArgs/AppArgsFlags.cs index 3fadae5145b..d332d9bdc23 100644 --- a/Explorer/Assets/DCL/Infrastructure/Global/AppArgs/AppArgsFlags.cs +++ b/Explorer/Assets/DCL/Infrastructure/Global/AppArgs/AppArgsFlags.cs @@ -64,7 +64,6 @@ public static class AppArgsFlags public const string SIMULATE_MEMORY = "simulateMemory"; public const string LAUNCH_CDP_MONITOR_ON_START = "launch-cdp-monitor-on-start"; - public const string CREATOR_HUB_BIN_PATH = "creator-hub-bin-path"; public const string USE_LOG_MATRIX = "use-log-matrix"; public const string GRAPHICS = "graphics"; diff --git a/Explorer/Assets/DCL/Infrastructure/Global/AppArgs/ApplicationParametersParser.cs b/Explorer/Assets/DCL/Infrastructure/Global/AppArgs/ApplicationParametersParser.cs index a2715257e4f..ae6348775b7 100644 --- a/Explorer/Assets/DCL/Infrastructure/Global/AppArgs/ApplicationParametersParser.cs +++ b/Explorer/Assets/DCL/Infrastructure/Global/AppArgs/ApplicationParametersParser.cs @@ -101,11 +101,16 @@ public static Dictionary ProcessDeepLinkParameters(string deepLi var uri = new Uri(deepLinkString); NameValueCollection uriQuery = HttpUtility.ParseQueryString(uri.Query); + var droppedKeys = new List(); + + // Tier 1: always-permitted (base) navigation/login params. foreach (string uriQueryKey in uriQuery.AllKeys) { // if the deep link is not constructed correctly (AKA 'decentraland://?&blabla=blabla') a 'null' parameter can be detected... if (uriQueryKey == null) continue; - output[uriQueryKey] = uriQuery.Get(uriQueryKey); + + if (DeepLinkAllowlist.IsPermitted(uriQueryKey)) + output[uriQueryKey] = uriQuery.Get(uriQueryKey); } if (output.TryGetValue(AppArgsFlags.REALM, out string? realmParamValue)) @@ -120,9 +125,26 @@ public static Dictionary ProcessDeepLinkParameters(string deepLi output[AppArgsFlags.REALM] = realmParamValue; } - // Patch for WinOS sometimes affecting the 'skip-auth-screen' parameter in deep links putting a '/' at the end - if (output.TryGetValue(AppArgsFlags.SKIP_AUTH_SCREEN, out string? value) && value?.EndsWith('/') == true) - output[AppArgsFlags.SKIP_AUTH_SCREEN] = value[..^1]; + // Tier 2 (SEC-019/020): the local-development params Creator Hub / sdk-commands attach to preview deep + // links (local-scene, dclenv, hub, skip-auth-screen, landscape-terrain-enabled, multi-instance) are + // permitted only when the target realm is loopback — a remote-realm deep link from a web page cannot + // enable them. Everything not in either tier is dropped. + bool realmIsLoopback = output.TryGetValue(AppArgsFlags.REALM, out string? loopbackRealm) + && Uri.TryCreate(loopbackRealm, UriKind.Absolute, out Uri? loopbackRealmUri) + && loopbackRealmUri.IsLoopback; + + foreach (string uriQueryKey in uriQuery.AllKeys) + { + if (uriQueryKey == null || output.ContainsKey(uriQueryKey)) continue; + + if (realmIsLoopback && DeepLinkAllowlist.IsPermittedForLoopbackRealm(uriQueryKey)) + output[uriQueryKey] = uriQuery.Get(uriQueryKey); + else + droppedKeys.Add(uriQueryKey); + } + + if (droppedKeys.Count > 0) + ReportHub.LogWarning(ReportCategory.ALWAYS, $"Dropped {droppedKeys.Count} non-allowlisted deep-link param(s): {string.Join(", ", droppedKeys)}"); return output; } diff --git a/Explorer/Assets/DCL/Infrastructure/Global/AppArgs/DeepLinkAllowlist.cs b/Explorer/Assets/DCL/Infrastructure/Global/AppArgs/DeepLinkAllowlist.cs new file mode 100644 index 00000000000..e18a351202f --- /dev/null +++ b/Explorer/Assets/DCL/Infrastructure/Global/AppArgs/DeepLinkAllowlist.cs @@ -0,0 +1,99 @@ +using System.Collections.Generic; + +namespace Global.AppArgs +{ + /// + /// Deny-by-default allowlist of query params a decentraland:// deep link may inject into app-args. + /// Shared by the cold-start argv path and the runtime bridge path (both funnel through + /// ). + /// + /// A deep link is fully attacker-controllable — anyone can craft one and get a victim to open it — so + /// params fall into three tiers: + /// + /// + /// + /// Always permitted — benign navigation / share / login intents whose worst case is already + /// gated elsewhere (a consent prompt, a matching login token, or a plain coordinate): realm, + /// position, community, signin, authRequestId, force-open-backpack. + /// + /// + /// Permitted only for a loopback realm — the local-development params Creator Hub and the + /// SDK (sdk-commands) attach to their preview deep links: local-scene, dclenv, hub, + /// skip-auth-screen, landscape-terrain-enabled, multi-instance. They are gated on + /// Uri.IsLoopback of the target realm (127.0.0.1 / localhost / [::1]) so a remote-realm deep + /// link from a web page can never enable them, while a legitimate local-dev launch (which always + /// targets loopback) works. Each is individually low-harm — an analytics tag, a cosmetic toggle, an + /// instance count, an env enum, or a screen skip that still forces auth when no valid identity is + /// cached — and the loopback gate confines them to the dev context. + /// + /// + /// Never permitted — everything else, in particular params that launch code + /// (creator-hub-bin-path, launch-cdp-monitor-on-start — SEC-005); point the client at + /// attacker infrastructure (comms-adapter, gatekeeper-url, friends-api-url — + /// SEC-052, feature-flags-url/-hostname, optimized-assets-url, + /// lsd-remote-ab-server/-world, pulse); bypass a version/specs screen + /// (skip-version-check, skip-minimum-specs-screen); or enable other dev/test modes + /// (debug, scene-console, autopilot, alttester, simulate*). + /// + /// + /// Both permitted sets are a product decision (SEC-019/020 "Design affected") — changing them requires sign-off. + /// + public static class DeepLinkAllowlist + { + private static readonly HashSet PERMITTED_KEYS = new() + { + // Which world/realm to enter. Attacker-controllable, but never applied silently: the switch is routed + // through the ChangeRealm consent prompt (SEC-004) and a host-suffix environment check. + AppArgsFlags.REALM, + + // Target parcel "x,y" to land on. A plain coordinate — no security impact. + AppArgsFlags.POSITION, + + // Community id shown as a "view this community" notification; opening the card is a user action. Benign UI. + AppArgsFlags.COMMUNITY, + + // Login flow: opaque identity id from the auth website's signin link. Consumed only while a local login + // is actively awaiting one AND AUTH_REQUEST_ID matches the request that minted it (see DeepLinkHandle). + AppArgsFlags.SIGNIN, + + // Login flow: binds a signin link to the login that requested it; inert without a matching pending login. + AppArgsFlags.AUTH_REQUEST_ID, + + // Opens the user's own backpack panel on landing (shipped deep-link feature). Benign in-client navigation. + AppArgsFlags.FORCE_OPEN_BACKPACK, + }; + + // Local-development params Creator Hub / sdk-commands attach to preview deep links. Permitted ONLY when the + // target realm is loopback (see ApplicationParametersParser.ProcessDeepLinkParameters) — a remote-realm deep + // link can never enable them. The SEC-005 exec params (creator-hub-bin-path, launch-cdp-monitor-on-start) + // are deliberately NOT here; they stay dropped for every realm. + private static readonly HashSet LOOPBACK_REALM_PERMITTED_KEYS = new() + { + // Enables local-scene-development mode (opens an LSD websocket to the realm). Only meaningful against a + // local server; loopback-gated so an attacker can't point LSD at a remote realm (SEC-020). + AppArgsFlags.LOCAL_SCENE, + + // Target environment (org/zone/today). A DCL-owned enum, not a URL — cannot point at attacker infra. + AppArgsFlags.ENVIRONMENT, + + // Marks the session as launched from the Creator Hub (analytics trait only — no capability unlock). + AppArgsFlags.DCL_EDITOR, + + // Skips the login screen. Cannot bypass authentication: auth is still forced when no valid identity is + // cached (RealUserInAppInitializationFlow). A convenience for the local-dev loop. + AppArgsFlags.SKIP_AUTH_SCREEN, + + // Toggles landscape terrain rendering. Cosmetic. + AppArgsFlags.LANDSCAPE_TERRAIN_ENABLED, + + // Allows multiple client instances (local multi-instance dev workflow). + AppArgsFlags.MULTIPLE_RUNNING_INSTANCES, + }; + + public static bool IsPermitted(string key) => + PERMITTED_KEYS.Contains(key); + + public static bool IsPermittedForLoopbackRealm(string key) => + LOOPBACK_REALM_PERMITTED_KEYS.Contains(key); + } +} diff --git a/Explorer/Assets/DCL/Infrastructure/Global/AppArgs/DeepLinkAllowlist.cs.meta b/Explorer/Assets/DCL/Infrastructure/Global/AppArgs/DeepLinkAllowlist.cs.meta new file mode 100644 index 00000000000..1a699565aa2 --- /dev/null +++ b/Explorer/Assets/DCL/Infrastructure/Global/AppArgs/DeepLinkAllowlist.cs.meta @@ -0,0 +1,2 @@ +fileFormatVersion: 2 +guid: a578a6538c5cdc84f8229ac2ead3828b \ No newline at end of file diff --git a/Explorer/Assets/DCL/Infrastructure/Global/AppArgs/Tests/AppArgsTests.cs b/Explorer/Assets/DCL/Infrastructure/Global/AppArgs/Tests/AppArgsTests.cs index baab2acbe97..8bd937184a9 100644 --- a/Explorer/Assets/DCL/Infrastructure/Global/AppArgs/Tests/AppArgsTests.cs +++ b/Explorer/Assets/DCL/Infrastructure/Global/AppArgs/Tests/AppArgsTests.cs @@ -48,5 +48,89 @@ public void DebugArgContainsTest() IAppArgs args = new ApplicationParametersParser(false, "--debug"); Assert.True(args.HasDebugFlag(false), $"flags in args: {string.Join(", ", args.Flags())}"); } + + [Test] + public void DeepLinkDropsInternalFlags() + { + Dictionary output = ApplicationParametersParser.ProcessDeepLinkParameters( + "decentraland://?creator-hub-bin-path=%5C%5Cattacker%5Cshare%5Cp.exe&launch-cdp-monitor-on-start&local-scene=true&comms-adapter=x&skip-auth-screen=true"); + + Assert.IsFalse(output.ContainsKey("creator-hub-bin-path"), "creator-hub-bin-path must be dropped from deep links (not an app-arg; the Creator Hub path is resolved at runtime)"); + Assert.IsFalse(output.ContainsKey(AppArgsFlags.LAUNCH_CDP_MONITOR_ON_START), "launch-cdp-monitor-on-start must be dropped"); + Assert.IsFalse(output.ContainsKey(AppArgsFlags.LOCAL_SCENE), "local-scene must be dropped"); + Assert.IsFalse(output.ContainsKey(AppArgsFlags.COMMS_ADAPTER), "comms-adapter must be dropped"); + Assert.IsFalse(output.ContainsKey(AppArgsFlags.SKIP_AUTH_SCREEN), "skip-auth-screen must be dropped"); + } + + [Test] + public void DeepLinkKeepsAllowlistedParams() + { + Dictionary output = ApplicationParametersParser.ProcessDeepLinkParameters( + "decentraland://?realm=https://peer.decentraland.org&position=10,20&community=abc&signin=id1&authRequestId=req1&force-open-backpack=true"); + + Assert.AreEqual("https://peer.decentraland.org", output.GetValueOrDefault(AppArgsFlags.REALM)); + Assert.AreEqual("10,20", output.GetValueOrDefault(AppArgsFlags.POSITION)); + Assert.AreEqual("abc", output.GetValueOrDefault(AppArgsFlags.COMMUNITY)); + Assert.AreEqual("id1", output.GetValueOrDefault(AppArgsFlags.SIGNIN)); + Assert.AreEqual("req1", output.GetValueOrDefault(AppArgsFlags.AUTH_REQUEST_ID)); + Assert.IsTrue(output.ContainsKey(AppArgsFlags.FORCE_OPEN_BACKPACK), "force-open-backpack must survive (shipped feature #9398)"); + } + + [Test] + public void DeepLinkKeepsLocalSceneForLoopbackRealm() + { + Dictionary output = ApplicationParametersParser.ProcessDeepLinkParameters( + "decentraland://?realm=http://127.0.0.1:8000&position=100,100&local-scene=true"); + + Assert.AreEqual("true", output.GetValueOrDefault(AppArgsFlags.LOCAL_SCENE), "local-scene must survive for a loopback (local dev) realm"); + } + + [Test] + public void DeepLinkDropsLocalSceneForRemoteRealm() + { + Dictionary output = ApplicationParametersParser.ProcessDeepLinkParameters( + "decentraland://?realm=https://evil.example&local-scene=true"); + + Assert.IsFalse(output.ContainsKey(AppArgsFlags.LOCAL_SCENE), "local-scene must be dropped for a non-loopback (remote) realm (SEC-020)"); + } + + [Test] + public void DeepLinkKeepsSdkAndCreatorHubDevParamsForLoopbackRealm() + { + Dictionary output = ApplicationParametersParser.ProcessDeepLinkParameters( + "decentraland://?realm=http://127.0.0.1:8000&position=10,20&local-scene=true&dclenv=zone&hub=true&skip-auth-screen=true&landscape-terrain-enabled=true&multi-instance=true"); + + Assert.AreEqual("true", output.GetValueOrDefault(AppArgsFlags.LOCAL_SCENE), "local-scene"); + Assert.AreEqual("zone", output.GetValueOrDefault(AppArgsFlags.ENVIRONMENT), "dclenv"); + Assert.AreEqual("true", output.GetValueOrDefault(AppArgsFlags.DCL_EDITOR), "hub"); + Assert.AreEqual("true", output.GetValueOrDefault(AppArgsFlags.SKIP_AUTH_SCREEN), "skip-auth-screen"); + Assert.AreEqual("true", output.GetValueOrDefault(AppArgsFlags.LANDSCAPE_TERRAIN_ENABLED), "landscape-terrain-enabled"); + Assert.AreEqual("true", output.GetValueOrDefault(AppArgsFlags.MULTIPLE_RUNNING_INSTANCES), "multi-instance"); + } + + [Test] + public void DeepLinkDropsSdkAndCreatorHubDevParamsForRemoteRealm() + { + Dictionary output = ApplicationParametersParser.ProcessDeepLinkParameters( + "decentraland://?realm=https://peer.decentraland.org&local-scene=true&dclenv=zone&hub=true&skip-auth-screen=true&landscape-terrain-enabled=true&multi-instance=true"); + + Assert.IsFalse(output.ContainsKey(AppArgsFlags.LOCAL_SCENE), "local-scene must be dropped for a remote realm"); + Assert.IsFalse(output.ContainsKey(AppArgsFlags.ENVIRONMENT), "dclenv must be dropped for a remote realm"); + Assert.IsFalse(output.ContainsKey(AppArgsFlags.DCL_EDITOR), "hub must be dropped for a remote realm"); + Assert.IsFalse(output.ContainsKey(AppArgsFlags.SKIP_AUTH_SCREEN), "skip-auth-screen must be dropped for a remote realm"); + Assert.IsFalse(output.ContainsKey(AppArgsFlags.LANDSCAPE_TERRAIN_ENABLED), "landscape-terrain-enabled must be dropped for a remote realm"); + Assert.IsFalse(output.ContainsKey(AppArgsFlags.MULTIPLE_RUNNING_INSTANCES), "multi-instance must be dropped for a remote realm"); + } + + [Test] + public void DeepLinkDropsExecAndInfraParamsEvenForLoopbackRealm() + { + Dictionary output = ApplicationParametersParser.ProcessDeepLinkParameters( + "decentraland://?realm=http://127.0.0.1:8000&creator-hub-bin-path=x&launch-cdp-monitor-on-start=true&comms-adapter=y"); + + Assert.IsFalse(output.ContainsKey("creator-hub-bin-path"), "creator-hub-bin-path must never be permitted (SEC-005), even for a loopback realm"); + Assert.IsFalse(output.ContainsKey(AppArgsFlags.LAUNCH_CDP_MONITOR_ON_START), "launch-cdp-monitor-on-start must never be permitted, even for a loopback realm"); + Assert.IsFalse(output.ContainsKey(AppArgsFlags.COMMS_ADAPTER), "comms-adapter must never be permitted, even for a loopback realm"); + } } } diff --git a/Explorer/Assets/DCL/Infrastructure/Global/Dynamic/BootstrapContainer.cs b/Explorer/Assets/DCL/Infrastructure/Global/Dynamic/BootstrapContainer.cs index b830f820ad2..3172fe1a5e4 100644 --- a/Explorer/Assets/DCL/Infrastructure/Global/Dynamic/BootstrapContainer.cs +++ b/Explorer/Assets/DCL/Infrastructure/Global/Dynamic/BootstrapContainer.cs @@ -128,7 +128,7 @@ await bootstrapContainer.InitializeContainerAsync chatContainer.ChatMessagesBus.SendWithUtcNowTimestamp(ChatChannel.NEARBY_CHANNEL, $"/{ChatCommandsUtils.COMMAND_GOTO} {realmUrl}", ChatMessageOrigin.RESTRICTED_ACTION_API)), + (realmUrl, position) => + { + // With a target parcel: teleport with the typed position (works for URL realms too). + // Without one: keep the existing chat-command route so the switch surfaces in nearby chat. + if (position.HasValue) + // TODO: surface the teleport result (chat bus / notification) like the no-position path below, + // and plumb a real cancellation token instead of None (composition-root fire-and-forget for now). + chatContainer.ChatTeleporter.TeleportToRealmAsync(realmUrl, position.Value, CancellationToken.None).Forget(); + else + chatContainer.ChatMessagesBus.SendWithUtcNowTimestamp(ChatChannel.NEARBY_CHANNEL, $"/{ChatCommandsUtils.COMMAND_GOTO} {realmUrl}", ChatMessageOrigin.RESTRICTED_ACTION_API); + }), new NftPromptPlugin(assetsProvisioner, webBrowser, uiShellContainer.MvcManager, nftInfoAPIClient, staticContainer.ImageControllerProvider, uiShellContainer.Cursor), staticContainer.CharacterContainer.CreateGlobalPlugin(), staticContainer.QualityContainer.CreatePlugin(), diff --git a/Explorer/Assets/DCL/Infrastructure/Global/Dynamic/RealmLaunchSettings.cs b/Explorer/Assets/DCL/Infrastructure/Global/Dynamic/RealmLaunchSettings.cs index 215a6289b83..6299406d016 100644 --- a/Explorer/Assets/DCL/Infrastructure/Global/Dynamic/RealmLaunchSettings.cs +++ b/Explorer/Assets/DCL/Infrastructure/Global/Dynamic/RealmLaunchSettings.cs @@ -1,3 +1,4 @@ +using DCL.Browser; using DCL.CommunicationData.URLHelpers; using ECS.SceneLifeCycle.Realm; using Global.AppArgs; @@ -101,7 +102,7 @@ private void ParseRealmAppParameter(IAppArgs appParameters, string realmParamVal bool isLocalSceneDevelopment = appParameters.TryGetValue(AppArgsFlags.LOCAL_SCENE, out string localSceneParamValue) && ParseLocalSceneParameter(localSceneParamValue) - && IsRealmAValidUrl(realmParamValue); + && ExternalUrlPolicy.IsWebScheme(realmParamValue); if (isLocalSceneDevelopment) { @@ -171,10 +172,6 @@ private bool ParseLocalSceneParameter(string localSceneParameter) private bool IsRealmAWorld(string realmParam) => realmParam.IsEns(); - private bool IsRealmAValidUrl(string realmParam) => - Uri.TryCreate(realmParam, UriKind.Absolute, out Uri? uriResult) - && (uriResult.Scheme == Uri.UriSchemeHttp || uriResult.Scheme == Uri.UriSchemeHttps); - public void CheckStartParcelOverride(IAppArgs appArgs, FeatureFlagsConfiguration featureFlagsConfigurationCache) { // Priority 1: App argument position (highest - from command line/Creator Hub) diff --git a/Explorer/Assets/DCL/Infrastructure/Global/Dynamic/RealmUrl/RealmUrls.cs b/Explorer/Assets/DCL/Infrastructure/Global/Dynamic/RealmUrl/RealmUrls.cs index 0a0fb5a1ca7..1cac06968ac 100644 --- a/Explorer/Assets/DCL/Infrastructure/Global/Dynamic/RealmUrl/RealmUrls.cs +++ b/Explorer/Assets/DCL/Infrastructure/Global/Dynamic/RealmUrl/RealmUrls.cs @@ -1,4 +1,5 @@ using Cysharp.Threading.Tasks; +using DCL.Browser; using DCL.Multiplayer.Connections.DecentralandUrls; using DCL.Utility; using ECS.SceneLifeCycle.Realm; @@ -52,7 +53,7 @@ private async UniTask CustomRealmAsync(CancellationToken ct) { string realm = realmLaunchSettings.customRealm; - if (realm.StartsWith("http://", StringComparison.Ordinal) || realm.StartsWith("https://", StringComparison.Ordinal)) + if (ExternalUrlPolicy.IsWebScheme(realm)) return realm; return await realmNames.UrlFromNameAsync(realm, ct); diff --git a/Explorer/Assets/DCL/Infrastructure/Global/Editor/DebugSettingsDrawer.cs b/Explorer/Assets/DCL/Infrastructure/Global/Editor/DebugSettingsDrawer.cs index 2890ea23ae4..f82ef383e85 100644 --- a/Explorer/Assets/DCL/Infrastructure/Global/Editor/DebugSettingsDrawer.cs +++ b/Explorer/Assets/DCL/Infrastructure/Global/Editor/DebugSettingsDrawer.cs @@ -26,9 +26,8 @@ public override void OnGUI(Rect position, SerializedProperty property, GUIConten SerializedProperty appParameters = property.FindPropertyRelative("appParameters"); bool cdpEnabled = HasFlag(appParameters, AppArgsFlags.LAUNCH_CDP_MONITOR_ON_START); - string? currentCreatorHubPath = GetFlagValue(appParameters, AppArgsFlags.CREATOR_HUB_BIN_PATH); - string detectedCreatorHubPath = FindCreatorHubPath(); + string? detectedCreatorHubPath = FindCreatorHubPath(); bool creatorHubInstalled = !string.IsNullOrEmpty(detectedCreatorHubPath); // CDP DevTools Section @@ -46,7 +45,7 @@ public override void OnGUI(Rect position, SerializedProperty property, GUIConten else { statusStyle.normal.textColor = new Color(0.9f, 0.5f, 0.1f); - EditorGUI.LabelField(position, "Creator Hub not found at default location", statusStyle); + EditorGUI.LabelField(position, "Creator Hub not found at default location (you'll be prompted to locate it when DevTools launches)", statusStyle); } position.y += SingleLineHeight; @@ -58,62 +57,13 @@ public override void OnGUI(Rect position, SerializedProperty property, GUIConten if (EditorGUI.EndChangeCheck() && newCdpEnabled != cdpEnabled) { if (newCdpEnabled) - EnableCdpDevTools(appParameters, detectedCreatorHubPath); + EnableCdpDevTools(appParameters); else DisableCdpDevTools(appParameters); } position.y += SingleLineHeight; - // Custom Creator Hub path (only if CDP is enabled) - if (cdpEnabled) - { - EditorGUI.BeginChangeCheck(); - - Rect pathFieldRect = position; - pathFieldRect.width -= 80; - - Rect browseButtonRect = position; - browseButtonRect.x = position.xMax - 75; - browseButtonRect.width = 75; - - string displayPath = currentCreatorHubPath ?? detectedCreatorHubPath ?? "Not found"; - string newPath = EditorGUI.TextField(pathFieldRect, new GUIContent("Creator Hub Path"), displayPath); - - if (GUI.Button(browseButtonRect, "Browse")) - { - string initialDir = !string.IsNullOrEmpty(currentCreatorHubPath) ? Path.GetDirectoryName(currentCreatorHubPath) : - !string.IsNullOrEmpty(detectedCreatorHubPath) ? Path.GetDirectoryName(detectedCreatorHubPath) : Environment.GetFolderPath(Environment.SpecialFolder.ProgramFiles); - -#if UNITY_EDITOR_WIN - string selectedPath = EditorUtility.OpenFilePanel("Select Creator Hub Executable", initialDir ?? "", "exe"); -#else - string selectedPath = EditorUtility.OpenFilePanel("Select Creator Hub Executable", initialDir ?? "", ""); -#endif - - if (!string.IsNullOrEmpty(selectedPath)) - newPath = selectedPath; - } - - if (EditorGUI.EndChangeCheck() && newPath != displayPath) - SetCreatorHubPath(appParameters, newPath); - - position.y += SingleLineHeight; - - // Clear custom path button if a custom path is set - if (!string.IsNullOrEmpty(currentCreatorHubPath)) - { - Rect clearButtonRect = position; - clearButtonRect.x = position.xMax - 150; - clearButtonRect.width = 150; - - if (GUI.Button(clearButtonRect, "Use Default Path")) - RemoveFlag(appParameters, AppArgsFlags.CREATOR_HUB_BIN_PATH); - - position.y += SingleLineHeight; - } - } - // Separator position.y += 5; EditorGUI.LabelField(position, "", GUI.skin.horizontalSlider); @@ -127,30 +77,13 @@ public override void OnGUI(Rect position, SerializedProperty property, GUIConten public override float GetPropertyHeight(SerializedProperty property, GUIContent label) { - SerializedProperty appParameters = property.FindPropertyRelative("appParameters"); - bool cdpEnabled = HasFlag(appParameters, AppArgsFlags.LAUNCH_CDP_MONITOR_ON_START); - string? currentCreatorHubPath = GetFlagValue(appParameters, AppArgsFlags.CREATOR_HUB_BIN_PATH); - - // CDP section: header + status + toggle - int lineCount = 3; - - // Custom path field (if CDP enabled) - if (cdpEnabled) - { - lineCount += 1; - - // Clear button (if custom path is set) - if (!string.IsNullOrEmpty(currentCreatorHubPath)) - lineCount += 1; - } - - // Separator - lineCount += 1; + // CDP section (header + status + toggle) + separator. + const int LINE_COUNT = 4; // Default properties (including appParameters as raw array) float defaultPropertiesHeight = GetDefaultPropertiesHeight(property); - return (lineCount * SingleLineHeight) + 5 + defaultPropertiesHeight; + return (LINE_COUNT * SingleLineHeight) + 5 + defaultPropertiesHeight; } private static Rect DrawDefaultProperties(Rect position, SerializedProperty property) @@ -261,20 +194,7 @@ private static bool HasFlag(SerializedProperty appParameters, string flag) return false; } - private static string? GetFlagValue(SerializedProperty appParameters, string flag) - { - string formattedFlag = FormatFlag(flag); - - for (int i = 0; i < appParameters.arraySize; i++) - { - if (appParameters.GetArrayElementAtIndex(i).stringValue == formattedFlag && i + 1 < appParameters.arraySize) - return appParameters.GetArrayElementAtIndex(i + 1).stringValue; - } - - return null; - } - - private static void EnableCdpDevTools(SerializedProperty appParameters, string? creatorHubPath) + private static void EnableCdpDevTools(SerializedProperty appParameters) { if (!HasFlag(appParameters, AppArgsFlags.LAUNCH_CDP_MONITOR_ON_START)) { @@ -289,33 +209,12 @@ private static void EnableCdpDevTools(SerializedProperty appParameters, string? appParameters.GetArrayElementAtIndex(insertIndex + 1).stringValue = "true"; } - // Add Creator Hub path if found - if (!string.IsNullOrEmpty(creatorHubPath)) - SetCreatorHubPath(appParameters, creatorHubPath); - appParameters.serializedObject.ApplyModifiedProperties(); } private static void DisableCdpDevTools(SerializedProperty appParameters) { RemoveFlag(appParameters, AppArgsFlags.LAUNCH_CDP_MONITOR_ON_START); - RemoveFlag(appParameters, AppArgsFlags.CREATOR_HUB_BIN_PATH); - appParameters.serializedObject.ApplyModifiedProperties(); - } - - private static void SetCreatorHubPath(SerializedProperty appParameters, string path) - { - // First remove existing path if any - RemoveFlagWithValue(appParameters, AppArgsFlags.CREATOR_HUB_BIN_PATH); - - // Add --flag and value - int insertIndex = appParameters.arraySize; - appParameters.InsertArrayElementAtIndex(insertIndex); - appParameters.GetArrayElementAtIndex(insertIndex).stringValue = FormatFlag(AppArgsFlags.CREATOR_HUB_BIN_PATH); - - appParameters.InsertArrayElementAtIndex(insertIndex + 1); - appParameters.GetArrayElementAtIndex(insertIndex + 1).stringValue = path; - appParameters.serializedObject.ApplyModifiedProperties(); } @@ -344,31 +243,5 @@ private static void RemoveFlag(SerializedProperty appParameters, string flag) } } } - - private static void RemoveFlagWithValue(SerializedProperty appParameters, string flag) - { - string formattedFlag = FormatFlag(flag); - - for (int i = appParameters.arraySize - 1; i >= 0; i--) - { - if (appParameters.GetArrayElementAtIndex(i).stringValue == formattedFlag) - { - // Remove the value first (if exists) - if (i + 1 < appParameters.arraySize) - { - string nextValue = appParameters.GetArrayElementAtIndex(i + 1).stringValue; - - // Only remove if it's not another flag - if (!nextValue.StartsWith("--")) - appParameters.DeleteArrayElementAtIndex(i + 1); - } - - // Then remove the flag - appParameters.DeleteArrayElementAtIndex(i); - appParameters.serializedObject.ApplyModifiedProperties(); - return; - } - } - } } } diff --git a/Explorer/Assets/DCL/NetworkDefinitions/Browser/ExternalUrlPolicy.cs b/Explorer/Assets/DCL/NetworkDefinitions/Browser/ExternalUrlPolicy.cs new file mode 100644 index 00000000000..b757fe95cca --- /dev/null +++ b/Explorer/Assets/DCL/NetworkDefinitions/Browser/ExternalUrlPolicy.cs @@ -0,0 +1,35 @@ +using System; + +namespace DCL.Browser +{ + /// + /// Central policy for opening external URLs: only http/https reach the OS handler. Custom schemes + /// (decentraland://, smb://, file://, mailto:, app-protocol handlers) are refused so a scene/profile + /// link can never re-invoke the launcher (SEC-028) or leak NTLM/local files (SEC-008). + /// + public static class ExternalUrlPolicy + { + /// + /// True if the URL is an absolute http/https web URL. Validates the scheme by prefix and does NOT + /// allocate a (callers that already hold a parsed Uri use it directly instead). + /// This is the single http/https predicate shared across the client — external-URL sink, realm + /// resolution, and realm launch settings all route through it. + /// + public static bool IsWebScheme(string? url) => + !string.IsNullOrEmpty(url) + && (url.StartsWith("https://", StringComparison.OrdinalIgnoreCase) + || url.StartsWith("http://", StringComparison.OrdinalIgnoreCase)); + + /// Trust-cache key on (scheme, host). Returns false for authority-less URIs (empty host), + /// which must never be cached (SEC-008 empty-host bypass). + public static bool TryGetTrustKey(Uri uri, out string key) + { + key = string.Empty; + if (string.IsNullOrEmpty(uri.Host)) + return false; + + key = $"{uri.Scheme}|{uri.Host}"; + return true; + } + } +} diff --git a/Explorer/Assets/DCL/NetworkDefinitions/Browser/ExternalUrlPolicy.cs.meta b/Explorer/Assets/DCL/NetworkDefinitions/Browser/ExternalUrlPolicy.cs.meta new file mode 100644 index 00000000000..c456ccdc392 --- /dev/null +++ b/Explorer/Assets/DCL/NetworkDefinitions/Browser/ExternalUrlPolicy.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 257d886dfa7b423297aeb6210b9ba95e +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Explorer/Assets/DCL/NetworkDefinitions/Browser/Tests/ExternalUrlPolicyShould.cs b/Explorer/Assets/DCL/NetworkDefinitions/Browser/Tests/ExternalUrlPolicyShould.cs new file mode 100644 index 00000000000..204696631e2 --- /dev/null +++ b/Explorer/Assets/DCL/NetworkDefinitions/Browser/Tests/ExternalUrlPolicyShould.cs @@ -0,0 +1,33 @@ +using NUnit.Framework; +using System; + +namespace DCL.Browser +{ + public class ExternalUrlPolicyShould + { + [TestCase("https://decentraland.org", true)] + [TestCase("http://example.com", true)] + [TestCase("smb://attacker/share", false)] + [TestCase("file:///etc/passwd", false)] + [TestCase("decentraland://?creator-hub-bin-path=x", false)] + [TestCase("mailto:a@b.com", false)] + [TestCase("steam://run/1", false)] + [TestCase("not a url", false)] + public void ClassifyScheme(string url, bool expected) => + Assert.AreEqual(expected, ExternalUrlPolicy.IsWebScheme(url)); + + [Test] + public void RefuseEmptyHostTrustKey() + { + var fileUri = new Uri("file:///x"); + Assert.IsFalse(ExternalUrlPolicy.TryGetTrustKey(fileUri, out _), "empty-host URIs must never be cacheable"); + } + + [Test] + public void TrustKeyIsSchemeAndHost() + { + Assert.IsTrue(ExternalUrlPolicy.TryGetTrustKey(new Uri("https://decentraland.org/x"), out string key)); + Assert.AreEqual("https|decentraland.org", key); + } + } +} diff --git a/Explorer/Assets/DCL/NetworkDefinitions/Browser/Tests/ExternalUrlPolicyShould.cs.meta b/Explorer/Assets/DCL/NetworkDefinitions/Browser/Tests/ExternalUrlPolicyShould.cs.meta new file mode 100644 index 00000000000..bfdc6c0adc8 --- /dev/null +++ b/Explorer/Assets/DCL/NetworkDefinitions/Browser/Tests/ExternalUrlPolicyShould.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 010396ed4c3e4c40bcf607597fafae41 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Explorer/Assets/DCL/NetworkDefinitions/Browser/UnityAppWebBrowser.cs b/Explorer/Assets/DCL/NetworkDefinitions/Browser/UnityAppWebBrowser.cs index 73266de52bf..a0924961ef3 100644 --- a/Explorer/Assets/DCL/NetworkDefinitions/Browser/UnityAppWebBrowser.cs +++ b/Explorer/Assets/DCL/NetworkDefinitions/Browser/UnityAppWebBrowser.cs @@ -1,3 +1,4 @@ +using DCL.Diagnostics; using DCL.Multiplayer.Connections.DecentralandUrls; using System; using UnityEngine; @@ -16,6 +17,12 @@ public UnityAppWebBrowser(IDecentralandUrlsSource decentralandUrlsSource) public virtual void OpenUrlMainThreadOnly(string url) { + if (!ExternalUrlPolicy.IsWebScheme(url)) + { + ReportHub.LogWarning(ReportCategory.UI, "Refused to open non-web URL scheme"); + return; + } + Application.OpenURL(Uri.EscapeUriString(url)); } diff --git a/Explorer/Assets/DCL/NetworkDefinitions/DCL.Network.asmdef b/Explorer/Assets/DCL/NetworkDefinitions/DCL.Network.asmdef index ab112aff717..116eac78eb8 100644 --- a/Explorer/Assets/DCL/NetworkDefinitions/DCL.Network.asmdef +++ b/Explorer/Assets/DCL/NetworkDefinitions/DCL.Network.asmdef @@ -12,7 +12,8 @@ "GUID:1087662aaf1c5462baa91fb9484296fd", "GUID:e0cd26848372d4e5c891c569017e11f1", "GUID:13c468c9ecfc44f249fb8fb69d3365ef", - "GUID:d8b63aba1907145bea998dd612889d6b" + "GUID:d8b63aba1907145bea998dd612889d6b", + "GUID:3acaaf1301d842f498099c27ba260121" ], "includePlatforms": [], "excludePlatforms": [], diff --git a/Explorer/Assets/DCL/PluginSystem/Global/ChangeRealmPromptPlugin.cs b/Explorer/Assets/DCL/PluginSystem/Global/ChangeRealmPromptPlugin.cs index 032fd48568d..1c49af48ed6 100644 --- a/Explorer/Assets/DCL/PluginSystem/Global/ChangeRealmPromptPlugin.cs +++ b/Explorer/Assets/DCL/PluginSystem/Global/ChangeRealmPromptPlugin.cs @@ -16,14 +16,14 @@ public class ChangeRealmPromptPlugin : IDCLGlobalPlugin changeRealmCallback; + private readonly Action changeRealmCallback; private ChangeRealmPromptController? changeRealmPromptController; public ChangeRealmPromptPlugin( IAssetsProvisioner assetsProvisioner, IMVCManager mvcManager, ICursor cursor, - Action changeRealmCallback) + Action changeRealmCallback) { this.assetsProvisioner = assetsProvisioner; this.mvcManager = mvcManager; @@ -55,7 +55,7 @@ public class ChangeRealmPromptSettings : IDCLPluginSettings [field: Header(nameof(ChangeRealmPromptPlugin) + "." + nameof(ChangeRealmPromptSettings))] [field: Space] [field: SerializeField] - public AssetReferenceGameObject ChangeRealmPromptPrefab; + public AssetReferenceGameObject ChangeRealmPromptPrefab = null!; } } } diff --git a/Explorer/Assets/DCL/Prefs/DCLPrefKeys.cs b/Explorer/Assets/DCL/Prefs/DCLPrefKeys.cs index 2e931cba04a..04bb1836b51 100644 --- a/Explorer/Assets/DCL/Prefs/DCLPrefKeys.cs +++ b/Explorer/Assets/DCL/Prefs/DCLPrefKeys.cs @@ -6,6 +6,10 @@ public static class DCLPrefKeys public const string LAUNCH_COUNT = "LaunchCount"; + // Developer-selected path to the Creator Hub executable, remembered across launches so the Chrome + // DevTools bridge can relaunch it without re-prompting. Not an app-arg / deep-link input (SEC-005). + public const string CREATOR_HUB_BIN_PATH = "CreatorHub.BinPath"; + public const string PREVIOUS_SEARCHES = "previous_searches"; public const string WEB3_IDENTITY = "Web3Authentication.Identity"; diff --git a/Explorer/Assets/DCL/RuntimeDeepLink/DeepLinkHandleImplementation.cs b/Explorer/Assets/DCL/RuntimeDeepLink/DeepLinkHandleImplementation.cs index 5cd3ba6ccf7..d917115a9e5 100644 --- a/Explorer/Assets/DCL/RuntimeDeepLink/DeepLinkHandleImplementation.cs +++ b/Explorer/Assets/DCL/RuntimeDeepLink/DeepLinkHandleImplementation.cs @@ -1,5 +1,6 @@ using CommunicationData.URLHelpers; using Cysharp.Threading.Tasks; +using DCL.ChangeRealmPrompt; using DCL.Chat.Commands; using DCL.Communities; using DCL.ExplorePanel; @@ -71,11 +72,7 @@ public DeepLinkHandleResult HandleDeepLink(DeepLink deeplink) if (realm.HasValue) { - if(position.HasValue) - chatTeleporter.TeleportToRealmAsync(realm.Value.Value, position.Value, token).Forget(); - else - chatTeleporter.TeleportToRealmAsync(realm.Value.Value, token).Forget(); - + ShowRealmChangePromptAsync(realm.Value.Value, position).Forget(); handled = true; } else if (position.HasValue) @@ -105,6 +102,19 @@ public DeepLinkHandleResult HandleDeepLink(DeepLink deeplink) return handled ? DeepLinkHandleResult.CONSUMED : DeepLinkHandleResult.NO_MATCHES; } + private async UniTaskVoid ShowRealmChangePromptAsync(string realm, Vector2Int? position) + { + // Empty message → the prompt renders the default confirmation text and requires an explicit click (SEC-003). + if (token.IsCancellationRequested) + return; + + await UniTask.SwitchToMainThread(token); + + var parameters = new ChangeRealmPromptController.Params(string.Empty, realm, position); + + await mvcManager.ShowAsync(ChangeRealmPromptController.IssueCommand(parameters), token); + } + private static URLDomain? RealmFrom(DeepLink deepLink) { string? rawRealm = deepLink.ValueOf(AppArgsFlags.REALM); diff --git a/Explorer/Assets/DCL/Tests/Editor/ChangeRealmPromptDestinationHostShould.cs b/Explorer/Assets/DCL/Tests/Editor/ChangeRealmPromptDestinationHostShould.cs new file mode 100644 index 00000000000..ae3abc700a0 --- /dev/null +++ b/Explorer/Assets/DCL/Tests/Editor/ChangeRealmPromptDestinationHostShould.cs @@ -0,0 +1,20 @@ +using DCL.ChangeRealmPrompt; +using NUnit.Framework; + +namespace DCL.Tests.Editor +{ + public class ChangeRealmPromptDestinationHostShould + { + [TestCase("https://evil.com/path", "evil.com")] + [TestCase("https://evil.com:8080/path", "evil.com:8080")] // port kept in the displayed authority + [TestCase("https://decentraland.org@evil.com", "evil.com")] // userinfo stripped — real host shown + [TestCase("https://user:pass@evil.com:443/x", "evil.com:443")] // userinfo + port + [TestCase("world-name", "world-name")] // no scheme → shown unchanged + [TestCase("https://host?q=1", "host")] // query stripped + [TestCase("https://host#frag", "host")] // fragment stripped + public void ExtractsTrueHostForDisplay(string realm, string expected) + { + Assert.AreEqual(expected, ChangeRealmPromptController.DestinationHostFor(realm), realm); + } + } +} diff --git a/Explorer/Assets/DCL/Tests/Editor/ChangeRealmPromptDestinationHostShould.cs.meta b/Explorer/Assets/DCL/Tests/Editor/ChangeRealmPromptDestinationHostShould.cs.meta new file mode 100644 index 00000000000..36d472b31bb --- /dev/null +++ b/Explorer/Assets/DCL/Tests/Editor/ChangeRealmPromptDestinationHostShould.cs.meta @@ -0,0 +1,2 @@ +fileFormatVersion: 2 +guid: 6937ae263a04c1b4486661b8361d27bf \ No newline at end of file diff --git a/Explorer/Assets/DCL/Tests/Editor/ChatEnvironmentValidatorShould.cs b/Explorer/Assets/DCL/Tests/Editor/ChatEnvironmentValidatorShould.cs new file mode 100644 index 00000000000..27a72d69c9a --- /dev/null +++ b/Explorer/Assets/DCL/Tests/Editor/ChatEnvironmentValidatorShould.cs @@ -0,0 +1,31 @@ +using DCL.Chat.Commands; +using DCL.Multiplayer.Connections.DecentralandUrls; +using NUnit.Framework; + +namespace DCL.Tests.Editor +{ + public class ChatEnvironmentValidatorShould + { + [TestCase("https://peer.decentraland.org", true)] + [TestCase("https://worlds-content-server.decentraland.org/world/x.dcl.eth", true)] + [TestCase("https://evil.example/org", false)] // substring "org" no longer passes + [TestCase("https://decentraland.org.attacker.com", false)] // suffix-spoof + [TestCase("https://attacker-decentraland.org", false)] // not a real subdomain boundary + [TestCase("https://decentraland.org@evil.com", false)] // userinfo spoof — real host is evil.com + [TestCase("https://decentraland.org:1234@evil.com", false)] // colon-before-@ userinfo spoof (real host evil.com) + [TestCase("https://peer.decentraland.org:443", true)] // legit host with an explicit port + public void ValidateOrgBySuffix(string realm, bool expectSuccess) + { + var validator = new ChatEnvironmentValidator(DecentralandEnvironment.Org); + Assert.AreEqual(expectSuccess, validator.ValidateTeleport(realm).Success, realm); + } + + [TestCase("https://peer.decentraland.zone", true)] + [TestCase("https://evil.example/zone", false)] + public void ValidateZoneBySuffix(string realm, bool expectSuccess) + { + var validator = new ChatEnvironmentValidator(DecentralandEnvironment.Zone); + Assert.AreEqual(expectSuccess, validator.ValidateTeleport(realm).Success, realm); + } + } +} diff --git a/Explorer/Assets/DCL/Tests/Editor/ChatEnvironmentValidatorShould.cs.meta b/Explorer/Assets/DCL/Tests/Editor/ChatEnvironmentValidatorShould.cs.meta new file mode 100644 index 00000000000..73ca7156280 --- /dev/null +++ b/Explorer/Assets/DCL/Tests/Editor/ChatEnvironmentValidatorShould.cs.meta @@ -0,0 +1,2 @@ +fileFormatVersion: 2 +guid: 20aae7d5f1561264c8c0f06825f4a2a9 \ No newline at end of file diff --git a/Explorer/Assets/DCL/UI/AssemblyInfo.cs b/Explorer/Assets/DCL/UI/AssemblyInfo.cs new file mode 100644 index 00000000000..7debebc1e54 --- /dev/null +++ b/Explorer/Assets/DCL/UI/AssemblyInfo.cs @@ -0,0 +1,5 @@ +using System.Runtime.CompilerServices; + +// Exposes internal members (e.g. ChangeRealmPromptController.DestinationHostFor) to the EditMode test assembly +// so security-critical host parsing can be unit-tested without widening the members to public. +[assembly: InternalsVisibleTo("DCL.EditMode.Tests")] diff --git a/Explorer/Assets/DCL/UI/AssemblyInfo.cs.meta b/Explorer/Assets/DCL/UI/AssemblyInfo.cs.meta new file mode 100644 index 00000000000..fb89c5edd82 --- /dev/null +++ b/Explorer/Assets/DCL/UI/AssemblyInfo.cs.meta @@ -0,0 +1,2 @@ +fileFormatVersion: 2 +guid: d8c10cbfea997674e938377903c2ccf9 \ No newline at end of file diff --git a/Explorer/Assets/DCL/WebRequests/ChromeDevtool/ChromeDevToolHandler.cs b/Explorer/Assets/DCL/WebRequests/ChromeDevtool/ChromeDevToolHandler.cs index c6e8e7ea7d0..f62645fbbe5 100644 --- a/Explorer/Assets/DCL/WebRequests/ChromeDevtool/ChromeDevToolHandler.cs +++ b/Explorer/Assets/DCL/WebRequests/ChromeDevtool/ChromeDevToolHandler.cs @@ -3,7 +3,6 @@ using DCL.Diagnostics; using DCL.Optimization.Pools; using DCL.WebRequests.Analytics; -using Global.AppArgs; using System; using System.Collections.Generic; using System.Linq; @@ -50,10 +49,10 @@ public static ChromeDevToolHandler NewForTest() => new (1, new Bridge()); #endif - public static ChromeDevToolHandler New(bool startOnCreation, IAppArgs appArgs) + public static ChromeDevToolHandler New(bool startOnCreation) { #if UNITY_WEBGL // ChromeDevtoolProtocolClient is not supported on WebGL. WebSocket server cannot be instantiated from a webpage, use mock - return new ChromeDevToolHandler(0, null!); + return new ChromeDevToolHandler(0, null!); #else const int PORT = 1473; @@ -61,7 +60,7 @@ public static ChromeDevToolHandler New(bool startOnCreation, IAppArgs appArgs) var bridge = new Bridge( handleMethod: HandleCdpMethod, - browser: new CreatorHubBrowser(appArgs, PORT), + browser: new CreatorHubBrowser(PORT), logger: new DCLLogger(ReportCategory.CHROME_DEVTOOL_PROTOCOL), port: PORT ); diff --git a/Explorer/Assets/DCL/WebRequests/ChromeDevtool/CreatorHubBrowser.cs b/Explorer/Assets/DCL/WebRequests/ChromeDevtool/CreatorHubBrowser.cs index e50cfc4ce2f..83bb6cf81bb 100644 --- a/Explorer/Assets/DCL/WebRequests/ChromeDevtool/CreatorHubBrowser.cs +++ b/Explorer/Assets/DCL/WebRequests/ChromeDevtool/CreatorHubBrowser.cs @@ -1,6 +1,7 @@ using CDPBridges; +using Crosstales.FB; using DCL.Diagnostics; -using Global.AppArgs; +using DCL.Prefs; using Plugins.DclNativeProcesses; using RichTypes; using System; @@ -11,7 +12,7 @@ namespace DCL.WebRequests.ChromeDevtool public class CreatorHubBrowser : IBrowser { private const string DEVTOOL_PORT_ARG = "--open-devtools-with-port="; - private readonly IAppArgs appArgs; + private readonly int port; #if UNITY_EDITOR_WIN || UNITY_STANDALONE_WIN || PLATFORM_STANDALONE_WIN @@ -21,35 +22,34 @@ public class CreatorHubBrowser : IBrowser Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData), "Programs", "creator-hub", "Decentraland Creator Hub.exe" ); + + private const string EXECUTABLE_EXTENSION = "exe"; #else public static readonly string DEFAULT_CREATOR_HUB_BIN_PATH = "/Applications/Decentraland Creator Hub.app/Contents/MacOS/Decentraland Creator Hub"; + + private const string EXECUTABLE_EXTENSION = "*"; #endif - public CreatorHubBrowser(IAppArgs appArgs, int port) + public CreatorHubBrowser(int port) { - this.appArgs = appArgs; this.port = port; } public BrowserOpenResult OpenUrl(string url) { - string path = CreatorHubExecutablePath(); + // Resolve lazily: the CDP bridge is the only consumer of the Creator Hub path, so we only + // touch PlayerPrefs / prompt the developer when DevTools actually needs to launch it. + string path = ResolveBinPath(); - if (File.Exists(path) == false) + if (string.IsNullOrEmpty(path) || File.Exists(path) == false) { - BrowserOpenError error = BrowserOpenError.FromException(new Exception($"Creator Hub is not installed in path: {path}")); + BrowserOpenError error = BrowserOpenError.FromException(new Exception($"Creator Hub executable not found or not selected (resolved path: '{path}')")); return BrowserOpenResult.FromBrowserOpenError(error); } ReportHub.LogWarning(ReportCategory.CHROME_DEVTOOL_PROTOCOL, "Url always ignored by Creator Hub Browser, port is used"); - Result result = DclProcesses.Start( - path, - new[] - { - $"{DEVTOOL_PORT_ARG}{port}", - } - ); + Result result = DclProcesses.Start(path, new[] { $"{DEVTOOL_PORT_ARG}{port}" }); if (result.Success == false) { @@ -60,13 +60,48 @@ public BrowserOpenResult OpenUrl(string url) return BrowserOpenResult.Success(); } - private string CreatorHubExecutablePath() + /// + /// Resolves the Creator Hub executable: a path the developer previously selected (remembered in + /// ) wins; otherwise the pinned default install path; otherwise the + /// developer is prompted once to locate it and the choice is remembered. There is no app-arg / + /// deep-link input (SEC-005) — the path is either the known install location or a local file the + /// developer explicitly picks. + /// + private static string ResolveBinPath() { - if (appArgs.TryGetValue(AppArgsFlags.CREATOR_HUB_BIN_PATH, out string? path)) - return path!; + string savedPath = DCLPlayerPrefs.GetString(DCLPrefKeys.CREATOR_HUB_BIN_PATH); + + if (!string.IsNullOrEmpty(savedPath) && File.Exists(savedPath)) + return savedPath; + + if (File.Exists(DEFAULT_CREATOR_HUB_BIN_PATH)) + return DEFAULT_CREATOR_HUB_BIN_PATH; + + string picked = PromptForExecutable(); + + if (!string.IsNullOrEmpty(picked)) + { + DCLPlayerPrefs.SetString(DCLPrefKeys.CREATOR_HUB_BIN_PATH, picked, save: true); + return picked; + } + + return string.Empty; + } + + private static string PromptForExecutable() + { + FileBrowser fileBrowser = FileBrowser.Instance; + + // Enable synchronous picking on macOS (off by default) so the dialog fits OpenUrl's synchronous contract. + fileBrowser.AllowSyncCalls = true; + + string startDirectory = File.Exists(DEFAULT_CREATOR_HUB_BIN_PATH) + ? Path.GetDirectoryName(DEFAULT_CREATOR_HUB_BIN_PATH) ?? string.Empty + : string.Empty; + + string? selected = fileBrowser.OpenSingleFile("Select the Decentraland Creator Hub executable", startDirectory, string.Empty, EXECUTABLE_EXTENSION); - ReportHub.LogWarning(ReportCategory.CHROME_DEVTOOL_PROTOCOL, "Creator Hub path is not provided, fallback to default path"); - return DEFAULT_CREATOR_HUB_BIN_PATH; + return selected ?? string.Empty; } } } diff --git a/docs/app-arguments.md b/docs/app-arguments.md index 7f8f6938688..36a7224d2c9 100644 --- a/docs/app-arguments.md +++ b/docs/app-arguments.md @@ -337,17 +337,6 @@ decentraland://?force-open-backpack=true --- -### `creator-hub-bin-path` -**Type:** String (file path) -**Description:** Specifies a custom path to the Creator Hub binary. Used when the Creator Hub needs to be launched from a non-standard location. - -**Usage:** -```bash ---creator-hub-bin-path /path/to/creator-hub -``` - ---- - ### `use-log-matrix` **Type:** String (file path) **Description:** Enables logging to a matrix file. The value should be the path to the log matrix file.