fix: require destination consent on event notification clicks - #9590
fix: require destination consent on event notification clicks#9590mikhail-dcl wants to merge 2 commits into
Conversation
Clicking an "event starting" notification changed the realm and teleported the player straight from the `realm`/`position` query params of `notification.Metadata.Link`. That link is event content authored by whoever created the event through the open Events API, so any wallet could pick where a subscriber lands. Every sibling path (scene `changeRealm()`, deep link, chat world link) already asks for consent first; this one did not. `NotificationHandler` now issues the existing consent prompt instead of navigating: `ChangeRealmPromptController` for a world destination (carrying the target parcel, as the deep link path does) and `TeleportPromptController` for a parcel-only destination. Approving the prompt is what performs the navigation, so declining moves the player nowhere. The realm name is also validated with `IsEns()` — normalized first, since the check is case sensitive on the ".eth" suffix — which keeps the destination on the official world server instead of an arbitrary catalyst. Parsing of the link is now total. `new Uri(...)`, `int.Parse` and `split[1]` could each throw on a crafted link, and `NotificationsBusController.ClickNotification` invokes its subscribers as a multicast delegate with no try/catch, so a throw escaped into the click dispatch and dropped the later subscribers for that notification type. Malformed input is now reported via `ReportHub.LogWarning` and the click is ignored. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
New build in progress, come back later! |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
STEP 1 — Context & Scope
Files reviewed: NotificationHandler.cs, CommunitiesPlugin.cs, NotificationHandlerShould.cs, .meta
Surrounding context loaded: NotificationsBusController.cs (multicast delegate dispatch, subscribe/unsubscribe API), CancellationTokenExtensions.cs (SafeRestart, SafeCancelAndDispose), EnsExtensions.cs (IsEns regex), ChangeRealmPromptController.Params.cs, TeleportPromptController.Prams.cs, DeepLinkHandleImplementation.cs, RestrictedActionsAPIImplementation.cs, MVCManagerMenusAccessFacade.cs, IMVCManager.cs, EventStartedNotification.cs.
STEP 2 — Root-cause check: PASS ✅
Problem: clicking an "event starting" notification navigated the user to an attacker-chosen destination with no consent prompt. The link is authored by whoever created the event through the open Events API.
Fix: replaces direct IRealmNavigator calls with the existing consent-prompt controllers (ChangeRealmPromptController / TeleportPromptController), making approval the precondition for navigation — the same arrangement scene changeRealm(), deep links, and chat world links already use. Additionally hardens all parsing to be total (no exceptions can escape the multicast delegate dispatch). This fixes the root cause, not a symptom.
STEP 3 — Design & integration: PASS ✅
NotificationHandler is not a new class — it already existed and managed the EVENTS_STARTED notification click subscription. The diff changes its internals only: from direct navigation via IRealmNavigator to consent-prompted navigation via IMVCManager.ShowAsync. No new lifecycle, no new long-lived unit.
Owner search: NotificationHandler is created and disposed by CommunitiesPlugin (the lifecycle owner). Subscription in constructor, disposal in CommunitiesPlugin.Dispose(). No parallel mechanism introduced.
Pattern consistency verified: The usage of ChangeRealmPromptController.IssueCommand and TeleportPromptController.IssueCommand matches the existing patterns in DeepLinkHandleImplementation.cs (line 127–129), RestrictedActionsAPIImplementation.cs (lines 280, 286), and MVCManagerMenusAccessFacade.cs (lines 104, 107). The deep link handler uses the identical signature: new ChangeRealmPromptController.Params(string.Empty, realm, position).
Teardown trace:
eventStartsCts: created in field initializer →SafeRestart()on each click →SafeCancelAndDispose()inDispose(). ✅SubscribeToNotificationTypeClick(EVENTS_STARTED, EventStartSoonClicked)in constructor → no matchingUnsubscribeFromNotificationTypeClickinDispose(). See P2 note below. (Pre-existing — identical to old code.)
STEP 4 — Member audit: N/A
No new public members added. All new methods (TryParseDestination, TryParseParcel, ConfirmDestinationAsync) are private / private static.
STEP 5 — Line-level findings
[P2] Missing unsubscribe in Dispose() (pre-existing)
The constructor subscribes to NotificationsBusController (line 32) but Dispose() (line 35–36) only cancels/disposes the CTS — it never calls UnsubscribeFromNotificationTypeClick. After disposal, if the singleton bus fires a click event, EventStartSoonClicked will execute against disposed state. SafeRestart() catches ObjectDisposedException and creates a new CTS (which then leaks). This is pre-existing behavior (the old code had the same gap), and the practical impact is negligible since CommunitiesPlugin.Dispose() and singleton teardown happen together at shutdown. Non-blocking, but worth closing in a follow-up:
public void Dispose()
{
NotificationsBusController.Instance.UnsubscribeFromNotificationTypeClick(
NotificationType.EVENTS_STARTED, EventStartSoonClicked);
eventStartsCts.SafeCancelAndDispose();
}No other line-level issues found. All parsing uses Try-pattern methods, async handling follows CLAUDE.md §9 (ct.IsCancellationRequested before work, catch (OperationCanceledException) + catch (Exception) → ReportHub.LogException), and ReportHub is used for logging.
STEP 6 — Complexity: COMPLEX
Changes dependency injection (IRealmNavigator → IMVCManager), introduces new async flow (UniTaskVoid), and modifies a security-sensitive notification click handler.
STEP 7 — QA: YES
Affects user-facing runtime navigation behavior when clicking event notifications.
STEP 8 — Non-blocking warnings: None
Main scene not modified.
Security review
- Consent mechanism: Complete. Every navigation path goes through
mvcManager.ShowAsync()— no silent fallback. ✅ - Input validation:
Uri.TryCreate(absolute URI only),int.TryParsewithInvariantCulture,IsEns()regex (^[a-zA-Z0-9.]+\.eth$) afterToLowerInvariant(). Thorough. ✅ - ENS regex: Character set (alphanumeric + dot) is restrictive enough — prevents URLs, hosts, aliases, and parameter pollution (commas from
ParseQueryStringduplicate-key joining are not in the charset). ✅ - Exception totality:
TryParseDestinationis fully total.EventStartedNotificationMetadatais a struct (value type, can't be null), andLinkbeing null is handled byUri.TryCreate(null, ...)→ false. ✅ - Async exception handling:
ConfirmDestinationAsynccatchesOperationCanceledExceptionand logs general exceptions viaReportHub.LogException. Follows CLAUDE.md §9. ✅ - SSRF / redirect: Not applicable — realm/position values are passed to UI prompt controllers, not used for HTTP requests. ✅
- No secrets, no sensitive data exposure. ✅
Test coverage
20 EditMode tests covering:
- Consent prompts for world, parcel, and world-only destinations
- Case normalization of realm names
- Prompt-pending and prompt-declined scenarios (consent property verification)
- 12 crafted-link rejection cases (null, empty, malformed URI, relative URI, invalid position formats, non-ENS realm, URL realm, alias realm, host realm, no destination)
- Multicast delegate survival (later subscribers still fire after a crafted link)
- Foreign payload type guards
Coverage is thorough and directly validates the security properties.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies security-sensitive notification handler, changes DI from IRealmNavigator to IMVCManager, introduces new async UniTaskVoid consent flow
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
| this.decentralandUrlsSource = decentralandUrlsSource; | ||
| this.mvcManager = mvcManager; | ||
|
|
||
| NotificationsBusController.Instance.SubscribeToNotificationTypeClick(NotificationType.EVENTS_STARTED, EventStartSoonClicked); |
There was a problem hiding this comment.
[P2 — pre-existing] This subscription has no matching UnsubscribeFromNotificationTypeClick in Dispose(). After disposal, if the singleton bus fires a click, EventStartSoonClicked runs against disposed state — SafeRestart() silently creates a new CTS that then leaks. Practically harmless since handler and bus are torn down together at shutdown, but worth closing for correctness in a follow-up:
| NotificationsBusController.Instance.SubscribeToNotificationTypeClick(NotificationType.EVENTS_STARTED, EventStartSoonClicked); | |
| NotificationsBusController.Instance.SubscribeToNotificationTypeClick(NotificationType.EVENTS_STARTED, EventStartSoonClicked); |
(The fix belongs in Dispose() — add NotificationsBusController.Instance.UnsubscribeFromNotificationTypeClick(NotificationType.EVENTS_STARTED, EventStartSoonClicked); before SafeCancelAndDispose(). Can't target that line with a suggestion since it's outside the diff.)
|
🔍 Jarvis reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging. |
|
Warnings not reduced: 13940 => 13940 — remove at least 1 warning to merge. Warnings/errors in files changed by this PR (10) |
|
All Unity tests passed ✅
|
🚦 CI Status
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Warnings not reduced: 13156 => 13165 — remove at least 10 warnings to merge. Warnings/errors in files changed by this PR (10)
|
|
PR #9590, run #32032889896 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
2 participants
Pull Request Description
What does this PR change?
Clicking an "event starting" notification changed the user's realm and teleported them straight away, using
realm/positionquery params parsed out ofnotification.Metadata.Link. That link is event content authored by whoever created the event through the open Events API, so any wallet could create an event and choose where a subscribed victim lands — on one click, with no confirmation. Fixes SEC-076.The realm host stays bound to the official world server, so this is a lure/phishing primitive rather than an arbitrary-catalyst takeover — the bounded cousin of SEC-003 (scene
changeRealm) and SEC-004 (deep-link realm), both already fixed.Changes
ChangeRealmPromptControllerfor a world,TeleportPromptControllerfor a bare parcel. Approving the prompt is what navigates, which is the same arrangement scenechangeRealm(), deep links and chat world links already use, so declining goes nowhere structurally rather than conditionallyrealmmust name an ENS world (IsEns()), normalized to lower case before validating — the regex is case-sensitive on.eth, so a legitimateMyWorld.DCL.ETHwould otherwise be rejected and, downstream, get a second.dcl.ethappendedUri.TryCreate/int.TryParse/ a comma-count check replacenew Uri(...),int.Parse(split[0])andsplit[1]. A rejected link logs a warning and returns instead of throwingIRealmNavigatorandIDecentralandUrlsSourceare no longer needed by the handler; it takesIMVCManager. No plugin signature changed — both are still used elsewhere inCommunitiesPluginWhy the parsing matters beyond tidiness:
NotificationsBusController.ClickNotificationinvokes its subscribers as a multicast delegate with no try/catch, so a throw here escaped into the click dispatch and dropped every subscriber registered after this one for that notification type. A crafted event was a cheap way to break notification clicking.Two intentional behaviour changes, both reviewed and accepted:
allowsSpawnPointerOverride: true, so the world's own spawn point wins instead of forcing parcel(0,0). This matches deep-link semantics./goto, which posts a visible nearby-chat line. That is inherited from the existing prompt callback, not added here.Otherwise the approved path is argument-for-argument equivalent to the old direct call, and it additionally inherits the prompt callback's
ValidEnvironmentandIsAlreadyOnRealmchecks.Not in scope: guarding
ClickNotificationitself. Because subscribers share one multicast delegate perNotificationType, one throwing subscriber permanently disables every later one for that type, andSubscribeToAllNotificationTypesClickputs such a subscriber in all ~30 chains. Fixing it properly means iteratingGetInvocationList(), and the other ~20 subscribers are unaudited — its own change. Theeventsandnotifications-workershalves of the finding (validating event-supplied destinations at the source) are separate repos.Test Instructions
Steps (standard run):
Steps (fresh account):
Prerequisites
EVENTS_STARTEDnotification is delivered — the notification only arrives for events the account is interested in*.dcl.ethname) for rows 1–3, and to a Genesis parcel for rows 4–5Test Steps
Additional Testing Notes
position=abc,position=5with no comma, arealmthat is not an ENS name) are covered by 20 EditMode tests inNotificationHandlerShould— they are impractical to produce by hand without crafting event payloads through the Events API, so they are verified there rather than manually.dev.Quality Checklist
🤖 Generated with Claude Code