fix: neutralize untrusted text and gate external URLs in UI sinks - #9565
fix: neutralize untrusted text and gate external URLs in UI sinks#9565mikhail-dcl wants to merge 3 commits into
Conversation
…C-008/034/050/084) Attacker-authored strings reached rich-text TMP labels, and an attacker-authored link could open without consent. SEC-008 — the prompt rendered its destination, and a passport link its title, as rich text, so markup could dress an arbitrary URL up as a familiar one and make the single approval easy to obtain. Rich text is off in the prefabs for labels that carry nothing but untrusted text. The http(s) allowlist and (scheme, host) trust key from #9466 were verified, not changed. SEC-084 — event and place descriptions were linkified without escaping the author's own markup, and a resulting click went straight to the browser sink with no prompt. Descriptions now escape before linkification and route clicks through the external-URL prompt. The escaping is opt-in via a new entry point because the four other linkifier callers build links from pre-authored <link=ID> markup that must stay live. SEC-034 / SEC-050 — profile, member, announcement and notification sinks bound raw names. They now use the filtered ValidatedName where available, escape where the label's own copy is markup that must keep working, and cap length everywhere. RichTextSanitizer is the single escaper, replacing a private duplicate in TransactionRecipientUtils. It also escapes the backslash: TMP decodes a backslash-u sequence into the character it denotes inside the array its tag parser reads, so a brackets-only filter let markup through unparsed. That branch is gated by neither parseCtrlCharacters nor the input-source check above it, which ships commented out, so escaping is the only available defence. This also closes the hole in the transaction-confirmation copy, where a scene name could otherwise hide the recipient and amount. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follow-up to 7243c73, covering the sinks the first pass left and three more found while checking them. Prefab flags, for labels that render nothing but text another user wrote: place and event names, coordinates and event list entries in the navmap; the community result card and card header descriptions; the NFT prompt description; the chat username; and NameContainer, which covers the announcement author, friend panel, push notification and photo-detail person card at once. Code escaping, for labels whose own copy is markup and would break if turned plain: the navmap <b> host and creator templates, and FriendRequestController's three message sinks. Not previously listed, same class, now fixed: FriendPushNotificationView and FriendPanelUserView bound the raw Name rather than the filtered ValidatedName, FriendRequestController assigned a raw message body to a rich-text label, and the community name, title and owner were unescaped alongside the descriptions. PlaceToast.prefab binds four view fields — LiveEventNameLabel, DescriptionLabel, CoordinatesLabel and ParcelCountLabel — to one TMP component, so turning rich text off for the coordinates would also silence the description's links. Those writers escape in code instead. UntrustedTextLabelsShould now pins both halves of the contract, nine labels plain and five deliberately rich, and asserts that shared binding so the guard cannot be "fixed" by flipping the flag it protects. Also drops the SimpleUserNameElement overload that had no callers, and folds the announcement body cap into RichTextSanitizer.DEFAULT_BODY_LENGTH. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
STEP 2 — Root-cause check: PASS
The PR correctly identifies and fixes the root causes:
- TMP markup injection — Untrusted text (user-authored names, descriptions, announcements) reached rich-text TMP labels without escaping, allowing an attacker to inject
<size>,<color>,<link>tags. - Missing consent prompt — Event/place description links opened directly via the OS browser without asking the user to approve.
\uXXXXbypass — TMP decodes backslash-u escape sequences insidePopulateTextProcessingArray(case 117), bypassing a brackets-only filter. The fix neutralizes the backslash alongside the angle brackets.
The fix is at the right level: a shared escaper (RichTextSanitizer) applied at every sink, complemented by prefab-level richText=false where the label renders nothing but untrusted text.
STEP 3 — Design & integration: PASS
RichTextSanitizer is a stateless static utility class — it holds no lifecycle state, manages no entities or resources, and belongs in DCL.UI alongside the TMP labels it protects. No lifecycle owner search needed.
SetAuthorTextWithClickeableLinks combines escaping, assignment, linkification, and consent-prompt routing in a single call. The coupling is intentional — the docstring correctly notes that separating escape from assignment invites drift (SEC-084). It accesses ViewDependencies.GlobalUIViews statically, trading constructor-injected UnityAppWebBrowser for a hidden dependency. ViewDependencies is an established pattern in this codebase, and the trade buys a simpler constructor surface and the guarantee that all description links route through consent. Worth documenting as tech debt for testability in a follow-up.
The removal of webBrowser from EventInfoPanelController and PlaceInfoPanelController constructors is clean — all callers in ExplorePanelPlugin are updated.
STEP 4 — Member audit
| Member | Consumers | Assessment |
|---|---|---|
RichTextSanitizer.Escape |
~6 direct + via SetAuthorTextWithClickeableLinks |
Well-scoped |
RichTextSanitizer.EscapeAttribute |
1 (TransactionRecipientUtils.HighlightLink) |
Distinct responsibility (attribute context); not a merge candidate |
RichTextSanitizer.EscapeAndTruncate |
~15 sinks | Well-scoped |
RichTextSanitizer.Truncate |
~4 sinks | Well-scoped |
SetAuthorTextWithClickeableLinks |
2 sinks (event/place description) | Documents its coupling contract |
IsTrusted |
2 within ExternalUrlPromptController |
Clean extraction of duplicated condition |
SetUserName (private) |
1 (Setup) |
Thin setter with a security contract documented in its summary — acceptable per §11 |
No single-use derived predicates or absent-≠-false problems found.
STEP 5 — Line-level findings
See inline comments. Three P2 findings, no P0/P1.
Additional note (not inline): In ExternalUrlPromptController, the consent dialog displays uri.AbsoluteUri (line 103), but all three OpenUrlMainThreadOnly calls (lines 49, 69, 72) pass uri.OriginalString. The comment at line 100 claims AbsoluteUri is “the canonical form UnityAppWebBrowser hands to Application.OpenURL” — if that’s true, using OriginalString in the controller creates a theoretical mismatch between what the user consents to and what opens. Not blocking (the domain is always the same, and the labels have richText off), but worth aligning in a follow-up.
Security review: No issues found
The escaping is complete:
<,>,\are the only independent markup entry points in TMP.{/}only matter inside<sprite>attributes (which require<first). Unicode normalization (NFKC) would map＼back to\, but TMP does not normalize its input buffer, so no bypass exists.\uXXXX/\UXXXXXXXXsequences are handled by escaping\→＼.- All description links now route through the consent prompt. Non-web schemes are blocked by the existing
http(s)allowlist inExternalUrlPromptController.Params. - Prefab guards (
UntrustedTextLabelsShould) pin the richText flags on shipped assets, preventing accidental re-enablement.
Summary
Thorough, well-implemented security fix. Two defence layers (prefab flags + code escaping) cover all UI sinks that render untrusted text. The RichTextSanitizer is allocation-free for the common case (no markup) and handles edge cases (surrogate pairs, \uXXXX sequences). Six new test classes (~870 lines) provide comprehensive coverage of the sanitizer, name element, notification text, description links, external URL prompt, and prefab label assertions.
No P0 or P1 issues. Three P2 findings noted in inline comments.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Security hardening across UI rendering of untrusted text in navmap, friends, notifications, communities, chat, passport, external URL prompt, with a new shared sanitizer utility and async consent flow changes
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. |
|
Warnings count reduced: 13999 => 13944 Warnings/errors in files changed by this PR (114) |
|
All Unity tests passed ✅
|
Review feedback on #9565. The confirmation dialog's mainText renders rich text, and sixteen call sites format community, member and place names into it — the reviewer flagged one of them (CommunityResultCardView's raw currentCommunityName). None of the nine dialog text arguments carries markup of its own, so the fix is the flag rather than an escape at each site: richText off on mainText and subText. additonalUrlText stays rich, since it carries the linkified URL the dialog can show. The ValidatedName-with-fallback pattern that had spread to four call sites now lives on Profile.CompactInfo as ValidatedNameOrRaw. Named for what it does rather than "safe": the fallback is verbatim user input, so a label rendering it still needs richText off or an escape, and a name implying otherwise is how this class of bug comes back. Also clears 36 nullable-reference warnings across the files this PR touches, which unblocks the warning ratchet. Mostly `= null!` on Unity-serialized fields, the established convention here; three private fields in NftPromptController that were declared non-nullable while every call site already treated them as nullable; and PlaceInfoPanelController.thumbnailImage, where the annotation was the lie — it is readonly, assigned unconditionally from a factory that always returns a new instance, and already dereferenced unconditionally elsewhere. Left alone deliberately: Profile.CompactInfo's uninitialized members, because MentionName and UnclaimedName genuinely end up null for a name with no alphanumerics and seeding them would change behaviour for every consumer; and two latent NREs in PlaceInfoPanelController where cameraReelGalleryController and homeButton are assigned under a null check but dereferenced without one. Both want their own change, not this one. 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.
PR Review: fix: neutralize untrusted text and gate external URLs in UI sinks
STEP 2 — Root-cause check
Problem: Text written by other users — profile and community names, event/place descriptions, announcements, friend-request messages — reached rich-text TMP labels unsanitized. An attacker-authored <link> could open a URL with no consent prompt, and <size=0> / <color=#00000000> could hide critical copy (such as a transaction's recipient and amount). Additionally, TMP's \uXXXX escape-sequence decoder could be used to bypass a brackets-only filter.
Does the diff fix the cause? Yes. The PR addresses the root cause at every UI sink rather than patching a symptom. Two correct mechanisms are used per label:
richTextoff in the prefab for labels that render nothing but untrusted text.- Code-level escaping via the new
RichTextSanitizerfor labels whose own copy is styled markup that must keep working.
Description links that previously opened through webBrowser.OpenUrlMainThreadOnly (no consent) are now routed through the external-URL consent prompt. The webBrowser dependency is cleanly removed from both PlaceInfoPanelController and EventInfoPanelController.
PASS ✅
STEP 3 — Design & integration
RichTextSanitizer (new static utility, DCL.UI namespace)
This is a pure, stateless string transformation — no ECS entities, no Unity lifecycle, no persistent state. A static class is the correct shape; it belongs with the TMP UI infrastructure it protects, not in an ECS system. The codebase already uses TextMeshProExtensions in the same namespace for TMP helpers.
The string.Create with SpanAction approach is well-suited to the GC-pressure constraints:
- Common case (no markup):
IndexOfMarkupshort-circuits and returns the originalstringreference — zero allocation. Pinned byReturnTheSameInstanceWhenThereIsNothingToEscape. - Escape case: One allocation via
string.Create; thestaticlambda avoids closure allocation. - Truncate+escape: Cut, swaps, and ellipsis all land in one buffer — one allocation instead of Substring + escape + concat.
ValidatedNameOrRaw on Profile.CompactInfo — correctly placed on the type that owns both ValidatedName and Name. Pure computed property, no side effects, no allocation. Used by 4+ call sites.
SetAuthorTextWithClickeableLinks — correct composition: escape → assign → linkify → route clicks through consent prompt. Centralizing this prevents the escape-then-linkify steps from drifting apart across callers.
Removed webBrowser parameters — confirmed both EventInfoPanelController.OpenUrl and PlaceInfoPanelController.OpenUrl were the sole consumers. Three call sites in ExplorePanelPlugin updated. Clean removal, no dangling references.
Deleted SimpleUserNameElement.Setup(string, string, bool, Color) overload — all 5 callers (ProfileInputSuggestionElement, DonationLoadingView, DonationConfirmedView, SimpleProfileView, DonationDefaultView) use the CompactInfo overload. No external consumers of the deleted overload exist.
IsTrusted extraction in ExternalUrlPromptController — used by two call sites (OnViewShow and WaitForCloseIntentAsync), so the extraction eliminates duplication and is justified.
PASS ✅
STEP 4 — Member audit
| Member | Consumer count | Verdict |
|---|---|---|
RichTextSanitizer.Escape |
13 (direct + via EscapeAndTruncate + via SetAuthorTextWithClickeableLinks) |
Core escaper, not single-use |
RichTextSanitizer.EscapeAttribute |
1 (TransactionRecipientUtils.HighlightLink) |
Justified: distinct threat model (attribute position vs content position) |
RichTextSanitizer.Truncate |
5 call sites | Justified: labels with richText off need only a cap |
RichTextSanitizer.EscapeAndTruncate |
15+ call sites | Primary consumer entry point |
ValidatedNameOrRaw |
4 call sites | Justified, all callers pass through EscapeAndTruncate |
IsTrusted |
2 call sites | Justified, eliminates duplication |
PASS ✅
STEP 5 — Line-level findings
See inline comments below. All findings are P2 (minor).
Security review
The sanitizer is sound against TMP's markup parser:
<and>replacement blocks all tag types (<size>,<color>,<link>,<b>,<sprite>, etc.).\replacement closes the\uXXXX/\UXXXXXXXXdecode path inTMP_Text.PopulateTextProcessingArray— tested byNeutralizeMarkupSmuggledAsAUtf16EscapeSequenceand the UTF-32 variant."replacement in attribute position prevents early<link="...">closure.AbsoluteUri(notOriginalString) displayed in the consent prompt — percent-encoding is a second barrier against markup smuggling in the displayed URL.- Null-URI path correctly clears the previous prompt's destination and callback.
- The
http(s)-only scheme allowlist inExternalUrlPolicyis tight and tested. SetAuthorTextWithClickeableLinksorder of operations is correct: escape first (neutralizing injected<link>tags), then linkify barehttps://URLs, then route clicks through the consent prompt.
No P0 or P1 security issues found. ✅
STEP 8 — Non-blocking warnings
No Main.unity modification detected. ✅
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches UI sinks across navmap, friends, communities, chat, notifications, donations, passport, NFT prompt, and external URL prompt; introduces a shared security utility and rearchitects description-link routing.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by mikhail-dcl via GitHub
| [TestCase("Assets/DCL/Communities/CommunitiesBrowser/Prefabs/CommunityResultCard.prefab", nameof(CommunityResultCardView), "communityDescription")] | ||
| [TestCase("Assets/DCL/NftPrompt/Assets/NftPrompt.prefab", nameof(NftPromptView), "<TextDescription>k__BackingField")] |
There was a problem hiding this comment.
[P2] Test coverage gap for CommunityCardHeader.prefab description label. This prefab's description label had richText changed from 1 to 0 in this PR, and CommunityCardView.ConfigureCommunity uses Truncate (no escaping) for that description — relying entirely on the prefab flag for safety. If someone reverts the flag in the inspector, the description becomes injectable and no test catches it.
| [TestCase("Assets/DCL/Communities/CommunitiesBrowser/Prefabs/CommunityResultCard.prefab", nameof(CommunityResultCardView), "communityDescription")] | |
| [TestCase("Assets/DCL/NftPrompt/Assets/NftPrompt.prefab", nameof(NftPromptView), "<TextDescription>k__BackingField")] | |
| [TestCase("Assets/DCL/Communities/CommunitiesBrowser/Prefabs/CommunityResultCard.prefab", nameof(CommunityResultCardView), "communityDescription")] | |
| [TestCase("Assets/DCL/Communities/CommunitiesCard/Prefabs/CommunityCardHeader.prefab", "CommunityCardView", "<communityDescription>k__BackingField")] | |
| [TestCase("Assets/DCL/NftPrompt/Assets/NftPrompt.prefab", nameof(NftPromptView), "<TextDescription>k__BackingField")] |
2 participants
Pull Request Description
What does this PR change?
Text written by other users — profile and community names, event/place descriptions, announcements, friend-request messages — reached rich-text TMP labels unsanitized, and an attacker-authored link could open with no consent prompt. Fixes SEC-008, SEC-034, SEC-050 and the client half of SEC-084.
Two mechanisms, chosen per label:
richTextoff in the prefab where a label renders nothing but untrusted text.RichTextSanitizerwhere the label's own copy is markup (a<b>run, or the<link>the description linkifier emits) and turning it plain would break it.RichTextSanitizeralso escapes the backslash, which is not cosmetic: TMP rewrites a\uXXXXsequence into the character it denotes inside the array its tag parser reads (TMP_Text.PopulateTextProcessingArray,case 117). That branch is gated by neitherparseCtrlCharactersnor the input-source check above it, which ships commented out — so a brackets-only filter let markup through unparsed. This also closes the same hole in the already-shipped transaction-confirmation copy, where a scene name could otherwise hide the recipient and amount.Changes
RichTextSanitizer(new)Escape(content),EscapeAttribute(tag attribute),Truncate(cap only), shared name/body caps. One allocation per escaping call; none when there is nothing to escape<b>templates keep rich text, values escapedValidatedName; falls back to the raw name when the filter leaves nothing (emoji-only names)Namereplaced with filtered name; message bodies escaped inside their<b>templateTransactionRecipientUtilsSimpleUserNameElement.Setupoverload and unusedwebBrowserctor parameters removedAlready landed in #9466 and only verified here, not changed: the
http(s)-only scheme allowlist and the(scheme, host)trust key that closed the empty-host prompt bypass.Note for reviewers:
PlaceToast.prefabbinds four view fields —LiveEventNameLabel,DescriptionLabel,CoordinatesLabel,ParcelCountLabel— to a single TMP component, so turning rich text off for the coordinates would silence the description's links. Those writers escape in code instead.UntrustedTextLabelsShouldpins both directions (nine labels plain, five deliberately rich) and asserts that shared binding, so the guard cannot be "fixed" by flipping the flag it protects.Server-side halves of SEC-084 (
decentraland/eventsapproval-reset bug,decentraland/placesoutput sanitization) are out of scope for this repo.Test Instructions
Steps (standard run):
Steps (fresh account):
Prerequisites
Set these up first — every check below reuses them.
<size=400%><color=#00FF00>Verified Admin\u003Csize=400%\u003EAdmin👽👽<link="smb://attacker/share">click here</link>,<size=400%>huge, and a plainhttps://decentraland.org<b><size=400%>urgentWhat "pass" looks like
Two different outcomes depending on the row below:
<size=400%>, possibly with slightly odd-looking angle brackets‹ ›). Nothing is huge, coloured, hidden, or clickable.dev. These are the regression rows; a bug here is caused by this PR.UI surfaces to check
created by …, coordinates/world name all inert<link>inert, but the plainhttps://is still blue, clickable, and promptshosted by X - at Yinerthosted by/atmust still be bold@in chatName:)Additional Testing Notes
…, bodies at 1000. That is expected, not a bug.‹ ›lookalikes. Expected. Straight quotes and apostrophes in ordinary prose must be untouched.devbuild side by side.Quality Checklist
🤖 Generated with Claude Code