fix(agent-templates): featuring is the dashboard's, not the owner's

[saulmc]

Follow-up to #363 — these two commits were pushed to that branch after it was squash-merged, so they didn't land with it. Nothing new since; this is the same work, cherry-picked onto the merged otr-dev and re-verified there.

Curation is the dashboard's. Two holes closed.

The front door was open

Any signed-in user could put their own template into the convos.org featured gallery. create accepts featured in the body, and PATCH accepts it from whoever passes the ownership guard — which is the owner. Proven end to end with a plain JWT, no admin key anywhere:

Plain user JWT	Before	After
create with featured: true	201, featured stored	403
publish own template	200	200 (still theirs)
PATCH featured: true	200	403
PATCH featuredRank: 999	403	403
Lands in the public gallery?	YES (rank 0)	NO

Only the slot was gated (#362), so a self-featured template entered at weight 0 — dead last. That contains it while the gallery holds a homepage's worth of curated templates above it, but containment is not a closed door: it inflates the featured set, and the moment the gallery holds fewer than eight weighted templates, an outsider renders on the homepage.

Withdrawal stays the owner's. featured: false can only ever take something off the homepage, and the runtime's builder sends it on every create — which is why the gate keys on === true, not !== undefined. Gating that too would 403 every template the builder makes.

I checked that nothing legitimate features templates: across the runtime, website and workers, the only non-admin writer of featured is build.mjs, writing false. The admin console reaches the backend as an API-key caller and is unaffected — and the full suite passing with no test needing an update is itself evidence no existing flow relied on a user featuring anything.

PUT /featured-order was mounted as a user endpoint

It admitted any signed-in caller at the middleware and then refused them inside with an isApiKeyListener check. That makes a pure curation tool part of the user-facing API surface and leaves its safety resting on a guard in the handler body.

It now takes the agent key alone: a signed-in caller is turned away at the door (401) rather than let in and told no, and the key is the identity, so there's no account to require. The in-handler check goes with it — once the route can't be reached without the key, a check for one is dead code shaped like a lock, and the next reader would reasonably infer the route was reachable without it.

The auth-wiring guard test pins the shape both ways, so it can't be quietly re-mounted behind the permissive middleware later:

expect(source).toContain("agentApiKeyAuth,\n  featuredOrderHandler");
expect(source).not.toContain("requireAccount,\n  featuredOrderHandler");

The dashboard is unaffected: the worker already injects the same X-Agent-API-Key (AGENT_ASSETS_API_KEY) that this stricter guard validates.

Tests

• a user can't create a template that is already featured (403, and nothing is written)
• a user can't feature their own template afterwards (403, featured unchanged)
• a user may still create with featured: false, and unfeature their own
• the dashboard still features templates
• the order endpoint: a signed-in caller and an anonymous one both get 401, with the stored weights byte-identical afterwards

Full suite green (1492) on top of the merged otr-dev.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DxFzZNgXaSnFWAYR8XPpCR

---

^{Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.}

Note

Restrict agent template featuring to dashboard (agent API key) callers only

• PUT /featured-order now uses agentApiKeyAuth exclusively; JWT-authenticated and anonymous callers receive 401.
• POST and PATCH handlers for agent templates now return 403 if a non-agent-key caller attempts to set featured: true; setting featured: false remains permitted for owners.
• The in-handler auth check is removed from featured-order.ts since the middleware now enforces it at the router level.
• New test suite in agent-templates.featuring-authz.test.ts covers create/patch featuring authorization for both user and agent-key callers.
• Behavioral Change: template owners can no longer feature their own templates via the public API; only the dashboard agent key can set featured: true.

^{Macroscope summarized 699779c.}

Summary by CodeRabbit

• New Features

  • Restricted featured gallery ordering to authorized API-key access.
  • Added authorization controls for featuring agent templates during creation and updates.
  • Allowed authorized dashboard actions to feature or unfeature templates.

• Bug Fixes

  • Unauthorized attempts to feature templates now receive clear access-denied responses.
  • Unauthorized featured-order requests are rejected without changing gallery ordering.

[claude]

Claude finished @saulmc's task —— View job

---

Code Review Complete ✅

I've reviewed this PR thoroughly. The implementation is excellent — well-architected, thoroughly tested, and properly secured. Here's my detailed feedback:

---

Security ✅ Strong

Authorization enforcement is layered and complete:

1. Router-level protection (agent-templates.router.ts:90-94): The /featured-order endpoint now uses agentApiKeyAuth exclusively, rejecting non-API-key callers at the door with 401 instead of letting them reach the handler. This is the correct approach — authentication should be enforced at the middleware layer.

2. Handler-level guards are properly implemented:

   • create.ts:127-134: Rejects featured: true from non-API-key callers (403)
   • patch.ts:198-209: Same protection for patching existing templates
   • The featured: false allowance is well-reasoned — it can only remove from gallery, never add

3. Guard placement is smart: The check if (parsedBody.data.featured === true && !isApiKeyListener) keys on === true rather than !== undefined, which correctly allows the runtime builder's featured: false on every create while blocking self-featuring attempts.

One observation (not a blocker): The old in-handler isApiKeyListener check was removed from featured-order.ts (lines previously ~63-70, now gone). This is correct — once the route requires agentApiKeyAuth at the router level, the in-handler check becomes dead code. The comment in the router (router.ts:84-86) explains this well.

---

Code Quality ✅ Excellent

Router changes (agent-templates.router.ts:81-94):

• Clear, comprehensive comment explaining why this endpoint is agent-key-only
• Properly positioned before the /:id wildcard to avoid route capture
• Clean separation of concerns

Handler authorization logic:

• create.ts:118-134: The guard is well-placed after body parsing but before any database operations. The warning log provides good audit trail context.

• patch.ts:188-228: Three-tiered authorization is cleanly structured:

  1. Ownership guard (line 174-186)
  2. Featuring guard (line 198-209)
  3. FeaturedRank guard (line 215-228)

  The ownership guard correctly allows both owners AND API-key callers, then the curation guards layer on top of that.

Small suggestion (minor polish): The featuredRank guard message at patch.ts:226 says "Not authorized to set the featured gallery order" — consider making it slightly more specific like "Not authorized to set featured gallery position" to distinguish it from the whole-gallery /featured-order endpoint. Not critical, just a clarity improvement.

PII handling: Both create.ts and patch.ts maintain fail-closed semantics for redaction (502 on scan failure), which is the correct defensive posture.

---

Test Coverage ✅ Comprehensive

New test file (agent-templates.featuring-authz.test.ts):

• Covers all the critical paths:
  • User can't create featured (line 94-109)
  • User can't patch to featured (line 111-124)
  • User CAN create with featured: false and unfeature (line 128-148)
  • Dashboard still works (line 150-160)
• Tests verify both HTTP status AND database state, which catches partial writes
• Clean setup/teardown with proper isolation

Featured-order tests (agent-templates.featured-order.test.ts:287-311):

• New auth tests verify both signed-in users AND anonymous callers get 401
• Crucially, they verify weights are unchanged after rejection (line 293, 302, 310) — this proves the transaction semantics are working

Router auth wiring test (auth-middleware.test.ts:247-315):

• This is outstanding defensive testing. It reads the router source file and asserts the middleware chain format, so a future refactor can't quietly swap agentApiKeyAuth for authOrAgentApiKeyAuth without failing CI.
• The whitespace-agnostic regex (line 275-276) is smart — it won't break on formatting changes
• Lines 287-294 specifically pin that featuredOrderHandler requires agentApiKeyAuth AND rejects the permissive middlewares

No gaps identified. The test coverage matches the security requirements exactly.

---

Potential Issues / Edge Cases 🔍

None found. I looked for:

• ✅ Race conditions: The patch.ts:338-347 optimistic lock (updateMany with WHERE on original state) prevents TOCTOU issues
• ✅ Data integrity: The gallery-slot-clearing logic (patch.ts:243-258) ensures templates leaving the gallery forfeit their weight
• ✅ Backward compatibility: Existing flows that set featured: false continue working (runtime builder unaffected)
• ✅ Error responses: All rejection paths use appropriate status codes (403 for authz, 400 for validation, 409 for conflicts)

---

Performance Considerations ✅

No concerns. The authorization checks are simple boolean comparisons on res.locals.isApiKeyListener, which is already resolved by middleware. The database operations are unchanged except for the removed no-op check in featured-order.ts.

---

Best Practices ✅

1. Defense in depth: Both router-level auth AND handler-level guards
2. Fail-secure defaults: Templates default to featured: false (create.ts:94)
3. Audit logging: Warning logs on authz failures provide incident response trail
4. Transaction semantics: The featured-order handler's SERIALIZABLE transaction prevents gallery corruption
5. Test coverage: Behavioral verification, not just unit tests — end-to-end flows with real HTTP requests

---

Recommendations

None blocking. This is production-ready as-is. Two optional polish items:

1. Consider slightly more specific error message for featuredRank guard (mentioned above)
2. Consider adding a comment in create.ts near line 127 explaining why === true is used instead of a truthy check (the PR description explains it well, but future maintainers might wonder)

Both are minor documentation improvements, not functional issues.

---

Summary

This PR successfully closes two authorization holes:

1. Users can no longer self-feature templates via create or patch
2. The featured-order endpoint is now dashboard-only at the router level

The implementation is clean, well-tested, and follows security best practices. The test coverage is exemplary — particularly the router source-parsing test, which is a clever way to prevent silent auth regressions. All 1492 tests passing, no breaking changes to legitimate flows.

Verdict: Approved for merge. 🚀

---

[coderabbitai]

Walkthrough

The PR restricts featured-template creation, patching, and ordering to API-key-authorized flows. Router middleware, handler checks, and integration tests are updated to reflect the new authorization behavior.

Changes

Featuring authorization

Layer / File(s)	Summary
Featured-order route authorization src/api/v2/agent-templates/agent-templates.router.ts, src/api/v2/agent-templates/handlers/featured-order.ts, tests/agent-templates.featured-order.test.ts, tests/auth-middleware.test.ts	The featured-order route now uses agentApiKeyAuth without requireAccount, and tests verify unauthorized requests receive 401 responses.
Create and patch featuring checks src/api/v2/agent-templates/handlers/create.ts, src/api/v2/agent-templates/handlers/patch.ts	Non-API-key callers receive 403 responses when attempting to set featured: true.
Featuring authorization integration coverage tests/agent-templates.featuring-authz.test.ts	Integration tests cover rejected user operations, dashboard and owner transitions, and persisted featured state.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• xmtplabs/convos-backend#178: Introduced the agentApiKeyAuth middleware used by these routes.
• xmtplabs/convos-backend#267: Added related PATCH support for updating template featuring.
• xmtplabs/convos-backend#363: Directly relates to the featured-order authentication and handler changes.

Suggested reviewers: macroscopeapp, neekolas

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant Router
  participant AgentAPIKeyAuth
  participant Handler
  participant Database
  Caller->>Router: Create or patch featured template
  Router->>AgentAPIKeyAuth: Resolve API-key listener
  AgentAPIKeyAuth->>Handler: Continue with authorization context
  Handler->>Database: Create or update template
  Database-->>Handler: Persisted featured state
  Handler-->>Caller: HTTP response

Loading

Poem

I’m a rabbit guarding the gallery gate,
API keys decide who may curate.
Featured blooms are checked with care,
Order flows through guarded air.
Tests hop by, confirming the state—
Secure little templates, nice and straight!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly reflects the main change: featured-template curation is moved from owners to the dashboard/API-key flow.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch saul/featuring-is-admin-only

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[macroscopeapp]

Approvability

Verdict: Needs human review

This PR modifies authentication middleware and authorization logic for the featured gallery endpoints. Changes to auth/security pathways warrant human review, and all modified files are owned by xmtplabs/engineering rather than the PR author.

^{You can customize Macroscope's approvability policy. Learn more.}

[saulmc]

Took one of the three. Adjudicating all of them.

✅ Whitespace-brittle assertions — taken (699779c)

Fair, and worth more than it looks: a source-level assertion that fails on a reflow is noise, and a test that cries wolf is a test the next person weakens. They match the chain whitespace-agnostically now.

The interesting part is what fixing it surfaced. The naive regex is wrong:

const chain = (middleware: string, handler: string) =>
  new RegExp(`(?<![A-Za-z])${middleware},\\s+${handler}\\b`);

That lookbehind is load-bearing. Without it, agentApiKeyAuth matches inside authOrAgentApiKeyAuth — and those two differ by exactly the thing the assertion exists to tell apart: whether a signed-in caller can reach this endpoint. The original toContain happened to survive only because of a capital A. Both permissive middlewares are now named in explicit negative assertions rather than left to case-sensitivity to catch.

I mutation-checked the guard rather than trusting it: re-wiring /featured-order back to authOrAgentApiKeyAuth + requireAccount turns the test red; reverting turns it green.

❌ PATCH concurrent-modification 409 — the premise doesn't hold

the updateMany pins to the row state including featured and featuredRank. If a concurrent request changes featured, this update gets a 409.

It doesn't. The WHERE pins id, ownerAccountId, slug, status, firstPublishedAt — not featured, and not featuredRank:

where: {
  id: template.id,
  ownerAccountId: template.ownerAccountId,
  slug: template.slug,
  status: template.status,
  firstPublishedAt: template.firstPublishedAt,
},

So the scenario described — an owner editing description while the dashboard features the template — does not 409. Only a concurrent change to status/slug/owner/first-publish trips it, which is the intent (those are the fields the handler's own transition rules were computed against). That optimistic-concurrency clause is also pre-existing (#199) and untouched here.

❌ Extract a shared canFeature helper — declining

Two call sites, and they aren't the same call: each carries its own log context (create.featured vs patch.featured) and its own refusal. Wrapping featured === true && !isApiKeyListener in a named predicate adds a hop without adding meaning — the rule is already stated once in each place it's enforced, next to the reason it's enforced there. Same reasoning I gave for hasDuplicates on #363.

Full suite green (1492).