Conversation
Target a non-decentraland.* deployment (e.g. interconnected.online) by
resolving every backend host through DecentralandUrlsSource.ResolveDomain,
which swaps the whole decentraland.{ENV} host token for a custom base domain
supplied by the --base-domain app arg (AppArgsFlags.BASE_DOMAIN). The default
path is byte-identical, swapping only the {ENV} TLD.
Consumers that follow the resolved domain: teleport validation
(ChatEnvironmentValidator), realm trust (DeepLinkAllowlist), the main-realm
comms fallback (RealmController), the realm-name server list (RealmNamesMap),
the pre-login feature-flag whitelist URL (GetFeatureFlagsUrl), the
smart-wearable content fallback (SmartWearableCache), and the local scene
adapter (LocalGateKeeperSceneAdapter). Gateway routing already follows it: the
transform runs on the {ENV} template and ResolveDomain rewrites the whole
token, gateway host included. The default "Empty place" no longer hardcodes a
peer.decentraland.org thumbnail and falls back to the built-in placeholder.
Characterization tests pin every DecentralandUrl across org/zone/custom
domains and gateway routing (gatekeeper family included); unit tests cover
custom-domain teleport validation and deep-link realm trust.
Contributor
🚦 CI Status
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below.
Warnings count reduced: 13156 => 13085 Warnings/errors in files changed by this PR (65)All Unity tests passed ✅
|
decentraland-bot
added
the
ext-contribution
Contributor
|
Slack notification sent to #explorer-ext-contributions for external review. |
decentraland-bot
left a comment
decentraland-bot
left a comment
Contributor
There was a problem hiding this comment.
PR Review: feat(urls): configurable base domain via --base-domain
STEP 2 — Root-cause check: PASS ✅
The PR introduces a --base-domain CLI argument so the client can target a non-decentraland.* deployment (e.g. interconnected.online). The approach is sound: DecentralandUrlsSource.ResolveDomain() centralizes domain substitution. The default path replaces only {ENV} (byte-identical to before, today-mixture safe); a custom base domain replaces the whole decentraland.{ENV} token, moving every backend host. This is a feature addition, not a symptom fix.
STEP 3 — Design & integration: PASS ✅
No new long-lived units — all changes extend existing classes. The design introduces:
- A
customBaseDomainfield onDecentralandUrlsSource(immutable after construction) - A static
customBaseDomainonDeepLinkAllowlist(follows existingwhitelistedWorldspattern, set once during initialization, reset in test teardown) - New constructor parameters on
SmartWearableCache,RealmNamesMap,ChatEnvironmentValidator
Owner search results:
DecentralandUrlsSourcealready owns all URL resolution —ResolveDomain()andResolvedBaseDomainbelong here.DeepLinkAllowlistalready owns host trust checks — extendingIsDecentralandHostwith the custom domain is the right place.ChatEnvironmentValidatoralready owns teleport validation — parameterizing its domain suffix rather than hardcoding it is correct.GatewayUrlsSourcederives gateway routing from the base class — usingResolvedBaseDomain(protected) is legitimate inheritance, not leaky abstraction.
Teardown/consumption trace:
- No new subscriptions, event hookups, connections, or disposable resources introduced. All changes are constructor-time wiring or one-time static initialization.
STEP 4 — Member audit: PASS ✅
| Member | Consumers | Verdict |
|---|---|---|
ResolveDomain() (private) |
Probe(), Url(), ResolveOptimizedAssetsUrl() — central substitution point |
Correctly encapsulated |
ResolvedBaseDomain (protected) |
GatewayUrlsSource ctor (3 refs: resolvedNonClientHosts, gatewayPrefix, domainSuffix) |
Legitimate protected accessor for inheritance |
SetCustomBaseDomain() (public static) |
MainSceneLoader.InitializeFlowAsync(), test teardown |
Follows existing SetWhitelistedWorlds pattern |
DOMAIN_TOKEN (protected const) |
ResolveDomain(), GatewayUrlsSource ctor, GetFeatureFlagsUrl() |
Correctly scoped |
No single-use intermediaries, no absent-≠-false predicates, no redundant guards.
STEP 5 — Line-level review
Security review: No security issues found.
BASE_DOMAINis a CLI arg, NOT in the deep linkPERMITTED_KEYSorWHITELISTED_REALM_PERMITTED_KEYS— an attacker cannot inject it via deep link ✅IsDecentralandHostboundary checks are correct:host.Length > customBaseDomain.Length+host[..] == '.'+EndsWith— rejects both bare domain and suffix-spoof attacks ✅ChatEnvironmentValidator.HostHasSuffixrejects userinfo-based host confusion (@check before:) ✅- No secrets, credentials, or PII exposed ✅
- Gateway routing works correctly:
IsGatewayTransformableruns on unresolved templates (which always contain.decentraland.), thenResolveDomainsubstitutes the custom domain afterward ✅
Findings: Two P2 issues (see line comments below).
STEP 6 — Complexity: COMPLEX
20 files changed (+444 −37). Touches URL infrastructure (DecentralandUrlsSource, GatewayUrlsSource), gateway routing, deep link trust model (DeepLinkAllowlist), teleport validation, realm name resolution, and smart wearable content fallback across multiple assemblies.
STEP 7 — QA assessment: YES
Runtime code changes affecting URL routing, teleport validation, and realm trust. Although the default path is byte-identical (backward compatible when --base-domain is not supplied), the URL resolution and gateway routing paths have changed.
STEP 8 — Non-blocking warnings
None. Main scene not modified.
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches URL infrastructure, gateway routing, deep link trust model, and teleport validation across 7+ assemblies
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by unknown via Slack
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Rename catalyrst -> catalyst in the base-domain doc and drop the ReSharper CheckNamespace suppression from the new characterization test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
decentraland-bot
left a comment
decentraland-bot
left a comment
Contributor
There was a problem hiding this comment.
STEP 1 — Context & Scope
PR: feat(urls): configurable base domain via --base-domain
Target: dev
Files: 20 changed (+442 −37)
Subsystems: NetworkDefinitions (DecentralandUrlsSource, GatewayUrlsSource), Infrastructure (DeepLinkAllowlist, ChatContainer, RealmController, MainSceneLoader, StaticContainer, BootstrapContainer), Chat (ChatEnvironmentValidator), PlacesAPIService, SmartWearables.
Loaded: CLAUDE.md, docs/README.md, review-instructions.md. Reviewed full source of DecentralandUrlsSource.cs, GatewayUrlsSource.cs, IDecentralandUrlsSource.cs, DeepLinkAllowlist.cs, RealmController.cs, ChatContainer.cs, SmartWearableCache.cs, ThumbnailLoader.cs.
STEP 2 — Root-cause check
PASS. This PR adds a feature (configurable base domain via --base-domain), not a bug fix. The approach centralizes domain resolution through ResolveDomain() — a new private method that substitutes either {ENV} (default, byte-identical to prior behavior) or the full decentraland.{ENV} token (custom domain). The design correctly addresses the need to target non-decentraland.* deployments without modifying the DecentralandEnvironment enum.
STEP 3 — Design & integration
PASS. No new long-lived units introduced. The changes parameterize existing classes:
DecentralandUrlsSource: gainscustomBaseDomainfield,DOMAIN_TOKENconstant,ResolveDomain()method, andResolvedBaseDomainproperty. All contained within the existing class hierarchy.GatewayUrlsSource: derives gateway prefix/suffix/non-client hosts fromResolvedBaseDomaininstead of direct ENV replacement. Gateway transform runs on raw templates beforeResolveDomain()— these compose correctly (confirmed by characterization tests).DeepLinkAllowlist.SetCustomBaseDomain(): follows the establishedSetWhitelistedWorlds()pattern (static mutable state, same class). Domain-trust check mirrors the existingALL_DOMAINSloop with identical dot-boundary validation. Suffix-spoofing correctly rejected (test at line 800 confirmsinterconnected.online.attacker.com→false).- Consumer updates (
ChatEnvironmentValidator,RealmNamesMap,SmartWearableCache,RealmController) receive the resolved domain via dependency injection or the URL source — no lifecycle duplication, no new reconciliation loops.
Owner search: No new lifecycle-owning units to audit.
Teardown/consumption trace: No new subscriptions, event hookups, or connections. RealmData.RealmType.OnUpdate += ResetRealmDependentUrls is pre-existing and unchanged.
STEP 4 — Member audit
| Member | Consumers | Assessment |
|---|---|---|
ResolvedBaseDomain (protected) |
ResolveDomain(), GatewayUrlsSource ctor (3 reads) |
Multi-use, justified. Two external call sites re-derive this value — see inline finding. |
ResolveDomain() (private) |
Probe(), Url(), ResolveOptimizedAssetsUrl() |
4 call sites, cleanly encapsulates branching logic. |
DOMAIN_TOKEN (const) |
ResolveDomain(), GetFeatureFlagsUrl(), GatewayUrlsSource ctor |
3 call sites across 2 files, justified. |
SetCustomBaseDomain() |
MainSceneLoader.InitializeFlowAsync, test teardown |
2 call sites, follows SetWhitelistedWorlds pattern. |
realmDomainSuffix (ChatEnvironmentValidator) |
ValidateTeleport() Zone/Org cases |
Replaces previously hardcoded domain constants. Legitimate parameterization. |
Note: GetFeatureFlagsUrl (line 255) duplicates the custom-domain-or-env-fallback branching from ResolveDomain() as a static method. This is necessary since it runs before the instance exists (InitializeDeepLinkWorldWhitelistAsync), but the parallel logic could drift — worth extracting a shared static helper if this method gains more callers.
STEP 5 — Line-level review
See inline comments for each finding with suggestion blocks.
Summary of findings:
| # | Sev | File | Issue |
|---|---|---|---|
| 1 | P2 | ChatContainer.cs:106-108 |
Fragile .Replace("https://", "") re-derives ResolvedBaseDomain |
| 2 | P2 | RealmController.cs:375 |
Same .Replace pattern (second occurrence) |
| 3 | P2 | DeepLinkAllowlist.cs:172-173 |
No format validation on custom domain — single-label domains trust entire TLDs |
| 4 | P2 | DecentralandUrlsSource.cs:68 |
Redundant null-forgiving ! operator |
Security review: No secrets committed. --base-domain is correctly excluded from PERMITTED_KEYS (not injectable via deep links). Domain-trust expansion in IsDecentralandHost uses proper dot-boundary validation matching the existing ALL_DOMAINS pattern. Gateway routing correctly composes with domain resolution. No auth bypass vectors identified.
Characterization tests: The 246-line DecentralandUrlsSourceCharacterizationShould.cs is excellent — it pins every domain-bearing URL for both Org and Zone environments, verifies env-independent URLs stay unchanged, tests gateway routing round-trips, and covers custom-domain retargeting. This is exactly the safety net needed for this change.
STEP 6 — Complexity assessment
COMPLEX. Touches URL resolution, domain trust (DeepLinkAllowlist), gateway routing, realm validation, and chat teleport validation across 20 files spanning NetworkDefinitions, Infrastructure, Chat, PlacesAPIService, and SmartWearables assemblies.
STEP 7 — QA assessment
QA_REQUIRED: YES. Changes runtime URL resolution affecting realm connections, teleportation validation, gateway routing, smart wearable content loading, and realm name resolution. However, the PR is designed to be byte-identical on the default path (no custom domain), so the risk to existing behavior is low.
STEP 8 — Non-blocking warnings
None. Main.unity is not modified.
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches URL resolution, domain trust, gateway routing, and realm validation across 20 files in NetworkDefinitions, Infrastructure, Chat, and SmartWearables assemblies
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by eordano via GitHub
Comment on lines
+106
to
+108
| string realmDomainSuffix = bootstrapContainer.DecentralandUrlsSource | ||
| .Url(DCL.Multiplayer.Connections.DecentralandUrls.DecentralandUrl.Host) | ||
| .Replace("https://", string.Empty); |
Contributor
There was a problem hiding this comment.
[P2] Re-derives ResolvedBaseDomain via fragile scheme stripping (CLAUDE.md: "Don't re-derive what already exists").
DecentralandUrlsSource.ResolvedBaseDomain already computes this exact value, but is protected and not on IDecentralandUrlsSource. Both this site and RealmController.ResolveHostname (line 375) independently strip the scheme with .Replace("https://", ""), which silently breaks if the scheme ever changes.
Ideal fix: expose BaseDomain on IDecentralandUrlsSource (backed by ResolvedBaseDomain). Immediate improvement: use Uri.Host for safer parsing:
Suggested change
| string realmDomainSuffix = bootstrapContainer.DecentralandUrlsSource | |
| .Url(DCL.Multiplayer.Connections.DecentralandUrls.DecentralandUrl.Host) | |
| .Replace("https://", string.Empty); | |
| string realmDomainSuffix = new Uri(bootstrapContainer.DecentralandUrlsSource | |
| .Url(DCL.Multiplayer.Connections.DecentralandUrls.DecentralandUrl.Host)).Host; |
| // TODO: take in consideration the web3-network. If its sepolia then it should be .zone | ||
| ? "realm-provider." + IDecentralandUrlsSource.ORG_DOMAIN | ||
| // Consider it as the "main" realm which shares the comms with many catalysts. | ||
| ? "realm-provider." + decentralandUrlsSource.Url(DecentralandUrl.Host).Replace("https://", string.Empty) |
Contributor
There was a problem hiding this comment.
[P2] Same .Replace("https://", "") pattern — second occurrence of the fragile domain extraction. Same BaseDomain interface property would eliminate both.
Suggested change
| ? "realm-provider." + decentralandUrlsSource.Url(DecentralandUrl.Host).Replace("https://", string.Empty) | |
| ? "realm-provider." + new Uri(decentralandUrlsSource.Url(DecentralandUrl.Host)).Host |
Comment on lines
+172
to
+173
| public static void SetCustomBaseDomain(string? domain) => | ||
| customBaseDomain = string.IsNullOrWhiteSpace(domain) ? null : domain.Trim(); |
Contributor
There was a problem hiding this comment.
[P2] Defense-in-depth: validate domain shape. A single-label domain (e.g. --base-domain com) would cause IsDecentralandHost to trust every .com host. While --base-domain is CLI-only (not in PERMITTED_KEYS, so deep links cannot inject it), basic format validation prevents accidental misconfiguration and hardens the public API surface of SetCustomBaseDomain().
Suggested change
| public static void SetCustomBaseDomain(string? domain) => | |
| customBaseDomain = string.IsNullOrWhiteSpace(domain) ? null : domain.Trim(); | |
| public static void SetCustomBaseDomain(string? domain) | |
| { | |
| if (string.IsNullOrWhiteSpace(domain)) | |
| { | |
| customBaseDomain = null; | |
| return; | |
| } | |
| string trimmed = domain.Trim(); | |
| // Require at least two labels and reject URL control characters (defense-in-depth). | |
| if (!trimmed.Contains('.') || trimmed.IndexOfAny(new[] { '/', '?', '#', '@', ':' }) >= 0) | |
| throw new ArgumentException($"Invalid base domain: '{trimmed}'"); | |
| customBaseDomain = trimmed; | |
| } |
| string? customBaseDomain = null) | ||
| { | ||
| decentralandDomain = environment.ToString()!.ToLower(); | ||
| this.customBaseDomain = string.IsNullOrWhiteSpace(customBaseDomain) ? null : customBaseDomain!.Trim(); |
Contributor
There was a problem hiding this comment.
[P2] Redundant null-forgiving ! — IsNullOrWhiteSpace returning false guarantees non-null on the else branch; the ! is unnecessary noise.
Suggested change
| this.customBaseDomain = string.IsNullOrWhiteSpace(customBaseDomain) ? null : customBaseDomain!.Trim(); | |
| this.customBaseDomain = string.IsNullOrWhiteSpace(customBaseDomain) ? null : customBaseDomain.Trim(); |
Gateway-eligible URLs resolved before feature flags load are no longer cached in raw form for the process lifetime; they stay uncacheable until the use-gateway flag is known. The main-realm comms fallback pins realm-provider.decentraland.org for every decentraland.* environment and follows only a custom base domain. Characterization tests pin the pre-flags -> post-flags resolution sequence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SystemSpecUtils honors TryGetJsonPayload's result and absent individual fields instead of dereferencing null members; a failing specs evaluation reports the exception and never blocks startup, so --skip-minimum-specs-screen stays effective. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A payload defining only one platform's requirements no longer rejects the other platform: when every field of a dimension (cpu/gpu/os) is absent the check passes, while an explicitly-present empty list still rejects. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Clears the 50 InspectCode findings in files this PR touches (CS8618 uninitialized non-nullables via null! / nullable annotations per file idiom, CS8602/03/04 via real guards or invariant-backed forgiveness), bringing the warning count back under the ratchet baseline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…missing The tolerance fixes land without a regression test for the missing-payload path; this pins every SystemSpecUtils check against an empty flags configuration, live-hit on a --base-domain deployment whose flags backend served no alfa-minimum-requirements payload (boot died on splash). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
PR #9728, run #31804214385 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
…domain check Pre-flags gateway-eligible URLs get a dedicated FlagsPending cache state: cached provisionally, then dropped and re-resolved once feature flags load. This closes the torn-read window that could permanently cache an un-gatewayed URL when flags landed mid-resolution, and bounds flags-never-load sessions to one resolution per URL instead of one per call. RealmController classifies environment domains by exact membership in ALL_DOMAINS instead of a spoofable name prefix, and SmartWearableCache returns the tracked item instead of null! on the cancelled path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
StaticContainer.GPUInstancingService is nullable for real: the disabled path (no render feature / instancing off) now skips registering the GPU-instancing feature set (plugin, RoadsPresence, debug system) instead of carrying a null! that consumers dereference; intentional disable and misconfiguration are logged distinctly. RealmController's main-realm hostname derivation becomes an internal static seam with tests pinning org, the zone->org re-pin, custom domains, and the env-domain spoof shape. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Unity-bundled NUnit predates Assert.Multiple (CS0117), which broke compilation of DCL.EditMode.Tests and with it the Lint and EditMode jobs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
PR #9728, run #31818100451 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
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.
Target a non-decentraland.* deployment (e.g. interconnected.online) by resolving every backend host through DecentralandUrlsSource.ResolveDomain, which swaps the whole decentraland.{ENV} host token for a custom base domain supplied by the --base-domain app arg (AppArgsFlags.BASE_DOMAIN). The default path is byte-identical, swapping only the {ENV} TLD.
Consumers that follow the resolved domain: teleport validation (ChatEnvironmentValidator), realm trust (DeepLinkAllowlist), the main-realm comms fallback (RealmController), the realm-name server list (RealmNamesMap), the pre-login feature-flag whitelist URL (GetFeatureFlagsUrl), the smart-wearable content fallback (SmartWearableCache), and the local scene adapter (LocalGateKeeperSceneAdapter). Gateway routing already follows it: the transform runs on the {ENV} template and ResolveDomain rewrites the whole token, gateway host included. The default "Empty place" no longer hardcodes a peer.decentraland.org thumbnail and falls back to the built-in placeholder.
Characterization tests pin every DecentralandUrl across org/zone/custom domains and gateway routing (gatekeeper family included); unit tests cover custom-domain teleport validation and deep-link realm trust.
QA: Full automated testing should not raise any errors or changes -- this is internal only. Test --base-domain interconnected.online to see if it works on a different catalyst setup