feat(#575): bind an attachment handle to the room that minted it - #738
Merged
Conversation
Guard 3 checked the floor at REDEMPTION — may this room redeem a storage handle at all — and its own header said what that leaves open: a key issued in a private chat is just a string, and a string can be pasted into a group chat that happens to hold `attachment:read`. Each key is now pinned on first sighting to the ScopeId it was resolved in (migration 0036), and every later resolution must come from the same room. Four decisions worth stating: 1. The room is a ScopeId, never the raw scope string. `turnContext.sessionScope` carries its own warning that it is "NOT safe as a key on its own" — `resolveScope` hands every unscoped HTTP turn the literal 'http-default', which was the live cross-user hole in #445, and `teams-unknown` was the same hole in a second place. Keying a security binding on that would declare every unrelated caller to be the same room: enforcement in appearance, universal access in fact. `isAddressableScope` is the gate, and a non-addressable scope disables the check rather than approximating it. 2. The check rides on the reader, not on the call sites — same argument that put the floor check there. A storage key outlives its turn. 3. The floor is evaluated FIRST, so a room that may not read attachments at all is refused without a row ever being written that would bind the handle to a room that was never allowed to see it. 4. `bindIfAbsent` is ON CONFLICT DO NOTHING, and that clause is the security property: an UPSERT would let a room that was just refused re-bind the handle to itself and read it on the next attempt. It is also what makes a concurrent first sighting safe rather than a 23505. `readByUrl` is untouched — a URL is not a storage key and carries no binding. Verification: middleware suite 6687 tests, 0 fail; the pg suite against a real Postgres in a throwaway container. typecheck, lint, #470 (3294) and #573 (406, unchanged) all green. Mutation-checked, all four died: - ON CONFLICT DO UPDATE instead of DO NOTHING -> kills the no-overwrite test - binding non-addressable scopes -> kills both stand-down tests - a failing binding store reading as "unbound" -> kills the outage test - comparing scope_ref without scope_kind -> kills the kind test
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The last open piece of #575, and the one
audienceFloorGuard's own header has been advertising:A storage key is just a string, and a string can be pasted into a group chat.
Each key is now pinned, on first sighting, to the
ScopeIdit was resolved in. Every later resolution has to come from the same room.The room is a
ScopeId, never the raw scope stringThis is the decision the whole change turns on.
turnContext.sessionScopecarries its own warning:And
unsharedConversationScope's header records the same hole in a second place: a Teams activity without a conversation id yieldsteams-unknown, shared by every unrelated caller who hits that gap.Keying a security binding on those strings would not merely fail to restrict — it would declare every unrelated caller to be the same room, which is worse than no binding at all, because it reads as enforcement. So the scope is parsed into
ScopeIdand only addressable scopes are bound. A non-addressable scope switches the check off rather than approximating it, and two tests pin that.Three smaller decisions, each with a failure mode that looks fine
The check rides on the reader, not the call sites. Same argument that put the floor check there in #733: a storage key outlives the turn that minted it, so a check at one resolution site holds exactly until somebody adds the next site and forgets.
The floor is evaluated first. A room that may not read attachments at all is refused before this touches the database — so no row is ever written that would bind a handle to a room that was never allowed to see it.
bindIfAbsentisON CONFLICT DO NOTHING, and that clause is the security property. AnUPSERTwould let a room that was just refused re-bind the handle to itself and succeed on the next attempt — the leak reintroduced through the fix, by one word. It is also what makes a concurrent first sighting safe rather than a23505; a pg test races three writers to prove it.readByUrlis deliberately untouched: a URL is not a storage key, carries no binding, and its reachability is the channel's business.Blast radius
AUDIENCE_FLOOR_ENABLEDunsetVerification
npm run typecheck✅ ·npm run lint✅ · Epic: Integrated dev platform — isolated per-job runners that clone, develop, test, and ship PRs #470 ratchet 3294 ✅ ·npm run typechecknever typechecksmiddleware/test/#573 ratchet 406, unchanged ✅Mutation-checked — all four died:
ON CONFLICT DO UPDATEinstead ofDO NOTHINGscope_refwithoutscope_kindWhere #575 stands
ScopeId(phase 1)Refs #575
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.