From 6267ba7b0d45271d8f5d24d6bc28fe246bd2de59 Mon Sep 17 00:00:00 2001 From: wangbill Date: Fri, 24 Jul 2026 09:51:40 -0700 Subject: [PATCH 01/21] feat(durable-functions): restore worker-side callHttp (#318) Restore the classic durable-functions v3 `context.df.callHttp` API on the v4 gRPC-based provider. The durabletask gRPC engine has no native durable-HTTP action, so the feature is reconstructed from core primitives, porting Andy Staples' durabletask-python#155 design: - Built-in HTTP activity (`BuiltIn__HttpActivity`): performs one `fetch` request, captures non-2xx/202 responses instead of throwing, guards against non-http(s) schemes (SSRF), and acquires a Managed Identity bearer token via the optional `@azure/identity` package when a tokenSource is supplied. - Built-in poll orchestrator (`BuiltIn__HttpPollOrchestrator`): a core-native async generator that re-polls while the endpoint returns `202 Accepted` with a `Location`, waiting on replay-safe durable timers that honor `Retry-After` (delta-seconds and HTTP-date forms), resolving relative Locations, and honoring the v3 `enablePolling` opt-out. - `callHttp(options)` builds the request and schedules the poll orchestrator as a sub-orchestration (single yield), returning `Task`. - Both builtins auto-register once when the package is imported. Also restores the v3 public types (`DurableHttpRequest`, `DurableHttpResponse`, `CallHttpOptions`, `ManagedIdentityTokenSource`, `TokenSource`) and documents the trust-boundary change: the HTTP request now runs as a durable activity in the app/worker process (via `fetch`), not in the Functions host extension, so egress path, outbound identity, and firewall/VNet behavior differ from v3. Managed identity requires the optional `@azure/identity` package. Fixes #318 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e1120c1a-9b20-4aef-b34a-f7986dd06382 --- package-lock.json | 4 + packages/azure-functions-durable/README.md | 17 +- packages/azure-functions-durable/package.json | 3 + packages/azure-functions-durable/src/app.ts | 15 + .../src/http/builtin.ts | 222 +++++++++++++ .../src/http/models.ts | 133 ++++++++ packages/azure-functions-durable/src/index.ts | 6 + .../src/orchestration-context.ts | 39 ++- .../unit/http-builtin-registration.spec.ts | 47 +++ .../test/unit/http-builtin.spec.ts | 291 ++++++++++++++++++ .../test/unit/orchestration-context.spec.ts | 53 +++- 11 files changed, 818 insertions(+), 12 deletions(-) create mode 100644 packages/azure-functions-durable/src/http/builtin.ts create mode 100644 packages/azure-functions-durable/src/http/models.ts create mode 100644 packages/azure-functions-durable/test/unit/http-builtin-registration.spec.ts create mode 100644 packages/azure-functions-durable/test/unit/http-builtin.spec.ts diff --git a/package-lock.json b/package-lock.json index f5976749..5e3a8f42 100644 --- a/package-lock.json +++ b/package-lock.json @@ -7,6 +7,7 @@ "": { "name": "durabletask-js-monorepo", "version": "0.0.0", + "hasInstallScript": true, "license": "MIT", "workspaces": [ "packages/*" @@ -7587,6 +7588,9 @@ }, "engines": { "node": ">=22.0.0" + }, + "optionalDependencies": { + "@azure/identity": "^4.0.0" } }, "packages/azure-functions-durable/node_modules/@types/node": { diff --git a/packages/azure-functions-durable/README.md b/packages/azure-functions-durable/README.md index d45ea14d..cc648f15 100644 --- a/packages/azure-functions-durable/README.md +++ b/packages/azure-functions-durable/README.md @@ -44,12 +44,19 @@ changed: `LockHandle` — call `release()`, ideally in a `finally`) and query with `context.entities.isInCriticalSection()`. Restoring the v3 `df.lock` / `isLocked` surface is tracked in [#317](https://github.com/microsoft/durabletask-js/issues/317). -- **`context.df.callHttp(...)` now throws.** v3 ran durable HTTP as a host-managed activity; the - consolidated gRPC backend has no equivalent primitive. Implement an HTTP activity in your app and - call it from the orchestrator. Restoring `callHttp` as a worker-side durable activity is tracked in - [#318](https://github.com/microsoft/durabletask-js/issues/318). +- **`context.df.callHttp(...)` is restored** as a worker-side durable HTTP call + ([#318](https://github.com/microsoft/durabletask-js/issues/318)). It accepts the v3 + `CallHttpOptions` (`method`, `url`, `body`, `headers`, `tokenSource`, `enablePolling`) and returns a + `Task` (`{ statusCode, headers, content }`), including automatic `202 Accepted` + polling that honors `Retry-After` via durable timers. **Trust-boundary change:** in v3 the Functions + **host** extension executed the HTTP request; here it runs as a durable **activity inside your + app/worker process** (via `fetch`). Outbound network path, source identity, and firewall/VNet rules + therefore follow the worker process, not the host — re-verify egress and any IP allow-lists. A + managed-identity `tokenSource` requires the optional + [`@azure/identity`](https://www.npmjs.com/package/@azure/identity) package + (`npm install @azure/identity`); without it, a request that uses a `tokenSource` throws a clear error. - **Some v3 top-level exports were removed** — `DummyOrchestrationContext` / `DummyEntityContext` - (testing utilities), `ManagedIdentityTokenSource`, and the entity-lock types above. `TaskFailedError` + (testing utilities) and the entity-lock types above. `TaskFailedError` is re-exported from the core SDK (aggregate failures surface as JS-native `AggregateError`); use the core `TestOrchestrationWorker` / `TestOrchestrationClient` for orchestration unit tests. - **A plain non-generator classic orchestrator is no longer supported.** A classic v3 orchestrator diff --git a/packages/azure-functions-durable/package.json b/packages/azure-functions-durable/package.json index 778799c6..70d16811 100644 --- a/packages/azure-functions-durable/package.json +++ b/packages/azure-functions-durable/package.json @@ -52,6 +52,9 @@ "@grpc/grpc-js": "^1.14.4", "@microsoft/durabletask-js": "0.4.0" }, + "optionalDependencies": { + "@azure/identity": "^4.0.0" + }, "devDependencies": { "@types/jest": "^29.5.1", "@types/node": "^22.0.0", diff --git a/packages/azure-functions-durable/src/app.ts b/packages/azure-functions-durable/src/app.ts index fccaab51..b1105233 100644 --- a/packages/azure-functions-durable/src/app.ts +++ b/packages/azure-functions-durable/src/app.ts @@ -13,6 +13,12 @@ import * as trigger from "./trigger"; import { ClassicEntity, wrapEntity } from "./entity-context"; import { ClassicOrchestrator, wrapOrchestrator } from "./orchestration-context"; import { DurableFunctionsWorker } from "./worker"; +import { + BUILTIN_HTTP_ACTIVITY_NAME, + BUILTIN_HTTP_POLL_ORCHESTRATOR_NAME, + builtinHttpActivity, + builtinHttpPollOrchestrator, +} from "./http/builtin"; // The `client` namespace groups the durable-client starter registration helpers so that // `df.app.client.http(...)` (and siblings) restore the v3 client-starter sugar. `index.ts` @@ -157,3 +163,12 @@ function extractBase64Request(triggerInput: unknown): string { } throw new TypeError("Durable trigger did not provide a base64-encoded request body."); } + +// Auto-register the built-in durable-HTTP functions exactly once when this module is first imported, +// so classic `context.df.callHttp` works with no user registration. The poll orchestrator is a +// core-native `async function*` (passed through by `wrapOrchestrator`) and the activity is a plain +// host-dispatched handler. Names are reserved (see `./http/builtin`); registering here — rather than +// per app instance — means they are wired once for the whole function app. Ported from the +// durabletask-python design (Andy Staples, durabletask-python#155). +orchestration(BUILTIN_HTTP_POLL_ORCHESTRATOR_NAME, { handler: builtinHttpPollOrchestrator }); +activity(BUILTIN_HTTP_ACTIVITY_NAME, { handler: builtinHttpActivity }); diff --git a/packages/azure-functions-durable/src/http/builtin.ts b/packages/azure-functions-durable/src/http/builtin.ts new file mode 100644 index 00000000..7fa50ef9 --- /dev/null +++ b/packages/azure-functions-durable/src/http/builtin.ts @@ -0,0 +1,222 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +/** + * Built-in durable HTTP support for the Azure Functions compatibility layer. + * + * @remarks + * The v3 `context.df.callHttp` API relied on the Durable Functions host extension to execute the + * HTTP request (including automatic `202 Accepted` polling and Managed Identity token acquisition). + * The durabletask gRPC engine this provider is built on has no native durable-HTTP action, so the + * feature is reconstructed here from core primitives: + * + * - a built-in **activity** ({@link builtinHttpActivity}) performs a single HTTP request — acquiring + * a bearer token via the optional `@azure/identity` package when a token source is supplied — and + * returns the response, and + * - a built-in **poll orchestrator** ({@link builtinHttpPollOrchestrator}) issues the request and, + * while the endpoint returns `202` with a `Location` header, waits on a durable timer (honoring + * `Retry-After`) and re-polls until the operation completes. + * + * `DurableOrchestrationContext.callHttp` schedules the poll orchestrator as a sub-orchestration, + * preserving the single-`yield` v3 ergonomics while keeping the 202 polling loop durable + * (checkpointed across restarts). + * + * Both functions are auto-registered under reserved names when this package is imported (see + * `../app.ts`) so existing apps that call `callHttp` work with no changes. Ported from the + * durabletask-python design (Andy Staples, durabletask-python#155). + */ + +import { OrchestrationContext, Task } from "@microsoft/durabletask-js"; +import { DurableHttpRequestPayload, DurableHttpResponse } from "./models"; + +/** + * Reserved built-in function names. The v3 host used `BuiltIn::HttpActivity`; `::` is not a valid + * Azure Functions function name, so `__` is used here. The reserved names are unlikely to collide + * with user-defined functions. + */ +export const BUILTIN_HTTP_ACTIVITY_NAME = "BuiltIn__HttpActivity"; +export const BUILTIN_HTTP_POLL_ORCHESTRATOR_NAME = "BuiltIn__HttpPollOrchestrator"; + +/** Fallback interval (seconds) between polls when a `202` response carries no usable `Retry-After`. */ +const DEFAULT_POLL_INTERVAL_SECONDS = 1; + +/** Case-insensitively look up `name` in `headers`. */ +function getHeader(headers: { [key: string]: string }, name: string): string | undefined { + const lowered = name.toLowerCase(); + for (const key of Object.keys(headers)) { + if (key.toLowerCase() === lowered) { + return headers[key]; + } + } + return undefined; +} + +/** + * Parse the `Retry-After` header into a delay in seconds. + * + * @remarks + * Supports both the delta-seconds and HTTP-date forms; falls back to + * {@link DEFAULT_POLL_INTERVAL_SECONDS} when absent or unparseable. For the HTTP-date form the delay + * is computed against `now` — which the caller supplies as the orchestration's replay-safe + * `currentUtcDateTime` — so the resulting timer fire time is deterministic across replays. + * + * @internal Exported for unit testing. + */ +export function retryAfterSeconds(headers: { [key: string]: string }, now: Date): number { + const raw = getHeader(headers, "Retry-After"); + if (raw === undefined || raw === null) { + return DEFAULT_POLL_INTERVAL_SECONDS; + } + const trimmed = raw.trim(); + if (/^\d+$/.test(trimmed)) { + return Math.max(parseInt(trimmed, 10), 0); + } + const retryAtMs = Date.parse(trimmed); + if (Number.isNaN(retryAtMs)) { + return DEFAULT_POLL_INTERVAL_SECONDS; + } + return Math.max(Math.floor((retryAtMs - now.getTime()) / 1000), 0); +} + +/** + * Acquire an AAD bearer token for `resource` via the optional `@azure/identity` package. + * + * @remarks + * Loaded lazily with `require` (mirroring the core SDK's optional-peer-dependency pattern) so the + * dependency is only touched when a token source is actually used; `require` also keeps the module + * out of the compiled type graph, so an app that never uses a token source needs no `@azure/identity` + * install. Throws a clear, actionable error when the package is missing but a token source was used. + */ +async function acquireBearerToken(resource: string): Promise { + let identity: { + DefaultAzureCredential: new () => { getToken(scope: string): Promise<{ token: string } | null> }; + }; + try { + identity = require("@azure/identity"); + } catch { + throw new Error( + "callHttp with a tokenSource requires the optional '@azure/identity' package. " + + "Install it with `npm install @azure/identity`.", + ); + } + const credential = new identity.DefaultAzureCredential(); + const scope = resource.replace(/\/+$/, "") + "/.default"; + const result = await credential.getToken(scope); + const token = result?.token; + if (!token) { + throw new Error(`Failed to acquire a bearer token for resource '${resource}'.`); + } + return token; +} + +/** + * Built-in activity: execute a single HTTP request and return the response. + * + * @remarks + * `input` is the JSON form of a durable HTTP request (`method`, `uri`, `content`, `headers`, + * `tokenSource`). Non-2xx responses (including `202`) are captured rather than thrown — the global + * `fetch` only rejects on network errors, not on HTTP status — so the poll orchestrator can inspect + * the status code and headers. Only http/https URIs are permitted (an SSRF guard that closes off + * `file://`, `ftp://`, ... schemes from orchestration-supplied URLs). + */ +export async function builtinHttpActivity(input: DurableHttpRequestPayload): Promise { + const request = input ?? ({} as DurableHttpRequestPayload); + const method = String(request.method ?? "GET").toUpperCase(); + const uri = request.uri; + if (!uri) { + throw new Error("A non-empty 'uri' is required for a durable HTTP call."); + } + // Durable HTTP only ever means http(s); reject other schemes (file://, ftp://, ...) that fetch + // (or a redirect) might otherwise honor, closing off local-file reads / SSRF to non-HTTP endpoints. + let scheme: string; + try { + scheme = new URL(uri).protocol.replace(/:$/, "").toLowerCase(); + } catch { + throw new Error(`callHttp only supports http/https URLs; got ${JSON.stringify(uri)}.`); + } + if (scheme !== "http" && scheme !== "https") { + throw new Error(`callHttp only supports http/https URLs; got ${JSON.stringify(uri)}.`); + } + + const headers: { [key: string]: string } = { ...(request.headers ?? {}) }; + const resource = request.tokenSource?.resource; + if (resource) { + const token = await acquireBearerToken(resource); + if (headers["Authorization"] === undefined) { + headers["Authorization"] = `Bearer ${token}`; + } + } + + // `content` was already serialized to a string by `callHttp`, so it is sent as-is. GET/HEAD + // requests cannot carry a body under the fetch spec, so a body is only attached for other methods. + const includeBody = typeof request.content === "string" && method !== "GET" && method !== "HEAD"; + const response = await fetch(uri, { + method, + headers, + body: includeBody ? request.content : undefined, + }); + + const responseHeaders: { [key: string]: string } = {}; + response.headers.forEach((value, key) => { + responseHeaders[key] = value; + }); + const content = await response.text(); + + return { statusCode: response.status, headers: responseHeaders, content }; +} + +/** + * Built-in poll orchestrator: issue a durable HTTP request and poll while it returns `202`. + * + * @remarks + * Written as a core-native async generator so the durabletask engine drives it directly (the + * orchestration input arrives as the second argument). It calls the built-in HTTP activity and, + * while the response is `202 Accepted` with a `Location` header (and polling is enabled), waits on a + * durable timer (honoring `Retry-After`) before re-polling the `Location` URL, resolving a relative + * `Location` against the current request URI. Returns the final response. All time math uses the + * replay-safe `currentUtcDateTime`, never `Date.now()`, so replays are deterministic. + */ +export async function* builtinHttpPollOrchestrator( + ctx: OrchestrationContext, + input: DurableHttpRequestPayload, +): AsyncGenerator, DurableHttpResponse, unknown> { + const request = input ?? ({} as DurableHttpRequestPayload); + // v3 opt-out: when polling is disabled the first response is returned as-is (no 202 loop). + const enablePolling = request.enablePolling !== false; + + let response = (yield ctx.callActivity(BUILTIN_HTTP_ACTIVITY_NAME, request)) as DurableHttpResponse; + // Track the URI of the most recent request so a relative `Location` can be resolved against it. + let currentUri = String(request.uri ?? ""); + + while (enablePolling && response.statusCode === 202) { + const headers = response.headers ?? {}; + const location = getHeader(headers, "Location"); + if (!location) { + // Cannot poll without a Location; return the 202 as-is. + break; + } + + // A `Location` may be relative (e.g. `/operations/42`); resolve it against the current request + // URI so the next poll targets an absolute http(s) URL (the activity rejects non-absolute URIs). + const resolved = new URL(location, currentUri).toString(); + + const now = ctx.currentUtcDateTime; + const delaySeconds = retryAfterSeconds(headers, now); + const fireAt = new Date(now.getTime() + delaySeconds * 1000); + yield ctx.createTimer(fireAt); + + const pollRequest: DurableHttpRequestPayload = { method: "GET", uri: resolved, enablePolling }; + // Preserve auth for the polling requests. + if (request.headers !== undefined) { + pollRequest.headers = request.headers; + } + if (request.tokenSource !== undefined) { + pollRequest.tokenSource = request.tokenSource; + } + + currentUri = resolved; + response = (yield ctx.callActivity(BUILTIN_HTTP_ACTIVITY_NAME, pollRequest)) as DurableHttpResponse; + } + + return response; +} diff --git a/packages/azure-functions-durable/src/http/models.ts b/packages/azure-functions-durable/src/http/models.ts new file mode 100644 index 00000000..7e824b68 --- /dev/null +++ b/packages/azure-functions-durable/src/http/models.ts @@ -0,0 +1,133 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +/** + * Durable HTTP request/response models (durable-functions v3 compatible). + * + * @remarks + * In v3 the Durable Functions host extension executed `context.df.callHttp` natively (including + * `202 Accepted` polling and Managed Identity token acquisition). The durabletask gRPC engine this + * provider is built on has no native durable-HTTP action, so the feature is reconstructed from core + * primitives (a built-in activity + a built-in polling sub-orchestration — see `./builtin`). These + * types mirror the v3 public shapes and double as the JSON wire payload exchanged with the built-in + * activity. Ported from the durabletask-python design (Andy Staples, durabletask-python#155). + */ + +/** + * The source of an OAuth token to attach to a durable HTTP request. + * + * @remarks + * Mirrors the classic durable-functions v3 `TokenSource` union, which currently has a single + * implementation, {@link ManagedIdentityTokenSource}. + */ +export type TokenSource = ManagedIdentityTokenSource; + +/** + * Token source backed by an [Azure Managed Identity](https://learn.microsoft.com/azure/active-directory/managed-identities-azure-resources/overview). + * + * @remarks + * v3-compatible: exposes the target `resource` and a `kind` discriminator. When a request carrying + * this token source reaches the built-in HTTP activity, a bearer token is acquired for `resource` + * via the optional `@azure/identity` package and added as an `Authorization: Bearer ` header. + */ +export class ManagedIdentityTokenSource { + /** @hidden Discriminator matching the classic durable-functions v3 token-source shape. */ + readonly kind: string = "AzureManagedIdentity"; + + /** + * @param resource The Azure Active Directory resource identifier of the web API being invoked, + * e.g. `https://management.core.windows.net/` or `https://graph.microsoft.com/`. + */ + constructor(public readonly resource: string) {} +} + +/** + * Options accepted by `context.df.callHttp` (classic durable-functions v3 shape). + */ +export interface CallHttpOptions { + /** The HTTP request method. */ + method: string; + /** The HTTP request URL. */ + url: string; + /** The HTTP request body. An `object` is JSON-serialized; a `string` is sent as-is. */ + body?: string | object; + /** The HTTP request headers. */ + headers?: { [key: string]: string }; + /** The source of the OAuth token to add to the request. */ + tokenSource?: TokenSource; + /** + * Whether to keep polling the request after receiving a `202 Accepted` response. Replaces the + * deprecated `asynchronousPatternEnabled`; if both are specified, `enablePolling` takes precedence. + * + * @default true + */ + enablePolling?: boolean; + /** + * @deprecated Use `enablePolling` instead. If both are specified, `enablePolling` takes precedence. + */ + asynchronousPatternEnabled?: boolean; +} + +/** + * The response returned by `context.df.callHttp` (classic durable-functions v3 shape). + * + * @remarks + * The value crosses the sub-orchestration boundary as JSON, so `callHttp` resolves to a plain object + * of this shape. Consumers read `response.statusCode` / `response.content` / `response.headers`. + */ +export interface DurableHttpResponse { + /** The HTTP response status code. */ + statusCode: number; + /** The HTTP response headers (keys are lower-cased by the underlying `fetch` implementation). */ + headers: { [key: string]: string }; + /** The HTTP response body. */ + content?: string; +} + +/** + * Data structure representing a durable HTTP request (classic durable-functions v3 shape). + * + * @remarks + * Exported for import compatibility with durable-functions v3. `callHttp` builds the internal + * {@link DurableHttpRequestPayload} wire form directly rather than constructing this class. + */ +export class DurableHttpRequest { + /** + * @param method The HTTP request method. + * @param uri The HTTP request URL. + * @param content The HTTP request content. + * @param headers The HTTP request headers. + * @param tokenSource The source of the OAuth token to add to the request. + * @param asynchronousPatternEnabled Whether the request should handle the asynchronous (202) pattern. + */ + constructor( + public readonly method: string, + public readonly uri: string, + public readonly content?: string, + public readonly headers?: { [key: string]: string }, + public readonly tokenSource?: TokenSource, + public readonly asynchronousPatternEnabled: boolean = true, + ) {} +} + +/** + * @hidden + * Internal JSON wire payload exchanged between `callHttp`, the built-in poll orchestrator, and the + * built-in HTTP activity. Uses `uri` (not `url`) and pre-serialized `content` (not `body`), matching + * the v3 `DurableHttpRequest` wire shape, plus an `enablePolling` flag so the poll orchestrator can + * honor the v3 opt-out of `202` polling. + */ +export interface DurableHttpRequestPayload { + /** The HTTP request method. */ + method: string; + /** The absolute http(s) request URI. */ + uri: string; + /** The pre-serialized HTTP request body. */ + content?: string; + /** The HTTP request headers. */ + headers?: { [key: string]: string }; + /** The OAuth token source (JSON form); only `resource` is required by the activity. */ + tokenSource?: { kind?: string; resource: string }; + /** Whether to keep polling on `202 Accepted`. Defaults to `true` when absent. */ + enablePolling?: boolean; +} diff --git a/packages/azure-functions-durable/src/index.ts b/packages/azure-functions-durable/src/index.ts index 514ca36e..0b6fe23f 100644 --- a/packages/azure-functions-durable/src/index.ts +++ b/packages/azure-functions-durable/src/index.ts @@ -47,6 +47,12 @@ export { EntityStateResponse } from "./entity-state-response"; export { PurgeHistoryResult } from "./purge-history-result"; export { OrchestrationFilter } from "./orchestration-filter"; +// Durable HTTP (classic v3 `context.df.callHttp`) models. Value exports (classes) and type exports +// so `import { DurableHttpRequest, ManagedIdentityTokenSource, CallHttpOptions, DurableHttpResponse, +// TokenSource } from ...` resolves as it did in durable-functions v3. +export { DurableHttpRequest, ManagedIdentityTokenSource } from "./http/models"; +export type { CallHttpOptions, DurableHttpResponse, TokenSource } from "./http/models"; + // Legacy durable-functions v3 API compatibility aliases (types only). These let orchestrator/ // activity code written against the classic `durable-functions` v3 API type-check unchanged. export type { ActivityHandler } from "./app"; diff --git a/packages/azure-functions-durable/src/orchestration-context.ts b/packages/azure-functions-durable/src/orchestration-context.ts index e839d41d..451a7753 100644 --- a/packages/azure-functions-durable/src/orchestration-context.ts +++ b/packages/azure-functions-durable/src/orchestration-context.ts @@ -12,6 +12,8 @@ import { whenAny, } from "@microsoft/durabletask-js"; import { RetryOptions } from "./retry-options"; +import { BUILTIN_HTTP_POLL_ORCHESTRATOR_NAME } from "./http/builtin"; +import { CallHttpOptions, DurableHttpRequestPayload, DurableHttpResponse } from "./http/models"; /** * Classic Durable Functions (v3) orchestration context, exposed to migrating orchestrators as @@ -150,11 +152,40 @@ export class DurableOrchestrationContext { * Schedules a durable HTTP call (classic v3 API). * * @remarks - * Not supported: the durabletask engine has no durable-HTTP (`callHttp`) equivalent, so this - * throws. This mirrors the Python provider, which raises for the same reason. + * The durabletask gRPC engine has no native durable-HTTP action (unlike the v3 Durable Functions + * host extension, which executed the request itself), so this is reconstructed from core + * primitives: the request is handed to a built-in polling sub-orchestration + * ({@link BUILTIN_HTTP_POLL_ORCHESTRATOR_NAME}) that runs a built-in HTTP activity and, while the + * endpoint returns `202 Accepted`, waits on durable timers (honoring `Retry-After`) and re-polls + * until completion. The whole flow is a single `yield`, preserving the v3 ergonomics. + * + * Trust-boundary change vs v3: the HTTP request now runs as a durable **activity inside the app / + * worker process** (via `fetch`), not in the Functions host extension. Outbound network path, + * identity, and firewall/VNet behavior therefore follow the worker process. Managed-identity + * token acquisition (`tokenSource`) requires the optional `@azure/identity` package. + * + * @returns A {@link Task} resolving to the final {@link DurableHttpResponse}. */ - callHttp(_options: unknown): never { - throw new Error("callHttp is not supported: the durabletask engine has no durable-HTTP (callHttp) equivalent."); + callHttp(options: CallHttpOptions): Task { + const enablePolling = options.enablePolling ?? options.asynchronousPatternEnabled ?? true; + const request: DurableHttpRequestPayload = { + method: options.method, + uri: options.url, + enablePolling, + }; + if (options.body !== undefined) { + request.content = typeof options.body === "string" ? options.body : JSON.stringify(options.body); + } + if (options.headers !== undefined) { + request.headers = options.headers; + } + if (options.tokenSource !== undefined) { + request.tokenSource = { kind: options.tokenSource.kind, resource: options.tokenSource.resource }; + } + return this._ctx.callSubOrchestrator( + BUILTIN_HTTP_POLL_ORCHESTRATOR_NAME, + request, + ); } /** Calls an entity operation and waits for its result. */ diff --git a/packages/azure-functions-durable/test/unit/http-builtin-registration.spec.ts b/packages/azure-functions-durable/test/unit/http-builtin-registration.spec.ts new file mode 100644 index 00000000..3d788cc4 --- /dev/null +++ b/packages/azure-functions-durable/test/unit/http-builtin-registration.spec.ts @@ -0,0 +1,47 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { BUILTIN_HTTP_ACTIVITY_NAME, BUILTIN_HTTP_POLL_ORCHESTRATOR_NAME } from "../../src/http/builtin"; + +// The built-in durable-HTTP functions are auto-registered as a side effect of importing `../../src/app`. +// `jest.doMock` (not hoisted) lets us install a fresh `app.generic` spy BEFORE the module is required +// inside `jest.isolateModules`, so the import-time registration calls are captured deterministically. +describe("built-in durable HTTP auto-registration", () => { + it("registers both built-in functions when the app module is imported", () => { + const mockGeneric = jest.fn(); + + jest.isolateModules(() => { + jest.doMock("@azure/functions", () => { + const actual = jest.requireActual("@azure/functions"); + return { ...actual, app: { ...actual.app, generic: mockGeneric } }; + }); + require("../../src/app"); + }); + + const registeredNames = mockGeneric.mock.calls.map((call) => call[0] as string); + expect(registeredNames).toContain(BUILTIN_HTTP_POLL_ORCHESTRATOR_NAME); + expect(registeredNames).toContain(BUILTIN_HTTP_ACTIVITY_NAME); + + const pollRegistration = mockGeneric.mock.calls.find( + (call) => call[0] === BUILTIN_HTTP_POLL_ORCHESTRATOR_NAME, + ); + const activityRegistration = mockGeneric.mock.calls.find((call) => call[0] === BUILTIN_HTTP_ACTIVITY_NAME); + + // The poll orchestrator is wired as a durable orchestration trigger; the HTTP worker as an activity. + expect((pollRegistration?.[1] as { trigger: { type: string } }).trigger.type).toBe("orchestrationTrigger"); + expect((activityRegistration?.[1] as { trigger: { type: string } }).trigger.type).toBe("activityTrigger"); + }); + + it("registers the built-in orchestrator on the shared worker so it can be dispatched by name", () => { + let registeredOrchestratorNames: string[] = []; + + jest.isolateModules(() => { + const { getSharedWorker } = require("../../src/app") as typeof import("../../src/app"); + // `_registry` is a TypeScript-private field (accessible at runtime) exposing the registered names. + const worker = getSharedWorker() as unknown as { _registry: { getOrchestratorNames(): string[] } }; + registeredOrchestratorNames = worker._registry.getOrchestratorNames(); + }); + + expect(registeredOrchestratorNames).toContain(BUILTIN_HTTP_POLL_ORCHESTRATOR_NAME); + }); +}); diff --git a/packages/azure-functions-durable/test/unit/http-builtin.spec.ts b/packages/azure-functions-durable/test/unit/http-builtin.spec.ts new file mode 100644 index 00000000..06311dfa --- /dev/null +++ b/packages/azure-functions-durable/test/unit/http-builtin.spec.ts @@ -0,0 +1,291 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { OrchestrationContext, Task } from "@microsoft/durabletask-js"; +import { + BUILTIN_HTTP_ACTIVITY_NAME, + builtinHttpActivity, + builtinHttpPollOrchestrator, + retryAfterSeconds, +} from "../../src/http/builtin"; +import { DurableHttpRequestPayload, DurableHttpResponse } from "../../src/http/models"; + +// `@azure/identity` is an OPTIONAL dependency loaded lazily via `require` inside the activity, and is +// not installed in this workspace — a virtual mock stands in so the token-acquisition path can be +// exercised and the REAL (mocked) token asserted on the outgoing request. +const mockGetToken = jest.fn(async (_scope: string) => ({ token: "REAL_TOKEN_123" })); +jest.mock( + "@azure/identity", + () => ({ + DefaultAzureCredential: jest.fn().mockImplementation(() => ({ getToken: mockGetToken })), + }), + { virtual: true }, +); + +/** A minimal fetch Response stand-in (avoids depending on the global `Response` constructor). */ +function fakeResponse(status: number, headers: { [key: string]: string }, body: string) { + return { + status, + headers: { + forEach: (cb: (value: string, key: string) => void) => + Object.entries(headers).forEach(([key, value]) => cb(value, key)), + }, + text: async () => body, + }; +} + +/** + * Builds a `fetch` mock with a typed `(input, init)` signature so `mock.calls[i][1]` is a + * `RequestInit` (a bare `jest.fn(async () => ...)` types its calls as an empty tuple). + */ +function makeFetchMock(response: ReturnType) { + return jest.fn((_input: string, _init?: RequestInit) => Promise.resolve(response)); +} + +describe("retryAfterSeconds", () => { + const now = new Date("2026-01-01T00:00:00.000Z"); + + it("parses the delta-seconds form", () => { + expect(retryAfterSeconds({ "Retry-After": "5" }, now)).toBe(5); + }); + + it("parses the HTTP-date form relative to the replay-safe clock", () => { + expect(retryAfterSeconds({ "Retry-After": "Thu, 01 Jan 2026 00:00:10 GMT" }, now)).toBe(10); + }); + + it("is case-insensitive on the header name", () => { + expect(retryAfterSeconds({ "retry-after": "7" }, now)).toBe(7); + }); + + it("falls back to 1s when the header is missing or unparseable", () => { + expect(retryAfterSeconds({}, now)).toBe(1); + expect(retryAfterSeconds({ "Retry-After": "not-a-date" }, now)).toBe(1); + }); + + it("never returns a negative delay for a past HTTP-date", () => { + expect(retryAfterSeconds({ "Retry-After": "Thu, 01 Jan 2026 00:00:00 GMT" }, new Date("2026-01-01T00:01:00.000Z"))).toBe( + 0, + ); + }); +}); + +describe("builtinHttpActivity", () => { + const originalFetch = global.fetch; + + afterEach(() => { + global.fetch = originalFetch; + jest.clearAllMocks(); + }); + + it("performs the request and passes the 200 response through", async () => { + const fetchMock = makeFetchMock(fakeResponse(200, { "content-type": "text/plain" }, "hello")); + global.fetch = fetchMock as unknown as typeof fetch; + + const response = await builtinHttpActivity({ method: "GET", uri: "https://example.test/data" }); + + expect(fetchMock).toHaveBeenCalledTimes(1); + const [calledUri, calledInit] = fetchMock.mock.calls[0] as unknown as [string, RequestInit]; + expect(calledUri).toBe("https://example.test/data"); + expect(calledInit.method).toBe("GET"); + expect(calledInit.body).toBeUndefined(); + expect(response).toEqual({ + statusCode: 200, + headers: { "content-type": "text/plain" }, + content: "hello", + }); + }); + + it("sends a body for non-GET/HEAD methods but omits it for GET", async () => { + const fetchMock = makeFetchMock(fakeResponse(200, {}, "")); + global.fetch = fetchMock as unknown as typeof fetch; + + await builtinHttpActivity({ method: "POST", uri: "https://example.test/", content: '{"a":1}' }); + expect((fetchMock.mock.calls[0][1] as RequestInit).body).toBe('{"a":1}'); + + await builtinHttpActivity({ method: "GET", uri: "https://example.test/", content: '{"a":1}' }); + expect((fetchMock.mock.calls[1][1] as RequestInit).body).toBeUndefined(); + }); + + it("throws when the uri is missing", async () => { + global.fetch = jest.fn() as unknown as typeof fetch; + await expect(builtinHttpActivity({ method: "GET" } as DurableHttpRequestPayload)).rejects.toThrow(/uri/i); + }); + + it("rejects non-http/https schemes (SSRF guard)", async () => { + global.fetch = jest.fn() as unknown as typeof fetch; + await expect( + builtinHttpActivity({ method: "GET", uri: "file:///etc/passwd" }), + ).rejects.toThrow(/http\/https/); + await expect(builtinHttpActivity({ method: "GET", uri: "ftp://host/f" })).rejects.toThrow(/http\/https/); + }); + + it("captures a 202 response instead of throwing", async () => { + const fetchMock = makeFetchMock(fakeResponse(202, { location: "https://example.test/op/1" }, "")); + global.fetch = fetchMock as unknown as typeof fetch; + + const response = await builtinHttpActivity({ method: "POST", uri: "https://example.test/start" }); + + expect(response.statusCode).toBe(202); + expect(response.headers.location).toBe("https://example.test/op/1"); + }); + + it("acquires a real bearer token via @azure/identity when a tokenSource is present", async () => { + const fetchMock = makeFetchMock(fakeResponse(200, {}, "ok")); + global.fetch = fetchMock as unknown as typeof fetch; + + await builtinHttpActivity({ + method: "GET", + uri: "https://example.test/secure", + tokenSource: { kind: "AzureManagedIdentity", resource: "https://management.core.windows.net/" }, + }); + + // The scope is the resource with any trailing slashes stripped, plus `/.default`. + expect(mockGetToken).toHaveBeenCalledWith("https://management.core.windows.net/.default"); + const sentHeaders = (fetchMock.mock.calls[0][1] as RequestInit).headers as { [key: string]: string }; + // The REAL token is forwarded — never a masked placeholder. + expect(sentHeaders["Authorization"]).toBe("Bearer REAL_TOKEN_123"); + }); + + it("does not overwrite an explicit Authorization header", async () => { + const fetchMock = makeFetchMock(fakeResponse(200, {}, "ok")); + global.fetch = fetchMock as unknown as typeof fetch; + + await builtinHttpActivity({ + method: "GET", + uri: "https://example.test/secure", + headers: { Authorization: "Bearer caller-supplied" }, + tokenSource: { resource: "https://graph.microsoft.com/" }, + }); + + const sentHeaders = (fetchMock.mock.calls[0][1] as RequestInit).headers as { [key: string]: string }; + expect(sentHeaders["Authorization"]).toBe("Bearer caller-supplied"); + }); +}); + +/** Drives the poll orchestrator generator with a fake core context, feeding activity/timer results. */ +function createPollContext(now: Date) { + const ctx = { + currentUtcDateTime: now, + callActivity: jest.fn((name: string, input: unknown) => ({ kind: "activity", name, input }) as unknown as Task), + createTimer: jest.fn((fireAt: Date | number) => ({ kind: "timer", fireAt }) as unknown as Task), + }; + return { ctx: ctx as unknown as OrchestrationContext, raw: ctx }; +} + +describe("builtinHttpPollOrchestrator", () => { + const now = new Date("2026-01-01T00:00:00.000Z"); + + it("returns the first response immediately when it is not a 202 (no timer)", async () => { + const { ctx, raw } = createPollContext(now); + const gen = builtinHttpPollOrchestrator(ctx, { method: "GET", uri: "https://host/api", enablePolling: true }); + + const firstYield = await gen.next(); + expect(raw.callActivity).toHaveBeenCalledWith( + BUILTIN_HTTP_ACTIVITY_NAME, + expect.objectContaining({ method: "GET", uri: "https://host/api" }), + ); + expect(firstYield.done).toBe(false); + + const ok: DurableHttpResponse = { statusCode: 200, headers: {}, content: "done" }; + const result = await gen.next(ok); + expect(result.done).toBe(true); + expect(result.value).toEqual(ok); + expect(raw.createTimer).not.toHaveBeenCalled(); + }); + + it("polls on 202+Location: activity, then durable timer, then re-polls Location by GET", async () => { + const { ctx, raw } = createPollContext(now); + const request: DurableHttpRequestPayload = { + method: "POST", + uri: "https://host/api/start", + enablePolling: true, + headers: { "x-custom": "1" }, + tokenSource: { resource: "https://management.core.windows.net/" }, + }; + const gen = builtinHttpPollOrchestrator(ctx, request); + + // 1) initial activity + await gen.next(); + expect(raw.callActivity).toHaveBeenNthCalledWith(1, BUILTIN_HTTP_ACTIVITY_NAME, request); + + // 2) 202 -> a durable timer honoring Retry-After (5s) is created + const afterFirst = await gen.next({ + statusCode: 202, + headers: { Location: "https://host/api/status/1", "Retry-After": "5" }, + }); + expect(afterFirst.done).toBe(false); + expect(raw.createTimer).toHaveBeenCalledTimes(1); + expect(raw.createTimer).toHaveBeenCalledWith(new Date("2026-01-01T00:00:05.000Z")); + + // 3) after the timer, re-poll the Location with GET, carrying headers + tokenSource + const afterTimer = await gen.next(); + expect(afterTimer.done).toBe(false); + expect(raw.callActivity).toHaveBeenNthCalledWith(2, BUILTIN_HTTP_ACTIVITY_NAME, { + method: "GET", + uri: "https://host/api/status/1", + enablePolling: true, + headers: { "x-custom": "1" }, + tokenSource: { resource: "https://management.core.windows.net/" }, + }); + + // 4) final 200 completes the orchestration + const done = await gen.next({ statusCode: 200, headers: {}, content: "final" }); + expect(done.done).toBe(true); + expect(done.value).toEqual({ statusCode: 200, headers: {}, content: "final" }); + }); + + it("honors an HTTP-date Retry-After when scheduling the poll timer", async () => { + const { ctx, raw } = createPollContext(now); + const gen = builtinHttpPollOrchestrator(ctx, { method: "GET", uri: "https://host/api", enablePolling: true }); + + await gen.next(); + await gen.next({ + statusCode: 202, + headers: { Location: "https://host/api/status", "Retry-After": "Thu, 01 Jan 2026 00:00:30 GMT" }, + }); + expect(raw.createTimer).toHaveBeenCalledWith(new Date("2026-01-01T00:00:30.000Z")); + }); + + it("resolves a relative Location against the current request URI", async () => { + const { ctx, raw } = createPollContext(now); + const gen = builtinHttpPollOrchestrator(ctx, { method: "POST", uri: "https://host/api/op", enablePolling: true }); + + await gen.next(); + await gen.next({ statusCode: 202, headers: { Location: "/status/42", "Retry-After": "1" } }); + await gen.next(); // advance past the timer to the second poll + + expect(raw.callActivity).toHaveBeenNthCalledWith( + 2, + BUILTIN_HTTP_ACTIVITY_NAME, + expect.objectContaining({ method: "GET", uri: "https://host/status/42" }), + ); + }); + + it("returns the first 202 without looping when polling is disabled", async () => { + const { ctx, raw } = createPollContext(now); + const gen = builtinHttpPollOrchestrator(ctx, { method: "GET", uri: "https://host/api", enablePolling: false }); + + await gen.next(); + const result = await gen.next({ + statusCode: 202, + headers: { Location: "https://host/api/status", "Retry-After": "5" }, + }); + + expect(result.done).toBe(true); + expect((result.value as DurableHttpResponse).statusCode).toBe(202); + expect(raw.createTimer).not.toHaveBeenCalled(); + expect(raw.callActivity).toHaveBeenCalledTimes(1); + }); + + it("stops polling when a 202 has no Location header", async () => { + const { ctx, raw } = createPollContext(now); + const gen = builtinHttpPollOrchestrator(ctx, { method: "GET", uri: "https://host/api", enablePolling: true }); + + await gen.next(); + const result = await gen.next({ statusCode: 202, headers: {} }); + + expect(result.done).toBe(true); + expect((result.value as DurableHttpResponse).statusCode).toBe(202); + expect(raw.createTimer).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/azure-functions-durable/test/unit/orchestration-context.spec.ts b/packages/azure-functions-durable/test/unit/orchestration-context.spec.ts index add2a1bf..eb8f7e2d 100644 --- a/packages/azure-functions-durable/test/unit/orchestration-context.spec.ts +++ b/packages/azure-functions-durable/test/unit/orchestration-context.spec.ts @@ -21,6 +21,7 @@ import { wrapOrchestrator, } from "../../src/orchestration-context"; import { RetryOptions } from "../../src/retry-options"; +import { BUILTIN_HTTP_POLL_ORCHESTRATOR_NAME } from "../../src/http/builtin"; /** Builds a fake core OrchestrationContext whose methods return sentinel values via jest mocks. */ function createFakeCoreContext() { @@ -150,11 +151,57 @@ describe("DurableOrchestrationContext", () => { expect(entities.signalEntity).toHaveBeenCalledWith(entityId, "reset", undefined); }); - it("throws for callHttp, which has no durabletask equivalent", () => { - const { ctx } = createFakeCoreContext(); + it("schedules callHttp as the built-in poll sub-orchestration with the built request payload", () => { + const { ctx, raw } = createFakeCoreContext(); + const df = new DurableOrchestrationContext(ctx, undefined); + + // A string body is sent as-is; polling defaults to enabled. + const task = df.callHttp({ method: "GET", url: "https://example.test/api", headers: { "x-a": "1" } }); + expect(task).toBe("callSub-task"); + expect(raw.callSubOrchestrator).toHaveBeenCalledWith(BUILTIN_HTTP_POLL_ORCHESTRATOR_NAME, { + method: "GET", + uri: "https://example.test/api", + enablePolling: true, + headers: { "x-a": "1" }, + }); + }); + + it("JSON-stringifies an object body and forwards the token source when scheduling callHttp", () => { + const { ctx, raw } = createFakeCoreContext(); + const df = new DurableOrchestrationContext(ctx, undefined); + + df.callHttp({ + method: "POST", + url: "https://example.test/api", + body: { hello: "world" }, + tokenSource: { kind: "AzureManagedIdentity", resource: "https://management.core.windows.net/" } as never, + }); + + expect(raw.callSubOrchestrator).toHaveBeenCalledWith(BUILTIN_HTTP_POLL_ORCHESTRATOR_NAME, { + method: "POST", + uri: "https://example.test/api", + enablePolling: true, + content: JSON.stringify({ hello: "world" }), + tokenSource: { kind: "AzureManagedIdentity", resource: "https://management.core.windows.net/" }, + }); + }); + + it("honors enablePolling=false (and the deprecated asynchronousPatternEnabled alias) for callHttp", () => { + const { ctx, raw } = createFakeCoreContext(); const df = new DurableOrchestrationContext(ctx, undefined); - expect(() => df.callHttp({ method: "GET", uri: "https://example.test" })).toThrow(/callHttp/); + df.callHttp({ method: "GET", url: "https://example.test/api", enablePolling: false }); + expect(raw.callSubOrchestrator).toHaveBeenLastCalledWith( + BUILTIN_HTTP_POLL_ORCHESTRATOR_NAME, + expect.objectContaining({ enablePolling: false }), + ); + + // enablePolling takes precedence over the deprecated alias when both are present. + df.callHttp({ method: "GET", url: "https://example.test/api", asynchronousPatternEnabled: false }); + expect(raw.callSubOrchestrator).toHaveBeenLastCalledWith( + BUILTIN_HTTP_POLL_ORCHESTRATOR_NAME, + expect.objectContaining({ enablePolling: false }), + ); }); it("exposes Task.all / Task.any that forward to the core combinators", () => { From 81c6000c23e52a6ac9f326904cafe4b68a76ad34 Mon Sep 17 00:00:00 2001 From: wangbill Date: Fri, 24 Jul 2026 16:24:20 -0700 Subject: [PATCH 02/21] test(e2e-functions): add callHttp host e2e (sync/polling/no-poll) Add end-to-end callHttp coverage to the gated Functions-host suite (test/e2e-functions). The suite previously had zero callHttp coverage; this drives the restored context.df.callHttp API through a real func host + Azurite over three modes: - sync: callHttp -> 200 passthrough (HttpEcho). - polling: callHttp follows a 202 -> Location poll loop to a final 200 (HttpAsyncEcho, stateless attempt-encoded 202 so it is deterministic across worker processes). - nopoll: enablePolling:false returns the first 202 without looping. The test-app functions target the app's own loopback endpoints so the suite stays hermetic (no external network). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e1120c1a-9b20-4aef-b34a-f7986dd06382 --- test/e2e-functions/call-http.spec.ts | 67 ++++++++++++++ .../src/functions/CallHttpOrchestration.ts | 91 +++++++++++++++++++ 2 files changed, 158 insertions(+) create mode 100644 test/e2e-functions/call-http.spec.ts create mode 100644 test/e2e-functions/test-app/src/functions/CallHttpOrchestration.ts diff --git a/test/e2e-functions/call-http.spec.ts b/test/e2e-functions/call-http.spec.ts new file mode 100644 index 00000000..d29df1d3 --- /dev/null +++ b/test/e2e-functions/call-http.spec.ts @@ -0,0 +1,67 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +/** + * End-to-end coverage for the restored v3 `context.df.callHttp` API. + * + * Drives the `CallHttpOrchestration` app function (see test-app) through the real + * Functions host: a durable callHttp against the app's own endpoints exercises the + * synchronous 200 path, the 202 -> Location poll loop, and the enablePolling=false + * opt-out. Kept hermetic (loopback only) so no external network is required. + * + * Gated: skips cleanly unless the shared host was started by globalSetup. + */ + +import { + invokeHttpTrigger, + parseStatusQueryGetUri, + readPreflight, + waitForOrchestrationState, +} from "./harness"; + +const preflight = readPreflight(); +const describeMaybe = preflight.ok ? describe : describe.skip; +const baseUrl = preflight.baseUrl ?? ""; + +if (!preflight.ok) { + console.warn(`[functions-e2e] call-http.spec skipped: ${preflight.reason}`); +} + +describeMaybe("Functions host E2E — callHttp (AzureStorage)", () => { + it("callHttp returns a synchronous 200 response", async () => { + const response = await invokeHttpTrigger(baseUrl, "CallHttp_HttpStart", "?mode=sync"); + expect(response.status).toBe(202); // HttpStatusCode.Accepted (check-status payload) + + const statusQueryGetUri = parseStatusQueryGetUri(response); + const details = await waitForOrchestrationState(statusQueryGetUri, "Completed", 60); + + const output = details.output as { statusCode: number; content: string }; + expect(output.statusCode).toBe(200); + expect(JSON.parse(output.content).echoed).toBe("hello"); + }, 120_000); + + it("callHttp follows the 202 -> Location poll loop to the final 200", async () => { + const response = await invokeHttpTrigger(baseUrl, "CallHttp_HttpStart", "?mode=polling"); + expect(response.status).toBe(202); + + const statusQueryGetUri = parseStatusQueryGetUri(response); + const details = await waitForOrchestrationState(statusQueryGetUri, "Completed", 60); + + const output = details.output as { statusCode: number; content: string }; + expect(output.statusCode).toBe(200); + expect(JSON.parse(output.content).echoed).toBe("async-done"); + }, 120_000); + + it("callHttp with enablePolling:false returns the 202 without polling", async () => { + const response = await invokeHttpTrigger(baseUrl, "CallHttp_HttpStart", "?mode=nopoll"); + expect(response.status).toBe(202); + + const statusQueryGetUri = parseStatusQueryGetUri(response); + const details = await waitForOrchestrationState(statusQueryGetUri, "Completed", 60); + + // enablePolling=false returns the first 202 as-is; a 202 carries no body, so + // only the status code is asserted (do not JSON.parse the empty content). + const output = details.output as { statusCode: number; content: string }; + expect(output.statusCode).toBe(202); + }, 120_000); +}); diff --git a/test/e2e-functions/test-app/src/functions/CallHttpOrchestration.ts b/test/e2e-functions/test-app/src/functions/CallHttpOrchestration.ts new file mode 100644 index 00000000..b820c851 --- /dev/null +++ b/test/e2e-functions/test-app/src/functions/CallHttpOrchestration.ts @@ -0,0 +1,91 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +// Exercises the restored v3 `context.df.callHttp` API end-to-end. The orchestration +// calls the app's OWN http endpoints (HttpEcho / HttpAsyncEcho) so the suite stays +// hermetic — no external network is required. HttpAsyncEcho drives a stateless +// 202 -> Location -> 200 poll loop keyed off an `attempt` query param. + +import { app, HttpHandler, HttpRequest, HttpResponse, HttpResponseInit, InvocationContext } from '@azure/functions'; +import * as df from 'durable-functions'; +import { CallHttpOptions, OrchestrationContext, OrchestrationHandler } from 'durable-functions'; + +// Orchestration: issue a single durable callHttp and return the final status + body. +const CallHttpOrchestration: OrchestrationHandler = function* (context: OrchestrationContext) { + const input = context.df.getInput<{ url: string; enablePolling?: boolean }>(); + const options: CallHttpOptions = { method: 'GET', url: input.url }; + if (input.enablePolling !== undefined) { + options.enablePolling = input.enablePolling; + } + const response = (yield context.df.callHttp(options)) as { statusCode: number; content?: string }; + return { statusCode: response.statusCode, content: response.content }; +}; +df.app.orchestration('CallHttpOrchestration', CallHttpOrchestration); + +// Downstream endpoint (sync path): echoes the `value` query param as JSON. +const HttpEcho: HttpHandler = async (request: HttpRequest, _context: InvocationContext): Promise => { + return { + status: 200, + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ echoed: request.query.get('value') ?? 'default', method: request.method }), + }; +}; +app.http('HttpEcho', { + route: 'HttpEcho', + methods: ['GET', 'POST'], + authLevel: 'anonymous', + handler: HttpEcho, +}); + +// Downstream endpoint (async 202 pattern): stateless poll loop keyed off `attempt`. +// The first request returns 202 with a Location pointing at attempt+1; the next +// returns the final 200. Encoding attempt in the URL keeps it deterministic across +// worker processes (no module-level state). +const HttpAsyncEcho: HttpHandler = async (request: HttpRequest, _context: InvocationContext): Promise => { + const attempt = parseInt(request.query.get('attempt') ?? '1', 10); + if (attempt < 2) { + const next = new URL(request.url); + next.searchParams.set('attempt', String(attempt + 1)); + return { + status: 202, + headers: { Location: next.toString(), 'Retry-After': '1' }, + }; + } + return { + status: 200, + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ echoed: 'async-done', attempt }), + }; +}; +app.http('HttpAsyncEcho', { + route: 'HttpAsyncEcho', + methods: ['GET'], + authLevel: 'anonymous', + handler: HttpAsyncEcho, +}); + +// HTTP starter: schedule CallHttpOrchestration pointed at one of the app's own +// endpoints. `mode` selects the sync (HttpEcho), polling, or no-poll variant. +const CallHttp_HttpStart: HttpHandler = async (request: HttpRequest, context: InvocationContext): Promise => { + const client = df.getClient(context); + const origin = new URL(request.url).origin; + const mode = request.query.get('mode') ?? 'sync'; + + let input: { url: string; enablePolling?: boolean }; + if (mode === 'sync') { + input = { url: `${origin}/api/HttpEcho?value=hello` }; + } else if (mode === 'nopoll') { + input = { url: `${origin}/api/HttpAsyncEcho`, enablePolling: false }; + } else { + input = { url: `${origin}/api/HttpAsyncEcho` }; + } + + const instanceId = await client.startNew('CallHttpOrchestration', { input }); + return client.createCheckStatusResponse(request, instanceId); +}; +app.http('CallHttp_HttpStart', { + route: 'CallHttp_HttpStart', + extraInputs: [df.input.durableClient()], + methods: ['GET', 'POST'], + handler: CallHttp_HttpStart, +}); From ad3ae8596913f357394ed4dbe0138faf6bde7005 Mon Sep 17 00:00:00 2001 From: wangbill Date: Mon, 27 Jul 2026 10:00:53 -0700 Subject: [PATCH 03/21] fix(azure-functions-durable): harden callHttp 202 poll (cross-origin creds, top-level guard, compat) Address code-review defects in the restored context.df.callHttp: - P0-1: strip caller credentials on a cross-origin 202 Location poll (Authorization/Cookie/tokenSource dropped cross-origin; x-functions-key always dropped), with a defensive per-iteration header copy so stripping one hop never corrupts a later same-origin hop. Mirrors Azure/azure-functions-durable-extension#3443. - P0-2: reject a top-level start of BuiltIn__HttpPollOrchestrator (it is only ever a callHttp sub-orchestration) to close an SSRF / managed-identity token-minting vector. - P1-3: make the AAD /.default scope idempotent (bare and already-scoped resources). - P1-4: a tokenSource now overwrites any caller Authorization case-insensitively. - P1-5: resolve a relative Location against the effective (post-redirect) URI. - P1-6: default poll interval 1s -> 30s (host default); round the HTTP-date form up. - P1-7: throw on a GET/HEAD request body instead of silently dropping it. - P1-8 (documented gap): DurableHttpResponse stays a plain object across the sub-orchestration JSON boundary, so v3's getHeader() is unavailable; read response.headers["name"]. Restoring it safely needs a wrapOrchestrator drive-loop rewrite that risks classic-orchestrator error propagation; deferred to a follow-up. Tests: rewrite the http-builtin unit spec (cross/same-origin, multi-hop, no-parent guard, scope idempotency, auth override, retry-after 30s/ceil, GET/HEAD body throw); extend the host e2e with same-origin-forwards and cross-origin-strips cases proving the policy end-to-end against a real func host. README updated. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d86f5f69-4e1a-4c1d-b017-5e290c85cc05 --- packages/azure-functions-durable/README.md | 21 ++ .../src/http/builtin.ts | 195 +++++++++++++--- .../test/unit/http-builtin.spec.ts | 221 ++++++++++++++++-- test/e2e-functions/call-http.spec.ts | 39 +++- .../src/functions/CallHttpOrchestration.ts | 69 +++++- 5 files changed, 492 insertions(+), 53 deletions(-) diff --git a/packages/azure-functions-durable/README.md b/packages/azure-functions-durable/README.md index cc648f15..00e86264 100644 --- a/packages/azure-functions-durable/README.md +++ b/packages/azure-functions-durable/README.md @@ -55,6 +55,27 @@ changed: managed-identity `tokenSource` requires the optional [`@azure/identity`](https://www.npmjs.com/package/@azure/identity) package (`npm install @azure/identity`); without it, a request that uses a `tokenSource` throws a clear error. + A behavior note and several deliberate hardening/compat differences from v3: + - **Cross-origin `202` poll credentials are stripped.** The `Location` returned with a `202` is + callee-controlled, so when it points to a **different origin** (scheme/host/port) the poll drops + `Authorization`, `Cookie`, and the `tokenSource` (no token is re-minted for the attacker), and the + `x-functions-key` header is **always** dropped (both same- and cross-origin). Same-origin polls + still forward headers and the `tokenSource`, so legitimate async patterns keep working. This + mirrors the .NET extension's policy + ([Azure/azure-functions-durable-extension#3443](https://github.com/Azure/azure-functions-durable-extension/pull/3443)). + - **The built-in poll orchestrator cannot be started directly.** It is registered under a reserved + name (`BuiltIn__HttpPollOrchestrator`) and refuses a top-level start (it is only ever a + sub-orchestration of `callHttp`), so a dynamic `orchestrators/{name}` starter cannot be abused to + drive arbitrary SSRF or Managed-Identity token minting. + - **The default poll interval is 30 s** (matching the classic host) when a `202` carries no usable + `Retry-After`, rather than polling once per second. + - **A body on a `GET`/`HEAD` request throws.** The Fetch standard forbids it; v3 silently sent it, so + rather than change the request semantics the call fails loudly — remove `body` or use + `POST`/`PUT`/`PATCH`. + - **`DurableHttpResponse` is a plain object, not a class.** The response crosses the poll + sub-orchestration's JSON boundary, so the v3 `response.getHeader(name)` method is not available; read + headers directly via `response.headers["name"]` (response header names are lower-cased by `fetch`). + Restoring the case-insensitive accessor is tracked as a follow-up. - **Some v3 top-level exports were removed** — `DummyOrchestrationContext` / `DummyEntityContext` (testing utilities) and the entity-lock types above. `TaskFailedError` is re-exported from the core SDK (aggregate failures surface as JS-native `AggregateError`); use the diff --git a/packages/azure-functions-durable/src/http/builtin.ts b/packages/azure-functions-durable/src/http/builtin.ts index 7fa50ef9..9a2f9fb2 100644 --- a/packages/azure-functions-durable/src/http/builtin.ts +++ b/packages/azure-functions-durable/src/http/builtin.ts @@ -21,6 +21,13 @@ * preserving the single-`yield` v3 ergonomics while keeping the 202 polling loop durable * (checkpointed across restarts). * + * Security: the `Location` returned with a `202` is callee-controlled data. When it points to a + * different origin the poll drops the caller's credentials (`Authorization`/`Cookie`/`tokenSource`), + * and the `x-functions-key` is always dropped, so a malicious or compromised endpoint cannot harvest + * credentials by redirecting the poll to a host it controls. This mirrors the .NET extension's policy + * (Azure/azure-functions-durable-extension#3443). The poll orchestrator also rejects being started as + * a top-level orchestration (it is only ever a sub-orchestration of `callHttp`). + * * Both functions are auto-registered under reserved names when this package is imported (see * `../app.ts`) so existing apps that call `callHttp` work with no changes. Ported from the * durabletask-python design (Andy Staples, durabletask-python#155). @@ -37,8 +44,25 @@ import { DurableHttpRequestPayload, DurableHttpResponse } from "./models"; export const BUILTIN_HTTP_ACTIVITY_NAME = "BuiltIn__HttpActivity"; export const BUILTIN_HTTP_POLL_ORCHESTRATOR_NAME = "BuiltIn__HttpPollOrchestrator"; -/** Fallback interval (seconds) between polls when a `202` response carries no usable `Retry-After`. */ -const DEFAULT_POLL_INTERVAL_SECONDS = 1; +/** + * Fallback interval (seconds) between polls when a `202` response carries no usable `Retry-After`. + * Matches the classic Durable Functions host default (`HttpOptions.DefaultAsyncRequestSleepTime`, + * 30000 ms) rather than hammering the status endpoint once per second. + */ +const DEFAULT_POLL_INTERVAL_SECONDS = 30; + +/** + * @internal + * Result of the built-in HTTP activity: the public {@link DurableHttpResponse} plus the effective + * (post-redirect) request URI. `fetch` follows redirects by default, so a relative `Location` must be + * resolved against the URI that actually produced the response (RFC 9110 §7.1.2 / §10.2.2), not the + * URI originally requested. `effectiveUri` is kept off the public {@link DurableHttpResponse} shape; + * the poll orchestrator strips it before returning to `callHttp`, so v3 consumers see exactly + * `{ statusCode, headers, content }`. + */ +interface BuiltinHttpActivityResult extends DurableHttpResponse { + effectiveUri?: string; +} /** Case-insensitively look up `name` in `headers`. */ function getHeader(headers: { [key: string]: string }, name: string): string | undefined { @@ -51,6 +75,35 @@ function getHeader(headers: { [key: string]: string }, name: string): string | u return undefined; } +/** Case-insensitively delete every variant of `name` from `headers` (mutates in place). */ +function deleteHeader(headers: { [key: string]: string }, name: string): void { + const lowered = name.toLowerCase(); + for (const key of Object.keys(headers)) { + if (key.toLowerCase() === lowered) { + delete headers[key]; + } + } +} + +/** + * Whether `a` and `b` share an origin (scheme + host + port, case-insensitive, with default ports + * normalized). An unparseable or non-absolute URI is treated as **cross-origin** (conservative), + * matching the .NET `IsSameOrigin` policy (Azure/azure-functions-durable-extension#3443). + * + * @remarks + * `URL.origin` already lower-cases the scheme/host and drops default ports (`:80` for http, `:443` + * for https), so a plain string comparison of the two origins is exactly the scheme+host+port check. + * + * @internal Exported for unit testing. + */ +export function isSameOrigin(a: string, b: string): boolean { + try { + return new URL(a).origin === new URL(b).origin; + } catch { + return false; + } +} + /** * Parse the `Retry-After` header into a delay in seconds. * @@ -58,7 +111,8 @@ function getHeader(headers: { [key: string]: string }, name: string): string | u * Supports both the delta-seconds and HTTP-date forms; falls back to * {@link DEFAULT_POLL_INTERVAL_SECONDS} when absent or unparseable. For the HTTP-date form the delay * is computed against `now` — which the caller supplies as the orchestration's replay-safe - * `currentUtcDateTime` — so the resulting timer fire time is deterministic across replays. + * `currentUtcDateTime` — so the resulting timer fire time is deterministic across replays, and is + * rounded **up** so the poll never fires before the server-specified instant. * * @internal Exported for unit testing. */ @@ -75,7 +129,20 @@ export function retryAfterSeconds(headers: { [key: string]: string }, now: Date) if (Number.isNaN(retryAtMs)) { return DEFAULT_POLL_INTERVAL_SECONDS; } - return Math.max(Math.floor((retryAtMs - now.getTime()) / 1000), 0); + return Math.max(Math.ceil((retryAtMs - now.getTime()) / 1000), 0); +} + +/** + * Build an AAD `.../.default` scope from a `resource` identifier. + * + * @remarks + * Idempotent: a resource already expressed as a scope (ending in `/.default`) is returned unchanged, + * so both the bare form (`https://management.core.windows.net/`) and the already-scoped form + * (`https://management.core.windows.net/.default`) work — matching the v3 host, which accepts either. + */ +function toDefaultScope(resource: string): string { + const trimmed = resource.replace(/\/+$/, ""); + return /\/\.default$/i.test(trimmed) ? trimmed : `${trimmed}/.default`; } /** @@ -100,8 +167,7 @@ async function acquireBearerToken(resource: string): Promise { ); } const credential = new identity.DefaultAzureCredential(); - const scope = resource.replace(/\/+$/, "") + "/.default"; - const result = await credential.getToken(scope); + const result = await credential.getToken(toDefaultScope(resource)); const token = result?.token; if (!token) { throw new Error(`Failed to acquire a bearer token for resource '${resource}'.`); @@ -118,8 +184,14 @@ async function acquireBearerToken(resource: string): Promise { * `fetch` only rejects on network errors, not on HTTP status — so the poll orchestrator can inspect * the status code and headers. Only http/https URIs are permitted (an SSRF guard that closes off * `file://`, `ftp://`, ... schemes from orchestration-supplied URLs). + * + * When a `tokenSource` is present, the acquired bearer token **overwrites** any caller-supplied + * `Authorization` header (matching v3, which applies the caller's headers first and then the token), + * removing every case variant so `fetch` cannot merge a lowercase `authorization` and the bearer into + * one malformed comma-joined header. A body on a `GET`/`HEAD` request is rejected: the Fetch standard + * forbids it, and silently dropping it (as a naive port would) would change the request. */ -export async function builtinHttpActivity(input: DurableHttpRequestPayload): Promise { +export async function builtinHttpActivity(input: DurableHttpRequestPayload): Promise { const request = input ?? ({} as DurableHttpRequestPayload); const method = String(request.method ?? "GET").toUpperCase(); const uri = request.uri; @@ -138,18 +210,27 @@ export async function builtinHttpActivity(input: DurableHttpRequestPayload): Pro throw new Error(`callHttp only supports http/https URLs; got ${JSON.stringify(uri)}.`); } + // The Fetch standard forbids a request body on GET/HEAD. v3 (the host extension) attached content + // regardless of method; rather than silently drop it and change the request, fail loudly. + if (request.content !== undefined && (method === "GET" || method === "HEAD")) { + throw new Error( + `callHttp: an HTTP ${method} request cannot carry a body; remove 'body' or use POST/PUT/PATCH.`, + ); + } + const headers: { [key: string]: string } = { ...(request.headers ?? {}) }; const resource = request.tokenSource?.resource; if (resource) { const token = await acquireBearerToken(resource); - if (headers["Authorization"] === undefined) { - headers["Authorization"] = `Bearer ${token}`; - } + // The token source overwrites any caller-supplied Authorization (v3 semantics). Strip every case + // variant first so a lowercase `authorization` is not merged with ours into one bad header. + deleteHeader(headers, "Authorization"); + headers["Authorization"] = `Bearer ${token}`; } - // `content` was already serialized to a string by `callHttp`, so it is sent as-is. GET/HEAD - // requests cannot carry a body under the fetch spec, so a body is only attached for other methods. - const includeBody = typeof request.content === "string" && method !== "GET" && method !== "HEAD"; + // `content` was already serialized to a string by `callHttp`, and GET/HEAD bodies were rejected + // above, so a body is attached only when present. + const includeBody = typeof request.content === "string"; const response = await fetch(uri, { method, headers, @@ -162,7 +243,53 @@ export async function builtinHttpActivity(input: DurableHttpRequestPayload): Pro }); const content = await response.text(); - return { statusCode: response.status, headers: responseHeaders, content }; + const result: BuiltinHttpActivityResult = { statusCode: response.status, headers: responseHeaders, content }; + // Record the post-redirect URL so a relative `Location` is resolved against the URI that actually + // produced this response (fetch follows redirects by default). + if (response.url) { + result.effectiveUri = response.url; + } + return result; +} + +/** + * Build the next poll request for a `202` `Location`, applying the cross-origin credential policy. + * + * @remarks + * `Location` is callee-controlled, so credentials are handled per + * Azure/azure-functions-durable-extension#3443: + * - `x-functions-key` is **always** dropped (a function-level key won't open the master-key-protected + * status endpoint, and it must never leak to another origin); + * - on a **cross-origin** `Location` the `Authorization`/`Cookie` headers and the `tokenSource` are + * dropped, so an attacker-controlled first hop cannot harvest credentials; + * - on a **same-origin** `Location` headers and the `tokenSource` are forwarded — the async polling + * pattern legitimately re-authenticates back to the same service. + * + * The header object is copied fresh from the **original** request on every iteration, so stripping on + * one hop never corrupts a later (same-origin) hop. + */ +function buildPollRequest( + request: DurableHttpRequestPayload, + base: string, + resolved: string, + enablePolling: boolean, +): DurableHttpRequestPayload { + const sameOrigin = isSameOrigin(base, resolved); + const pollRequest: DurableHttpRequestPayload = { method: "GET", uri: resolved, enablePolling }; + + if (request.headers !== undefined) { + const headers = { ...request.headers }; + deleteHeader(headers, "x-functions-key"); + if (!sameOrigin) { + deleteHeader(headers, "Authorization"); + deleteHeader(headers, "Cookie"); + } + pollRequest.headers = headers; + } + if (sameOrigin && request.tokenSource !== undefined) { + pollRequest.tokenSource = request.tokenSource; + } + return pollRequest; } /** @@ -173,20 +300,32 @@ export async function builtinHttpActivity(input: DurableHttpRequestPayload): Pro * orchestration input arrives as the second argument). It calls the built-in HTTP activity and, * while the response is `202 Accepted` with a `Location` header (and polling is enabled), waits on a * durable timer (honoring `Retry-After`) before re-polling the `Location` URL, resolving a relative - * `Location` against the current request URI. Returns the final response. All time math uses the + * `Location` against the effective request URI. Returns the final response. All time math uses the * replay-safe `currentUtcDateTime`, never `Date.now()`, so replays are deterministic. + * + * It rejects being started as a **top-level** orchestration: `callHttp` always schedules it as a + * sub-orchestration, so a legitimate invocation always has a parent. A top-level start (e.g. via a + * dynamic `orchestrators/{name}` starter) would let an attacker point the built-in at an arbitrary + * URI with a token source — SSRF plus Managed-Identity token minting — so it is refused. */ export async function* builtinHttpPollOrchestrator( ctx: OrchestrationContext, input: DurableHttpRequestPayload, ): AsyncGenerator, DurableHttpResponse, unknown> { + if (!ctx.parent) { + throw new Error( + `${BUILTIN_HTTP_POLL_ORCHESTRATOR_NAME} is an internal built-in and cannot be started as a ` + + `top-level orchestration; use context.df.callHttp instead.`, + ); + } + const request = input ?? ({} as DurableHttpRequestPayload); // v3 opt-out: when polling is disabled the first response is returned as-is (no 202 loop). const enablePolling = request.enablePolling !== false; - let response = (yield ctx.callActivity(BUILTIN_HTTP_ACTIVITY_NAME, request)) as DurableHttpResponse; - // Track the URI of the most recent request so a relative `Location` can be resolved against it. - let currentUri = String(request.uri ?? ""); + let response = (yield ctx.callActivity(BUILTIN_HTTP_ACTIVITY_NAME, request)) as BuiltinHttpActivityResult; + // Base URI for resolving a relative `Location`: the effective (post-redirect) URI when known. + let currentUri = response.effectiveUri ?? String(request.uri ?? ""); while (enablePolling && response.statusCode === 202) { const headers = response.headers ?? {}; @@ -196,7 +335,7 @@ export async function* builtinHttpPollOrchestrator( break; } - // A `Location` may be relative (e.g. `/operations/42`); resolve it against the current request + // A `Location` may be relative (e.g. `/operations/42`); resolve it against the effective request // URI so the next poll targets an absolute http(s) URL (the activity rejects non-absolute URIs). const resolved = new URL(location, currentUri).toString(); @@ -205,18 +344,12 @@ export async function* builtinHttpPollOrchestrator( const fireAt = new Date(now.getTime() + delaySeconds * 1000); yield ctx.createTimer(fireAt); - const pollRequest: DurableHttpRequestPayload = { method: "GET", uri: resolved, enablePolling }; - // Preserve auth for the polling requests. - if (request.headers !== undefined) { - pollRequest.headers = request.headers; - } - if (request.tokenSource !== undefined) { - pollRequest.tokenSource = request.tokenSource; - } - - currentUri = resolved; - response = (yield ctx.callActivity(BUILTIN_HTTP_ACTIVITY_NAME, pollRequest)) as DurableHttpResponse; + const pollRequest = buildPollRequest(request, currentUri, resolved, enablePolling); + response = (yield ctx.callActivity(BUILTIN_HTTP_ACTIVITY_NAME, pollRequest)) as BuiltinHttpActivityResult; + currentUri = response.effectiveUri ?? resolved; } - return response; + // Strip the internal `effectiveUri` so `callHttp` resolves to exactly the v3 + // `{ statusCode, headers, content }` shape. + return { statusCode: response.statusCode, headers: response.headers, content: response.content }; } diff --git a/packages/azure-functions-durable/test/unit/http-builtin.spec.ts b/packages/azure-functions-durable/test/unit/http-builtin.spec.ts index 06311dfa..1ff09e7d 100644 --- a/packages/azure-functions-durable/test/unit/http-builtin.spec.ts +++ b/packages/azure-functions-durable/test/unit/http-builtin.spec.ts @@ -6,6 +6,7 @@ import { BUILTIN_HTTP_ACTIVITY_NAME, builtinHttpActivity, builtinHttpPollOrchestrator, + isSameOrigin, retryAfterSeconds, } from "../../src/http/builtin"; import { DurableHttpRequestPayload, DurableHttpResponse } from "../../src/http/models"; @@ -23,9 +24,10 @@ jest.mock( ); /** A minimal fetch Response stand-in (avoids depending on the global `Response` constructor). */ -function fakeResponse(status: number, headers: { [key: string]: string }, body: string) { +function fakeResponse(status: number, headers: { [key: string]: string }, body: string, url = "") { return { status, + url, headers: { forEach: (cb: (value: string, key: string) => void) => Object.entries(headers).forEach(([key, value]) => cb(value, key)), @@ -57,15 +59,52 @@ describe("retryAfterSeconds", () => { expect(retryAfterSeconds({ "retry-after": "7" }, now)).toBe(7); }); - it("falls back to 1s when the header is missing or unparseable", () => { - expect(retryAfterSeconds({}, now)).toBe(1); - expect(retryAfterSeconds({ "Retry-After": "not-a-date" }, now)).toBe(1); + it("falls back to the 30s host default when the header is missing or unparseable", () => { + // v3's host default is 30s (HttpOptions.DefaultAsyncRequestSleepTime); polling once per second + // would be up to 30x more activity + timer executions. + expect(retryAfterSeconds({}, now)).toBe(30); + expect(retryAfterSeconds({ "Retry-After": "not-a-date" }, now)).toBe(30); + }); + + it("rounds an HTTP-date delay UP so the poll never fires early", () => { + // now = 08.800, retry-at = 10.000 -> 1.2s remaining. Math.ceil => 2; Math.floor/round would + // give 1 and poll up to ~1.2s early. + expect( + retryAfterSeconds( + { "Retry-After": "Thu, 01 Jan 2026 00:00:10 GMT" }, + new Date("2026-01-01T00:00:08.800Z"), + ), + ).toBe(2); }); it("never returns a negative delay for a past HTTP-date", () => { - expect(retryAfterSeconds({ "Retry-After": "Thu, 01 Jan 2026 00:00:00 GMT" }, new Date("2026-01-01T00:01:00.000Z"))).toBe( - 0, - ); + expect( + retryAfterSeconds({ "Retry-After": "Thu, 01 Jan 2026 00:00:00 GMT" }, new Date("2026-01-01T00:01:00.000Z")), + ).toBe(0); + }); +}); + +describe("isSameOrigin", () => { + it("treats identical scheme/host/port (differing path) as same-origin", () => { + expect(isSameOrigin("https://svc.test/api/start", "https://svc.test/api/status/1")).toBe(true); + }); + + it("normalizes default ports and is case-insensitive on scheme/host", () => { + expect(isSameOrigin("https://svc.test:443/a", "https://SVC.TEST/b")).toBe(true); + expect(isSameOrigin("http://svc.test:80/a", "http://svc.test/b")).toBe(true); + }); + + it("treats a differing scheme, host, or explicit port as cross-origin", () => { + expect(isSameOrigin("http://svc.test/a", "https://svc.test/a")).toBe(false); + expect(isSameOrigin("https://svc.test/a", "https://attacker.test/a")).toBe(false); + expect(isSameOrigin("http://svc.test:8080/a", "http://svc.test/a")).toBe(false); + // 127.0.0.1 and localhost are different origin strings even though both are loopback. + expect(isSameOrigin("http://127.0.0.1:7071/a", "http://localhost:7071/a")).toBe(false); + }); + + it("treats a non-absolute or unparseable URI as cross-origin (conservative)", () => { + expect(isSameOrigin("/relative/path", "https://svc.test/a")).toBe(false); + expect(isSameOrigin("https://svc.test/a", "not a url")).toBe(false); }); }); @@ -88,6 +127,7 @@ describe("builtinHttpActivity", () => { expect(calledUri).toBe("https://example.test/data"); expect(calledInit.method).toBe("GET"); expect(calledInit.body).toBeUndefined(); + // No redirect (response.url === "") -> no internal effectiveUri leaks into the v3 shape. expect(response).toEqual({ statusCode: 200, headers: { "content-type": "text/plain" }, @@ -95,15 +135,22 @@ describe("builtinHttpActivity", () => { }); }); - it("sends a body for non-GET/HEAD methods but omits it for GET", async () => { + it("sends a body for non-GET/HEAD methods", async () => { const fetchMock = makeFetchMock(fakeResponse(200, {}, "")); global.fetch = fetchMock as unknown as typeof fetch; await builtinHttpActivity({ method: "POST", uri: "https://example.test/", content: '{"a":1}' }); expect((fetchMock.mock.calls[0][1] as RequestInit).body).toBe('{"a":1}'); + }); - await builtinHttpActivity({ method: "GET", uri: "https://example.test/", content: '{"a":1}' }); - expect((fetchMock.mock.calls[1][1] as RequestInit).body).toBeUndefined(); + it("throws when a GET or HEAD request carries a body (fetch forbids it; v3 silently sent it)", async () => { + global.fetch = jest.fn() as unknown as typeof fetch; + await expect( + builtinHttpActivity({ method: "GET", uri: "https://example.test/", content: "x" }), + ).rejects.toThrow(/cannot carry a body/i); + await expect( + builtinHttpActivity({ method: "HEAD", uri: "https://example.test/", content: "x" }), + ).rejects.toThrow(/cannot carry a body/i); }); it("throws when the uri is missing", async () => { @@ -113,9 +160,7 @@ describe("builtinHttpActivity", () => { it("rejects non-http/https schemes (SSRF guard)", async () => { global.fetch = jest.fn() as unknown as typeof fetch; - await expect( - builtinHttpActivity({ method: "GET", uri: "file:///etc/passwd" }), - ).rejects.toThrow(/http\/https/); + await expect(builtinHttpActivity({ method: "GET", uri: "file:///etc/passwd" })).rejects.toThrow(/http\/https/); await expect(builtinHttpActivity({ method: "GET", uri: "ftp://host/f" })).rejects.toThrow(/http\/https/); }); @@ -129,6 +174,15 @@ describe("builtinHttpActivity", () => { expect(response.headers.location).toBe("https://example.test/op/1"); }); + it("returns the post-redirect effective URI when fetch followed a redirect", async () => { + const fetchMock = makeFetchMock(fakeResponse(200, {}, "ok", "https://example.test/v2/final")); + global.fetch = fetchMock as unknown as typeof fetch; + + const response = await builtinHttpActivity({ method: "GET", uri: "https://example.test/v1/start" }); + + expect((response as { effectiveUri?: string }).effectiveUri).toBe("https://example.test/v2/final"); + }); + it("acquires a real bearer token via @azure/identity when a tokenSource is present", async () => { const fetchMock = makeFetchMock(fakeResponse(200, {}, "ok")); global.fetch = fetchMock as unknown as typeof fetch; @@ -146,25 +200,52 @@ describe("builtinHttpActivity", () => { expect(sentHeaders["Authorization"]).toBe("Bearer REAL_TOKEN_123"); }); - it("does not overwrite an explicit Authorization header", async () => { + it("builds the .default scope idempotently for bare and already-scoped resources", async () => { + const fetchMock = makeFetchMock(fakeResponse(200, {}, "ok")); + global.fetch = fetchMock as unknown as typeof fetch; + + await builtinHttpActivity({ + method: "GET", + uri: "https://x.test/", + tokenSource: { resource: "https://graph.microsoft.com/" }, + }); + expect(mockGetToken).toHaveBeenLastCalledWith("https://graph.microsoft.com/.default"); + + // Already-scoped resource must NOT become `.../.default/.default`. + await builtinHttpActivity({ + method: "GET", + uri: "https://x.test/", + tokenSource: { resource: "https://graph.microsoft.com/.default" }, + }); + expect(mockGetToken).toHaveBeenLastCalledWith("https://graph.microsoft.com/.default"); + }); + + it("overwrites any caller-supplied Authorization with the token (case-insensitive)", async () => { const fetchMock = makeFetchMock(fakeResponse(200, {}, "ok")); global.fetch = fetchMock as unknown as typeof fetch; await builtinHttpActivity({ method: "GET", uri: "https://example.test/secure", - headers: { Authorization: "Bearer caller-supplied" }, + headers: { authorization: "Bearer caller-supplied" }, // lowercase variant tokenSource: { resource: "https://graph.microsoft.com/" }, }); const sentHeaders = (fetchMock.mock.calls[0][1] as RequestInit).headers as { [key: string]: string }; - expect(sentHeaders["Authorization"]).toBe("Bearer caller-supplied"); + // Token wins over the caller value (v3 semantics), and the lowercase variant is removed so fetch + // cannot merge two Authorization headers into one malformed comma-joined value. + expect(sentHeaders["Authorization"]).toBe("Bearer REAL_TOKEN_123"); + expect(sentHeaders["authorization"]).toBeUndefined(); }); }); /** Drives the poll orchestrator generator with a fake core context, feeding activity/timer results. */ -function createPollContext(now: Date) { +function createPollContext( + now: Date, + parent: unknown = { name: "root", instanceId: "root-id", taskScheduledId: 0 }, +) { const ctx = { + parent, currentUtcDateTime: now, callActivity: jest.fn((name: string, input: unknown) => ({ kind: "activity", name, input }) as unknown as Task), createTimer: jest.fn((fireAt: Date | number) => ({ kind: "timer", fireAt }) as unknown as Task), @@ -175,6 +256,15 @@ function createPollContext(now: Date) { describe("builtinHttpPollOrchestrator", () => { const now = new Date("2026-01-01T00:00:00.000Z"); + it("throws when started as a top-level orchestration (no parent)", async () => { + // Refuses direct top-level invocation: callHttp always schedules it as a sub-orchestration, so a + // parentless start is an attacker pointing the built-in at an arbitrary URI + token source. `null` + // stands in for the core's top-level `parent: undefined` (both are falsy). + const { ctx } = createPollContext(now, null); + const gen = builtinHttpPollOrchestrator(ctx, { method: "GET", uri: "https://host/api" }); + await expect(gen.next()).rejects.toThrow(/top-level/i); + }); + it("returns the first response immediately when it is not a 202 (no timer)", async () => { const { ctx, raw } = createPollContext(now); const gen = builtinHttpPollOrchestrator(ctx, { method: "GET", uri: "https://host/api", enablePolling: true }); @@ -217,7 +307,7 @@ describe("builtinHttpPollOrchestrator", () => { expect(raw.createTimer).toHaveBeenCalledTimes(1); expect(raw.createTimer).toHaveBeenCalledWith(new Date("2026-01-01T00:00:05.000Z")); - // 3) after the timer, re-poll the Location with GET, carrying headers + tokenSource + // 3) after the timer, re-poll the Location with GET, carrying same-origin headers + tokenSource const afterTimer = await gen.next(); expect(afterTimer.done).toBe(false); expect(raw.callActivity).toHaveBeenNthCalledWith(2, BUILTIN_HTTP_ACTIVITY_NAME, { @@ -261,6 +351,101 @@ describe("builtinHttpPollOrchestrator", () => { ); }); + it("resolves a relative Location against the effective (post-redirect) URI, not the requested URI", async () => { + const { ctx, raw } = createPollContext(now); + const gen = builtinHttpPollOrchestrator(ctx, { method: "GET", uri: "https://host/v1/start", enablePolling: true }); + + await gen.next(); + // The activity followed a redirect to /v2/start; the relative Location must resolve against it. + await gen.next({ + statusCode: 202, + headers: { Location: "status/1", "Retry-After": "1" }, + effectiveUri: "https://host/v2/start", + }); + await gen.next(); + + const pollReq = raw.callActivity.mock.calls[1][1] as DurableHttpRequestPayload; + expect(pollReq.uri).toBe("https://host/v2/status/1"); + }); + + it("drops Authorization/Cookie/tokenSource and x-functions-key when the Location is cross-origin", async () => { + const { ctx, raw } = createPollContext(now); + const request: DurableHttpRequestPayload = { + method: "GET", + uri: "https://original.test/api/start", + enablePolling: true, + headers: { Authorization: "Bearer caller", Cookie: "sid=1", "x-functions-key": "fkey", "x-keep": "yes" }, + tokenSource: { resource: "https://management.core.windows.net/" }, + }; + const gen = builtinHttpPollOrchestrator(ctx, request); + + await gen.next(); + await gen.next({ statusCode: 202, headers: { Location: "https://attacker.test/harvest", "Retry-After": "1" } }); + await gen.next(); // past timer -> second poll to the cross-origin Location + + const pollReq = raw.callActivity.mock.calls[1][1] as DurableHttpRequestPayload; + expect(pollReq.uri).toBe("https://attacker.test/harvest"); + // Only the neutral header survives; credentials are stripped. + expect(pollReq.headers).toEqual({ "x-keep": "yes" }); + expect(pollReq.tokenSource).toBeUndefined(); + // The ORIGINAL request object must be untouched (defensive per-iteration copy). + expect(request.headers).toEqual({ + Authorization: "Bearer caller", + Cookie: "sid=1", + "x-functions-key": "fkey", + "x-keep": "yes", + }); + }); + + it("forwards headers + tokenSource on a same-origin Location but always drops x-functions-key", async () => { + const { ctx, raw } = createPollContext(now); + const request: DurableHttpRequestPayload = { + method: "GET", + uri: "https://svc.test/api/start", + enablePolling: true, + headers: { Authorization: "Bearer caller", "x-functions-key": "fkey", "x-keep": "yes" }, + tokenSource: { resource: "https://management.core.windows.net/" }, + }; + const gen = builtinHttpPollOrchestrator(ctx, request); + + await gen.next(); + await gen.next({ statusCode: 202, headers: { Location: "https://svc.test/api/status/1", "Retry-After": "1" } }); + await gen.next(); + + const pollReq = raw.callActivity.mock.calls[1][1] as DurableHttpRequestPayload; + // x-functions-key is dropped even same-origin; Authorization + tokenSource are forwarded. + expect(pollReq.headers).toEqual({ Authorization: "Bearer caller", "x-keep": "yes" }); + expect(pollReq.tokenSource).toEqual({ resource: "https://management.core.windows.net/" }); + }); + + it("re-forwards same-origin credentials across multiple hops without corrupting later iterations", async () => { + const { ctx, raw } = createPollContext(now); + const request: DurableHttpRequestPayload = { + method: "GET", + uri: "https://svc.test/api/start", + enablePolling: true, + headers: { Authorization: "Bearer caller", "x-functions-key": "fkey" }, + tokenSource: { resource: "https://management.core.windows.net/" }, + }; + const gen = builtinHttpPollOrchestrator(ctx, request); + + await gen.next(); + await gen.next({ statusCode: 202, headers: { Location: "https://svc.test/api/status/1", "Retry-After": "1" } }); + await gen.next(); // second poll (call index 1) + await gen.next({ statusCode: 202, headers: { Location: "https://svc.test/api/status/2", "Retry-After": "1" } }); + await gen.next(); // third poll (call index 2) + + const poll1 = raw.callActivity.mock.calls[1][1] as DurableHttpRequestPayload; + const poll2 = raw.callActivity.mock.calls[2][1] as DurableHttpRequestPayload; + // Iteration 2 must still carry the (same-origin) credentials — proof the header stripping on each + // hop copies from the original request and never mutates it in place. + expect(poll1.headers).toEqual({ Authorization: "Bearer caller" }); + expect(poll1.tokenSource).toEqual({ resource: "https://management.core.windows.net/" }); + expect(poll2.headers).toEqual({ Authorization: "Bearer caller" }); + expect(poll2.tokenSource).toEqual({ resource: "https://management.core.windows.net/" }); + expect(request.headers).toEqual({ Authorization: "Bearer caller", "x-functions-key": "fkey" }); + }); + it("returns the first 202 without looping when polling is disabled", async () => { const { ctx, raw } = createPollContext(now); const gen = builtinHttpPollOrchestrator(ctx, { method: "GET", uri: "https://host/api", enablePolling: false }); diff --git a/test/e2e-functions/call-http.spec.ts b/test/e2e-functions/call-http.spec.ts index d29df1d3..52f6ff24 100644 --- a/test/e2e-functions/call-http.spec.ts +++ b/test/e2e-functions/call-http.spec.ts @@ -6,8 +6,10 @@ * * Drives the `CallHttpOrchestration` app function (see test-app) through the real * Functions host: a durable callHttp against the app's own endpoints exercises the - * synchronous 200 path, the 202 -> Location poll loop, and the enablePolling=false - * opt-out. Kept hermetic (loopback only) so no external network is required. + * synchronous 200 path, the 202 -> Location poll loop, the enablePolling=false + * opt-out, and the cross-origin credential policy (Authorization forwarded on a + * same-origin poll, stripped on a cross-origin one). Kept hermetic (loopback only) + * so no external network is required. * * Gated: skips cleanly unless the shared host was started by globalSetup. */ @@ -64,4 +66,37 @@ describeMaybe("Functions host E2E — callHttp (AzureStorage)", () => { const output = details.output as { statusCode: number; content: string }; expect(output.statusCode).toBe(202); }, 120_000); + + it("callHttp forwards the Authorization header on a same-origin 202 poll", async () => { + // Baseline for the cross-origin case: first hop and poll Location share the `localhost` + // origin, so the caller's Authorization header must reach the poll target. This rules out a + // false positive where the host simply drops Authorization on the wire. + const response = await invokeHttpTrigger(baseUrl, "CallHttp_HttpStart", "?mode=xorigin-same"); + expect(response.status).toBe(202); + + const statusQueryGetUri = parseStatusQueryGetUri(response); + const details = await waitForOrchestrationState(statusQueryGetUri, "Completed", 60); + + const output = details.output as { statusCode: number; content: string }; + expect(output.statusCode).toBe(200); + const body = JSON.parse(output.content); + expect(body.echoed).toBe("xorigin-done"); + expect(body.authorization).toBe("Bearer e2e-secret"); + }, 120_000); + + it("callHttp strips the Authorization header on a cross-origin 202 poll", async () => { + // First hop via the 127.0.0.1 origin, poll Location on the localhost origin -> cross-origin, + // so the Authorization header carried on the first hop must NOT reach the poll target. + const response = await invokeHttpTrigger(baseUrl, "CallHttp_HttpStart", "?mode=xorigin"); + expect(response.status).toBe(202); + + const statusQueryGetUri = parseStatusQueryGetUri(response); + const details = await waitForOrchestrationState(statusQueryGetUri, "Completed", 60); + + const output = details.output as { statusCode: number; content: string }; + expect(output.statusCode).toBe(200); + const body = JSON.parse(output.content); + expect(body.echoed).toBe("xorigin-done"); + expect(body.authorization).toBeNull(); + }, 120_000); }); diff --git a/test/e2e-functions/test-app/src/functions/CallHttpOrchestration.ts b/test/e2e-functions/test-app/src/functions/CallHttpOrchestration.ts index b820c851..3f8b1b5a 100644 --- a/test/e2e-functions/test-app/src/functions/CallHttpOrchestration.ts +++ b/test/e2e-functions/test-app/src/functions/CallHttpOrchestration.ts @@ -5,6 +5,11 @@ // calls the app's OWN http endpoints (HttpEcho / HttpAsyncEcho) so the suite stays // hermetic — no external network is required. HttpAsyncEcho drives a stateless // 202 -> Location -> 200 poll loop keyed off an `attempt` query param. +// +// HttpCrossOriginStart / HttpAuthEcho additionally exercise the cross-origin credential +// policy: the poll Location always targets the `localhost` origin, so a first hop made via +// the `127.0.0.1` origin is cross-origin (Authorization stripped) while a first hop via +// `localhost` is same-origin (Authorization forwarded). Both resolve to loopback. import { app, HttpHandler, HttpRequest, HttpResponse, HttpResponseInit, InvocationContext } from '@azure/functions'; import * as df from 'durable-functions'; @@ -12,8 +17,15 @@ import { CallHttpOptions, OrchestrationContext, OrchestrationHandler } from 'dur // Orchestration: issue a single durable callHttp and return the final status + body. const CallHttpOrchestration: OrchestrationHandler = function* (context: OrchestrationContext) { - const input = context.df.getInput<{ url: string; enablePolling?: boolean }>(); + const input = context.df.getInput<{ + url: string; + enablePolling?: boolean; + headers?: { [key: string]: string }; + }>(); const options: CallHttpOptions = { method: 'GET', url: input.url }; + if (input.headers !== undefined) { + options.headers = input.headers; + } if (input.enablePolling !== undefined) { options.enablePolling = input.enablePolling; } @@ -64,6 +76,48 @@ app.http('HttpAsyncEcho', { handler: HttpAsyncEcho, }); +// Downstream endpoint (cross-origin poll start): returns 202 with a Location that ALWAYS +// targets the `localhost` origin. When the first hop arrived via the `127.0.0.1` origin the +// poll Location is therefore a *different* origin (credentials must be stripped); when it +// arrived via `localhost` the Location is the *same* origin (credentials forwarded). Both host +// strings resolve to loopback, so the suite stays hermetic. +const HttpCrossOriginStart: HttpHandler = async ( + request: HttpRequest, + _context: InvocationContext, +): Promise => { + const u = new URL(request.url); + const location = `${u.protocol}//localhost:${u.port}/api/HttpAuthEcho`; + return { + status: 202, + headers: { Location: location, 'Retry-After': '1' }, + }; +}; +app.http('HttpCrossOriginStart', { + route: 'HttpCrossOriginStart', + methods: ['GET'], + authLevel: 'anonymous', + handler: HttpCrossOriginStart, +}); + +// Downstream endpoint (poll target): echoes back the Authorization header it received so the +// test can assert whether the poll forwarded (same-origin) or stripped (cross-origin) it. +const HttpAuthEcho: HttpHandler = async ( + request: HttpRequest, + _context: InvocationContext, +): Promise => { + return { + status: 200, + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ echoed: 'xorigin-done', authorization: request.headers.get('authorization') }), + }; +}; +app.http('HttpAuthEcho', { + route: 'HttpAuthEcho', + methods: ['GET'], + authLevel: 'anonymous', + handler: HttpAuthEcho, +}); + // HTTP starter: schedule CallHttpOrchestration pointed at one of the app's own // endpoints. `mode` selects the sync (HttpEcho), polling, or no-poll variant. const CallHttp_HttpStart: HttpHandler = async (request: HttpRequest, context: InvocationContext): Promise => { @@ -71,11 +125,22 @@ const CallHttp_HttpStart: HttpHandler = async (request: HttpRequest, context: In const origin = new URL(request.url).origin; const mode = request.query.get('mode') ?? 'sync'; - let input: { url: string; enablePolling?: boolean }; + let input: { url: string; enablePolling?: boolean; headers?: { [key: string]: string } }; if (mode === 'sync') { input = { url: `${origin}/api/HttpEcho?value=hello` }; } else if (mode === 'nopoll') { input = { url: `${origin}/api/HttpAsyncEcho`, enablePolling: false }; + } else if (mode === 'xorigin' || mode === 'xorigin-same') { + // Carry an Authorization header the poll must handle per the cross-origin policy. The first + // hop uses the `127.0.0.1` origin for `xorigin` (so the localhost Location is cross-origin and + // the credential is stripped) and the `localhost` origin for `xorigin-same` (so the Location is + // same-origin and the credential is forwarded). Both share the host's port. + const u = new URL(request.url); + const firstHost = mode === 'xorigin' ? '127.0.0.1' : 'localhost'; + input = { + url: `${u.protocol}//${firstHost}:${u.port}/api/HttpCrossOriginStart`, + headers: { Authorization: 'Bearer e2e-secret' }, + }; } else { input = { url: `${origin}/api/HttpAsyncEcho` }; } From 5015da6503371ef0a7f0fb0313055931d6dada52 Mon Sep 17 00:00:00 2001 From: wangbill Date: Mon, 27 Jul 2026 10:17:15 -0700 Subject: [PATCH 04/21] test(azure-functions-durable): add direct-invocation e2e for callHttp guard; document deferred redirect/getHeader gaps Follow-up to ad3ae85 (append-only); no runtime behavior change. - e2e (P0-2 proof): start BuiltIn__HttpPollOrchestrator directly by name via the generic StartOrchestration starter and assert it reaches Failed carrying the top-level-guard message. This proves ctx.parent IS populated on the Functions host path (real func + Azurite: call-http 6/6 passed). - P0-1 (redirect): document why builtinHttpActivity keeps the default redirect:"follow" -- undici drops Authorization/Cookie on cross-origin 3xx per the Fetch Standard but NOT custom headers such as x-functions-key; a redirect:"manual" loop is deferred as a single-request behavior change the hermetic loopback e2e cannot cover. The higher-severity 202 Location poll loop (which re-mints Managed-Identity tokens) is fully guarded regardless. - P1-8 (getHeader): document why v3 DurableHttpResponse.getHeader() is not restored across the sub-orchestration JSON boundary (core Task is a plain data holder read directly by the executor). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d86f5f69-4e1a-4c1d-b017-5e290c85cc05 --- .../src/http/builtin.ts | 18 +++++++++++++++ test/e2e-functions/call-http.spec.ts | 23 ++++++++++++++++++- 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/packages/azure-functions-durable/src/http/builtin.ts b/packages/azure-functions-durable/src/http/builtin.ts index 9a2f9fb2..6886ae9f 100644 --- a/packages/azure-functions-durable/src/http/builtin.ts +++ b/packages/azure-functions-durable/src/http/builtin.ts @@ -231,6 +231,14 @@ export async function builtinHttpActivity(input: DurableHttpRequestPayload): Pro // `content` was already serialized to a string by `callHttp`, and GET/HEAD bodies were rejected // above, so a body is attached only when present. const includeBody = typeof request.content === "string"; + // `redirect: "follow"` (the default): `fetch`/undici transparently follows 3xx redirects and, per the + // Fetch Standard, drops `Authorization` (and `Cookie`) when a redirect crosses origins — but it does + // NOT drop custom credential headers such as `x-functions-key`. Switching to `redirect: "manual"` and + // re-applying our cross-origin policy per hop would close that residual gap, but it would change + // observable single-request semantics (hop count, effective URL, cookie handling) in ways the + // hermetic loopback e2e cannot faithfully cover, so it is intentionally deferred. The higher-severity + // path — the 202 `Location` poll loop, which re-mints Managed-Identity tokens — is fully guarded in + // `buildPollRequest` regardless of this choice. const response = await fetch(uri, { method, headers, @@ -351,5 +359,15 @@ export async function* builtinHttpPollOrchestrator( // Strip the internal `effectiveUri` so `callHttp` resolves to exactly the v3 // `{ statusCode, headers, content }` shape. + // + // Known v3 gap: v3's `DurableHttpResponse` was a class exposing a case-insensitive `getHeader()`. + // The response here crosses this sub-orchestration's JSON boundary back to `callHttp`, so it can only + // be a plain object; and core `Task` is a plain data holder whose `_result` the executor reads + // directly (bypassing any accessor), so neither subclassing `Task` nor reviving a class instance in + // the caller survives serialization. Restoring `getHeader()` would require replacing + // `wrapOrchestrator`'s `yield*` delegation with a manual drive loop that still preserves `.throw()` + // and `.return()` for every classic orchestrator — too broad a blast radius to justify here. + // Migrating consumers read headers by lower-cased key (`response.headers["location"]`), since `fetch` + // lower-cases response header names. Tracked for a follow-up issue. return { statusCode: response.statusCode, headers: response.headers, content: response.content }; } diff --git a/test/e2e-functions/call-http.spec.ts b/test/e2e-functions/call-http.spec.ts index 52f6ff24..5488506e 100644 --- a/test/e2e-functions/call-http.spec.ts +++ b/test/e2e-functions/call-http.spec.ts @@ -8,7 +8,8 @@ * Functions host: a durable callHttp against the app's own endpoints exercises the * synchronous 200 path, the 202 -> Location poll loop, the enablePolling=false * opt-out, and the cross-origin credential policy (Authorization forwarded on a - * same-origin poll, stripped on a cross-origin one). Kept hermetic (loopback only) + * same-origin poll, stripped on a cross-origin one). It also asserts that a direct top-level start of + * the built-in poll orchestrator is refused (P0-2). Kept hermetic (loopback only) * so no external network is required. * * Gated: skips cleanly unless the shared host was started by globalSetup. @@ -99,4 +100,24 @@ describeMaybe("Functions host E2E — callHttp (AzureStorage)", () => { expect(body.echoed).toBe("xorigin-done"); expect(body.authorization).toBeNull(); }, 120_000); + + it("rejects a direct top-level start of the built-in poll orchestrator", async () => { + // BuiltIn__HttpPollOrchestrator is auto-registered on every app and takes a caller-supplied URI + + // token source. Starting it directly (not as a callHttp sub-orchestration) must FAIL closed rather + // than perform the request; otherwise it is an SSRF / Managed-Identity token oracle (P0-2). The + // test-app's generic StartOrchestration starter lets an external caller name it directly. + const response = await invokeHttpTrigger( + baseUrl, + "StartOrchestration", + "?orchestrationName=BuiltIn__HttpPollOrchestrator", + ); + expect(response.status).toBe(202); + + const statusQueryGetUri = parseStatusQueryGetUri(response); + const details = await waitForOrchestrationState(statusQueryGetUri, "Failed", 60); + expect(details.runtimeStatus).toBe("Failed"); + // Prove the top-level guard fired (not the activity's uri-required error): the failure carries the + // guard's message. This confirms `ctx.parent` is populated on the Functions host path. + expect(details.outputString).toContain("cannot be started as a top-level orchestration"); + }, 120_000); }); From e409567a17ec8294728e43897d3aeffd73fcd874 Mon Sep 17 00:00:00 2001 From: wangbill Date: Mon, 27 Jul 2026 10:38:35 -0700 Subject: [PATCH 05/21] fix(azure-functions-durable): use the original request URI as the callHttp poll trust anchor (P0-1 anchor-drift) Follow-up to 5015da6 (append-only). Fixes a Managed-Identity token exfiltration bypass in builtinHttpPollOrchestrator. The 202 poll loop used `currentUri` -- reassigned each hop to the callee-controlled `response.effectiveUri` -- as the same-origin trust anchor passed to buildPollRequest. Once any hop landed on an attacker origin (via a 202 Location, or a cross-origin 3xx that fetch follows), the anchor drifted there, so the next attacker->attacker hop was judged "same-origin" and buildPollRequest re-derived the pristine original Authorization/Cookie AND tokenSource -- minting a fresh MI token for the original resource and sending it to the attacker. Fix: separate the two roles that were sharing one variable. - originalUri = credential trust anchor: the originally-requested request.uri, captured once and never reassigned. Same-origin is now always judged against it, so a callee-controlled Location/redirect cannot move the origin we send credentials to. Mirrors .NET DurableOrchestrationContext.CallHttpAsync, whose `req` is never reassigned across hops. - currentUri = relative-Location resolution base only (the effective post-redirect URI), per RFC 9110 s10.2.2. Behavior unchanged. Rename buildPollRequest `base` param to `trustAnchorUri`. Behavioral consequence (same as .NET, safe default): if a service legitimately 302s to a different host and then polls there, credentials are now stripped. Tests (written RED first, confirmed failing on 5015da6 -- both leaked Authorization+Cookie -- then green after the fix): - double-202 cross-origin chain: the attacker's own same-origin Location must not restore creds/tokenSource. - redirect-moved anchor: a first hop whose effective URI is a cross-origin redirect target must still strip creds on the first poll. Existing same-origin multi-hop forwarding and relative-Location-against- effective-URI tests remain green (no over-stripping). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d86f5f69-4e1a-4c1d-b017-5e290c85cc05 --- .../src/http/builtin.ts | 33 +++++++-- .../test/unit/http-builtin.spec.ts | 74 +++++++++++++++++++ 2 files changed, 100 insertions(+), 7 deletions(-) diff --git a/packages/azure-functions-durable/src/http/builtin.ts b/packages/azure-functions-durable/src/http/builtin.ts index 6886ae9f..067d804c 100644 --- a/packages/azure-functions-durable/src/http/builtin.ts +++ b/packages/azure-functions-durable/src/http/builtin.ts @@ -263,13 +263,21 @@ export async function builtinHttpActivity(input: DurableHttpRequestPayload): Pro /** * Build the next poll request for a `202` `Location`, applying the cross-origin credential policy. * + * @param trustAnchorUri The credential trust anchor: the **originally-requested** URI, never the + * previous hop's effective/post-redirect URI. Same-origin is judged against this, so a + * callee-controlled `Location` or a followed redirect cannot move the origin we send credentials to. + * @param resolved The absolute URI of the next poll (a `Location` already resolved against the + * effective request URI). + * * @remarks * `Location` is callee-controlled, so credentials are handled per - * Azure/azure-functions-durable-extension#3443: + * Azure/azure-functions-durable-extension#3443 (`CreateLocationPollRequest`), whose poll loop passes + * the ORIGINAL request on every hop (its `req` is never reassigned): * - `x-functions-key` is **always** dropped (a function-level key won't open the master-key-protected * status endpoint, and it must never leak to another origin); * - on a **cross-origin** `Location` the `Authorization`/`Cookie` headers and the `tokenSource` are - * dropped, so an attacker-controlled first hop cannot harvest credentials; + * dropped, so an attacker-controlled hop cannot harvest credentials or force a fresh Managed-Identity + * token to be minted for the original resource and exfiltrated; * - on a **same-origin** `Location` headers and the `tokenSource` are forwarded — the async polling * pattern legitimately re-authenticates back to the same service. * @@ -278,11 +286,11 @@ export async function builtinHttpActivity(input: DurableHttpRequestPayload): Pro */ function buildPollRequest( request: DurableHttpRequestPayload, - base: string, + trustAnchorUri: string, resolved: string, enablePolling: boolean, ): DurableHttpRequestPayload { - const sameOrigin = isSameOrigin(base, resolved); + const sameOrigin = isSameOrigin(trustAnchorUri, resolved); const pollRequest: DurableHttpRequestPayload = { method: "GET", uri: resolved, enablePolling }; if (request.headers !== undefined) { @@ -332,8 +340,18 @@ export async function* builtinHttpPollOrchestrator( const enablePolling = request.enablePolling !== false; let response = (yield ctx.callActivity(BUILTIN_HTTP_ACTIVITY_NAME, request)) as BuiltinHttpActivityResult; - // Base URI for resolving a relative `Location`: the effective (post-redirect) URI when known. - let currentUri = response.effectiveUri ?? String(request.uri ?? ""); + // Two DISTINCT URIs, deliberately NOT merged into one variable: + // - `originalUri` is the credential TRUST ANCHOR: the URI the app author declared. It is captured + // once and NEVER reassigned, so a callee-controlled `Location` (or a redirect `fetch` followed) + // can never move the origin we are willing to send Authorization/Cookie/tokenSource to. Mirrors + // .NET `DurableOrchestrationContext.CallHttpAsync`, whose `req` is never reassigned across hops. + // - `currentUri` is ONLY the base for resolving a RELATIVE `Location` (RFC 9110 §10.2.2): the + // effective (post-redirect) URI of the latest hop. It legitimately moves each hop. + // Merging them (using the moving effective URI as the trust anchor) is a token-exfiltration bypass: + // once one hop lands on an attacker origin, a second attacker->attacker 202 would be judged + // "same-origin" and re-mint the original credentials to the attacker. + const originalUri = String(request.uri ?? ""); + let currentUri = response.effectiveUri ?? originalUri; while (enablePolling && response.statusCode === 202) { const headers = response.headers ?? {}; @@ -352,7 +370,8 @@ export async function* builtinHttpPollOrchestrator( const fireAt = new Date(now.getTime() + delaySeconds * 1000); yield ctx.createTimer(fireAt); - const pollRequest = buildPollRequest(request, currentUri, resolved, enablePolling); + // Trust anchor is the ORIGINAL request URI, never `currentUri` (see the note above). + const pollRequest = buildPollRequest(request, originalUri, resolved, enablePolling); response = (yield ctx.callActivity(BUILTIN_HTTP_ACTIVITY_NAME, pollRequest)) as BuiltinHttpActivityResult; currentUri = response.effectiveUri ?? resolved; } diff --git a/packages/azure-functions-durable/test/unit/http-builtin.spec.ts b/packages/azure-functions-durable/test/unit/http-builtin.spec.ts index 1ff09e7d..bd383348 100644 --- a/packages/azure-functions-durable/test/unit/http-builtin.spec.ts +++ b/packages/azure-functions-durable/test/unit/http-builtin.spec.ts @@ -446,6 +446,80 @@ describe("builtinHttpPollOrchestrator", () => { expect(request.headers).toEqual({ Authorization: "Bearer caller", "x-functions-key": "fkey" }); }); + it("does NOT restore credentials when a cross-origin hop later polls its own same-origin Location", async () => { + // Anchor-drift bypass: once a hop lands on an attacker origin, the same-origin trust anchor must + // NOT move to that origin. Otherwise the attacker returns a second 202 whose Location is on its own + // origin, that hop is judged "same-origin", and the pristine Authorization/Cookie/tokenSource are + // re-derived and sent to the attacker (a fresh Managed-Identity token is minted for the ORIGINAL + // resource and exfiltrated). The trust anchor must stay the originally-requested URI — mirrors .NET + // DurableOrchestrationContext.CallHttpAsync, whose `req` is never reassigned across hops. + const { ctx, raw } = createPollContext(now); + const request: DurableHttpRequestPayload = { + method: "GET", + uri: "https://victim.test/op", + enablePolling: true, + headers: { Authorization: "auth-original", Cookie: "sid=1", "x-functions-key": "fkey", "x-keep": "yes" }, + tokenSource: { resource: "https://management.core.windows.net/" }, + }; + const gen = builtinHttpPollOrchestrator(ctx, request); + + await gen.next(); // initial request (call 0) + // Hop 1 lands cross-origin: Location on the attacker origin -> creds stripped (already correct today). + await gen.next({ + statusCode: 202, + headers: { Location: "https://attacker.test/a", "Retry-After": "1" }, + effectiveUri: "https://victim.test/op", + }); + await gen.next(); // second poll to attacker/a (call 1) + // The attacker now 202s to a Location on ITS OWN origin (attacker -> attacker == "same origin"). + await gen.next({ + statusCode: 202, + headers: { Location: "https://attacker.test/b", "Retry-After": "1" }, + effectiveUri: "https://attacker.test/a", + }); + await gen.next(); // third poll to attacker/b (call 2) -- must remain credential-free + + const firstPoll = raw.callActivity.mock.calls[1][1] as DurableHttpRequestPayload; + expect(firstPoll.uri).toBe("https://attacker.test/a"); + expect(firstPoll.headers).toEqual({ "x-keep": "yes" }); + expect(firstPoll.tokenSource).toBeUndefined(); + + const secondPoll = raw.callActivity.mock.calls[2][1] as DurableHttpRequestPayload; + expect(secondPoll.uri).toBe("https://attacker.test/b"); + // Cross-origin relative to the ORIGINAL victim URI, so credentials must stay stripped. + expect(secondPoll.headers).toEqual({ "x-keep": "yes" }); + expect(secondPoll.tokenSource).toBeUndefined(); + }); + + it("keeps credentials stripped when the first hop's effective URI is a cross-origin redirect target", async () => { + // A single cross-origin 3xx on the first hop is enough: `fetch` follows it, so `effectiveUri` is + // already the attacker origin. The poll must be judged against the originally-requested URI, not the + // redirected-to effective URI, or the very first poll leaks credentials. + const { ctx, raw } = createPollContext(now); + const request: DurableHttpRequestPayload = { + method: "GET", + uri: "https://victim.test/op", + enablePolling: true, + headers: { Authorization: "auth-original", Cookie: "sid=1", "x-keep": "yes" }, + tokenSource: { resource: "https://management.core.windows.net/" }, + }; + const gen = builtinHttpPollOrchestrator(ctx, request); + + await gen.next(); // initial request (call 0) + // fetch followed victim -> attacker (3xx); the 202 body's Location is on the attacker origin too. + await gen.next({ + statusCode: 202, + headers: { Location: "https://attacker.test/poll", "Retry-After": "1" }, + effectiveUri: "https://attacker.test/landing", + }); + await gen.next(); // first poll (call 1) -- must be credential-free + + const poll = raw.callActivity.mock.calls[1][1] as DurableHttpRequestPayload; + expect(poll.uri).toBe("https://attacker.test/poll"); + expect(poll.headers).toEqual({ "x-keep": "yes" }); + expect(poll.tokenSource).toBeUndefined(); + }); + it("returns the first 202 without looping when polling is disabled", async () => { const { ctx, raw } = createPollContext(now); const gen = builtinHttpPollOrchestrator(ctx, { method: "GET", uri: "https://host/api", enablePolling: false }); From dbc5f47365f18742e080d474cdd3f463c8bb7f3e Mon Sep 17 00:00:00 2001 From: wangbill Date: Mon, 27 Jul 2026 12:31:32 -0700 Subject: [PATCH 06/21] Fix default sub-orchestration instance ID collision across continue-as-new The default (auto-derived) child instance ID was `${parentInstanceId}:${seqHex}`. Both inputs are constant across a continue-as-new generation: the parent instance ID is unchanged by continue-as-new, and the per-work-item sequence number resets to 0 on every work item. continue-as-new truncates history, so the new generation has no SubOrchestrationInstanceCreated event to dedupe against and emits a genuine, first -time CreateSubOrchestration for a child instance ID that still exists in the backend -> "Orchestration instance '

:0001' already exists". Replay-dedupe is scoped to one execution, but the ID was scoped to the instance; continue-as-new splits those scopes. This newly bit v4 callHttp, which schedules the built-in HTTP poll orchestrator as a default-ID sub-orchestration, so callHttp -> continueAsNew -> callHttp failed (v3 was immune because callHttp was a host-registered activity, never an instance). Fix: include the per-execution executionId (fresh on every generation) in the derived child ID -> `${parentInstanceId}:${executionId}:${seqHex}`. Mirrors DurableTask.Core TaskOrchestrationContext (ExecutionId + ":" + id) and the fresh ExecutionId minted on continue-as-new in TaskOrchestrationDispatcher. Falls back to the legacy `${parentInstanceId}:${seqHex}` when executionId is empty, so backends that do not populate it are left exactly as they are today (no regression) rather than newly broken. To make executionId actually available on the in-memory path (where it was always ""): - newExecutionStartedEvent gains an optional executionId param that sets the proto OrchestrationInstance.executionId. - InMemoryOrchestrationBackend mints a fresh executionId on createInstance and a NEW one on continue-as-new, stores it on the instance record, and threads it through. - OrchestrationExecutor.execute accepts an optional authoritative executionId (seeded from the gRPC OrchestratorRequest / backend record); the replayed ExecutionStarted value still applies and wins, and a mismatch is logged (not thrown). Replay-safe for in-flight orchestrations: handleSubOrchestrationCreated matches on taskId + action type + sub-orchestration name only, never the recorded instance ID, so histories that recorded old-format child IDs still match on replay and do not re-emit. BREAKING (behavioral): the default sub-orchestration instance ID FORMAT CHANGED. Code that hard-coded or parsed the old `${parentId}:${hex4}` shape is affected. An explicitly supplied options.instanceId is unaffected. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d86f5f69-4e1a-4c1d-b017-5e290c85cc05 --- CHANGELOG.md | 2 + .../unit/http-builtin-continue-as-new.spec.ts | 86 +++++++++++++++++++ .../src/testing/in-memory-backend.ts | 23 ++++- .../durabletask-js/src/testing/test-worker.ts | 2 +- .../src/utils/pb-helper.util.ts | 9 +- .../src/worker/orchestration-executor.ts | 24 +++++- .../worker/runtime-orchestration-context.ts | 19 +++- .../src/worker/task-hub-grpc-worker.ts | 7 +- .../test/in-memory-backend.spec.ts | 73 ++++++++++++++++ .../sub-orchestration-instance-id.spec.ts | 57 ++++++++++++ 10 files changed, 290 insertions(+), 12 deletions(-) create mode 100644 packages/azure-functions-durable/test/unit/http-builtin-continue-as-new.spec.ts create mode 100644 packages/durabletask-js/test/sub-orchestration-instance-id.spec.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index a725396a..afe39f72 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ ### Fixes +- Fix default sub-orchestration instance ID colliding across `continueAsNew` generations. The auto-derived child instance ID now includes the per-execution ID, changing its format from `${parentId}:${hex4}` to `${parentId}:${executionId}:${hex4}`. This unblocks `callHttp` after a `continueAsNew` (the built-in HTTP poll orchestrator is scheduled as a default-ID sub-orchestration and previously collided with the prior generation's child). Orchestrations that hard-coded or parsed the old `${parentId}:${hex4}` shape are affected; an explicitly supplied `options.instanceId` is unchanged. ([#333](https://github.com/microsoft/durabletask-js/pull/333)) + ## v0.3.0 (2026-03-06) diff --git a/packages/azure-functions-durable/test/unit/http-builtin-continue-as-new.spec.ts b/packages/azure-functions-durable/test/unit/http-builtin-continue-as-new.spec.ts new file mode 100644 index 00000000..f96f1ed2 --- /dev/null +++ b/packages/azure-functions-durable/test/unit/http-builtin-continue-as-new.spec.ts @@ -0,0 +1,86 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { + InMemoryOrchestrationBackend, + TestOrchestrationClient, + TestOrchestrationWorker, + OrchestrationStatus, + OrchestrationContext, + ActivityContext, + TOrchestrator, +} from "@microsoft/durabletask-js"; +import { + BUILTIN_HTTP_ACTIVITY_NAME, + BUILTIN_HTTP_POLL_ORCHESTRATOR_NAME, + builtinHttpPollOrchestrator, +} from "../../src/http/builtin"; +import { DurableHttpRequestPayload } from "../../src/http/models"; + +// End-to-end proof (item 5) that the callHttp shape survives continueAsNew. +// +// `callHttp` schedules the built-in poll orchestrator as a DEFAULT-ID sub-orchestration +// (no explicit instanceId). Before the core instance-ID fix, the default child ID was +// `${parentId}:${seqHex}` — which repeats every continueAsNew generation — so a +// `callHttp -> continueAsNew -> callHttp` orchestration failed on the second generation with +// "Orchestration instance '...:0001' already exists". This drives the REAL poll orchestrator +// through the in-memory core backend (stub HTTP activity, no network) to prove it now completes. +describe("callHttp built-in poll orchestrator across continue-as-new", () => { + let backend: InMemoryOrchestrationBackend; + let client: TestOrchestrationClient; + let worker: TestOrchestrationWorker; + + beforeEach(() => { + backend = new InMemoryOrchestrationBackend(); + client = new TestOrchestrationClient(backend); + worker = new TestOrchestrationWorker(backend); + }); + + afterEach(async () => { + try { + await worker.stop(); + } catch { + // ignore if not running + } + backend.reset(); + }); + + it("completes callHttp -> continueAsNew -> callHttp without an instance-ID collision", async () => { + // Stub the built-in HTTP activity: a terminal 200 so the poll orchestrator returns in one hop + // (no 202 loop, no durable timer, no network). + const httpActivity = async (_ctx: ActivityContext, _req: DurableHttpRequestPayload) => ({ + statusCode: 200, + headers: {}, + content: "ok", + }); + + // Mirror callHttp exactly: a single yield scheduling the built-in poll orchestrator with a + // default (auto-derived) instance ID, then continue-as-new and do it again. + const parent: TOrchestrator = async function* (ctx: OrchestrationContext, gen: number): any { + const res = yield ctx.callSubOrchestrator(BUILTIN_HTTP_POLL_ORCHESTRATOR_NAME, { + uri: "https://example.com/op", + method: "GET", + }); + if (gen < 1) { + ctx.continueAsNew(gen + 1, true); + return; + } + return (res as { statusCode: number }).statusCode; + }; + + worker.addNamedActivity(BUILTIN_HTTP_ACTIVITY_NAME, httpActivity); + worker.addNamedOrchestrator( + BUILTIN_HTTP_POLL_ORCHESTRATOR_NAME, + builtinHttpPollOrchestrator as unknown as TOrchestrator, + ); + worker.addNamedOrchestrator("CallHttpParent", parent); + await worker.start(); + + const id = await client.scheduleNewOrchestration("CallHttpParent", 0); + const state = await client.waitForOrchestrationCompletion(id, true, 15); + + expect(state).toBeDefined(); + expect(state?.runtimeStatus).toEqual(OrchestrationStatus.COMPLETED); + expect(state?.serializedOutput).toEqual(JSON.stringify(200)); + }); +}); diff --git a/packages/durabletask-js/src/testing/in-memory-backend.ts b/packages/durabletask-js/src/testing/in-memory-backend.ts index 68b5bfe5..c1748da6 100644 --- a/packages/durabletask-js/src/testing/in-memory-backend.ts +++ b/packages/durabletask-js/src/testing/in-memory-backend.ts @@ -5,12 +5,19 @@ import * as pb from "../proto/orchestrator_service_pb"; import * as pbh from "../utils/pb-helper.util"; import { OrchestrationStatus as ClientOrchestrationStatus } from "../orchestration/enum/orchestration-status.enum"; import { ParentOrchestrationInstance } from "../types/parent-orchestration-instance.type"; +import { randomUUID } from "crypto"; + +/** Mints a fresh per-execution ID (DTFx `Guid.ToString("N")` idiom: 32 hex chars, no dashes). */ +function newExecutionId(): string { + return randomUUID().replace(/-/g, ""); +} /** * Internal orchestration instance state stored by the in-memory backend. */ export interface OrchestrationInstance { instanceId: string; + executionId: string; name: string; status: pb.OrchestrationStatus; input?: string; @@ -92,8 +99,13 @@ export class InMemoryOrchestrationBackend { const now = new Date(); const startTime = scheduledStartTime && scheduledStartTime > now ? scheduledStartTime : now; + // A fresh per-execution ID. On continue-as-new a new one is minted (see completeOrchestration), + // which is what keeps default-derived child instance IDs unique across generations. + const executionId = newExecutionId(); + const instance: OrchestrationInstance = { instanceId, + executionId, name, status: pb.OrchestrationStatus.ORCHESTRATION_STATUS_PENDING, input, @@ -106,7 +118,7 @@ export class InMemoryOrchestrationBackend { // Add initial events to start the orchestration const orchestratorStarted = pbh.newOrchestratorStartedEvent(startTime); - const executionStarted = pbh.newExecutionStartedEvent(name, instanceId, input, parentInstance); + const executionStarted = pbh.newExecutionStartedEvent(name, instanceId, input, parentInstance, executionId); instance.pendingEvents.push(orchestratorStarted); instance.pendingEvents.push(executionStarted); @@ -574,6 +586,13 @@ export class InMemoryOrchestrationBackend { instance.customStatus = undefined; instance.failureDetails = undefined; instance.status = pb.OrchestrationStatus.ORCHESTRATION_STATUS_PENDING; + // Mint a NEW execution ID for the next generation. This mirrors DTFx + // (TaskOrchestrationDispatcher mints ExecutionId = Guid.NewGuid() on continue-as-new) and is + // the crux of the default child instance ID fix: because the parent instance ID and the + // per-work-item sequence number both repeat every generation, the executionId is the only + // input that varies, so it is what stops generation N+1 from re-deriving generation N's child + // IDs and colliding in createInstance. + instance.executionId = newExecutionId(); // Add new execution started events first, then carryover events. // This matches the real sidecar behavior where OrchestratorStarted and @@ -582,7 +601,7 @@ export class InMemoryOrchestrationBackend { // because it sets currentUtcDateTime, and ExecutionStarted must come before // carryover events because it initializes the orchestrator generator. const orchestratorStarted = pbh.newOrchestratorStartedEvent(new Date()); - const executionStarted = pbh.newExecutionStartedEvent(instance.name, instance.instanceId, newInput); + const executionStarted = pbh.newExecutionStartedEvent(instance.name, instance.instanceId, newInput, undefined, instance.executionId); instance.pendingEvents = [orchestratorStarted, executionStarted, ...carryoverEvents]; this.enqueueOrchestration(instance.instanceId); diff --git a/packages/durabletask-js/src/testing/test-worker.ts b/packages/durabletask-js/src/testing/test-worker.ts index 09855c66..ed389b8f 100644 --- a/packages/durabletask-js/src/testing/test-worker.ts +++ b/packages/durabletask-js/src/testing/test-worker.ts @@ -142,7 +142,7 @@ export class TestOrchestrationWorker { try { const executor = new OrchestrationExecutor(this.registry); - const result = await executor.execute(instanceId, instance.history, instance.pendingEvents); + const result = await executor.execute(instanceId, instance.history, instance.pendingEvents, instance.executionId); this.backend.completeOrchestration(instanceId, completionToken, result.actions, result.customStatus); } catch (error: unknown) { diff --git a/packages/durabletask-js/src/utils/pb-helper.util.ts b/packages/durabletask-js/src/utils/pb-helper.util.ts index 586de9ea..9f353798 100644 --- a/packages/durabletask-js/src/utils/pb-helper.util.ts +++ b/packages/durabletask-js/src/utils/pb-helper.util.ts @@ -22,11 +22,18 @@ export function newOrchestratorStartedEvent(timestamp?: Date | null): pb.History return event; } -export function newExecutionStartedEvent(name: string, instanceId: string, encodedInput?: string, parentInstance?: { name: string; instanceId: string; taskScheduledId: number }): pb.HistoryEvent { +export function newExecutionStartedEvent(name: string, instanceId: string, encodedInput?: string, parentInstance?: { name: string; instanceId: string; taskScheduledId: number }, executionId?: string): pb.HistoryEvent { const ts = new Timestamp(); const orchestrationInstance = new pb.OrchestrationInstance(); orchestrationInstance.setInstanceid(instanceId); + // The executionId is the per-generation identity of the orchestration. It changes on every + // continue-as-new, so it is what makes default-derived child instance IDs unique across + // generations (see RuntimeOrchestrationContext.callSubOrchestrator). Leave it unset when not + // provided so existing callers/behavior are unchanged. + if (executionId) { + orchestrationInstance.setExecutionid(getStringValue(executionId)); + } const executionStartedEvent = new pb.ExecutionStartedEvent(); executionStartedEvent.setName(name); diff --git a/packages/durabletask-js/src/worker/orchestration-executor.ts b/packages/durabletask-js/src/worker/orchestration-executor.ts index dfcb5df4..1e0a9206 100644 --- a/packages/durabletask-js/src/worker/orchestration-executor.ts +++ b/packages/durabletask-js/src/worker/orchestration-executor.ts @@ -61,6 +61,7 @@ export class OrchestrationExecutor { instanceId: string, oldEvents: pb.HistoryEvent[], newEvents: pb.HistoryEvent[], + executionId?: string, ): Promise { if (!newEvents?.length) { throw new OrchestrationStateError("The new history event list must have at least one event in it"); @@ -84,6 +85,12 @@ export class OrchestrationExecutor { } const ctx = new RuntimeOrchestrationContext(instanceId); + // Seed the execution ID from the authoritative source (the OrchestratorRequest on the gRPC path, + // or the backend record on the in-memory path). The ExecutionStarted event replayed below may + // also carry it; handleExecutionStarted reconciles the two. + if (executionId) { + ctx._executionId = executionId; + } try { // Rebuild the local state by replaying the history events into the orchestrator function @@ -251,10 +258,19 @@ export class OrchestrationExecutor { throw new OrchestratorNotRegisteredError(executionStartedEvent?.getName()); } - // Set the execution ID from the orchestration instance - const executionId = executionStartedEvent?.getOrchestrationinstance()?.getExecutionid()?.getValue(); - if (executionId) { - ctx._executionId = executionId; + // Set the execution ID from the orchestration instance. If the executor was already seeded with + // an authoritative executionId (from the OrchestratorRequest / backend record), the two should + // agree. If both are present and differ, that is a real anomaly worth surfacing — log it, but do + // not throw, and let the history (ExecutionStarted) value win since it is what replays. + const eventExecutionId = executionStartedEvent?.getOrchestrationinstance()?.getExecutionid()?.getValue(); + if (eventExecutionId) { + if (ctx._executionId && ctx._executionId !== eventExecutionId) { + this._logger.warn( + `executionId mismatch for instance '${ctx._instanceId}': seeded='${ctx._executionId}' ` + + `history='${eventExecutionId}'; using the history value`, + ); + } + ctx._executionId = eventExecutionId; } // Track the orchestrator name for lifecycle logs diff --git a/packages/durabletask-js/src/worker/runtime-orchestration-context.ts b/packages/durabletask-js/src/worker/runtime-orchestration-context.ts index 594b43f9..cc25d4de 100644 --- a/packages/durabletask-js/src/worker/runtime-orchestration-context.ts +++ b/packages/durabletask-js/src/worker/runtime-orchestration-context.ts @@ -376,11 +376,24 @@ export class RuntimeOrchestrationContext extends OrchestrationContext { // Get instance ID from options or generate a deterministic one let instanceId = options?.instanceId; - // Create a deterministic instance ID based on the parent instance ID - // use the instanceId and append the id to it in hexadecimal with 4 digits (e.g. 0001) + // Derive a deterministic instance ID for the child. The inputs must be replay-stable (re-derived + // identically on every work item) yet unique across continue-as-new generations. The sequence + // number resets to 0 on every work item and the parent instance ID is unchanged by + // continue-as-new, so those two alone repeat every generation — which caused generation N+1 to + // re-derive generation N's child IDs and collide ("instance already exists") the moment an + // orchestration did continue-as-new and then scheduled a default-ID sub-orchestration (e.g. + // callHttp). Including the per-execution executionId (fresh on every generation) makes the ID + // unique per generation. Mirrors DurableTask.Core TaskOrchestrationContext (ExecutionId + ":" + id). + // + // FALLBACK: if executionId is empty (a backend that does not populate it), fall back to the + // legacy `${instanceId}:${suffix}` format. This re-exposes the cross-generation collision for + // such backends, but that leaves them exactly as they are today (no regression), whereas + // throwing would newly break working orchestrations. if (!instanceId) { const instanceIdSuffix = id.toString(16).padStart(4, "0"); - instanceId = `${this._instanceId}:${instanceIdSuffix}`; + instanceId = this._executionId + ? `${this._instanceId}:${this._executionId}:${instanceIdSuffix}` + : `${this._instanceId}:${instanceIdSuffix}`; } const encodedInput = input !== undefined ? JSON.stringify(input) : undefined; diff --git a/packages/durabletask-js/src/worker/task-hub-grpc-worker.ts b/packages/durabletask-js/src/worker/task-hub-grpc-worker.ts index 952fee9d..56bad094 100644 --- a/packages/durabletask-js/src/worker/task-hub-grpc-worker.ts +++ b/packages/durabletask-js/src/worker/task-hub-grpc-worker.ts @@ -810,7 +810,12 @@ export class TaskHubGrpcWorker { try { const executor = new OrchestrationExecutor(this._registry, this._logger); - const result = await executor.execute(req.getInstanceid(), req.getPasteventsList(), req.getNeweventsList()); + const result = await executor.execute( + req.getInstanceid(), + req.getPasteventsList(), + req.getNeweventsList(), + req.getExecutionid()?.getValue(), + ); // Process actions to inject trace context into scheduled tasks, sub-orchestrations, etc. if (tracingResult) { diff --git a/packages/durabletask-js/test/in-memory-backend.spec.ts b/packages/durabletask-js/test/in-memory-backend.spec.ts index 6ce4bf03..e2fac556 100644 --- a/packages/durabletask-js/test/in-memory-backend.spec.ts +++ b/packages/durabletask-js/test/in-memory-backend.spec.ts @@ -331,6 +331,79 @@ describe("In-Memory Backend", () => { expect(state?.serializedOutput).toEqual(JSON.stringify(5)); }); + it("should not collide default sub-orchestration instance IDs across continue-as-new generations", async () => { + // Regression for the callHttp-on-continueAsNew collision: a default (auto-derived) child + // instance ID must be unique per generation. Before the fix the derived ID was + // `${parentId}:${seqHex}`, and since neither input varies across a continueAsNew generation + // the second generation re-derived the first generation's child ID verbatim and the backend + // rejected it with "Orchestration instance '...:0001' already exists". + const childInstanceIds: string[] = []; + + const child: TOrchestrator = async (ctx: OrchestrationContext, input: string) => { + if (!ctx.isReplaying) { + childInstanceIds.push(ctx.instanceId); + } + return `child-${input}`; + }; + + const parent: TOrchestrator = async function* (ctx: OrchestrationContext, gen: number): any { + const r = yield ctx.callSubOrchestrator(child, `gen${gen}`); + if (gen < 1) { + ctx.continueAsNew(gen + 1, true); + return; + } + return r; + }; + + worker.addOrchestrator(child); + worker.addOrchestrator(parent); + await worker.start(); + + const id = await client.scheduleNewOrchestration(parent, 0); + const state = await client.waitForOrchestrationCompletion(id, true, 10); + + expect(state).toBeDefined(); + expect(state?.runtimeStatus).toEqual(OrchestrationStatus.COMPLETED); + expect(state?.serializedOutput).toEqual(JSON.stringify("child-gen1")); + + // Both generations scheduled a default-ID sub-orchestration; the IDs must differ. + expect(childInstanceIds.length).toBe(2); + expect(childInstanceIds[0]).not.toEqual(childInstanceIds[1]); + }); + + it("gives distinct deterministic default IDs to sequential sub-orchestrations within one execution", async () => { + // Control for the fix above: within a SINGLE execution (no continueAsNew) two sequential + // default-ID sub-orchestrations must still get stable, distinct IDs (their sequence numbers + // differ). A fix that made every child ID identical would pass the regression test's + // "different across generations" check only by accident and would break this one. + const childInstanceIds: string[] = []; + + const child: TOrchestrator = async (ctx: OrchestrationContext) => { + if (!ctx.isReplaying) { + childInstanceIds.push(ctx.instanceId); + } + return "ok"; + }; + + const parent: TOrchestrator = async function* (ctx: OrchestrationContext): any { + yield ctx.callSubOrchestrator(child); + yield ctx.callSubOrchestrator(child); + return "done"; + }; + + worker.addOrchestrator(child); + worker.addOrchestrator(parent); + await worker.start(); + + const id = await client.scheduleNewOrchestration(parent); + const state = await client.waitForOrchestrationCompletion(id, true, 10); + + expect(state).toBeDefined(); + expect(state?.runtimeStatus).toEqual(OrchestrationStatus.COMPLETED); + expect(childInstanceIds.length).toBe(2); + expect(new Set(childInstanceIds).size).toBe(2); + }); + it("should deliver carryover events after ExecutionStarted during continue-as-new", async () => { // This test verifies that carryover events (saved external events) are // delivered AFTER OrchestratorStarted and ExecutionStarted when diff --git a/packages/durabletask-js/test/sub-orchestration-instance-id.spec.ts b/packages/durabletask-js/test/sub-orchestration-instance-id.spec.ts new file mode 100644 index 00000000..eb172839 --- /dev/null +++ b/packages/durabletask-js/test/sub-orchestration-instance-id.spec.ts @@ -0,0 +1,57 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { RuntimeOrchestrationContext } from "../src/worker/runtime-orchestration-context"; +import * as pb from "../src/proto/orchestrator_service_pb"; + +/** Returns the instance IDs of every CreateSubOrchestration action pending on the context. */ +function pendingSubOrchestrationInstanceIds(ctx: RuntimeOrchestrationContext): string[] { + return Object.values(ctx._pendingActions) + .filter((action: pb.OrchestratorAction) => action.hasCreatesuborchestration()) + .map((action: pb.OrchestratorAction) => action.getCreatesuborchestration()!.getInstanceid()); +} + +describe("default sub-orchestration instance ID derivation", () => { + it("includes the per-execution executionId so IDs are unique across continue-as-new generations", () => { + const ctx = new RuntimeOrchestrationContext("parent-instance"); + ctx._executionId = "execA"; + + ctx.callSubOrchestrator("Child"); + + expect(pendingSubOrchestrationInstanceIds(ctx)).toEqual(["parent-instance:execA:0001"]); + }); + + it("falls back to the legacy `${parentId}:${suffix}` format when executionId is empty", () => { + // A backend that does not populate executionId leaves callers exactly as they are today (the + // legacy format, which can collide across continue-as-new) rather than throwing and newly + // breaking working orchestrations. + const ctx = new RuntimeOrchestrationContext("parent-instance"); + ctx._executionId = ""; + + ctx.callSubOrchestrator("Child"); + + expect(pendingSubOrchestrationInstanceIds(ctx)).toEqual(["parent-instance:0001"]); + }); + + it("gives sequential sub-orchestrations within one execution distinct deterministic IDs", () => { + const ctx = new RuntimeOrchestrationContext("parent-instance"); + ctx._executionId = "execA"; + + ctx.callSubOrchestrator("Child"); + ctx.callSubOrchestrator("Child"); + + const ids = pendingSubOrchestrationInstanceIds(ctx); + expect(new Set(ids).size).toBe(2); + expect(ids).toContain("parent-instance:execA:0001"); + expect(ids).toContain("parent-instance:execA:0002"); + }); + + it("does not touch an explicitly provided instance ID", () => { + const ctx = new RuntimeOrchestrationContext("parent-instance"); + ctx._executionId = "execA"; + + ctx.callSubOrchestrator("Child", undefined, { instanceId: "my-explicit-id" }); + + expect(pendingSubOrchestrationInstanceIds(ctx)).toEqual(["my-explicit-id"]); + }); +}); From 5961a40349b59b16cbb8d9ae092b9d279b50f35f Mon Sep 17 00:00:00 2001 From: wangbill Date: Mon, 27 Jul 2026 12:49:39 -0700 Subject: [PATCH 07/21] test(e2e-azuremanaged): DTS regression for sub-orch ID collision across continue-as-new MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a DTS-emulator e2e test to orchestration.spec.ts (the spec path already listed in the dts-e2e-tests CI matrix, so it actually runs) for the default-derived sub-orchestration instance ID collision fixed in dbc5f47. The three tests added in dbc5f47 all run on the in-memory backend, which populates the per-execution executionId itself — so they prove the derivation formula but not its load-bearing premise: that the REAL DTS backend supplies an executionId (via OrchestratorRequest.executionId and/or ExecutionStarted.orchestrationInstance.executionId). If it does not, the fix silently falls back to the legacy colliding `${parentId}:${hex4}` format and every in-memory test still passes. This test closes that gap on the actual DTS emulator: a parent schedules a default-ID (no explicit instanceId) sub-orchestration, continue-as-news, then schedules another default-ID sub-orchestration, carrying generation 0's child instance ID forward via the continue-as-new input. It asserts the parent COMPLETES, both children ran their activity (childActivityRuns === 2), the two generations' child IDs differ, and — the premise check — each child ID is `${parentId}:${executionId}:${hex4}` (the remainder after the parent prefix contains a colon, i.e. an executionId segment), not the legacy 2-segment fallback. The child returns its own ctx.instanceId and the test logs both IDs so the real DTS-derived value is visible in CI. Pre-fix this fails with "Orchestration instance ':0001' already exists" (or the parent never completes); if it fails post-fix because DTS does not supply executionId, that is a finding to escalate rather than paper over. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d86f5f69-4e1a-4c1d-b017-5e290c85cc05 --- test/e2e-azuremanaged/orchestration.spec.ts | 90 +++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/test/e2e-azuremanaged/orchestration.spec.ts b/test/e2e-azuremanaged/orchestration.spec.ts index 38f5d9c5..04ec1e34 100644 --- a/test/e2e-azuremanaged/orchestration.spec.ts +++ b/test/e2e-azuremanaged/orchestration.spec.ts @@ -590,6 +590,96 @@ describe("Durable Task Scheduler (DTS) E2E Tests", () => { expect(state?.serializedOutput).toEqual(JSON.stringify(10)); }, 31000); + // Regression for the default-derived sub-orchestration instance ID colliding across + // continue-as-new generations (issue #318 / PR #333). A parent that schedules a DEFAULT-ID + // (no explicit instanceId) sub-orchestration, then continue-as-news, then schedules another + // default-ID sub-orchestration must NOT collide. Pre-fix the derived child ID was + // `${parentId}:${hex4}` — identical in every generation because continue-as-new truncates the + // history and resets the per-work-item sequence counter — so generation 1 re-derived + // generation 0's child ID. + // + // Crucially, this runs against the REAL DTS backend (not the in-memory harness), so it is the + // only test that verifies the fix's load-bearing premise: that the backend actually supplies a + // per-execution executionId (via OrchestratorRequest.executionId and/or + // ExecutionStarted.orchestrationInstance.executionId). If it does not, the fix silently falls + // back to the legacy colliding `${parentId}:${hex4}` format and this test fails — surfacing the + // regression instead of letting the in-memory tests pass for the wrong reason. + it("should not collide default-derived sub-orchestration instance IDs across continue-as-new", async () => { + let childActivityRuns = 0; + const bumpChild = (_: ActivityContext, gen: number): number => { + childActivityRuns++; + return gen; + }; + + // The child returns its OWN instance ID so the test can inspect the real DTS-derived shape. + const child: TOrchestrator = async function* (ctx: OrchestrationContext, gen: number): any { + yield ctx.callActivity(bumpChild, gen); + return ctx.instanceId; + }; + + // Parent: generation 0 schedules a default-ID child and carries its ID forward via the + // continue-as-new input; generation 1 schedules another default-ID child, then returns BOTH + // children's instance IDs so the test can compare them. + const parent: TOrchestrator = async function* ( + ctx: OrchestrationContext, + input: { gen: number; gen0ChildId?: string }, + ): any { + const childId: string = yield ctx.callSubOrchestrator(child, input.gen); + if (input.gen < 1) { + ctx.continueAsNew({ gen: input.gen + 1, gen0ChildId: childId }, true); + return; + } + return { gen0ChildId: input.gen0ChildId, gen1ChildId: childId }; + }; + + taskHubWorker.addActivity(bumpChild); + taskHubWorker.addOrchestrator(child); + taskHubWorker.addOrchestrator(parent); + await taskHubWorker.start(); + + const id = await taskHubClient.scheduleNewOrchestration(parent, { gen: 0 }); + const state = await taskHubClient.waitForOrchestrationCompletion(id, undefined, 60); + + expect(state).toBeDefined(); + // Pre-fix this is FAILED with "Orchestration instance ':0001' already exists" (or the + // parent never completes because generation 1's colliding child cannot be created). + expect(state?.runtimeStatus).toEqual(OrchestrationStatus.ORCHESTRATION_STATUS_COMPLETED); + + const output = JSON.parse(state!.serializedOutput!) as { + gen0ChildId: string; + gen1ChildId: string; + }; + // Surface the real DTS-derived child instance IDs in the CI log — the whole point of this test. + console.log( + `[suborch-id-collision-e2e] parentId=${id} gen0ChildId=${output.gen0ChildId} ` + + `gen1ChildId=${output.gen1ChildId} childActivityRuns=${childActivityRuns}`, + ); + + // Both generations' children actually ran their activity exactly once each. (A silent + // second-child-never-created would leave this at 1.) + expect(childActivityRuns).toEqual(2); + + // Both children produced an ID, and the two generations got DIFFERENT IDs (no collision, and no + // silent reuse of generation 0's completed child for generation 1). + expect(output.gen0ChildId).toBeTruthy(); + expect(output.gen1ChildId).toBeTruthy(); + expect(output.gen0ChildId).not.toEqual(output.gen1ChildId); + + // Premise check: both child IDs are prefixed by the (top-level, colon-free) parent ID, and the + // remainder after that prefix contains an executionId segment — i.e. the 3-segment + // `${parentId}:${executionId}:${hex4}` shape, NOT the legacy 2-segment `${parentId}:${hex4}` + // fallback. The remainders differ because the executionId is minted fresh per generation, which + // is exactly what stops the collision. + for (const childId of [output.gen0ChildId, output.gen1ChildId]) { + expect(childId.startsWith(`${id}:`)).toBe(true); + } + const gen0Rest = output.gen0ChildId.slice(id.length + 1); + const gen1Rest = output.gen1ChildId.slice(id.length + 1); + expect(gen0Rest.includes(":")).toBe(true); + expect(gen1Rest.includes(":")).toBe(true); + expect(gen0Rest).not.toEqual(gen1Rest); + }, 61000); + it("should be able to run a single orchestration without activity", async () => { const orchestrator: TOrchestrator = async (ctx: OrchestrationContext, startVal: number) => { return startVal + 1; From 2a71dc6bae9bf8d20e09f8a4a8dfcf871156d75e Mon Sep 17 00:00:00 2001 From: wangbill Date: Mon, 27 Jul 2026 12:56:22 -0700 Subject: [PATCH 08/21] chore: revert CHANGELOG.md entry per maintainer preference Removes the CHANGELOG.md entry added in dbc5f47. The maintainer's standing preference is to not modify CHANGELOG.md in this repo. The BREAKING default sub-orchestration instance ID format change remains permanently documented in the dbc5f47 commit message; it is not relocated to any other doc. This restores CHANGELOG.md to its origin/main state. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d86f5f69-4e1a-4c1d-b017-5e290c85cc05 --- CHANGELOG.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index afe39f72..a725396a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,8 +4,6 @@ ### Fixes -- Fix default sub-orchestration instance ID colliding across `continueAsNew` generations. The auto-derived child instance ID now includes the per-execution ID, changing its format from `${parentId}:${hex4}` to `${parentId}:${executionId}:${hex4}`. This unblocks `callHttp` after a `continueAsNew` (the built-in HTTP poll orchestrator is scheduled as a default-ID sub-orchestration and previously collided with the prior generation's child). Orchestrations that hard-coded or parsed the old `${parentId}:${hex4}` shape are affected; an explicitly supplied `options.instanceId` is unchanged. ([#333](https://github.com/microsoft/durabletask-js/pull/333)) - ## v0.3.0 (2026-03-06) From 074d419fab1353d7879ba3b77b94a1edb182b5df Mon Sep 17 00:00:00 2001 From: wangbill Date: Mon, 27 Jul 2026 13:12:17 -0700 Subject: [PATCH 09/21] fix(core): key default sub-orchestration instance ID on executionId only (DTS 100-char limit) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 5 (dbc5f47) fixed the cross-generation collision of default-derived sub-orchestration instance IDs by including the per-execution executionId, but used the shape `${parentInstanceId}:${executionId}:${hex4}`. Because that prepends the FULL parent instance ID — which itself already contains every ancestor's ID — the derived ID grows ~1+len(parentId) chars per nesting level. Measured against real DTS IDs it adds 38 chars per level, so a two-level nest (top-level orchestrator -> sub-orchestrator -> callHttp) produces a ~112-char instance ID and exceeds the Durable Task Scheduler 100-character instance-ID limit, which DTS enforces server-side (the JS client/worker does not, so it was not caught locally). That silently broke a mainstream pattern — and callHttp is itself scheduled as a default-ID sub-orchestration, so `callHttp` from within a sub-orchestration would fail on DTS. Change the default derivation to `${executionId}:${hex4}`, dropping the parent instance ID prefix. This mirrors DurableTask.Core TaskOrchestrationContext.cs:197 (`OrchestrationInstance.ExecutionId + ":" + id`), which omits the parent prefix for two reasons: (1) executionId is a globally-unique GUID, so the parent instance ID contributes nothing to uniqueness; (2) each nesting level gets its own fresh executionId, so the ID is constant-length (~37 chars) at every depth instead of growing without bound. Uniqueness guarantees are unchanged: distinct across continue-as-new generations (fresh executionId per generation), distinct between sibling calls within one execution (sequence-number suffix), and distinct between different parents (distinct executionId). The legacy `${parentInstanceId}:${hex4}` fallback for backends that do not populate executionId is preserved exactly as-is. Tests: - sub-orchestration-instance-id.spec.ts: update the derivation assertions to the two-segment `${executionId}:${hex4}` shape; add a length-regression test that nests four levels from a realistic 36-char auto-GUID and asserts every derived ID is <= 100 chars and constant-length (37). - test/e2e-azuremanaged/orchestration.spec.ts (real DTS emulator): update the continue-as-new premise assertions to the two-segment shape (<= 100 chars, two colon-separated segments, and NOT prefixed by the parent instance ID — the fallback would be, which would signal DTS did not supply executionId); add a nesting e2e test (top -> child -> grandchild, all default IDs) asserting every derived ID stays within the DTS length limit at the depth that previously broke. BREAKING CHANGE: the DEFAULT (auto-derived) sub-orchestration instance ID format changed to `${executionId}:${hex4}` (was `${parentInstanceId}:${hex4}` before dbc5f47, and briefly `${parentInstanceId}:${executionId}:${hex4}` in dbc5f47). Orchestrations that hard-coded or parsed the default child instance ID shape are affected. Passing an explicit `options.instanceId` is unaffected. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d86f5f69-4e1a-4c1d-b017-5e290c85cc05 --- .../worker/runtime-orchestration-context.ts | 28 ++++--- .../sub-orchestration-instance-id.spec.ts | 42 ++++++++++- test/e2e-azuremanaged/orchestration.spec.ts | 75 ++++++++++++++++--- 3 files changed, 121 insertions(+), 24 deletions(-) diff --git a/packages/durabletask-js/src/worker/runtime-orchestration-context.ts b/packages/durabletask-js/src/worker/runtime-orchestration-context.ts index cc25d4de..4dfb27bc 100644 --- a/packages/durabletask-js/src/worker/runtime-orchestration-context.ts +++ b/packages/durabletask-js/src/worker/runtime-orchestration-context.ts @@ -376,14 +376,24 @@ export class RuntimeOrchestrationContext extends OrchestrationContext { // Get instance ID from options or generate a deterministic one let instanceId = options?.instanceId; - // Derive a deterministic instance ID for the child. The inputs must be replay-stable (re-derived - // identically on every work item) yet unique across continue-as-new generations. The sequence - // number resets to 0 on every work item and the parent instance ID is unchanged by - // continue-as-new, so those two alone repeat every generation — which caused generation N+1 to - // re-derive generation N's child IDs and collide ("instance already exists") the moment an - // orchestration did continue-as-new and then scheduled a default-ID sub-orchestration (e.g. - // callHttp). Including the per-execution executionId (fresh on every generation) makes the ID - // unique per generation. Mirrors DurableTask.Core TaskOrchestrationContext (ExecutionId + ":" + id). + // Derive a deterministic instance ID for the child: `${executionId}:${suffix}`. The inputs must + // be replay-stable (re-derived identically on every work item) yet unique across continue-as-new + // generations. The sequence number resets to 0 on every work item and the parent instance ID is + // unchanged by continue-as-new, so those two alone repeat every generation — which caused + // generation N+1 to re-derive generation N's child IDs and collide ("instance already exists") + // the moment an orchestration did continue-as-new and then scheduled a default-ID + // sub-orchestration (e.g. callHttp). The per-execution executionId is minted fresh on every + // generation, so keying the ID on it makes it unique per generation. + // + // We deliberately DO NOT prepend the parent instance ID. Mirrors DurableTask.Core + // TaskOrchestrationContext.cs:197 (`OrchestrationInstance.ExecutionId + ":" + id`), which omits + // it for two reasons: (1) executionId is a globally-unique GUID, so the parent instance ID adds + // no uniqueness — it is pure redundancy; (2) because each nesting level gets its own fresh + // executionId, the ID stays constant-length at every depth (~37 chars). Prepending the parent + // instance ID instead would concatenate every ancestor's ID, growing ~1+len(instanceId) chars per + // level and exceeding the Durable Task Scheduler 100-character instance-ID limit at only ~2 levels + // of nesting (e.g. a top-level orchestrator -> sub-orchestrator -> callHttp), silently breaking a + // mainstream pattern on the server side (the limit is not enforced client-side). // // FALLBACK: if executionId is empty (a backend that does not populate it), fall back to the // legacy `${instanceId}:${suffix}` format. This re-exposes the cross-generation collision for @@ -392,7 +402,7 @@ export class RuntimeOrchestrationContext extends OrchestrationContext { if (!instanceId) { const instanceIdSuffix = id.toString(16).padStart(4, "0"); instanceId = this._executionId - ? `${this._instanceId}:${this._executionId}:${instanceIdSuffix}` + ? `${this._executionId}:${instanceIdSuffix}` : `${this._instanceId}:${instanceIdSuffix}`; } diff --git a/packages/durabletask-js/test/sub-orchestration-instance-id.spec.ts b/packages/durabletask-js/test/sub-orchestration-instance-id.spec.ts index eb172839..3d482c99 100644 --- a/packages/durabletask-js/test/sub-orchestration-instance-id.spec.ts +++ b/packages/durabletask-js/test/sub-orchestration-instance-id.spec.ts @@ -12,13 +12,16 @@ function pendingSubOrchestrationInstanceIds(ctx: RuntimeOrchestrationContext): s } describe("default sub-orchestration instance ID derivation", () => { - it("includes the per-execution executionId so IDs are unique across continue-as-new generations", () => { + it("keys the default ID on the per-execution executionId (not the parent instance ID) so IDs are unique across continue-as-new generations", () => { const ctx = new RuntimeOrchestrationContext("parent-instance"); ctx._executionId = "execA"; ctx.callSubOrchestrator("Child"); - expect(pendingSubOrchestrationInstanceIds(ctx)).toEqual(["parent-instance:execA:0001"]); + // `${executionId}:${suffix}` — deliberately NOT prefixed with the parent instance ID (see the + // derivation comment: executionId is globally unique, and prepending the parent ID would blow the + // DTS 100-char instance-ID limit when sub-orchestrations nest). + expect(pendingSubOrchestrationInstanceIds(ctx)).toEqual(["execA:0001"]); }); it("falls back to the legacy `${parentId}:${suffix}` format when executionId is empty", () => { @@ -42,8 +45,8 @@ describe("default sub-orchestration instance ID derivation", () => { const ids = pendingSubOrchestrationInstanceIds(ctx); expect(new Set(ids).size).toBe(2); - expect(ids).toContain("parent-instance:execA:0001"); - expect(ids).toContain("parent-instance:execA:0002"); + expect(ids).toContain("execA:0001"); + expect(ids).toContain("execA:0002"); }); it("does not touch an explicitly provided instance ID", () => { @@ -54,4 +57,35 @@ describe("default sub-orchestration instance ID derivation", () => { expect(pendingSubOrchestrationInstanceIds(ctx)).toEqual(["my-explicit-id"]); }); + + it("keeps default-derived IDs within the DTS 100-char instance-ID limit at every nesting depth", () => { + // The Durable Task Scheduler caps instance IDs at 100 characters (enforced server-side). Because + // the derived ID is `${executionId}:${suffix}` — keyed on the child's OWN fresh executionId and + // NOT prefixed with the parent instance ID — it must stay constant-length no matter how deeply + // sub-orchestrations nest. (The pre-fix `${parentId}:${executionId}:${suffix}` shape concatenated + // every ancestor and blew past 100 chars at ~2 levels deep.) Simulate a realistic top-level + // 36-char auto-GUID and a fresh 32-hex executionId per level, deriving each level's child from the + // previous level's derived ID. + const executionIds = ["0", "1", "2", "3"].map((c) => c.repeat(32)); // 32-hex-length, distinct per level + let parentInstanceId = "4cb1b016-ec71-4608-bdeb-328306cc0215"; // 36 chars, like an auto-generated GUID + const derived: string[] = []; + + for (const executionId of executionIds) { + const ctx = new RuntimeOrchestrationContext(parentInstanceId); + ctx._executionId = executionId; + ctx.callSubOrchestrator("Child"); + const childId = pendingSubOrchestrationInstanceIds(ctx)[0]; + derived.push(childId); + parentInstanceId = childId; // nest: this child becomes the next level's parent + } + + for (const childId of derived) { + expect(childId.length).toBeLessThanOrEqual(100); + } + // Constant length at every depth: each level is executionId(32) + ":" + suffix(4) = 37 chars, + // independent of how long the parent instance ID grew. + expect(new Set(derived.map((childId) => childId.length)).size).toBe(1); + expect(derived[0].length).toBe(37); + expect(derived[0]).toEqual("00000000000000000000000000000000:0001"); + }); }); diff --git a/test/e2e-azuremanaged/orchestration.spec.ts b/test/e2e-azuremanaged/orchestration.spec.ts index 04ec1e34..95698567 100644 --- a/test/e2e-azuremanaged/orchestration.spec.ts +++ b/test/e2e-azuremanaged/orchestration.spec.ts @@ -665,19 +665,72 @@ describe("Durable Task Scheduler (DTS) E2E Tests", () => { expect(output.gen1ChildId).toBeTruthy(); expect(output.gen0ChildId).not.toEqual(output.gen1ChildId); - // Premise check: both child IDs are prefixed by the (top-level, colon-free) parent ID, and the - // remainder after that prefix contains an executionId segment — i.e. the 3-segment - // `${parentId}:${executionId}:${hex4}` shape, NOT the legacy 2-segment `${parentId}:${hex4}` - // fallback. The remainders differ because the executionId is minted fresh per generation, which - // is exactly what stops the collision. + // Premise check: each child ID is `${executionId}:${hex4}` — keyed on the per-generation + // executionId that the REAL DTS backend minted, NOT the legacy `${parentId}:${hex4}` fallback we + // emit only when executionId is empty. So each ID: + // - is <= 100 chars (the DTS instance-ID limit; the executionId-keyed form is constant-length), + // - has exactly TWO colon-separated segments (executionId + suffix), and + // - does NOT start with the parent instance ID (the legacy fallback would start with `${id}:`; + // the correct form starts with the 32-hex executionId instead). + // If DTS had NOT populated executionId we would fall back to `${id}:${hex4}` and these assertions + // would fail — surfacing a silent production no-op instead of hiding it behind green in-memory + // tests. (gen0 != gen1 is asserted above: the executionId is minted fresh per generation.) for (const childId of [output.gen0ChildId, output.gen1ChildId]) { - expect(childId.startsWith(`${id}:`)).toBe(true); + expect(childId.length).toBeLessThanOrEqual(100); + expect(childId.split(":").length).toEqual(2); + expect(childId.startsWith(`${id}:`)).toBe(false); } - const gen0Rest = output.gen0ChildId.slice(id.length + 1); - const gen1Rest = output.gen1ChildId.slice(id.length + 1); - expect(gen0Rest.includes(":")).toBe(true); - expect(gen1Rest.includes(":")).toBe(true); - expect(gen0Rest).not.toEqual(gen1Rest); + }, 61000); + + // Companion length check for the same fix (PR #333): default-derived sub-orchestration instance IDs + // must stay within the DTS 100-character instance-ID limit even when sub-orchestrations NEST. The + // pre-fix `${parentId}:${executionId}:${hex4}` shape concatenated every ancestor, so a top-level + // orchestrator -> sub-orchestrator -> callHttp (2 levels) produced a ~112-char ID that DTS rejects. + // The `${executionId}:${hex4}` shape is constant-length (~37 chars) at every depth. Runs on the REAL + // DTS backend so the length is measured against the real server-side limit. + it("keeps default-derived sub-orchestration instance IDs within the DTS length limit when nested", async () => { + const grandchild: TOrchestrator = async (ctx: OrchestrationContext): Promise => { + return ctx.instanceId; + }; + const child: TOrchestrator = async function* (ctx: OrchestrationContext): any { + const grandchildId: string = yield ctx.callSubOrchestrator(grandchild); + return { childId: ctx.instanceId, grandchildId }; + }; + const top: TOrchestrator = async function* (ctx: OrchestrationContext): any { + const nested: { childId: string; grandchildId: string } = yield ctx.callSubOrchestrator(child); + return nested; + }; + + taskHubWorker.addOrchestrator(grandchild); + taskHubWorker.addOrchestrator(child); + taskHubWorker.addOrchestrator(top); + await taskHubWorker.start(); + + const id = await taskHubClient.scheduleNewOrchestration(top, null); + const state = await taskHubClient.waitForOrchestrationCompletion(id, undefined, 60); + + expect(state).toBeDefined(); + expect(state?.runtimeStatus).toEqual(OrchestrationStatus.ORCHESTRATION_STATUS_COMPLETED); + + const output = JSON.parse(state!.serializedOutput!) as { childId: string; grandchildId: string }; + // Surface the real DTS-derived nested instance IDs and their measured lengths in the CI log. + console.log( + `[suborch-id-nesting-e2e] parentId=${id} childId=${output.childId} ` + + `grandchildId=${output.grandchildId} childIdLen=${output.childId.length} ` + + `grandchildIdLen=${output.grandchildId.length}`, + ); + + // Two levels of default-ID nesting (the depth that broke pre-fix). Every derived ID stays within + // the DTS limit and is the constant-length two-segment `${executionId}:${hex4}` shape — not the + // parent-prefixed shape, so it never grows with depth. + for (const childId of [output.childId, output.grandchildId]) { + expect(childId).toBeTruthy(); + expect(childId.length).toBeLessThanOrEqual(100); + expect(childId.split(":").length).toEqual(2); + expect(childId.startsWith(`${id}:`)).toBe(false); + } + // Each level is keyed on its own fresh executionId, so the two levels' IDs differ. + expect(output.childId).not.toEqual(output.grandchildId); }, 61000); it("should be able to run a single orchestration without activity", async () => { From 81d51acf81f62df4e400a1d2990f361300d34caf Mon Sep 17 00:00:00 2001 From: wangbill Date: Mon, 27 Jul 2026 13:21:17 -0700 Subject: [PATCH 10/21] test: assert the exact `${executionId}:${hex4}` shape for default sub-orch IDs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to 074d419 (which changed the default sub-orchestration instance ID to `${executionId}:${hex4}`). Tighten the regression tests to assert that exact shape with the precise regex `/^[0-9a-f]{32}:[0-9a-f]{4}$/` (a 32-hex executionId — the `Guid.ToString("N")` form the DTS backend mints — plus the 4-hex sequence suffix), instead of the looser two-segment / not-parent-prefixed checks. The regex is a strictly stronger premise check: it also rules out the legacy `${parentId}:${hex4}` fallback (a 36-char parent GUID would fail it), so it catches a silent regression to the fallback path. - sub-orchestration-instance-id.spec.ts: the nesting length test now also asserts every derived ID matches the shape regex. - test/e2e-azuremanaged/orchestration.spec.ts (real DTS emulator): both the continue-as-new collision test and the two-level nesting test now assert the shape regex and `length <= 100`; the nesting test additionally asserts childId.length === grandchildId.length — the direct proof that the ID no longer grows per nesting level (the whole point of dropping the parent-instance-ID prefix). No production code change; assertions only. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d86f5f69-4e1a-4c1d-b017-5e290c85cc05 --- .../sub-orchestration-instance-id.spec.ts | 1 + test/e2e-azuremanaged/orchestration.spec.ts | 35 +++++++++---------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/packages/durabletask-js/test/sub-orchestration-instance-id.spec.ts b/packages/durabletask-js/test/sub-orchestration-instance-id.spec.ts index 3d482c99..e830f4b4 100644 --- a/packages/durabletask-js/test/sub-orchestration-instance-id.spec.ts +++ b/packages/durabletask-js/test/sub-orchestration-instance-id.spec.ts @@ -80,6 +80,7 @@ describe("default sub-orchestration instance ID derivation", () => { } for (const childId of derived) { + expect(childId).toMatch(/^[0-9a-f]{32}:[0-9a-f]{4}$/); expect(childId.length).toBeLessThanOrEqual(100); } // Constant length at every depth: each level is executionId(32) + ":" + suffix(4) = 37 chars, diff --git a/test/e2e-azuremanaged/orchestration.spec.ts b/test/e2e-azuremanaged/orchestration.spec.ts index 95698567..7d25b0eb 100644 --- a/test/e2e-azuremanaged/orchestration.spec.ts +++ b/test/e2e-azuremanaged/orchestration.spec.ts @@ -665,20 +665,17 @@ describe("Durable Task Scheduler (DTS) E2E Tests", () => { expect(output.gen1ChildId).toBeTruthy(); expect(output.gen0ChildId).not.toEqual(output.gen1ChildId); - // Premise check: each child ID is `${executionId}:${hex4}` — keyed on the per-generation - // executionId that the REAL DTS backend minted, NOT the legacy `${parentId}:${hex4}` fallback we - // emit only when executionId is empty. So each ID: - // - is <= 100 chars (the DTS instance-ID limit; the executionId-keyed form is constant-length), - // - has exactly TWO colon-separated segments (executionId + suffix), and - // - does NOT start with the parent instance ID (the legacy fallback would start with `${id}:`; - // the correct form starts with the 32-hex executionId instead). - // If DTS had NOT populated executionId we would fall back to `${id}:${hex4}` and these assertions - // would fail — surfacing a silent production no-op instead of hiding it behind green in-memory - // tests. (gen0 != gen1 is asserted above: the executionId is minted fresh per generation.) + // Premise check: each child ID is the two-segment `${executionId}:${hex4}` shape — a 32-hex + // executionId that the REAL DTS backend minted (Guid.ToString("N")) plus the 4-hex sequence + // suffix — NOT the legacy `${parentId}:${hex4}` fallback we emit only when executionId is empty + // (that would start with the 36-char parent GUID and fail this regex). Each is far within the DTS + // 100-char instance-ID limit. If DTS had NOT populated executionId these assertions would fail, + // surfacing a silent production no-op instead of hiding it behind green in-memory tests. + // (gen0 != gen1 is asserted above: the executionId is minted fresh per generation.) + const executionKeyedId = /^[0-9a-f]{32}:[0-9a-f]{4}$/; for (const childId of [output.gen0ChildId, output.gen1ChildId]) { + expect(childId).toMatch(executionKeyedId); expect(childId.length).toBeLessThanOrEqual(100); - expect(childId.split(":").length).toEqual(2); - expect(childId.startsWith(`${id}:`)).toBe(false); } }, 61000); @@ -720,17 +717,19 @@ describe("Durable Task Scheduler (DTS) E2E Tests", () => { `grandchildIdLen=${output.grandchildId.length}`, ); - // Two levels of default-ID nesting (the depth that broke pre-fix). Every derived ID stays within - // the DTS limit and is the constant-length two-segment `${executionId}:${hex4}` shape — not the - // parent-prefixed shape, so it never grows with depth. + // Two levels of default-ID nesting (the depth that broke pre-fix). Every derived ID is the + // two-segment `${executionId}:${hex4}` shape (32-hex executionId + 4-hex suffix = 37 chars) and + // far within the DTS 100-char limit — not the parent-prefixed shape, so it never grows with depth. + const executionKeyedId = /^[0-9a-f]{32}:[0-9a-f]{4}$/; for (const childId of [output.childId, output.grandchildId]) { expect(childId).toBeTruthy(); + expect(childId).toMatch(executionKeyedId); expect(childId.length).toBeLessThanOrEqual(100); - expect(childId.split(":").length).toEqual(2); - expect(childId.startsWith(`${id}:`)).toBe(false); } - // Each level is keyed on its own fresh executionId, so the two levels' IDs differ. + // Each level is keyed on its own fresh executionId, so the two levels' IDs differ — and, the whole + // point of the fix, are the SAME length (the pre-fix parent-prefixed shape grew ~38 chars/level). expect(output.childId).not.toEqual(output.grandchildId); + expect(output.childId.length).toEqual(output.grandchildId.length); }, 61000); it("should be able to run a single orchestration without activity", async () => { From fb5ad6695755407ec24b743070ac4aa6ccfb034a Mon Sep 17 00:00:00 2001 From: wangbill Date: Mon, 27 Jul 2026 13:42:46 -0700 Subject: [PATCH 11/21] fix(azure-functions-durable): rethrow non-MODULE_NOT_FOUND errors from the @azure/identity require The lazy `require("@azure/identity")` in the callHttp token path caught EVERY failure and reported "install the optional '@azure/identity' package". If the package IS installed but fails for any other reason -- it throws while initializing, a transitive dependency is broken, an ESM/CJS interop problem -- the user was told to install a package they already have and the real error was destroyed, on the hardest path to debug (Managed-Identity token acquisition). Now only a genuinely missing module (`code === "MODULE_NOT_FOUND"`) yields the install hint; any other error is rethrown unchanged. Uses the repo's inline `(e as { code?: unknown }).code` idiom (matching export-history-client.ts) -- no new import. Tests (http-builtin.spec.ts): the existing virtual `@azure/identity` mock now delegates to a swappable factory so a test can make `require` fail with a specific error, reusing the same mechanism rather than a bespoke harness. Added (1) MODULE_NOT_FOUND still yields the install guidance, and (2) a non-MODULE_NOT_FOUND require failure propagates the ORIGINAL error unchanged (asserted by identity via `.rejects.toBe`, not by message). Also (docs): document the residual initial-request redirect gap in the compat README, mirroring the builtin.ts comment -- the first HTTP hop uses fetch's default `redirect: "follow"`, which drops Authorization/Cookie on a cross-origin redirect but NOT custom credential headers such as x-functions-key; closing it (`redirect: "manual"` + per-hop policy) is deliberately deferred, so avoid sending custom credential headers to endpoints that may redirect cross-origin. Both changes address the Copilot PR review on 074d419 (review 4791153015). No public API change; the sub-orchestration instance-ID derivation is untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d86f5f69-4e1a-4c1d-b017-5e290c85cc05 --- packages/azure-functions-durable/README.md | 8 +++ .../src/http/builtin.ts | 9 ++- .../test/unit/http-builtin.spec.ts | 59 ++++++++++++++++--- 3 files changed, 68 insertions(+), 8 deletions(-) diff --git a/packages/azure-functions-durable/README.md b/packages/azure-functions-durable/README.md index 00e86264..499feefc 100644 --- a/packages/azure-functions-durable/README.md +++ b/packages/azure-functions-durable/README.md @@ -63,6 +63,14 @@ changed: still forward headers and the `tokenSource`, so legitimate async patterns keep working. This mirrors the .NET extension's policy ([Azure/azure-functions-durable-extension#3443](https://github.com/Azure/azure-functions-durable-extension/pull/3443)). + - **The initial request follows redirects with `fetch`'s defaults, which do not strip _custom_ + credential headers.** Distinct from the `202` poll loop above, the first HTTP hop uses `fetch`'s + default `redirect: "follow"`. Per the Fetch Standard the implementation drops `Authorization` and + `Cookie` when a redirect crosses origins, but it does **not** drop custom credential headers such as + `x-functions-key`. Switching to `redirect: "manual"` with a per-hop cross-origin policy would close + this residual gap but change observable single-request semantics (hop count, effective URL, cookie + handling), so it is deliberately deferred; until then, avoid sending custom credential headers (e.g. + `x-functions-key`) to endpoints that may redirect cross-origin. - **The built-in poll orchestrator cannot be started directly.** It is registered under a reserved name (`BuiltIn__HttpPollOrchestrator`) and refuses a top-level start (it is only ever a sub-orchestration of `callHttp`), so a dynamic `orchestrators/{name}` starter cannot be abused to diff --git a/packages/azure-functions-durable/src/http/builtin.ts b/packages/azure-functions-durable/src/http/builtin.ts index 067d804c..81c59e51 100644 --- a/packages/azure-functions-durable/src/http/builtin.ts +++ b/packages/azure-functions-durable/src/http/builtin.ts @@ -160,7 +160,14 @@ async function acquireBearerToken(resource: string): Promise { }; try { identity = require("@azure/identity"); - } catch { + } catch (e) { + // Only a genuinely missing module warrants the install hint. Any other failure — the package IS + // installed but throws while initializing, a broken transitive dependency, an ESM/CJS interop + // problem — must surface unchanged: telling the user to install a package they already have would + // destroy the real diagnostic on the hardest path to debug (Managed-Identity token acquisition). + if ((e as { code?: unknown }).code !== "MODULE_NOT_FOUND") { + throw e; + } throw new Error( "callHttp with a tokenSource requires the optional '@azure/identity' package. " + "Install it with `npm install @azure/identity`.", diff --git a/packages/azure-functions-durable/test/unit/http-builtin.spec.ts b/packages/azure-functions-durable/test/unit/http-builtin.spec.ts index bd383348..7844740d 100644 --- a/packages/azure-functions-durable/test/unit/http-builtin.spec.ts +++ b/packages/azure-functions-durable/test/unit/http-builtin.spec.ts @@ -15,13 +15,15 @@ import { DurableHttpRequestPayload, DurableHttpResponse } from "../../src/http/m // not installed in this workspace — a virtual mock stands in so the token-acquisition path can be // exercised and the REAL (mocked) token asserted on the outgoing request. const mockGetToken = jest.fn(async (_scope: string) => ({ token: "REAL_TOKEN_123" })); -jest.mock( - "@azure/identity", - () => ({ - DefaultAzureCredential: jest.fn().mockImplementation(() => ({ getToken: mockGetToken })), - }), - { virtual: true }, -); +// The virtual mock delegates to a swappable factory so an individual test can make +// `require("@azure/identity")` succeed (the default) OR fail with a specific error, reusing the same +// virtual-mock mechanism rather than a bespoke harness. `mock`-prefixed so the hoisted `jest.mock` +// factory may legally close over it. +const mockIdentityModuleDefault = () => ({ + DefaultAzureCredential: jest.fn().mockImplementation(() => ({ getToken: mockGetToken })), +}); +let mockRequireIdentity: () => unknown = mockIdentityModuleDefault; +jest.mock("@azure/identity", () => mockRequireIdentity(), { virtual: true }); /** A minimal fetch Response stand-in (avoids depending on the global `Response` constructor). */ function fakeResponse(status: number, headers: { [key: string]: string }, body: string, url = "") { @@ -237,6 +239,49 @@ describe("builtinHttpActivity", () => { expect(sentHeaders["Authorization"]).toBe("Bearer REAL_TOKEN_123"); expect(sentHeaders["authorization"]).toBeUndefined(); }); + + describe("@azure/identity loading failures", () => { + afterEach(() => { + // Restore the default (successful) require so later suites are unaffected. + mockRequireIdentity = mockIdentityModuleDefault; + }); + + // Freshly load the activity so its lazy `require("@azure/identity")` re-invokes the swapped mock + // (Jest caches a module after the first successful require; resetModules busts that cache). + function loadActivityFresh(): typeof builtinHttpActivity { + jest.resetModules(); + return require("../../src/http/builtin").builtinHttpActivity; + } + + const tokenRequest: DurableHttpRequestPayload = { + method: "GET", + uri: "https://example.test/secure", + tokenSource: { resource: "https://graph.microsoft.com/" }, + }; + + it("throws actionable install guidance when @azure/identity is not installed (MODULE_NOT_FOUND)", async () => { + mockRequireIdentity = () => { + const err = new Error("Cannot find module '@azure/identity'") as Error & { code?: string }; + err.code = "MODULE_NOT_FOUND"; + throw err; + }; + + await expect(loadActivityFresh()(tokenRequest)).rejects.toThrow( + "callHttp with a tokenSource requires the optional '@azure/identity' package.", + ); + }); + + it("propagates a non-MODULE_NOT_FOUND require failure unchanged instead of mislabeling it as missing", async () => { + const initFailure = new Error("boom while initializing @azure/identity"); + mockRequireIdentity = () => { + throw initFailure; + }; + + // The ORIGINAL error surfaces (same instance) — NOT the install-guidance message, which would + // wrongly tell the user to install a package they already have. + await expect(loadActivityFresh()(tokenRequest)).rejects.toBe(initFailure); + }); + }); }); /** Drives the poll orchestrator generator with a fake core context, feeding activity/timer results. */ From b8132e8c0e11341f3ce24fe18a41ee7c4974e7da Mon Sep 17 00:00:00 2001 From: wangbill Date: Mon, 27 Jul 2026 14:08:07 -0700 Subject: [PATCH 12/21] fix(azure-functions-durable): guard malformed poll Location and make @azure/identity an optional peer Two Copilot PR-review fixes for callHttp: 1. builtinHttpPollOrchestrator: a remote-controlled 202 `Location` that `new URL` cannot parse (e.g. `http://` or `///`) threw `TypeError [ERR_INVALID_URL]` and failed the whole orchestration with an opaque error. Treat an unparseable `Location` exactly like a missing one: break the poll loop and return the 202 for the caller to inspect. `new URL` is pure computation over history-derived values, so replay re-takes the identical branch (deterministic). 2. package.json: `@azure/identity` was declared under `optionalDependencies`, which npm still installs by default (only *build* failures are tolerated) - forcing the full MSAL tree onto every compat-package consumer that never calls callHttp with a tokenSource. Move it to an optional `peerDependenciesMeta` entry, which npm does NOT auto-install, matching the lazy-require + actionable-install-error runtime contract. The workspace lockfile subtree is unchanged because the sibling azuremanaged package still hard-depends on @azure/identity; the lockfile diff is limited to the compat node's dependency fields. Tests: two poll-orchestrator cases (`http://`, `///`) assert no throw, the 202 returned unchanged, and no further timer/activity scheduled. Compat unit 143 -> 145. Also corrects the spec's stale "@azure/identity is not installed" comment (it stays installed via the sibling azuremanaged package). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d86f5f69-4e1a-4c1d-b017-5e290c85cc05 --- package-lock.json | 7 ++++- packages/azure-functions-durable/package.json | 7 ++++- .../src/http/builtin.ts | 14 ++++++++- .../test/unit/http-builtin.spec.ts | 29 +++++++++++++++++-- 4 files changed, 51 insertions(+), 6 deletions(-) diff --git a/package-lock.json b/package-lock.json index 5e3a8f42..7a192f15 100644 --- a/package-lock.json +++ b/package-lock.json @@ -7589,8 +7589,13 @@ "engines": { "node": ">=22.0.0" }, - "optionalDependencies": { + "peerDependencies": { "@azure/identity": "^4.0.0" + }, + "peerDependenciesMeta": { + "@azure/identity": { + "optional": true + } } }, "packages/azure-functions-durable/node_modules/@types/node": { diff --git a/packages/azure-functions-durable/package.json b/packages/azure-functions-durable/package.json index 70d16811..5a4206a5 100644 --- a/packages/azure-functions-durable/package.json +++ b/packages/azure-functions-durable/package.json @@ -52,9 +52,14 @@ "@grpc/grpc-js": "^1.14.4", "@microsoft/durabletask-js": "0.4.0" }, - "optionalDependencies": { + "peerDependencies": { "@azure/identity": "^4.0.0" }, + "peerDependenciesMeta": { + "@azure/identity": { + "optional": true + } + }, "devDependencies": { "@types/jest": "^29.5.1", "@types/node": "^22.0.0", diff --git a/packages/azure-functions-durable/src/http/builtin.ts b/packages/azure-functions-durable/src/http/builtin.ts index 81c59e51..1ebedcc0 100644 --- a/packages/azure-functions-durable/src/http/builtin.ts +++ b/packages/azure-functions-durable/src/http/builtin.ts @@ -370,7 +370,19 @@ export async function* builtinHttpPollOrchestrator( // A `Location` may be relative (e.g. `/operations/42`); resolve it against the effective request // URI so the next poll targets an absolute http(s) URL (the activity rejects non-absolute URIs). - const resolved = new URL(location, currentUri).toString(); + // + // `Location` is remote-controlled: an absent one already means "cannot poll, return the 202 as-is" + // (see the `if (!location) break` above), and an unparseable one is the same situation. `new URL` + // throws `TypeError [ERR_INVALID_URL]` on input like `http://` or `///`, which would otherwise fail + // the whole orchestration with an opaque error instead of surfacing the 202 the caller can inspect. + // Determinism holds: `new URL` is pure computation over history-derived values, so replay re-takes + // the identical branch. + let resolved: string; + try { + resolved = new URL(location, currentUri).toString(); + } catch { + break; + } const now = ctx.currentUtcDateTime; const delaySeconds = retryAfterSeconds(headers, now); diff --git a/packages/azure-functions-durable/test/unit/http-builtin.spec.ts b/packages/azure-functions-durable/test/unit/http-builtin.spec.ts index 7844740d..ba60551a 100644 --- a/packages/azure-functions-durable/test/unit/http-builtin.spec.ts +++ b/packages/azure-functions-durable/test/unit/http-builtin.spec.ts @@ -11,9 +11,11 @@ import { } from "../../src/http/builtin"; import { DurableHttpRequestPayload, DurableHttpResponse } from "../../src/http/models"; -// `@azure/identity` is an OPTIONAL dependency loaded lazily via `require` inside the activity, and is -// not installed in this workspace — a virtual mock stands in so the token-acquisition path can be -// exercised and the REAL (mocked) token asserted on the outgoing request. +// `@azure/identity` is an OPTIONAL peer dependency loaded lazily via `require` inside the activity. +// A `{ virtual: true }` mock stands in so the token-acquisition path can be exercised and the REAL +// (mocked) token asserted on the outgoing request — independent of whether the real package happens +// to be resolvable in the workspace (it is today, only because the sibling azuremanaged package +// depends on it; a standalone consumer of this compat package would not have it installed). const mockGetToken = jest.fn(async (_scope: string) => ({ token: "REAL_TOKEN_123" })); // The virtual mock delegates to a swappable factory so an individual test can make // `require("@azure/identity")` succeed (the default) OR fail with a specific error, reusing the same @@ -592,4 +594,25 @@ describe("builtinHttpPollOrchestrator", () => { expect((result.value as DurableHttpResponse).statusCode).toBe(202); expect(raw.createTimer).not.toHaveBeenCalled(); }); + + // A malformed, remote-controlled `Location` (e.g. `http://` or `///`) makes `new URL` throw + // `TypeError [ERR_INVALID_URL]`. That must be treated exactly like a missing `Location`: return the + // 202 for the caller to inspect, never fail the orchestration with an opaque error. + it.each(["http://", "///"])( + "stops polling and returns the 202 as-is when the Location %j is unparseable", + async (badLocation) => { + const { ctx, raw } = createPollContext(now); + const gen = builtinHttpPollOrchestrator(ctx, { method: "GET", uri: "https://host/api", enablePolling: true }); + + await gen.next(); + const response = { statusCode: 202, headers: { Location: badLocation }, content: "pending" }; + const result = await gen.next(response); + + // (a) does NOT throw, (b) returns the 202 unchanged, (c) schedules NO further timer/activity. + expect(result.done).toBe(true); + expect(result.value).toEqual({ statusCode: 202, headers: { Location: badLocation }, content: "pending" }); + expect(raw.createTimer).not.toHaveBeenCalled(); + expect(raw.callActivity).toHaveBeenCalledTimes(1); + }, + ); }); From 6f9ced933219631e08ed0d15e256028a5edb307b Mon Sep 17 00:00:00 2001 From: wangbill Date: Mon, 27 Jul 2026 14:35:37 -0700 Subject: [PATCH 13/21] fix(azure-functions-durable): cache identity credential + harden test-app poll URL Two Copilot PR-review fixes on PR #333 (comments 3659318818, 3659318771). builtin.ts -- DefaultAzureCredential was constructed inside acquireBearerToken on every activity invocation, i.e. every 202 poll hop. That re-probes the environment and discards the credential's in-memory token cache, so under Managed Identity each hop forces a fresh, rate-limited IMDS token call. Cache the credential at module scope (getCredential) and share it across acquisitions; `resource` is supplied per call to getToken, so a single instance serves every resource. Only a successful construction is cached, so a require/constructor failure caches nothing and the next call retries with the correct actionable error (the MODULE_NOT_FOUND install-hint discrimination is preserved). Add __resetCredentialCacheForTests plus a beforeEach that clears the cache between tests, and a test asserting the constructor runs once across two token acquisitions. CallHttpOrchestration.ts (test-app) -- the cross-origin poll Location was built by string interpolation on `${u.port}`, which emits an invalid empty `localhost:` on a protocol default port (80/443). Build the target with URL instead: it omits the port when it is the scheme default and preserves it otherwise, so the hazard disappears while the deliberate 127.0.0.1 -> localhost origin flip is unchanged. Latent today only because func binds a random high port. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d86f5f69-4e1a-4c1d-b017-5e290c85cc05 --- .../src/http/builtin.ts | 48 +++++++++++++++---- .../test/unit/http-builtin.spec.ts | 25 +++++++++- .../src/functions/CallHttpOrchestration.ts | 11 ++++- 3 files changed, 72 insertions(+), 12 deletions(-) diff --git a/packages/azure-functions-durable/src/http/builtin.ts b/packages/azure-functions-durable/src/http/builtin.ts index 1ebedcc0..be53405d 100644 --- a/packages/azure-functions-durable/src/http/builtin.ts +++ b/packages/azure-functions-durable/src/http/builtin.ts @@ -145,19 +145,32 @@ function toDefaultScope(resource: string): string { return /\/\.default$/i.test(trimmed) ? trimmed : `${trimmed}/.default`; } +/** A credential able to mint bearer tokens — the subset of `@azure/identity`'s `TokenCredential` used here. */ +type BearerCredential = { getToken(scope: string): Promise<{ token: string } | null> }; + +/** Lazily constructed credential, cached at module scope and shared across every token acquisition. */ +let cachedCredential: BearerCredential | undefined; + /** - * Acquire an AAD bearer token for `resource` via the optional `@azure/identity` package. + * Resolve the shared bearer credential, loading `@azure/identity` and constructing it on first use. * * @remarks * Loaded lazily with `require` (mirroring the core SDK's optional-peer-dependency pattern) so the - * dependency is only touched when a token source is actually used; `require` also keeps the module - * out of the compiled type graph, so an app that never uses a token source needs no `@azure/identity` - * install. Throws a clear, actionable error when the package is missing but a token source was used. + * dependency is only touched when a token source is actually used; `require` also keeps the module out + * of the compiled type graph, so an app that never uses a token source needs no `@azure/identity` + * install. The credential is cached because `DefaultAzureCredential` probes the environment on + * construction and keeps its own in-memory token cache: constructing a fresh one per activity + * invocation would defeat that cache — under Managed Identity every 202 poll hop would then force a + * fresh, **rate-limited** IMDS token call. `resource` is passed per call to `getToken`, not to the + * constructor, so a single instance correctly serves every resource. Only a *successful* construction + * is cached: if `require` or the constructor throws, nothing is cached, so the next call retries and + * still produces the correct actionable error when the package is missing but a token source was used. */ -async function acquireBearerToken(resource: string): Promise { - let identity: { - DefaultAzureCredential: new () => { getToken(scope: string): Promise<{ token: string } | null> }; - }; +function getCredential(): BearerCredential { + if (cachedCredential) { + return cachedCredential; + } + let identity: { DefaultAzureCredential: new () => BearerCredential }; try { identity = require("@azure/identity"); } catch (e) { @@ -173,7 +186,24 @@ async function acquireBearerToken(resource: string): Promise { "Install it with `npm install @azure/identity`.", ); } - const credential = new identity.DefaultAzureCredential(); + // Assigned only after a successful construction (a throwing constructor never reaches this + // assignment), so a failed attempt leaves the cache empty for the next call to retry. + cachedCredential = new identity.DefaultAzureCredential(); + return cachedCredential; +} + +/** + * Test-only hook: drop the cached credential so a re-swapped virtual `@azure/identity` mock (or a + * fresh-construction assertion) is honored rather than served from a prior test's cache. + * @internal + */ +export function __resetCredentialCacheForTests(): void { + cachedCredential = undefined; +} + +/** Acquire an AAD bearer token for `resource` from the shared, lazily constructed credential. */ +async function acquireBearerToken(resource: string): Promise { + const credential = getCredential(); const result = await credential.getToken(toDefaultScope(resource)); const token = result?.token; if (!token) { diff --git a/packages/azure-functions-durable/test/unit/http-builtin.spec.ts b/packages/azure-functions-durable/test/unit/http-builtin.spec.ts index ba60551a..27c2e74e 100644 --- a/packages/azure-functions-durable/test/unit/http-builtin.spec.ts +++ b/packages/azure-functions-durable/test/unit/http-builtin.spec.ts @@ -8,6 +8,7 @@ import { builtinHttpPollOrchestrator, isSameOrigin, retryAfterSeconds, + __resetCredentialCacheForTests, } from "../../src/http/builtin"; import { DurableHttpRequestPayload, DurableHttpResponse } from "../../src/http/models"; @@ -120,6 +121,12 @@ describe("builtinHttpActivity", () => { jest.clearAllMocks(); }); + // The credential is cached at module scope; clear it between tests so a swapped virtual mock (or a + // construction-count assertion) is never served a prior test's cached instance. + beforeEach(() => { + __resetCredentialCacheForTests(); + }); + it("performs the request and passes the 200 response through", async () => { const fetchMock = makeFetchMock(fakeResponse(200, { "content-type": "text/plain" }, "hello")); global.fetch = fetchMock as unknown as typeof fetch; @@ -242,7 +249,7 @@ describe("builtinHttpActivity", () => { expect(sentHeaders["authorization"]).toBeUndefined(); }); - describe("@azure/identity loading failures", () => { + describe("@azure/identity lazy loading and caching", () => { afterEach(() => { // Restore the default (successful) require so later suites are unaffected. mockRequireIdentity = mockIdentityModuleDefault; @@ -283,6 +290,22 @@ describe("builtinHttpActivity", () => { // wrongly tell the user to install a package they already have. await expect(loadActivityFresh()(tokenRequest)).rejects.toBe(initFailure); }); + + it("constructs DefaultAzureCredential once and reuses it across token acquisitions", async () => { + const credentialCtor = jest.fn().mockImplementation(() => ({ getToken: mockGetToken })); + mockRequireIdentity = () => ({ DefaultAzureCredential: credentialCtor }); + const fetchMock = makeFetchMock(fakeResponse(200, {}, "ok")); + global.fetch = fetchMock as unknown as typeof fetch; + + const activity = loadActivityFresh(); + await activity(tokenRequest); + await activity(tokenRequest); + + // The credential is cached at module scope, so the environment-probing constructor runs once + // even though two hops acquired a token — a long 202 poll loop cannot re-probe (rate-limited) + // IMDS on every hop. + expect(credentialCtor).toHaveBeenCalledTimes(1); + }); }); }); diff --git a/test/e2e-functions/test-app/src/functions/CallHttpOrchestration.ts b/test/e2e-functions/test-app/src/functions/CallHttpOrchestration.ts index 3f8b1b5a..b67bb179 100644 --- a/test/e2e-functions/test-app/src/functions/CallHttpOrchestration.ts +++ b/test/e2e-functions/test-app/src/functions/CallHttpOrchestration.ts @@ -85,8 +85,15 @@ const HttpCrossOriginStart: HttpHandler = async ( request: HttpRequest, _context: InvocationContext, ): Promise => { - const u = new URL(request.url); - const location = `${u.protocol}//localhost:${u.port}/api/HttpAuthEcho`; + // Build the poll target by construction rather than string interpolation: a template literal on + // `${u.port}` emits an invalid empty `localhost:` whenever the incoming request used a protocol + // default port (80/443, where `URL.port` is ""). `URL` omits the port when it is the scheme default + // and preserves it otherwise, so the hazard disappears while the origin still flips to `localhost`. + const target = new URL(request.url); + target.hostname = 'localhost'; + target.pathname = '/api/HttpAuthEcho'; + target.search = ''; + const location = target.toString(); return { status: 202, headers: { Location: location, 'Retry-After': '1' }, From 29ddc34eee058945f529b967943c22f0cf4b24f6 Mon Sep 17 00:00:00 2001 From: wangbill Date: Mon, 27 Jul 2026 14:47:08 -0700 Subject: [PATCH 14/21] docs(azure-functions-durable): sharpen credential-cache rationale in builtin.ts Follow-up to 6f9ced9 (credential caching). Comment-only change to the `getCredential()` @remarks block; no code or behavior change. The prior comment justified caching mainly via "rate-limited IMDS call per poll hop", which overstates the risk for a single orchestration (a lone 30s-interval poll loop cannot itself throttle). Reframe around the documented Azure Identity guidance instead: - "Reuse credential instances" is the official best practice. Reuse lets the underlying MSAL dependency serve tokens from its in-memory cache; a fresh `DefaultAzureCredential` per activity invocation discards that per-instance cache and issues a new token request on every 202 poll hop. - The real failure mode is aggregate: across the many concurrent orchestrations one worker serves, that token traffic risks HTTP 429 throttling from Microsoft Entra ID (app outages). The per-VM IMDS limits (20 req/s, 5 concurrent under Managed Identity) are noted only as a secondary constraint. - Records that `DefaultAzureCredential` is kept deliberately (not narrowed to `ManagedIdentityCredential`) to preserve the local-development fallback chain. No functional change: the cached-once behavior and its tests already landed in 6f9ced9. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d86f5f69-4e1a-4c1d-b017-5e290c85cc05 --- .../src/http/builtin.ts | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/packages/azure-functions-durable/src/http/builtin.ts b/packages/azure-functions-durable/src/http/builtin.ts index be53405d..586e14fa 100644 --- a/packages/azure-functions-durable/src/http/builtin.ts +++ b/packages/azure-functions-durable/src/http/builtin.ts @@ -158,13 +158,19 @@ let cachedCredential: BearerCredential | undefined; * Loaded lazily with `require` (mirroring the core SDK's optional-peer-dependency pattern) so the * dependency is only touched when a token source is actually used; `require` also keeps the module out * of the compiled type graph, so an app that never uses a token source needs no `@azure/identity` - * install. The credential is cached because `DefaultAzureCredential` probes the environment on - * construction and keeps its own in-memory token cache: constructing a fresh one per activity - * invocation would defeat that cache — under Managed Identity every 202 poll hop would then force a - * fresh, **rate-limited** IMDS token call. `resource` is passed per call to `getToken`, not to the - * constructor, so a single instance correctly serves every resource. Only a *successful* construction - * is cached: if `require` or the constructor throws, nothing is cached, so the next call retries and - * still produces the correct actionable error when the package is missing but a token source was used. + * install. The credential is cached to reuse a single instance across invocations — the documented + * Azure Identity best practice ("Reuse credential instances"): reuse lets the underlying MSAL + * dependency serve tokens from its in-memory cache, whereas constructing a fresh + * `DefaultAzureCredential` per activity invocation discards that per-instance cache and issues a new + * token request on every 202 poll hop. Aggregated across the many concurrent orchestrations one worker + * serves, that token traffic risks HTTP 429 throttling from Microsoft Entra ID (and, secondarily, the + * per-VM IMDS limits of 20 req/s and 5 concurrent under Managed Identity); a single 30s-interval poll + * loop cannot itself throttle. `resource` is passed per call to `getToken`, not to the constructor, so + * one instance correctly serves every resource; `DefaultAzureCredential` is kept deliberately (not + * narrowed to `ManagedIdentityCredential`) to preserve the local-development fallback chain. Only a + * *successful* construction is cached: if `require` or the constructor throws, nothing is cached, so + * the next call retries and still produces the correct actionable error when the package is missing + * but a token source was used. */ function getCredential(): BearerCredential { if (cachedCredential) { From e1c6226f2f801f91d1e62ec4a72e76217c609f79 Mon Sep 17 00:00:00 2001 From: wangbill Date: Mon, 27 Jul 2026 14:52:48 -0700 Subject: [PATCH 15/21] docs(azure-functions-durable): align credential-cache test comment with corrected rationale Follow-up to 29ddc34, which sharpened the credential-cache rationale in builtin.ts but left the parallel explanatory comment in the "constructs once" test carrying the old "rate-limited IMDS on every hop" framing. Reframe the test comment to match: the constructor runs once because the credential is cached at module scope, so the underlying MSAL in-memory token cache is reused across invocations instead of discarded per hop (the documented "reuse credential instances" best practice; reuse avoids Entra 429 throttling at aggregate scale). Adds the explicit "a single poll loop cannot itself throttle" caveat. Comment-only; the assertion and behavior are unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d86f5f69-4e1a-4c1d-b017-5e290c85cc05 --- .../azure-functions-durable/test/unit/http-builtin.spec.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/azure-functions-durable/test/unit/http-builtin.spec.ts b/packages/azure-functions-durable/test/unit/http-builtin.spec.ts index 27c2e74e..125a28a7 100644 --- a/packages/azure-functions-durable/test/unit/http-builtin.spec.ts +++ b/packages/azure-functions-durable/test/unit/http-builtin.spec.ts @@ -302,8 +302,9 @@ describe("builtinHttpActivity", () => { await activity(tokenRequest); // The credential is cached at module scope, so the environment-probing constructor runs once - // even though two hops acquired a token — a long 202 poll loop cannot re-probe (rate-limited) - // IMDS on every hop. + // and the underlying MSAL in-memory token cache is reused across invocations instead of being + // discarded and re-created per hop (the documented "reuse credential instances" best practice; + // reuse avoids Entra 429 throttling at aggregate scale). A single poll loop cannot itself throttle. expect(credentialCtor).toHaveBeenCalledTimes(1); }); }); From 4d2709c410902deacc0700251d8cbe2303fcd835 Mon Sep 17 00:00:00 2001 From: wangbill Date: Mon, 27 Jul 2026 15:00:19 -0700 Subject: [PATCH 16/21] fix(azure-functions-durable): stop polling on a non-http(s) 202 Location MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comment 3661095099 (builtin.ts, anchored to 6f9ced9) — the direct completion of the malformed-Location guard shipped in b8132e8. The 202 poll loop resolved any parseable `Location` and re-polled it. A callee-controlled `Location` with a non-http(s) scheme (e.g. `file:///etc/passwd`, `ftp://…`) parses cleanly through `new URL`, so the existing try/catch did NOT fire; execution reached `callActivity`, whose scheme guard then threw `callHttp only supports http/https URLs` and FAILED the whole orchestration instead of returning the first 202 for the caller to inspect. This is a robustness bug, not an exfiltration hole: the activity's scheme guard still blocks the `file://` fetch, so nothing is read. But a remote endpoint could kill the caller's orchestration with an opaque error. Fix it by treating a non-http(s) scheme exactly like a missing (`if (!location) break`) or unparseable Location: stop polling, return the 202 as-is. `URL.protocol` is WHATWG-normalized lowercase, so `FILE://`/`Ftp://` are covered; determinism holds since the check is pure computation over history-derived values. Tests (RED on the prior code, GREEN now): mirror the malformed-Location cases — `it.each(["file:///etc/passwd", "ftp://example.com/x"])` asserts the poll orchestrator does not throw, returns the first 202 unchanged, and schedules no further timer/activity (createTimer not called, callActivity called once). Compat unit 146 -> 148. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d86f5f69-4e1a-4c1d-b017-5e290c85cc05 --- .../src/http/builtin.ts | 19 +++++++++++----- .../test/unit/http-builtin.spec.ts | 22 +++++++++++++++++++ 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/packages/azure-functions-durable/src/http/builtin.ts b/packages/azure-functions-durable/src/http/builtin.ts index 586e14fa..d847ad1f 100644 --- a/packages/azure-functions-durable/src/http/builtin.ts +++ b/packages/azure-functions-durable/src/http/builtin.ts @@ -408,14 +408,21 @@ export async function* builtinHttpPollOrchestrator( // URI so the next poll targets an absolute http(s) URL (the activity rejects non-absolute URIs). // // `Location` is remote-controlled: an absent one already means "cannot poll, return the 202 as-is" - // (see the `if (!location) break` above), and an unparseable one is the same situation. `new URL` - // throws `TypeError [ERR_INVALID_URL]` on input like `http://` or `///`, which would otherwise fail - // the whole orchestration with an opaque error instead of surfacing the 202 the caller can inspect. - // Determinism holds: `new URL` is pure computation over history-derived values, so replay re-takes - // the identical branch. + // (see the `if (!location) break` above), and both an UNPARSEABLE and a parseable-but-non-http(s) + // one are the same situation. `new URL` throws `TypeError [ERR_INVALID_URL]` on input like `http://` + // or `///`; and it PARSES a `file://`/`ftp://` Location cleanly, yet the activity's scheme guard + // then rejects it — either way, continuing would fail the whole orchestration with an opaque error + // instead of surfacing the 202 the caller can inspect. So a non-http(s) scheme stops polling too. + // Determinism holds: `new URL` and the protocol check are pure computation over history-derived + // values, so replay re-takes the identical branch. let resolved: string; try { - resolved = new URL(location, currentUri).toString(); + const url = new URL(location, currentUri); + // `URL.protocol` is WHATWG-normalized to lowercase, so `FILE://`/`Ftp://` are covered too. + if (url.protocol !== "http:" && url.protocol !== "https:") { + break; + } + resolved = url.toString(); } catch { break; } diff --git a/packages/azure-functions-durable/test/unit/http-builtin.spec.ts b/packages/azure-functions-durable/test/unit/http-builtin.spec.ts index 125a28a7..0ab535ac 100644 --- a/packages/azure-functions-durable/test/unit/http-builtin.spec.ts +++ b/packages/azure-functions-durable/test/unit/http-builtin.spec.ts @@ -639,4 +639,26 @@ describe("builtinHttpPollOrchestrator", () => { expect(raw.callActivity).toHaveBeenCalledTimes(1); }, ); + + // A parseable but non-http(s) `Location` (e.g. `file://`, `ftp://`) is just as unpollable as a + // missing or unparseable one: the activity's scheme guard rejects non-http(s) URIs, so continuing + // to poll would fail the orchestration with an opaque error instead of surfacing the 202. Since + // `Location` is callee-controlled, treat it the same way — stop polling, return the 202 as-is. + it.each(["file:///etc/passwd", "ftp://example.com/x"])( + "stops polling and returns the 202 as-is when the Location %j has a non-http(s) scheme", + async (badLocation) => { + const { ctx, raw } = createPollContext(now); + const gen = builtinHttpPollOrchestrator(ctx, { method: "GET", uri: "https://host/api", enablePolling: true }); + + await gen.next(); + const response = { statusCode: 202, headers: { Location: badLocation }, content: "pending" }; + const result = await gen.next(response); + + // (a) does NOT throw, (b) returns the 202 unchanged, (c) schedules NO further timer/activity. + expect(result.done).toBe(true); + expect(result.value).toEqual({ statusCode: 202, headers: { Location: badLocation }, content: "pending" }); + expect(raw.createTimer).not.toHaveBeenCalled(); + expect(raw.callActivity).toHaveBeenCalledTimes(1); + }, + ); }); From c79b77abe79eceacb95fff15c2654e01289d7e3e Mon Sep 17 00:00:00 2001 From: wangbill Date: Mon, 27 Jul 2026 15:15:56 -0700 Subject: [PATCH 17/21] Align callHttp non-http(s) Location fix to reviewer spec Round 11 (commit 4d2709c) already fixed the non-http(s) `Location` poll robustness bug for PR comment 3661095099; that commit crossed in flight with the reviewer's now-explicit spec. Align the two remaining deltas without changing behavior: - builtin.ts poll loop: narrow the `try` to wrap only `new URL(...)`, moving the http/https `protocol` check to after the catch. This is clearer about what can throw: only URL construction can, while the `URL.protocol` getter and `toString()` cannot. Consolidate the rationale into a single comment block covering both unusable cases (unparseable and non-http(s)). Behavior is identical: a parseable but non-http(s) Location still breaks the poll loop and returns the 202 for the caller to inspect. - http-builtin.spec.ts: add a NOTE to the non-http(s) test making explicit that `ctx.callActivity` is mocked, so the test does NOT reproduce the production orchestration failure (which originates from the activity's scheme guard that the mock bypasses). It asserts the unit-level cause instead: the poll loop breaks -- createTimer never called, callActivity called exactly once. No behavior change. compat unit 148, core unit 1170, lint/build clean. Derivation, CHANGELOG, and lockfile diffs remain EMPTY. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d86f5f69-4e1a-4c1d-b017-5e290c85cc05 --- .../src/http/builtin.ts | 30 +++++++++---------- .../test/unit/http-builtin.spec.ts | 5 ++++ 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/packages/azure-functions-durable/src/http/builtin.ts b/packages/azure-functions-durable/src/http/builtin.ts index d847ad1f..b13b2294 100644 --- a/packages/azure-functions-durable/src/http/builtin.ts +++ b/packages/azure-functions-durable/src/http/builtin.ts @@ -407,25 +407,25 @@ export async function* builtinHttpPollOrchestrator( // A `Location` may be relative (e.g. `/operations/42`); resolve it against the effective request // URI so the next poll targets an absolute http(s) URL (the activity rejects non-absolute URIs). // - // `Location` is remote-controlled: an absent one already means "cannot poll, return the 202 as-is" - // (see the `if (!location) break` above), and both an UNPARSEABLE and a parseable-but-non-http(s) - // one are the same situation. `new URL` throws `TypeError [ERR_INVALID_URL]` on input like `http://` - // or `///`; and it PARSES a `file://`/`ftp://` Location cleanly, yet the activity's scheme guard - // then rejects it — either way, continuing would fail the whole orchestration with an opaque error - // instead of surfacing the 202 the caller can inspect. So a non-http(s) scheme stops polling too. - // Determinism holds: `new URL` and the protocol check are pure computation over history-derived - // values, so replay re-takes the identical branch. - let resolved: string; + // `Location` is remote-controlled, so an unusable one is treated exactly like a missing one + // (`if (!location) break` above): stop polling and return the 202 for the caller to inspect, + // never fail the orchestration with an opaque error. Two unusable cases: + // 1. UNPARSEABLE — `new URL` throws `TypeError [ERR_INVALID_URL]` on input like `http://` or `///`. + // 2. NON-http(s) — `new URL` parses a `file://`/`ftp://` Location cleanly, but the activity's + // scheme guard would then reject it, failing the whole orchestration. + // `URL.protocol` is WHATWG-normalized to lowercase (with a trailing `:`), so `FILE://`/`Ftp://` + // are covered without extra casing. Determinism holds: `new URL` and the protocol check are pure + // computation over history-derived values, so replay re-takes the identical branch. + let resolvedUrl: URL; try { - const url = new URL(location, currentUri); - // `URL.protocol` is WHATWG-normalized to lowercase, so `FILE://`/`Ftp://` are covered too. - if (url.protocol !== "http:" && url.protocol !== "https:") { - break; - } - resolved = url.toString(); + resolvedUrl = new URL(location, currentUri); } catch { break; } + if (resolvedUrl.protocol !== "http:" && resolvedUrl.protocol !== "https:") { + break; + } + const resolved = resolvedUrl.toString(); const now = ctx.currentUtcDateTime; const delaySeconds = retryAfterSeconds(headers, now); diff --git a/packages/azure-functions-durable/test/unit/http-builtin.spec.ts b/packages/azure-functions-durable/test/unit/http-builtin.spec.ts index 0ab535ac..9cd10ae2 100644 --- a/packages/azure-functions-durable/test/unit/http-builtin.spec.ts +++ b/packages/azure-functions-durable/test/unit/http-builtin.spec.ts @@ -644,6 +644,11 @@ describe("builtinHttpPollOrchestrator", () => { // missing or unparseable one: the activity's scheme guard rejects non-http(s) URIs, so continuing // to poll would fail the orchestration with an opaque error instead of surfacing the 202. Since // `Location` is callee-controlled, treat it the same way — stop polling, return the 202 as-is. + // + // NOTE: `ctx.callActivity` is MOCKED here, so this test does NOT reproduce that production + // orchestration failure (the real failure comes from the activity's scheme guard, which the mock + // bypasses). It asserts the unit-level CAUSE instead: the poll loop breaks — `createTimer` is never + // called and `callActivity` runs exactly once — rather than scheduling a doomed poll to `file://`. it.each(["file:///etc/passwd", "ftp://example.com/x"])( "stops polling and returns the 202 as-is when the Location %j has a non-http(s) scheme", async (badLocation) => { From 71e020515e3ddc57d58a807f6d92f6a7f066ad85 Mon Sep 17 00:00:00 2001 From: wangbill Date: Mon, 27 Jul 2026 15:29:03 -0700 Subject: [PATCH 18/21] Fix test-fixture typo (drop circular PR ref) and second u.port invalid-URL bug Two append-only test-fixture corrections, both raised by the PR reviewer bot against c79b77a: 1. test/e2e-azuremanaged/orchestration.spec.ts (comment 3661191997) -- comment only. Fix the "continue-as-news" -> "continues-as-new" typo and drop the circular self-referential "PR #333" from two comments, keeping the durable "issue #318" reference (git blame/merge metadata already record the PR). No it(...)/assertion/executionKeyedId regex touched. 2. test/e2e-functions/test-app/src/functions/CallHttpOrchestration.ts (comment 3661309482) -- a SECOND occurrence of the u.port invalid-URL bug already fixed at HttpCrossOriginStart (comment 3659318771, commit 6f9ced9). The ${u.protocol}//${host}:${u.port}/... template emits an invalid dangling ":" (e.g. https://127.0.0.1:/api/...) whenever the host binds a protocol-default port (80/443, where URL.port is ""). Rebuild the URL via new URL + hostname/ pathname/search mutation, which preserves the host's actual port and omits only scheme-default ports -- mirroring the HttpCrossOriginStart fix. Latent today (the Functions host binds 7071) but a real correctness bug in a fixture whose entire purpose is URL/origin handling; the line-89 fix had missed it. lint clean; test-app tsc --noEmit clean. Derivation, CHANGELOG, and lockfile diffs remain EMPTY. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d86f5f69-4e1a-4c1d-b017-5e290c85cc05 --- test/e2e-azuremanaged/orchestration.spec.ts | 6 +++--- .../test-app/src/functions/CallHttpOrchestration.ts | 12 +++++++++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/test/e2e-azuremanaged/orchestration.spec.ts b/test/e2e-azuremanaged/orchestration.spec.ts index 7d25b0eb..58ae8ede 100644 --- a/test/e2e-azuremanaged/orchestration.spec.ts +++ b/test/e2e-azuremanaged/orchestration.spec.ts @@ -591,8 +591,8 @@ describe("Durable Task Scheduler (DTS) E2E Tests", () => { }, 31000); // Regression for the default-derived sub-orchestration instance ID colliding across - // continue-as-new generations (issue #318 / PR #333). A parent that schedules a DEFAULT-ID - // (no explicit instanceId) sub-orchestration, then continue-as-news, then schedules another + // continue-as-new generations (issue #318). A parent that schedules a DEFAULT-ID + // (no explicit instanceId) sub-orchestration, then continues-as-new, then schedules another // default-ID sub-orchestration must NOT collide. Pre-fix the derived child ID was // `${parentId}:${hex4}` — identical in every generation because continue-as-new truncates the // history and resets the per-work-item sequence counter — so generation 1 re-derived @@ -679,7 +679,7 @@ describe("Durable Task Scheduler (DTS) E2E Tests", () => { } }, 61000); - // Companion length check for the same fix (PR #333): default-derived sub-orchestration instance IDs + // Companion length check for the same fix: default-derived sub-orchestration instance IDs // must stay within the DTS 100-character instance-ID limit even when sub-orchestrations NEST. The // pre-fix `${parentId}:${executionId}:${hex4}` shape concatenated every ancestor, so a top-level // orchestrator -> sub-orchestrator -> callHttp (2 levels) produced a ~112-char ID that DTS rejects. diff --git a/test/e2e-functions/test-app/src/functions/CallHttpOrchestration.ts b/test/e2e-functions/test-app/src/functions/CallHttpOrchestration.ts index b67bb179..955d7c21 100644 --- a/test/e2e-functions/test-app/src/functions/CallHttpOrchestration.ts +++ b/test/e2e-functions/test-app/src/functions/CallHttpOrchestration.ts @@ -142,10 +142,16 @@ const CallHttp_HttpStart: HttpHandler = async (request: HttpRequest, context: In // hop uses the `127.0.0.1` origin for `xorigin` (so the localhost Location is cross-origin and // the credential is stripped) and the `localhost` origin for `xorigin-same` (so the Location is // same-origin and the credential is forwarded). Both share the host's port. - const u = new URL(request.url); - const firstHost = mode === 'xorigin' ? '127.0.0.1' : 'localhost'; + // Build the URL by construction, not string interpolation: a `${u.port}` template emits an + // invalid dangling `:` (e.g. `https://127.0.0.1:/api/...`) whenever the host used a protocol + // default port (80/443, where `URL.port` is ""). Setting `hostname` keeps the host's actual + // port, and `URL` omits it only when it is the scheme default. Mirrors `HttpCrossOriginStart` above. + const target = new URL(request.url); + target.hostname = mode === 'xorigin' ? '127.0.0.1' : 'localhost'; + target.pathname = '/api/HttpCrossOriginStart'; + target.search = ''; input = { - url: `${u.protocol}//${firstHost}:${u.port}/api/HttpCrossOriginStart`, + url: target.toString(), headers: { Authorization: 'Bearer e2e-secret' }, }; } else { From 1d1d63594c8d09e123ada6d6a5fd329f418b76ae Mon Sep 17 00:00:00 2001 From: wangbill Date: Mon, 27 Jul 2026 15:49:56 -0700 Subject: [PATCH 19/21] Correct sub-orch ID test comment: scheduler's executionId, not child's The comment on the DTS 100-char-limit nesting test claimed the derived child instance ID is "keyed on the child's OWN fresh executionId". That is false about how the code behaves. RuntimeOrchestrationContext. callSubOrchestrator derives the default child ID from `this._executionId` -- the SCHEDULING (parent) orchestration's per-generation execution ID (runtime-orchestration-context.ts:404-406) -- because the ID must be derived deterministically before the child exists, so only values the scheduler already holds can be used. A child-minted executionId cannot be in play. A wrong comment here is a live trap: a reader who believes a child value is used would conclude that swapping which executionId keys the ID is harmless, when in fact anything but the scheduler's replay-stable executionId breaks determinism -- in the one area of this codebase that has already produced two bugs (the cross-generation collision, then the 112-char length regression). The rewrite fixes the attribution and states why it must be the scheduler's, which is what prevents the misreading. Comment-only: no it(), assertion, or fixture setup changed, and the derivation source is untouched. Addresses review comment 3661384000. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d86f5f69-4e1a-4c1d-b017-5e290c85cc05 --- .../test/sub-orchestration-instance-id.spec.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/durabletask-js/test/sub-orchestration-instance-id.spec.ts b/packages/durabletask-js/test/sub-orchestration-instance-id.spec.ts index e830f4b4..2a0259bc 100644 --- a/packages/durabletask-js/test/sub-orchestration-instance-id.spec.ts +++ b/packages/durabletask-js/test/sub-orchestration-instance-id.spec.ts @@ -59,13 +59,15 @@ describe("default sub-orchestration instance ID derivation", () => { }); it("keeps default-derived IDs within the DTS 100-char instance-ID limit at every nesting depth", () => { - // The Durable Task Scheduler caps instance IDs at 100 characters (enforced server-side). Because - // the derived ID is `${executionId}:${suffix}` — keyed on the child's OWN fresh executionId and - // NOT prefixed with the parent instance ID — it must stay constant-length no matter how deeply - // sub-orchestrations nest. (The pre-fix `${parentId}:${executionId}:${suffix}` shape concatenated - // every ancestor and blew past 100 chars at ~2 levels deep.) Simulate a realistic top-level - // 36-char auto-GUID and a fresh 32-hex executionId per level, deriving each level's child from the - // previous level's derived ID. + // The Durable Task Scheduler caps instance IDs at 100 characters (enforced server-side). The + // derived ID is `${executionId}:${suffix}`, keyed on the SCHEDULING (parent) orchestration's + // per-generation executionId — it must be the scheduler's, since the ID is derived + // deterministically before the child exists — and NOT prefixed with the parent instance ID. + // Each nesting level therefore contributes only its own fixed-length executionId, so the ID + // stays constant-length no matter how deeply sub-orchestrations nest. (The pre-fix + // `${parentId}:${executionId}:${suffix}` shape concatenated every ancestor and blew past 100 + // chars at ~2 levels deep.) Simulate a realistic top-level 36-char auto-GUID and a fresh 32-hex + // executionId per level, deriving each level's child from the previous level's derived ID. const executionIds = ["0", "1", "2", "3"].map((c) => c.repeat(32)); // 32-hex-length, distinct per level let parentInstanceId = "4cb1b016-ec71-4608-bdeb-328306cc0215"; // 36 chars, like an auto-generated GUID const derived: string[] = []; From e6f286124a4da3b678a5c7e27c0268235344557f Mon Sep 17 00:00:00 2001 From: wangbill Date: Tue, 28 Jul 2026 09:17:06 -0700 Subject: [PATCH 20/21] Remove test-only credential-cache reset hook from production source `builtin.ts` exported `__resetCredentialCacheForTests()` solely so the unit tests could clear the module-scope `cachedCredential` between cases. Shipping a test-only hook in production code is undesirable (per review feedback), so the export is deleted and the tests achieve the same isolation the way the nested "@azure/identity" suite already does: `jest.resetModules()` + re-`require` of the module before each test, which yields a fresh module instance with an empty credential cache. No production behavior changes -- `cachedCredential`, `getCredential()`, `acquireBearerToken()`, and the poll loop are untouched. Test changes (http-builtin.spec.ts): - Drop `builtinHttpActivity` and `__resetCredentialCacheForTests` from the static import; bind `builtinHttpActivity` per-test via a block-scoped `let` assigned in `beforeEach` from the freshly required module. - Replace the reset-hook `beforeEach` with `jest.resetModules()` + re-`require`, and document why (module-scope credential state -> re-require for an empty cache, deliberately avoiding a production reset export). Pure refactor: compat unit 148 and core unit 1170 both unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d86f5f69-4e1a-4c1d-b017-5e290c85cc05 --- .../azure-functions-durable/src/http/builtin.ts | 9 --------- .../test/unit/http-builtin.spec.ts | 16 +++++++++++----- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/packages/azure-functions-durable/src/http/builtin.ts b/packages/azure-functions-durable/src/http/builtin.ts index b13b2294..7cf91a78 100644 --- a/packages/azure-functions-durable/src/http/builtin.ts +++ b/packages/azure-functions-durable/src/http/builtin.ts @@ -198,15 +198,6 @@ function getCredential(): BearerCredential { return cachedCredential; } -/** - * Test-only hook: drop the cached credential so a re-swapped virtual `@azure/identity` mock (or a - * fresh-construction assertion) is honored rather than served from a prior test's cache. - * @internal - */ -export function __resetCredentialCacheForTests(): void { - cachedCredential = undefined; -} - /** Acquire an AAD bearer token for `resource` from the shared, lazily constructed credential. */ async function acquireBearerToken(resource: string): Promise { const credential = getCredential(); diff --git a/packages/azure-functions-durable/test/unit/http-builtin.spec.ts b/packages/azure-functions-durable/test/unit/http-builtin.spec.ts index 9cd10ae2..30946174 100644 --- a/packages/azure-functions-durable/test/unit/http-builtin.spec.ts +++ b/packages/azure-functions-durable/test/unit/http-builtin.spec.ts @@ -4,11 +4,9 @@ import { OrchestrationContext, Task } from "@microsoft/durabletask-js"; import { BUILTIN_HTTP_ACTIVITY_NAME, - builtinHttpActivity, builtinHttpPollOrchestrator, isSameOrigin, retryAfterSeconds, - __resetCredentialCacheForTests, } from "../../src/http/builtin"; import { DurableHttpRequestPayload, DurableHttpResponse } from "../../src/http/models"; @@ -116,15 +114,23 @@ describe("isSameOrigin", () => { describe("builtinHttpActivity", () => { const originalFetch = global.fetch; + // Re-required fresh before each test (see `beforeEach`), so this is a runtime-assigned binding + // rather than a static import — that is what lets each test observe an empty credential cache. + let builtinHttpActivity: typeof import("../../src/http/builtin").builtinHttpActivity; + afterEach(() => { global.fetch = originalFetch; jest.clearAllMocks(); }); - // The credential is cached at module scope; clear it between tests so a swapped virtual mock (or a - // construction-count assertion) is never served a prior test's cached instance. + // The lazily constructed credential is cached at MODULE scope, so a test cannot clear it without a + // production reset hook. Instead, re-require the module before each test for an empty cache: + // `jest.resetModules()` busts Jest's require cache so the next `require` returns a fresh module + // instance (the same fresh-load pattern the nested "@azure/identity" suite uses via + // `loadActivityFresh()`). This keeps the test-only reset concern in the test, out of `builtin.ts`. beforeEach(() => { - __resetCredentialCacheForTests(); + jest.resetModules(); + builtinHttpActivity = require("../../src/http/builtin").builtinHttpActivity; }); it("performs the request and passes the 200 response through", async () => { From fde1b78e7b9d21b501ad0e73cd457e5b046ce5f8 Mon Sep 17 00:00:00 2001 From: wangbill Date: Tue, 28 Jul 2026 13:38:55 -0700 Subject: [PATCH 21/21] Guard callHttp Retry-After against Date overflow; correct v3-compat docs Addresses Andy Staples' review on #333 (callHttp restore). A2 (fix): `retryAfterSeconds` accepted arbitrarily large delays. A huge delta-seconds string (parseInt can even reach Infinity) OR an HTTP-date whose `Math.ceil` rounds one step past the maximum ECMAScript time value produced a delay for which `new Date(now + seconds*1000)` is Invalid; `createTimer` then throws on the NaN timestamp and fails the whole orchestration. Retry-After is a callee-controlled response header, so this was remote-triggerable. Restructure so a single guard covers both the delta-seconds and HTTP-date branches: clamp to the 30s default when `seconds` is non-finite or exceeds `floor((MAX_TIME_VALUE_MS - now)/1000)`. The bound is derived from the Date range (rejects the minimum necessary, so a large-but-in-range delay still passes) and also subsumes non-safe-integer values. Note the bug bites at 13 digits, before Infinity, so an isFinite()-only check is insufficient. Add 4 tests (Infinity, finite-out-of-range, HTTP-date ceil overflow, large-in-range passthrough); the 6 existing tests are unchanged. A3 (docs only): keep failing loudly on a GET/HEAD body, but relabel it a known incompatibility rather than a hardening choice. v3 and both sibling SDKs send the body regardless of method (verified .NET TaskHttpActivityShim and durabletask-python builtin.py have no method check); the Fetch Standard forbids it, so fetch-based callHttp cannot match without a different transport. A4 (docs only): stop describing callHttp as fully v3-compatible. `getHeader()` is unavailable on the plain-object response, so existing `response.getHeader(...)` calls fail at runtime and must index `response.headers[...]`. Reword the README headline, the DurableHttpResponse bullet, models.ts JSDoc, and add a pointer from the callHttp JSDoc to the README differences. No hydration is attempted. No production behavior change for A3/A4 (docs only); A2 changes only the out-of-range fallback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d86f5f69-4e1a-4c1d-b017-5e290c85cc05 --- packages/azure-functions-durable/README.md | 25 ++++++++----- .../src/http/builtin.ts | 36 ++++++++++++++----- .../src/http/models.ts | 13 ++++--- .../src/orchestration-context.ts | 4 +++ .../test/unit/http-builtin.spec.ts | 28 +++++++++++++++ 5 files changed, 86 insertions(+), 20 deletions(-) diff --git a/packages/azure-functions-durable/README.md b/packages/azure-functions-durable/README.md index 499feefc..3d642a08 100644 --- a/packages/azure-functions-durable/README.md +++ b/packages/azure-functions-durable/README.md @@ -45,7 +45,9 @@ changed: `context.entities.isInCriticalSection()`. Restoring the v3 `df.lock` / `isLocked` surface is tracked in [#317](https://github.com/microsoft/durabletask-js/issues/317). - **`context.df.callHttp(...)` is restored** as a worker-side durable HTTP call - ([#318](https://github.com/microsoft/durabletask-js/issues/318)). It accepts the v3 + ([#318](https://github.com/microsoft/durabletask-js/issues/318)) — though **not** as a drop-in, fully + v3-equivalent replacement: the known incompatibilities and behavior differences listed below are + load-bearing for migration, so review them before relying on it. It accepts the v3 `CallHttpOptions` (`method`, `url`, `body`, `headers`, `tokenSource`, `enablePolling`) and returns a `Task` (`{ statusCode, headers, content }`), including automatic `202 Accepted` polling that honors `Retry-After` via durable timers. **Trust-boundary change:** in v3 the Functions @@ -77,13 +79,20 @@ changed: drive arbitrary SSRF or Managed-Identity token minting. - **The default poll interval is 30 s** (matching the classic host) when a `202` carries no usable `Retry-After`, rather than polling once per second. - - **A body on a `GET`/`HEAD` request throws.** The Fetch standard forbids it; v3 silently sent it, so - rather than change the request semantics the call fails loudly — remove `body` or use - `POST`/`PUT`/`PATCH`. - - **`DurableHttpResponse` is a plain object, not a class.** The response crosses the poll - sub-orchestration's JSON boundary, so the v3 `response.getHeader(name)` method is not available; read - headers directly via `response.headers["name"]` (response header names are lower-cased by `fetch`). - Restoring the case-insensitive accessor is tracked as a follow-up. + - **Known incompatibility — a body on a `GET`/`HEAD` request throws.** v3 attached request content + regardless of method, and both the .NET extension (`TaskHttpActivityShim` builds the message with + no method check) and the durabletask-python SDK still pass the body to the request unconditionally. + The [Fetch Standard](https://fetch.spec.whatwg.org/) forbids a body on `GET`/`HEAD` and the + underlying `fetch` implementation rejects it, so this cannot be matched while `callHttp` is built on + `fetch`. Failing loudly was chosen over silently dropping the body (which would change the request + the app asked for): a migrated v3 workflow that relied on it must drop the `body` or switch to + `POST`/`PUT`/`PATCH`. Restoring the v3 behavior would require replacing `fetch` with a lower-level + HTTP transport, which is not planned. + - **Known incompatibility — `DurableHttpResponse` is a plain object, not a class.** The response + crosses the poll sub-orchestration's JSON boundary, so the v3 `response.getHeader(name)` method is + **not** available — existing `response.getHeader(...)` calls **fail at runtime** and must be + rewritten to index `response.headers[...]` by lower-cased key (response header names are lower-cased + by `fetch`). - **Some v3 top-level exports were removed** — `DummyOrchestrationContext` / `DummyEntityContext` (testing utilities) and the entity-lock types above. `TaskFailedError` is re-exported from the core SDK (aggregate failures surface as JS-native `AggregateError`); use the diff --git a/packages/azure-functions-durable/src/http/builtin.ts b/packages/azure-functions-durable/src/http/builtin.ts index 7cf91a78..ffb8b674 100644 --- a/packages/azure-functions-durable/src/http/builtin.ts +++ b/packages/azure-functions-durable/src/http/builtin.ts @@ -51,6 +51,9 @@ export const BUILTIN_HTTP_POLL_ORCHESTRATOR_NAME = "BuiltIn__HttpPollOrchestrato */ const DEFAULT_POLL_INTERVAL_SECONDS = 30; +/** Maximum ECMAScript time value (ECMA-262); a `Date` beyond this has a NaN timestamp. */ +const MAX_TIME_VALUE_MS = 8_640_000_000_000_000; + /** * @internal * Result of the built-in HTTP activity: the public {@link DurableHttpResponse} plus the effective @@ -109,10 +112,12 @@ export function isSameOrigin(a: string, b: string): boolean { * * @remarks * Supports both the delta-seconds and HTTP-date forms; falls back to - * {@link DEFAULT_POLL_INTERVAL_SECONDS} when absent or unparseable. For the HTTP-date form the delay - * is computed against `now` — which the caller supplies as the orchestration's replay-safe - * `currentUtcDateTime` — so the resulting timer fire time is deterministic across replays, and is - * rounded **up** so the poll never fires before the server-specified instant. + * {@link DEFAULT_POLL_INTERVAL_SECONDS} when the header is absent, unparseable, or specifies a delay + * so large that `now + seconds` would exceed the maximum representable {@link Date} (see the guard + * below). For the HTTP-date form the delay is computed against `now` — which the caller supplies as + * the orchestration's replay-safe `currentUtcDateTime` — so the resulting timer fire time is + * deterministic across replays, and is rounded **up** so the poll never fires before the + * server-specified instant. * * @internal Exported for unit testing. */ @@ -122,14 +127,29 @@ export function retryAfterSeconds(headers: { [key: string]: string }, now: Date) return DEFAULT_POLL_INTERVAL_SECONDS; } const trimmed = raw.trim(); + let seconds: number; if (/^\d+$/.test(trimmed)) { - return Math.max(parseInt(trimmed, 10), 0); + seconds = parseInt(trimmed, 10); + } else { + const retryAtMs = Date.parse(trimmed); + if (Number.isNaN(retryAtMs)) { + return DEFAULT_POLL_INTERVAL_SECONDS; + } + seconds = Math.ceil((retryAtMs - now.getTime()) / 1000); } - const retryAtMs = Date.parse(trimmed); - if (Number.isNaN(retryAtMs)) { + // `Retry-After` is remote-controlled: the caller feeds this into `new Date(now + seconds * 1000)` + // and hands the result to `createTimer`, which throws on a NaN timestamp. A delay that pushes past + // the maximum ECMAScript time value would therefore let a callee fail the whole orchestration, so + // clamp to the default instead. The bound is DERIVED from the Date range — `seconds <= + // floor((MAX - now) / 1000)` guarantees `now + seconds * 1000 <= MAX` exactly — rather than an + // arbitrary "sane poll interval" cap, so it rejects the minimum necessary and also subsumes + // non-finite / non-safe-integer values. It covers BOTH forms: a huge delta-seconds string (parseInt + // can even reach Infinity) AND an HTTP-date whose `Math.ceil` rounds one step past the boundary. + const maxSeconds = Math.floor((MAX_TIME_VALUE_MS - now.getTime()) / 1000); + if (!Number.isFinite(seconds) || seconds > maxSeconds) { return DEFAULT_POLL_INTERVAL_SECONDS; } - return Math.max(Math.ceil((retryAtMs - now.getTime()) / 1000), 0); + return Math.max(seconds, 0); } /** diff --git a/packages/azure-functions-durable/src/http/models.ts b/packages/azure-functions-durable/src/http/models.ts index 7e824b68..82a99179 100644 --- a/packages/azure-functions-durable/src/http/models.ts +++ b/packages/azure-functions-durable/src/http/models.ts @@ -2,7 +2,8 @@ // Licensed under the MIT License. /** - * Durable HTTP request/response models (durable-functions v3 compatible). + * Durable HTTP request/response models that **mirror** the durable-functions v3 shapes (they are not + * the v3 classes themselves). * * @remarks * In v3 the Durable Functions host extension executed `context.df.callHttp` natively (including @@ -69,11 +70,15 @@ export interface CallHttpOptions { } /** - * The response returned by `context.df.callHttp` (classic durable-functions v3 shape). + * The response returned by `context.df.callHttp` (**shape-compatible** with the classic + * durable-functions v3 `DurableHttpResponse`, not the v3 type itself). * * @remarks - * The value crosses the sub-orchestration boundary as JSON, so `callHttp` resolves to a plain object - * of this shape. Consumers read `response.statusCode` / `response.content` / `response.headers`. + * v3's `DurableHttpResponse` was a **class** exposing a case-insensitive `getHeader(name)` accessor. + * Here the value crosses the sub-orchestration boundary as JSON, so `callHttp` resolves to a plain + * **object** with no methods — `getHeader()` is unavailable; read headers directly via + * `response.headers[...]` by lower-cased key. Consumers read `response.statusCode` / + * `response.content` / `response.headers`. */ export interface DurableHttpResponse { /** The HTTP response status code. */ diff --git a/packages/azure-functions-durable/src/orchestration-context.ts b/packages/azure-functions-durable/src/orchestration-context.ts index 451a7753..b4102e3b 100644 --- a/packages/azure-functions-durable/src/orchestration-context.ts +++ b/packages/azure-functions-durable/src/orchestration-context.ts @@ -164,6 +164,10 @@ export class DurableOrchestrationContext { * identity, and firewall/VNet behavior therefore follow the worker process. Managed-identity * token acquisition (`tokenSource`) requires the optional `@azure/identity` package. * + * This is **not** a fully v3-equivalent drop-in; see the package README for the documented + * differences from v3 — notably the `GET`/`HEAD` body restriction, the absence of + * `response.getHeader()`, and cross-origin `202` poll credential stripping. + * * @returns A {@link Task} resolving to the final {@link DurableHttpResponse}. */ callHttp(options: CallHttpOptions): Task { diff --git a/packages/azure-functions-durable/test/unit/http-builtin.spec.ts b/packages/azure-functions-durable/test/unit/http-builtin.spec.ts index 30946174..f556ddb6 100644 --- a/packages/azure-functions-durable/test/unit/http-builtin.spec.ts +++ b/packages/azure-functions-durable/test/unit/http-builtin.spec.ts @@ -85,6 +85,34 @@ describe("retryAfterSeconds", () => { retryAfterSeconds({ "Retry-After": "Thu, 01 Jan 2026 00:00:00 GMT" }, new Date("2026-01-01T00:01:00.000Z")), ).toBe(0); }); + + it("clamps a delta-seconds value that overflows to Infinity to the 30s default", () => { + // Andy's literal case: an arbitrarily long digit string. parseInt("9".repeat(400)) === Infinity; + // new Date(now + Infinity) is Invalid and createTimer() would then fail the orchestration. + expect(retryAfterSeconds({ "Retry-After": "9".repeat(400) }, now)).toBe(30); + }); + + it("clamps a FINITE delta-seconds value that exceeds the Date range to the 30s default", () => { + // The bug bites long before Infinity: at 13 digits parseInt is still finite, but now + seconds + // overflows the max ECMAScript time value (8.64e15 ms), so new Date(...) is Invalid. An + // isFinite()-only check would miss this — the range bound is what catches it. + expect(retryAfterSeconds({ "Retry-After": "8640000000000" }, now)).toBe(30); + }); + + it("clamps an HTTP-date whose ceil rounds past the max time value to the 30s default", () => { + // The HTTP-date branch has the same overflow via Math.ceil, which the delta-seconds guard alone + // would not cover. This date parses to exactly the max time value; one ms past the boundary, + // Math.ceil rounds the delay one step beyond it, so new Date(now + seconds) is Invalid. + expect( + retryAfterSeconds({ "Retry-After": "Sat, 13 Sep 275760 00:00:00 GMT" }, new Date(1785268800001)), + ).toBe(30); + }); + + it("passes a large but in-range delta-seconds value through unchanged", () => { + // Regression guard: the overflow clamp must reject the minimum necessary and NOT over-reject a + // large-but-valid delay (no arbitrary "sane maximum" policy is applied). + expect(retryAfterSeconds({ "Retry-After": "86400" }, now)).toBe(86400); + }); }); describe("isSameOrigin", () => {