fix: normalize name case in ownership check and document L1 committee bypass - #351
fix: normalize name case in ownership check and document L1 committee bypass#351LautaroPetaccio wants to merge 6 commits into
Conversation
Test this pull request
|
- Lowercase claimed names before ownership check to handle case-insensitive ENS name matching - Add documentation comment for L1 v1 collection Decentraland address fallback in subgraph collection-asset validation
… check
Names claimed in profile metadata may have mixed case ("Some Name") but
the subgraph stores them in lowercase. The ownership check must lowercase
names before comparing, otherwise legitimate name claims fail validation.
33996e7 to
ceec519
Compare
decentraland-bot
left a comment
There was a problem hiding this comment.
Code Review
Good fix — the name case normalization is a real bug that could cause false ownership rejections (or bypasses) due to the subgraph's case-sensitive name_in matching. The L1 fallback documentation is also a welcome addition. Two findings below.
P2 — Important
1. [Bug] Same case-sensitivity issue exists in outfits.ts
The parallel code path in createOutfitsNamesOwnershipValidateFn (src/validations/access/common/outfits.ts:62) passes namesForExtraSlots to ownsNamesAtTimestamp without lowercasing — the exact same bug this PR fixes in profile.ts.
// outfits.ts:62 — no lowercasing
const names = namesForExtraSlots.filter((name: string) => name && name.trim().length > 0)Suggested fix:
const names = namesForExtraSlots
.map((name: string) => name?.toLowerCase())
.filter((name: string) => name && name.trim().length > 0)Consider extracting the name normalization logic into a shared utility to prevent future divergence.
P3 — Nice-to-Have
2. [TypeScript] Optional chaining produces string | undefined but filter annotation assumes string
avatar.name?.toLowerCase() yields string | undefined, but the subsequent .filter((name: string) => ...) annotates the parameter as string. This works at runtime because the filter logic handles undefined, but the type annotation is inaccurate. A proper type predicate would be more precise:
.filter((name): name is string => !!name && name.trim().length > 0)Tests
The new test correctly verifies the happy path for mixed-case normalization. Minor suggestions:
- Consider adding
expect(response.errors).toBeUndefined()to the happy-path test for completeness. - A test for
undefined/emptyavatar.namewithhasClaimedName: truewould exercise the defensive filter.
Security
No secrets, injection risks, or auth bypass issues found in this PR. The name normalization is a security improvement. The L1 fallback logic is unchanged (comment-only) and the authorization checks are sound.
CI
All checks passing ✅ (build, tests, coverage +0.06%, title convention)
Requested by Lautaro Petaccio via Slack
| const names = deployment.entity.metadata.avatars | ||
| .filter((avatar: Avatar) => avatar.hasClaimedName) | ||
| .map((avatar: Avatar) => avatar.name) | ||
| .map((avatar: Avatar) => avatar.name?.toLowerCase()) |
There was a problem hiding this comment.
The same case-sensitivity bug exists in src/validations/access/common/outfits.ts:62 — namesForExtraSlots are passed to ownsNamesAtTimestamp without lowercasing. Worth fixing in this PR or a follow-up.
Also, minor type nit: avatar.name?.toLowerCase() returns string | undefined, but the next .filter() annotates name as string. A type predicate would be more accurate:
.filter((name): name is string => !!name && name.trim().length > 0)Address review feedback: - Apply the same lowercase normalization to namesForExtraSlots in outfits.ts that was already applied to profile claimed names. Without this, outfits with mixed-case extra slot names would fail the ownership check against a case-sensitive subgraph. - Use proper type predicate (name): name is string in the filter callbacks so TypeScript correctly narrows string | undefined to string after the filter. - Add test for mixed-case name normalization happy path with errors undefined assertion. - Add test for undefined avatar name with hasClaimedName: true to verify the defensive filter handles it gracefully.
6eaa1e9 to
821fabb
Compare
decentraland-bot
left a comment
There was a problem hiding this comment.
Review — Focus on Breaking Changes & Lowercasing
🔴 Lowercasing Issue: Asymmetric Case Normalization
This PR lowercases names before passing them to ownsNamesAtTimestamp:
.map((avatar: Avatar) => avatar.name?.toLowerCase())However, the subgraph response is NOT lowercased in the mapper at the-graph-client.ts:41-42:
mapper: (response) => new Set(response.names.map(({ name }) => name))
// ...
const notOwned = namesToCheck.filter((name) => !ownedNames.has(name))This creates an asymmetry:
- You send
'alice'(lowercased) to the GraphQL query asname_in: ['alice'] - The subgraph's
name_infilter is case-sensitive in The Graph - Even if the subgraph returns a result, it stores the original case (e.g.,
'Alice') in the Set ownedNames.has('alice')would fail against a Set containing'Alice'
For DCL names specifically, names are stored lowercase in the DCL Registrar contract, so in practice this may work. But the code doesn't enforce this assumption — it's fragile.
Recommended Fix
Also lowercase the subgraph response in the mapper (the-graph-client.ts):
mapper: (response) => new Set(response.names.map(({ name }) => name.toLowerCase()))This ensures both sides of the comparison use the same casing, regardless of how the subgraph stores names.
Same issue in outfits path
outfits.ts lowercases namesForExtraSlots before the ownership check, but the downstream comparison in the subgraph mapper has the same asymmetry.
On-chain path
L1.checker.checkNames receives lowercased names. Verify that the contract call handles this correctly (ENS names are typically normalized to lowercase at the protocol level, so this is likely safe).
L1 v1 collection bypass comment
The added comment documenting the intentional fallback for L1 v1 collections is a good documentation improvement. No concerns.
Breaking Change Risk
Medium. If the subgraph stores any names with mixed case and the name_in filter is case-sensitive, legitimate deployments would be rejected. The asymmetric normalization (lowercase input but original-case comparison) makes this especially risky.
Requested by Lautaro Petaccio via Slack
…ring[] Unlike profile avatars where avatar.name can be undefined, namesForExtraSlots is typed as string[] so every element is guaranteed to be a string. Remove unnecessary optional chaining and type predicate — a plain .toLowerCase() and length check suffice.
dd7c071 to
b7ad46c
Compare
DCL names (as opposed to standard ENS names) may preserve their original casing in the subgraph. Lowercasing before the ownership query could cause legitimate names to fail matching. Revert to passing names as-is from metadata, which is how they were originally stored. Keep the L1 committee bypass documentation.
|
Closing: the name lowercasing change was reverted after discovering DCL names may not be stored lowercase in the subgraph, which would break legitimate ownership checks. The remaining L1 committee documentation comment is too thin for a standalone PR. |
Summary
Two access control improvements: a name case normalization fix and documentation for an intentional L1 fallback path.
Name case normalization before ownership check
In
createNamesOwnershipValidateFn, claimed names from avatar metadata are passed directly toownsNamesAtTimestampwithout case normalization. The subgraph query usesname_in: $nameListwhich does case-sensitive matching against the subgraph data. ENS names are case-insensitive by convention, so a user who claims the name"Alice"but whose subgraph record stores"alice"would fail the ownership check.The fix lowercases names before passing them to the ownership checker, matching how Ethereum addresses are consistently lowercased throughout the codebase before comparison.
L1 v1 collection committee bypass documentation
In the subgraph collection-asset validator, when
checkCollectionAccessreturnsfalsefor an L1 asset, the code falls back to checking whether the deployer is Decentraland's address and the collection is an allowlisted v1 collection. This fallback exists because committee-based access (creator/manager checks) may not apply to L1 v1 collections, which are deployed exclusively by Decentraland.The on-chain path has the same structure (only Decentraland's address can deploy L1 v1 collections). Added a comment documenting why the fallback exists so future maintainers don't mistake it for a bypass.
Test plan
npm run buildcompiles cleanlynpm test— all 1300 tests pass (one test assertion updated to match lowercased name)npm run lint:check— no errors🤖 Generated with Claude Code