Commit Graph

3 Commits

Author SHA1 Message Date
Vineeth Voruganti 10655792bd fix(scopes): validate peer names on create, close namespace squatting and upsert race
Addresses three review findings against 48047a6a.

1. `PeerSpec` let API callers create invalid and reserved-prefix peers.
   Widening `get_or_create_peers` to accept a pattern-free schema fixed the
   lookup 500s but also removed validation from the *insert* path, and
   request-controlled names reach it via message authors, session peer maps, and
   the chat observer path — none of which carry a charset pattern of their own.
   Confirmed: `POST /sessions/{id}/messages` with `peer_id: "scope.x"` returned
   201 and minted an unflagged squatter, after which `POST /scopes {id: x}` was
   permanently 409-blocked — namespace denial of service by any caller able to
   post a message. `peer_id: "not a valid name!@#"` was likewise created.

   Fixed by validating only names about to be INSERTed
   (`_validate_new_peer_names`), so already-existing names — legacy dotted
   names, scope peers — still resolve without a spurious 422. That keeps the
   Dreamer fix intact, since it reads through `get_peer`.

2. Existing reserved-prefix squatters could not be updated. The name-based guard
   on `PUT /peers/{peer_id}` refused every `scope.` name, contradicting the
   invariant that an unflagged squatter stays a normal peer. Now flag-based, so
   behavior is three-way: a real scope is refused, an existing unflagged peer
   updates, and a missing reserved-prefix name is refused by (1) rather than
   minted.

3. Scope checks raced with get-or-create and the membership upsert. The
   route-level guards run before peers are resolved, so a scope created
   concurrently in that window would be attached by the generic path with a
   default `SessionPeerConfig()`, clobbering its observer membership config.
   Adds `_reject_resolved_scope_peers`, which runs on the resolved rows in the
   same transaction as the upsert — no window, no extra query. The early checks
   stay for better error messages.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-29 13:03:27 -04:00
Vineeth Voruganti 48047a6a99 fix(scopes): make scope identity unforgeable, unblock non-pattern peer names
Three coupled changes to the scopes facade.

1. `PeerCreate` no longer gates internal lookups. It exists to validate a new,
   user-supplied peer id at the API boundary, but crud used it as a DTO for
   names that already exist, so any name outside RESOURCE_NAME_PATTERN raised a
   raw pydantic ValidationError — which is not a HonchoException, so it fell
   through to the catch-all handler as an HTTP 500. Adds `PeerSpec` (same
   fields, no charset pattern) as `PeerCreate`'s base, widens
   `get_or_create_peers` to accept it, and changes `get_peer` to take a plain
   str. All 13 construction sites converted; the create route keeps full
   validation.

   This unbreaks the Dreamer: DreamScheduler passes `collection.observer`
   straight into the specialist preflight, and scope peers have
   `observe_others=true`, so every `(scope.x, peer)` dream died there — the
   feature scopes exist to enable. It also fixes a pre-existing bug unrelated
   to scopes: a peer named `alice.smith` (legal before d429de0e5338, which
   validated names by length alone) 500s on message create, session peer add,
   and peer update.

2. The `kind` flag moves from `configuration` to `internal_metadata`.
   `configuration` is user-writable — `PeerCreate`/`PeerUpdate` accept a
   free-form dict and `update_peer` replaces it wholesale — so a legitimate
   `{"observe_me": true}` update silently dropped the flag, and a forged
   `{"kind": "scope"}` injected an ordinary peer into `POST /scopes/list`.
   `internal_metadata` appears in no API schema. `observe_me: false` stays in
   `configuration`, where it belongs.

3. Scope identity requires prefix AND flag, via `is_scope_peer()` and
   `scope_peer_clause()`. Neither half is forgeable: the prefix sits outside
   RESOURCE_NAME_PATTERN, `internal_metadata` is unreachable. Usage-site guards
   become flag-based so a legacy peer merely occupying the namespace keeps
   working rather than 422-ing on its own traffic; peer create and update stay
   name-based, since those must stop new names entering the namespace.
   `update_peer` now returns 422 instead of 500.

Also swaps the reserved prefix from `scope__` to `scope.`: `_` is inside
RESOURCE_NAME_PATTERN, so any tenant could already own a `scope__x` peer.

No DB migration — `internal_metadata` already exists on `peers`, and no scope
peers exist in any deployment yet.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-29 12:07:17 -04:00
Vineeth Voruganti e1b6c6295f test: cover scopes facade, guardrails, and observer semantics
- create-or-get idempotency, list/get, name validation, legacy-collision
  rejection (409), auth scoping (workspace key ok, peer/session keys 401)
- reserved prefix rejected on peer create; peers.list kind filtering
- scope peers rejected as message authors, chat/representation targets,
  and on the generic session-peer routes
- membership add/list/remove with observe_others=true / observe_me=false
  row shape asserted via DB, and facade-less equivalence with a
  hand-built observer peer
- end-to-end litmus: after adding a session to a scope, the deriver
  enqueue fan-out includes the scope peer as an observer
- session creation with scopes: [a, b] creates both memberships

Part of DEV-1997 (Scopes RFC DEV-1970).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-07 15:41:17 -04:00