Skip to content

Commit 11f240b

Browse files
izadoesdevizadoesdev
committed
fix(platform): harden external and client trust boundaries
1 parent bb93605 commit 11f240b

31 files changed

Lines changed: 596 additions & 368 deletions

‎apps/api/src/http/errors.test.ts‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,29 @@ describe("handleAppError", () => {
8383
requestId: "req_test_validation",
8484
});
8585
expect(payload.details).toEqual([
86-
expect.objectContaining({ field: "body.name" }),
86+
{ field: "body.name", message: "Invalid value" },
8787
]);
8888
});
89+
90+
it("does not reflect Elysia validation values or messages in production", async () => {
91+
process.env.NODE_ENV = "production";
92+
const response = handleAppError({
93+
code: "VALIDATION",
94+
requestId: "req_test_reflection",
95+
error: new ValidationError(
96+
"body",
97+
t.Object({ profile: t.Object({ email: t.String() }) }),
98+
{ profile: { email: 867_530_900 } }
99+
),
100+
});
101+
const payload = await readPayload(response);
102+
const serialized = JSON.stringify(payload);
103+
104+
expect(payload.details).toEqual([
105+
{ field: "body.profile.email", message: "Invalid value" },
106+
]);
107+
expect(serialized).not.toContain("867530900");
108+
expect(serialized).not.toContain("Expected");
109+
expect(serialized).not.toContain("found");
110+
});
89111
});

‎apps/api/src/http/errors.ts‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ export function handleAppError({
5959
isClientError,
6060
statusCode,
6161
});
62-
const validationDetails = getValidationDetails(error);
62+
const validationDetails = getValidationDetails(error, isDevelopment);
6363
const headers: Record<string, string> = {
6464
"Content-Type": "application/json",
6565
"X-Request-ID": responseRequestId,
@@ -91,7 +91,10 @@ interface ValidationDetail {
9191
message: string;
9292
}
9393

94-
function getValidationDetails(error: unknown): ValidationDetail[] {
94+
function getValidationDetails(
95+
error: unknown,
96+
isDevelopment: boolean
97+
): ValidationDetail[] {
9598
if (!(error instanceof ValidationError) || error.type === "response") {
9699
return [];
97100
}
@@ -111,10 +114,9 @@ function getValidationDetails(error: unknown): ValidationDetail[] {
111114
seenFields.add(field);
112115
details.push({
113116
field,
114-
message:
115-
typeof issue.summary === "string" && issue.summary
116-
? issue.summary
117-
: issue.message,
117+
message: isDevelopment
118+
? getDevelopmentValidationMessage(issue)
119+
: "Invalid value",
118120
});
119121
if (details.length === 20) {
120122
break;
@@ -123,6 +125,15 @@ function getValidationDetails(error: unknown): ValidationDetail[] {
123125
return details;
124126
}
125127

128+
function getDevelopmentValidationMessage(issue: {
129+
message: string;
130+
summary?: string;
131+
}): string {
132+
return typeof issue.summary === "string" && issue.summary
133+
? issue.summary
134+
: issue.message;
135+
}
136+
126137
function getErrorCode({
127138
explicitCode,
128139
parsedCode,

‎apps/api/src/integration/public-flags-http.test.ts‎

Lines changed: 37 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -259,7 +259,7 @@ describe("public flags HTTP integration", () => {
259259
`/v1/flags/bulk?clientId=${website.id}&userId=${user.id}&keys=global-a,personal-c,missing`
260260
);
261261
expect(response.status).toBe(200);
262-
expect(response.headers.get("cache-control")).toContain("public");
262+
expect(response.headers.get("cache-control")).toBe("private, no-store");
263263
const body = await json(response);
264264

265265
expect(body.count).toBe(2);
@@ -269,6 +269,40 @@ describe("public flags HTTP integration", () => {
269269
]);
270270
expect(body.flags["global-a"]).toMatchObject({ enabled: true });
271271
expect(body.flags["personal-c"]).toMatchObject({ enabled: true });
272+
273+
const omittedGet = await json(
274+
await get(`/v1/flags/bulk?clientId=${website.id}&userId=${user.id}`)
275+
);
276+
const omittedPost = await json(
277+
await post("/v1/flags/bulk", {
278+
clientId: website.id,
279+
userId: user.id,
280+
})
281+
);
282+
for (const result of [omittedGet, omittedPost]) {
283+
expect(result.count).toBe(3);
284+
expect(Object.keys(result.flags).sort()).toEqual([
285+
"global-a",
286+
"global-b",
287+
"personal-c",
288+
]);
289+
}
290+
291+
const emptyGet = await json(
292+
await get(
293+
`/v1/flags/bulk?clientId=${website.id}&userId=${user.id}&keys=`
294+
)
295+
);
296+
const emptyPost = await json(
297+
await post("/v1/flags/bulk", {
298+
clientId: website.id,
299+
keys: [],
300+
userId: user.id,
301+
})
302+
);
303+
for (const result of [emptyGet, emptyPost]) {
304+
expect(result).toEqual({ count: 0, flags: {} });
305+
}
272306
});
273307

274308
iit("returns safe defaults for missing params and malformed properties", async () => {
@@ -461,7 +495,7 @@ describe("public flags HTTP integration", () => {
461495
{ "x-api-key": keyA.secret }
462496
)
463497
).status
464-
).toBe(403);
498+
).toBe(404);
465499
});
466500

467501
iit("requires user-associated API keys for writes", async () => {
@@ -485,7 +519,7 @@ describe("public flags HTTP integration", () => {
485519

486520
expect(response.status).toBe(403);
487521
expect(await json(response)).toEqual({
488-
error: "API key must be associated with a user",
522+
error: "Forbidden",
489523
});
490524
});
491525
});

‎apps/api/src/routes/public/flags-boundary.test.ts‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
import "@databuddy/test/env";
2+
import { Elysia } from "elysia";
3+
import { describe, expect, it, vi } from "vitest";
4+
5+
const state = vi.hoisted(() => ({
6+
flags: [
7+
{
8+
defaultValue: true,
9+
dependencies: null,
10+
flagsToTargetGroups: [],
11+
key: "enabled-for-everyone",
12+
payload: null,
13+
rolloutBy: null,
14+
rolloutPercentage: null,
15+
rules: null,
16+
status: "active",
17+
type: "boolean",
18+
variants: null,
19+
},
20+
],
21+
}));
22+
23+
vi.mock("@databuddy/db", async (importOriginal) => ({
24+
...(await importOriginal<typeof import("@databuddy/db")>()),
25+
db: {
26+
query: {
27+
flags: {
28+
findMany: vi.fn(async () => state.flags),
29+
},
30+
},
31+
},
32+
}));
33+
34+
vi.mock("@databuddy/redis", async (importOriginal) => ({
35+
...(await importOriginal<typeof import("@databuddy/redis")>()),
36+
cacheable: (fn: (...args: never[]) => unknown) => fn,
37+
}));
38+
39+
vi.mock("@databuddy/redis/rate-limit", () => ({
40+
getRateLimitHeaders: () => ({}),
41+
ratelimit: async () => ({ success: true }),
42+
}));
43+
44+
const { flagsRoute } = await import("./flags");
45+
const app = new Elysia().use(flagsRoute);
46+
47+
function request(path: string, body?: unknown) {
48+
return app.handle(
49+
new Request(`http://localhost${path}`, {
50+
body: body === undefined ? undefined : JSON.stringify(body),
51+
headers:
52+
body === undefined ? undefined : { "content-type": "application/json" },
53+
method: body === undefined ? "GET" : "POST",
54+
})
55+
);
56+
}
57+
58+
describe("public bulk flags boundary", () => {
59+
it("returns all flags only when the key filter is omitted", async () => {
60+
for (const response of [
61+
await request("/v1/flags/bulk?clientId=site_1"),
62+
await request("/v1/flags/bulk", { clientId: "site_1" }),
63+
]) {
64+
expect(response.status).toBe(200);
65+
expect(await response.json()).toMatchObject({
66+
count: 1,
67+
flags: { "enabled-for-everyone": { enabled: true } },
68+
});
69+
}
70+
});
71+
72+
it("returns no flags for explicitly empty or blank key lists", async () => {
73+
for (const response of [
74+
await request("/v1/flags/bulk?clientId=site_1&keys="),
75+
await request("/v1/flags/bulk?clientId=site_1&keys=%20,%20"),
76+
await request("/v1/flags/bulk", { clientId: "site_1", keys: [] }),
77+
await request("/v1/flags/bulk", {
78+
clientId: "site_1",
79+
keys: ["", " "],
80+
}),
81+
]) {
82+
expect(response.status).toBe(200);
83+
expect(await response.json()).toEqual({ count: 0, flags: {} });
84+
}
85+
});
86+
87+
it("rejects more than 100 requested keys", async () => {
88+
const keys = Array.from({ length: 101 }, (_, index) => `flag-${index}`);
89+
const getResponse = await request(
90+
`/v1/flags/bulk?clientId=site_1&keys=${keys.join(",")}`
91+
);
92+
expect(getResponse.status).toBe(400);
93+
94+
const postResponse = await request("/v1/flags/bulk", {
95+
clientId: "site_1",
96+
keys,
97+
});
98+
expect(postResponse.status).toBe(422);
99+
});
100+
101+
it("rejects keys longer than 128 characters", async () => {
102+
const key = "x".repeat(129);
103+
const getResponse = await request(
104+
`/v1/flags/bulk?clientId=site_1&keys=${key}`
105+
);
106+
expect(getResponse.status).toBe(400);
107+
108+
const postResponse = await request("/v1/flags/bulk", {
109+
clientId: "site_1",
110+
keys: [key],
111+
});
112+
expect(postResponse.status).toBe(422);
113+
});
114+
});

‎apps/api/src/routes/public/flags.ts‎

Lines changed: 41 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,11 @@ interface EvaluableFlag {
8686
variants?: FlagVariant[] | null;
8787
}
8888

89+
const MAX_BULK_FLAG_KEYS = 100;
90+
const MAX_FLAG_KEY_LENGTH = 128;
91+
const MAX_BULK_FLAG_QUERY_LENGTH =
92+
MAX_BULK_FLAG_KEYS * (MAX_FLAG_KEY_LENGTH + 1);
93+
8994
const flagQuerySchema = t.Object({
9095
key: t.String(),
9196
clientId: t.String(),
@@ -99,7 +104,7 @@ const flagQuerySchema = t.Object({
99104

100105
const bulkFlagQuerySchema = t.Object({
101106
clientId: t.String(),
102-
keys: t.Optional(t.String()),
107+
keys: t.Optional(t.String({ maxLength: MAX_BULK_FLAG_QUERY_LENGTH })),
103108
userId: t.Optional(t.String()),
104109
email: t.Optional(t.String()),
105110
organizationId: t.Optional(t.String()),
@@ -110,7 +115,11 @@ const bulkFlagQuerySchema = t.Object({
110115

111116
const bulkFlagBodySchema = t.Object({
112117
clientId: t.String(),
113-
keys: t.Optional(t.Array(t.String())),
118+
keys: t.Optional(
119+
t.Array(t.String({ maxLength: MAX_FLAG_KEY_LENGTH }), {
120+
maxItems: MAX_BULK_FLAG_KEYS,
121+
})
122+
),
114123
userId: t.Optional(t.String()),
115124
email: t.Optional(t.String()),
116125
organizationId: t.Optional(t.String()),
@@ -776,16 +785,43 @@ async function evaluateBulkFlags(
776785
};
777786
}
778787

788+
if (input.keys && input.keys.length > MAX_BULK_FLAG_KEYS) {
789+
set.status = 400;
790+
return {
791+
flags: {},
792+
count: 0,
793+
error: `A maximum of ${MAX_BULK_FLAG_KEYS} flag keys is allowed`,
794+
};
795+
}
796+
797+
const normalizedKeys = input.keys?.map((key) => key.trim());
798+
if (normalizedKeys?.some((key) => key.length > MAX_FLAG_KEY_LENGTH)) {
799+
set.status = 400;
800+
return {
801+
flags: {},
802+
count: 0,
803+
error: `Flag keys must be at most ${MAX_FLAG_KEY_LENGTH} characters`,
804+
};
805+
}
806+
779807
const context: UserContext = {
780808
userId: input.userId,
781809
email: input.email,
782810
organizationId: input.organizationId,
783811
teamId: input.teamId,
784812
properties: input.properties,
785813
};
786-
const requestedKeys = input.keys
787-
? new Set(input.keys.map((key) => key.trim()).filter(Boolean))
788-
: null;
814+
const filteredKeys = normalizedKeys?.filter(Boolean);
815+
const requestedKeys =
816+
filteredKeys === undefined ? null : new Set(filteredKeys);
817+
if (requestedKeys?.size === 0) {
818+
mergeWideEvent({
819+
flag_total_flags: 0,
820+
flag_evaluated: 0,
821+
flag_count: 0,
822+
});
823+
return { flags: {}, count: 0 };
824+
}
789825

790826
const clientFlags = await fromMemory(
791827
`fc:${input.clientId}:${input.environment || ""}`,

‎apps/basket/src/index.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {
1717
handleUncaughtException,
1818
handleUnhandledRejection,
1919
} from "@lib/process-errors";
20+
import { sanitizeRequestId } from "@lib/request-id";
2021
import { buildBasketErrorPayload } from "@lib/structured-errors";
2122
import { captureError } from "@lib/tracing";
2223
import basketRouter from "@routes/basket";
@@ -104,7 +105,8 @@ const app = new Elysia()
104105
}
105106

106107
const requestId =
107-
request.headers.get("x-request-id") ?? crypto.randomUUID();
108+
sanitizeRequestId(request.headers.get("x-request-id")) ??
109+
crypto.randomUUID();
108110
captureError(error, { requestId });
109111

110112
const { status, payload } = buildBasketErrorPayload(error, {

‎apps/basket/src/lib/request-id.test.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
import { describe, expect, it } from "vitest";
2+
import { sanitizeRequestId } from "./request-id";
3+
4+
describe("sanitizeRequestId", () => {
5+
it("keeps bounded request identifiers used by common tracing systems", () => {
6+
expect(sanitizeRequestId(" req_123:trace-456.7 ")).toBe(
7+
"req_123:trace-456.7"
8+
);
9+
});
10+
11+
it.each([
12+
null,
13+
"",
14+
"request id",
15+
"request\r\nid",
16+
"x".repeat(129),
17+
])("rejects unsafe inbound request IDs: %j", (value) => {
18+
expect(sanitizeRequestId(value)).toBeNull();
19+
});
20+
});

‎apps/basket/src/lib/request-id.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
const REQUEST_ID_PATTERN = /^[A-Za-z0-9._:-]{1,128}$/;
2+
3+
export function sanitizeRequestId(value: string | null): string | null {
4+
const requestId = value?.trim();
5+
return requestId && REQUEST_ID_PATTERN.test(requestId) ? requestId : null;
6+
}

0 commit comments

Comments
 (0)