decentraland / decentraland/bevy-explorer

Scene AvatarShape override of real players is ungated, and leaves a stale EmoteCommand

Open
#1,242 3 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
27
Forks
20
Avg merge
1d 30m
Merged PRs (30d)
78

Description

A scene can override a **real player's** avatar shape — wearables, colours, body shape, nametag label and the active emote — by writing `PbAvatarShape` to that player's reserved scene entity id. This is ungated: no permission is checked anywhere on the path. Found while reviewing #1227; it is long-standing behaviour, not something that PR introduced.

## The path

`CrdtContext::init` (`crates/dcl/src/interface/crdt_context.rs:67`) rejects only *dead* ids — there is no reserved-id guard, and `FOREIGN_PLAYER_RANGE` has exactly three references in the tree (the constant, the allocator, its doc comment). So any id a scene touches goes to `nascent` and gets a bevy proxy entity in `process_scene_entity_lifecycle` (`crates/scene_runner/src/lib.rs:1278`). `AvatarShape` is registered `ComponentPosition::Any` (`crates/avatar/src/lib.rs:160`).

`select_avatar` keys its `updates` map by player scene id — primary → `SceneEntityId::PLAYER`, foreign → `ForeignPlayer.scene_id` (allocated in `FOREIGN_PLAYER_RANGE`, 32..=255) — and matches `scene_avatar_defs` on `SceneEntity.id` (`crates/avatar/src/lib.rs:405`, `423`). A proxy carrying `AvatarShape` therefore becomes `update.current_source` and its shape is written into the real player's `AvatarSelection` (`lib.rs:503`), replacing the whole shape (`..scene_avatar_shape.0.clone()`, only `name` falling back to the profile).

It reaches both primary and foreign players. Constraints: the player must be inside that scene (`update.active_scenes.contains(&scene_ent.root)`), and for foreign the id generation must match the live allocation (`free()` bumps it, `is_dead` rejects stale writes).

This looks deliberate rather than accidental — `select_avatar` has had this shape since #19, where the *miss* branch is commented `// this is an NPC avatar`, so the hit branch was the designed case. It was foreign-only then; #20 widened the query to include `PrimaryUser`.

**What is overridden:** everything `update_render_avatar` builds `AvatarDefinition` from — body shape, wearables, hides, skin/hair/eye colours, nametag label, and the emote command.

**What is not:** the `emotes` slot list is never read off the selection (the wheel and the numeric-slot lookup read `CurrentUserProfile` — `system_ui/src/emote_select.rs:303`, `animate.rs:1302`), and the base `AvatarShape` on the player entity is untouched, so nothing writes back to the profile.

## 1. Permission-gate the override

`PermissionType` (`crates/system_api_types/src/lib.rs:322`) has fifteen variants and none covers avatar shape.

The wearables half and the emote half should be gated **separately**. `PlayEmote` is the natural fit for the emote; the wearables half has no existing variant to reuse and needs a new one.

The wearables half is the more consequential of the two, and it is also the one with no gate at all today. An override-driven emote on the primary player is classified `ActiveEmoteSource::TriggeredEmote` (`animate.rs:580`), which is exactly what `broadcast_emote` gates on (`animate.rs:161`), so it is not local-only — it goes out over comms to every other player and to `SubscribePlayerExpression` subscribers.

## 2. Drop the stale `EmoteCommand` when the override goes away

When the override ends — the scene deletes the component, or the player leaves the scene area, both of which are `current_source → None` — the `EmoteCommand` stays on the player entity and keeps playing. Appearance reverts (it lives in `AvatarSelection.shape`); the emote does not.

The obstacle is telling a scene-originated command apart from one written by `triggerEmote`, the console command, the quick-emote UI or the wire. Suggested approach: infer it by value rather than tagging a source. `process_avatar` already knows exactly what the override wrote (`def.emote`, `lib.rs:1144`) — store that alongside the command, and on the transition drop `EmoteCommand` only if it is still identical. Any other producer writing since makes the values differ, so no producer has to cooperate and the next one added is handled for free. This is the idiom already used by `LastEmoteCommand` (`animate.rs:574`) and `EmoteReportQueue.last_command`.

Open detail: where the check lives. `update_render_avatar` has the transition (it runs on `Changed`) but `def.emote` is `None` by then, so removal would need a third state on that field rather than an `Option`; `process_avatar` has the write site and the entity.

## Why these are one issue

A permission gate adds a third drop transition — granted then revoked, or denied at the first ask — on top of "scene cleared the trigger" and "override went away". The value-equality discriminator covers all three identically, so it is worth building once against all three rather than landing the drop first and reworking it.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.