Skip to content

Commit 7b11e05

Browse files
saulmcclaude
andauthored
fix(agent-templates): featuring is the dashboard's, not the owner's (#364)
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent a10175f commit 7b11e05

7 files changed

Lines changed: 273 additions & 39 deletions

File tree

src/api/v2/agent-templates/agent-templates.router.ts

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { Router } from "express";
22
import {
3+
agentApiKeyAuth,
34
authOrAgentApiKeyAuth,
45
optionalAuthOrAgentApiKeyAuth,
56
} from "@/middleware/agentAuth";
@@ -77,12 +78,18 @@ agentTemplatesRouter.get(
7778
listCountsHandler,
7879
);
7980

80-
// The featured gallery's order, written whole and in one transaction. Mounted
81-
// before /:id so the wildcard doesn't capture "featured-order" as a template id.
81+
// The featured gallery's order, written whole and in one transaction.
82+
//
83+
// Curation is not something a user does, so this isn't an endpoint a user
84+
// reaches: the agent key is required outright, and a signed-in caller is turned
85+
// away at the door rather than admitted and refused inside. The key IS the
86+
// identity here — there is no account to require.
87+
//
88+
// Mounted before /:id so the wildcard doesn't capture "featured-order" as a
89+
// template id.
8290
agentTemplatesRouter.put(
8391
"/featured-order",
84-
authOrAgentApiKeyAuth,
85-
requireAccount,
92+
agentApiKeyAuth,
8693
featuredOrderHandler,
8794
);
8895

src/api/v2/agent-templates/handlers/create.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,25 @@ export async function createHandler(req: Request, res: Response) {
114114
// (users can't create rows owned by someone else).
115115
// - Anonymous: no account → 403 below.
116116
const isApiKeyListener = res.locals.isApiKeyListener ?? false;
117+
118+
// Featuring is curation, and the gallery is the homepage's — not the template
119+
// owner's. Anyone may create a template; only the dashboard may put one in
120+
// front of everybody. Without this an ordinary signed-in user could mint a
121+
// self-featured template in a single request and publish it into the gallery
122+
// convos.org renders.
123+
//
124+
// Asking for `featured: false` is a no-op and passes: the runtime's builder
125+
// sends it on every create, and a template that isn't featured is exactly what
126+
// a create would produce anyway.
127+
if (parsed.data.featured === true && !isApiKeyListener) {
128+
req.log.warn(
129+
{ callerAccountId: res.locals.accountId, action: "create.featured" },
130+
"Unauthorized agent-template curation attempt",
131+
);
132+
res.status(403).json({ error: "Not authorized to feature a template" });
133+
return;
134+
}
135+
117136
let ownerAccountId: string | undefined;
118137
if (isApiKeyListener && parsed.data.ownerAccountId !== undefined) {
119138
const assertedAccountId = parsed.data.ownerAccountId;

src/api/v2/agent-templates/handlers/featured-order.ts

Lines changed: 3 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -56,22 +56,10 @@ export const isSerializationFailure = (error: unknown): boolean => {
5656
* won't match and the write is refused rather than clobbering a set the caller
5757
* never saw.
5858
*/
59+
// Reachable only behind `agentApiKeyAuth` (see the router): curation is not a
60+
// thing a user does, so there is no caller here to authorize — a request without
61+
// the agent key never arrives.
5962
export async function featuredOrderHandler(req: Request, res: Response) {
60-
// Curation is the dashboard's, not an owner's — same rule the rank field
61-
// itself carries in PATCH.
62-
const isApiKeyListener = res.locals.isApiKeyListener ?? false;
63-
if (!isApiKeyListener) {
64-
req.log.warn(
65-
{ callerAccountId: res.locals.accountId, action: "featured-order" },
66-
"Unauthorized agent-template curation attempt",
67-
);
68-
sendError(res, 403, {
69-
code: "FORBIDDEN",
70-
message: "Not authorized to set the featured gallery order",
71-
});
72-
return;
73-
}
74-
7563
const parsed = bodySchema.safeParse(req.body);
7664
if (!parsed.success) {
7765
res.status(400).json({

src/api/v2/agent-templates/handlers/patch.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,29 @@ export async function patchHandler(req: Request, res: Response) {
185185
return;
186186
}
187187

188+
// Getting INTO the gallery is curation too, not just where you sit in it.
189+
// The guard above admits the template's owner, so without this an owner
190+
// could feature their own template onto convos.org — they couldn't pick a
191+
// slot (that's gated below, and they'd enter at weight 0, last), but they
192+
// would be in the gallery, and in front of everyone the moment it holds
193+
// fewer than a homepage's worth of curated templates.
194+
//
195+
// Taking a template back OUT stays theirs: `featured: false` is a
196+
// withdrawal, like unpublishing, and it can only ever remove something from
197+
// the homepage.
198+
if (parsedBody.data.featured === true && !isApiKeyListener) {
199+
req.log.warn(
200+
{
201+
callerAccountId,
202+
templateId: template.id,
203+
action: "patch.featured",
204+
},
205+
"Unauthorized agent-template curation attempt",
206+
);
207+
res.status(403).json({ error: "Not authorized to feature a template" });
208+
return;
209+
}
210+
188211
// Gallery curation is not an ownership right. `featuredRank` decides what
189212
// leads the convos.org homepage, and the guard above admits the template's
190213
// owner — so without this an owner could weight their own template above

tests/agent-templates.featured-order.test.ts

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -284,18 +284,29 @@ describe("Agent templates — featured gallery order", () => {
284284
expect(stranded).toEqual([]);
285285
});
286286

287-
test("only the dashboard's API key may write the order", async () => {
287+
// Curation isn't something a user does, so this isn't an endpoint a user
288+
// reaches: a signed-in caller is turned away at the door (401, no agent key)
289+
// rather than admitted and refused inside.
290+
test("a signed-in caller can't reach the endpoint at all", async () => {
288291
const one = await seedGalleryTemplate("Fone");
289292
const two = await seedGalleryTemplate("Ftwo");
290293
const before = await weights();
291294

292295
// The templates' own owner, authenticated as a user rather than as the
293-
// dashboard: curation is not an ownership right.
294-
const res = await setOrder({
296+
// dashboard.
297+
const asUser = await setOrder({
295298
templateIds: [two, one],
296299
headers: await jwtHeaders(),
297300
});
298-
expect(res.response.status).toBe(403);
301+
expect(asUser.response.status).toBe(401);
302+
expect(await weights()).toBe(before);
303+
304+
// And with no credentials at all.
305+
const anonymous = await setOrder({
306+
templateIds: [two, one],
307+
headers: { "Content-Type": "application/json" },
308+
});
309+
expect(anonymous.response.status).toBe(401);
299310
expect(await weights()).toBe(before);
300311
});
301312
});
Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
import {
2+
afterAll,
3+
beforeAll,
4+
beforeEach,
5+
describe,
6+
expect,
7+
test,
8+
} from "vitest";
9+
import { __setAgentAssetsApiKeyOverrideForTests } from "@/middleware/agentAuth";
10+
import { createJwtToken } from "@/utils/jwt";
11+
import { prisma } from "@/utils/prisma";
12+
import {
13+
agentKeyHeaders,
14+
startAgentTemplatesServer,
15+
validAgentAssetsApiKey,
16+
} from "./agent-templates.cross.helpers";
17+
18+
// The featured gallery is the homepage's, not the template owner's. These pin
19+
// the whole path shut: an ordinary signed-in user could otherwise mint a
20+
// self-featured template, publish it (owners may publish their own), and appear
21+
// in the gallery convos.org renders. They couldn't pick a slot — that was
22+
// already gated — but being in the gallery at all is the front door.
23+
24+
// A plain punter: not the admin account, no API key.
25+
const USER = "00000000-0000-4000-8000-eeeeeeee0001";
26+
27+
let baseURL: string;
28+
let close: () => Promise<void>;
29+
30+
const userHeaders = async () => ({
31+
"Content-Type": "application/json",
32+
"X-Convos-AuthToken": await createJwtToken({
33+
deviceId: "featuring-authz-device",
34+
accountId: USER,
35+
}),
36+
});
37+
38+
const create = async (
39+
headers: Record<string, string>,
40+
body: Record<string, unknown>,
41+
) => {
42+
const response = await fetch(`${baseURL}/api/v2/agent-templates`, {
43+
method: "POST",
44+
headers,
45+
body: JSON.stringify(body),
46+
});
47+
return { response, body: (await response.json()) as Record<string, unknown> };
48+
};
49+
50+
const patch = async (
51+
headers: Record<string, string>,
52+
id: string,
53+
body: Record<string, unknown>,
54+
) => {
55+
const response = await fetch(`${baseURL}/api/v2/agent-templates/${id}`, {
56+
method: "PATCH",
57+
headers,
58+
body: JSON.stringify(body),
59+
});
60+
return { response, body: (await response.json()) as Record<string, unknown> };
61+
};
62+
63+
const featuredOf = async (id: string) =>
64+
(
65+
await prisma.agentTemplate.findUniqueOrThrow({
66+
where: { id },
67+
select: { featured: true },
68+
})
69+
).featured;
70+
71+
describe("Featuring is the dashboard's, not the owner's", () => {
72+
beforeAll(async () => {
73+
__setAgentAssetsApiKeyOverrideForTests(validAgentAssetsApiKey);
74+
await prisma.account.upsert({
75+
where: { id: USER },
76+
create: { id: USER },
77+
update: {},
78+
});
79+
const server = await startAgentTemplatesServer(4098);
80+
baseURL = server.baseURL;
81+
close = server.close;
82+
});
83+
84+
afterAll(async () => {
85+
await prisma.agentTemplate.deleteMany({ where: { ownerAccountId: USER } });
86+
await close();
87+
__setAgentAssetsApiKeyOverrideForTests(undefined);
88+
});
89+
90+
beforeEach(async () => {
91+
await prisma.agentTemplate.deleteMany({ where: { ownerAccountId: USER } });
92+
});
93+
94+
test("a user can't create a template that is already featured", async () => {
95+
const headers = await userHeaders();
96+
const res = await create(headers, {
97+
agentName: "Self Featured",
98+
prompt: "p",
99+
slug: "authz-self-featured",
100+
featured: true,
101+
});
102+
expect(res.response.status).toBe(403);
103+
104+
// And nothing was written.
105+
const rows = await prisma.agentTemplate.findMany({
106+
where: { ownerAccountId: USER },
107+
});
108+
expect(rows).toEqual([]);
109+
});
110+
111+
test("a user can't feature their own template afterwards", async () => {
112+
const headers = await userHeaders();
113+
const created = await create(headers, {
114+
agentName: "Ordinary",
115+
prompt: "p",
116+
slug: "authz-ordinary",
117+
});
118+
const id = created.body.id as string;
119+
expect(await featuredOf(id)).toBe(false);
120+
121+
const res = await patch(headers, id, { featured: true });
122+
expect(res.response.status).toBe(403);
123+
expect(await featuredOf(id)).toBe(false);
124+
});
125+
126+
// The builder sends `featured: false` on every create, and withdrawing a
127+
// template from the homepage can only ever remove something from it.
128+
test("a user may still create with featured:false, and unfeature their own", async () => {
129+
const headers = await userHeaders();
130+
const created = await create(headers, {
131+
agentName: "Opts Out",
132+
prompt: "p",
133+
slug: "authz-opts-out",
134+
featured: false,
135+
});
136+
expect(created.response.status).toBe(201);
137+
const id = created.body.id as string;
138+
139+
// The dashboard features it...
140+
const featured = await patch(agentKeyHeaders(), id, { featured: true });
141+
expect(featured.response.status).toBe(200);
142+
expect(await featuredOf(id)).toBe(true);
143+
144+
// ...and the owner can take it back out.
145+
const withdrawn = await patch(headers, id, { featured: false });
146+
expect(withdrawn.response.status).toBe(200);
147+
expect(await featuredOf(id)).toBe(false);
148+
});
149+
150+
test("the dashboard still features templates", async () => {
151+
const created = await create(agentKeyHeaders(), {
152+
agentName: "Curated",
153+
prompt: "p",
154+
slug: "authz-curated",
155+
featured: true,
156+
ownerAccountId: USER,
157+
});
158+
expect(created.response.status).toBe(201);
159+
expect(await featuredOf(created.body.id as string)).toBe(true);
160+
});
161+
});

tests/auth-middleware.test.ts

Lines changed: 41 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -260,28 +260,53 @@ describe("agent-templates router auth wiring", () => {
260260
expect(source).toContain("requireAccount");
261261
expect(source).toContain('from "@/middleware/auth"');
262262

263-
// Write routes (POST /, PUT /featured-order, PATCH /:id, DELETE /:id,
264-
// POST /:id/publish) chain `authOrAgentApiKeyAuth + requireAccount`. That's
265-
// 5 routes, and one import, for 6 occurrences of `requireAccount` total.
263+
// Write routes (POST /, PATCH /:id, DELETE /:id, POST /:id/publish)
264+
// chain `authOrAgentApiKeyAuth + requireAccount`. That's 4 routes,
265+
// and one import, for 5 occurrences of `requireAccount` total.
266266
const requireAccountCount = (source.match(/requireAccount/g) ?? []).length;
267-
expect(requireAccountCount).toBe(6);
268-
269-
expect(source).toContain("requireAccount,\n createHandler");
270-
expect(source).toContain("requireAccount,\n featuredOrderHandler");
271-
expect(source).toContain("requireAccount,\n patchHandler");
272-
expect(source).toContain("requireAccount,\n deleteHandler");
273-
expect(source).toContain("requireAccount,\n publishHandler");
267+
expect(requireAccountCount).toBe(5);
268+
269+
// These pin each route's middleware CHAIN by reading the router's source, so
270+
// they're matched whitespace-agnostically: a formatter reflowing the file
271+
// must not be able to turn a real auth regression into a green test, nor a
272+
// harmless reflow into a red one. The leading lookbehind is what stops
273+
// `agentApiKeyAuth` from matching inside `authOrAgentApiKeyAuth` — the two
274+
// differ by exactly the thing these assertions exist to tell apart.
275+
const chain = (middleware: string, handler: string) =>
276+
new RegExp(`(?<![A-Za-z])${middleware},\\s+${handler}\\b`);
277+
278+
expect(source).toMatch(chain("requireAccount", "createHandler"));
279+
expect(source).toMatch(chain("requireAccount", "patchHandler"));
280+
expect(source).toMatch(chain("requireAccount", "deleteHandler"));
281+
expect(source).toMatch(chain("requireAccount", "publishHandler"));
282+
283+
// Curating the featured gallery is not a user action, so the order endpoint
284+
// takes the agent key ALONE — no JWT path reaches it, and there is no
285+
// account to require. Either permissive middleware here would admit a
286+
// signed-in caller and leave the refusal to the handler.
287+
expect(source).toMatch(chain("agentApiKeyAuth", "featuredOrderHandler"));
288+
expect(source).not.toMatch(
289+
chain("authOrAgentApiKeyAuth", "featuredOrderHandler"),
290+
);
291+
expect(source).not.toMatch(
292+
chain("optionalAuthOrAgentApiKeyAuth", "featuredOrderHandler"),
293+
);
294+
expect(source).not.toMatch(chain("requireAccount", "featuredOrderHandler"));
274295

275296
// Public routes use the optional middleware: GET /, GET /:idOrUrlSlug,
276297
// POST /generations, GET /generations/:generationId. The optional
277298
// middleware MUST NOT be followed by requireAccount.
278-
expect(source).toContain("optionalAuthOrAgentApiKeyAuth, listHandler");
279-
expect(source).toContain("optionalAuthOrAgentApiKeyAuth,\n detailHandler");
280-
expect(source).toContain(
281-
"optionalAuthOrAgentApiKeyAuth,\n generationsPostHandler",
299+
expect(source).toMatch(
300+
chain("optionalAuthOrAgentApiKeyAuth", "listHandler"),
301+
);
302+
expect(source).toMatch(
303+
chain("optionalAuthOrAgentApiKeyAuth", "detailHandler"),
304+
);
305+
expect(source).toMatch(
306+
chain("optionalAuthOrAgentApiKeyAuth", "generationsPostHandler"),
282307
);
283-
expect(source).toContain(
284-
"optionalAuthOrAgentApiKeyAuth,\n generationsGetHandler",
308+
expect(source).toMatch(
309+
chain("optionalAuthOrAgentApiKeyAuth", "generationsGetHandler"),
285310
);
286311
expect(source).not.toMatch(
287312
/optionalAuthOrAgentApiKeyAuth,\s*requireAccount/,

0 commit comments

Comments
 (0)