Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ MONGODB_URI="mongodb://localhost:27017/acbu_db"
RABBITMQ_URL="amqp://guest:guest@localhost:5672"
JWT_SECRET="dev-jwt-secret-change-me"

# CORS — comma-separated exact origins (no *; required in production).
# Unset in development defaults to http://localhost:3000
# CORS_ORIGIN=http://localhost:3000,https://app.example.com

# B-058: PII field-level encryption key (AES-256-GCM).
# Must be a 64-character hex string (32 bytes). Required in production.
# Generate: node -e "console.log(require('crypto').randomBytes(32).toString('hex'))"
Expand Down
63 changes: 63 additions & 0 deletions src/config/corsOrigins.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/** Default browser origin for local development when CORS_ORIGIN is unset. */
export const DEFAULT_DEV_CORS_ORIGIN = "http://localhost:3000";

/**
* Parse and validate CORS_ORIGIN for credentialed cross-origin requests.
* Wildcard (*) is rejected: browsers forbid ACAO:* with Allow-Credentials.
*/
export function parseCorsOrigins(raw: string | undefined, nodeEnv: string): string[] {
const trimmed = raw?.trim();

if (!trimmed) {
if (nodeEnv === "production") {
throw new Error(
"CORS_ORIGIN is required in production. Set a comma-separated list of exact origins " +
"(e.g. https://app.example.com). Wildcard * is not allowed with credentials.",
);
}
return [DEFAULT_DEV_CORS_ORIGIN];
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

const origins = trimmed
.split(",")
.map((entry) => entry.trim())
.filter(Boolean);

if (origins.length === 0) {
throw new Error(
"CORS_ORIGIN must list at least one origin when set (wildcard * is not allowed).",
);
}

if (origins.some((origin) => origin === "*")) {
throw new Error(
"CORS_ORIGIN must not contain wildcard (*). List explicit origins; " +
"credentialed responses require Access-Control-Allow-Origin to match the request origin.",
);
}

const normalized: string[] = [];
for (const origin of origins) {
let url: URL;
try {
url = new URL(origin);
} catch {
throw new Error(
`Invalid CORS origin "${origin}". Use full origins such as https://app.example.com`,
);
}
if (url.protocol !== "http:" && url.protocol !== "https:") {
throw new Error(
`Invalid CORS origin "${origin}". Only http:// and https:// origins are supported.`,
);
}
if (url.username || url.password || url.pathname !== "/" || url.search || url.hash) {
throw new Error(
`Invalid CORS origin "${origin}". Provide scheme, host, and port only (no path or credentials).`,
);
}
normalized.push(url.origin);
}

return [...new Set(normalized)];
}
5 changes: 3 additions & 2 deletions src/config/env.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import dotenv from "dotenv";
import { z } from "zod";
import { parseCorsOrigins } from "./corsOrigins";

dotenv.config();

Expand Down Expand Up @@ -422,6 +423,6 @@ export const config = {
maxTokensPerRequest: env.OPENAI_MAX_TOKENS_PER_REQUEST,
},

// CORS
corsOrigin: process.env.CORS_ORIGIN?.split(",") || [],
// CORS — explicit origins only; wildcard * is rejected (incompatible with credentials)
corsOrigin: parseCorsOrigins(process.env.CORS_ORIGIN, env.NODE_ENV),
};
54 changes: 42 additions & 12 deletions src/middleware/cors.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,47 @@
import cors from "cors";
import cors, { type CorsOptions } from "cors";
import { config } from "../config/env";

export const corsMiddleware = cors({
origin: (origin, callback) => {
if (
!origin || config.corsOrigin.includes(origin)
) {
callback(null, true);
} else {
callback(new Error("Not allowed by CORS"));
}
},
/**
* Resolve the Access-Control-Allow-Origin value for a request.
* Returns the exact origin string when allowed, false when denied,
* or null when the request has no Origin (non-browser / same-origin tools).
*/
export function resolveCorsOrigin(
requestOrigin: string | undefined,
allowedOrigins: readonly string[],
): string | false | null {
if (!requestOrigin) {
return null;
}
if (allowedOrigins.includes(requestOrigin)) {
return requestOrigin;
}
return false;
}

const corsOptionsBase: Omit<CorsOptions, "origin"> = {
credentials: true,
methods: ["GET", "POST", "PUT", "DELETE", "PATCH", "OPTIONS"],
allowedHeaders: ["Content-Type", "Authorization", "X-API-Key"],
});
};

/** Build CORS middleware for a fixed allowlist (used in tests and production). */
export function createCorsMiddleware(allowedOrigins: readonly string[]) {
return cors({
...corsOptionsBase,
origin: (origin, callback) => {
const resolved = resolveCorsOrigin(origin, allowedOrigins);
if (resolved === null) {
callback(null, true);
return;
}
if (resolved === false) {
callback(new Error("Not allowed by CORS"));
return;
}
callback(null, resolved);
},
});
}

export const corsMiddleware = createCorsMiddleware(config.corsOrigin);
101 changes: 101 additions & 0 deletions tests/cors.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
import express from "express";
import request from "supertest";
import { parseCorsOrigins, DEFAULT_DEV_CORS_ORIGIN } from "../src/config/corsOrigins";
import { createCorsMiddleware, resolveCorsOrigin } from "../src/middleware/cors";

describe("parseCorsOrigins", () => {
it("defaults to localhost in non-production when unset", () => {
expect(parseCorsOrigins(undefined, "development")).toEqual([DEFAULT_DEV_CORS_ORIGIN]);
expect(parseCorsOrigins(undefined, "test")).toEqual([DEFAULT_DEV_CORS_ORIGIN]);
});

it("requires explicit origins in production when unset", () => {
expect(() => parseCorsOrigins(undefined, "production")).toThrow(
/CORS_ORIGIN is required in production/,
);
});

it("rejects wildcard *", () => {
expect(() => parseCorsOrigins("*", "development")).toThrow(/wildcard/i);
expect(() => parseCorsOrigins("https://a.com,*", "development")).toThrow(/wildcard/i);
});

it("parses, trims, and deduplicates comma-separated origins", () => {
expect(
parseCorsOrigins(
" https://app.example.com ,https://admin.example.com,https://app.example.com ",
"development",
),
).toEqual(["https://app.example.com", "https://admin.example.com"]);
});

it("normalizes valid origins to URL.origin form", () => {
expect(parseCorsOrigins("https://app.example.com:443", "development")).toEqual([
"https://app.example.com",
]);
});

it("rejects origins with paths", () => {
expect(() => parseCorsOrigins("https://app.example.com/dashboard", "development")).toThrow(
/scheme, host, and port only/i,
);
});
});

describe("resolveCorsOrigin", () => {
const allowed = ["http://localhost:3000", "https://app.example.com"];

it("returns null when Origin header is absent", () => {
expect(resolveCorsOrigin(undefined, allowed)).toBeNull();
});

it("returns the request origin when it is on the allowlist", () => {
expect(resolveCorsOrigin("https://app.example.com", allowed)).toBe("https://app.example.com");
});

it("returns false for origins not on the allowlist", () => {
expect(resolveCorsOrigin("https://evil.example.com", allowed)).toBe(false);
});
});

describe("createCorsMiddleware", () => {
const allowedOrigin = "http://localhost:3000";

function buildApp(allowedOrigins: string[]) {
const app = express();
app.use(createCorsMiddleware(allowedOrigins));
app.get("/ping", (_req, res) => {
res.json({ ok: true });
});
return app;
}

it("reflects an allowed Origin (never *) with credentials", async () => {
const res = await request(buildApp([allowedOrigin]))
.get("/ping")
.set("Origin", allowedOrigin)
.expect(200);

expect(res.headers["access-control-allow-origin"]).toBe(allowedOrigin);
expect(res.headers["access-control-allow-origin"]).not.toBe("*");
expect(res.headers["access-control-allow-credentials"]).toBe("true");
});

it("rejects disallowed cross-origin requests", async () => {
await request(buildApp([allowedOrigin]))
.get("/ping")
.set("Origin", "https://evil.example.com")
.expect(500);
});
Comment on lines +90 to +97

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check cors library behavior when origin callback returns an error

# Search for error handling in the cors middleware and related Express error patterns
rg -nP -C5 'callback\(new Error.*CORS' --type=ts

# Look for any existing documentation or tests about expected status codes
rg -nP 'expect\(\d{3}\)|toHaveStatus|status.*\d{3}' tests/cors.test.ts

Repository: Pi-Defi-world/acbu-backend

Length of output: 641


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) Show the whole relevant test file section around the disallowed-origin test
nl -ba tests/cors.test.ts | sed -n '1,220p'

# 2) Inspect CORS middleware implementation
nl -ba src/middleware/cors.ts | sed -n '1,220p'

# 3) Find where buildApp registers error handlers (to see what status errors map to)
#    (Look for common Express error-handler signatures or `app.use((err, req, res, next) => ...)`)
rg -n --hidden --glob='!**/node_modules/**' "buildApp|function buildApp|app\.use\(\s*\(err" src tests || true
rg -n --hidden --glob='!**/node_modules/**' "res\.status\(|status\(" src | head -n 50

# 4) Check if there's a centralized error handler file
fd -t f -a 'error|errors|middleware.*error|http.*error' . 2>/dev/null | head -n 50
rg -n --hidden --glob='!**/node_modules/**' "Not allowed by CORS|cors" src | head -n 50

Repository: Pi-Defi-world/acbu-backend

Length of output: 113


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) Show relevant parts of tests/cors.test.ts (first 220 lines; adjust if needed)
echo "=== tests/cors.test.ts (1-220) ==="
cat -n tests/cors.test.ts | sed -n '1,220p'

# 2) Inspect CORS middleware implementation
echo "=== src/middleware/cors.ts (1-220) ==="
cat -n src/middleware/cors.ts | sed -n '1,220p'

# 3) Locate Express error handler(s) in the app setup
echo "=== searching for error-handler signatures ==="
rg -n --hidden --glob='!**/node_modules/**' "(err\s*,\s*req\s*,\s*res\s*,\s*next)|(function\s*\(\s*err\s*,\s*req)|(app\.use\(\s*\(\s*err)" src tests || true

# 4) Find buildApp registration of middleware/error handling
echo "=== searching for buildApp ==="
rg -n --hidden --glob='!**/node_modules/**' "buildApp" src tests || true

# 5) If there's an error-handling module, inspect it
echo "=== searching for error handling modules ==="
fd -t f -a 'error|errors|http.*error|middleware.*error' . 2>/dev/null | head -n 50

# 6) If we find a candidate error handler file, print it (top portion)
#    (Try common filename matches first)
for f in $(fd -t f -a 'error|errors' src 2>/dev/null | head -n 10); do
  echo "=== candidate: $f (first 200 lines) ==="
  cat -n "$f" | sed -n '1,200p'
done

# 7) Also inspect where routes/middleware are wired (if there is an app file)
for f in $(fd -t f -a 'app\.ts$|server\.ts$|index\.ts$|main\.ts$' src 2>/dev/null | head -n 10); do
  echo "=== candidate: $f (first 200 lines) ==="
  cat -n "$f" | sed -n '1,200p'
done

Repository: Pi-Defi-world/acbu-backend

Length of output: 34068


🌐 Web query:

How does the cors npm package handle errors passed to the origin callback (callback(new Error(...))) in Express? What HTTP status is returned?

💡 Result:

cors does not translate callback(new Error(...)) into an Express error response with a specific HTTP status. Instead, the middleware treats the dynamic origin callback error as an origin rejection: it will not set Access-Control-Allow-Origin, and the request continues to the next middleware/route handler (so the final HTTP status is whatever your Express app ultimately sends). Authoritative behavior: the origin callback is documented as callback(error, origin) where error can be an error object to indicate failure [1][2][3]. For rejection/denial cases, the project specifically discusses handling when the origin callback returns false / rejects and focuses on CORS header behavior rather than sending an HTTP error response; e.g., a PR notes the middleware must set Vary: Origin even when the origin callback rejects, implying the middleware is controlling headers (not returning an app-level status) [4]. So, there isn’t a single fixed HTTP status returned by cors for callback(new Error(...)) in Express; your app’s normal response status (e.g., 200, 404, 500, etc.) will be used, and the client will typically see a CORS error because the required CORS headers are missing.

Citations:


Update the disallowed-origin CORS test to assert the correct HTTP status (200, not 500).

In tests/cors.test.ts the request should still reach the /ping handler; disallowed origins are handled by omitting Access-Control-* headers, not by producing an Express error response. Add a status assertion (.expect(200)) alongside the existing checks for missing CORS headers.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/cors.test.ts` around lines 90 - 97, The test "rejects disallowed
cross-origin requests" currently only checks that CORS headers are missing but
doesn't assert the response status; update the assertion so the request to
buildApp([allowedOrigin]).get("/ping").set("Origin", "https://evil.example.com")
also verifies a 200 status (e.g., add .expect(200) to the request chain or
assert res.status === 200) while keeping the existing checks for missing
access-control headers.


it("handles preflight OPTIONS with reflected origin", async () => {
const res = await request(buildApp([allowedOrigin]))
.options("/ping")
.set("Origin", allowedOrigin)
.set("Access-Control-Request-Method", "GET")
.expect(204);

expect(res.headers["access-control-allow-origin"]).toBe(allowedOrigin);
expect(res.headers["access-control-allow-credentials"]).toBe("true");
});
});
5 changes: 5 additions & 0 deletions tests/env.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,9 @@ describe("env validation", () => {
process.env.LOG_LEVEL = "invalid_level";
expect(() => require("../src/config/env")).toThrow(/LOG_LEVEL/);
});

it("throws when CORS_ORIGIN contains wildcard", () => {
process.env.CORS_ORIGIN = "*";
expect(() => require("../src/config/env")).toThrow(/wildcard/i);
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
});