fix(approvals): force create attribution from the actor + per-agent creation caps

[claudegoogl-sudo]

Follows CONTRIBUTING.md PR template — sections present: Thinking Path, Linked Issues or Issue Description, What Changed, Verification, Risks, Model Used, Checklist.

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work.
• Agents create approval cards with POST /companies/:companyId/approvals. The route copied a requestedByAgentId body field into the new card.
• Any agent could set that field to another agent's id. The card then showed the wrong requester. The wrong agent also gained the requester-only rights that follow the field (for example the resubmit guard on POST /approvals/:id/resubmit).
• The route also had no creation cap. A looping or compromised agent could open new cards without limit and flood reviewer queues.
• This pull request forces attribution from the authenticated actor. A mismatched requestedByAgentId now gets a 403. A user or board caller that sends the field gets a 400. Rejected attempts are logged and written to the activity log.
• It also adds two per-agent caps, both returning 429: a sliding-window burst cap and a pending-card cap. User and board callers are exempt from both.
• The benefit: an approval card can no longer be attributed to an agent that did not ask for it, and card creation is bounded per agent.

Linked Issues or Issue Description

Refs #296 (related work in the same file: request-payload requirements and the agent withdraw route; this change is independent and stays conflict-free with it).

Described in-PR (no public issue exists for this yet):

What happened?

The approval create route accepted requestedByAgentId from the request body for every caller. An agent-authenticated caller could set it to any agent's uuid. The created card was then attributed to that other agent: the card showed the wrong requester, wakeups went to the wrong agent, and the requester-only resubmit right followed the forged field. The route also had no per-agent creation cap, so an agent could create unlimited pending cards.

Expected behavior

An approval card is always attributed to its real requester. An agent caller may omit requestedByAgentId (then the card is attributed to the caller) or echo its own id. Any other value is rejected with 403 and no row is created. User and board callers must not send the field; a non-null value is rejected with 400. Agent callers are capped: at most 10 creates per 60-second sliding window, and at most 5 simultaneously pending cards. Exceeding either cap returns 429 and creates no row.

Steps to reproduce

On a build without this change, as an agent with a valid agent API key:

curl -X POST https://<host>/api/companies/<companyId>/approvals \
  -H "Authorization: Bearer <agent-api-key>" \
  -H "Content-Type: application/json" \
  -d '{"type":"approve_ceo_strategy","requestedByAgentId":"<another-agent-uuid>","payload":{}}'

The request returns 201 and creates a card attributed to the other agent. With this change it returns 403, creates no row, and logs approval.create_denied.

Paperclip version or commit

Fork master at b2d6b8bb0 (this branch bases on it).

Deployment mode

Any deployment that issues agent API keys (single-node and multi-node alike). No database migration is involved.

Access context

The spoof path needs a valid agent API key for the target company, so the caller already passes tenant checks. The forged field crosses an authorization boundary inside the tenant: it changes who owns the approval card and who may act on it.

What Changed

• server/src/routes/approvals.ts:
  • Attribution guard on the create route. Agent actor + body requestedByAgentId present and different from the caller → 403, no row. Equal to self → accepted (idempotent clients keep working). Omitted → falls back to the caller. User/board actor + non-null field → 400, no row.
  • The created row now always takes requestedByAgentId from the authenticated actor. The body value can no longer influence attribution even if a guard regresses.
  • Rejected attempts emit a structured warn log plus an activity-log row (approval.create_denied). The log names the rule; details never include payload contents.
  • Burst cap: limiter inspect before the create, 429 with Retry-After when exhausted. The hit is recorded only after the row is created, so a rejected request never spends budget.
  • Pending-card cap: count of the agent's pending cards before the create, 429 naming the cap and the remedy when at the cap. No Retry-After — the cap frees on a human resolve or withdraw, not on a clock.
  • This route has no idempotency or dedupe path: every successful create consumes exactly one burst hit. A retried create is a second card, so it is billed a second hit. Rejected requests are never billed.
• server/src/services/approval-create-rate-limit.ts (new):
  • createApprovalCreateRateLimiter() built on the shared sliding-window store (sliding-window-rate-limit-store.ts) — the same primitive as the plugin webhook limiter. No new dependency, sweep-on-write, bounded live keys, module-scope default so re-registering routers cannot reset the ceiling.
  • countPendingApprovalsForAgent() for the pending-cap budget check.
• packages/shared/src/constants.ts: APPROVAL_CREATE_RATE_LIMIT_WINDOW_MS (60s), APPROVAL_CREATE_RATE_LIMIT_MAX_PER_AGENT (10), APPROVAL_CREATE_PENDING_CARD_CAP_PER_AGENT (5), each with rationale. Values are a starting point; happy to tune them.
• server/src/__tests__/approval-routes-hardening.test.ts (new): 12 route-level tests that drive the real router.
• docs/api/approvals.md: documents the attribution rules, the approval.create_denied activity action, and both caps.

Internal callers of the approvals create service (not the HTTP route) were audited and are unchanged: server/src/routes/built-in-agents.ts (~L190), server/src/routes/agents.ts (~L3256 and ~L3276) — all three derive the field from the authenticated actor; server/src/services/plugin-capability-escalation.ts (~L210) — passes null (system-attributed card). None accept client input for this field.

Verification

All commands run from the repo root on this branch:

• New route-level tests: cd server && ../node_modules/.bin/vitest run src/__tests__/approval-routes-hardening.test.ts --testTimeout=120000 → 12 passed. The suite drives the real approvalRoutes router with an agent actor. Cases: spoof → 403 with row count unchanged; self-echo → 201; omitted field → 201 attributed to caller; user actor with field → 400; both caps → 429 (burst with Retry-After, pending naming cap and remedy); cap exemption for board/user actors; rejected requests never consume burst budget; per-agent bucket isolation; dedupe-path statement test.
• Mutation check (AC-style): with the route's guard and cap lines removed, 8 of 12 tests fail; with the lines restored, all pass. The red set proves the tests bind to the route, not to a helper.
• Focused regression suites: vitest run over approval-routes-idempotency, approvals-service, agents-pending-approval-config, built-in-agent-routes, built-in-agents, plus the four agents-* service suites → 137 passed across 10 files.
• Typecheck: tsc --noEmit in server/ and in packages/shared/ → both clean.
• Docs: docs/api/approvals.md create section updated.

Risks

• Behavior change: clients that (wrongly) set requestedByAgentId to another agent's id now get 403 instead of a forged card. Legitimate clients either omit the field or echo their own id, and both keep working. The MCP create tool never sends the field.
• The burst cap can in principle reject a legitimate agent that files more than 10 cards within one minute. Observed legitimate cadence is a handful per minute at most; the rejected response carries Retry-After. The constants are trivially tunable.
• The pending-cap query adds one indexed count(*) per agent create; negligible next to the create itself.
• No database migration, no schema change, no dependency change. Route-level only; the approvals service signature and all internal callers are unchanged.

Model Used

Exact model id not exposed to this session; operating as agent "Coder" under the Paperclip runtime (extended thinking + tool use: code search, edits, test execution, and GitHub CLI).

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have not referenced internal/instance-local Paperclip issues or links (only public GitHub #NNN / github.com/paperclipai/paperclip URLs)
• My branch name describes the change (e.g. docs/..., fix/...) and contains no internal Paperclip ticket id or instance-derived details
• I have run tests locally and they pass
• I have added or updated tests where applicable
• I have updated relevant documentation to reflect my changes
• I have considered and documented any risks above
• All Paperclip CI gates are green — opened just now; will tick once the first CI run completes
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — N/A: Greptile does not engage on this fork's PRs; reviewer review is the gate here
• I will address all Greptile and reviewer comments before requesting merge

[claudegoogl-sudo]

Security-sensitive change (authorization surface on the approval create route), so requesting a security review before merge per our process. Review focus: (1) the 403/400 attribution guard and the forced attribution at the row build, (2) the cap values in packages/shared/src/constants.ts (60s window, burst 10, pending 5) and whether the exempt origins are the right set, (3) the observability choice (structured warn + activity-log approval.create_denied — trim one if both is wrong).