# Backend Code Quality Guide Things backend code should **at least** handle. This is a floor, not a standard: it is deliberately not exhaustive, and clearing everything here does not make a change correct. Expect it to grow as we find more worth checking. `backend/AGENTS.md` still covers where code goes and which pattern to follow. - [Errors users can understand](#errors-users-can-understand) - [Validate every API input](#validate-every-api-input) - [Paginate external API calls](#paginate-external-api-calls) - [Do not create deadlock conditions](#do-not-create-deadlock-conditions) - [Intuitive API interfaces](#intuitive-api-interfaces) - [Audit log events admins can read](#audit-log-events-admins-can-read) - [License checks](#license-checks) --- ## Errors Users Can Understand The person reading an error is not looking at our code. If the message only makes sense to someone who is, it is not a usable error. **Write the message for the user, not for the developer who wrote the throw.** Say what went wrong in product vocabulary, and where possible what to do about it. Technical detail belongs in the logs. ```ts // Bad: internal vocabulary, means nothing to the caller throw new BadRequestError({ message: "kmsKeyId resolution failed for orgId scope" }); // Good: the caller learns what is wrong and what to do next throw new BadRequestError({ message: `Project '${projectName}' has no Gateway assigned. Assign one under Project Settings > Gateways before creating a PAM resource.` }); ``` Include the identifier the caller actually passed, so the failure is reproducible: `PKI subscriber named '${subscriberName}' not found` is the existing house style. Quote it so empty strings and stray whitespace are visible. ### No pointless 500s A 500 with a generic message is the worst outcome for a user. Nothing tells them whether to fix their request, retry, or open a ticket, so it just blocks them. Most 500s are a failure we could have anticipated and named. - If the caller's input caused it, it is a `BadRequestError` naming the field. - If an upstream dependency failed, catch it and say which one, in terms the user recognizes (the provider, not our internal service name). - Never let a raw Knex or SDK error reach the client. Its message can carry column names, constraint names, and SQL fragments, and it tells the user nothing useful. - A 500 that does survive still needs a log line with full context and identifiers. Use the classes in `src/lib/errors/index.ts`. One thing to get right: a resource in another org returns `NotFoundError`, not `ForbiddenRequestError`, so the API cannot be used to confirm that another tenant's resource IDs exist. Never interpolate a secret, token, key, connection string, or full outbound URL into a message. Errors get logged and pasted into tickets. For URLs, use `sanitizeUrlForLog` from `@app/lib/logger`. --- ## Validate Every API Input **Every parameter and body property a user can send needs an explicit constraint**: a length bound, and where it applies, the allowed character set. That is what makes a bad request fail immediately with a message naming the problem, instead of failing deep in a service or landing in the database as a row nothing else can handle. `z.string()` is almost never right. It accepts a 40MB string. ```ts // Bad: unbounded, untrimmed, accepts "" and " " name: z.string(), // Good: reuse the shared helpers in src/server/lib/schemas.ts name: GenericResourceNameSchema, slug: slugSchema({ max: 32, field: "Environment slug" }), orgId: z.string().uuid(), projectId: z.string().trim().min(1).max(36), description: z.string().trim().max(500).optional(), port: z.number().int().min(1).max(65535), ``` - **Reuse the shared schemas** (`slugSchema`, `GenericResourceNameSchema`, `SecretNameSchema`) instead of re-deriving the rules, so one endpoint does not accept a name that another rejects. - **Bound every string with `.max()`**, matched to the real column width. - **`.trim()` anything used as an identifier**, so `"prod "` and `"prod"` are not two rows. - **IDs are `.uuid()`; enums are `z.nativeEnum(...)` or `z.enum([...])`**, so the accepted values appear in the generated docs. - **Except `projectId`.** `projects.id` is `varchar(36)`, not `uuid`, and projects carried over from MongoDB kept their 24-character ObjectId as their ID. Customers hardcode that ID in `.infisical.json`, CI, and operator manifests, so it cannot be rewritten. `.uuid()` rejects those projects, and no project created since the Postgres move will show it. Check the column in `src/db/schemas/` before adding `.uuid()` to any other ID declared as `t.string("id", 36)`. - **`z.coerce.number().int()`** for querystring numbers, since everything arrives as a string. - **Never `.default(x).optional()`.** The `.optional()` wraps the default, so `undefined` stays `undefined` and the default never applies. Zod only validates shape and bounds. Rules that need to read other rows (does this `caId` exist, is it in this project, is this state transition legal) belong in the service, and every ID the caller supplies must be checked against the org or project the request is authorized for, not just checked for existence. --- ## Paginate External API Calls When we list resources from a third-party API (Cloudflare zones, GCP projects, CircleCI env vars, and every other app connection or secret sync), **the first response is usually one page, not the whole set.** Most providers default to 20 to 50 items and say nothing about the rest. If we read `data.result` and return it, the list is silently truncated and the user gets a resource picker missing the zone they need. This is a live bug source, not a hypothetical. Treat any request that returns a list as paginated until you have checked the provider's docs and confirmed it is not. ```ts // Cursor / page-token style (GCP, CircleCI): loop until the token is gone let pageToken: string | undefined; do { const { data } = await request.get(url, { params: { pageSize: 100, pageToken } }); items.push(...data.projects); pageToken = data.nextPageToken; } while (pageToken); // Page-number style (Cloudflare): ask for the max page size, follow total_pages while (page <= totalPages && page <= MAX_PAGES) { const { data } = await request.get(url, { params: { page, per_page: PER_PAGE } }); items.push(...data.result); totalPages = data.result_info?.total_pages ?? 1; page += 1; } ``` - **Request the provider's maximum page size**, so the common case stays one round trip. - **Follow the provider's own signal** (`nextPageToken`, `has_more`, `total_pages`, a `Link` header), never an assumption about how many pages exist. - **Cap the loop.** An unbounded `while` over a third-party API hangs if the provider's cursor never clears. See `CLOUDFLARE_MAX_PAGES` in `src/services/app-connection/cloudflare/cloudflare-connection-fns.ts`. - **Do not truncate silently.** If you hit the cap, log it. In that same Cloudflare file, `listCloudflareZones` paginates correctly while `listCloudflarePagesProjects` and `listCloudflareWorkersScripts` return only the first page. That is exactly the bug, and it is easy to write by accident. --- ## Do Not Create Deadlock Conditions **Each instance gets about 10 database connections** (`DB_POOL_MAX`, default 10). That is the entire budget, and it fills up far more easily than it looks. The deadlocks we hit are not exotic lock-ordering problems, they are ordinary code exhausting that pool. They are also very hard to catch while developing, because everything passes when you are the only request. **This is the check we most need help with: spot the trigger in review, because we will not see it until production.** The two triggers to look for are a query inside a transaction that does not use the transaction, and a transaction held open longer than it needs to be. ### A missed `tx` costs a second connection Every DAL method takes an optional trailing `tx`. `ormify` resolves the connection as `(tx || db)` for writes and `(tx || db.replicaNode())` for reads, so a call that omits `tx` does not join your transaction: it checks out **a second connection** and runs outside it. (`db.replicaNode()` falls back to the primary pool when no read replica is configured, the normal self-hosted case, so a stray read does not escape to another pool either.) One request now holds two connections at once. Once concurrent requests on that path reach the pool size, every request is holding its transaction connection and waiting for a second one that nothing can free. That is a deadlock rather than slowness: it does not clear when load drops. With a pool of 10, the threshold is ordinary traffic. ```ts // WRONG: these two run on separate connections, outside the transaction await projectDAL.transaction(async (tx) => { const project = await projectDAL.findById(projectId); // <- missing tx const env = await projectEnvDAL.findOne({ projectId }); // <- missing tx await projectDAL.updateById(projectId, { name }, tx); }); ``` **Inside a `transaction()` callback, every DB call passes `tx`**, with no exception for reads or for "just a quick lookup". The same applies through helpers: a function that touches the database and can be called from inside a transaction must accept `tx?: Knex` and forward it, or everything it does happens outside the transaction. Do not open a nested `transaction()` (pass the existing `tx` down), and do not use `requestMemoize` inside one, since it can hand transactional code a value read outside it. Reviewing for this means reading the callback and checking that **every `await` on a DAL method ends in `tx`**. A trailing `)` where you expected `, tx)` is the whole bug. ### A slow transaction holds its connection the whole time A transaction pins one connection from `BEGIN` to `COMMIT` and holds its locks for that entire window. A transaction that takes 4 seconds against a pool of 10 caps that path at roughly 2.5 requests per second and starves everything else sharing the pool. Long transactions also block vacuum across the whole database, so one slow writer degrades tables it never touched. **Nothing slow or unpredictable goes between `BEGIN` and `COMMIT`:** - **No network calls.** No HTTP, no AWS/GCP/Azure SDK calls, no email, no Slack or webhook delivery, no gateway round-trips, no external KMS or HSM. An upstream that takes 30 seconds to time out holds your connection and your locks for 30 seconds. - **No waiting.** No sleeps, no polling, no retry loops. - **No expensive CPU work.** Key generation, certificate signing, password hashing, bulk encrypt/decrypt. Node is single-threaded, so this blocks every other request as well. - **No unbounded row counts.** A transaction whose size is set by tenant data will eventually exceed any timeout. Chunk it, each chunk in its own transaction with a per-batch `SET LOCAL statement_timeout`. The shape that works: read and validate, do the expensive and external work, then open a short transaction for only the writes that must be atomic together, then fire side effects after commit. **Enqueue queue jobs after commit**, never inside, or a worker can pick up a job for a row that has not committed yet. --- ## Intuitive API Interfaces **We align to REST.** Resources are nouns, the verb lives in the HTTP method, status codes mean what they mean, and `GET` never mutates. An API is also a contract we cannot take back, so before writing the handler, check that a competent engineer could call the endpoint correctly from the OpenAPI docs alone. - **Resource-shaped URLs, plural nouns, the verb in the method.** Genuinely non-CRUD actions get a sub-path (`POST /pki/subscribers/:id/issue-certificate`). What they do not get is an `action: "grant" | "revoke"` field in the body that switches behavior. That is two endpoints. - **`GET` is safe, `PUT`/`DELETE` are idempotent.** A `GET` that writes breaks caches, retries, and prefetching. Repeating a `PUT` or `DELETE` must not change the outcome. - **Status codes carry meaning.** 201 with the created resource, 204 for a delete with no body, 404 for a missing or out-of-scope resource, 409 for a genuine conflict. Never return 200 with an error inside the body. - **Match names that already exist.** Use `projectId`, not `workspaceId` (which now survives only in `deprecated-*` routers). Timestamps are `createdAt` / `updatedAt` in UTC ISO 8601. Durations carry their unit (`ttlSeconds`), never a bare `ttl`. - **Return the resource, not a receipt.** `POST` and `PATCH` return the full object so the client does not need a follow-up `GET`. `{ "success": true }` is not a response. - **Wrap it in a named key** (`{ subscriber: {...} }`) so the shape can be extended later. - **Select response fields deliberately.** Never return a DB row wholesale: rows carry `encrypted*` / `hashed*` columns and internal FKs, and every accidental field becomes a contract we owe. - **The schema is the docs.** Routes use the Zod type provider, so `operationId` and a `.describe()` on every field are part of the deliverable (put the strings in `src/lib/api-docs/`). Every route also needs `config.rateLimit` and an `onRequest: verifyAuth([...])` listing only the auth modes that genuinely need access. - **Adding an optional field is safe; everything else is not.** Renaming a field, tightening validation, changing a default, or changing a status code breaks callers. Add a new route version and deprecate the old one. ### When a design has to break REST Sometimes it genuinely does: a bulk operation that cannot be expressed per-resource, a batch endpoint, an RPC-shaped action, matching a protocol we do not control (ACME, SCIM, SCEP), or staying consistent with a neighbouring endpoint that already deviates. **Do not just implement it, and do not silently "fix" it either. Point it out.** Say which REST expectation the design breaks and what it costs, propose the conforming alternative, and let the author confirm the deviation is intentional. An intentional deviation is fine and should be noted in a comment on the route so the next person does not read it as an accident. An unintentional one is much cheaper to catch now than after customers are calling it. --- ## Audit Log Events Admins Can Read An audit log event is read by an admin investigating an incident, in a UI that renders the `metadata` body as raw JSON, or in their own SIEM. **Name the field after what it identifies.** `requestId` on an event that carries an approval request's id sends the reader looking for a certificate request that does not match. Prefer `approvalRequestId`. **Record the outcome, not just the attempt.** A handler that logs the same event whether the operation succeeded, failed, or is awaiting approval produces a log that cannot answer "did this happen?". Where a service returns a status, put it in the metadata, and only emit result fields (a new id, a serial number) when the result actually exists. **Renaming, retyping, or removing a field on an existing event is a breaking change.** These bodies go to customer SIEMs unchanged, so a field that switches from string to number, or disappears, silently breaks rules built on it. Adding a field is safe; changing one needs a deliberate call. --- ## License Checks A license check answers exactly one question: **is this org entitled to use this feature right now?** It must never be allowed to answer a second one, which is whether the data the org already has still means what it meant yesterday. Plans move. A customer gets a feature, builds on it, and later downgrades. So: **Check the license when someone creates, enables, or edits something that uses the feature. Do not check it when reading, resolving, evaluating, listing, or deleting.** Losing an entitlement makes a feature read-only. It does not delete, hide, or reinterpret what the customer built while they had it. ### The failure this prevents #### Example 1 An org had cross-project secret sharing and used it to structure real projects, with folders pulling secrets in from another project. Then they downgraded. `expandSecretReferences` was consulting `plan.crossProjectSecretSharing` on the read path (`checkCrossProjectAllowed` in `src/services/secret-v2-bridge/secret-reference-fns.ts`), so every cross-project reference stopped resolving and their applications started reading empty values from a `GET` they had not changed. Nobody wrote anything. Nothing was deleted. Their secrets simply stopped being their secrets. That shape is not specific to secrets management. Any license check on a read path gives a customer a different answer to the same request depending on their billing state, and the customer finds out at runtime, in production, on a request they did not make. #### Example 2 Custom project roles are a paid feature, and they show what the correct outcome looks like. An org on a paid plan creates custom roles and assigns them to users. Later they downgrade to a free plan. Those roles keep working: the rows stay, the assignments stay, and every user holding one keeps exactly the access that role grants. `permission-service` never reads the plan while it builds a CASL ability, so a downgrade cannot quietly narrow someone's access or hand them a different set of permissions than they had the day before. What the downgrade takes away is the ability to change the set. Creating a new custom role and editing an existing one both fail with a message naming the plan, and the UI opens the upgrade modal on those same two actions. `role-service.ts` is the model to copy: `createRole` and `updateRole` check `plan.rbac`, while `listRoles`, `getRoleBySlug`, and `deleteRole` check nothing. One rule produces both outcomes. Entitlement decides what a customer can build. It never decides what an already-built thing does. ### What that means in practice - **The plan must never change a response.** Not its shape, not its contents, not whether a reference resolves. Same request and same data means the same answer, whatever the license says. - **Gate the write that introduces the dependency**, at the moment the customer asks for the feature: creating a cross-project grant, creating a secret import from another project, creating or updating a custom role, turning a configuration on. - **Never gate a delete or a revoke.** A downgraded customer has to be able to remove the thing they can no longer create. `deleteGrant` in `src/services/project-folder-grant/project-folder-grant-service.ts` refuses without the entitlement, so the grant stays in place with no way to take it back. - **Never gate a list, a read, or a usage lookup of the feature's own records.** `listGrantsByProject` in that same file returns `[]` once the entitlement is gone, so an admin cannot see, audit, or clean up grants that are still live and still resolving. Hiding a record does not disable it, it only removes the operator's way to deal with it. - **Existing objects keep working, and keep being enforced.** Whatever a customer built while entitled stays in effect after the downgrade, exactly as it was. Only creating and editing it is blocked (Example 2). - **Quantitative limits are write-path checks too.** Enforce a seat, project, or retention cap where the count grows. Never enforce one by hiding or dropping rows that already exist. - **Audit every call site of an entitlement helper, not just the helper.** A predicate such as `isCrossProjectEnabled` is fine to share; one caller of it on a read path is the bug. - **Do not hide the feature in the UI either.** Leave the entry point where it is, and when an unentitled user tries to use it, open `UpgradePlanModal` (`frontend/src/components/license/UpgradePlanModal`) so they learn what the feature is and how to get it. A hidden feature teaches a customer nothing and looks like it does not exist. The modal is the frontend half of the same rule the backend follows: the block lands on the attempt to create or edit, never on viewing what is already there.