Repository navigation
Conversation
Over T3 Connect each request carries a DPoP proof that signs the request URL. The thread snapshot, bounded snapshot and history loaders signed a URL built by interpolating the raw thread id, while the request itself percent-encodes the path parameter. Ids containing `:` or `%`, such as delegated-task ids, made the signed and requested URLs differ, and the server rejected the proof with url_mismatch. The loaders now build the signed URL with the same contract-derived builder the request uses, so the two cannot diverge.
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — The PR is a focused fix that aligns DPoP-signed URLs with the encoded URLs actually sent for thread snapshots and history, with targeted coverage for delegated-task IDs. Because it changes authentication-bound request construction across production paths, human validation is warranted. You can add or adjust custom eligibility rules. Learn more. |
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe thread snapshot, bounded snapshot, and history requests now build orchestration URLs with the HTTP API URL builder. Authentication tests cover URL signing for encoded thread IDs and UUIDs. ChangesThread request URL alignment
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable issue is established for this change. It is ready to merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change aligns authentication proofs with the thread URLs actually requested. Existing authentication and read permissions remain in place, and no new bypass was identified. Production relay behavior was not verified end to end. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Note Grok responding on behalf of Julius. Thanks for this! The same fix (encoding the thread id in the DPoP-signed snapshot, bounded snapshot, and history URLs) just landed on main in #17599, which closed #15772, so I'm closing this one as superseded. If there's test coverage here that #17599 doesn't have, a small follow-up PR adding it is welcome. |
Fixes #15772
Problem
Over T3 Connect, every request carries a DPoP proof that signs the request URL. For thread snapshots, bounded snapshots and older history, the client signed a URL built by pasting the raw thread id into the path. The request itself goes through Effect's
HttpApiClient, which runs path params throughencodeURIComponent.For a UUID the two URLs are the same. Delegated-task ids (
thread:delegated-task:command%3Amcp%3A…) contain:and%, so the signed path kept them as-is while the sent path carriedthread%3A…%253A…. The server'snormalizeDpopHtucomparison failed withurl_mismatch, and opening a subagent thread returned 401. Other ids with:, such as provider child threads, hit the same mismatch.Change
boundedThreadSnapshotHttp.ts,threadSnapshotHttp.tsandthreadHistoryHttp.tsnow build the signed URL withmakeEnvironmentHttpApiUrlBuilder, the contract-derived builder thatpullRequestDiffHttp.tsalready uses. It encodes the path the same way the request does, so the signed and sent URLs cannot drift apart.The history URL now includes the
cursorquery. The web and mobile signers and the server all strip query and fragment before comparing, so the proof does not change.Only T3 Connect relay connections sign requests. Local, LAN/Tailscale cookie and bearer connections are unaffected. Web, desktop and mobile share this code in
packages/client-runtime.Verification
New cases in
environmentHttpAuth.test.tsrun each loader (snapshot, bounded, history) with a delegated-task id and a UUID. Each one sends a realHttpApiClientrequest to a mockedfetch, then checks that the signed URL equals the URLfetchreceived. It also checks the exact encoded path afternormalizeDpopHtu.…/threads/thread:delegated-task:command%3Amcp%3Arequest-1/bounded, sent…/threads/thread%3Adelegated-task%3Acommand%253Amcp%253Arequest-1/bounded?cursor=query, which signers and the server stripvp test run packages/client-runtime/src/state/environmentHttpAuth.test.ts packages/client-runtime/src/state/boundedThreadSnapshotHttp.test.tsvp run typecheckinpackages/client-runtimevp fmt --checkon the 4 changed filesvp linton the 4 changed filesvp run knip:checkWith the old source and the new tests, 6 cases fail: the 3 delegated-id cases and 3 history cases that differ only by the stripped query.
No screenshot or video: the bug only happens through the T3 Connect relay. The evidence is the comparison of the signed and sent URLs in the tests.
Not checked: a live relay connection, so no in-client capture. That the relay proxy keeps the encoded path as sent comes from the reporter's log. The signer is mocked in tests, so real proof signing and server acceptance are not exercised. Encoded cursors and a retry with a delegated id are not covered.
Implemented with Claude Opus 5.5, verified with GPT-6 Astra, coordinated by Claude Fable 5.1 in Claude Code.