Return 400 for malformed referral JSON#467
Conversation
Greptile SummaryThis PR fixes a bug where malformed or non-object JSON in the referral POST body would fall through to the outer
Confidence Score: 4/5Safe to merge — the change is narrowly scoped to early-exit on bad input and does not affect the happy path. The fix is straightforward and well-tested for the malformed-JSON case. The only gaps are a slightly misleading error message when valid-but-non-object JSON is submitted, and a missing test case for that second branch of the guard. Both changed files are low-risk; the test file could benefit from an additional case covering a valid JSON array or null body. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant POST as POST /api/referrals
participant Auth as getAuthContext
participant readJsonObject
participant DB as Supabase / Email
Client->>POST: POST with body
POST->>Auth: getAuthContext(request)
Auth-->>POST: "null (Unauthorized) OR {user, supabase}"
alt Unauthorized
POST-->>Client: 401 Unauthorized
else Authorized
POST->>readJsonObject: request.json()
alt Malformed JSON or non-object
readJsonObject-->>POST: null
POST-->>Client: 400 Invalid JSON body
else Valid JSON object
readJsonObject-->>POST: "Record<string, unknown>"
POST->>DB: Rate-limit checks, DB insert, sendEmail
DB-->>POST: result
POST-->>Client: 200 / 4xx / 5xx
end
end
Reviews (1): Last reviewed commit: "Handle malformed referral JSON" | Re-trigger Greptile |
| async function readJsonObject(request: NextRequest) { | ||
| try { | ||
| const body = await request.json(); | ||
| if (!body || typeof body !== "object" || Array.isArray(body)) { | ||
| return null; | ||
| } | ||
| return body as Record<string, unknown>; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
The error message "Invalid JSON body" is returned for two distinct cases: a parse failure (truly malformed JSON) and a successful parse that yields a non-object (e.g. a JSON array or
null). Clients who send ["email@example.com"] will get "Invalid JSON body" even though their JSON is perfectly valid — which can make debugging confusing. Distinguishing the two cases gives callers a clearer signal.
| async function readJsonObject(request: NextRequest) { | |
| try { | |
| const body = await request.json(); | |
| if (!body || typeof body !== "object" || Array.isArray(body)) { | |
| return null; | |
| } | |
| return body as Record<string, unknown>; | |
| } catch { | |
| return null; | |
| } | |
| } | |
| type JsonObjectResult = | |
| | { ok: true; body: Record<string, unknown> } | |
| | { ok: false; reason: "malformed" | "not-object" }; | |
| async function readJsonObject(request: NextRequest): Promise<JsonObjectResult> { | |
| let parsed: unknown; | |
| try { | |
| parsed = await request.json(); | |
| } catch { | |
| return { ok: false, reason: "malformed" }; | |
| } | |
| if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) { | |
| return { ok: false, reason: "not-object" }; | |
| } | |
| return { ok: true, body: parsed as Record<string, unknown> }; | |
| } |
| describe("POST /api/referrals invalid JSON handling", () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| mocks.mockGetAuthContext.mockResolvedValue({ | ||
| user: { id: "user1" }, | ||
| supabase: { from: vi.fn() }, | ||
| }); | ||
| }); | ||
|
|
||
| it("returns 400 for malformed JSON before referral side effects", async () => { | ||
| const res = await POST(makeRawPostRequest("{not valid json")); | ||
| const body = await res.json(); | ||
|
|
||
| expect(res.status).toBe(400); | ||
| expect(body.error).toBe("Invalid JSON body"); | ||
| expect(mocks.mockCreateServiceClient).not.toHaveBeenCalled(); | ||
| expect(mocks.mockSendEmail).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Test covers only the parse-failure path
readJsonObject has two distinct rejection branches — a JSON.parse throw (malformed input) and a type check (valid JSON that isn't a plain object, e.g. arrays, null, or primitives). The test exercises only the first branch. Sending "[]" or "null" as the body would exercise the second branch and confirm both are mapped to 400 correctly.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
CI is green for PR #467. Verification:
uGig invoice evidence has been sent for this PR. |
Fixes #466.
Changes
Verification
corepack pnpm vitest run src/app/api/referrals/route.invalid-json.test.tscorepack pnpm tsc --noEmit