Skip to content

Commit f9215a7

Browse files
schmugclaude
andcommitted
security: scope this PR to the per-analyzer DoS caps; drop the rate-limit write-race fix (superseded by #549)
The Cache-API await fix here (GHSA-v7qc-7qh8-h69g) is only a partial mitigation; #549 fully supersedes it with an atomic Durable Object rate limiter and deletes the checkRateLimitCache path this touched. Revert the rate-limit.ts change, the rateLimitMiddleware hunk in index.ts, and the f30 test so this PR scopes cleanly to the per-analyzer caps that nothing else duplicates: DKIM selector count/length (GHSA-6fqp-4vhc-59mf) and MTA-STS policy body size (GHSA-p676-gc7j-96mx). Avoids a same-line conflict with #549's middleware rewrite. Signed-off-by: schmug <38227427+schmug@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 2379a8f commit f9215a7

3 files changed

Lines changed: 9 additions & 59 deletions

File tree

src/index.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -409,9 +409,10 @@ type RateLimitBlockedResponder = (
409409
export function rateLimitMiddleware(onBlocked: RateLimitBlockedResponder) {
410410
return async (c: Context, next: () => Promise<void>) => {
411411
const { identity, config } = await resolveRateLimitScope(c);
412-
// checkRateLimit now awaits the counter write internally (GHSA-v7qc-7qh8-h69g),
413-
// so there is no deferred write to hand to waitUntil here.
414412
const result = await checkRateLimit(identity, config);
413+
if (result.pendingWrite) {
414+
c.executionCtx.waitUntil(result.pendingWrite.catch(() => {}));
415+
}
415416

416417
const headers = rateLimitHeaders(result);
417418

src/rate-limit.ts

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ export interface RateLimitResult {
99
limit: number;
1010
windowSec: number;
1111
resetAt: number;
12+
pendingWrite?: Promise<void>;
1213
}
1314

1415
export type PlanTier = "free" | "pro";
@@ -100,23 +101,18 @@ async function checkRateLimitCache(
100101
"Cache-Control": `s-maxage=${ttl}`,
101102
},
102103
});
103-
// SECURITY (GHSA-v7qc-7qh8-h69g): await the counter write before returning.
104-
// Deferring it via executionCtx.waitUntil() (a prior perf optimization)
105-
// widened the read-modify-write race: a sequential burst from one identity in
106-
// the same isolate could each read a stale count before any write landed,
107-
// bypassing the limit. Awaiting closes that intra-isolate window. NOTE: the
108-
// Cache API offers no atomic increment/CAS, so a cross-isolate/cross-colo race
109-
// remains; fully closing it needs a Durable Object or the native Rate Limiting
110-
// binding (tracked in the advisory). The small write latency is the accepted
111-
// cost of a correctly-incrementing counter.
112-
await cache.put(key, response);
104+
// ⚡ Bolt Optimization: Do not await cache.put on the critical path.
105+
// Return the promise so the caller can pass it to executionCtx.waitUntil(),
106+
// removing Cache API write latency from every rate-limited request.
107+
const pendingWrite = cache.put(key, response);
113108

114109
return {
115110
allowed,
116111
remaining,
117112
limit: config.limit,
118113
windowSec: config.windowSec,
119114
resetAt,
115+
pendingWrite,
120116
};
121117
}
122118

test/rate-limit.test.ts

Lines changed: 0 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -232,51 +232,4 @@ describe("rate-limit", () => {
232232
expect(headers["X-RateLimit-Remaining"]).toBe("0");
233233
});
234234
});
235-
236-
// f30 / GHSA-v7qc-7qh8-h69g — the Cache-API counter write must be awaited
237-
// before the result is returned, not deferred via executionCtx.waitUntil().
238-
// Deferring it (the prior "Bolt" optimization) widened the read-modify-write
239-
// race so a sequential burst within one isolate could read a stale count.
240-
// Awaiting the put closes the intra-isolate window. (Cross-isolate atomicity
241-
// is a fundamental Cache-API limitation and a separate, larger fix.)
242-
describe("Cache API path — counter write durability (f30)", () => {
243-
let store: Map<string, string>;
244-
let putResolved: boolean;
245-
let putCalls: number;
246-
247-
beforeEach(() => {
248-
store = new Map();
249-
putResolved = false;
250-
putCalls = 0;
251-
const mockCache = {
252-
match: async (key: Request) => {
253-
const val = store.get(key.url);
254-
return val === undefined ? undefined : new Response(val);
255-
},
256-
put: async (key: Request, resp: Response) => {
257-
putCalls++;
258-
await Promise.resolve(); // simulate async write latency
259-
store.set(key.url, await resp.text());
260-
putResolved = true;
261-
},
262-
};
263-
vi.stubGlobal("caches", { default: mockCache });
264-
});
265-
266-
it("awaits the cache write before returning (no deferred pendingWrite)", async () => {
267-
const result = await checkRateLimit("ip:9.9.9.9", FREE);
268-
expect(putCalls).toBe(1);
269-
// If the write were deferred (handed back for waitUntil instead of
270-
// awaited), putResolved would still be false at this point.
271-
expect(putResolved).toBe(true);
272-
expect(result.allowed).toBe(true);
273-
expect(result.remaining).toBe(FREE.limit - 1);
274-
});
275-
276-
it("persists the incremented count for the next sequential request", async () => {
277-
await checkRateLimit("ip:9.9.9.9", FREE);
278-
const second = await checkRateLimit("ip:9.9.9.9", FREE);
279-
expect(second.remaining).toBe(FREE.limit - 2);
280-
});
281-
});
282235
});

0 commit comments

Comments
 (0)