fix: allow explicitly opted-in local HTTP comms adapters - #9876
fix: allow explicitly opted-in local HTTP comms adapters#9876pentreathm wants to merge 2 commits into
Conversation
decentraland-bot
added
the
ext-contribution
🚦 CI Status
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Warnings not reduced: 12566 => 12618 — remove at least 53 warnings to merge. No warnings in files changed by this PR — showing general ones you can remove to unblock (50 of 12618)All Unity tests passed ✅
|
|
Slack notification sent to #explorer-ext-contributions for external review. |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
STEP 2 — Root-cause check
Problem: ForkGlobalRealmRoom.ChooseRoom() throws InvalidOperationException when a local fixture realm returns a signed comms adapter over HTTP (fixed-adapter:signed-login:http://127.0.0.1:8080/...), because only WSS and HTTPS schemes were recognized.
Does the diff fix the cause? Yes. The diff adds a new HTTP-loopback branch to ChooseRoom() that routes the adapter to the existing FixedConnectiveRoom. The branch requires both an explicit CLI opt-in (--accept-untrusted-realm) and a validated loopback host. This is a proper feature addition for a previously unsupported protocol path, not a symptom workaround.
PASS ✅
STEP 3 — Design & integration
Owner search: ForkGlobalRealmRoom already owns the protocol-selection logic in ChooseRoom(). No new long-lived unit is introduced — the diff adds a branch to the existing method and a static helper. The composition root (CommsContainer.Create) already constructs the ForkGlobalRealmRoom via IArchipelagoIslandRoom.NewDefault; the flag is threaded from IAppArgs → bool at that boundary. No lifecycle duplication, no reconciliation, no persistent state outside ECS.
Assembly boundary: The diff adds using Global.AppArgs; to IArchipelagoIslandRoom.cs (in the Multiplayer.Connections assembly). This is used only in the factory method NewDefault to read IAppArgs. Since CommsContainer.cs (which calls NewDefault) already depends on Global.AppArgs, and the factory is the composition seam where the flag is resolved, this is a justified boundary extension.
Rename accuracy: httpsRoomFactory → fixedRoomFactory is correct: the factory now produces FixedConnectiveRoom instances for both HTTPS and HTTP-loopback adapters, so the old name was too narrow.
Teardown/consumption trace: No new subscriptions, events, connections, or disposables are introduced. The bool allowInsecureLocalHttp is a constructor-injected immutable value. No teardown needed.
PASS ✅
STEP 4 — Member audit
| Member | Consumers | Assessment |
|---|---|---|
IsLoopbackHttpAdapter(string, bool) |
ChooseRoom() + tests |
internal static — appropriate for testability. Encapsulates URL parsing and security validation; not a single-use predicate that should be inlined. |
allowInsecureLocalHttp field |
IsLoopbackHttpAdapter call in ChooseRoom() |
Constructor-injected config value, not a derived predicate. Legitimate. |
fixedRoomFactory field (renamed) |
ChooseRoom() (two branches: HTTPS and HTTP-loopback) |
Correctly renamed to reflect broader usage. |
IAppArgs? appArgs parameter on NewDefault |
CommsContainer.Create |
Nullable optional — appropriate since NewDefault is an interface default method and other callers may not have IAppArgs. The == true pattern handles null correctly. |
No issues found.
STEP 5 — Line-level review
A. Blocking-issue categories
No P0 or P1 issues found across all categories (bugs, security, performance, error handling, resource leaks, nullability, detached async, false-intent conditions).
B. Design, encapsulation & resource smells
No issues found. The change is well-scoped within the existing owner, naming is accurate, no magic values, no leaked encapsulation.
P2 findings are attached as inline comments.
Security review
- ✅ Deep-link denial:
accept-untrusted-realmis not inPERMITTED_KEYSorWHITELISTED_REALM_PERMITTED_KEYSinDeepLinkAllowlist.cs, so it is automatically denied. The testDeepLinkDropsInternalFlagsverifies this. - ✅ Default closed:
allowInsecureLocalHttpdefaults tofalse;IsLoopbackHttpAdapterreturnsfalseimmediately when disabled. - ✅ Loopback validation: Uses
Uri.TryCreate+Uri.IsLoopback— checks the parsed hostname, not a string match, so userinfo and subdomain attacks are rejected. - ✅ Scheme validation: Double-checks
uri.Scheme == Uri.UriSchemeHttpafter theContainspre-filter. - ✅ No SSRF risk: Traffic is restricted to loopback addresses.
- ✅ No DNS rebinding risk:
Uri.IsLoopbackchecks the literal hostname in the URL, not a DNS-resolved address. - ✅ No secrets or credentials committed.
STEP 6 — Complexity
COMPLEX — Modifies the comms protocol selection path (ForkGlobalRealmRoom.ChooseRoom) and extends a shared interface (IArchipelagoIslandRoom.NewDefault). Introduces a security-sensitive feature gating HTTP acceptance behind a CLI flag with loopback validation.
STEP 7 — QA assessment
QA_REQUIRED: YES — Changes runtime comms connection selection code in Explorer/. The new code path affects which room type is instantiated for local fixture development.
STEP 8 — Non-blocking warnings
main rather than dev. Unity Explorer PRs typically target dev — verify this is intentional (e.g., a hotfix needed directly on main).
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies comms protocol selection (ForkGlobalRealmRoom.ChooseRoom) and shared interface (IArchipelagoIslandRoom.NewDefault) with security-sensitive loopback validation
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested via Slack
|
PR #9876, run #33017651983 Overall: ✅ no significant changes Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Apple M1
|
2 participants
What does this PR change?
Allows the Archipelago global-room selector to use the fixed comms room when a local fixture returns a signed adapter over HTTP, for example
fixed-adapter:signed-login:http://127.0.0.1:8080/....The fallback is fail-closed:
--accept-untrusted-realm.localhost,127.0.0.1, or::1).decentraland://deep links.Test Instructions
Automated tests
The new protocol-selection tests cover loopback allow/deny cases, remote lookalikes, HTTPS, and the explicit opt-in requirement. The existing deep-link allowlist tests also verify that
accept-untrusted-realmis dropped from deep links.Run the Unity test suite in CI, or locally with the project Unity version:
Manual fixture test
Start the local fixture with the Unity Explorer helper from
explorer-e2e-infra:Expected result: the local signed HTTP comms adapter is selected as a fixed room instead of failing with
Cannot determine the protocol from the about url.Quality Checklist
6000.4.0f1and run in CI.