Bound the /api/prices/[slab] fallback cache - #2470
Conversation
The route's module-level `fallbackCache` was a plain Map with no size cap. The GeckoTerminal fallback path serves any slab and writes an entry on every miss (including null misses), keyed by the attacker-controllable slab; TTL is checked only on read and nothing else evicts. A flood of distinct slabs (GET /api/prices/<random pubkey>) therefore grew the Map without bound (memory-exhaustion DoS on a warm instance), while also amplifying each miss into an internal /api/markets fetch and a GeckoTerminal fetch. Cap every write with boundedSet(..., FALLBACK_CACHE_MAX_ENTRIES = 10_000) — the same LRU bound the sibling /api/chart route already uses for its GeckoTerminal caches. Cached values and TTL behavior are unchanged for realistic loads. - import boundedSet; add FALLBACK_CACHE_MAX_ENTRIES - route all fallbackCache writes (Pyth + GeckoTerminal paths) through boundedSet - add a regression test asserting the cap holds and evicts oldest-first under a flood of distinct slabs, where a raw Map.set does not Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@0x-SquidSol is attempting to deploy a commit to the Khubair Nasir's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe prices route now limits its fallback cache to 10,000 entries. Pyth and GeckoTerminal writes use ChangesPrices cache bounding
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/__tests__/api/prices-cache-bounded.test.ts (1)
30-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftTest the route cache contract.
These tests call
boundedSetdirectly. They pass if a route cache write changes back tofallbackCache.set.Exercise the Pyth and GeckoTerminal fallback paths through the route, or extract a testable cache adapter. Assert that all write outcomes retain the 10,000-entry limit. Also assert that
gt:slab_0throughgt:slab_4are absent and thatgt:slab_5through the newest key remain present.🤖 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 `@app/__tests__/api/prices-cache-bounded.test.ts` around lines 30 - 40, The tests currently validate boundedSet directly rather than the route cache contract. Exercise the Pyth and GeckoTerminal fallback paths through the route, or expose a testable cache adapter, then assert every write path enforces the 10,000-entry limit; verify gt:slab_0 through gt:slab_4 are absent and gt:slab_5 through the newest key remain present.
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@app/__tests__/api/prices-cache-bounded.test.ts`:
- Around line 30-40: The tests currently validate boundedSet directly rather
than the route cache contract. Exercise the Pyth and GeckoTerminal fallback
paths through the route, or expose a testable cache adapter, then assert every
write path enforces the 10,000-entry limit; verify gt:slab_0 through gt:slab_4
are absent and gt:slab_5 through the newest key remain present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3cac532e-fbe9-4280-b812-684384ef26a9
📒 Files selected for processing (2)
app/__tests__/api/prices-cache-bounded.test.tsapp/app/api/prices/[slab]/route.ts
dcccrypto
left a comment
dcccrypto
left a comment
There was a problem hiding this comment.
Reviewed on the PR head (ef61e030). The fix is correct; same test-binding gap
as #2467, plus the new test duplicates coverage that already exists.
Verified locally
Merge Gate green is not evidence on a fork PR — Unit Tests, Integration Tests,
Security Tests and Coverage Gate all report SKIPPED while the gate reports
SUCCESS (#2447). Ran it directly:
cd app && npx vitest run
Test Files 283 passed | 1 skipped (284)
Tests 2922 passed | 16 skipped (2938)
The fix itself checks out: all five fallbackCache write sites are converted
(both the pythStatsFallback null/parse/finite branches and the GeckoTerminal
setCache), TTL and cached values are unchanged, and 10 000 matches the sibling
chart route's cap.
The three red Vercel checks are the fork-authorization prompt again
(target_url is vercel.com/git/authorize), not a build failure.
The test does not bind the fix — and it is already covered elsewhere
Two separate problems with prices-cache-bounded.test.ts:
1. It tests boundedSet, which __tests__/lib/bounded-map.test.ts already
tests — more thoroughly. That existing file asserts the cap holds over 10 000
inserts, oldest-first FIFO eviction, and two cases this PR's test doesn't:
hot-key refresh (re-writing a key moves it to newest, so it survives eviction)
and update-does-not-grow. The new file's only addition is a new Map() control
demonstrating that an unbounded Map is unbounded.
2. It never imports the route, so it cannot fail if the wiring is removed.
I reverted all five call sites back to fallbackCache.set(...) — GH#2469 fully
restored — and:
prices-cache-bounded.test.ts 3 passed <- vulnerability restored, still green
prices-cache-bounded-route-enforcement 3 failed <- catches it
Drop-in below. It drives the real GET handler with every upstream stubbed to
502, which is the attacker-cheap null-miss write path the issue describes, and
asserts the route's own cache object is written through a finite bound. 3/3 with
the fix, 3/3 under the revert.
One gotcha worth keeping in the comments: fallbackCache is module-level with a
60s TTL, so a slab used by an earlier test is a cache hit in a later one and
performs no write at all. Each test therefore needs its own slabs — reusing one
yields zero boundedSet calls and a spy assertion that looks fine but checks
nothing. That bit me while writing this.
app/__tests__/api/prices-cache-bounded-route-enforcement.test.ts
/**
* Regression: the /api/prices/[slab] fallback cache must be bounded BY THE ROUTE.
*
* prices-cache-bounded.test.ts asserts that `boundedSet` caps a Map — but that is
* already covered by __tests__/lib/bounded-map.test.ts, and neither test imports
* the route. Reverting every `boundedSet(fallbackCache, ...)` back to
* `fallbackCache.set(...)` — i.e. fully restoring GH#2469 — leaves both green.
*
* This test drives the real GET handler and asserts the route's own cache write
* goes through the bound, with a finite cap. It fails if the wiring is reverted,
* which is the binding the library-level tests cannot provide.
*/
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import { NextRequest } from "next/server";
// importActual bypasses the mock below — a plain import would resolve to the
// spy and recurse infinitely.
const { boundedSet: realBoundedSet } =
await vi.importActual<typeof import("@/lib/bounded-map")>("@/lib/bounded-map");
// Spy that still performs the real bounding, so behaviour is unchanged.
const boundedSetSpy = vi.fn(realBoundedSet);
vi.mock("@/lib/bounded-map", () => ({
boundedSet: (...args: Parameters<typeof realBoundedSet>) => boundedSetSpy(...args),
}));
vi.mock("@sentry/nextjs", () => ({ captureException: vi.fn(), captureMessage: vi.fn() }));
const { GET } = await import("@/app/api/prices/[slab]/route");
// Valid base58 pubkeys that are NOT in PLAYGROUND_SLAB_META, so the request
// reaches the GeckoTerminal fallback — the path that serves ANY slab.
//
// NOTE: `fallbackCache` is module-level and its TTL is 60s, so a slab used by an
// earlier test is a cache HIT in a later one and performs no write at all. Every
// test below therefore uses its own slabs; reusing one silently produces zero
// boundedSet calls and a confusingly "passing-looking" spy.
const SLAB_A = "DJ54k4wH92NTtNP8RuHAwG8si1bevXEknzctDdqYN8eC";
const SLAB_B = "LBUZKhRxPF3XUpBCjp4YzTKgLccjZhTSDM9YuVaPwxo";
const SLABS_C = [
"pAMMBay6oceH9fJKBRHGP5D4bD4sWpmSwMn52FMfXEA",
"TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA",
"ATokenGPvbdGVxr1b2hvZbsiqW5xWH25efTNsLJA8knL",
];
const get = (slab: string) =>
GET(new NextRequest(`http://localhost/api/prices/${slab}`), {
params: Promise.resolve({ slab }),
});
describe("prices/[slab] routes its cache writes through the bound", () => {
beforeEach(() => {
boundedSetSpy.mockClear();
// Every upstream fails → the fallback caches a null miss, which is exactly
// the attacker-cheap write path described in GH#2469.
vi.stubGlobal(
"fetch",
vi.fn(async () => new Response("nope", { status: 502 })),
);
});
afterEach(() => {
vi.unstubAllGlobals();
});
it("writes the fallback cache via boundedSet, never a raw Map.set", async () => {
const res = await get(SLAB_A);
expect(res.status).toBe(200);
expect(boundedSetSpy).toHaveBeenCalled();
});
it("passes a finite cap, and the cap bounds the route's own cache object", async () => {
await get(SLAB_B);
const [map, , , maxEntries] = boundedSetSpy.mock.calls.at(-1)!;
expect(Number.isFinite(maxEntries)).toBe(true);
expect(maxEntries).toBeGreaterThan(0);
// Flood the ROUTE'S OWN cache object (captured from the call) past the cap
// it was given. An unbounded cache would grow 1:1 with attacker slabs.
const before = (map as Map<string, unknown>).size;
for (let i = 0; i < (maxEntries as number) + 500; i++) {
realBoundedSet(map as Map<string, unknown>, `flood:${i}`, { v: i }, maxEntries as number);
}
expect((map as Map<string, unknown>).size).toBeLessThanOrEqual(maxEntries as number);
expect((map as Map<string, unknown>).size).toBeGreaterThanOrEqual(
Math.min(before, maxEntries as number),
);
});
it("caches distinct slabs under distinct keys (no accidental key collapse)", async () => {
for (const s of SLABS_C) await get(s);
const keys = boundedSetSpy.mock.calls.map((c) => c[1]);
expect(new Set(keys).size).toBe(SLABS_C.length);
});
});Happy to push it here or send it as a follow-up — your call.
While verifying, I swept the rest of the routes
Every module-level Map under app/api, checked for a cap or prune. Seven more
are unbounded, same class as this one — all get/set only, no delete, no
prune, no cap:
| Route | Map | Key |
|---|---|---|
playground/faucet |
claimStore |
wallet |
auto-fund |
_autoFundClaims |
wallet |
faucet |
_faucetClaims |
wallet:type |
devnet-airdrop |
_airdropClaims |
wallet:mint |
devnet-register-mint |
requestCounts |
client IP |
markets/[slab]/logo |
uploadTimestamps |
slab |
tokens/[mint]/logo |
uploadTimestamps |
mint |
Calibrating honestly, these are materially weaker than #2469 and I would not
call any of them a DoS on today's evidence:
- Entries are
string → number, not cached objects, so per-key cost is tiny. - Writes are gated behind expensive work — an airdrop/claim transaction, or a
successful image upload — not a bareGET. - Four of them (
faucet,auto-fund,devnet-airdrop, and the mirror-mint
fallback) only populate when Supabase is unavailable, per their own comments. devnet-register-mintkeys ongetClientIp, which peels trusted hops from the
right, so the key is the real client IP and not spoofable under correct
TRUSTED_PROXY_DEPTH.
So: same class, much lower severity, one-line fix each with the helper that now
exists. Worth closing as a batch rather than one PR per route. I'll file an
inventory issue so it does not get rediscovered a route at a time.
For completeness, these were checked and are bounded: rpc (MAX_CACHE_SIZE
500 + inflightRequests.delete on settle), oracle/resolve/[ca] (256 / 64),
devnet-mirror-mint (prunes expired on each request), chart/[mint]
(boundedSet), and markets' MINT_TO_KNOWN_SYMBOL (a static constant).
2 participants
What
Bound the
/api/prices/[slab]in-memory fallback cache so a flood of distinctslabs can no longer grow it without limit.
Closes #2469.
Why
fallbackCachewas a plain module-levelMapwith no size cap. The GeckoTerminalfallback path serves any slab and writes an entry on every miss (including null
misses), keyed by the attacker-controllable slab; TTL is checked only on read and
nothing else evicts.
GET /api/prices/<random pubkey>in a loop therefore growsthe Map without bound (memory-exhaustion DoS on a warm instance) and amplifies each
miss into an internal
/api/markets/<slab>fetch plus a GeckoTerminal fetch. Thesibling
/api/chart/[mint]route already caps its GeckoTerminal caches withboundedSet; this route was missed.Changes
boundedSet; addFALLBACK_CACHE_MAX_ENTRIES = 10_000fallbackCachewrites (Pyth + GeckoTerminal paths) throughboundedSetflood of distinct slabs, where a raw
Map.setdoes notCached values and TTL behavior are unchanged for realistic loads (well under the
10k cap of distinct slabs).
Testing
npx tsc --noEmit— clean.boundedSethas its own existing testcoverage (
__tests__/lib/bounded-map.test.ts).Notes
Summary by CodeRabbit