Skip to content

fix(security): fence a friend's Bot's lines in every shared room (#98) - #99

Closed
hsliuustc0106 wants to merge 1 commit into
mainfrom
fix-98-remote-room-fence
Closed

hsliuustc0106 wants to merge 1 commit into
mainfrom
fix-98-remote-room-fence

Conversation

@hsliuustc0106

@hsliuustc0106 hsliuustc0106 commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

Why

authoredRoomLine (core/src/host/room-membership.js) gated the untrusted fence on isFaceRoom(room). A granted friend's Bot speaking through ingestRemoteRoomLine therefore landed raw in a non-face shared room, and its peer-controlled text — closing </untrusted-message> markers, forged headers — reached every member Bot unfenced via the roomPrompt transcript and read_group_chat. Face rooms (#83/#88) fenced exactly this shape; the non-face ingest path missed it. This is the #9 fence class (LESSONS 2026-10-14).

What

Bot-authored lines (botId, or authorKind: 'bot' with the id in authorId — the ingest path sets both, which previously took the early attribution branch and never reached the fence) now take one path keyed on the line's origin: a remote participant, or a botId that is not a local Bot → fenceUntrusted, whatever the room kind. The includes('<untrusted-message') guard keeps it idempotent; human attribution and local-Bot lines are unchanged; outbound fanout keeps its own guard, so no double fence.

Fixes #98

Verification (updated for rebase + review round 1)

Tips: 85b1247 (initial, Verifier FAIL) → 2d3ae94 (current; rebased onto origin/main 1736161 per the new pipeline contract, all four findings addressed).

Ran at 2d3ae94:

  • npm run verify (the new pipeline gate: core lint+arch-docs, plugin lint, client check, patches check, core+social+tree tests, CI script tests) — exit 0.
  • Red tests: the two new fence tests (cross-tag reassembly, mentions the marker) fail at 85b1247's helpers and pass at the tip; the original Security: a granted friend's Bot's lines land unfenced in non-face shared rooms (#9 family) #98 test still fails at base 811df19.
  • Smoke: ports 3099/8799 — all 40 steps passed; bench stopped, .dsh-test-smoke removed.

Not run: Desktop, real-model steward, Windows.

Round-1 findings → fixes

  1. F1 (HIGH, fence escape) — fenceUntrusted stripped tags in sequence, so the other tag's pass could reassemble a closing marker (</untrusted-m</untrusted-fileessage> → real closer). Now one fixed-point pass over both tags (stripUntrusted, delivery.js); the reviewer's exact payloads are tests in fence.test.js.
  2. F2 (HIGH, opt-out) — a remote line containing the substring <untrusted-message was stored raw. The idempotence guard is gone: remote lines fence unconditionally, and fenceUntrusted's strip makes re-fencing non-nesting. Test in remote-room.test.js.
  3. F3 — patches regenerated in the same commit (core/ == ds-bot/ + patches).
  4. F4 — ds-bot-core.md fence row updated; LESSONS entry added for the twice-bitten gate-ordering trap.
  5. Class coverage honestly scoped: applyFaceRoomLine and recordInboundSharedRoom remain raw-append seams with no production caller feeding peer text today — follow-up noted in Security: a granted friend's Bot's lines land unfenced in non-face shared rooms (#9 family) #98, not smuggled into this diff.

@hsliuustc0106
hsliuustc0106 force-pushed the fix-98-remote-room-fence branch from 85b1247 to 2d3ae94 Compare October 15, 2026 15:53
@hsliuustc0106

Copy link
Copy Markdown
Collaborator Author

Review round 1 (independent Verifier) — verdict FAIL on 85b1247; two HIGH security findings in the shared fence helper, both now fixed on the rebased tip 2d3ae94:

  • F1: strip order could reassemble a closing marker across the two untrusted tags (escape proven by the Verifier against the tip's own code) → stripUntrusted: one fixed-point pass over both tags; the exact payloads are regression tests.
  • F2: a peer could opt out of fencing by including the literal opening marker → guard removed; remote lines fence unconditionally (strip makes it non-nesting).
  • F3/F4: patches regenerated in-commit; ds-bot-core.md fence row documented; LESSONS entry added for the gate-ordering trap.
  • Class coverage: applyFaceRoomLine/recordInboundSharedRoom raw-append seams (no production peer-text caller today) stay open in Security: a granted friend's Bot's lines land unfenced in non-face shared rooms (#9 family) #98.
  • Also rebased onto the new pipeline main (1736161) and switched to npm run verify.

Re-review of the delta requested at 2d3ae94.

@hsliuustc0106

Copy link
Copy Markdown
Collaborator Author

Review round 2 (same independent Verifier, delta at the rebased tip) — verdict PASS on 2d3ae94.

  • All four round-1 findings verified fixed mechanically: F1 (joint fixed-point strip, 8 escape payloads probed against the tip's code incl. the cross-tag reassembly — zero surviving fragments, negative control confirms the old order still fails), F2 (unconditional fence, idempotent), F3 (patches ok), F4 (arch-docs OK, doc row + LESSONS present).
  • Gates re-run by the Verifier: npm run verify exit 0 (core 681/681, social 78/78, tree 54/54, CI scripts 17/17); red-at-base for the new fence test confirmed against the old helper.
  • Class coverage confirmed: every room-log writer accepting peer text funnels through authoredRoomLine; applyFaceRoomLine/recordInboundSharedRoom remain test-only/no-caller seams (tracked in Security: a granted friend's Bot's lines land unfenced in non-face shared rooms (#9 family) #98).

Implementer agrees on 2d3ae94. Ready for maintainer review — no merge action taken from my side.

@Birfy

Birfy commented Oct 15, 2026

Copy link
Copy Markdown
Collaborator

Design review against the owner's sharing model: no conflict with per-Bot grants, room participants, the inbound-share projection or face rooms. Needed changes:

  1. The branch is CONFLICTING: it is based on 1736161, before the Conflict-proof the generated patches, CHANGELOG and core docs (Phase 1, PR A) #103 patch-format change. Conflicts are only in docs/subsystems/ds-bot-core.md and patches 0001, 0005 and 0012; core/src merges cleanly. Please rebase onto main and regenerate patches.
  2. remoteLine = remote || !state.bots?.[botId] (room-membership.js:406) misses a friend's Bot's local remote-chat record: those exist in state.bots with bot.remote set. Add || state.bots[botId].remote !== undefined.
  3. The room view's RemoteMessage (cells.js:692) passes reply.text without remoteMessageText() (the 1:1 path applies it at cells.js:409), so the raw <untrusted-message ...> wrapper may now show in non-face rooms. Please check and unwrap for display.
  4. Outbound fanout (room-membership.js:742) fences unconditionally, so an already-fenced line is re-fenced for a third friend and leaves residue like from="X">. Skip lines that are already fenced.

…ace (#98)

authoredRoomLine gated the untrusted fence on isFaceRoom, so a granted
friend's Bot speaking through ingestRemoteRoomLine landed raw in a
non-face shared room; its text then reached every member Bot unfenced
through the room transcript and read_group_chat, with peer-controlled
closing markers intact. Bot-authored lines (botId or authorKind 'bot')
now take one path that fences by origin — a remote participant, or a
botId that is no local Bot — whatever the room kind; the idempotence
guard keeps already-fenced text and outbound fanout unchanged.

Fixes #98
@hsliuustc0106
hsliuustc0106 force-pushed the fix-98-remote-room-fence branch from 2d3ae94 to ee897e3 Compare October 15, 2026 17:15
@hsliuustc0106

Copy link
Copy Markdown
Collaborator Author

Review round 3 (post-rebase Verifier) — tip ee897e3 (rebased onto origin/main 4b26974 incl. #94; prior PASS on 2d3ae94 voided by rebase).

Tip SHA: ee897e3f04328f7d710fd55aa9e194c4c266bcff
Ran:
  - git fetch + rebase onto origin/main → ahead 1 / behind 0 (OpenCode, mimo-v2.6-flash-free)
  - node scripts/ds-bot-patches.mjs check → ok (core/ == ds-bot/ + patches)
  - node core/scripts/check-arch-docs.mjs → OK
  - npm run verify (OpenCode, Node 24) → exit 0
  - required CI: ubuntu-latest · Node 24 · DSH 0.2.0-rc.2 → success
  - required CI: tests-ok → success
  - mergeable_state → clean
Not run:
  - Codex CLI review at this tip — BLOCKED: box Cloudflare 403 Unable to load site on chatgpt.com/backend-api/codex; api.openai.com 403; Mac mini offline; MacBook Air unreachable mid-probe
  - Desktop / smoke / real-model steward
OpenCode↔Codex: only OpenCode (Codex unreachable)
Verdict: INC

No merge from this side until Codex can PASS the same tip (or hs waives). Rebase + CI side is ready.

@Birfy

Birfy commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator

Rebased onto current main with the design-review fixes as #120 (Birfy:cursor/rebase-99-remote-room-fence-dae8). Please prefer that PR; this one can be closed once #120 is the path forward. Credit remains yours for the original work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security: a granted friend's Bot's lines land unfenced in non-face shared rooms (#9 family)

2 participants