Validate market symbol/name on the live registration path - #2467
Open
0x-SquidSol wants to merge 1 commit into
Open
Validate market symbol/name on the live registration path#24670x-SquidSol wants to merge 1 commit into
0x-SquidSol wants to merge 1 commit into
Conversation
The market-creation flow registers markets via /api/playground/keeper-register, which wrote the market row (and the Blob registry) after only a non-empty-string check on caller-supplied symbol/name/label. The symbol/name validation — charset, length, control-character and invisible/bidirectional-character rejection — lived only on the POST /api/markets path, which the launch flow no longer calls. As a result a market creator could register deceptive metadata (homoglyph / RTL-override / zero-width names, control characters, or overlong values) and impersonate an existing market across every surface that renders these fields. Extract the validation into lib/market-metadata-validation as the single source of truth and apply it on keeper-register before any write. Refactor POST /api/markets to consume the same module so the two paths cannot drift. - add lib/market-metadata-validation (checkSymbol/checkName + validate* wrappers) - keeper-register: reject invalid symbol/name/label (top-level body and payload) with HTTP 400 before the DB row or Blob registry write; server-derived fallbacks are unaffected and the retry path (which sends nulls) is unchanged - markets/route: consume the shared validator (behavior- and message-preserving) - add a regression test asserting the shared validator rejects the impersonation corpus the previous non-empty gate accepted 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 change adds shared market symbol and name validation, applies it to keeper registration and market registration routes, and adds regression tests for impersonation, unsafe characters, whitespace-only values, and excessive length. ChangesMarket metadata validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant KeeperRegister
participant MetadataValidation
participant Database
participant BlobRegistry
Client->>KeeperRegister: submit market metadata
KeeperRegister->>MetadataValidation: validateSymbol and validateName
MetadataValidation-->>KeeperRegister: validated values or error
KeeperRegister->>Database: persist validated metadata
KeeperRegister->>BlobRegistry: persist validated metadata
🚥 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/__tests__/api/keeper-register-metadata-validation.test.ts (1)
21-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExercise the keeper registration route.
This test only calls the shared helpers. It does not prove that
/api/playground/keeper-registerreturns HTTP 400 beforeupsertRegisteredMarketRowandupsertRegisteredMarket.Mock the persistence boundaries. Invoke
POSTwith invalid body and payload metadata. Assert HTTP 400 and zero write calls.🤖 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/keeper-register-metadata-validation.test.ts` around lines 21 - 22, Replace the helper-only coverage in the keeper registration test with an exercise of the route’s POST handler: mock upsertRegisteredMarketRow and upsertRegisteredMarket, submit invalid body and payload metadata to /api/playground/keeper-register, assert an HTTP 400 response, and verify both persistence functions receive zero calls.
🤖 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.
Inline comments:
In `@app/lib/market-metadata-validation.ts`:
- Around line 61-72: Update the resolution logic in both validateSymbol and
validateName functions to explicitly reject non-null non-string raw values
before defaulting to the fallback. Instead of treating all non-string inputs
(like objects or numbers) as absent and using the fallback, check if raw is null
or undefined to use the fallback, check if raw is a string with length greater
than 0 to use raw, and return an error response for any non-null non-string
values to prevent invalid types from being processed as successful with a
fallback value.
---
Nitpick comments:
In `@app/__tests__/api/keeper-register-metadata-validation.test.ts`:
- Around line 21-22: Replace the helper-only coverage in the keeper registration
test with an exercise of the route’s POST handler: mock
upsertRegisteredMarketRow and upsertRegisteredMarket, submit invalid body and
payload metadata to /api/playground/keeper-register, assert an HTTP 400
response, and verify both persistence functions receive zero calls.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 246a62f6-78c1-41ee-a909-52000cf5600b
📒 Files selected for processing (4)
app/__tests__/api/keeper-register-metadata-validation.test.tsapp/app/api/markets/route.tsapp/app/api/playground/keeper-register/route.tsapp/lib/market-metadata-validation.ts
Comment on lines
+61
to
+72
| const resolved = typeof raw === "string" && raw.length > 0 ? raw : fallback; | ||
| const r = checkSymbol(resolved); | ||
| return r.ok ? { ok: true, value: resolved } : { ok: false, error: r.error }; | ||
| } | ||
|
|
||
| /** Resolve `raw` (or `fallback` when raw is empty/absent), then validate as a name. */ | ||
| export function validateName( | ||
| raw: unknown, | ||
| fallback: string, | ||
| ): { ok: boolean; error?: string; value?: string } { | ||
| const resolved = typeof raw === "string" && raw.length > 0 ? raw : fallback; | ||
| const r = checkName(resolved); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject non-string raw metadata.
validateSymbol and validateName treat {} and 42 as absent input. POST /api/markets can then return success with a fallback value instead of HTTP 400.
Use the fallback only for absent or empty strings. Reject non-null non-string values before resolution.
🤖 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/lib/market-metadata-validation.ts` around lines 61 - 72, Update the
resolution logic in both validateSymbol and validateName functions to explicitly
reject non-null non-string raw values before defaulting to the fallback. Instead
of treating all non-string inputs (like objects or numbers) as absent and using
the fallback, check if raw is null or undefined to use the fallback, check if
raw is a string with length greater than 0 to use raw, and return an error
response for any non-null non-string values to prevent invalid types from being
processed as successful with a fallback value.
dcccrypto
reviewed
Aug 4, 2026
dcccrypto
left a comment
dcccrypto
left a comment
Owner
There was a problem hiding this comment.
Reviewed on the PR head (40ff630b). The fix is correct; one gap in the test.
Verified locally
The green Merge Gate is not evidence here — Unit Tests, Integration Tests,
Security Tests and Coverage Gate all report SKIPPED on this PR while the gate
reports SUCCESS (GH#2466's sibling problem, tracked in #2447). So I ran the suite
directly:
cd app && npx vitest run
Test Files 283 passed | 1 skipped (284)
Tests 2930 passed | 16 skipped (2946)
Clean — no regressions against the playground baseline.
I also checked the substance rather than just the diff:
- The guard sits before both writes (
upsertRegisteredMarketRowand the Blob
upsertRegisteredMarket), so nothing deceptive can be persisted. - All four caller-controlled fields are covered (
symbol,label,
payload.symbol,payload.name); server-derived fallbacks are correctly left
unvalidated, and the retry path (which sends nulls) is unaffected. - The
markets/route.tsrefactor is behaviour-preserving:symbol || fallback
andvalidateSymbol(raw, fallback)resolve identically for every input class.
Gap: the regression test does not bind the fix
keeper-register-metadata-validation.test.ts exercises checkSymbol/checkName
in isolation. It never imports the route, so it cannot fail if the wiring — which
is the fix — is removed. I mutation-tested it: deleting the entire SEC guard
block from app/app/api/playground/keeper-register/route.ts, i.e. fully restoring
the vulnerability, leaves the test at 11 passed / 0 failed.
Drop-in route-level test below. It drives the real handler; with the guard present
it is 6/6 green, and with the guard deleted it fails 5/6 — the request falls
through to the H1 auth error instead of the metadata 400, which is exactly the
binding that is missing. It also asserts no registry write happens.
app/__tests__/api/keeper-register-metadata-route-enforcement.test.ts
/**
* Regression: the metadata validator must be ENFORCED BY THE ROUTE, not merely
* exist as a library.
*
* keeper-register-metadata-validation.test.ts covers lib/market-metadata-validation
* in isolation. That unit alone cannot fail if the guard is deleted from
* app/api/playground/keeper-register/route.ts — i.e. it does not bind the actual
* fix for GH#2466, which is the WIRING of that validator into the live path.
*
* This test drives the real route handler: an impersonation payload must be
* rejected with 400 and the validator's own error text, before any registry
* write. Deleting the guard from the route makes this fail (the request instead
* falls through to the H1 auth check), which is exactly the binding the unit
* test is missing.
*/
import { afterAll, beforeEach, describe, expect, it, vi } from "vitest";
import { NextRequest } from "next/server";
// The route reads NETWORK at module scope — must be devnet before the import.
const prevNetwork = process.env.NEXT_PUBLIC_DEFAULT_NETWORK;
process.env.NEXT_PUBLIC_DEFAULT_NETWORK = "devnet";
afterAll(() => {
process.env.NEXT_PUBLIC_DEFAULT_NETWORK = prevNetwork;
});
// Any registry write is a failure for this test: the guard runs before them.
const blobPut = vi.fn(async () => ({ url: "https://blob.invalid/x" }));
const supabaseUpsert = vi.fn(async () => ({ error: null }));
vi.mock("@vercel/blob", () => ({
put: blobPut,
list: vi.fn(async () => ({ blobs: [] })),
head: vi.fn(async () => null),
del: vi.fn(async () => undefined),
}));
vi.mock("@/lib/supabase", () => ({
getServerNetwork: () => "devnet",
getServiceClient: () => ({
from: () => ({
upsert: supabaseUpsert,
select: () => ({ eq: () => ({ maybeSingle: async () => ({ data: null, error: null }) }) }),
}),
}),
}));
vi.mock("@sentry/nextjs", () => ({
captureException: vi.fn(),
captureMessage: vi.fn(),
}));
const { POST } = await import("@/app/api/playground/keeper-register/route");
// Real base58 pubkeys so the route's address validation passes and execution
// reaches the metadata guard.
const SLAB = "DJ54k4wH92NTtNP8RuHAwG8si1bevXEknzctDdqYN8eC";
const POOL = "LBUZKhRxPF3XUpBCjp4YzTKgLccjZhTSDM9YuVaPwxo";
const post = (body: Record<string, unknown>) =>
POST(
new NextRequest("http://localhost/api/playground/keeper-register", {
method: "POST",
headers: { "content-type": "application/json" },
body: JSON.stringify({ slabAddress: SLAB, dexPoolAddress: POOL, ...body }),
}),
);
describe("keeper-register enforces market-metadata validation on the live path", () => {
beforeEach(() => {
blobPut.mockClear();
supabaseUpsert.mockClear();
});
const cp = (n: number) => String.fromCodePoint(n);
const cases: Array<[string, Record<string, unknown>, RegExp]> = [
["body symbol — Cyrillic homoglyph", { symbol: "ЅОL" }, /Invalid symbol/],
["body label — RTL override", { label: "SOL/USD Perpetual" + cp(0x202e) }, /Invalid name/],
["payload.symbol — overlong", { payload: { symbol: "A".repeat(21) } }, /Invalid symbol/],
["payload.name — zero-width", { payload: { name: "SOL" + cp(0x200b) + "/USD" } }, /Invalid name/],
["payload.name — control char", { payload: { name: "SOL/USD\x00" } }, /Invalid name/],
];
it.each(cases)("rejects %s with 400 and writes nothing", async (_label, body, expected) => {
const res = await post(body);
expect(res.status).toBe(400);
expect(String(((await res.json()) as { error?: string }).error)).toMatch(expected);
expect(blobPut).not.toHaveBeenCalled();
expect(supabaseUpsert).not.toHaveBeenCalled();
});
it("does not reject legitimate metadata at the metadata guard", async () => {
const res = await post({ symbol: "SOL", label: "Solana Perpetual" });
// It must get PAST the metadata guard. It is then stopped by H1 auth
// (no deployer/signature), which is a different, non-400-metadata outcome.
if (res.status === 400) {
const err = String(((await res.json()) as { error?: string }).error);
expect(err).not.toMatch(/Invalid symbol|Invalid name/);
}
});
});Happy to push this to the branch (maintainerCanModify is on) or open it as a
follow-up PR — your call.
Not a problem: the three red Vercel checks
Vercel – percolator-launch/mainnet/playground all show FAILURE, but their
target_url is a vercel.com/git/authorize?... link — that is the fork-PR
authorization prompt, not a build failure. The base commit f2a3bbe5 is green on
all three, and Type Check and Build & Fast Tests pass here. Nothing to chase.
This was referenced Aug 4, 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
2 participants
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.
What
Validate market
symbol/nameon the live registration path(
/api/playground/keeper-register), closing a metadata-impersonation gap where amarket creator could store deceptive names/tickers (homoglyph / RTL-override /
zero-width names, control characters, or overlong values).
Closes #2466.
Why
The launch flow registers markets via
keeper-register, which wrote themarketsrow and the Blob registry after only a non-empty-string check. The symbol/name
validation lived solely on the now-unused
POST /api/marketspath, so it was deadcode on the live path. Both the top-level
symbol/labelbody fields andpayload.symbol/payload.nameare caller-controlled and reach both stores.Changes
lib/market-metadata-validation(new) — single source of truth:checkSymbol/checkNameplusvalidateSymbol/validateNameresolve-then-validate wrappers. Rules are unchanged from the original create
path: symbol 1–20
[A-Za-z0-9._-]; name 1–64, no control chars, noinvisible/bidirectional formatting chars (visible Unicode — accents, CJK, emoji
— still allowed).
keeper-register— validatesymbol/label(body) andpayload.symbol/payload.namebefore any write; return HTTP 400 on invalidinput. Server-derived fallbacks (
"UNKNOWN", the derived label,Market <slab>) are safe and unvalidated; the retry path (which sends nulls) isunaffected.
POST /api/markets— refactored to consume the shared validator(behavior- and message-preserving) so the two paths cannot drift again.
corpus the previous non-empty gate accepted, and accepts legitimate metadata.
Testing
npx tsc --noEmit— clean.gh1963-markets-input-validation+text-safety— 45/45 pass.useCreateMarket-keeper-register, keeper-register-concurrency-race,
market-registration, mobile-create-market-blockhash-recovery) — 48/48 pass.
(
blocklist-indexer-sync, a pre-existing cross-repo blocklist drift) isuntouched by this change.
Notes
script injection.
Summary by CodeRabbit
Release Notes
Tests
New Features