chore: deeplink non-whitelist params confirmation dialog - #9527
Conversation
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
pravusjif
changed the title
|
Warnings count reduced: 14034 => 14010 |
|
All Unity tests passed ✅
|
pravusjif
added
force-build
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — chore: deeplink non-whitelist params confirmation dialog
STEP 1 — Context & Scope
Loaded CLAUDE.md, review-instructions.md. Inspected the full diff across 10 changed files (+2555 −38), cloned the PR branch, and read the surrounding files in full: ApplicationParametersParser.cs, MainSceneLoader.cs, DeepLinkParamsWarningView.cs, ViewBase.cs, and the existing ApplicationGuards/ directory. Verified the prefab YAML to confirm rich-text and TMP settings. Searched for all consumers of DeniedDeepLinkParams / ApplyDeniedDeepLinkParams across the codebase.
STEP 2 — Root-cause check
✅ PASS. The deep-link allowlist previously dropped denied params silently, which broke debug workflows that rely on deep-link flags (dclenv, skip-version-check, etc.). The PR replaces the silent drop with an explicit user consent dialog — the user can exit or accept the risk. This is the correct fix: instead of losing params or blindly accepting them, the user makes an informed decision. The allowlisted-param path (Tier 1 and Tier 2 in ProcessDeepLinkParameters) is unchanged.
STEP 3 — Design & integration
✅ PASS.
Owner search — DeepLinkParamsWarningView: This is a stateless UI view (extends ViewBase, IView) placed in DCL/ApplicationGuards/ApplicationDeepLinkGuard/ — consistent with existing guards: MinimumSpecsScreenView, LauncherRedirectionScreenView, LivekitHealthGuardView, BlockedScreenView. The view holds no persistent state; it receives params via SetDeniedParams(), formats them for display, and exposes two buttons. The lifecycle is fully owned by MainSceneLoader.ShowDeniedDeepLinkParamsConfirmationAsync, following the same pattern as the existing ShowSingleRunningInstancePopupAsync. No duplicate lifecycle.
Owner search — deniedDeepLinkParams: Lives on ApplicationParametersParser as a private Dictionary exposed as IReadOnlyDictionary via the [AutoInterface]-generated IAppArgs interface. The parser is the natural owner — it processes the deep link, knows which params passed/failed the allowlist, and owns the merge of accepted params into appParameters. The data flow is clean: parser captures → MainSceneLoader shows dialog → parser merges on consent.
Placement: The dialog runs immediately after InitializeDeepLinks() and before any consumer reads app-args (environment, launch settings, containers, version check). The AddressablesProvisioner was hoisted earlier in the flow to be available for the dialog — this is a lightweight constructor with no side effects.
Teardown/consumption trace:
popup.ContinueButton.onClick.AddListener(...)/popup.ExitButton.onClick.AddListener(...)— on Continue,Destroy(popup.gameObject)cleans up both listeners. On Exit,ExitUtils.Exit()terminates the process. On cancellation (ctfires),AttachExternalCancellationthrows, butctisdestroyCancellationTokenso Unity handles cleanup. ✅UniTaskCompletionSource<bool> decision— consumed byawait decision.Task, completes on button click. ✅deniedDeepLinkParams— populated inInitializeDeepLinks(), cleared inApplyDeniedDeepLinkParams()on consent, or left unmerged if the user exits. ✅
STEP 4 — Member audit
DeniedDeepLinkParams (property on ApplicationParametersParser): 2 production consumers (MainSceneLoader lines 227 and 495) + 3 test consumers. Legitimate public API for the consent flow; not single-use since it is checked both to decide whether to show the dialog and to populate the dialog content.
ApplyDeniedDeepLinkParams() (method): 1 production consumer (MainSceneLoader line 505) + 1 test consumer. Legitimate public method; the consent semantics require a separate method from the constructor/init path.
ContinueButton / ExitButton / SetDeniedParams() on DeepLinkParamsWarningView: 1 production consumer each (MainSceneLoader). Standard for view components — the consumer is the presenting code.
No issues found — no single-use merges needed, no absent-vs-false conflation, no redundant guards.
STEP 5 — Line-level review
P2 findings:
- [P2] Key display length not capped (
DeepLinkParamsWarningView.cs:48) —AppendSingleLine(sb, key, key.Length)passeskey.LengthasmaxLength, so keys are never truncated. A crafted deep link with an extremely long query-param name (e.g. hundreds of chars) would produce an unwieldy dialog. Values are correctly capped atMAX_SHOWN_VALUE_LENGTH(48); keys should be capped similarly. The TMP auto-sizing (m_enableAutoSizing: 1, min 10pt) partially mitigates this, but a very long key would still reduce readability. See inline comment with suggestion.
Security verification (per security-review skill):
- TMP injection: ✅ Verified
m_isRichText: 0on the Description TMP component (fileID: 8007467556033942281in the prefab YAML). Rich text disabled — tags like<b>,<color>render as literal text. - Control character injection: ✅
AppendSingleLineflattens all control characters to spaces, preventing fake line entries or UI manipulation. - Value truncation: ✅ Values capped at 48 chars with
...ellipsis. - No secrets committed: ✅
- No auth/authz changes: ✅
- Input handling: ✅ Deep link params are untrusted input; sanitization is thorough (three layers: allowlist gate → control-char flattening → rich-text disabled).
Additional observations (non-blocking, informational):
- The
#if UNITY_EDITORrestructuring inShouldForceSingleRunningInstanceis functionally equivalent and eliminates the unreachable-code path in editor builds. Clean. - The
simulatedMemorysimplification,ct→cancellationTokenrename,splashScreen→splashrename, andswitch→ ternary refactoring are all correct and improve readability. - The
// ReSharper disableadditions (UnusedMember.Localon Unity lifecycle methods,EmptyGeneralCatchClause,InconsistentNamingon DTO field) follow project conventions. - The fallback in
ShowDeniedDeepLinkParamsConfirmationAsyncwhen the prefab fails to load (return true→ skip dialog, keep params dropped) correctly preserves pre-dialog behavior. - Tests cover the three main scenarios: capture-without-apply, apply-on-consent, and no-denied-params. Good coverage.
STEP 6 — Complexity
COMPLEX — Introduces new public API surface on ApplicationParametersParser/IAppArgs, new addressable prefab with serialized reference on MainSceneLoader, and modifies the startup flow ordering (hoisted AddressablesProvisioner, added a consent gate before any app-arg consumer).
STEP 7 — QA assessment
YES — Runtime code with a new user-facing UI dialog at application startup; affects deep-link launch behavior.
STEP 8 — Non-blocking warnings
⚠️ Main scene modified (Explorer/Assets/Scenes/Main.unity). This file is rarely changed intentionally — thedeepLinkParamsWarningPopupPrefabserialized reference addition is expected; theuseLocalAssetBundles: 0line appears to be a Unity editor serialization side-effect from opening the scene.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Adds new public API surface to IAppArgs, new addressable prefab, and modifies the startup flow ordering in MainSceneLoader.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
|
🔍 Jarvis reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging. |
pravusjif
removed
force-build
DafGreco
left a comment
DafGreco
left a comment
There was a problem hiding this comment.
✔️ PR reviewed and approved by QA on both platforms following instructions playing both happy and un-happy path
Regressions for this ticket had been performed in order to verify that the normal flow is working as expected:
- [ ✔️] Backpack and wearables in world
- [✔️ ] Emotes in world and in backpack
- [✔️ ] Teleport with map/coordinates/Jump In
- [ ✔️] Chat and multiplayer
- [ ✔️] Profile card
- [ ✔️] Camera
- [ ✔️] Skybox
Evidence :
github-project-automation
Bot
moved this from With QA / Awaiting Review
to In Progress
in Explorer Alpha
4 participants
The deep-link allowlist (deny-by-default) silently drops any param that is not explicitly permitted, which broke the daily debug workflows that rely on deep links (
dclenv,skip-version-check, etc.). This PR adds a confirmation dialog at application start — styled like the "Update Required" screen — that enumerates the denied params with a one-line description of what each one does and lets the user either exit or explicitly accept the risk and continue with those params applied. App-arg (argv) launches are unaffected; only the deep-link path is gated.The dialog is modelled on a browser certificate warning: Exit Application is the highlighted default, and continuing takes two deliberate steps — Advanced only reveals the responsibility statement plus a de-emphasised Continue Anyway (Unsafe) button, so the warning cannot be dismissed with a single reflex click.
Technical description
ApplicationParametersParser: denied deep-link params are now captured (key + value) intoDeniedDeepLinkParamsinstead of being lost;ApplyDeniedDeepLinkParams()merges them into the app-args on consent. NewProcessDeepLinkParameters(string, Dictionary?)overload collects them; the runtime bridge path (DeepLink.cs) is unchanged and still silently drops.MainSceneLoader: the dialog runs right afterInitializeDeepLinks(), before any consumer reads the app-args (environment, launch settings, containers, version check), so consented params actually take effect. Since the MVC manager doesn't exist yet at that point, the prefab is instantiated directly (same pattern as the single-instance popup) and awaited viaUniTaskCompletionSource. Exit callsExitUtils.Exit().DCL/ApplicationGuards/ApplicationDeepLinkGuard/:DeepLinkParamsWarningView+DeepLinkParamsWarningScreen.prefab(cloned fromVersionUpdateScreen.prefab; three buttons — Exit as the red primary, Advanced, and a hidden Continue). Hardening: rich text is disabled on the description and control characters are flattened, so a crafted link cannot inject TMP markup or fake dialog lines; values are truncated, and the list is capped at 6 entries (with a count of the rest) so a padded link cannot push the warning over the buttons.DeepLinkParamDescriptions(next toDeepLinkAllowlist): key → one-line, plain-language effect used by the dialog, with an explicit "not recognized by this version" fallback.MainSceneLoader(Main.unity) + Addressables entry in the UI group.skip-version-checkskips the "Update Required" screen; without denied params the version guard behaves as before.QA Test Instructions
DecentralandLauncherLight/Latest/directory and rename the explorer in there (suggestion: just add-originalat the end)decentraland://?realm=pravus.dcl.eth&skip-version-check=true&self-preview-builder-collections=a2041268-189e-4cef-902d-70272aed077cdecentraland://?skip-version-check=true&self-preview-builder-collections=3062136a-065d-4d94-b28c-f57d6ef04860,371a0f06-d200-42ab-97d2-620188fc1bbf,a2041268-189e-4cef-902d-70272aed077c&position=100,100&skip-auth-screen=truedecentraland://?realm=sdk7testscenes.dcl.eth&position=88,-10&dclenv=zone&skip-version-check=true&multi-instance=true