diff --git a/.changeset/fix-cors-vary-merge.md b/.changeset/fix-cors-vary-merge.md new file mode 100644 index 0000000000..2856d33514 --- /dev/null +++ b/.changeset/fix-cors-vary-merge.md @@ -0,0 +1,5 @@ +--- +"effect": patch +--- + +Fix `HttpMiddleware.cors` overwriting the `Vary: Origin` header on preflight responses. When `allowedOrigins` is a predicate or has multiple entries and the preflight request includes `Access-Control-Request-Headers`, the middleware previously emitted only `Vary: Access-Control-Request-Headers`, dropping `Origin`. This could let a shared cache serve a preflight response cached for one origin to a request from a different origin. Vary entries are now merged into a single `Vary: Origin, Access-Control-Request-Headers` header. diff --git a/packages/effect/src/unstable/http/HttpMiddleware.ts b/packages/effect/src/unstable/http/HttpMiddleware.ts index f21966abfc..f95eae0070 100644 --- a/packages/effect/src/unstable/http/HttpMiddleware.ts +++ b/packages/effect/src/unstable/http/HttpMiddleware.ts @@ -313,12 +313,13 @@ export const cors = (options?: { ? opts.allowedOrigins : (origin: string) => (opts.allowedOrigins as ReadonlyArray).includes(origin) + const originVaries = typeof opts.allowedOrigins === "function" || opts.allowedOrigins.length > 0 + const allowOrigin = typeof opts.allowedOrigins === "function" || opts.allowedOrigins.length > 1 ? ((originHeader: string) => { if (!isAllowedOrigin(originHeader)) return undefined return { - "access-control-allow-origin": originHeader, - vary: "Origin" + "access-control-allow-origin": originHeader } }) : opts.allowedOrigins.length === 0 @@ -326,8 +327,7 @@ export const cors = (options?: { "access-control-allow-origin": "*" }) : constant({ - "access-control-allow-origin": opts.allowedOrigins[0], - vary: "Origin" + "access-control-allow-origin": opts.allowedOrigins[0] }) const allowMethods = opts.allowedMethods.length > 0 @@ -343,7 +343,6 @@ export const cors = (options?: { ): ReadonlyRecord | undefined => { if (opts.allowedHeaders.length === 0 && accessControlRequestHeaders) { return { - vary: "Access-Control-Request-Headers", "access-control-allow-headers": accessControlRequestHeaders } } @@ -367,23 +366,35 @@ export const cors = (options?: { const headersFromRequest = (request: HttpServerRequest) => { const origin = request.headers["origin"] + const allowOriginHeaders = allowOrigin(origin) return Headers.fromRecordUnsafe({ - ...allowOrigin(origin), + ...allowOriginHeaders, ...allowCredentials, - ...exposeHeaders + ...exposeHeaders, + ...(originVaries ? { vary: "Origin" } : undefined) }) } const headersFromRequestOptions = (request: HttpServerRequest) => { const origin = request.headers["origin"] const accessControlRequestHeaders = request.headers["access-control-request-headers"] + const allowOriginHeaders = allowOrigin(origin) + const allowHeadersHeaders = allowHeaders(accessControlRequestHeaders) + const varyParts: Array = [] + if (originVaries) { + varyParts.push("Origin") + } + if (opts.allowedHeaders.length === 0 && accessControlRequestHeaders) { + varyParts.push("Access-Control-Request-Headers") + } return Headers.fromRecordUnsafe({ - ...allowOrigin(origin), + ...allowOriginHeaders, ...allowCredentials, ...exposeHeaders, ...allowMethods, - ...allowHeaders(accessControlRequestHeaders), - ...maxAge + ...allowHeadersHeaders, + ...maxAge, + ...(varyParts.length > 0 ? { vary: varyParts.join(", ") } : undefined) }) } diff --git a/packages/effect/test/unstable/http/HttpMiddleware.test.ts b/packages/effect/test/unstable/http/HttpMiddleware.test.ts index 544f37c7d3..29c335def3 100644 --- a/packages/effect/test/unstable/http/HttpMiddleware.test.ts +++ b/packages/effect/test/unstable/http/HttpMiddleware.test.ts @@ -2,6 +2,7 @@ import { assert, describe, it } from "@effect/vitest" import * as Effect from "effect/Effect" import * as Logger from "effect/Logger" import * as References from "effect/References" +import * as HttpEffect from "effect/unstable/http/HttpEffect" import * as HttpMiddleware from "effect/unstable/http/HttpMiddleware" import * as HttpServerRequest from "effect/unstable/http/HttpServerRequest" import * as HttpServerResponse from "effect/unstable/http/HttpServerResponse" @@ -54,4 +55,120 @@ describe("HttpMiddleware", () => { assert.deepStrictEqual(spans, [["http.span"], ["http.span"]]) })) }) + + describe("cors", () => { + it.effect("preflight Vary header includes both Origin and Access-Control-Request-Headers when origin is dynamic", () => + Effect.gen(function*() { + const request = HttpServerRequest.fromWeb( + new Request("http://api.example.com/resource", { + method: "OPTIONS", + headers: { + "origin": "https://app-a.example.com", + "access-control-request-method": "POST", + "access-control-request-headers": "content-type" + } + }) + ) + + const response = yield* HttpMiddleware.cors({ + allowedOrigins: ["https://app-a.example.com", "https://app-b.example.com"] + })(Effect.succeed(HttpServerResponse.empty({ status: 200 }))).pipe( + Effect.provideService(HttpServerRequest.HttpServerRequest, request) + ) + + assert.strictEqual(response.headers["access-control-allow-origin"], "https://app-a.example.com") + // Bug: vary is just "Access-Control-Request-Headers" — the "Origin" entry + // set by allowOrigin is overwritten by the spread from allowHeaders. + // Without `Vary: Origin`, a shared cache may serve a preflight cached + // for app-a.example.com to a request from app-b.example.com. + const vary = response.headers["vary"] ?? "" + assert.include(vary, "Origin", `expected Vary to include "Origin", got: ${vary}`) + assert.include( + vary, + "Access-Control-Request-Headers", + `expected Vary to include "Access-Control-Request-Headers", got: ${vary}` + ) + })) + + it.effect("preflight Vary is just Origin when allowedHeaders is configured statically", () => + Effect.gen(function*() { + const request = HttpServerRequest.fromWeb( + new Request("http://api.example.com/resource", { + method: "OPTIONS", + headers: { + "origin": "https://app-a.example.com", + "access-control-request-method": "POST", + "access-control-request-headers": "content-type" + } + }) + ) + + const response = yield* HttpMiddleware.cors({ + allowedOrigins: ["https://app-a.example.com", "https://app-b.example.com"], + allowedHeaders: ["content-type", "authorization"] + })(Effect.succeed(HttpServerResponse.empty({ status: 200 }))).pipe( + Effect.provideService(HttpServerRequest.HttpServerRequest, request) + ) + + assert.strictEqual(response.headers["vary"], "Origin") + })) + + it.effect("preflight Vary includes Origin when a dynamic origin is rejected", () => + Effect.gen(function*() { + const request = HttpServerRequest.fromWeb( + new Request("http://api.example.com/resource", { + method: "OPTIONS", + headers: { + "origin": "https://denied.example.com", + "access-control-request-method": "POST", + "access-control-request-headers": "content-type" + } + }) + ) + + const response = yield* HttpMiddleware.cors({ + allowedOrigins: ["https://app-a.example.com", "https://app-b.example.com"] + })(Effect.succeed(HttpServerResponse.empty({ status: 200 }))).pipe( + Effect.provideService(HttpServerRequest.HttpServerRequest, request) + ) + + assert.strictEqual(response.headers["access-control-allow-origin"], undefined) + assert.strictEqual(response.headers["vary"], "Origin, Access-Control-Request-Headers") + })) + + it("response Vary includes Origin when a dynamic origin is rejected", async () => { + const response = await HttpEffect.toWebHandler( + HttpMiddleware.cors({ + allowedOrigins: (origin) => origin === "https://app-a.example.com" + })(Effect.succeed(HttpServerResponse.empty({ status: 200 }))) + )( + new Request("http://api.example.com/resource", { + headers: { origin: "https://denied.example.com" } + }) + ) + + assert.strictEqual(response.headers.get("access-control-allow-origin"), null) + assert.strictEqual(response.headers.get("vary"), "Origin") + }) + + it.effect("preflight has no Vary header for wildcard origin", () => + Effect.gen(function*() { + const request = HttpServerRequest.fromWeb( + new Request("http://api.example.com/resource", { + method: "OPTIONS", + headers: { + "origin": "https://app-a.example.com", + "access-control-request-method": "POST" + } + }) + ) + + const response = yield* HttpMiddleware.cors()( + Effect.succeed(HttpServerResponse.empty({ status: 200 })) + ).pipe(Effect.provideService(HttpServerRequest.HttpServerRequest, request)) + + assert.strictEqual(response.headers["access-control-allow-origin"], "*") + assert.strictEqual(response.headers["vary"], undefined) + })) + }) })