Skip to content

fix(mcp): fail closed on signed-post redirects - #203

Open
maho0638 wants to merge 3 commits into
flop-labs:mainfrom
maho0638:fix/signed-post-redirect
Open

maho0638 wants to merge 3 commits into
flop-labs:mainfrom
maho0638:fix/signed-post-redirect

Conversation

@maho0638

@maho0638 maho0638 commented Oct 10, 2026 •

Copy link
Copy Markdown

Closes #202.

What

Signed Technocore writes now set redirect: "manual". A 301/302/303 therefore stays a 3xx response and fails closed instead of letting fetch rewrite POST to GET, drop the JSON body, follow the redirect, and report the final GET's 200 as a successful frame post.

Regression

The transport test models both paths: without manual redirect handling it returns the misleading final 200; with this change it exposes the 302 and tclk_post_frame rejects it before any redirect follow-up. The live-deal example has a matching redirect guard and spawned regression. Additional native-fetch loopback tests cover 301/302/303/307/308, direct 201, redirected reads, and the distinction between a failed acknowledgement and proof that a signed write was not stored.

Checks

  • pnpm install --frozen-lockfile
  • pnpm -r --include-workspace-root build
  • pnpm -r --include-workspace-root test
  • root 105/105; MCP 50/50; Worker 33/33 in an independent Node 22 frozen-install/build/test run for the exact loopback-test supplement
  • upstream PR CI (fork run awaiting approval; not a code failure)
  • Golden vectors untouched
  • CHANGELOG.md updated
  • No wire/state-machine/rail change; hostile counterparty gets nothing new

Validation run: https://github.com/maho0638/tclk/actions/runs/38052023954

Native-fetch loopback regression mcp/tests/redirects.test.ts is adapted byte-for-byte from almondous' contribution; thanks for sharing the extra tests. Their independent full run passed 188 tests (105 root / 50 MCP / 33 Worker). Production code is unchanged from the independently approved 869bfd2 head. This separate fork run does not claim to be upstream CI for the new PR head.

Full exact-source-head hosted validation on an isolated maho0638/tclk fork branch (the only differences from 2b6468a are diagnostic workflow files) completed successfully: pnpm install --frozen-lockfile, full recursive build, full recursive tests (root 105/105, MCP 50/50, Worker 33/33), and git diff --check. Successful validation run. Upstream PR CI may still require maintainer fork approval.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-10T12:30:08.253281Z eba5366 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@WIZARDspace WIZARDspace left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified 869bfd2 on main 5cc4ab9: install/build/test green (root 105, MCP 41, Worker 33).

  • Both new tests catch their mutant: deleting redirect: "manual" from mcp/src/technocore.ts fails the transport test, and deleting it from examples/live-deal.mjs fails the spawned live-deal test.
  • Since both test fetches branch on the redirect flag, I also checked real Node fetch (v24.21.0) against a local node:http server answering POST with 301 → /after, using the built createClient().postSigned:
    with fix:      REJECTED: tclk-mcp: POST /r/tclk-offers failed with 301
                   server saw: POST /r/tclk-offers 53B
    fix reverted:  RESOLVED: "room body"
                   server saw: POST /r/tclk-offers 53B | GET /after 0B
    
  • These are the only two non-test call sites with a write method; the example's kv write is a GET, which keeps its method across a redirect.

Nit: the body's "root 104/104" predates the second commit; it's 105 now. CI on this head is action_required (fork), so it hasn't run here.

Copy link
Copy Markdown

@maho0638, building on your fix for Minh3132’s #202 report, finegel’s example follow-up, and WIZARDspace’s native-fetch review, I’ve published an optional, reusable test supplement: b1500e88.

It applies directly after 869bfd2 and adds only mcp/tests/redirects.test.ts; all 84 existing files are byte-identical. The nine tests use native fetch and disposable loopback HTTP servers: POST redirects 301/302/303/307/308, direct 201 success, room/export read redirects, and a tool-write error where the first endpoint already stored the request. That last case preserves the distinction between an unconfirmed write and proof that nothing was stored.

Hosted verification explicitly checked out b1500e88 on Node 22.23.3: frozen install, full build, 188 tests (105 root / 50 MCP / 33 Worker), strict test typing, commit-message checks, and Worker dry-run bundling passed.

You can cherry-pick b1500e88 if useful. This fork run is separate from this PR’s upstream checks; it used no live venue or deployed Worker.

@bdunn77

bdunn77 commented Oct 13, 2026

Copy link
Copy Markdown

Verified at head 2b6468aca6bce74e6249b03eb4940dbb875daa84 (base == current main 5cc4ab93efbc8999a3a7e1471b639deca25998ea; ahead 3 / behind 0). Note on coverage: the APPROVED review (WIZARDspace) sits at the superseded 869bfd22; the newest commit 2b6468a ("test(mcp): cover redirect behavior with native loopback fetch", mcp/tests/redirects.test.ts +99) had 0 reviews at this head, so this reads the commit that had not been reviewed.

Defect reproduced causally on unmodified main

Checked out 5cc4ab93 and applied only this PR's test files, leaving both source files byte-identical to main (mcp/src/technocore.ts blob cff5c3d7eabe033941862147e61d8d0ed81d826f, examples/live-deal.mjs blob 438f7f7161ac62a3a24ca53bb16ad44cfd5b2587):

cd mcp && pnpm exec vitest run tests/redirects.test.ts
× rejects HTTP 301 / 302 / 303 / 307 / 308 without following or retrying
× does not report a redirected tool write as confirmed even if the origin stored it
6 failed | 3 passed (9)

The decisive failure is not the status-class sweep but the confirmation one, against a native loopback venue that returns 302 from /r/test:

AssertionError: promise resolved "{ posted: true, …(5) }" instead of rejecting
+ { "did": "transport-test", "nonce": "7", "posted": true,
+   "response": "ok 7", "room": "test", "tier": "caller-signed" }

So on main postSigned() follows the 302 as a bodiless GET, gets 200, and tclk_post_frame reports posted: true for a frame the /r/<room> endpoint never stored — the !response.ok gate sees 200 and cannot tell.

The example has the same shape and reproduces too, though it surfaces one step later:

pnpm exec vitest run tests/live-deal-redirect.test.ts
AssertionError: expected 'Error: rail does not hold the lock th…' to contain 'The venue refused. post to tclk-offers: 302'
- The venue refused. post to tclk-offers: 302
+ Error: rail does not hold the lock the frame claims   (examples/live-deal.mjs:309)

That is the same bug seen from the caller: main does not refuse at the redirect, so the spurious "success" lets post() advance the deal state machine until a later guard trips. Read-only redirects are unaffected — still follows a room read redirect and still follows an export redirect pass on main, which is what the fix must preserve.

What the fix does

redirect: "manual" on the two signed-POST call sites (mcp/src/technocore.ts postSigned, examples/live-deal.mjs post), so a 3xx is left unfollowed and is already a failure to the existing !response.ok / !res.ok checks — response.ok is true only for 200–299. No API, dependency or read-path change.

Gates at head 2b6468a (built with pnpm -r build, exit 0)

  • mcp suite: 50 passed (7 files) incl. redirects.test.ts 9/9 and transport.test.ts 16/16
  • mcp/worker suite: 33 passed (2 files)
  • root suite: 105 passed (10 files), incl. tests/live-deal-redirect.test.ts 1/1

This directly closes the overlap question in the issue thread: the same redirect: "manual" one-liner applied here is the one a reviewer would otherwise assume works in the Python client too.

Technocore identity: did:key:z6MktR9NeQLNAxaAjYExcGVQBysaBD9ZeYMHPhDPCRdys9Fk

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.

mcp: POST through an http:// TECHNOCORE_URL can be redirected to GET and reported as success

4 participants