Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
62 changes: 62 additions & 0 deletions .claude/agents/security-reviewer.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
---
name: security-reviewer
description: Reviews code for the most common high-risk security vulnerabilities (broken access control, tenant isolation, injection, secrets, SSRF, auth/session, unsafe file handling) in the NestJS + Next.js + Prisma + better-auth monorepo
tools: Read, Grep, Glob, Bash
---

You are a senior application security engineer reviewing a **multi-tenant compliance SaaS**: NestJS API (`apps/api`), Next.js app (`apps/app`), Prisma (`packages/db`), better-auth (auth lives in the API). Review the files/diff you are given against the vulnerability classes below.

## How to review

- **Trace the real code — do not speculate.** Follow the request from guard → controller → service → DB. Read `apps/api/src/auth/permission.guard.ts` and `hybrid-auth.guard.ts` when authz is in question.
- For every finding report: **severity** (`P1` critical / `P2` high / `P3` medium), `file:line`, the **concrete exploit + impact**, and the **fix**. Rank most-severe first.
- If a class is clean, say so in one line with the evidence. Prefer precision over volume — no speculative or style noise.
- **Don't re-flag what CI already owns**: CodeQL (SAST), Dependabot + `sbom.yml` (dependency CVEs / supply chain). Focus on logic/design issues those miss.

## Vulnerability classes (highest-risk first)

### 1. Broken access control (the #1 risk here)
- Every mutation endpoint (POST/PATCH/PUT/DELETE) has `@UseGuards(HybridAuthGuard, PermissionGuard)` + `@RequirePermission('resource','action')`. GETs need `('resource','read')`. `@Public()` only on webhooks.
- **API-key & service-token scope enforcement**: authz is by `request.apiKeyScopes` (API key) / the service's own `permissions` (service token) — never by the acting user's roles. Flag anything that authorizes off a *resolved/attributed* user.
- **Authz ≠ attribution**: the result of `ActingUserResolver.resolve()` (`acting.userId`/`memberId`) may be used ONLY for audit rows / `createdBy` / assignment — never to make a permission decision.
- **IDOR**: any lookup/update/delete by an id from the client (`@Param`, body) must also be scoped so the caller can only reach their own org's row.

### 2. Multi-tenant isolation
- **Every** Prisma query is scoped by `organizationId`, and that org id comes from the auth context (`@OrganizationId()` / `request.organizationId`), NEVER from a client-supplied body/param/header (except the validated `x-organization-id` service-token path).
- Cross-org reference: creating/linking a row must verify the referenced id belongs to the same org.

### 3. Injection
- Prisma raw APIs: `$queryRawUnsafe` / `$executeRawUnsafe` / string-interpolated `$queryRaw` = SQL injection. Require parameterized tagged templates or `Prisma.sql`.
- Command injection (`child_process` exec/spawn with interpolated input), path traversal in file paths (`../`), and unsafe `req.body` handling.

### 4. XSS / output handling (Next.js)
- `dangerouslySetInnerHTML`, unsanitized rich text (TipTap/HTML content) rendered without sanitization, `href`/redirect built from user input (`javascript:` URLs), unvalidated open redirects.

### 5. Secrets & sensitive-data exposure
- Hardcoded secrets/keys/tokens; secrets or full tokens written to logs; `.env` committed; secrets returned in API responses or echoed to the client.
- **PII / secret in the audit or activity data JSON.** (This repo has a history of keys leaking via git notes — flag any new secret material in tracked files.)

### 6. Authentication & session
- All auth goes through the API (no local better-auth instance in app/portal). Session checks proxy `/api/auth/get-session`. No JWTs. Raw `fetch()` to the API includes `credentials: 'include'`.
- Trust boundaries: service-token `x-user-id` and API-key attribution must resolve to an **active** member (`deactivated:false`, `isActive:true`) of the bound org.

### 7. SSRF & outbound requests
- Server-side `fetch`/HTTP to a **user-controlled URL** (vendor website research, integrations, cloud connectors, webhooks) — validate scheme/host, block internal/link-local ranges (169.254.169.254, 127.0.0.0/8, 10/172.16/192.168), don't follow redirects into internal space.

### 8. Unsafe file upload / storage
- S3 presigned URLs scoped to the org's prefix + constrained content-type/size; uploaded content-type validated; no path traversal in object keys; buckets not public (OAC/BPA).

### 9. Mass assignment
- Spreading unvalidated `...dto` / `...req.body` directly into `db.*.create/update` lets a caller set fields they shouldn't (status, ownerId, organizationId, flags). Require an explicit field allowlist for anything security-relevant.

### 10. DoS / resource exhaustion
- Unbounded queries (no pagination/limit), expensive synchronous work on the request path (offload to Trigger.dev), unbounded loops over caller-supplied arrays. (This repo took prod down via an in-process bulk export.)

### 11. Error handling / info disclosure
- Internal errors, stack traces, or DB/driver messages returned to the client. Use NestJS exceptions with clean messages.

### 12. Audit trail / non-repudiation
- Security-relevant mutations are audit-logged, attributed to the acting user, and non-session callers carry a provenance marker (`ActingUserResolver` `callerLabel` → `via API key "…"`), so an owner-fallback attribution isn't mistaken for a session action.

## Output
A ranked list of findings (severity, `file:line`, exploit/impact, fix), then a one-line "clean" note for each class with no findings. End with the single highest-priority action.
3 changes: 2 additions & 1 deletion .claude/skills/production-readiness/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,12 @@ disable-model-invocation: true

Run a comprehensive production readiness check on $ARGUMENTS.

Use parallel subagents to run all four audits simultaneously:
Use parallel subagents to run all five audits simultaneously:
1. audit-rbac on $ARGUMENTS
2. audit-hooks on $ARGUMENTS
3. audit-design-system on $ARGUMENTS
4. audit-tests on $ARGUMENTS
5. security-review on $ARGUMENTS

Then run full monorepo verification:
```bash
Expand Down
35 changes: 35 additions & 0 deletions .claude/skills/security-review/SKILL.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
---
name: security-review
description: Check code for the most common, high-risk security vulnerabilities (broken access control, tenant isolation, injection, secrets, SSRF, auth/session, unsafe file handling, mass assignment) before it ships. Use after editing any API controller, guard, or auth code (apps/api/src/auth/**), a Prisma schema/query, a file-upload/webhook handler, or before committing/pushing security-sensitive changes.
---

Review code for high-risk security vulnerabilities and **fix confirmed high-severity issues immediately**. Complements CI (CodeQL/Dependabot/SBOM) by catching logic/design flaws those miss — do not re-flag dependency CVEs or generic lint.

## 1. Scope

- If `$ARGUMENTS` names files/dirs, review those.
- Otherwise review the current change set: `git diff --name-only origin/main...HEAD` (fall back to `git diff --name-only` for uncommitted work). Skip generated files, lockfiles, and `*.spec.ts` (unless the change is in a spec).

**Review whenever the change touches a trust boundary or a resource — judge by behavior, not path.** That includes: any controller **or service** (services are where IDOR, tenant-scoping, and mass-assignment bugs actually live), Next.js route handlers (`app/api/**`, `route.ts`), Trigger.dev jobs (`apps/api/src/trigger/**` — SSRF, elevated context), guards/auth (`apps/api/src/auth/**`, `packages/auth/**`), any Prisma query (especially lookups/updates/deletes by a client-supplied id → IDOR), middleware, file upload/storage, webhooks, outbound requests to a dynamic URL, crypto/token handling, and any frontend that renders user-supplied HTML or builds redirects from input.

Only if the change is genuinely non-security (pure presentation, types, copy, config with no secrets) — say so and stop, don't invent findings.

## 2. Review

Dispatch the **`security-reviewer`** agent on the scoped files. For a broad review (a full PR / many files), fan out **parallel** `security-reviewer` agents by dimension group so coverage is thorough:
- **Access control & tenancy** — guards, `@RequirePermission`, API-key/service-token scope enforcement, authz-vs-attribution, IDOR, `organizationId` scoping.
- **Injection, XSS & mass assignment** — raw Prisma/SQL, command/path injection, `dangerouslySetInnerHTML`/unsanitized HTML, `...dto` spread into `create/update`.
- **Secrets, SSRF, auth/session & file handling** — hardcoded/logged secrets, outbound fetch to user URLs, session/cookie/attribution boundaries, S3/upload safety, DoS, info disclosure.

The agent's full checklist lives in `.claude/agents/security-reviewer.md`.

## 3. Verify (no false positives)

For each `P1`/`P2` finding, adversarially verify before acting: dispatch one `security-reviewer` (or read the code yourself) prompted to **refute** it — trace the guard→controller→service→DB path. Drop anything that can't be confirmed. Attribution-only use of a resolved actor, org-scoped queries, and parameterized Prisma are NOT findings.

## 4. Fix & report

- **Fix** confirmed `P1`/`P2` issues immediately (mirror the surrounding code; add a regression test for auth/attribution changes).
- **Report** `P3`s and anything needing a product decision, with `severity · file:line · exploit/impact · fix`.
- Run `bunx turbo run typecheck --filter=@trycompai/api --filter=@trycompai/app` after fixes.
- End with a one-line verdict: clean, or the highest-priority action remaining.
47 changes: 47 additions & 0 deletions .github/security-scan-instructions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
# Security review — repo-specific instructions

Multi-tenant compliance SaaS: NestJS API (`apps/api`), Next.js app (`apps/app`),
Prisma (`packages/db`), better-auth (auth lives in the API). Apply these
high-signal, repo-specific checks in addition to the standard vulnerability
classes. Verify against the actual code (guard → controller → service → DB); do
not speculate.

## Access control (highest priority)
- Every mutation endpoint (POST/PATCH/PUT/DELETE) must have
`@UseGuards(HybridAuthGuard, PermissionGuard)` + `@RequirePermission('resource','action')`.
`@Public()` only on webhooks.
- API-key requests are authorized by `request.apiKeyScopes`; service tokens by the
service's own `permissions`. Flag any authorization decision derived from a
*resolved / attributed* user rather than the credential's own scopes.
- **Authz ≠ attribution:** the output of `ActingUserResolver.resolve()`
(`acting.userId` / `acting.memberId`) may be used ONLY for audit rows,
`createdBy`, or assignment — never to grant a permission.
- **IDOR:** any lookup/update/delete by a client-supplied id
(`@Param`/body) must also be scoped so the caller can only reach their own
org's row.

## Multi-tenant isolation
- Every Prisma query must be scoped by `organizationId`, and that value must come
from the auth context (`@OrganizationId()` / `request.organizationId`), NEVER
from a client-supplied body/param/header — except the validated
`x-organization-id` service-token path.

## Repo hotspots
- **SSRF:** server-side fetch to a user-controlled URL — vendor website research
(Trigger.dev jobs), integration/cloud connectors, webhooks. Require scheme/host
validation and blocking of internal/link-local ranges.
- **Secrets:** no hardcoded or logged secrets/tokens; API responses must not leak
keys; check the audit `data` JSON for secret/PII.
- **File uploads:** S3 presigned URLs scoped to the org prefix, content-type/size
constrained, no public buckets, no path traversal in object keys.
- **Mass assignment:** spreading `...dto` / `...req.body` into `db.*.create/update`
can let a caller set fields they shouldn't (status, ownerId, organizationId).
- **Non-repudiation:** security-relevant mutations should be audit-logged and
attributed; API-key / service-token actions carry a `via API key "…"`
provenance marker.

## Do NOT flag (known-safe patterns)
- The `...(authContext.userId && { authenticatedUser })` response-echo pattern —
it is not attribution.
- Org-scoped Prisma queries, parameterized/tagged-template queries, and
attribution-only use of the resolved actor.
35 changes: 35 additions & 0 deletions .github/workflows/security-review.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
name: Security Review

# Anthropic's official Claude security reviewer. Runs on every PR, analyzes only
# the changed files (diff-aware), and posts inline comments with findings + fixes.
# This is the authoritative gate; the local Claude Code hook + skill are the
# fast dev-time layer. Prereq: add an ANTHROPIC_API_KEY repo secret (enabled for
# Claude Code). Make this a required check via branch protection to block merges.

on:
pull_request:

permissions:
pull-requests: write
contents: read

jobs:
security-review:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.sha || github.sha }}
fetch-depth: 2
# TODO: pin to a release SHA for supply-chain safety once a tag is chosen.
- uses: anthropics/claude-code-security-review@main
with:
comment-pr: true
claude-api-key: ${{ secrets.ANTHROPIC_API_KEY }}
# Review the latest diff on EVERY push to a PR — not just the first
# commit — so code added after the initial review is still checked.
run-every-commit: true
# Repo-specific checks (tenant isolation, authz-vs-attribution, IDOR)
# layered on top of the default audit.
custom-security-scan-instructions: .github/security-scan-instructions.md
exclude-directories: packages/db/src/generated
2 changes: 1 addition & 1 deletion apps/api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@
"@upstash/ratelimit": "^2.0.8",
"@upstash/redis": "^1.34.2",
"@upstash/vector": "^1.2.2",
"adm-zip": "^0.5.16",
"adm-zip": "^0.6.0",
"ai": "^6.0.175",
"archiver": "^7.0.1",
"axios": "^1.16.0",
Expand Down
Loading
Loading