diff --git a/.changeset/eff-216-secure-http-redirects.md b/.changeset/eff-216-secure-http-redirects.md new file mode 100644 index 00000000000..fbfe9fde491 --- /dev/null +++ b/.changeset/eff-216-secure-http-redirects.md @@ -0,0 +1,5 @@ +--- +"effect": patch +--- + +Strip credential headers on cross-origin HTTP redirects and align redirected request methods with fetch. diff --git a/packages/effect/src/unstable/http/HttpClient.ts b/packages/effect/src/unstable/http/HttpClient.ts index 102237da248..7e28a67ca22 100644 --- a/packages/effect/src/unstable/http/HttpClient.ts +++ b/packages/effect/src/unstable/http/HttpClient.ts @@ -34,6 +34,7 @@ import type { EqualsWith, ExcludeTag, ExtractTag, NoExcessProperties, NoInfer, T import type * as RateLimiter from "../persistence/RateLimiter.ts" import * as Cookies from "./Cookies.ts" import * as Headers from "./Headers.ts" +import * as HttpBody from "./HttpBody.ts" import * as Error from "./HttpClientError.ts" import * as HttpClientRequest from "./HttpClientRequest.ts" import * as HttpClientResponse from "./HttpClientResponse.ts" @@ -1466,17 +1467,29 @@ export const followRedirects: { ): Effect.Effect => Effect.flatMap( self.postprocess(Effect.succeed(request)), - (response) => - response.status >= 300 && response.status < 400 && response.headers.location && - redirects < (maxRedirects ?? 10) - ? loop( - HttpClientRequest.setUrl( - request, - new URL(response.headers.location, response.request.url) - ), - redirects + 1 - ) - : Effect.succeed(response) + (response) => { + if ( + response.status < 300 || response.status >= 400 || !response.headers.location || + redirects >= (maxRedirects ?? 10) + ) { + return Effect.succeed(response) + } + const url = new URL(response.headers.location, response.request.url) + let nextRequest = request + if ( + ((response.status === 301 || response.status === 302) && request.method === "POST") || + (response.status === 303 && request.method !== "GET" && request.method !== "HEAD") + ) { + nextRequest = HttpClientRequest.setMethod(nextRequest, "GET") + nextRequest = HttpClientRequest.setBody(nextRequest, HttpBody.empty) + } + if (url.origin !== new URL(response.request.url).origin) { + nextRequest = HttpClientRequest.removeHeader(nextRequest, "authorization") + nextRequest = HttpClientRequest.removeHeader(nextRequest, "proxy-authorization") + nextRequest = HttpClientRequest.removeHeader(nextRequest, "cookie") + } + return loop(HttpClientRequest.setUrl(nextRequest, url), redirects + 1) + } ) return Effect.flatMap(request, (request) => loop(request, 0)) }, diff --git a/packages/effect/test/unstable/http/HttpClient.test.ts b/packages/effect/test/unstable/http/HttpClient.test.ts index 4660a42cf65..34c47c9f74a 100644 --- a/packages/effect/test/unstable/http/HttpClient.test.ts +++ b/packages/effect/test/unstable/http/HttpClient.test.ts @@ -2,7 +2,7 @@ import { assert, describe, it } from "@effect/vitest" import { strictEqual } from "@effect/vitest/utils" import { Clock, Duration, Effect, Fiber, Layer, Ref, Stream } from "effect" import { TestClock } from "effect/testing" -import { HttpClient, HttpClientResponse } from "effect/unstable/http" +import { HttpClient, HttpClientRequest, HttpClientResponse } from "effect/unstable/http" import { RateLimiter } from "effect/unstable/persistence" const makeStatusClient = Effect.fnUntraced(function*(status: number) { @@ -16,9 +16,133 @@ const makeStatusClient = Effect.fnUntraced(function*(status: number) { return { attempts, client } as const }) +const makeRedirectClient = Effect.fnUntraced(function*(status: number, location: string | ReadonlyArray) { + const locations = typeof location === "string" ? [location] : location + const requests = yield* Ref.make>([]) + const client = HttpClient.make((request) => + Effect.map( + Ref.updateAndGet(requests, (requests) => [...requests, request]), + (requests) => + HttpClientResponse.fromWeb( + request, + requests.length <= locations.length + ? new Response(null, { status, headers: { location: locations[requests.length - 1] } }) + : new Response(null, { status: 200 }) + ) + ) + ).pipe(HttpClient.followRedirects()) + return { client, requests } as const +}) + const RateLimiterTestLayer = RateLimiter.layer.pipe(Layer.provide(RateLimiter.layerStoreMemory)) describe("HttpClient", () => { + describe("followRedirects", () => { + it.effect("preserves credential headers on same-origin redirects", () => + Effect.gen(function*() { + const { client, requests } = yield* makeRedirectClient(302, "https://origin.test/destination") + yield* client.get("https://origin.test/start", { + headers: { + Authorization: "Bearer secret", + Cookie: "session=secret" + } + }) + + const redirected = (yield* Ref.get(requests))[1] + assert.strictEqual(redirected.headers.authorization, "Bearer secret") + assert.strictEqual(redirected.headers.cookie, "session=secret") + })) + + it.effect("strips credential headers on cross-origin redirects", () => + Effect.gen(function*() { + const { client, requests } = yield* makeRedirectClient(302, "https://redirect.test/destination") + yield* client.get("https://origin.test/start", { + headers: { + Authorization: "Bearer secret", + Cookie: "session=secret", + "Proxy-Authorization": "Basic secret", + "X-Test": "retained" + } + }) + + const redirected = (yield* Ref.get(requests))[1] + assert.isUndefined(redirected.headers.authorization) + assert.isUndefined(redirected.headers.cookie) + assert.isUndefined(redirected.headers["proxy-authorization"]) + assert.strictEqual(redirected.headers["x-test"], "retained") + })) + + it.effect("strips credential headers on scheme downgrade redirects", () => + Effect.gen(function*() { + const { client, requests } = yield* makeRedirectClient(302, "http://origin.test/destination") + yield* client.get("https://origin.test/start", { + headers: { + Authorization: "Bearer secret", + Cookie: "session=secret", + "Proxy-Authorization": "Basic secret" + } + }) + + const redirected = (yield* Ref.get(requests))[1] + assert.isUndefined(redirected.headers.authorization) + assert.isUndefined(redirected.headers.cookie) + assert.isUndefined(redirected.headers["proxy-authorization"]) + })) + + it.effect("resolves relative locations against the current hop", () => + Effect.gen(function*() { + const { client, requests } = yield* makeRedirectClient(302, ["../next/step", "destination"]) + yield* client.get("https://origin.test/path/start", { + headers: { Authorization: "Bearer secret" } + }) + + const redirected = (yield* Ref.get(requests))[2] + assert.strictEqual(redirected.url, "https://origin.test/next/destination") + assert.strictEqual(redirected.headers.authorization, "Bearer secret") + })) + + it.effect("switches 303 requests to GET and drops the body", () => + Effect.gen(function*() { + const { client, requests } = yield* makeRedirectClient(303, "/destination") + yield* HttpClientRequest.post("https://origin.test/start").pipe( + HttpClientRequest.bodyText("payload"), + client.execute + ) + + const redirected = (yield* Ref.get(requests))[1] + assert.strictEqual(redirected.method, "GET") + assert.strictEqual(redirected.body._tag, "Empty") + assert.isUndefined(redirected.headers["content-type"]) + assert.isUndefined(redirected.headers["content-length"]) + })) + + it.effect.each([301, 302])("switches POST to GET on %s redirects", (status) => + Effect.gen(function*() { + const { client, requests } = yield* makeRedirectClient(status, "/destination") + yield* HttpClientRequest.post("https://origin.test/start").pipe( + HttpClientRequest.bodyText("payload"), + client.execute + ) + + const redirected = (yield* Ref.get(requests))[1] + assert.strictEqual(redirected.method, "GET") + assert.strictEqual(redirected.body._tag, "Empty") + })) + + it.effect.each([301, 302])("preserves non-POST methods on %s redirects", (status) => + Effect.gen(function*() { + const { client, requests } = yield* makeRedirectClient(status, "/destination") + yield* HttpClientRequest.put("https://origin.test/start").pipe( + HttpClientRequest.bodyText("payload"), + client.execute + ) + + const redirected = (yield* Ref.get(requests))[1] + assert.strictEqual(redirected.method, "PUT") + assert.strictEqual(redirected.body._tag, "Uint8Array") + })) + }) + describe("retryTransient", () => { it.effect("retries transient responses with retryOn errors-and-responses", () => Effect.gen(function*() {