fix: validate builder collection ids before signing requests with them - #9525
fix: validate builder collection ids before signing requests with them#9525mikhail-dcl wants to merge 3 commits into
Conversation
The "self-preview-builder-collections" argument carries a comma-separated list of Builder collection ids, each substituted into the builder-api URL template and then requested with the user's Web3 signature attached. No format check was applied. The template prefix is absolute so the host cannot be swapped, but an id carrying "?", "#" or "../" resolved to a different builder-api endpoint - "#" truncates the "/items" suffix - and spent that signature on it. Builder collection ids are GUIDs, so both call sites now skip and log entries that are not well-formed GUIDs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Now that every collection id must parse as a GUID before it reaches a signed Builder API URL (previous commit), the param is safe to accept from a deep link again. The deny-by-default allowlist introduced in #9476 had been dropping it, which silently broke the documented one-click creator flow in docs/unreleased-wearables-emotes-preview.md. Placed on the always-permitted tier, not the whitelisted-realm one: the documented link carries no realm (decentraland://?position=100,100&self-preview-builder-collections=<uuid>), so the whitelisted-realm tier could never serve it. What bounds the exposure: ids are GUID-validated, the Builder API only serves a collection to its owner or an authorized curator, and the preview is local - equipping does not publish a profile change. Verified: Unity 6000.4.0f1 batch EditMode - 0 compile errors, 63/63 tests (AppArgs incl. the new pinning test, plus the provider GUID-rejection suites). Co-Authored-By: Claude Opus 4.8 <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.
PR Review: fix: validate builder collection ids before signing requests with them
STEP 2 — Root-cause check: ✅ PASS
The PR fixes the root cause: unsanitized, attacker-controllable collection IDs were being interpolated directly into a URL template (/v1/collections/[COL-ID]/items) and requested with the user's Web3 signature attached. Characters like ?, #, or ../ could redirect the signed request to a different builder-api endpoint. Demanding GUID shape via Guid.TryParse eliminates the entire class of injection inputs at the source — this is a correct, minimal allowlist fix.
STEP 3 — Design & integration: ✅ PASS
Owner search completed. BuilderCollectionIds is a stateless static utility class (no persistent state, no lifecycle management). It is not a "long-lived unit" — it owns no entity, scene, connection, or resource lifecycle. The validation is placed at the correct layer: inside the loop in both ApplicationParamsEmoteProvider and ApplicationParametersWearablesProvider, immediately before the collection ID is substituted into the URL template. This is defense-in-depth — the deep-link allowlist permits the param to arrive, and IsValid sanitizes each individual ID before it reaches the network.
The class has 2 consumers (emote + wearable provider), so it is not a single-use helper (CLAUDE.md §11 does not apply). It lives in DCL.AvatarRendering.Loading, the correct shared namespace.
The deep-link allowlist addition is well-reasoned: the param belongs on the always-permitted tier because the documented creator flow (docs/unreleased-wearables-emotes-preview.md) carries no realm, so the whitelisted-realm tier could never serve it. Three properties justify this placement: GUID validation, Builder API's own auth check, and local-only preview.
Teardown/consumption trace: No subscriptions, event hookups, connections, or buffers are added by this diff. The new CancellationTokenSource() calls are pre-existing and not introduced by this PR.
STEP 4 — Member audit: ✅ PASS
BuilderCollectionIds.IsValid(string, ReportData) — 2 consumers (both providers). It is a shared validation utility, not a single-use intermediate. The coupling to ReportHub logging is shallow (via implicit conversion from ReportCategory string constant to ReportData struct). The method validates and logs in one call, which is the correct pattern for a guard that needs to explain why an input was rejected.
STEP 5 — Line-level review: ✅ No issues found
Pass A — Blocking-issue categories:
- Code quality: PascalCase naming, XML doc comments,
ReportHublogging — all follow CLAUDE.md conventions. ✅ - Bugs:
Guid.TryParsecorrectly rejects all URL-manipulation characters (/,?,#,..). Accepts only[0-9a-fA-F-{}()]. Even brace/paren GUID formats produce no RFC 3986 path delimiters. ✅ - Security: GUID-shape constraint is an effective allowlist. Deep-link allowlist addition is bounded by three independent defense layers documented in the code. ✅
- Performance:
Guid.TryParseis a negligible cost, called once per collection ID at startup. Not a hot path. ✅ - Error handling: Invalid IDs produce a
ReportHub.LogWarningandcontinue— appropriate for user-supplied input that may be malformed but not exceptional. ✅ - Logic: The guard is placed before the URL substitution and signing, which is the correct order. ✅
- Resource leaks: No new subscriptions, events, or connections introduced. ✅
8-11. No other issues found.
Pass B — Design, encapsulation & resource smells: No issues. No new constructors, no magic values, no naming concerns, no encapsulation leaks.
Security Review: ✅ No issues
- Input validation:
Guid.TryParsecannot be bypassed — it rejects all non-hex, non-structural characters. - URL injection: A valid GUID cannot cause URL manipulation (no
/,?,#, or..in the accepted character set). - Log injection: The warning message includes the rejected
collectionId, but Unity's logging serializes entries atomically. The value is already rejected and never reaches a URL. Acceptable. - Deep-link allowlist: Safe — bounded by GUID validation + Builder API auth + local-only preview.
- Secrets/credentials: None committed.
Test Coverage: ✅ Thorough
12 new tests across 3 files:
- Provider tests (both emote and wearable): Valid GUID accepted with correct URL, path traversal rejected (
../../lands), query injection rejected (collections?id=1), fragment injection rejected (GUID#/items), non-GUID rejected (not-a-guid), mixed CSV filters correctly. - Deep-link pinning test: Confirms
self-preview-builder-collectionssurvives deep-link processing without a realm. - Tests follow AAA pattern with NUnit + NSubstitute per project conventions.
STEP 6 — Complexity
SIMPLE — 4 production files changed (1 new 30-line utility, 2 two-line guards, 1 allowlist entry), plus well-proportioned tests. No ECS, async, plugin, or infrastructure changes.
STEP 7 — QA assessment
YES — The changes affect runtime wearable/emote loading and deep-link parameter handling, both of which are user-facing.
STEP 8 — Non-blocking warnings
None. Main.unity is not modified.
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Stateless input-validation utility added to the avatar-rendering loading layer, with guards in two existing providers and a deep-link allowlist entry.
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}
|
All Unity tests passed ✅
|
|
Warnings not reduced: 14034 => 14034 — remove at least 1 warning to merge. Warnings/errors in files changed by this PR (6) |
pravusjif
left a comment
pravusjif
left a comment
There was a problem hiding this comment.
Tested with the build and works correctly for previewing unreleased wearables and emotes, cannot test with deep link due to the skip-version-check dropped param.
# Conflicts: # Explorer/Assets/DCL/Infrastructure/Global/AppArgs/DeepLinkAllowlist.cs
Ludmilafantaniella
left a comment
Ludmilafantaniella
left a comment
There was a problem hiding this comment.
QA Approval — tested on Windows and Mac
✅ Deep link (decentraland://?position=100,100&self-preview-builder-collections=) — unreleased items appear in backpack and are equippable
✅ CLI with valid UUID — same result
✅ Malformed id skipped correctly (not-a-guid,) — valid collection still loads; log warning categories are disabled by design (confirmed by @mikhail Agapov)
✅ CSV with two valid UUIDs — both load
No blocking issues found. Approving from QA.
9525-TC1.mp4
9525-TC2.mp4
9525-TC3.mp4
✅Smoke test performed:
- ✔️ Log In/Log Out
- ✔️ Backpack and wearables in world
- ✔️ Emotes in world and in backpack
- ✔️ Teleport with map/coordinates/Jump In
- ✔️ Chat and multiplayer
- ✔️ Profile card
- ✔️ Camera
- ✔️ Skybox
4 participants
Pull Request Description
What does this PR change?
Two related changes to
self-preview-builder-collections(the Builder preview flag for unreleased wearables/emotes).1. Validate the collection ids (
a974929934). Each comma-separated id is substituted intohttps://builder-api.decentraland.{ENV}/v1/collections/[COL-ID]/itemsand requested withneedsBuilderAPISigning: true— i.e. signed with the user's Web3 identity — with no format check. The host cannot be swapped (the template prefix is absolute), but an id containing?,#or../resolves to a different builder-api endpoint (#simply truncates the trailing/items) and spends that signature on it. Impact was limited — GET-only, and the response never reaches whoever supplied the id — but the whole family of inputs disappears if ids must be GUIDs, which they are.New
BuilderCollectionIds.IsValid(Guid.TryParse, else aReportHubwarning) is now an earlycontinuein the collections loop of bothApplicationParametersWearablesProviderandApplicationParamsEmoteProvider. The URL template, the.Replacecall and the signing flag are untouched; valid ids behave exactly as before.2. Re-permit the param in deep links (
ca49dcfe37). The deny-by-default allowlist from #9476 had been dropping it, which silently broke the documented one-click creator flow indocs/unreleased-wearables-emotes-preview.md. It goes on the always-permitted tier rather than the whitelisted-realm one because the documented link carries no realm (decentraland://?position=100,100&self-preview-builder-collections=<uuid>), so the whitelisted-realm tier could never serve it.What bounds the exposure now: ids are GUID-validated; the Builder API only serves a collection to its owner or an authorized curator (
docs/app-arguments.md); and the preview is local — equipping does not publish a profile change.One residual worth a reviewer's opinion
Curators are authorized against any collection, and this flag enables the raw-GLTF wearable path plus a
GlobalGLTFLoadingPlugin, bypassing the asset-bundle conversion gate. So a one-click link can hand a curator attacker-authored GLTF. Narrow population, but a high-value target — flagging it explicitly rather than burying it.Test Instructions
Expected result: no change for normal launches. Deep links and CLI launches that pass valid collection UUIDs behave as before.
Prerequisites
Test Steps
decentraland://?position=100,100&self-preview-builder-collections=<your-collection-uuid>into a browser. The unreleased items appear in your backpack and can be equipped (visible only to you). Before this PR the param was dropped and nothing appeared.--self-preview-builder-collections <uuid-1>,<uuid-2>.--self-preview-builder-collections not-a-guid,<valid-uuid>. The log showsSkipping Builder collection id 'not-a-guid': expected a GUID.and only the valid collection loads.Additional Testing Notes
docs/unreleased-wearables-emotes-preview.mdneeds no change — its deep-link example works again.Quality Checklist
../../lands,collections?id=1,<guid>#/items,not-a-guid) and a mixed CSV; plus a pinning test that the param survives deep-link processing. The rejection tests were sanity-checked with a negative control (guards removed → 10 failures).