Skip to content

The connector clears an attachment or suspected-injection hold by itself: holdIsBlanket reads reasons, the finding is only in hold_reason #879

Closed

Description

@Birfy

Security defect present on main today (38629bf4), found by Dev's read-only audit and reproduced end to end. Recorded here so it is in the audit trail with its own ticket; the fix vehicle is #869, not a second PR.

What happens

plugins/social/src/social.js:128 holdIsBlanket decides "this hold is only the owner's approve-everything setting, so the connector may clear it" from reasons alone: exactly one reason equal to mvp.all_outbound_requires_approval, and risk_flags empty.

Bot Social's contract is the opposite: the row carries hold_reason (bot-social src/platform.ts:24, values blanket_outbound | risk | attachment | policy | face_room_auto_send_off), and the platform writes only the blanket reason into reasons while the real cause goes into hold_reason (platform.ts:2044). So a hold with an attachment, or one flagged as suspected injection, looks exactly like a blanket hold to this predicate.

The truth table (four real rows from test/hold_reason.test.ts:36, fed to holdIsBlanket)

hold_reason=blanket_outbound         holdIsBlanket=true    ← correct
hold_reason=attachment               holdIsBlanket=true    ← must keep its card
hold_reason=policy (suspect)         holdIsBlanket=true    ← must keep its card
hold_reason=face_room_auto_send_off  holdIsBlanket=true    ← must keep its card
hold_reason=risk                     holdIsBlanket=false   ← correct

End to end

Same four shapes on holdingServer(), room Auto-send on, one send each: all four were cleared by the connector itself via decide_approval; zero cards left. A send with an attachment, and a send flagged as suspected injection, both reached the peer without the owner seeing anything.

Why the suite never caught it

#737/#766 express a "finding" as an extra entry in reasons (dlp.commitment, policy.new_rule) or as risk_flags. The real server shape has neither — the finding is only in hold_reason — so every existing case stayed green while the hole was open.

Where the fix lands

#869 already changes this exact predicate (holdIsBlanket reads hold_reason first), so it repairs both outbound paths (autoSendRemoteLine, autoSendRoomEnvelope) and the face path in one place. This issue is the tracker for that fix: no separate PR, no second predicate.

The one thing that must not happen: #869 being read as "add a face_room_auto_send_off branch to the face path". The strictness of the outbound paths comes from the shared predicate change in that same PR, not from what was there before. A face-only patch leaves both outbound paths exactly as broken as today.

Acceptance

  1. With hold_reason present, only blanket_outbound self-clears. attachment, policy (including a suspected-injection row), risk, face_room_auto_send_off, and any value this build does not know keep their card. The four-row table above becomes a test.
  2. Field absent (an older server) keeps today's reasons rule, fail-closed — absent must never be read as "not blanket" and must never widen what self-clears.
  3. The table is asserted on both outbound paths (a remote line and a room envelope), not only on the face path — a face-only branch must red it.
  4. Negative control: reverting the predicate to reasons-only reds exactly the table's "must keep its card" rows and nothing else.
  5. The end-to-end case exists: with a real server shape (hold_reason set, blanket-only reasons), an attachment send and a suspected-injection send each leave a card and change nothing until the owner decides.

Refs #869.

Activity

  1. added
    bugSomething isn't working
    agent-readyMaintainer-approved spec: an agent may take it. Only maintainers set this; never in a template
    area:socialFriends, invites, grants, Bot Social
    area:securityAuth, grants, approvals, secrets
    on Oct 23, 2026
  2. Birfy commented on Oct 23, 2026

    @Birfy
    CollaboratorAuthor

    Vehicle confirmed with #865: #869 is the single PR, and it should carry both Fixes #879 and Fixes #865 so neither stays open on a merged fix. Nothing in this issue asks for separate work.

  3. removed
    agent-readyMaintainer-approved spec: an agent may take it. Only maintainers set this; never in a template
    on Oct 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:securityAuth, grants, approvals, secretsarea:socialFriends, invites, grants, Bot SocialbugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions