Skip to content

Validate affiliate conversion ids before mutation queries #459

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ralyodio merged 1 commit into from
Jun 14, 2026
Merged

Validate affiliate conversion ids before mutation queries #459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 73 additions & 1 deletion src/app/api/affiliates/offers/[id]/conversions/route.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { describe, it, expect, vi, beforeEach } from "vitest";
import { GET, POST, PUT } from "./route";
import { DELETE, GET, POST, PUT } from "./route";
import { NextRequest } from "next/server";

// Mock auth
Expand Down Expand Up @@ -44,6 +44,14 @@ function makePutRequest(id: string, body: Record<string, unknown>) {
});
}

function makeDeleteRequest(id: string, body: Record<string, unknown>) {
return new NextRequest(`http://localhost/api/affiliates/offers/${id}/conversions`, {
method: "DELETE",
headers: { "Content-Type": "application/json" },
body: JSON.stringify(body),
});
}

function makeParams(id: string) {
return { params: Promise.resolve({ id }) };
}
Expand Down Expand Up @@ -390,6 +398,37 @@ describe("PUT /api/affiliates/offers/[id]/conversions", () => {
expect(mockCalculateCommission).not.toHaveBeenCalled();
});

it("rejects non-string conversion_id before updating conversions", async () => {
mockGetAuthContext.mockResolvedValue({
user: { id: "user-seller", authMethod: "session" },
});

mockFrom.mockImplementation((table: string) => {
if (table === "affiliate_offers") {
return chainable({
id: "offer-1",
seller_id: "user-seller",
commission_rate: 0.2,
commission_type: "percentage",
commission_flat_sats: 0,
});
}
return chainable([]);
});

const res = await PUT(
makePutRequest("offer-1", {
conversion_id: { id: "conv-1" },
note: "paid elsewhere",
}),
makeParams("offer-1")
);

expect(res.status).toBe(400);
await expect(res.json()).resolves.toEqual({ error: "conversion_id is required" });
expect(mockFrom).not.toHaveBeenCalledWith("affiliate_conversions");
});
Comment on lines +401 to +430

@greptile-apps greptile-apps Bot Jun 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Whitespace-only conversion_id not tested

The new isNonEmptyString guard rejects whitespace-only strings (e.g., " "), which is a behavior change from the old !conversion_id check — a whitespace string is truthy and previously would have been forwarded to Supabase. Neither the PUT nor the DELETE new tests cover this path, so that specific validation branch has no regression coverage.


it("updates integer sale_amount_sats and recalculates commission", async () => {
mockGetAuthContext.mockResolvedValue({
user: { id: "user-seller", authMethod: "session" },
Expand Down Expand Up @@ -477,6 +516,39 @@ describe("PUT /api/affiliates/offers/[id]/conversions", () => {
});
});

describe("DELETE /api/affiliates/offers/[id]/conversions", () => {
beforeEach(() => {
vi.clearAllMocks();
});

it("rejects non-string conversion_id before querying conversions", async () => {
mockGetAuthContext.mockResolvedValue({
user: { id: "user-seller", authMethod: "session" },
});

mockFrom.mockImplementation((table: string) => {
if (table === "affiliate_offers") {
return chainable({
id: "offer-1",
seller_id: "user-seller",
});
}
return chainable([]);
});

const res = await DELETE(
makeDeleteRequest("offer-1", {
conversion_id: ["conv-1"],
}),
makeParams("offer-1")
);

expect(res.status).toBe(400);
await expect(res.json()).resolves.toEqual({ error: "conversion_id is required" });
expect(mockFrom).not.toHaveBeenCalledWith("affiliate_conversions");
});
});
Comment on lines +519 to +550

@greptile-apps greptile-apps Bot Jun 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 DELETE handler has only one negative test

The new describe("DELETE ...") block adds a single rejection test. The DELETE handler also guards against unauthorized users, non-existent conversions (404), and deleting paid conversions — none of which are covered. A regression on any of those branches would go undetected.

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!


describe("PUT /api/affiliates/offers/[id]/conversions", () => {
beforeEach(() => {
vi.clearAllMocks();
Expand Down
16 changes: 11 additions & 5 deletions src/app/api/affiliates/offers/[id]/conversions/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ function isPositiveIntegerSats(value: unknown): value is number {
return typeof value === "number" && Number.isInteger(value) && value > 0;
}

function isNonEmptyString(value: unknown): value is string {
return typeof value === "string" && value.trim().length > 0;
}

/**
* GET /api/affiliates/offers/[id]/conversions - List conversions for an offer (seller only)
*/
Expand Down Expand Up @@ -226,9 +230,10 @@ export async function PUT(request: NextRequest, { params }: { params: Promise<{
const body = await request.json();
const { conversion_id, sale_amount_sats, note, status } = body;

if (!conversion_id) {
if (!isNonEmptyString(conversion_id)) {
return NextResponse.json({ error: "conversion_id is required" }, { status: 400 });
}
const conversionId = conversion_id.trim();

const updateData: Record<string, unknown> = {};
if (typeof note === "string") updateData.note = note.trim();
Expand Down Expand Up @@ -261,7 +266,7 @@ export async function PUT(request: NextRequest, { params }: { params: Promise<{
const { error: updateErr } = await (admin as AnySupabase)
.from("affiliate_conversions")
.update(updateData)
.eq("id", conversion_id)
.eq("id", conversionId)
.eq("offer_id", id);

if (updateErr) {
Expand Down Expand Up @@ -304,14 +309,15 @@ export async function DELETE(
const body = await request.json();
const { conversion_id } = body;

if (!conversion_id) {
if (!isNonEmptyString(conversion_id)) {
return NextResponse.json({ error: "conversion_id is required" }, { status: 400 });
}
const conversionId = conversion_id.trim();

const { data: conv } = await (admin as AnySupabase)
.from("affiliate_conversions")
.select("id, status")
.eq("id", conversion_id)
.eq("id", conversionId)
.eq("offer_id", id)
.single();

Expand All @@ -326,7 +332,7 @@ export async function DELETE(
const { error: delErr } = await (admin as AnySupabase)
.from("affiliate_conversions")
.delete()
.eq("id", conversion_id)
.eq("id", conversionId)
.eq("offer_id", id);

if (delErr) {
Expand Down
Loading