chore: e2e containerized infra - #9822
Conversation
The startup gate that decides whether a custom realm needs the untrusted-realm consent prompt lived as a run of inline host comparisons in MainSceneLoader. Move it into TrustedRealms, split into two tiers: exact hosts, and domains whose every subdomain is Decentraland-controlled. The domain tier is what lets the ephemeral e2e-fixtures.decentraland.zone Catalyst fixtures be trusted at all -- their hostname is minted per run, so it cannot be enumerated as an exact host. Only decentraland.zone gets domain-level trust. Production stays per-host, so that neither one production subdomain nor one dangling DNS record under it can become a consent-free realm switch for every user -- realm is a query param a decentraland:// deep link may inject, and it sits on DeepLinkAllowlist's always-permitted tier precisely because this gate exists to catch it. Domain trust is https-only: a remote realm reached over cleartext can be answered by a network attacker, so inheriting the domain's trust would hand that trust to anyone on the path. Loopback opts out and stays scheme-agnostic, since local scene development and a locally hosted E2E fixture are plain http and have no meaningful network to attack. Suffix matching is anchored to a label boundary at the end of the host, which is what rejects evildecentraland.zone and decentraland.zone.example.com. TrustedRealmsShould pins the policy, negative cases included: look-alike registrations, cleartext inside a trusted domain, whole-domain production trust, and trusted-looking strings in userinfo, query and fragment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🚦 CI Status
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Warnings count reduced: 12191 => 12190 Warnings/errors in files changed by this PR (18)All Unity tests passed ✅
|
mikhail-dcl
changed the title
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review — TrustedRealms extraction
STEP 2 — Root-cause check: PASS
This PR extracts inline trusted-realm host checks from MainSceneLoader.IsTrustedRealmAsync into a dedicated, testable TrustedRealms static class. The extraction solves the right problem: the inline if-chain was untestable in isolation and could not accommodate dynamically minted E2E fixture hostnames (f-{id}.e2e-fixtures.decentraland.zone). The new two-tier model (exact hosts + domain wildcards) addresses both needs cleanly.
STEP 3 — Design & integration: PASS
TrustedRealms is a pure static utility — compile-time-constant arrays and pure functions. No lifecycle, no subscriptions, no state across frames. The MANDATORY OWNER SEARCH does not apply (this is not a system, plugin, manager, or stateful helper).
Existing domain-suffix logic in ChatEnvironmentValidator.HostHasSuffix: Both implement dot-anchored suffix matching, but they serve different purposes (trust gating vs. environment validation for chat teleport) and operate at different levels (pre-parsed Uri.Host vs. raw string with span-based parsing). Consolidation would couple unrelated concerns — no action needed.
Teardown / consumption trace: N/A — no subscriptions, callbacks, connections, or disposable resources.
STEP 4 — Member audit: PASS
| Member | Consumers | Verdict |
|---|---|---|
IsTrusted(Uri) |
1 (MainSceneLoader.IsTrustedRealmAsync) |
Justified extraction for testability at a security boundary — the 78-line test suite covering 20+ attack vectors is the payoff. Not a single-use wrapper. |
IsWithinDomain(string, string) |
1 (internal to IsTrusted) |
Private helper encapsulating the boundary-anchored suffix match. Appropriate scope. |
STEP 5 — Line-level review: No blocking issues
Behavioral changes (intentional, verified by tests):
-
Trust broadened for
.zone: Three specific*.decentraland.zonehosts → all*.decentraland.zonesubdomains. Required for dynamic E2E fixture hostnames. Mitigated by HTTPS-only enforcement on domain-level trust (line 73). -
Trust tightened for cleartext
.zone:http://sdk-test-scenes.decentraland.zone(and the other two.zonehosts) was trusted before (old code was scheme-agnostic); now untrusted since domain-level trust is HTTPS-only. This is a security improvement. TestsNotTrustCleartextInsideAControlledDomainpin this. -
Case sensitivity fixed: Old code used
==(case-sensitive); new code usesStringComparison.OrdinalIgnoreCase. Correct since DNS is case-insensitive.
Security analysis:
IsWithinDomainsuffix match is correctly anchored to a label boundary (dot check athost[host.Length - domain.Length - 1]), preventing look-alikes (evildecentraland.zone). Thehost.Length > domain.Length + 1guard prevents a bare-dot prefix from matching.- HTTPS-only gate for domain trust prevents MITM on cleartext connections.
- Tests cover: look-alike domains, URI part spoofing (userinfo, query, fragment), cleartext rejection, FQDN trailing dot, unrelated hosts. Negative cases are comprehensive.
- No hardcoded secrets, no injection vectors, no input validation gaps.
Code quality:
- SCREAMING_SNAKE_CASE for
TRUSTED_HOSTS/TRUSTED_DOMAINSis consistent with project convention (IDecentralandUrlsSource.ORG_DOMAIN,ZONE_DOMAIN,ALL_DOMAINS). - Comprehensive XML documentation on every member, explaining security rationale.
- Non-nullable
Uri realmparameter correctly has no null guard (CLAUDE.md §11: "don't null-check non-null declarations"). - Test naming follows
TrustedRealmsShould.*convention with descriptive method names.
No security issues found. (Security review skill completed.)
STEP 6 — Complexity: SIMPLE
Three meaningful files changed (excluding .meta), under 150 lines of logic. Pure static utility extraction with no ECS, async, plugin, or lifecycle involvement.
STEP 7 — QA: YES
Modifies runtime trust-check behavior — the set of realms that bypass the consent prompt has changed (broadened for HTTPS .zone, tightened for cleartext .zone).
STEP 8 — Warnings: None
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Pure static utility extraction from MainSceneLoader; no ECS, async, or lifecycle changes
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Halve the comment volume on the trusted-realm host policy and state the rules as constraints rather than narration: what may join TRUSTED_HOSTS and TRUSTED_DOMAINS, why domain trust is https-only, and what the label-boundary match rejects. Drop the claim that malformed-realm handling stays with the caller -- the caller does a bare new Uri(realm), which throws. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Gateway routing is gated on the use-gateway feature flag, which a fixture run cannot set: an offline fixture serves no feature-flags backend, so the one code path the gateway mock exists to exercise was unreachable from a test launch. --gateway overrides the flag in both directions (--gateway / --gateway true force it on, --gateway false forces it off) and leaves the decision to the flag when absent. It overrides the flag, never the environment: today has no gateway and stays unrouted however the arg is set. No --debug is required, mirroring --pulse, because the arg only picks between Decentraland's own gateway and its own service hosts. The override reaches GatewayUrlsSource as a bool? rather than a resolved bool because the source reads the flag lazily -- feature flags land after it is constructed, so the value to fall back to does not exist at parse time. ResolveFeatureFlagOverride is that tri-state read; ResolveFeatureFlagArg now delegates to it, so both spellings share one definition of the semantics. Deep links can never set it. It stays out of both permitted sets, which is deny-by-default, and is named in the never-permitted tier next to pulse; the DeepLinkParamDescriptions entry makes a denial render as what it does rather than as "not recognized". A deep link that reroutes every backend url for a session is exactly the class of param that tier exists for. Verified in Unity batch EditMode: 75/75 pass with the six new cases among them, no compile errors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
STEP 1 — Context & Scope
Files read: CLAUDE.md, all 13 changed files, plus surrounding context: IDecentralandUrlsSource.cs (domain constants), DeepLinkAllowlist.cs (full — deep link security model), GatewayUrlsSource.cs (full — gateway routing), MainSceneLoader.cs (construction site lines 255–290 + IsTrustedRealmAsync lines 837–862), FeaturesRegistry.cs (all 30+ ResolveFeatureFlagArg callers), IRealmNavigator.cs (TEST_SCENES_URL). Repo cloned and checked out to PR head for rg searches.
Subsystem docs: App arguments, deep link security model (SEC-004/005/019/020/052), gateway URL routing, trusted realm consent prompt.
STEP 2 — Root-cause check
Problem: E2E fixture runs need (a) trusted realm status for dynamically-minted hostnames under decentraland.zone and (b) gateway routing without a feature-flags backend.
Does the diff fix the cause? Yes. Both features address genuine infrastructure gaps:
TrustedRealmsextracts previously-untestable inline host checks into a policy class with a domain-tier that covers dynamically-minted fixture hostnames — the only way to trust them without hardcoding each one.--gatewayadds a CLI override for a feature flag that e2e fixtures cannot serve, using the existingResolveFeatureFlagArgpattern decomposed into a three-valuedResolveFeatureFlagOverride.
PASS — no symptom masking.
STEP 3 — Design & Integration
TrustedRealms — new static utility class. Stateless (no fields, no lifecycle, no persistent collections). Pure function: Uri → bool. Not a system, plugin, manager, or controller. The extraction from MainSceneLoader is justified: it makes the trust policy unit-testable (the inline code was previously untestable) and consolidates the policy in one place with a documented two-tier model.
Owner search: IsTrustedRealmAsync in MainSceneLoader (line 848) is the sole consumer. The trusted-host checks were inline in that method. No other class creates or destroys trust state — there is no lifecycle owner to conflict with. Files searched: MainSceneLoader.cs, RealmController.cs, ChatEnvironmentValidator.cs, IRealmNavigator.cs. The static utility is the correct home.
ResolveFeatureFlagOverride — decomposes the existing ResolveFeatureFlagArg into a nullable override (the three-valued part) + fallback application. ResolveFeatureFlagArg delegates to it with ?? fallback, preserving exact behavioral identity for all 30+ existing callers in FeaturesRegistry.cs. No duplication — the new method returns bool? which models the three states (set-true, set-false, not-set) that bool cannot express. Needed because GatewayUrlsSource.enabled evaluates the flag lazily at access time, so it cannot resolve a fallback at construction.
GatewayUrlsSource.cliUseGateway — constructor-injected bool? with default null. Backward-compatible: all existing callers verified (MainSceneLoader line 277, DecentralandUrlsSourceShould tests, CreateForTest factory). The enabled property correctly short-circuits: envSupported && (cliUseGateway ?? flag) — the environment check always runs first, so today is never routed even with cliUseGateway: true. Tested by KeepTodayOffTheGatewayWhenTheArgForcesItOn.
Teardown trace: No subscriptions, event hookups, connections, or disposable resources introduced. All new code is pure computation.
PASS — no design issues.
STEP 4 — Member audit
| Member | Consumers | Verdict |
|---|---|---|
TrustedRealms.IsTrusted(Uri) |
MainSceneLoader.IsTrustedRealmAsync (1 prod), TrustedRealmsShould (tests) |
Static utility extracted from inline code; testability gain justifies the class. Not single-use indirection — the class adds a domain-tier the inline code lacked. |
TrustedRealms.IsWithinDomain(string, string) |
IsTrusted (1, private) |
Private helper, correct scope. |
ResolveFeatureFlagOverride |
MainSceneLoader (1 prod), ResolveFeatureFlagArg (1 delegation), AppArgsTests (2 tests) |
Legitimate new API: the nullable return models three states that bool cannot express. |
GatewayUrlsSource.cliUseGateway |
enabled property (1, private) |
Private field, clean. |
AppArgsFlags.GATEWAY |
MainSceneLoader, AppArgsTests, DeepLinkParamDescriptions, deep link test |
Proper constant following existing pattern. |
PASS — no single-use-merge, absent≠false, or redundant-guard issues.
STEP 5 — Line-level review
Pass A (blocking issues): None found.
- No CLAUDE.md violations: naming PascalCase ✓, no LINQ ✓, allocation-free utility (
foreachover smallstring[]) ✓, no persistent state ✓ - No bugs:
IsWithinDomainboundary math verified for all edge cases —host[host.Length - domain.Length - 1] == '.'correctly rejects suffix-lookalikes (evildecentraland.zone→ char at boundary isl, not.),host.Length > domain.Length + 1prevents out-of-bounds,EndsWithanchors to the right - No security vulnerabilities:
Uri.Hostis used (immune to userinfo/fragment/query spoofing), HTTPS-only for domain tier, deny-by-default deep link model - No performance issues:
foreachover small arrays,string.Equals/EndsWithwithOrdinalIgnoreCase - No missing error handling, no resource leaks, no detached async, no nullability violations
Pass B (design smells): None found.
- Naming correct —
TrustedRealmsnames the responsibility,IsWithinDomainnames the check - No magic values — domains reference
IDecentralandUrlsSource.ORG_DOMAIN/ZONE_DOMAINconstants - Comments explain security rationale with SEC ticket references without narrating caller behavior
- No YAGNI: both features serve the stated E2E fixture use case
Security review:
- ✅ Domain trust:
IsWithinDomaindot-boundary check rejectsevildecentraland.zone,decentraland.zone.example.com. Tests pin all negative cases including trailing-dot and multi-label-depth hosts. - ✅ HTTPS-only for domain tier: cleartext downgrade attack prevented; exact hosts (loopback) opt out since loopback has no meaningful network to attack.
- ✅ Deep link denial:
gatewaynot inPERMITTED_KEYSorWHITELISTED_REALM_PERMITTED_KEYS→ dropped by deny-by-default. TestDeepLinkDropsExecAndInfraParamsEvenForLoopbackRealmverifies. Description inDeepLinkParamDescriptionsfor consent dialog. - ✅
requireDebug: false: appropriate — the arg picks between Decentraland's own gateway and its own service hosts, no debug-mode capability unlock. - ✅
IsGatewayTransformableconstrains transformation to single-label-subdomain HTTPS URLs under.decentraland.{tld}with no port or userinfo — CLI flag cannot redirect traffic to attacker infrastructure. - ✅ No secrets, no injection vectors, no auth bypass.
Parallel sub-agent review results:
- Security agent: No issues. Verified domain boundary checks, URI spoofing resistance, HTTPS enforcement, deep link denial,
requireDebugappropriateness. - Architecture agent: No issues. TrustedRealms extraction justified,
ResolveFeatureFlagOverrideis proper decomposition,GatewayUrlsSourcechange backward-compatible, no CLAUDE.md violations. - Code quality agent: No issues in PR scope. Naming conventions followed throughout, allocation-free paths, excellent test coverage with security-regression pinning, idiomatic NUnit
[TestCase]attributes.
STEP 6 — Complexity
COMPLEX — modifies security-sensitive trust policy (realm consent-prompt bypass), public API surface (ResolveFeatureFlagOverride), and gateway URL routing logic across 13 files.
STEP 7 — QA assessment
QA_REQUIRED: YES — changes affect runtime behavior: which realms show the consent prompt (user-visible) and how gateway routing resolves (network connectivity).
STEP 8 — Non-blocking warnings
None. Main.unity is not modified.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies the trusted-realm security policy (consent-prompt bypass) and gateway URL routing, both runtime-critical and security-sensitive.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by mikhail-dcl via GitHub
Dismissed: latest Jarvis review no longer auto-approves this PR.
|
🔍 Jarvis reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging. |
…e flag
The first cut read --gateway as a boolean over the use-gateway flag, mirroring
--pulse. That controls whether the transform runs, not where it points: the
gateway host is built from --dclenv and stayed gateway.decentraland.{org,zone},
which is the one host a fixture run cannot reach. As a counterpart to the
fixture's gateway mock it was useless -- it aimed harder at production.
--gateway now takes the base to route through, and naming one forces routing on,
since that is the opt-in the flag would otherwise carry:
--gateway https://gateway.localhost
https://places.decentraland.org/api/places -> https://gateway.localhost/places/api/places
GetOriginalUrl reverses against the same base, so signed fetch keeps recovering
the un-gatewayed url it signed. The default path is untouched byte for byte: with
no arg the host is still derived from the url's own domain, so no production
routing moves. The arg outranks the flag, never the environment -- today has no
gateway and stays direct.
That makes it a host override rather than a toggle, which is why it sits in the
never-permitted deep-link tier next to gatekeeper-url and comms-adapter rather
than beside the dev-mode flags: a link that could set it would route a session's
whole supported-service traffic through a server of its choosing.
ResolveFeatureFlagOverride is reverted with the boolean it existed for.
Verified in Unity batch EditMode: 107/107 (AppArgsTest, DecentralandUrlsSourceShould,
TrustedRealmsShould), no compile errors.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
--base-domain (#9826) landed in dev and overlaps every file --gateway touches. Five conflicts, resolved as follows. GatewayUrlsSource: dev moved the gateway host off the hardcoded env domain onto gateway.{BaseDomain}, so --gateway now overrides that resolved prefix rather than a decentraland-shaped one. gatewayPrefix became the single origin both paths build from, which removes the second branch the transform carried and drops the domain-derivation the default path used to do -- dev's IsGatewayTransformable already restricts transformable hosts to this deployment's own base domain, so nothing else can reach it. The shape follows #9845, which implements the same override on top of the same dev change. The value is now validated and normalized (NormalizeGatewayPrefix): an absolute http(s) url with a host and no query or fragment, reduced to a prefix ending in '/'. Anything else ends the launch instead of being coerced, because a mistyped gateway silently routes every supported service somewhere unintended. MainSceneLoader: kept TrustedRealms.IsTrusted over dev's re-inlined host list, and kept dev's --base-domain arm alongside it. --gateway is now captured while the deep link is still deferred and reported through WarnIfCommandLineOnlyArgCameFromTheDeepLink, matching --base-domain and --eth-network: reading it later meant accepting it in the denied-params dialog would have applied it, which is not what "command line only" means. AppArgsFlags, DeepLinkAllowlist and docs/app-arguments.md: both sides kept, with gateway named in the same never-permitted tier as the other infrastructure- pointing params. Verified in Unity batch EditMode: 203/203 across AppArgsTest, DecentralandUrlsSourceShould, TrustedRealmsShould, ChatEnvironmentValidatorShould and RealmLaunchSettingsShould -- dev's base-domain cases included -- no compile errors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Throwing ArgumentException from GatewayUrlsSource's constructor made a mistyped flag surface as a crash in bootstrap, while the flag next to it reports what is wrong and ends the launch. Same class of mistake, so same treatment. Validation moves to the capture site as CaptureGatewayArg, mirroring CaptureEthNetworkArg: it reports which value was rejected and what was expected, and returns false so the caller exits. The rule keeps one home -- GatewayUrlsSource.TryNormalizeGatewayPrefix -- and the constructor now takes the normalized prefix the launch path already validated. Verified in Unity batch EditMode: 205/205, no compile errors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
An e2e fixture serves its global comms adapter over cleartext (`fixed-adapter:signed-login:http://127.0.0.1:8080/...`), which the Archipelago fork had no tier for: the launch died on "Cannot determine the protocol from the about url". `--accept-untrusted-realm` widens the fixed-room tier to a cleartext adapter whose host is loopback. Remote http stays rejected, https/wss are untouched, and the flag is absent from the deep-link allowlist because it lowers a transport guarantee — never a link's call to make. Loopback detection becomes one implementation. `Utility.Networking.LoopbackUrls` reads the host out of the authority as a span — no `Uri` parse, no `Substring` — so it costs nothing on the per-request paths that used to duplicate it, and it rejects the lookalikes a prefix match let through (`127.0.0.1.example.com`, `127.0.0.1@example.com`). It replaces `WebRequestUtils.IsLocalhost`, `McpHttpServer`'s origin check (which also never matched an IPv6 loopback origin, since `Uri.Host` brackets it) and the two loopback entries in `TrustedRealms`. `DeepLinkAllowlist` deliberately keeps `Uri.IsLoopback`: its semantics are wider and pinned by its own cases. `RefinedAdapterAddresses` now strips the handshake pre-info in front of an http adapter too, as it already did for https and wss. Without that the fork picks `FixedConnectiveRoom` and the room signs a fetch of "fixed-adapter:signed-login:http://..." verbatim. The three near-identical strip methods collapse into one pass cutting at the earliest scheme, so a scheme inside the url's own query no longer truncates it. Verified: Unity 6000.4.0f1 batch EditMode, 251/251 — LoopbackUrlsShould, ArchipelagoProtocolSelectionShould, RefinedAdapterAddressesShould, AppArgsTest, TrustedRealmsShould, McpHttpServerShould, DecentralandUrlsSourceShould. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two conflicts, both the same shape: dev's abgen pipeline (#9835) and this branch's --gateway each appended a trailing optional parameter to GatewayUrlsSource. Kept both, with the base-class parameter in the position the base constructor declares it and the derived-only one last: GatewayUrlsSource(..., customBaseDomain, abgenPipelineForced, cliGatewayPrefix) MainSceneLoader passes both at the call site. No other call site is positional past customBaseDomain, so the order breaks nothing. The two features compose without further work: an abgen url resolves as FeatureFlagsDependent, which GatewayUrlsSource.RawUrl already declines to gateway-rewrite.
This comment has been minimized.
This comment has been minimized.
--gateway aims every supported service at one origin, and adding WorldServer to that set moved world realms onto it too. ChatEnvironmentValidator only accepted hosts under BaseDomain, so a local e2e fixture — whose gateway is loopback by design — failed every /goto <world> with "you cannot teleport to realms outside decentraland.org", rejecting the realms the client was itself routing. Expose the origin as IDecentralandUrlsSource.GatewayOrigin, null unless routing is actually on, and accept it ahead of the domain check. Naming a gateway is command line only, so this adds no link-reachable surface, and the origin keeps its trailing '/' — the authority boundary that stops http://127.0.0.1:8080.attacker.com/ from matching http://127.0.0.1:8080/. Today still refuses every realm: its guard stays ahead of both checks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pentreathm
added
force-build
This comment has been minimized.
This comment has been minimized.
pentreathm
added
force-build
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The three local rooms are started together and reported as one boolean, so both the health check failure and the 30s startup timeout above it read the same whichever room is stuck. IRoomHub now reports each started room's state, connect attempt and connection loop health, and both failure paths carry it: a room still handshaking shows as Starting/None, a refused one as Error, and a scene-banned one as ForbiddenAccess. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dev removed the Today environment (#9885), which is what the gateway's environment gate and the chat validator's first check existed for, so the resolutions drop both and keep the --gateway origin on top of dev's shape: - GatewayUrlsSource: dev's non-nullable prefix trio, built unconditionally, with the cli origin still overriding gateway.{BaseDomain}; enabled is now "an origin was named or the flag says so", no env gate. The null guard in TransformToGateway goes with it - the field can no longer be null. - ChatEnvironmentValidator: the gateway-origin check stays, the Today rejection and the environment ctor argument go. - Tests: the two Today cases drop, the gateway cases lose the environment argument, and the gatewayOrigin overload of the urls-source helper stays. - RequestEnvelope: keep Utility.Networking for the local certificate handler, take dev's removal of the unused UnityEngine.Pool. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
WorldCommsAdapter was missing from GatewayUrlsSource, so a session started with --gateway signed its scene-room handshake for the fixture and sent it to the public worlds-content-server, which answered 404. Its siblings WorldServer and WorldContentServer were already routed; this leaves an e2e run reaching the internet for the one request that decides whether a World's comms come up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
|
Claude finished @mikhail-dcl's task in 3m 53s —— View job Dependency & supply-chain security review
STEP 1 — Dependency changesExactly one, in
No other manifest entries changed. No STEP 1.5 — Registry metadata: could not be gatheredI have no STEP 2 — AssessmentA. Provenance — [PR] Decentraland-owned org, SHA-pinned, same repo as the existing pin. Bumping a first-party fork by SHA is the right shape. [UNKNOWN] the actual commit range, its authors, and whether the fork picked up upstream commits alongside the Decentraland change. B. Capability — this is the part that needs a human 🟡 The bump is not cosmetic. Commit
[Inference] the name maps to LiveKit's
Both comments assert a security property that lives entirely inside the unreviewable dependency. The client-side layers are sound on their own ( C. Shipping impact — [PR] runtime, all desktop platforms. The relaxed path is gated behind a CLI-only flag, so default retail behaviour is unchanged. D/E. Maintenance / transitive — [UNKNOWN] by the same access limitation. No new transitive deps. Pre-existing (not introduced here, but worth a separate ticket): STEP 3 — Risk classification
MEDIUM, not LOW — first-party and SHA-pinned, but it carries a transport-policy change whose safety argument is stated in this PR's comments and verifiable only in the dependency. Not HIGH — no unpinned ref, no unknown publisher, no dynamic loading, and the capability is proportionate to the stated e2e-fixture purpose. Recommendation: before merge, have a reviewer with repo access confirm on Non-dependency findings from the same diff1.
That was true when written (
The bypass itself looks correctly scoped (loopback-only, opt-in, default off), so this is a documentation accuracy problem on a security-relevant flag rather than a code defect — but for a flag whose whole job is lowering guarantees, the doc understating its blast radius is worth fixing before merge. Fix this → 2. Observations (no action needed):
DEPENDENCY_REVIEW: NEEDS_ATTENTION Not a block: the pin is immutable, first-party, and proportionate. It needs a human because the security argument for the ICE change rests on behaviour inside |
|
PR #9822, run #33516051543 Overall: ✅ no significant changes Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Apple M1
|
3 participants
What does this PR change?
Client-side support for running the InWorld suite against a local or ephemeral Catalyst fixture (server side: explorer-e2e-infra#15).
TrustedRealms— the startup gate that decides whether a custom realm needs the untrusted-realm consent prompt was a run of inline host comparisons inMainSceneLoader. It moves into one policy with two tiers: exact hosts, and domains whose every subdomain is Decentraland-controlled. The domain tier is what makes the fixtures trusted at all — their hostname is minted per run (f-{id}.e2e-fixtures.decentraland.zone), so it cannot be enumerated as an exact host. Onlydecentraland.zonegets domain trust, only over https; production stays per-host so no single subdomain or dangling DNS record under it becomes a consent-free realm switch. A match now also short-circuits thelambdas/contracts/serverslookup.--gateway <url>— routes every supported backend url through the given gateway base instead ofgateway.decentraland.{env}, which is the host a fixture run cannot reach:Naming a base also forces routing on, since that is the opt-in the
use-gatewayfeature flag would otherwise carry — a fixture serves no feature-flags backend.GetOriginalUrlreverses against the same base, so signed fetch still recovers the url it signed. With no arg the default path is unchanged byte for byte, and the arg outranks the flag but never the environment (todaystays direct). Command line only: it aims a session's whole supported-service traffic at the named host, so it sits in the never-permitted deep-link tier next togatekeeper-urlandcomms-adapter.Test Instructions
Expected result: unchanged behaviour on a normal launch — production realms still route directly and a non-Decentraland
--realmstill raises the consent prompt.Steps (the new paths):
--realm https://f-<id>.e2e-fixtures.decentraland.zone→ loads with no consent prompt and nopeer.decentraland.orglookup.--gateway https://gateway.localhost→ supported services are requested ashttps://gateway.localhost/{subdomain}/…, with theuse-gatewayflag off.decentraland://?realm=http://127.0.0.1:8000&gateway=https://evil.example→gatewayis dropped and listed in the denied-params dialog.Verified: Unity batch EditMode, 107/107 (
AppArgsTest,DecentralandUrlsSourceShould,TrustedRealmsShould), no compile errors.🤖 Generated with Claude Code