Skip to content

Security: feats attach path has no user_id scoping on any provider -- another user's homebrew feat can attach to your character #83

Description

@zacgoodwin

Context

The feats attach path has no user_id scoping, on any provider. Found by #76's
adversarial review while widening CharactersContext::Tlc::RefreshFeats#feats to the
::Feat.tlc_content union: the relation filters on origin_value only, so a
homebrew Dnd2024::Feat (a row carrying another user's user_id) whose
origin_value matches a character's species/class attaches to ANY user's character.

This is pre-existing and systemic, not introduced by #76: the dnd2024 sibling
(app/services/characters_context/dnd2024/refresh_feats.rb) queries
::Dnd2024::Feat.where(origin_value: ...) with the identical gap, and dnd5 likely
matches. The inconsistency is stark against the spells path the same commands use:
characters_context/tlc/create_command.rb:110-112 filters
user_id: [nil, input[:user].id] plus book-shared ids. The feats subsystem never
adopted that pattern.

Impact: another user's private homebrew feat can silently land on your character
(content leak + gameplay pollution). Reachability depends on homebrew feats carrying
stock-matching origin_values, which the homebrew import flows permit.

Files

  • app/services/characters_context/refresh_feats.rb
  • app/services/characters_context/dnd2024/refresh_feats.rb
  • app/services/characters_context/tlc/refresh_feats.rb
  • app/commands/characters_context/tlc/create_command.rb

Plan

  1. Reproduce first: a homebrew Dnd2024::Feat owned by user B with
    origin_value: 'wizard'; create user A's wizard (dnd2024 AND tlc); confirm B's
    feat attaches to A's character on both providers.
  2. Fix at the shared root, not per subclass: the feats(character) relations should
    scope user_id: [nil, character.user_id] plus the book-shared ids, mirroring the
    spells path's shape (create_command.rb:110-112 is the worked example, including
    the homebrew_item_ids book union).
  3. Decide the migration question explicitly: rows ALREADY attached through the gap are
    existing Character::Feat records. Removing them is a data change -- propose, do
    not silently delete.
  4. Verify every provider's refresh still attaches its own + stock + legitimately
    book-shared content; the A6 regression guard must stay green.

Acceptance Criteria

  • Setup: user B's private homebrew Dnd2024::Feat with origin_value: 'wizard' ->
    Action: user A creates a wizard (dnd2024 and tlc) -> Expected: B's feat does NOT
    attach to either character.
  • Setup: the same feat shared through a book user A subscribes to -> Action: same
    create -> Expected: it DOES attach.
  • Setup: user A's own homebrew feat -> Action: create -> Expected: attaches.
  • Setup: stock (user_id: nil) content -> Action: create -> Expected: unchanged; the
    A6 regression guard's 12 examples stay green.

Tests + evals

  • Service specs per acceptance case, on the shared parent so every provider inherits
    the coverage; mutation-checked (remove the user scope -> the cross-user case red).
  • No eval suite: no LLM surface.

Docs pages touched

  • None.

Out of scope

  • Cleaning up rows already attached through the gap (surfaced by step 3 as a proposal,
    its own decision).
  • The spells path, which already scopes correctly.

Depends on #76

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingready-for-agentFully specified, ready for an AFK agent

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions