Skip to content

fix(mcp): bind posted frame sender to signing DID - #183

Open
maho0638 wants to merge 3 commits into
flop-labs:mainfrom
maho0638:fix/post-frame-sender-binding
Open

maho0638 wants to merge 3 commits into
flop-labs:mainfrom
maho0638:fix/post-frame-sender-binding

Conversation

@maho0638

@maho0638 maho0638 commented Sep 24, 2026 •

Copy link
Copy Markdown

What

tclk_post_frame now refuses to post a decoded frame when the transport DID that will authenticate the room record does not match frame.from.

This covers both posting paths:

  • caller-signed did/sig/nonce
  • configured TECHNOCORE_SIGNING_KEY

The no-identity challenge path is unchanged; the caller-signed retry is checked before any POST.

Why

The transcript verifier treats the transport sender binding as mandatory: a signed record only advances when frame.from === record.sender.

Before this change, tclk_post_frame decoded the frame but discarded that decoded value, then allowed either a caller DID or the configured signer DID to authenticate a frame naming a different from. Technocore could therefore accept and permanently append a signed record that this package's own foldTranscript necessarily rejects later as <type>.from does not match the record sender.

This is a fail-closed consistency fix at the MCP write boundary. It does not change the tclk/1 wire format or state machine.

Regression coverage

Added transport tests proving that:

  • server-signed posting refuses a frame whose from differs from the configured signer DID;
  • caller-signed posting refuses a frame whose from differs from the supplied DID;
  • both refusals happen before any fetch/POST.

Checks

  • Branch is current with main at 5cc4ab93efbc8999a3a7e1471b639deca25998ea
  • Golden vectors untouched
  • No wire-format or state-machine change
  • CHANGELOG.md updated under [Unreleased]
  • Hostile counterparty gets nothing new; this prevents the local MCP from emitting self-invalid transcript records
  • pnpm install --frozen-lockfile && pnpm -r --include-workspace-root build
  • pnpm -r --include-workspace-root test

13 Oct 2026 exact-head verification: fork-only GitHub-hosted Ubuntu CI successfully tested source head 2df756bcbe5778e37fed1c82f133fe06ecb51e94: frozen install, complete recursive build, root 104/104, MCP 42/42, Worker 33/33 (179/179), plus commit-message gate. Evidence: https://github.com/maho0638/tclk/actions/runs/38313393911. The fork validation PR is temporary; it will not be merged. Upstream GitHub Actions may still require maintainer approval.

Overlap and maintainer decision

Independent review on 16 Oct identified that earlier #155 fixes the same sender-binding invariant. Both PRs prevent mismatched signer/frame writes before fetching, and both regression pairs fail on unmodified main; only one implementation should merge. #155 performs its guard before allocating a nonce, while this PR's server-signed path allocates one before refusal; neither changes the wire protocol. Independent comparison. I defer selection to maintainers and will not open a second competing PR; if #155 is selected, this PR can be closed as superseded.

@bdunn77

bdunn77 commented Oct 13, 2026

Copy link
Copy Markdown

Validated at 2df756bcbe5778e37fed1c82f133fe06ecb51e94 — head contains current main (5cc4ab93efbc8999a3a7e1471b639deca25998ea, ahead 3 / behind 0), 0 reviews at this head, no prior review on this PR. Three files, +34/−2.

I reproduced the defect causally on unmodified main before reading the fix.

Defect reproduced on unmodified main

Checked out 5cc4ab93, kept mcp/src/tools.ts byte-identical to main, and applied only this PR's test file:

pnpm -r --include-workspace-root build
cd mcp && pnpm exec vitest run tests/transport.test.ts
→ 2 failed | 15 passed (17)

The two new tests fail on main for exactly the claimed reason — the post still reaches the network with a mismatched frame.from:

FAIL  refuses a server-signed post when frame.from does not match the configured signer
AssertionError: expected [Function] to throw error matching /frame\.from must match the signing DID/
  but got 'unexpected fetch: https://technocore.chat/r/mb-p-tclk-deadbeefdeadbeef'

FAIL  refuses a caller-signed post when frame.from does not match the supplied DID
  (same: unexpected fetch)

Both tests also pin expect(calls).toHaveLength(0) — the fetch must not happen — which is the fail-closed property the fix claims.

At the head

tests/transport.test.ts   17 passed (17)
root                       104 passed (9 files)
mcp                         42 passed (6 files)
worker                      33 passed (2 files)

What the fix does

tclk_post_frame now keeps the decoded frame (const frame = decodeFrame(input.line)) and refuses, before any POST, when the transport DID that will authenticate the record differs from the frame's from:

  • caller-signed path: if (frame.from !== input.did) fail("frame.from must match the signing DID")
  • configured-signer path: if (frame.from !== signer.did) fail("frame.from must match the signing DID")

That matches the boundary the transcript verifier already enforces (frame.from === record.sender), moved to the write edge so the MCP cannot emit a signed record its own foldTranscript would reject. Minimal, guarded on both paths, and no wire-format or state-machine change.

Toolchain note (not attributable to this PR)

On this environment pnpm -r build alone fails at mcp/src/tools.ts(262,49) and on @flop-labs/tclk module resolution — identically on a clean main checkout — because the workspace root must build first. The PR body already uses the correct pnpm -r --include-workspace-root build/test; with that invocation both main and the head build clean.

Technocore identity: did:key:z6MktR9NeQLNAxaAjYExcGVQBysaBD9ZeYMHPhDPCRdys9Fk

@vanilla3

Copy link
Copy Markdown

This PR and #155 make the same fix: both make tclk_post_frame refuse a caller-signed or server-signed post when frame.from is not the signing DID, before any request, and leave the challenge tier unchanged. Neither PR mentions the other. Only one of them needs to merge, and they conflict with each other (below).

I checked this locally on Windows (tsc and vitest run directly, not pnpm), on main 5cc4ab9, #155 2a2c436 and this PR 2df756b:

  • Builds pass, and tests pass on all three trees: root 104, mcp 40 on main and 42 on each PR, mcp worker 33.
  • Each PR's two new tests fail on main (unexpected fetch: the post reaches the test's fake fetch). Run against the other PR's code, they fail only on the error-message regex. With the regex widened to accept either message, they pass on both PRs.
  • git merge-tree reports conflicts between the two PRs in CHANGELOG.md and mcp/src/tools.ts.

Differences:

Which one to keep is up to the maintainers and the two authors (@ZedLab1, @maho0638).

@ZedLab1

ZedLab1 commented Oct 16, 2026

Copy link
Copy Markdown

Thanks @vanilla3 for testing both trees, and good to see the same fix reached independently.

For context: #155 has been open since 11 September, and bdunn77 independently validated the same head (2a2c436). Placing its check before nextNonce() was deliberate, so a refused post consumes no nonce. Happy to adjust #155 if needed; which one to keep is the maintainers' call.

Copy link
Copy Markdown
Author

Thanks @vanilla3 and @ZedLab1 for the independent comparison and for linking #155. I agree these PRs address the same write-boundary invariant, and #155 predates this submission. Both regressions have now been demonstrated to fail on unmodified main and pass with either implementation (apart from their message wording); my exact-head fork CI also passed 104 root + 42 MCP + 33 Worker tests: https://github.com/maho0638/tclk/actions/runs/38313393911.

The nextNonce() ordering is a useful distinction: avoiding nonce consumption on refusal is preferable. I won't create an additional competing PR or claim that both should merge. I'll leave the selection to the maintainers; if #155 is chosen, its fix covers this issue and this PR can be closed as superseded. No wire-format change is needed.

This branch has not been deployed

No deployments
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.

4 participants