fix(launch): 13 fixes — a11y, bounded caches, network scoping, fail-closed validation, next 16.2.11 - #2527
Merged
Merged
Conversation
The four social links (GitHub / X / Discord / Telegram) carried `title` only. `title` is not a reliable accessible name — screen-reader support varies and it is invisible on keyboard focus — so each link announced as bare "link" and the decorative SVG glyph was exposed to the a11y tree. Each anchor now carries an explicit `aria-label`, and each glyph is `aria-hidden="true" focusable="false"`. Note the issue says "Header social icons"; they actually live in `components/layout/Footer.tsx`. The defect is exactly as described, only the file is different — `components/Header.tsx` does not exist. Test asserts one label per social plus a count invariant (labelled links <= aria-hidden glyphs), so adding a fifth link without hiding its glyph fails. Negative control run: stripping the attributes fails all 5. Launch suite: 3056 passed, 16 skipped, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
`entries` was only ever written, never pruned. Scoping the claim first: this was NOT a socket leak. The WebSocket and message subscriptions are already released when the last listener unsubscribes (`releaseWs`/`releaseMsg`, priceStore.ts:284-287). What grew without bound was the deliberately-retained snapshot — kept so a quick remount or market-switch-back does not flash back to loading — one small object per distinct slab visited. Cap at 64 and evict oldest-first, but ONLY entries with zero listeners. Map iteration is insertion-ordered, so the walk is oldest-first, and an entry with a live subscriber can never be evicted — the cap cannot disturb an open market. 64 is far above any realistic switch-back working set, so the no-flash behaviour the comment describes is preserved. Test drives the public API only: pins a subscribed slab, churns 300 idle ones past the cap, then asserts the pinned entry survives AND the earliest idle one is gone (unbounded growth would have kept it). Negative control run: removing the eviction call fails it. Launch suite: 3057 passed, 16 skipped, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
…e pane `hasVolumeData` gated the pane on `(c.volume ?? 0) > 0`. NaN was already excluded — NaN > 0 is false. The live hole was Infinity: `Infinity > 0` is TRUE, so one corrupt candle from external data enabled the volume pane and handed Infinity to the histogram series, which then scales the entire pane off that value. Guard on Number.isFinite first, then positivity. Test mirrors the predicate rather than importing TradingChart (which pulls in lightweight-charts + a canvas). It covers the case the old guard let through (Infinity, both signs), the cases it already caught (NaN, missing), the positive control, and a mixed array where one corrupt candle must not enable the pane. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
`setTimeout(() => refreshSlab?.(), 2000)` fired after a deposit with nothing holding the handle. If the user navigates inside that 2s window the callback runs against an unmounted tree. Timers are now tracked on a ref and cleared by an unmount effect. Scope note: #2323 inventories 11 orphaned timeouts across several hooks. This fixes the useDeposit one only (the hook that had NO clearTimeout at all — useAutoDeposit, useChartDrawings, useDuplicateMarket, usePortfolio and usePriceFlash already clear theirs). The remaining sites are listed on the issue and it stays open for them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
`trades` and `funding_history` both carry a `network` column, but no query in
indexer-db.ts used it as a predicate — every WHERE filtered only on
slab_address / trader / created_at. A devnet deployment could therefore serve
mainnet rows and vice versa.
All 12 query sites now carry `AND network = ${getServerNetwork()}`.
I first concluded this was blocked on #2229 (no server-side network source) and
said so on the issue. That was WRONG and I have corrected it there.
`getServerNetwork()` (lib/supabase.ts:19) reads NEXT_PUBLIC_DEFAULT_NETWORK, works
server-side, fail-safes to devnet, and was ALREADY used in 12 files — including
api/stake/pools/route.ts:310, which does exactly this scoping. I had checked only
config.ts's localStorage-based getNetwork() and stopped there.
The automated pass patched 10 of 12; the two COUNT queries use a single-line
`SELECT COUNT(*) FROM trades` form the line-oriented matcher missed. Patched by
hand. A partial fix would have been worse than none here — one unfiltered query is
enough to leak cross-network rows, the same way one unbounded write site would
have kept #2469 open.
Test asserts the INVARIANT structurally (every FROM has a predicate within its
statement) rather than testing the current queries' values, because the real
failure mode is query #13 being added without the filter. Includes a
non-vacuity check (>= 12 sites found) and a guard that the SERVER resolver is
imported, not the localStorage one. Negative control run: removing a single
filter fails it.
Launch suite: 3064 passed / 16 skipped / 0 failed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
…th it exposes
`fetch(getRpcUrl(...))` had no `signal`, so it inherited the platform default
(effectively none) and one unresponsive upstream held the route open until the
function itself timed out.
That is worse than a single slow request. Read-only calls are DEDUPLICATED on
`cacheKey`, so every later caller for the same method awaits the SAME promise
rather than issuing its own — one hung upstream call stalls all of them together.
Adds `AbortSignal.timeout(RPC_UPSTREAM_TIMEOUT_MS)` at 15s: above p99 for a
healthy provider, well under the platform function limit, so a hung upstream
surfaces as a clean error instead of consuming the whole invocation budget.
SECOND-ORDER FIX, which the timeout is what makes necessary. The deduped branch
awaits the shared promise OUTSIDE the try/catch:
const result = await inflightRequests.get(cacheKey)!;
BUG 14 deliberately converts upstream failures into a JSON-RPC error object so
one bad item cannot fail an entire Promise.all batch — but that protection only
covers the primary path. While the fetch merely HUNG, this await rarely rejected.
Adding a timeout makes rejection the normal outcome of a slow upstream, so the
dedup path would newly fail whole batches. It now returns the same well-formed
JSON-RPC error.
The existing `finally { inflightRequests.delete(cacheKey) }` already prevents a
rejected promise being cached for later callers, so no change was needed there.
Launch suite: 3064 passed / 16 skipped / 0 failed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
Every existing check on this route was SHAPE-only: valid PublicKey, printable name, ticker charset, decimals range. A syntactically valid pubkey for an account that was never created still upserted into `devnet_mints`, so the registry could carry entries with no mint behind them. Now fetches the account and requires it to be an SPL mint: owned by TOKEN_PROGRAM_ID and exactly 82 bytes. Same shape /api/devnet-airdrop already uses before trusting a mirror row. FAILS CLOSED on RPC error (503) rather than skipping the check. An advisory check that waves the request through when the RPC is down would leave this only half-closed — the failure mode it exists to catch is exactly when things are degraded. Scope: this is EXISTENCE, not ownership. It does not prove the caller controls the mint; that needs a signature and a client-contract change, tracked on the issue. Also corrected my own triage on the issue: I first grepped this file for `validate|verify|check|require`, got ZERO matches, and read that as "no validation at all". The helpers are named isPrintableName / isTicker / isRateLimited — a naming mismatch, not an absence. A zero-match grep is not evidence of absence. Test asserts the fetch, both rejection branches, the fail-closed 503, and that the check runs BEFORE the upsert. Negative control run: removing it fails 4 of 5. Launch suite: 3073 passed / 16 skipped / 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
if (distancePct <= 10) return "danger";
if (distancePct <= 30) return "warning";
return "safe";
Both comparisons are FALSE for NaN, so an unguarded non-finite distance fell
straight through to "safe" — a position at liquidation risk rendering as fine.
That is the one direction a risk indicator must never fail in.
NaN reaches here from ordinary upstream arithmetic, not from corruption: 0/0 when
a position's notional is momentarily zero mid-refresh, or a subtraction against an
undefined mark before the first oracle tick lands.
Fails to "danger". An absent risk signal is not evidence of safety, and the
asymmetry is the argument: a spurious warning is noise, a suppressed one is a
liquidation the user never saw coming. +Infinity is the only non-finite value that
would prefer "safe", and it is not worth a special case against that downside.
Test covers NaN, both infinities, the fail-direction (danger not merely warning),
and — importantly — that the guard did not FLATTEN the real classification: 5/10
danger, 20/30 warning, 31/100 safe. Negative control run: removing the guard fails
3 of 4, and the value-classification test correctly still passes.
Launch suite: 3077 passed / 16 skipped / 0 failed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
The last-resort throw said only:
"Unsupported market registration payload value"
`encodeCanonicalJson` is RECURSIVE, so that one sentence was the entire signal for
a bad value buried anywhere in the registration payload — the caller surfaced it as
an opaque "market creation failed" with nothing to act on.
Now reports `typeof value` and names the reachable causes (bigint, function,
symbol), so a creator or an integrator can find the field.
Deliberately does NOT interpolate the value itself. This error can reach a client
response and the logs, and the payload carries user-supplied market metadata; the
TYPE is enough to identify the culprit without putting contents anywhere. The test
asserts that absence explicitly, so a later "make it more helpful" change cannot
quietly start leaking.
The sibling throw for non-plain-object prototypes is unchanged.
Test: 4 assertions incl. the no-leak guard. Negative control run: restoring the old
message fails 2 of 4, and the no-leak / sibling assertions correctly still pass.
Launch suite: 3081 passed / 16 skipped / 0 failed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
…ound `bg-green-500/60` / `bg-red-500/60` are fixed sRGB from Tailwind's palette. They do not follow a theme change, so this sparkline was the last long/short surface in trade/ not on --long/--short (187 token uses elsewhere). The direction was ALSO wrong, which the colour swap surfaced. This same file renders a POSITIVE funding rate with --short — `:365` for eightHourRatePercent and `:298` for userPays — because positive funding means longs pay. The old green-for-positive pair read the opposite way, so the sparkline disagreed with the headline rate printed directly above it. I initially ported the old mapping verbatim (isPos -> --long) and caught it by checking the file's own convention before committing. Now isPos -> --short. Launch suite: 3086 passed / 16 skipped / 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
…nfigured The rate limiter's primary path is Upstash Redis (GH#1213), with an in-memory sliding window as a documented fallback for local dev and CI. On Vercel that fallback is per-isolate, so every cold start gets its own budget and an attacker spreading requests across instances bypasses the limit entirely — which is the issue's premise, and it is correct for any deployment that reaches it. The init-FAILURE branch already logged an error in production. MISSING env vars returned quietly. That is backwards: a Redis client that throws on construction is rare, while a deployment that simply never had UPSTASH_REDIS_REST_URL and UPSTASH_REDIS_REST_TOKEN set is the ordinary way to end up on the fallback — a fresh environment, or a preview promoted to production without the vars. The likely case was the silent one. Now both paths log in production. Logged rather than thrown deliberately: middleware runs on every request, so failing closed here would take the whole site down over a rate limiter. Trading availability for enforcement is the right call, and it is precisely why the degradation must not be silent — the operator has to be able to find out. This does not make the fallback distributed. It makes its absence visible, which is the part that can be fixed in code; configuring Upstash in production is an ops action and the issue stays open for it. Launch suite: 3094 passed / 16 skipped / 0 failed. Refs: #2341 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
… advisory #2220 reported next@16.2.3 against advisories fixed in >=16.2.5 / >=16.2.6. The dependency has since moved to 16.2.9, so those specific advisories are behind us — but the issue is NOT stale: newer 16.x advisories landed with a fix version of >=16.2.11, so `next` was still the one DIRECT dependency failing a production high-severity audit. This matters more here than a version number usually does, for the reason the issue gives: this app puts security-relevant controls in Next middleware — host gating, blocklisted markets, admin routes, rate limiting, security headers — and several of the outstanding advisories are middleware/proxy bypasses. A bypass in that layer is a bypass of those controls. Verified after the bump: typecheck clean, suite 3094 passed / 16 skipped / 0 failed, and `next build` completes. The transitive high-severity findings the issue also lists (nanoid, postcss, browserslist, undici, image-size, sharp, socket.io-parser, fast-uri, ip-address) are NOT addressed here. They arrive through @privy-io/react-auth and the build toolchain, so they need either an upstream release or a pnpm override, and an override that forces a version a dependency was not tested against can break the wallet path. Left for a deliberate pass rather than bundled into a security bump that is currently verifiable end to end. Refs: #2220 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
…ent citing it The #2525 half of an earlier combined commit, re-landed on its own. Its #2514 half is dropped — #2518 already fixed that on playground, deliberately choosing to stay non-fatal and surface the failure on the success screen. See the issue for the one thing the two approaches do differently. SECURITY.md:35 and README.md:451 both stated WS_AUTH_REQUIRED defaults to `false`. The server that implements it does not: percolator-api/src/routes/ws.ts:52-56 const WS_AUTH_REQUIRED = process.env.WS_AUTH_REQUIRED !== undefined ? process.env.WS_AUTH_REQUIRED === "true" : IS_PRODUCTION; So the default is environment-dependent — REQUIRED in production, optional otherwise — with fail-closed startup checks either side of it. That means the issue's stated concern ("if deployed on the SECURITY.md default, the price feed is public") does not hold on a production API. The real hazard ran the other way: a reader trusting the docs would believe production was open when it is not, and might "fix" it by setting something explicitly. priceStore.ts carried the same wrong figure in a SECURITY REVIEW comment, which is how a docs error becomes a code error — the next reader treats the comment as the specification and does not check. Corrected there too, with the live consequence stated the right way round: on a production API this client does not get an open feed, it fails to authenticate and silently falls back to REST. The genuine open question is preserved rather than closed over: whether this client should carry an HMAC token so it keeps the WS path in production instead of degrading to REST. Refs: #2525 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
#2512 (GH#2510) replaced the 10k-row cap with a database aggregation, which is the better fix — but the new query landed without a network predicate: FROM trades WHERE trader = ${wallet} so a wallet's stats summed devnet AND mainnet trades into one answer. Every field it returns was affected: totalTrades, totalVolume, totalFees, uniqueMarkets, and both timestamps. Found by the guard added for #2513 rather than by reading the diff. That test scans indexer-db.ts for `FROM trades` / `FROM funding_history` without a following network predicate, and it named the line: expected [ 'line 526: FROM trades' ] to deeply equal [] which is exactly the job it was written for. #2513 patched 12 sites; this is the 13th, added after that sweep by an unrelated change. The row-fetch path directly below it (:565) already had the predicate, so the two sibling queries disagreed. Worth noting for anyone reviewing similar work: the aggregation is a strict improvement over the cap it replaced, and the missing filter is not an argument against it. It is an argument for the guard — a cross-network sum is invisible on a single-network deployment and silently wrong on a dual one, and no amount of careful reading reliably catches a missing WHERE clause. Launch suite: 3111 passed / 16 skipped / 0 failed. Refs: #2513, #2510 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (21)
📝 WalkthroughWalkthroughThe pull request adds on-chain mint validation, network-scoped indexer queries, RPC timeouts, frontend data guards, timer cleanup, bounded price caching, accessibility attributes, clearer diagnostics, authentication documentation, production logging, and a Next.js update. ChangesApplication hardening and maintenance
Estimated code review effort: 4 (Complex) | ~45 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
This was referenced Sep 2, 2026
This was referenced Sep 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #2526 (same author), rebased onto
playgroundafter six commits landed under it. Three of the original fifteen commits were dropped as duplicates — see below.Dropped, because someone else got there first
8d3e280a(#2512)truncatedflag on top of it. They also found a sharper detail: the cap kept the oldest 10k, solastTradeAtwas the timestamp of the 10,000th oldest trade and would stop advancing once a wallet passed the cap.a277ae79(#2511)7ce465b2(#2518)And that drop surfaced a regression — caught by a guard, not by reading
The #2510 database aggregation landed without a network predicate:
So a wallet's stats summed devnet and mainnet trades into one answer —
totalTrades,totalVolume,totalFees,uniqueMarketsand both timestamps, all affected.The
#2513guard named it precisely:#2513 patched 12 sites; this is the 13th, added after that sweep. The sibling row-fetch query 40 lines below (
:565) already had the predicate, so the two disagreed.To be clear: the aggregation is a strict improvement over the cap it replaced, and the missing filter is not an argument against it. It is an argument for the guard — a cross-network sum is invisible on a single-network deployment and silently wrong on a dual one.
The 13 fixes
#2243footer a11y ·#2320bounded priceStore cache ·#2321finite-volume check ·#2323timer cleanup on unmount ·#2513network scoping (12 sites + this 13th) ·#2522/api/rpctimeout + dedup hardening ·#2520on-chain mint existence, fail-closed ·#2412non-finite liquidation distance must not read "safe" ·#2523name the offending type in the error ·#2368funding sparkline tokens and direction ·#2341log when the distributed limiter is unconfigured ·#2220next 16.2.9 → 16.2.11 ·#2525WS_AUTH_REQUIRED docsTwo worth a second look:
#2368 — porting the sparkline to design tokens surfaced a direction bug. This file maps a positive funding rate to
--shortat:298and:365, because positive funding means longs pay. The oldbg-green-500for positive was backwards, so the sparkline disagreed with the headline rate above it.#2220 —
nexthad already moved past the advisories in the issue (16.2.3 → 16.2.9), but new ones required ≥16.2.11. Closing it as stale would have been wrong. Now at 16.2.11;nexthigh/critical advisories: 0.Verification
next buildcompletes🤖 Generated with Claude Code
https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
Summary by CodeRabbit
Security
Bug Fixes
Accessibility & UI