From 64457122ca45cff2a673d459d3bf79117f5ed6b0 Mon Sep 17 00:00:00 2001 From: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com> Date: Fri, 9 Oct 2026 13:30:44 -0700 Subject: [PATCH] refactor(client): sign relay request URLs built from the HttpApi contract executeAuthenticatedEnvironmentHttpRequest took a hand-written URL to sign next to the typed client call that builds the sent request, so the two could drift. Callers now pick the endpoint from the group's contract URL builder, which encodes params and query exactly like the request client. Tests assert every loader signs the exact URL it sends. Closes #17600 Co-Authored-By: Claude Opus 5.5 --- .../src/state/boundedThreadSnapshotHttp.ts | 14 +++++------ .../src/state/deviceHubAccess.ts | 2 +- .../src/state/environmentHttpAuth.test.ts | 16 +++++------- .../src/state/environmentHttpAuth.ts | 16 +++++++++--- .../src/state/pullRequestDiffHttp.ts | 7 ++---- packages/client-runtime/src/state/session.ts | 3 +-- .../src/state/shellSnapshotHttp.ts | 3 +-- .../src/state/threadHistoryHttp.ts | 25 ++++++++----------- .../src/state/threadSnapshotHttp.ts | 7 +----- 9 files changed, 41 insertions(+), 52 deletions(-) diff --git a/packages/client-runtime/src/state/boundedThreadSnapshotHttp.ts b/packages/client-runtime/src/state/boundedThreadSnapshotHttp.ts index 1a487e9b9d67..9f9b896b42c1 100644 --- a/packages/client-runtime/src/state/boundedThreadSnapshotHttp.ts +++ b/packages/client-runtime/src/state/boundedThreadSnapshotHttp.ts @@ -8,7 +8,6 @@ import { HttpClient } from "effect/http"; import * as RemoteEnvironmentAuthorization from "../authorization/service.ts"; import type { PreparedConnection } from "../connection/model.ts"; -import { environmentEndpointUrl } from "../environment/endpoint.ts"; import * as ManagedRelay from "../relay/managedRelay.ts"; import { executeAuthenticatedEnvironmentHttpRequest, @@ -35,20 +34,19 @@ export const fetchEnvironmentBoundedThreadSnapshot = Effect.fn( >; readonly timeoutMs?: number; }) { + const endpoint = { + params: { threadId: input.threadId }, + query: { compactTurnItems: "1" }, + }; return yield* executeAuthenticatedEnvironmentHttpRequest({ ...input, group: "orchestration", method: "GET", - url: (httpBaseUrl) => - environmentEndpointUrl( - httpBaseUrl, - `/api/orchestration/threads/${encodeURIComponent(input.threadId)}/bounded`, - ), + url: (urls) => urls.threadBoundedSnapshot(endpoint), timeoutMs: input.timeoutMs ?? DEFAULT_BOUNDED_THREAD_SNAPSHOT_TIMEOUT_MS, request: ({ client, headers }) => client.threadBoundedSnapshot({ - params: { threadId: input.threadId }, - query: { compactTurnItems: "1" }, + ...endpoint, headers: withOrchestrationProtocolHeader(headers), }), }).pipe( diff --git a/packages/client-runtime/src/state/deviceHubAccess.ts b/packages/client-runtime/src/state/deviceHubAccess.ts index 9681f27f6ab5..f585efbcb727 100644 --- a/packages/client-runtime/src/state/deviceHubAccess.ts +++ b/packages/client-runtime/src/state/deviceHubAccess.ts @@ -46,7 +46,7 @@ export const resolveDeviceHubAccess = Effect.fn("clientRuntime.state.resolveDevi remoteAuthorization, group: "auth", method: "POST", - url: (httpBaseUrl) => environmentEndpointUrl(httpBaseUrl, "/api/auth/websocket-ticket"), + url: (urls) => urls.webSocketTicket(), timeoutMs: TICKET_TIMEOUT_MS, request: ({ client, headers }) => client.webSocketTicket({ headers }), }); diff --git a/packages/client-runtime/src/state/environmentHttpAuth.test.ts b/packages/client-runtime/src/state/environmentHttpAuth.test.ts index 0a2df6f936a5..197f1962a716 100644 --- a/packages/client-runtime/src/state/environmentHttpAuth.test.ts +++ b/packages/client-runtime/src/state/environmentHttpAuth.test.ts @@ -294,12 +294,9 @@ describe("authenticated environment HTTP requests", () => { ); } expect(harness.authorizations).toEqual([{ expectedEnvironmentId: TARGET.environmentId }]); + // The proof signs exactly the URL sent; signers drop the query for `htu`. expect(harness.proofs).toEqual([ - { - method: loader.method, - url: `${CURRENT_ORIGIN}${loader.path}`, - accessToken: "current-token", - }, + { method: loader.method, url: call.url, accessToken: "current-token" }, ]); if (loader.name === "older thread history") { expect(url.searchParams.get("cursor")).toBe("older-page"); @@ -345,10 +342,9 @@ describe("authenticated environment HTTP requests", () => { const harness = makeHarness(() => Response.json(loader.response)); yield* loader.load(harness.input).pipe(Effect.provide(harness.httpLayer)); - const sent = new URL(harness.calls[0]!.url); - expect(sent.pathname).toContain("/mcp%3A3534bc83-"); - sent.search = ""; - expect(harness.proofs.map((proof) => proof.url)).toEqual([sent.toString()]); + const sent = harness.calls[0]!.url; + expect(new URL(sent).pathname).toContain("/mcp%3A3534bc83-"); + expect(harness.proofs.map((proof) => proof.url)).toEqual([sent]); }), ); @@ -408,7 +404,7 @@ describe("authenticated environment HTTP requests", () => { ); expect(harness.proofs[1]).toEqual({ method: "GET", - url: `${RENEWED_ORIGIN}${loader.path}`, + url: retried.url, accessToken: "renewed-token", }); if (loader.name === "older thread history") { diff --git a/packages/client-runtime/src/state/environmentHttpAuth.ts b/packages/client-runtime/src/state/environmentHttpAuth.ts index d8aa88327c7b..9693c7215b3c 100644 --- a/packages/client-runtime/src/state/environmentHttpAuth.ts +++ b/packages/client-runtime/src/state/environmentHttpAuth.ts @@ -14,6 +14,7 @@ import type { ManagedRelayDpopSigner } from "../relay/managedRelay.ts"; import { executeEnvironmentHttpRequest, makeEnvironmentHttpApiGroupClient, + makeEnvironmentHttpApiUrlBuilder, RemoteEnvironmentAuthFetchError, RemoteEnvironmentAuthTimeoutError, type RemoteEnvironmentRequestError, @@ -131,7 +132,12 @@ const executeEnvironmentRequest = Effect.fnUntraced(function* < readonly signer: Option.Option; readonly remoteAuthorization?: Option.Option; readonly method: HttpMethod.HttpMethod; - readonly url: (httpBaseUrl: string) => string; + /** + * The URL to sign, from the group's contract URL builder. It encodes path + * params exactly like the request client, so the DPoP proof matches the + * URL actually sent. + */ + readonly url: (urls: ReturnType[Group]) => string; readonly timeoutMs: number; readonly group: Group; readonly request: (input: { @@ -146,6 +152,8 @@ const executeEnvironmentRequest = Effect.fnUntraced(function* < Effect.Services>> | R > { let httpBaseUrl = input.prepared.httpBaseUrl; + const requestUrlFor = (baseUrl: string) => + input.url(makeEnvironmentHttpApiUrlBuilder(baseUrl)[input.group]); return yield* Effect.gen(function* () { let rejectedAccessToken: string | undefined; for (;;) { @@ -179,7 +187,7 @@ const executeEnvironmentRequest = Effect.fnUntraced(function* < authorization = current.httpAuthorization; } - const requestUrl = input.url(httpBaseUrl); + const requestUrl = requestUrlFor(httpBaseUrl); const client = yield* makeEnvironmentHttpApiGroupClient(httpBaseUrl, input.group); const headers = yield* buildEnvironmentAuthHeaders( authorization, @@ -225,7 +233,9 @@ const executeEnvironmentRequest = Effect.fnUntraced(function* < Effect.timeoutOrElse({ duration: input.timeoutMs, orElse: () => - Effect.fail(new RemoteEnvironmentAuthTimeoutError(input.url(httpBaseUrl), input.timeoutMs)), + Effect.fail( + new RemoteEnvironmentAuthTimeoutError(requestUrlFor(httpBaseUrl), input.timeoutMs), + ), }), ); }); diff --git a/packages/client-runtime/src/state/pullRequestDiffHttp.ts b/packages/client-runtime/src/state/pullRequestDiffHttp.ts index 1ca490cb21f7..89c56a95c44b 100644 --- a/packages/client-runtime/src/state/pullRequestDiffHttp.ts +++ b/packages/client-runtime/src/state/pullRequestDiffHttp.ts @@ -13,10 +13,7 @@ import { HttpClient } from "effect/http"; import * as RemoteEnvironmentAuthorization from "../authorization/service.ts"; import type { PreparedConnection } from "../connection/model.ts"; import * as ManagedRelay from "../relay/managedRelay.ts"; -import { - makeEnvironmentHttpApiUrlBuilder, - type RemoteEnvironmentRequestError, -} from "../rpc/http.ts"; +import { type RemoteEnvironmentRequestError } from "../rpc/http.ts"; import { executeAuthenticatedEnvironmentHttpRequest } from "./environmentHttpAuth.ts"; const DEFAULT_PULL_REQUEST_DIFF_TIMEOUT_MS = 60_000; @@ -54,7 +51,7 @@ export const fetchEnvironmentPullRequestDiff = Effect.fn( ...input, group: "pullRequests", method: "POST", - url: (httpBaseUrl) => makeEnvironmentHttpApiUrlBuilder(httpBaseUrl).pullRequests.diff(), + url: (urls) => urls.diff(), timeoutMs: input.timeoutMs ?? DEFAULT_PULL_REQUEST_DIFF_TIMEOUT_MS, request: ({ client, headers }) => client.diff({ payload: input.diff, headers }), }).pipe( diff --git a/packages/client-runtime/src/state/session.ts b/packages/client-runtime/src/state/session.ts index 63c23d735fa9..d185d3b7ee91 100644 --- a/packages/client-runtime/src/state/session.ts +++ b/packages/client-runtime/src/state/session.ts @@ -11,7 +11,6 @@ import * as RemoteEnvironmentAuthorization from "../authorization/service.ts"; import * as EnvironmentRegistry from "../connection/registry.ts"; import type { PreparedConnection } from "../connection/model.ts"; import * as EnvironmentSupervisor from "../connection/supervisor.ts"; -import { environmentEndpointUrl } from "../environment/endpoint.ts"; import * as ManagedRelay from "../relay/managedRelay.ts"; import { safeErrorLogAttributes } from "../errors/safeLog.ts"; import { executeAuthenticatedEnvironmentHttpRequest } from "./environmentHttpAuth.ts"; @@ -59,7 +58,7 @@ export const fetchEnvironmentSessionState = Effect.fn( ...input, group: "auth", method: "GET", - url: (httpBaseUrl) => environmentEndpointUrl(httpBaseUrl, "/api/auth/session"), + url: (urls) => urls.session(), timeoutMs: input.timeoutMs ?? DEFAULT_SESSION_STATE_TIMEOUT_MS, request: ({ client, headers }) => client.session({ headers }), // This endpoint returns 200 with authenticated:false for expired credentials. diff --git a/packages/client-runtime/src/state/shellSnapshotHttp.ts b/packages/client-runtime/src/state/shellSnapshotHttp.ts index 14e57be1fa67..e0294ba0dfe7 100644 --- a/packages/client-runtime/src/state/shellSnapshotHttp.ts +++ b/packages/client-runtime/src/state/shellSnapshotHttp.ts @@ -9,7 +9,6 @@ import { HttpClient, HttpClientError, type HttpClientResponse } from "effect/htt import * as RemoteEnvironmentAuthorization from "../authorization/service.ts"; import type { PreparedConnection } from "../connection/model.ts"; -import { environmentEndpointUrl } from "../environment/endpoint.ts"; import * as ManagedRelay from "../relay/managedRelay.ts"; import { executeAuthenticatedEnvironmentHttpRequest, @@ -48,7 +47,7 @@ export const fetchEnvironmentShellSnapshot = Effect.fn( ...input, group: "orchestration", method: "GET", - url: (httpBaseUrl) => environmentEndpointUrl(httpBaseUrl, "/api/orchestration/shell"), + url: (urls) => urls.shellSnapshot(), timeoutMs: input.timeoutMs ?? DEFAULT_SHELL_SNAPSHOT_TIMEOUT_MS, // The body is decoded here so each thread's pull request links can decode after its // rows. Every declared error is an `EnvironmentHttpCommonError`, so decoding one keeps diff --git a/packages/client-runtime/src/state/threadHistoryHttp.ts b/packages/client-runtime/src/state/threadHistoryHttp.ts index 316e0567305a..2d99d7dbf0e0 100644 --- a/packages/client-runtime/src/state/threadHistoryHttp.ts +++ b/packages/client-runtime/src/state/threadHistoryHttp.ts @@ -4,7 +4,6 @@ import * as Option from "effect/Option"; import type { RemoteEnvironmentAuthorization } from "../authorization/service.ts"; import type { PreparedConnection } from "../connection/model.ts"; -import { environmentEndpointUrl } from "../environment/endpoint.ts"; import * as ManagedRelay from "../relay/managedRelay.ts"; import { executeAuthenticatedEnvironmentHttpRequest, @@ -25,25 +24,21 @@ export const fetchEnvironmentThreadHistoryPage = Effect.fn( readonly remoteAuthorization?: Option.Option; readonly timeoutMs?: number; }) { + const endpoint = { + params: { threadId: input.threadId }, + query: { + cursor: input.cursor, + ...(input.view === undefined ? {} : { view: input.view }), + ...(input.throughEntryId === undefined ? {} : { throughEntryId: input.throughEntryId }), + }, + }; return yield* executeAuthenticatedEnvironmentHttpRequest({ ...input, group: "orchestration", method: "GET", - url: (httpBaseUrl) => - environmentEndpointUrl( - httpBaseUrl, - `/api/orchestration/threads/${encodeURIComponent(input.threadId)}/history`, - ), + url: (urls) => urls.threadHistoryPage(endpoint), timeoutMs: input.timeoutMs ?? DEFAULT_THREAD_HISTORY_TIMEOUT_MS, request: ({ client, headers }) => - client.threadHistoryPage({ - params: { threadId: input.threadId }, - query: { - cursor: input.cursor, - ...(input.view === undefined ? {} : { view: input.view }), - ...(input.throughEntryId === undefined ? {} : { throughEntryId: input.throughEntryId }), - }, - headers: withOrchestrationProtocolHeader(headers), - }), + client.threadHistoryPage({ ...endpoint, headers: withOrchestrationProtocolHeader(headers) }), }); }); diff --git a/packages/client-runtime/src/state/threadSnapshotHttp.ts b/packages/client-runtime/src/state/threadSnapshotHttp.ts index 905d8872d589..4987a86bf0a6 100644 --- a/packages/client-runtime/src/state/threadSnapshotHttp.ts +++ b/packages/client-runtime/src/state/threadSnapshotHttp.ts @@ -5,7 +5,6 @@ import * as Option from "effect/Option"; import * as RemoteEnvironmentAuthorization from "../authorization/service.ts"; import type { PreparedConnection } from "../connection/model.ts"; -import { environmentEndpointUrl } from "../environment/endpoint.ts"; import * as ManagedRelay from "../relay/managedRelay.ts"; import type { RemoteEnvironmentRequestError } from "../rpc/http.ts"; import { @@ -65,11 +64,7 @@ export const fetchEnvironmentThreadSnapshot = Effect.fn( ...input, group: "orchestration", method: "GET", - url: (httpBaseUrl) => - environmentEndpointUrl( - httpBaseUrl, - `/api/orchestration/threads/${encodeURIComponent(input.threadId)}`, - ), + url: (urls) => urls.threadSnapshot({ params: { threadId: input.threadId } }), timeoutMs: input.timeoutMs ?? DEFAULT_THREAD_SNAPSHOT_TIMEOUT_MS, request: ({ client, headers }) => client.threadSnapshot({