Skip to content

Commit 373e86f

Browse files
Digidaiclaude
andcommitted
fix: harden security and API semantics (Agent Team findings)
Three AI agents (Codex GPT-5.4, Gemini CLI, Claude subagent) ran end-to-end scenarios against the live deployment. This commit fixes all findings they surfaced, ordered by severity. ── P0 security fixes ── 1. Magic Link IP rate limiting (Codex MEDIUM + Portal BUG#4) Previously only per-email. An attacker could rotate across many victim emails from one IP and burn the Resend quota. Added per-IP counter (10 per hour, cf-connecting-ip keyed), 1 KB request body cap, and unified the rate-limit helpers into `incrementRateCounter()` / `readRateCounter()`. src/handlers/auth.ts 2. Revocation propagation: LRU TTL 60s → 10s (Portal BUG#2) A revoked key kept working for ~73s because the auth LRU cache was per isolate with 60s TTL. 10s is a better worst-case guarantee; D1 read amplification is bounded by TIER_QUOTAS traffic and the hot cache below. Hard cross-isolate invalidation is Phase D territory. src/middleware/auth-d1.ts 3. CORS: add DELETE to Allow-Methods (Portal BUG#1) Portal worked same-origin, but any cross-origin client (CI dashboard, browser extension) couldn't preflight DELETE /api/keys/:id. src/config.ts 4. Portal CSP + HSTS (Portal BUG#5, BUG#6) The landing page had a CSP but /portal/ — the page that literally shows plaintext API keys in a modal — had none. Added PORTAL_CSP in helpers/response.ts (allows 'unsafe-inline' for the single-file SPA JS, Google Fonts for styles, same-origin for API calls). Added HSTS header (1 year, includeSubDomains) to landing, portal, and loading pages. src/helpers/response.ts, src/index.ts 5. Cache-Control: no-store, private on Portal API (Codex LOW) /api/me, /api/keys, /api/usage, /api/auth/* now set Cache-Control so session-authenticated data is never cached by browsers or intermediaries. src/handlers/keys.ts, usage.ts, auth.ts ── P1 correctness / semantics ── 6. /api/health now public-only by default (Codex LOW) Default response is { status, service, uptime_seconds } — no browser queue depth, paywall rule counts, or metric counters. Full metrics require /api/health?full=1 with API_TOKEN Bearer. Fallback behavior on stats-provider errors is preserved for the full path. src/handlers/health.ts (new handlePublicHealth/handleFullHealth/handleHealthRoute) 7. /api/stream now emits X-RateLimit-* headers (Codex INFO) Previously rate limit headers only appeared on 429. Now the SSE success response carries X-RateLimit-Limit/Remaining and X-Request-Cost so clients can self-throttle without a separate /api/usage call. src/handlers/stream.ts, src/index.ts 8. POST /api/keys prefix format consistency (Portal UX) GET returned `mk_abc12345...` but POST returned the raw `abc12345`. Clients that treat "prefix is prefix" broke on copy-paste. POST now returns the same `mk_xxx...` format. src/handlers/keys.ts 9. Too Many Keys: 400 → 409 (Portal UX) The request is well-formed; the account state prevents creation. 409 Conflict is the correct status for a resource-state refusal. src/handlers/keys.ts 10. DELETE is now idempotent (Portal BUG#4) A second DELETE on an already-revoked key returned 400 "Already Revoked". REST convention is that DELETE is idempotent. Now returns 200 with the original revocation timestamp. src/handlers/keys.ts ── Tests ── - 2 new health endpoint tests (public vs full with auth gate) - Updated portal-keys.test.ts for prefix format, 409 status, idempotent DELETE - Updated index-health-fallback.test.ts to use ?full=1 + admin Bearer - All 603 tests pass (up from 601 due to 2 new health tests) ── Not in this commit ── - HTTP 000 / SSL resets (Portal BUG#3): unable to reproduce outside Portal agent's network context, leaving for now - prompt/alert/confirm in portal JS: UI polish, deferred - __Host- session cookie prefix: breaks existing sessions, deferred - Google Fonts SRI: Google Fonts doesn't publish subresource-integrity hashes; CSP restricts to fonts.googleapis.com which is the best available mitigation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent b14ba64 commit 373e86f

File tree

13 files changed

+257
-48
lines changed

13 files changed

+257
-48
lines changed

package-lock.json

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/__tests__/index-endpoints.test.ts

Lines changed: 37 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,26 +25,48 @@ describe("worker endpoints", () => {
2525
expect(res.headers.get("Access-Control-Allow-Headers")).toContain("Idempotency-Key");
2626
});
2727

28-
it("serves health endpoint for GET", async () => {
28+
it("serves public health endpoint for GET (minimal info)", async () => {
2929
const req = new Request("https://md.example.com/api/health");
3030
const res = await worker.fetch(req, createMockEnv().env, mockCtx());
3131
const payload = await res.json() as {
3232
status?: string;
3333
service?: string;
34-
browser?: { active?: number; queued?: number };
34+
uptime_seconds?: number;
35+
browser?: unknown;
36+
metrics?: unknown;
37+
paywall?: unknown;
38+
};
39+
40+
expect(res.status).toBe(200);
41+
expect(payload.status).toBe("ok");
42+
expect(payload.service).toBe("md.example.com");
43+
expect(payload.uptime_seconds).toBeGreaterThanOrEqual(0);
44+
// Internal state must NOT be exposed on the public endpoint
45+
expect(payload.browser).toBeUndefined();
46+
expect(payload.metrics).toBeUndefined();
47+
expect(payload.paywall).toBeUndefined();
48+
});
49+
50+
it("serves full health via ?full=1 + API_TOKEN", async () => {
51+
const { env } = createMockEnv({ API_TOKEN: "admin-token" });
52+
const req = new Request("https://md.example.com/api/health?full=1", {
53+
headers: { Authorization: "Bearer admin-token" },
54+
});
55+
const res = await worker.fetch(req, env, mockCtx());
56+
const payload = await res.json() as {
57+
status?: string;
58+
browser?: { active?: number; maxConcurrent?: number };
3559
metrics?: {
36-
requestsTotal?: number;
3760
operational?: {
3861
throughput?: { requests_per_min?: number };
39-
latency_ms?: { convert?: { p50?: number; p95?: number } };
62+
latency_ms?: { convert?: { p50?: number } };
4063
};
4164
};
4265
paywall?: { source?: string };
4366
};
4467

4568
expect(res.status).toBe(200);
4669
expect(payload.status).toBe("ok");
47-
expect(payload.service).toBe("md.example.com");
4870
expect(payload.browser).toBeTruthy();
4971
expect(payload.metrics).toBeTruthy();
5072
expect(payload.metrics?.operational).toBeTruthy();
@@ -53,6 +75,16 @@ describe("worker endpoints", () => {
5375
expect(payload.paywall).toBeTruthy();
5476
});
5577

78+
it("rejects full health without API_TOKEN (returns public info)", async () => {
79+
const { env } = createMockEnv({ API_TOKEN: "admin-token" });
80+
const req = new Request("https://md.example.com/api/health?full=1");
81+
const res = await worker.fetch(req, env, mockCtx());
82+
const payload = await res.json() as { browser?: unknown; metrics?: unknown };
83+
expect(res.status).toBe(200);
84+
expect(payload.browser).toBeUndefined();
85+
expect(payload.metrics).toBeUndefined();
86+
});
87+
5688
it("serves health endpoint for HEAD without conversion", async () => {
5789
const fetchMock = vi.fn();
5890
vi.stubGlobal("fetch", fetchMock);

src/__tests__/index-health-fallback.test.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ afterEach(() => {
5050
});
5151

5252
describe("/api/health defensive fallback", () => {
53-
it("returns 200 even when stats providers throw", async () => {
53+
it("returns 200 even when stats providers throw (full health path)", async () => {
5454
mocked.getBrowserCapacityStats.mockImplementation(() => {
5555
throw new Error("browser stats unavailable");
5656
});
@@ -59,8 +59,10 @@ describe("/api/health defensive fallback", () => {
5959
});
6060

6161
const res = await worker.fetch(
62-
new Request("https://md.example.com/api/health"),
63-
createMockEnv().env,
62+
new Request("https://md.example.com/api/health?full=1", {
63+
headers: { Authorization: "Bearer admin-token" },
64+
}),
65+
createMockEnv({ API_TOKEN: "admin-token" }).env,
6466
mockCtx(),
6567
);
6668
const payload = await res.json() as {

src/__tests__/portal-keys.test.ts

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,8 @@ describe("Portal: /api/keys create", () => {
218218
expect(res.status).toBe(201);
219219
const body = await res.json() as any;
220220
expect(body.key).toMatch(/^mk_[0-9a-f]{64}$/);
221-
expect(body.prefix).toMatch(/^[0-9a-f]{8}$/);
221+
// Prefix format matches GET /api/keys for client consistency
222+
expect(body.prefix).toMatch(/^mk_[0-9a-f]{8}\.\.\.$/);
222223
expect(body.name).toBe("test-key");
223224
expect(body.warning).toBeTruthy();
224225
});
@@ -274,7 +275,9 @@ describe("Portal: /api/keys create", () => {
274275
headers: { Cookie: `md_session=${sessionToken}` },
275276
});
276277
const res = await worker.fetch(req, env, mockCtx());
277-
expect(res.status).toBe(400);
278+
// 409 Conflict is semantically correct for a quota violation,
279+
// distinguishing it from 400 (malformed input)
280+
expect(res.status).toBe(409);
278281
const body = await res.json() as any;
279282
expect(body.error).toBe("Too Many Keys");
280283
});
@@ -387,26 +390,33 @@ describe("Portal: /api/keys revoke", () => {
387390
expect(res.status).toBe(404);
388391
});
389392

390-
it("returns 400 when key is already revoked", async () => {
393+
it("is idempotent when key is already revoked (returns existing timestamp)", async () => {
391394
const d1 = createMockD1();
392395
const { accountId, sessionToken } = await seedAccountAndSession(d1);
393396
const keyId = crypto.randomUUID();
397+
const originalRevokedAt = "2026-04-10T00:00:00.000Z";
394398
d1.tables.api_keys.set(keyId, {
395399
id: keyId,
396400
account_id: accountId,
397401
prefix: "test1234",
398402
key_hash: "hash",
399403
name: null,
400404
created_at: new Date().toISOString(),
401-
revoked_at: new Date().toISOString(),
405+
revoked_at: originalRevokedAt,
402406
});
403407
const { env } = createMockEnv({ AUTH_DB: d1.db });
404408
const req = new Request(`https://md.example.com/api/keys/${keyId}`, {
405409
method: "DELETE",
406410
headers: { Cookie: `md_session=${sessionToken}` },
407411
});
408412
const res = await worker.fetch(req, env, mockCtx());
409-
expect(res.status).toBe(400);
413+
// DELETE is idempotent by REST convention: a second delete of a
414+
// revoked key returns 200 with the original revocation timestamp,
415+
// not an error.
416+
expect(res.status).toBe(200);
417+
const body = await res.json() as any;
418+
expect(body.id).toBe(keyId);
419+
expect(body.revoked_at).toBe(originalRevokedAt);
410420
});
411421
});
412422

src/config.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ export const CF_BLOCKED_DOMAINS_TTL = 2 * 60 * 60;
5959

6060
export const CORS_HEADERS: Record<string, string> = {
6161
"Access-Control-Allow-Origin": "*",
62-
"Access-Control-Allow-Methods": "GET, POST, OPTIONS",
62+
"Access-Control-Allow-Methods": "GET, POST, DELETE, OPTIONS",
6363
"Access-Control-Allow-Headers": "Accept, Content-Type, Authorization, Idempotency-Key",
6464
"Access-Control-Max-Age": "86400",
6565
};

src/handlers/auth.ts

Lines changed: 73 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,50 @@ import {
3434

3535
const TOKEN_TTL_MINUTES = 15;
3636
const EMAIL_RATE_LIMIT_PER_HOUR = 3;
37+
const IP_RATE_LIMIT_PER_HOUR = 10; // Per-IP cap to prevent cross-email abuse
38+
const MAX_BODY_BYTES = 1024; // Reject oversized magic-link request bodies
3739
const EMAIL_REGEX = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
3840
const MAX_EMAIL_LENGTH = 254;
3941

42+
/**
43+
* Increment a Cache-API-backed counter at `key` with `ttlSeconds` expiry.
44+
* Returns the new count. Non-atomic across isolates but good enough for
45+
* abuse prevention (a distributed attacker can still trivially stay under
46+
* the limit across isolates; this mostly prevents single-source bursts).
47+
*/
48+
async function incrementRateCounter(key: string, ttlSeconds: number): Promise<number> {
49+
if (typeof caches === "undefined") return 0;
50+
try {
51+
const req = new Request(key);
52+
const existing = await caches.default.match(req);
53+
const current = existing ? Number(await existing.text()) || 0 : 0;
54+
const next = current + 1;
55+
await caches.default.put(req, new Response(String(next), {
56+
headers: { "Cache-Control": `max-age=${ttlSeconds}` },
57+
}));
58+
return next;
59+
} catch {
60+
return 0;
61+
}
62+
}
63+
64+
async function readRateCounter(key: string): Promise<number> {
65+
if (typeof caches === "undefined") return 0;
66+
try {
67+
const existing = await caches.default.match(new Request(key));
68+
if (!existing) return 0;
69+
return Number(await existing.text()) || 0;
70+
} catch {
71+
return 0;
72+
}
73+
}
74+
75+
function getClientIp(request: Request): string {
76+
return request.headers.get("cf-connecting-ip")
77+
|| request.headers.get("x-forwarded-for")?.split(",")[0]?.trim()
78+
|| "unknown";
79+
}
80+
4081
// ─── POST /api/auth/magic-link ──────────────────────────────
4182

4283
export async function handleSendMagicLink(
@@ -48,9 +89,15 @@ export async function handleSendMagicLink(
4889
return Response.json({ error: "Service Unavailable" }, { status: 503, headers: CORS_HEADERS });
4990
}
5091

92+
// Body size cap — prevent memory pressure from giant JSON payloads
93+
const bodyText = await request.text();
94+
if (bodyText.length > MAX_BODY_BYTES) {
95+
return Response.json({ error: "Request too large" }, { status: 413, headers: CORS_HEADERS });
96+
}
97+
5198
let email: string;
5299
try {
53-
const body = await request.json() as { email?: string };
100+
const body = JSON.parse(bodyText) as { email?: string };
54101
email = String(body?.email || "").trim().toLowerCase();
55102
} catch {
56103
return Response.json({ error: "Invalid JSON body" }, { status: 400, headers: CORS_HEADERS });
@@ -60,18 +107,21 @@ export async function handleSendMagicLink(
60107
return Response.json({ error: "Invalid email address" }, { status: 400, headers: CORS_HEADERS });
61108
}
62109

63-
// Rate limit via Cache API (per email, per hour)
64-
const rateLimitKey = `https://md-rate-limit/magic-link/${encodeURIComponent(email)}`;
65-
let count = 0;
66-
if (typeof caches !== "undefined") {
67-
try {
68-
const existing = await caches.default.match(new Request(rateLimitKey));
69-
if (existing) {
70-
count = Number(await existing.text()) || 0;
71-
}
72-
} catch { /* ignore */ }
73-
}
74-
if (count >= EMAIL_RATE_LIMIT_PER_HOUR) {
110+
// ── Rate limiting (two-dimensional) ─────────────────────────
111+
// 1) Per-email cap: prevents mailbox flooding for a specific victim
112+
// 2) Per-IP cap: prevents a single attacker from burning Resend quota
113+
// by rotating across many victim emails
114+
const emailKey = `https://md-rate-limit/magic-link/email/${encodeURIComponent(email)}`;
115+
const ip = getClientIp(request);
116+
const ipKey = `https://md-rate-limit/magic-link/ip/${encodeURIComponent(ip)}`;
117+
118+
const [emailCount, ipCount] = await Promise.all([
119+
readRateCounter(emailKey),
120+
readRateCounter(ipKey),
121+
]);
122+
123+
// Silently 200 on rate limit (don't reveal which dimension tripped)
124+
if (emailCount >= EMAIL_RATE_LIMIT_PER_HOUR || ipCount >= IP_RATE_LIMIT_PER_HOUR) {
75125
return Response.json(
76126
{ ok: true, note: "If an account exists, an email was sent." },
77127
{ status: 200, headers: CORS_HEADERS },
@@ -118,15 +168,14 @@ export async function handleSendMagicLink(
118168
console.warn("RESEND_API_KEY not configured — magic link logged only:", verifyUrl);
119169
}
120170

121-
// Increment rate limit counter
122-
if (typeof caches !== "undefined") {
123-
try {
124-
const newCount = count + 1;
125-
const resp = new Response(String(newCount), {
126-
headers: { "Cache-Control": `max-age=${60 * 60}` },
127-
});
128-
await caches.default.put(new Request(rateLimitKey), resp);
129-
} catch { /* ignore */ }
171+
// Increment both rate limit counters (1-hour TTL)
172+
const ONE_HOUR = 60 * 60;
173+
try {
174+
await Promise.all([
175+
incrementRateCounter(emailKey, ONE_HOUR),
176+
incrementRateCounter(ipKey, ONE_HOUR),
177+
]);
178+
} catch { /* ignore */
130179
}
131180

132181
return Response.json({
@@ -256,6 +305,7 @@ export async function handleVerifyMagicLink(
256305
headers: {
257306
Location: `https://${host}/portal/`,
258307
"Set-Cookie": buildSessionCookie(sessionToken, sessionExpires),
308+
"Cache-Control": "no-store, private",
259309
},
260310
});
261311
} catch (err) {
@@ -279,6 +329,7 @@ export async function handleLogout(
279329
headers: {
280330
"Content-Type": "application/json",
281331
"Set-Cookie": buildClearCookie(),
332+
"Cache-Control": "no-store, private",
282333
...CORS_HEADERS,
283334
},
284335
});

src/handlers/health.ts

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,64 @@
11
// 健康检查端点
22

3+
import type { Env } from "../types";
34
import { CORS_HEADERS } from "../config";
45
import { getBrowserCapacityStats } from "../browser";
56
import { getPaywallRuleStats } from "../paywall";
67
import { buildOperationalMetricsSnapshot } from "../observability/metrics";
78
import { runtimeStartedAt, runtimeCounters } from "../runtime-state";
89
import { errorMessage } from "../utils";
10+
import { isAuthorizedByToken } from "../middleware/auth";
911

12+
/**
13+
* Default /api/health handler — returns public-only info.
14+
* Callers who need full operational metrics should use /api/health?full=1
15+
* with an API_TOKEN Bearer header (see handleHealthRoute).
16+
*/
1017
export function handleHealth(host: string): Response {
18+
return handlePublicHealth(host);
19+
}
20+
21+
/**
22+
* Router entry: decides between public and full based on ?full=1 + auth.
23+
*/
24+
export async function handleHealthRoute(request: Request, env: Env, host: string): Promise<Response> {
25+
const url = new URL(request.url);
26+
if (url.searchParams.get("full") === "1") {
27+
return handleFullHealth(request, env, host);
28+
}
29+
return handlePublicHealth(host);
30+
}
31+
32+
/**
33+
* Public health check — minimal, no internal state.
34+
* Returns just { status, service, uptime_seconds } so that monitoring
35+
* tools can liveness-check without leaking operational capacity data.
36+
*/
37+
export function handlePublicHealth(host: string): Response {
38+
return Response.json({
39+
status: "ok",
40+
service: host,
41+
uptime_seconds: Math.max(0, Math.floor((Date.now() - runtimeStartedAt) / 1000)),
42+
}, { headers: CORS_HEADERS });
43+
}
44+
45+
/**
46+
* Full health with browser capacity, paywall rule counts, metric counters.
47+
* Requires API_TOKEN (admin) to prevent attackers from probing queue depth,
48+
* rate limit state, browser concurrency, etc.
49+
*/
50+
export async function handleFullHealth(request: Request, env: Env, host: string): Promise<Response> {
51+
// Require admin auth for full metrics
52+
if (env.API_TOKEN) {
53+
const authorized = await isAuthorizedByToken(request, env.API_TOKEN);
54+
if (!authorized) {
55+
return handlePublicHealth(host);
56+
}
57+
}
58+
return buildFullHealthResponse(host);
59+
}
60+
61+
function buildFullHealthResponse(host: string): Response {
1162
let browserStats: {
1263
active: number;
1364
queued: number;

0 commit comments

Comments
 (0)