diff --git a/.agents/review-rules/README.md b/.agents/review-rules/README.md new file mode 100644 index 00000000000..2f4ce9dc976 --- /dev/null +++ b/.agents/review-rules/README.md @@ -0,0 +1,80 @@ +# Review rules + +Rules the AI reviewer (cubic) enforces on pull requests. One rule per file, +grouped by the agent that loads it. `cubic.yaml` links these files via +`file_paths`; the prose in `cubic.yaml` stays thin so the rules are reviewable +as normal markdown. + +## Layout + +| Directory | Agent | Scope | +|-------------|----------|-----------------------------------------------------| +| `security/` | Security | backend packages + nodes | +| `backend/` | Backend | `cli`, `@n8n/db`, `core`, `workflow`, node packages | +| `frontend/` | Frontend | `packages/frontend` | +| `qa-dx/` | QA & DX | `.github`, `docker`, `scripts`, `patches`, `packages/testing`, the lint/test/TS config packages, baselines | + +One slot of five is left. QA & DX covers the build, test, and CI surface — the +same paths `.github/OWNERS` assigns to `@n8n-io/qa-dx`. Code-quality rules that +happen to apply broadly (error classes, `any`, lazy imports) are backend rules, +not QA & DX ones. + +## Limits that bite + +cubic fails silently on all three of these, which is why `pnpm check:cubic-config` +enforces them in CI: + +- **5 enabled agents per repository.** Rules past the fifth never run and cubic + says nothing. One slot is deliberately left free. +- **10,000 characters per agent**, counting the `description` plus every linked + file, concatenated in the listed order. Everything past the limit is dropped + from the review prompt. +- **Repo-relative file paths only.** Globs, directories, parent-directory + traversal, and absolute paths are all rejected — list each file explicitly. + The schema caps `file_paths` at 10 entries per agent. + +## Adding a rule + +1. Write the file in the directory for the agent that should own it. Open with a + one-line "Applies to:" so the reviewer skips it on unrelated files — a + backend PR still loads the node rules, since include globs are per-agent. +2. Add its path to that agent's `file_paths` in `cubic.yaml`. +3. Run `pnpm check:cubic-config`. It validates `cubic.yaml` against cubic's + published JSON schema, then fails on a missing path, an over-budget agent, or + a rule file nobody links, and warns at 80% of the ceiling. + +The schema is vendored at `.github/scripts/quality/cubic-config.schema.json` so +the check needs no network. `util-refresh-cubic-schema.yml` re-pulls it on the 1st +of each month and opens a PR when it changed — review that diff for new cubic +options worth adopting. To refresh by hand: +`node .github/scripts/quality/check-cubic-config.mjs --refresh`. + +Don't restate something ESLint already errors on — see the "Don't repeat the +linter" section in `cubic.yaml`. Write what static analysis cannot see. + +## Working with cubic on a PR + +Tag `@cubic-dev-ai` in a PR comment (GitHub autocomplete won't offer it — type it). +Replying to one of cubic's own comments needs no tag, but a bare reply never +authorises code changes. + +| Ask | Comment | +|-----|---------| +| Re-review everything | `@cubic-dev-ai review this PR` | +| Review only what changed since its last pass | `@cubic-dev-ai incremental review` | +| Deeper pass with the stronger models | `@cubic-dev-ai ultrareview` — or `ultrareview: focus on ` | +| Fix a finding on this branch | `@cubic-dev-ai fix this issue in this branch` | +| One-off context for this run only | `@cubic-dev-ai review this and use ` | +| Ask about a finding | reply in the thread | + +Two things worth knowing: + +- Ultrareview is billed at 3× a normal review, which is why it is manual rather + than automatic here. +- A PR over 10,000 changed lines, or carrying one of the ignored automation + labels, gets no automatic review. `@cubic-dev-ai review this PR` overrides + that when you want it. + +Disagreeing in a thread is not wasted: cubic turns feedback into team-scoped +learnings, so a well-argued "this is fine because X" shapes later reviews. Vague +replies ("done", "thanks") don't. diff --git a/.agents/review-rules/backend/controller-request-validation.md b/.agents/review-rules/backend/controller-request-validation.md new file mode 100644 index 00000000000..c0b625f299d --- /dev/null +++ b/.agents/review-rules/backend/controller-request-validation.md @@ -0,0 +1,59 @@ +# Controller request bodies must use a DTO + +Applies to: `*.controller.ts` outside `packages/cli/src/public-api` (the public +API has its own lint rules). + +Controller endpoints that accept a request body must use the `@Body` decorator +with a DTO class. TypeScript types alone provide no runtime validation — only +`@Body` with a Zod-based DTO (extending `Z.class`) validates incoming data. + +This fails quietly rather than loudly: `controller.registry.ts` validates only +when the parameter type exposes `safeParse`, and otherwise pushes nothing into +the handler's arguments at all. + +Only flag when there is positive evidence the developer intended to accept a +body but didn't use the pattern: + +1. A parameter named `payload`, `body`, or `data` without `@Body` +2. Direct `req.body` access in a controller method +3. `@Body` with a type not ending in `Dto` — suggests unawareness of the pattern + +Do NOT flag: + +- `@Body` with a type ending in `Dto` — the developer knows the pattern +- POST/PUT/PATCH endpoints with no body parameter; they may legitimately have no body +- Webhook controllers + +Violation: + +```typescript +@Post('/') +async create(req: AuthenticatedRequest, payload: CreateUser) { + // payload is undefined - missing @Body decorator +} + +@Post('/') +async create(req: AuthenticatedRequest) { + const data = req.body; // No runtime validation +} + +@Post('/') +async create(@Body payload: CreateUser) { + // Type doesn't end in Dto - likely missing Zod validation +} +``` + +Allowed: + +```typescript +@Post('/') +async create(@Body payload: CreateUserDto) { ... } + +@Post('/activate') +async activate(req: AuthenticatedRequest) { + // Legitimately no body needed +} +``` + +Use a DTO from `@n8n/api-types` or a local `dto/` directory. DTOs must extend +`Z.class` from `zod-class` for runtime validation. diff --git a/.agents/review-rules/backend/error-classes.md b/.agents/review-rules/backend/error-classes.md new file mode 100644 index 00000000000..d7763d61907 --- /dev/null +++ b/.agents/review-rules/backend/error-classes.md @@ -0,0 +1,15 @@ +# Throw a typed error, not a plain Error + +Applies to: backend packages (`cli`, `@n8n/db`, `core`, `workflow`) and the node packages. + +The `no-plain-errors` lint rule is switched off repo-wide, so nothing catches +this. Flag `throw new Error(...)` in new code and pick by cause: + +- `UserError` — the user caused it: invalid input, unauthorized action, + business-rule violation +- `OperationalError` — transient and expected: a failing network request, a DB + timeout; something to handle gracefully +- `UnexpectedError` — a bug: logic mistake, unhandled case, failed assertion + +Do not flag `ApplicationError`; the `no-application-error` ESLint rule already +fails the build for it. diff --git a/.agents/review-rules/backend/explicit-any.md b/.agents/review-rules/backend/explicit-any.md new file mode 100644 index 00000000000..35e92dafbd3 --- /dev/null +++ b/.agents/review-rules/backend/explicit-any.md @@ -0,0 +1,10 @@ +# `any` where a real type is available + +Applies to: `packages/cli`, `packages/workflow`, `packages/nodes-base`, +`packages/@n8n/nodes-langchain`. + +`@typescript-eslint/no-explicit-any` is downgraded to a warning in these +packages, and `pnpm lint` runs `--quiet`, so warnings never surface. + +Flag a new `any` there when `unknown` plus a type guard, or an existing +interface, would do. Do not flag `any` in test code. diff --git a/.agents/review-rules/backend/hand-rolled-delays.md b/.agents/review-rules/backend/hand-rolled-delays.md new file mode 100644 index 00000000000..ee244a28cff --- /dev/null +++ b/.agents/review-rules/backend/hand-rolled-delays.md @@ -0,0 +1,22 @@ +# Hand-rolled delays the linter cannot see + +Applies to: backend packages (`cli`, `@n8n/db`, `core`, `workflow`) and the node packages. + +`no-restricted-sleep-definition` catches only helpers literally named `sleep` or +`sleepWithAbort`, so it misses the same thing under another name. + +Flag a delay re-implemented as `wait`, `delay`, `pause`, `waitFor`, … whose +entire body is a promise wrapping `setTimeout`, and an inline awaited +`new Promise((resolve) => setTimeout(resolve, ms))`. + +Use `sleep(ms, abortSignal?)` from `@n8n/utils/sleep`. It takes the abort signal, +so an abort-aware local waiter is a violation too. + +Do NOT flag: + +- Wrappers that do more than wait: racing another promise, rejecting with a + timeout error, retry backoff, debounce/throttle +- Bare `setTimeout` used for scheduling rather than awaited as a delay +- Fake timer helpers such as `vi.advanceTimersByTime` +- `packages/@n8n/node-cli/**` and `packages/@n8n/typeorm/**`, which cannot depend + on `@n8n/utils` diff --git a/.agents/review-rules/backend/lazy-load-heavy-modules.md b/.agents/review-rules/backend/lazy-load-heavy-modules.md new file mode 100644 index 00000000000..51dc212d398 --- /dev/null +++ b/.agents/review-rules/backend/lazy-load-heavy-modules.md @@ -0,0 +1,11 @@ +# Lazy-load heavy or native modules + +Applies to: backend packages (`cli`, `@n8n/db`, `core`, `workflow`) and the node packages. + +A top-level `import` of a module used only on a specific code path loads it into +every process at startup, raising baseline memory. Native modules (e.g. +`isolated-vm`) can crash instances that lack the binary. Large parsers (e.g. +`jsdom`, ~16 MB heap) waste memory when the path is rarely hit. + +Fix: `await import()` at point of use. For barrel files, use `export type` +instead of a value re-export when consumers only need the type. diff --git a/.agents/review-rules/frontend/design-system.md b/.agents/review-rules/frontend/design-system.md new file mode 100644 index 00000000000..480b727f5ed --- /dev/null +++ b/.agents/review-rules/frontend/design-system.md @@ -0,0 +1,15 @@ +# Design system enforcement + +Applies to: `packages/frontend`. + +The design system skill linked alongside this file is the source of truth for +which token to reach for. This file sets the enforcement level. + +Stylelint validates CSS custom-property *names* but never their values, so +nothing catches a hard-coded one. + +- Strong warning: hard-coded visual values (px, rem, hex colours, durations) + where a token exists; legacy token usage; deprecated style or component + surfaces. +- Soft warning: token-to-token substitutions. Ask for intent rather than + asserting a regression — a deliberate change looks identical to a mistake. diff --git a/.agents/review-rules/frontend/workflow-document-store.md b/.agents/review-rules/frontend/workflow-document-store.md new file mode 100644 index 00000000000..fdafc6a993c --- /dev/null +++ b/.agents/review-rules/frontend/workflow-document-store.md @@ -0,0 +1,37 @@ +# Migrated workflow fields must use workflowDocumentStore + +Applies to: `packages/frontend`. + +Workflow state is actively migrating from `workflowsStore` +(`packages/frontend/editor-ui/src/app/stores/workflows.store.ts`) to +`workflowDocumentStore` +(`packages/frontend/editor-ui/src/app/stores/workflowDocument.store.ts`). + +`workflowDocumentStore` is the single source of truth for migrated fields. New +code that reads or writes one of them must go through it, not through +`workflowsStore.workflow.*` or an equivalent accessor. + +Already-migrated fields — do NOT access these via `workflowsStore`: + +`active`, `activeVersion`, `activeVersionId`, `checksum`, `createdAt`, +`homeProject`, `meta`, `pinData`, `settings`, `tags`, `updatedAt` + +Flag when NEW code does any of: + +1. Reads a migrated field from `workflowsStore.workflow.` +2. Reads one from a destructured `workflow` ref originating in `workflowsStore` +3. Writes one through `workflowsStore`, e.g. `workflowsStore.workflow.active = true` +4. Calls a `workflowsStore` setter/action that only mutates a migrated field, + when `workflowDocumentStore` already exposes an equivalent method + +Do NOT flag: + +- Non-migrated fields, e.g. `workflowsStore.workflow.nodes`, `.connections` +- Code inside `workflowDocument.store.ts` or its sub-modules — those *are* the + source of truth +- Code inside `workflows.store.ts` itself — the migration is still in progress there + +Use `useWorkflowDocumentStore(createWorkflowDocumentId(workflowId))` from +`@/app/stores/workflowDocument.store.ts`. + +This is an active migration; the field list will grow. diff --git a/.agents/review-rules/qa-dx/docker-image-pinning.md b/.agents/review-rules/qa-dx/docker-image-pinning.md new file mode 100644 index 00000000000..8dd0706deb3 --- /dev/null +++ b/.agents/review-rules/qa-dx/docker-image-pinning.md @@ -0,0 +1,32 @@ +# Base images and pinning + +Applies to: `docker/images/**/Dockerfile*`. + +## A tag and its digest change together + +Base images are pinned as `tag@sha256:...`. The tag is documentation; the digest +is what actually resolves. Flag a diff that edits one without the other — a +bumped tag with a stale digest silently keeps building the old image, and a +bumped digest under an old tag makes the Dockerfile lie about what it runs. + +```dockerfile +ARG BUILDER_IMAGE=node:26.7.0-alpine3.24@sha256:aadf41... +ARG RUNTIME_IMAGE=n8nio/base:26.7.0@sha256:336873... +``` + +Also flag a base image reference that loses its digest entirely, and a +`FROM`/`ARG` that introduces a floating tag such as `latest`, `alpine`, or a +major-only version. + +## Node version consistency + +`ARG NODE_VERSION`, the builder tag, and the runtime base tag describe the same +Node release. Flag a bump that moves one and leaves another behind. + +## The runtime base only changes here + +A separate workflow builds `n8nio/base`; a new base reaches the n8n image only +when `RUNTIME_IMAGE` changes in this file. Flag a PR that expects a base change +to arrive on its own, and flag a runtime stage that installs a compiler or +build dependency — the runtime base ships without one deliberately, and build +tooling belongs in a builder stage that is discarded. diff --git a/.agents/review-rules/qa-dx/docker-native-modules.md b/.agents/review-rules/qa-dx/docker-native-modules.md new file mode 100644 index 00000000000..0fb2151d3bf --- /dev/null +++ b/.agents/review-rules/qa-dx/docker-native-modules.md @@ -0,0 +1,46 @@ +# Native module builds in the Docker images + +Applies to: `docker/images/**/Dockerfile*`. + +n8n compiles `sqlite3`, `isolated-vm`, and `@confluentinc/kafka-javascript` +inside the image. Each line of that setup exists because a specific build broke. +Flag a change that undoes one. + +## Never `npm rebuild` a native module in a pnpm tree + +A pnpm `node_modules` reaches the same package through several symlinks, and +`npm rebuild` runs one install script per link, concurrently, in the same store +directory. They collide on `build/node_gyp_bins` and on each other's make +output. This produced an intermittent Docker build failure twice, for sqlite3 +and for isolated-vm. + +Call node-gyp once, directly: + +```dockerfile +node /usr/local/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js rebuild --release +``` + +Flag `npm rebuild ` or `npm install` in a Dockerfile stage that builds a +native module. + +## Use npm's bundled node-gyp, not `npx node-gyp` + +`npx node-gyp` resolves a version at build time, so the toolchain drifts from +the pinned image digest and builds stop being reproducible. The path above +pins node-gyp to whatever the base image ships. Flag `npx node-gyp`. + +## Do not reintroduce prebuilds + +`node-gyp-build` prefers `prebuilds/` over `build/Release`, and reads +`/etc/alpine-release` to detect musl. The hardened Alpine base has no such +file, so the loader picks the glibc prebuild and the module crashes at runtime. +The `rm -rf prebuilds` steps are load-bearing — flag their removal, and flag a +new native module added without one. + +## Keep the native compile above `COPY ./compiled` + +`isolated-vm` builds in its own stage whose only cache input is the module +source, specifically so an application change does not trigger a recompile. +Flag a `COPY` of application code moved above a native build step, or a native +build folded into the stage that copies `./compiled` — it silently turns a +cached layer into a multi-minute compile on every build. diff --git a/.agents/review-rules/qa-dx/ratchets-and-allowlists.md b/.agents/review-rules/qa-dx/ratchets-and-allowlists.md new file mode 100644 index 00000000000..803b4eaafb8 --- /dev/null +++ b/.agents/review-rules/qa-dx/ratchets-and-allowlists.md @@ -0,0 +1,35 @@ +# Ratchets and allowlists + +Applies to: baseline files, `packages/cli/eslint.config.mjs`, lint configs. + +Several checks are ratchets: they permit what already exists and fail only on +growth. That design has one blind spot — adding the new violation to the +baseline is indistinguishable, to the tool, from fixing it. Only review catches +it. + +Flag a diff that **adds** entries to any of these, and ask for the fix instead: + +| File | Ratchet | +|------|---------| +| `.code-health-baseline.json` | `@n8n/code-health` violations | +| `.boundaries-baseline.json` | `turbo boundaries` issue count | +| `packages/testing/playwright/.janitor-baseline.json` | Playwright janitor findings | +| `packages/cli/eslint.config.mjs` | the `misplaced-n8n-typeorm-import` and public-API allowlists, each captioned "NEVER add to this list" | + +Removals are the healthy direction and need no comment. + +Two specifics: + +- `.boundaries-baseline.json` holds a single integer, not a fingerprinted list. + A fixed issue and a new one at the same count cancel out and the ratchet stays + green. Treat any change to that number as worth explaining. +- Regenerating a baseline wholesale, rather than appending to it, hides growth + inside a large diff. If a PR rewrites a baseline, ask what the net change to + the violation count is. + +## Downgrading a rule is the same move + +Flag a lint rule moved from `'error'` to `'warn'` or `'off'`, and a new +`eslint-disable` for one of the guarded rules. Most packages run +`eslint . --quiet`, so a warning never fails CI — downgrading to `'warn'` is +functionally deleting the rule while appearing to keep it. diff --git a/.agents/review-rules/qa-dx/workflow-safety.md b/.agents/review-rules/qa-dx/workflow-safety.md new file mode 100644 index 00000000000..45f08ba87f7 --- /dev/null +++ b/.agents/review-rules/qa-dx/workflow-safety.md @@ -0,0 +1,46 @@ +# GitHub Actions safety + +Applies to: `.github/workflows/**`, `.github/actions/**`, `.poutine.yml`. + +Poutine and Zizmor already fail CI for the classic hazards — `pull_request_target` +(also an error-level `@n8n/code-health` rule), actions unpinned from a full commit +SHA, untrusted checkout execution, self-hosted runner exposure. Do not repeat +them. What follows is what the scanners cannot see. + +## Silencing a scanner is the change to review + +`.poutine.yml` carries two allowlists — `github_action_from_unverified_creator_used` +(by purl) and `untrusted_checkout_exec` (by path). The file says "Add new +entries only after security review". Nothing enforces that. + +Flag any addition to `.poutine.yml`'s `skip:` block, any new `zizmor: ignore` +comment, and any new entry in `.github/poutine-rules/`'s exceptions. Ask what +was reviewed and why the finding is a false positive rather than a real one. +The entry needs a comment giving the reason, matching the ones already there. + +## A gate that cannot fail is not a gate + +Required checks are assembled by the `ci-filter` action in `mode: validate`, +fed from a `needs:` list. Flag: + +- `continue-on-error: true` on a job that appears in a required-checks `needs:` + list, or on the final validate step. It makes the gate green regardless of + outcome. `continue-on-error` on a nightly or a notification step is fine. +- A job removed from a required-checks `needs:` list without the PR saying why. +- `if:` conditions on a required job that can silently skip it. A skipped + required job and a passing one are hard to tell apart in the merge queue. + +## Path filters decide whether a test runs at all + +Jobs gated on a `ci-filter` filter only run when a matching file changed, so a +filter that is too narrow means the test quietly stops covering new code. When +a PR adds a directory that an existing filter was meant to cover — a new package +under `packages/testing/`, a new script under `.github/scripts/` — check the +filter still matches it. + +## Least privilege + +Every workflow declares a top-level `permissions:` block; without one it runs +with the repository's broad default token. Flag a new workflow that omits it, +and a job that widens `permissions` beyond what its steps use. A job calling a +reusable workflow must grant at least what that workflow declares. diff --git a/.agents/review-rules/security/auth-and-access-control.md b/.agents/review-rules/security/auth-and-access-control.md new file mode 100644 index 00000000000..eb677f783b6 --- /dev/null +++ b/.agents/review-rules/security/auth-and-access-control.md @@ -0,0 +1,36 @@ +# Authentication, authorization, and licensing + +Applies to: `packages/cli`, `packages/@n8n/db`. + +## Authentication and sessions + +- JWT handling that weakens validation or expiration +- Missing or disabled cookie security flags (HttpOnly, Secure, SameSite) +- Changes that bypass MFA enforcement +- SSO/SAML/OIDC flows with validation gaps +- OAuth 2.0 missing PKCE, or with weak redirect URI validation + +## Authorization + +No static check covers this: the `endpoint-scope-coverage` rule in +`@n8n/code-health` is disabled, so a new route without a scope decorator reaches +production unnoticed unless a reviewer catches it. + +- Every authenticated route needs `@GlobalScope` or `@ProjectScope`, unless it + declares `skipAuth`, `allowUnauthenticated`, or `apiKeyAuth` +- Missing credential permission validation before workflow execution +- Subworkflow execution bypassing caller policies or ownership validation +- Missing project-level or resource-level access controls + +## License enforcement + +- Missing `FeatureNotLicensedError` for unlicensed feature access +- Bypassed license quota checks +- New licensed features without middleware or module-level enforcement +- New endpoints in `*.ee.ts` that reach licensed features without `@Licensed` +- `@Licensed` not matching the feature (check `LICENSE_FEATURES` in `@n8n/constants`) +- Endpoints in `*.ee.ts` carrying a scope decorator but missing `@Licensed` — the + license check is separate from the permission check +- Prefer the `@Licensed` decorator over custom licensing middleware +- Decorator order: route decorator → `@Licensed` → scope decorators +- Enterprise code reachable outside `*.ee.ts` files or licensed modules diff --git a/.agents/review-rules/security/code-execution-and-sandboxing.md b/.agents/review-rules/security/code-execution-and-sandboxing.md new file mode 100644 index 00000000000..c84ffd34744 --- /dev/null +++ b/.agents/review-rules/security/code-execution-and-sandboxing.md @@ -0,0 +1,15 @@ +# Code execution and sandboxing + +Applies to: `packages/cli`, `packages/core`, `packages/workflow`. + +Flag changes that: + +- Weaken expression evaluation sandbox protections +- Reduce isolation in Code node sandboxes (JavaScript/Python) +- Grant sandboxed contexts new access to Node.js builtins or external modules +- Introduce prototype pollution or constructor access bypasses +- Weaken prototype sanitizers or function context validators +- Expose `process.env` access in Code nodes + +Higher scrutiny applies to the expression engine and the code execution nodes — +a regression there is reachable by any workflow author. diff --git a/.agents/review-rules/security/credentials-and-secrets.md b/.agents/review-rules/security/credentials-and-secrets.md new file mode 100644 index 00000000000..2af151273d8 --- /dev/null +++ b/.agents/review-rules/security/credentials-and-secrets.md @@ -0,0 +1,18 @@ +# Credentials and secrets + +Applies to: all backend packages. + +Flag: + +- Credentials logged or exposed in error messages +- Hardcoded secrets, API keys, or tokens +- Changes to credential encryption/decryption that weaken security +- OAuth state/CSRF token handling that bypasses validation +- Webhook requests that don't sanitize auth cookies +- External secrets provider integrations with insecure configurations +- Credential access not respecting scope boundaries (instance/project/user) +- Data fetched via credentials being exposed to unauthorized user groups + +An error message that interpolates a caught error from an integration is a +common way credential material reaches a persisted status field or an API +response. Sanitize before recording. diff --git a/.agents/review-rules/security/data-and-infrastructure.md b/.agents/review-rules/security/data-and-infrastructure.md new file mode 100644 index 00000000000..5d6c6f4a34c --- /dev/null +++ b/.agents/review-rules/security/data-and-infrastructure.md @@ -0,0 +1,32 @@ +# Filesystem, database, network, and audit trails + +Applies to: all backend packages. + +## Filesystem + +- Weakened file access restriction enforcement +- File operations bypassing allowlist/blocklist patterns +- Access to n8n internal directories +- Unsanitized user input flowing into a file path + +## Database + +- Missing indexes on frequently queried columns in migrations +- Resource-intensive queries without pagination or limits +- Missing encryption for sensitive fields beyond credentials +- Raw SQL that bypasses TypeORM protections + +## HTTP, webhooks, and network + +- SSRF risk in user-controlled URLs +- CORS allowing all origins +- CSP or iframe sandbox changes that weaken protections +- Rate limiting changes that reduce protection +- Disabled TLS certificate validation (`rejectUnauthorized: false`) +- Bearer tokens forwarded across a redirect to a different host + +## Audit logging + +- Missing logging for security-relevant events +- Sensitive data in logs or error messages +- Incomplete audit trails for authentication and authorization events diff --git a/.agents/review-rules/security/node-input-safety.md b/.agents/review-rules/security/node-input-safety.md new file mode 100644 index 00000000000..cfef69795b4 --- /dev/null +++ b/.agents/review-rules/security/node-input-safety.md @@ -0,0 +1,77 @@ +# Node input safety + +Applies to: `packages/nodes-base`, `packages/@n8n/nodes-langchain`. Skip this +file for other packages. + +Node code receives workflow-author-controlled input, mainly via +`this.getNodeParameter(...)` and incoming `item.json` keys. + +## Prototype pollution via node parameters + +A value derived from user-controlled input must never be used as a computed +object key in an **assignment** to a plain object. A workflow author can set it +to `__proto__`, `constructor`, or `prototype`, polluting the prototype chain. + +ESLint does not cover this — `no-prototype-builtins` is only a warning in these +packages, and `pnpm lint` runs with `--quiet`. + +Flag NEW code where a value tracing back to `this.getNodeParameter(...)` +(directly, via a variable, via a `.map`/`.reduce`/`.forEach` callback param, or +via a `getNodeParam` function passed into a helper such as `createTableStruct`) +is used as a computed key to BUILD A NESTED OR CONTAINER STRUCTURE on a plain +object (an `IDataObject` or object literal — not a `Map`, not +`Object.create(null)`): + +1. Container creation: `obj[key] = {}` or `obj[key] = []` +2. Nested write where the untrusted value is a key: `obj[k1][k2] = value` +3. The same via `obj[key] ??= {}` / `obj[key] ||= []` + +This nested/container shape is what actually pollutes `Object.prototype` — the +dangerous pattern is typically a shared accumulator. + +Do NOT flag: + +- Reads: `const x = obj[key]`, `if (obj[key] === undefined)` — reads don't pollute +- Single-level writes of a concrete (non-object) value, e.g. + `item.json[field] = value`, `item.binary[prop] = data`, `body[target] = value`. + Assigning a primitive to `__proto__` is a no-op, and setting one property on a + fresh per-item object is not prototype pollution. This is the overwhelmingly + common, benign case in nodes — flagging it is noise. +- Keys that are string/number literals, or validated by `isSafeObjectProperty(key)` +- Writes routed through `setSafeObjectProperty(...)` +- Targets that are a `Map` or created with `Object.create(null)` + +Violation: + +```typescript +const table = this.getNodeParameter('table', i) as string; +const key = this.getNodeParameter('deleteKey', i) as string; +if (acc[table] === undefined) acc[table] = {}; // acc['__proto__'] = {} +if (acc[table][key] === undefined) acc[table][key] = []; +``` + +Allowed: + +```typescript +import { setSafeObjectProperty, isSafeObjectProperty } from 'n8n-workflow'; + +if (isSafeObjectProperty(table) && acc[table] === undefined) { + setSafeObjectProperty(acc, table, {}); +} +// or: const acc = new Map(); +``` + +Prefer `setSafeObjectProperty` / `isSafeObjectProperty` from `n8n-workflow`. See +`nodes/Google/GSuiteAdmin/GSuiteAdmin.node.ts` and +`nodes/HttpRequest/V3/HttpRequestV3.node.ts` for reference usage. + +## Injection through node parameters + +Flag NEW code where a node parameter reaches a sink without escaping or +parameterisation: + +- SQL query fields built by string concatenation or expression interpolation + instead of bound parameters +- Command execution with unsanitized shell arguments +- File operations where a parameter reaches a path without traversal checks +- Community package names accepted without validation diff --git a/.github/WORKFLOWS.md b/.github/WORKFLOWS.md index 643f1699cfc..13c59c18b3b 100644 --- a/.github/WORKFLOWS.md +++ b/.github/WORKFLOWS.md @@ -432,6 +432,7 @@ Push to master/1.x | Monday 00:00 | `util-update-node-popularity.yml` | Node usage stats | | Monday 02:00 | `test-e2e-coverage-weekly.yml` | Weekly E2E coverage | | Saturday 22:00 | `test-evals-ai.yml` | AI workflow evals | +| 1st of month 04:00 | `util-refresh-cubic-schema.yml` | Refresh vendored cubic schema | --- @@ -553,6 +554,7 @@ Scripts in `.github/scripts/`: | `validate-docs-links.js`| Check doc URLs | `util-check-docs-urls.yml`| | `send-build-stats.mjs` | Build telemetry | `setup-nodejs` action | | `db-test-matrix.mjs` | DB test matrix from `postgres-versions.json` | `ci-pull-requests.yml` | +| `quality/check-cubic-config.mjs` | Validate `cubic.yaml` against the vendored cubic schema; enforce its silent agent/character limits. `--refresh` re-pulls the schema | `test-workflow-scripts-reusable.yml`, `util-refresh-cubic-schema.yml` | | `probe-registry.mjs` | Registry path throughput probe (temporary) | `util-probe-registry.yml` | ### Branch Replay Scripts diff --git a/.github/scripts/package.json b/.github/scripts/package.json index 88fb839a4c5..51c62541e91 100644 --- a/.github/scripts/package.json +++ b/.github/scripts/package.json @@ -17,16 +17,18 @@ "@actions/github": "9.0.0", "@cyclonedx/cdxgen": "12.4.0", "@octokit/core": "7.0.6", + "ajv": "8.20.0", "conventional-changelog": "7.2.0", "debug": "4.4.3", - "json-with-bigint": "3.5.11", "glob": "13.0.6", + "json-with-bigint": "3.5.11", "minimatch": "10.2.4", "semver": "7.7.4", "tempfile": "6.0.1", "yaml": "^2.8.3" }, "devDependencies": { + "@types/node": "^26.3.0", "conventional-changelog-angular": "8.3.0" } } diff --git a/.github/scripts/pnpm-lock.yaml b/.github/scripts/pnpm-lock.yaml index 96cf69d4e53..66a19161ae5 100644 --- a/.github/scripts/pnpm-lock.yaml +++ b/.github/scripts/pnpm-lock.yaml @@ -17,6 +17,9 @@ importers: '@octokit/core': specifier: 7.0.6 version: 7.0.6 + ajv: + specifier: 8.20.0 + version: 8.20.0 conventional-changelog: specifier: 7.2.0 version: 7.2.0(conventional-commits-filter@5.0.0) @@ -42,6 +45,9 @@ importers: specifier: ^2.8.3 version: 2.9.0 devDependencies: + '@types/node': + specifier: ^26.3.0 + version: 26.3.0 conventional-changelog-angular: specifier: 8.3.0 version: 8.3.0 @@ -323,6 +329,9 @@ packages: '@types/http-cache-semantics@4.2.0': resolution: {integrity: sha512-L3LgimLHXtGkWikKnsPg0/VFx9OGZaC+eN1u4r+OB1XRqH3meBIAVC2zr1WdMH+RHmnRkqliQAOHNJ/E0j/e0Q==} + '@types/node@26.3.0': + resolution: {integrity: sha512-L3fgrnchriRC2ExBflb8j4uZZURHZfQsmQeyVzhjcHW4kkwVyo8/0h1B2MVzMTrYUJYu6G7EWs14hW/L9putqw==} + '@types/normalize-package-data@2.4.4': resolution: {integrity: sha512-37i+OaWTh9qeK4LSHPsyRC7NahnGotNuZvjLSgcPzblpHB3rrCJxAOgI5gCdKm7coonsaX1Of0ILiTcnZjbfxA==} @@ -1072,6 +1081,9 @@ packages: engines: {node: '>=0.8.0'} hasBin: true + undici-types@8.3.0: + resolution: {integrity: sha512-j375ScV60dom+YkPFIfTLcOiPxkN/buHz5GobjLhixFuANaNs3C9l4GmrWqejgXWJ7BbJcFYpTEUkS1Ge8bpZQ==} + undici@6.28.0: resolution: {integrity: sha512-LIY910g9TI13YS95lrMFrs8Rm/u/irgHeTWoKCoteeJ04CUJ92eEfj0rVn+7VKMPBpUPiUoBKfhNyLI23EE/KA==} engines: {node: '>=18.17'} @@ -1523,6 +1535,10 @@ snapshots: '@types/http-cache-semantics@4.2.0': {} + '@types/node@26.3.0': + dependencies: + undici-types: 8.3.0 + '@types/normalize-package-data@2.4.4': {} ajv-formats@3.0.1(ajv@8.20.0): @@ -2324,6 +2340,8 @@ snapshots: uglify-js@3.19.3: optional: true + undici-types@8.3.0: {} + undici@6.28.0: {} undici@7.29.0: {} diff --git a/.github/scripts/quality/check-cubic-config.mjs b/.github/scripts/quality/check-cubic-config.mjs new file mode 100644 index 00000000000..21a81596dbb --- /dev/null +++ b/.github/scripts/quality/check-cubic-config.mjs @@ -0,0 +1,232 @@ +/** + * Validates `cubic.yaml` against cubic's published schema and the limits it + * enforces silently. + * + * cubic drops custom rules past the agent cap and truncates any rule past the + * character ceiling without reporting either, and it never resolves a repo path + * mentioned in prose — only `file_paths` entries. A rule that trips any of these + * simply stops running, which is invisible until someone counts review comments. + * + * The schema is vendored rather than fetched so the check has no network + * dependency. `--refresh` pulls the current copy from cubic.dev and exits without + * validating — a refreshed schema that rejects the config should surface as a red + * check on the refresh PR, not as a failure that stops the PR being opened. + * + * Exit codes: + * 0 – config is valid + * 1 – config has at least one violation + */ + +import { readdirSync, readFileSync, writeFileSync } from 'node:fs'; +import { dirname, join, relative, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import Ajv from 'ajv/dist/2020.js'; +import { parse } from 'yaml'; + +/** https://docs.cubic.dev/ai-review/custom-agents — only the first N agents take effect. */ +export const MAX_CUBIC_AGENTS = 5; + +/** Description plus linked file contents; characters past this are dropped from the prompt. */ +export const MAX_RULE_CHARS = 10_000; + +/** Fraction of the ceiling at which a rule is reported as close to silent truncation. */ +export const WARN_RATIO = 0.8; + +/** Every markdown file here must be linked by some agent, or it silently does nothing. */ +export const RULES_DIR = '.agents/review-rules'; + +/** Vendored copy of the schema the `# yaml-language-server:` directive points at. */ +export const SCHEMA_PATH = '.github/scripts/quality/cubic-config.schema.json'; + +export const SCHEMA_URL = 'https://www.cubic.dev/schema/cubic-repository-config.schema.json'; + +const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '../../..'); + +/** + * Characters, not bytes — cubic's ceiling is a character count, and a byte count + * overstates it for any non-ASCII content (an em dash is 3 bytes, one character). + * `.length` counts UTF-16 code units, matching how the description is measured. + * + * @param {string} path - repo-relative + * @returns {number} character count, or -1 when the path does not resolve + */ +export function fileCharacters(path) { + try { + return readFileSync(join(REPO_ROOT, path), 'utf8').length; + } catch { + return -1; + } +} + +/** + * Validate against cubic's own schema. Catches what the hand-written checks below + * cannot: a mistyped key inside `reviews` / `pr_descriptions` / `issues` (all + * `additionalProperties: false`), a bad enum value, a wrong type. + * + * @param {unknown} config + * @param {object} schema + * @returns {string[]} + */ +export function schemaErrors(config, schema) { + const ajv = new Ajv({ allErrors: true, strict: false }); + const validate = ajv.compile(schema); + if (validate(config)) return []; + + return (validate.errors ?? []).map((error) => { + const path = error.instancePath || '/'; + const { allowedValues, additionalProperty } = error.params ?? {}; + + if (additionalProperty) { + return `${path} has an unknown key \`${additionalProperty}\`.`; + } + + const allowed = allowedValues ? ` (allowed: ${allowedValues.join(', ')})` : ''; + return `${path} ${error.message}${allowed}`; + }); +} + +/** + * Markdown rule files on disk, repo-relative, excluding the README. + * + * @returns {string[]} + */ +function ruleFiles() { + try { + return readdirSync(join(REPO_ROOT, RULES_DIR), { recursive: true, withFileTypes: true }) + .filter((entry) => entry.isFile() && entry.name.endsWith('.md') && entry.name !== 'README.md') + .map((entry) => relative(REPO_ROOT, join(entry.parentPath, entry.name))) + .sort(); + } catch { + return []; + } +} + +/** + * @param {any} config - parsed cubic.yaml + * @param {(path: string) => number} charsIn - characters in a linked file, -1 if missing + * @param {string[]} [onDisk] - rule files that must each be linked by some agent + * @returns {{ violations: string[], warnings: string[], ruleLengths: Record }} + */ +export function checkConfig(config, charsIn, onDisk = []) { + const violations = []; + /** @type { string[] } */ + const warnings = []; + /** @type { Record } */ + const ruleLengths = {} + const linked = new Set(); + + if (config?.version !== 1) { + violations.push(`\`version\` must be 1, found ${JSON.stringify(config?.version)}.`); + } + + const rules = config?.reviews?.custom_rules ?? []; + if (!Array.isArray(rules)) { + violations.push('`reviews.custom_rules` must be a list.'); + return { violations, warnings, ruleLengths }; + } + + if (rules.length > MAX_CUBIC_AGENTS) { + const dropped = rules.slice(MAX_CUBIC_AGENTS).map((rule) => rule?.name ?? '(unnamed)'); + violations.push( + `${rules.length} custom rules defined but only the first ${MAX_CUBIC_AGENTS} take effect. ` + + `These never run: ${dropped.join(', ')}. Merge related rules instead of appending.`, + ); + } + + rules.forEach((rule, index) => { + const label = rule?.name ? `"${rule.name}"` : `rule #${index + 1}`; + + if (!rule?.name) { + violations.push(`${label} has no \`name\`.`); + } + + const description = rule?.description ?? ''; + const filePaths = rule?.file_paths ?? []; + + if (!description && filePaths.length === 0) { + violations.push(`${label} needs a \`description\`, \`file_paths\`, or both.`); + } + + let total = description.length; + for (const path of filePaths) { + linked.add(path); + const chars = charsIn(path); + if (chars < 0) { + violations.push(`${label} links \`${path}\`, which does not exist.`); + continue; + } + total += chars; + } + + if (total > MAX_RULE_CHARS) { + violations.push( + `${label} is ${total.toLocaleString()} characters; everything past ` + + `${MAX_RULE_CHARS.toLocaleString()} is dropped from the review prompt.`, + ); + } else if (total > MAX_RULE_CHARS * WARN_RATIO) { + warnings.push( + `${label} is at ${Math.round((total / MAX_RULE_CHARS) * 100)}% of the ` + + `${MAX_RULE_CHARS.toLocaleString()}-character ceiling. Trim it before adding more.`, + ); + } + + ruleLengths[label] = total + }); + + for (const path of onDisk) { + if (!linked.has(path)) { + violations.push(`\`${path}\` is not linked by any agent, so it is never applied.`); + } + } + + return { violations, warnings, ruleLengths }; +} + +async function refreshSchema() { + const response = await fetch(SCHEMA_URL); + if (!response.ok) { + console.error(`Could not fetch ${SCHEMA_URL}: HTTP ${response.status}`); + process.exit(1); + } + const schema = await response.json(); + writeFileSync(join(REPO_ROOT, SCHEMA_PATH), `${JSON.stringify(schema, null, '\t')}\n`); + console.log(`Refreshed ${SCHEMA_PATH} from ${SCHEMA_URL}.`); +} + +async function main() { + if (process.argv.includes('--refresh')) { + await refreshSchema(); + return; + } + + const config = parse(readFileSync(join(REPO_ROOT, 'cubic.yaml'), 'utf8')); + const schema = JSON.parse(readFileSync(join(REPO_ROOT, SCHEMA_PATH), 'utf8')); + + const { violations, warnings, ruleLengths } = checkConfig(config, fileCharacters, ruleFiles()); + violations.unshift(...schemaErrors(config, schema)); + + console.log("Rule sizes:") + for (const [label, ruleLength] of Object.entries(ruleLengths)) { + console.log(` ${label}: ${ruleLength} characters (${Math.floor(ruleLength / MAX_RULE_CHARS * 100)}%)`); + } + + for (const warning of warnings) { + console.log(`::warning file=cubic.yaml::${warning}`); + } + + if (violations.length === 0) { + console.log( + `cubic.yaml is valid (${config.reviews?.custom_rules?.length ?? 0}/${MAX_CUBIC_AGENTS} agents).`, + ); + return; + } + + for (const violation of violations) { + console.log(`::error file=cubic.yaml::${violation}`); + } + process.exit(1); +} + +if (import.meta.url === `file://${process.argv[1]}`) { + await main(); +} diff --git a/.github/scripts/quality/check-cubic-config.test.mjs b/.github/scripts/quality/check-cubic-config.test.mjs new file mode 100644 index 00000000000..1ff18ab8ba8 --- /dev/null +++ b/.github/scripts/quality/check-cubic-config.test.mjs @@ -0,0 +1,237 @@ +import assert from 'node:assert/strict'; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, relative } from 'node:path'; +import { after, before, describe, it } from 'node:test'; + +/** + * Run with: + * node --test .github/scripts/quality/check-cubic-config.test.mjs + */ + +let checkConfig, fileCharacters, schemaErrors, MAX_CUBIC_AGENTS, MAX_RULE_CHARS, WARN_RATIO; +let schema; +before(async () => { + ({ checkConfig, fileCharacters, schemaErrors, MAX_CUBIC_AGENTS, MAX_RULE_CHARS, WARN_RATIO } = + await import('./check-cubic-config.mjs')); + schema = JSON.parse(readFileSync(new URL('./cubic-config.schema.json', import.meta.url), 'utf8')); +}); + +/** Violations only, for the cases that assert nothing about warnings. */ +const violationsOf = (...args) => checkConfig(...args).violations; + +/** @param {Array} rules */ +const config = (rules) => ({ version: 1, reviews: { custom_rules: rules } }); + +/** @param {string} name */ +const rule = (name) => ({ name, description: 'x' }); + +const noFiles = () => -1; +const emptyFiles = () => 0; + +describe('limits', () => { + it('caps agents at 5', () => { + assert.equal(MAX_CUBIC_AGENTS, 5); + }); + + it('caps rule size at 10000', () => { + assert.equal(MAX_RULE_CHARS, 10_000); + }); + + it('warns at 80% of the ceiling', () => { + assert.equal(WARN_RATIO, 0.8); + }); +}); + +describe('checkConfig', () => { + it('accepts a config at the agent cap', () => { + const rules = Array.from({ length: MAX_CUBIC_AGENTS }, (_, i) => rule(`r${i}`)); + assert.deepEqual(violationsOf(config(rules), emptyFiles), []); + }); + + it('names every rule that will never run when the cap is exceeded', () => { + const rules = Array.from({ length: MAX_CUBIC_AGENTS + 2 }, (_, i) => rule(`r${i}`)); + const [violation, ...rest] = violationsOf(config(rules), emptyFiles); + + assert.deepEqual(rest, []); + assert.match(violation, /only the first 5 take effect/); + assert.match(violation, /r5, r6/); + }); + + it('flags a linked file that does not exist', () => { + const rules = [{ name: 'Frontend', description: '', file_paths: ['.agents/gone.md'] }]; + const violations = violationsOf(config(rules), noFiles); + + assert.equal(violations.length, 1); + assert.match(violations[0], /"Frontend" links `\.agents\/gone\.md`, which does not exist/); + }); + + it('counts linked file length toward the character ceiling', () => { + const rules = [{ name: 'Big', description: 'a'.repeat(9_000), file_paths: ['doc.md'] }]; + + assert.deepEqual( + violationsOf(config(rules), () => 500), + [], + ); + + const violations = violationsOf(config(rules), () => 2_000); + assert.equal(violations.length, 1); + assert.match(violations[0], /"Big" is 11,000 characters/); + }); + + it('measures a description in characters, not UTF-8 bytes', () => { + // Each em dash is 3 bytes but one character. By bytes this is 12,000 and + // would fail; by characters it is 4,000 and fits. + const rules = [{ name: 'Dashes', description: '—'.repeat(4_000) }]; + + assert.deepEqual(violationsOf(config(rules), emptyFiles), []); + }); + + it('flags a rule with neither a description nor linked files', () => { + const violations = violationsOf(config([{ name: 'Empty' }]), emptyFiles); + + assert.equal(violations.length, 1); + assert.match(violations[0], /needs a `description`, `file_paths`, or both/); + }); + + it('flags an unnamed rule', () => { + const violations = violationsOf(config([{ description: 'x' }]), emptyFiles); + + assert.equal(violations.length, 1); + assert.match(violations[0], /rule #1 has no `name`/); + }); + + it('flags a wrong schema version', () => { + const violations = violationsOf({ version: 2, reviews: { custom_rules: [] } }, emptyFiles); + + assert.equal(violations.length, 1); + assert.match(violations[0], /`version` must be 1, found 2/); + }); + + it('accepts a config with no custom rules at all', () => { + assert.deepEqual(violationsOf({ version: 1 }, emptyFiles), []); + }); + + it('flags a rule file that no agent links', () => { + const rules = [{ name: 'Backend', description: '', file_paths: ['rules/linked.md'] }]; + const onDisk = ['rules/linked.md', 'rules/orphan.md']; + const { violations } = checkConfig(config(rules), emptyFiles, onDisk); + + assert.equal(violations.length, 1); + assert.match(violations[0], /`rules\/orphan\.md` is not linked by any agent/); + }); + + it('accepts rule files that are all linked', () => { + const rules = [ + { name: 'A', description: '', file_paths: ['rules/a.md'] }, + { name: 'B', description: '', file_paths: ['rules/b.md'] }, + ]; + const { violations } = checkConfig(config(rules), emptyFiles, ['rules/a.md', 'rules/b.md']); + + assert.deepEqual(violations, []); + }); +}); + +describe('ceiling warnings', () => { + it('warns without failing once a rule passes 80%', () => { + const rules = [{ name: 'Security', description: 'a'.repeat(8_500) }]; + const { violations, warnings } = checkConfig(config(rules), emptyFiles); + + assert.deepEqual(violations, []); + assert.equal(warnings.length, 1); + assert.match(warnings[0], /"Security" is at 85% of the 10,000-character ceiling/); + }); + + it('stays quiet below the warning threshold', () => { + const rules = [{ name: 'Security', description: 'a'.repeat(7_000) }]; + const { violations, warnings } = checkConfig(config(rules), emptyFiles); + + assert.deepEqual(violations, []); + assert.deepEqual(warnings, []); + }); + + it('reports a violation rather than a warning once over the ceiling', () => { + const rules = [{ name: 'Security', description: 'a'.repeat(10_001) }]; + const { violations, warnings } = checkConfig(config(rules), emptyFiles); + + assert.equal(violations.length, 1); + assert.deepEqual(warnings, []); + }); +}); + +describe('schemaErrors', () => { + /** @param {object} reviews */ + const cfg = (reviews) => ({ version: 1, reviews }); + + it('accepts the settings we actually use', () => { + const errors = schemaErrors( + cfg({ enabled: true, sensitivity: 'medium', incremental_commits: true, check_drafts: true }), + schema, + ); + assert.deepEqual(errors, []); + }); + + it('rejects a sensitivity outside the enum and lists the allowed values', () => { + const [error, ...rest] = schemaErrors(cfg({ sensitivity: 'strict' }), schema); + + assert.deepEqual(rest, []); + assert.match(error, /\/reviews\/sensitivity must be equal to one of the allowed values/); + assert.match(error, /allowed: low, medium, high/); + }); + + it('names a mistyped key rather than saying "additional properties"', () => { + const [error, ...rest] = schemaErrors(cfg({ check_draft: true }), schema); + + assert.deepEqual(rest, []); + assert.equal(error, '/reviews has an unknown key `check_draft`.'); + }); + + it('rejects a wrong type', () => { + const [error] = schemaErrors(cfg({ incremental_commits: 'sometimes' }), schema); + assert.match(error, /\/reviews\/incremental_commits must be boolean/); + }); + + it('rejects a per-rule field cubic does not support', () => { + const config = cfg({ custom_rules: [{ name: 'A', description: 'x', severity: 'high' }] }); + const [error] = schemaErrors(config, schema); + + assert.equal(error, '/reviews/custom_rules/0 has an unknown key `severity`.'); + }); + + it('requires a version', () => { + const errors = schemaErrors({ reviews: {} }, schema); + assert.ok(errors.some((e) => e.includes('version'))); + }); +}); + +describe('fileCharacters', () => { + const REPO_ROOT = new URL('../../..', import.meta.url).pathname; + let dir; + + before(() => { + dir = mkdtempSync(join(tmpdir(), 'cubic-chars-')); + }); + after(() => rmSync(dir, { recursive: true, force: true })); + + /** @param {string} name @param {string} content */ + const write = (name, content) => { + const abs = join(dir, name); + writeFileSync(abs, content, 'utf8'); + return relative(REPO_ROOT, abs); + }; + + it('counts characters, not UTF-8 bytes', () => { + // 100 em dashes: 300 bytes, 100 characters. statSync().size would say 300. + const path = write('dashes.md', '—'.repeat(100)); + assert.equal(fileCharacters(path), 100); + }); + + it('counts ASCII unchanged, where bytes and characters agree', () => { + const path = write('ascii.md', 'a'.repeat(100)); + assert.equal(fileCharacters(path), 100); + }); + + it('returns -1 for a path that does not resolve', () => { + assert.equal(fileCharacters('.agents/review-rules/does-not-exist.md'), -1); + }); +}); diff --git a/.github/scripts/quality/cubic-config.schema.json b/.github/scripts/quality/cubic-config.schema.json new file mode 100644 index 00000000000..47f402e047f --- /dev/null +++ b/.github/scripts/quality/cubic-config.schema.json @@ -0,0 +1,348 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://cubic.dev/schema/cubic-repository-config.schema.json", + "title": "Cubic Repository Configuration", + "description": "JSON schema for cubic.yaml (and the legacy .cubic.yaml) so editors can validate and autocomplete repository settings.", + "type": "object", + "additionalProperties": true, + "required": [ + "version" + ], + "properties": { + "version": { + "const": 1, + "description": "Schema version. Must always be 1." + }, + "reviews": { + "type": "object", + "description": "AI review behavior and optional YAML-defined custom agents.", + "additionalProperties": false, + "properties": { + "enabled": { + "type": "boolean", + "description": "Master toggle for AI reviews on this repository." + }, + "sensitivity": { + "type": "string", + "enum": [ + "low", + "medium", + "high" + ], + "description": "Controls how strictly Cubic flags issues." + }, + "incremental_commits": { + "type": "boolean", + "default": true, + "description": "If true, reviews new commits pushed to open PRs (only new issues are posted). If false, reviews only when the PR is first opened." + }, + "check_drafts": { + "type": "boolean", + "default": false, + "description": "If true, reviews draft PRs immediately when opened. If false, skips them." + }, + "architecture_diagrams": { + "type": "boolean", + "default": false, + "description": "If true, includes AI-generated architecture diagrams in review summaries." + }, + "external_contributors_require_manual_review": { + "type": "boolean", + "default": false, + "description": "If true, cubic skips automatic reviews for public-repository PRs from external contributors until a trusted installation member manually triggers a review." + }, + "show_ai_feedback_buttons": { + "type": "boolean", + "default": false, + "deprecated": true, + "description": "Deprecated and ignored. Retained so existing cubic.yaml files continue to validate." + }, + "resolve_threads_when_addressed": { + "type": "boolean", + "default": true, + "description": "Automatically resolve GitHub review threads when cubic detects the issue has been addressed in a subsequent commit." + }, + "merge_confidence_summary": { + "type": "boolean", + "default": false, + "description": "If true, includes an AI-generated merge confidence summary on full reviews." + }, + "auto_approve_behavior": { + "type": "string", + "enum": [ + "disabled", + "shadow", + "live" + ], + "description": "Controls whether auto-approval is disabled, simulated in shadow mode, or submitted live to GitHub." + }, + "auto_approve": { + "type": "string", + "enum": [ + "disabled", + "always", + "low_risk_only", + "custom" + ], + "description": "Auto-approve mode: disabled, always approve clean runs, use low-risk heuristics, or use a custom prompt." + }, + "auto_approve_custom_prompt": { + "type": "string", + "description": "Custom criteria for auto-approval. Only used when auto_approve is custom." + }, + "auto_approve_rules": { + "$ref": "#/$defs/autoApproveRules" + }, + "ultrareview": { + "type": "string", + "enum": [ + "disabled", + "manual", + "automatic" + ], + "description": "Master switch for ultrareviews. disabled blocks all ultrareviews (manual and automatic), manual allows only manual triggers (the default), automatic also enables the automatic triggers." + }, + "auto_ultrareview": { + "type": "string", + "enum": [ + "disabled", + "high_risk_only", + "custom" + ], + "description": "Auto-trigger ultrareview mode: disabled, let cubic decide per PR using built-in high-risk criteria, or use a custom prompt." + }, + "auto_ultrareview_custom_prompt": { + "type": "string", + "description": "Custom criteria describing which PRs warrant an ultrareview. Only used when auto_ultrareview is custom." + }, + "auto_ultrareview_file_patterns": { + "$ref": "#/$defs/globList", + "description": "File or directory globs that always trigger an ultrareview. If any changed file in a PR matches, cubic runs an ultrareview regardless of the auto_ultrareview mode." + }, + "custom_instructions": { + "type": "string", + "description": "Free-form reviewer guidance. Leading/trailing whitespace is trimmed." + }, + "ignore": { + "$ref": "#/$defs/ignore" + }, + "custom_rules": { + "type": "array", + "description": "List of YAML-managed custom agents enforced before UI-defined agents.", + "items": { + "$ref": "#/$defs/customRule" + } + } + } + }, + "pr_descriptions": { + "type": "object", + "description": "Configuration for AI-authored pull request descriptions.", + "additionalProperties": false, + "properties": { + "generate": { + "type": "boolean", + "description": "Enable AI-authored PR descriptions." + }, + "instructions": { + "type": "string", + "description": "Extra guidance inserted into generated summaries." + }, + "cubic_review_link": { + "type": "boolean", + "description": "Include a link to review the PR in cubic (eg www.cubic.dev/pr/owner/repo/123)." + }, + "skip_if_author_description": { + "type": "boolean", + "description": "Skip AI description generation on new PRs when the author already wrote a substantive description. Empty or template-only descriptions still get an AI description." + } + } + }, + "issues": { + "type": "object", + "description": "Issue and PR comment fix settings.", + "additionalProperties": false, + "properties": { + "fix_with_cubic_buttons": { + "type": "boolean", + "description": "Toggle Fix with cubic buttons inside GitHub issues." + }, + "pr_comment_fixes": { + "type": "boolean", + "description": "Allow cubic to make code changes when requested from PR comment threads." + }, + "fix_commits_to_pr": { + "type": "boolean", + "description": "When true, fix commits are pushed directly to the PR branch instead of opening a new PR." + }, + "auto_fix_sign_commits": { + "type": "boolean", + "default": false, + "description": "When true, cubic signs the commits it pushes so GitHub marks them as verified. This applies only to the 'cubic' coding agent; the Cursor agent always signs its commits." + }, + "coding_agent_provider": { + "type": "string", + "enum": [ + "cubic", + "cursor_cloud_agent" + ], + "description": "Which coding agent applies automatic fixes. 'cubic' uses cubic's built-in agent; 'cursor_cloud_agent' dispatches a Cursor cloud agent (requires Cursor configuration)." + } + } + } + }, + "$defs": { + "globList": { + "type": "array", + "description": "List of glob patterns. Empty strings are ignored at runtime.", + "items": { + "type": "string", + "minLength": 1 + } + }, + "ignore": { + "type": "object", + "description": "Conditional ignore filters that skip AI reviews.", + "additionalProperties": false, + "properties": { + "files": { + "$ref": "#/$defs/globList", + "description": "File globs to skip entirely." + }, + "head_branches": { + "$ref": "#/$defs/globList", + "description": "Source branches that should not trigger reviews." + }, + "base_branches": { + "$ref": "#/$defs/globList", + "description": "Target branches that should not trigger reviews." + }, + "pr_labels": { + "$ref": "#/$defs/globList", + "description": "Labels that disable reviews when present." + }, + "pr_titles": { + "$ref": "#/$defs/globList", + "description": "Wildcard matches applied to pull request titles." + }, + "max_changed_lines": { + "type": "integer", + "minimum": 1, + "maximum": 2147483647, + "description": "Store the automatic-review threshold for reviewable added plus deleted lines. Positive integers up to 2,147,483,647 are preserved, but cubic applies an effective 50,000-line ceiling to every review. Manual triggers can bypass a lower configured threshold, but not the effective ceiling." + } + } + }, + "autoApproveRules": { + "type": "object", + "description": "Rules that control whether auto-approval evaluation is allowed.", + "additionalProperties": false, + "properties": { + "exclude": { + "$ref": "#/$defs/globList", + "description": "Changed files matching these globs are never eligible for auto-approval." + }, + "only_files": { + "$ref": "#/$defs/globList", + "description": "Every changed path must match at least one of these globs for the pull request to be eligible for auto-approval. Renamed files must match on both their current and previous paths." + }, + "exclude_external_contributors": { + "type": "boolean", + "default": false, + "description": "If true, public-repository PRs from external contributors are reviewed but never auto-approved." + }, + "exclude_authors": { + "$ref": "#/$defs/globList", + "description": "PRs from a matching author are never auto-approved. Logins match case-insensitively and brackets are literal, so `renovate[bot]` and `*[bot]` both work." + }, + "only_authors": { + "$ref": "#/$defs/globList", + "description": "Only PRs from a matching author can be auto-approved." + }, + "exclude_head_branches": { + "$ref": "#/$defs/globList", + "description": "PRs whose source branch matches are never auto-approved." + }, + "only_head_branches": { + "$ref": "#/$defs/globList", + "description": "Only PRs whose source branch matches can be auto-approved." + }, + "exclude_base_branches": { + "$ref": "#/$defs/globList", + "description": "PRs targeting a matching base branch are never auto-approved." + }, + "only_base_branches": { + "$ref": "#/$defs/globList", + "description": "Only PRs targeting a matching base branch can be auto-approved." + }, + "exclude_labels": { + "$ref": "#/$defs/globList", + "description": "PRs carrying a matching label are never auto-approved. Labels match exactly, case-insensitively." + }, + "only_labels": { + "$ref": "#/$defs/globList", + "description": "Only PRs carrying a matching label can be auto-approved. Labels match exactly, case-insensitively." + }, + "exclude_titles": { + "$ref": "#/$defs/globList", + "description": "PRs whose title matches are never auto-approved." + }, + "only_titles": { + "$ref": "#/$defs/globList", + "description": "Only PRs whose title matches can be auto-approved." + } + } + }, + "customRule": { + "type": "object", + "description": "Definition of a YAML-managed custom agent.", + "additionalProperties": false, + "required": [ + "name" + ], + "anyOf": [ + { + "required": [ + "description" + ] + }, + { + "required": [ + "file_paths" + ] + } + ], + "properties": { + "name": { + "type": "string", + "minLength": 1, + "description": "Rule title shown in the dashboard." + }, + "description": { + "type": "string", + "minLength": 1, + "description": "Explain what the rule enforces." + }, + "file_paths": { + "type": "array", + "minItems": 1, + "maxItems": 10, + "description": "Ordered repo-relative instruction files. The first 10,000 characters of the description plus concatenated file text are used.", + "items": { + "type": "string", + "minLength": 1, + "maxLength": 500 + } + }, + "include": { + "$ref": "#/$defs/globList", + "description": "Optional include globs. Defaults to all files." + }, + "exclude": { + "$ref": "#/$defs/globList", + "description": "Optional exclude globs that override includes." + } + } + } + } +} diff --git a/.github/workflows/ci-pull-requests.yml b/.github/workflows/ci-pull-requests.yml index 9735a6da94f..fa22ee586c2 100644 --- a/.github/workflows/ci-pull-requests.yml +++ b/.github/workflows/ci-pull-requests.yml @@ -117,6 +117,8 @@ jobs: .github/scripts/** scripts/licenses/** scripts/mutation-health/** + cubic.yaml + .agents/review-rules/** performance: packages/testing/performance/** packages/workflow/src/** diff --git a/.github/workflows/test-workflow-scripts-reusable.yml b/.github/workflows/test-workflow-scripts-reusable.yml index 4e12d629f5b..5755afd04a0 100644 --- a/.github/workflows/test-workflow-scripts-reusable.yml +++ b/.github/workflows/test-workflow-scripts-reusable.yml @@ -36,3 +36,7 @@ jobs: - name: Run tests id: run-tests run: npm test --prefix=.github/scripts + + - name: Check cubic.yaml + if: ${{ !cancelled() }} + run: node .github/scripts/quality/check-cubic-config.mjs diff --git a/.github/workflows/util-refresh-cubic-schema.yml b/.github/workflows/util-refresh-cubic-schema.yml new file mode 100644 index 00000000000..146e2c4ac13 --- /dev/null +++ b/.github/workflows/util-refresh-cubic-schema.yml @@ -0,0 +1,85 @@ +name: 'Util: Refresh cubic schema' + +on: + schedule: + # 1st of the month at 04:00 UTC + - cron: '0 4 1 * *' + workflow_dispatch: # Allow manual trigger for testing + +permissions: + contents: read + +jobs: + refresh-schema: + name: Refresh vendored cubic schema + if: | + github.event_name == 'workflow_dispatch' || + (github.event_name == 'schedule' && github.repository == 'n8n-io/n8n') + runs-on: ubuntu-latest + timeout-minutes: 10 + # Every write goes through the scoped app token below, so the job's own + # GITHUB_TOKEN only needs to read for checkout. + permissions: + contents: read + steps: + - name: Generate GitHub App Token + id: generate-token + uses: actions/create-github-app-token@29824e69f54612133e76f7eaac726eef6c875baf # v2.2.1 + with: + app-id: ${{ secrets.N8N_ASSISTANT_APP_ID }} + private-key: ${{ secrets.N8N_ASSISTANT_PRIVATE_KEY }} + # Scope the token to what create-pull-request does: push the branch, + # open the PR (the label already exists, so no issues access needed). + permission-contents: write + permission-pull-requests: write + + - name: Checkout repository + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + # create-pull-request unsets any persisted credential and configures + # its own from `token`, so nothing here needs to survive the checkout. + persist-credentials: false + + - name: Setup Node.js + uses: ./.github/actions/setup-nodejs + with: + build-command: '' + install-command: pnpm install --frozen-lockfile --dir ./.github/scripts --ignore-workspace + cache-dependency-path: .github/scripts/pnpm-lock.yaml + + # Refreshes only. Validation runs on the PR this opens, so a schema change + # that rejects the current cubic.yaml shows up as a red check to act on + # rather than a failure here that hides the change. + - name: Refresh vendored schema + run: node .github/scripts/quality/check-cubic-config.mjs --refresh + + - name: Create Pull Request + uses: peter-evans/create-pull-request@c0f553fe549906ede9cf27b5156039d195d2ece0 # v8.1.0 + with: + token: ${{ steps.generate-token.outputs.token }} + # Stage only the schema. setup-nodejs can leave unrelated files dirty, + # and this PR should never carry anything but the refreshed copy. + add-paths: .github/scripts/quality/cubic-config.schema.json + commit-message: 'chore: Refresh vendored cubic config schema (no-changelog)' + labels: 'automation:scheduled-update' + title: 'chore: Refresh vendored cubic config schema (no-changelog)' + body: | + cubic's published config schema has changed since the vendored copy was last updated. + + The copy at `.github/scripts/quality/cubic-config.schema.json` is what + `pnpm check:cubic-config` validates `cubic.yaml` against, so it is vendored + to keep that check off the network. + + **Review the diff for new options worth adopting** — new keys under `reviews` + are how cubic ships features, and the vendored copy is the only place they + become visible to us. + + If the `Workflow scripts` check is red, the new schema rejects something in + `cubic.yaml`; fix the config in this PR. + + _Generated by the monthly cubic schema refresh workflow._ + branch: refresh-cubic-schema + base: master + delete-branch: true + author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> + committer: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> diff --git a/AGENTS.md b/AGENTS.md index 6a669a3a3ce..5d0604a70b4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -237,7 +237,7 @@ a new import (or an inline `eslint-disable` of the rule) fails CI. - **All UI text must use i18n** - add translations to `@n8n/i18n` package - **Use CSS variables directly** - never hardcode spacing as px values - **data-testid must be a single value** (no spaces or multiple values) -- Always use `design-system-rules` skill in reviews +- Always use the `design-system` skill in reviews ### Testing Guidelines - **Always work from within the package directory** when running tests diff --git a/cubic.yaml b/cubic.yaml index b7c2a92db46..afee39d835a 100644 --- a/cubic.yaml +++ b/cubic.yaml @@ -5,27 +5,57 @@ # Place this file in your repository root to version-control your AI review settings. # Settings defined here take precedence over UI-configured settings. # See https://docs.cubic.dev/configure/cubic-yaml for documentation. +# +# Only the first 5 custom_rules take effect, and each one is truncated at 10,000 +# characters (description + linked file_paths). `pnpm check:cubic-config` enforces both. +# Scope every rule with include/exclude so it is not evaluated against the whole monorepo. version: 1 reviews: enabled: true sensitivity: medium incremental_commits: true - show_ai_feedback_buttons: false + check_drafts: true + ignore: + # Mechanical PRs: content was already reviewed on master, or there is nothing + # to review. Backports stay reviewed — their conflicts are resolved by hand. + pr_labels: + - automation:v3-sync + - automation:release + - automation:scheduled-update + # Above this, cubic skips the automatic review; `@cubic-dev-ai review this` + # still works. CI already caps PRs at 1000 lines, so crossing this is always + # a deliberate exception. Makes explicit a limit cubic otherwise applies on + # its own terms. + max_changed_lines: 10000 + files: + - CHANGELOG.md + - pnpm-lock.yaml + - '**/dist/**' + - '**/__snapshots__/**' + - '**/*.snap' + - '**/*.generated.yml' + - '**/*.generated.ts' custom_instructions: |- - ## Step 1: Fetch Current Guidelines + ## Scope - 1. Fetch the current [CONTRIBUTING.md](https://github.com/n8n-io/n8n/blob/master/CONTRIBUTING.md) from the repository's main branch - 2. Navigate to the "Community PR Guidelines" section - 3. Use ONLY this live version - ignore any cached/embedded rules + Review only the lines this PR adds or modifies. Each PR is meant to have a + limited scope — do not report problems in surrounding code that already + existed. - ## Step 2: Review Process + ## Don't repeat the linter - Evaluate the PR against the rules in the fetched Community PR Guidelines. + `pnpm lint` and `pnpm typecheck` run on every PR and already fail the build + for a large set of n8n-specific rules (`@n8n/eslint-config`). Do not spend a + comment on anything ESLint reports as an error — notably `ApplicationError` + usage, `sleep`/`sleepWithAbort` helpers, `sleep` imported from + `n8n-workflow`, `@n8n/typeorm` imports in `packages/cli` business logic, + uncaught `JSON.parse`, `JSON.parse(JSON.stringify())`, skipped tests, and + CSS custom-property naming. Report what static analysis cannot see. - ## Step 3: Test Requirement Interpretation + ## Test coverage - BE REASONABLE when evaluating test coverage: + BE REASONABLE when evaluating test coverage. **PASS if:** @@ -41,334 +71,119 @@ reviews: Approve if reasonably tested. Let humans handle edge cases. - ## Step 4: Design System Style Rules + ## Community contributions - Follow `.claude/plugins/n8n/skills/design-system-rules/SKILL.md` for all CSS/SCSS/Vue style - review guidance. - - Enforcement level: - - Strong warning: hard-coded visual values, legacy token usage, deprecated - style/component surfaces - - Soft warning: token-to-token substitutions (ask for intent to avoid - accidental regressions) + For PRs from outside the n8n organisation, the bar is set by the "Community + PR Guidelines" section of `CONTRIBUTING.md`. The golden rule there: a + contribution should be worth more to the project than the time it takes to + review it. custom_rules: - - name: Security Review + - name: Security + file_paths: + - .agents/review-rules/security/code-execution-and-sandboxing.md + - .agents/review-rules/security/credentials-and-secrets.md + - .agents/review-rules/security/auth-and-access-control.md + - .agents/review-rules/security/data-and-infrastructure.md + - .agents/review-rules/security/node-input-safety.md + include: + - packages/cli/** + - packages/@n8n/db/** + - packages/core/** + - packages/workflow/** + - packages/nodes-base/** + - packages/@n8n/nodes-langchain/** + exclude: + - '**/__tests__/**' + - '**/*.test.ts' + - '**/*.spec.ts' + - '**/test/**' description: |- - Proactively review PRs and flag security vulnerabilities specific to n8n's architecture: + Flag security defects introduced by this PR, using the linked rules. + Each rule file opens with the packages it applies to — skip a file when + the changed code is out of its scope. - **Code Execution & Sandboxing:** - - Changes that weaken expression evaluation sandbox protections - - Modifications to Code node sandboxes (JavaScript/Python) that reduce isolation - - New access to Node.js builtins or external modules in sandboxed contexts - - Prototype pollution or constructor access bypasses - - Weakening of prototype sanitizers or function context validators - - Changes that expose `process.env` access in Code nodes - - **Credential & Secret Handling:** - - Credentials being logged or exposed in error messages - - Hardcoded secrets, API keys, or tokens in code - - Changes to credential encryption/decryption that weaken security - - OAuth state/CSRF token handling that bypasses validation - - Webhook requests that don't sanitize auth cookies - - External secrets provider integrations with insecure configurations - - Credential access not respecting scope boundaries (instance/project/user) - - Data fetched via credentials being exposed to unauthorized user groups - - **Authentication & Session Security:** - - JWT token handling that weakens validation or expiration - - Missing or disabled cookie security flags (HttpOnly, Secure, SameSite) - - Changes that bypass MFA enforcement - - SSO/SAML/OIDC authentication flows with validation gaps - - OAuth 2.0 implementations missing PKCE or with weak redirect URI validation - - **Authorization & Access Control:** - - Missing or bypassed RBAC and scope-based permission checks - - Missing credential permission validation before workflow execution - - Subworkflow execution that bypasses caller policies or ownership validation - - Missing project-level or resource-level access controls - - **License Enforcement:** - - Changes that weaken license enforcement - - Missing `FeatureNotLicensedError` for unlicensed feature access - - Bypassed license quota checks - - New licensed features without proper middleware or module-level enforcement - - New controller endpoints in `*.ee.ts` files that access licensed features without `@Licensed` decorator - - `@Licensed` decorator usage that doesn't match feature requirements (check `LICENSE_FEATURES` in `@n8n/constants`) - - Endpoints in `*.ee.ts` files with `@GlobalScope` or `@ProjectScope` but missing `@Licensed` (license check is separate from permission check) - - Preferred pattern: `@Licensed` decorator over custom licensing middleware - - Decorator order should be: route decorator → `@Licensed` → scope decorators - - Enterprise features not properly isolated in modules (newer features should use the module system) - - Enterprise code accessible outside of `*.ee.ts` files or licensed modules - - name: Prototype Pollution via Node Parameters + Higher scrutiny for the expression engine, credential handling, code + execution nodes, license enforcement, and SSO integrations. Community + and custom nodes carry a higher risk profile than official ones. + - name: Backend + file_paths: + - .agents/review-rules/backend/controller-request-validation.md + - .agents/review-rules/backend/error-classes.md + - .agents/review-rules/backend/explicit-any.md + - .agents/review-rules/backend/lazy-load-heavy-modules.md + - .agents/review-rules/backend/hand-rolled-delays.md + include: + - packages/cli/** + - packages/@n8n/db/** + - packages/core/** + - packages/workflow/** + - packages/nodes-base/** + - packages/@n8n/nodes-langchain/** + exclude: + - '**/__tests__/**' + - '**/*.test.ts' + - '**/*.spec.ts' + - '**/test/**' description: |- - - Rule Statement: - In node code, a value derived from user-controlled input must never be - used as a computed object key in an **assignment** to a plain object. - User input reaches nodes mainly via `this.getNodeParameter(...)` (and - incoming `item.json` keys), and a workflow author can set such a value - to `__proto__`, `constructor`, or `prototype`, polluting the prototype - chain (denial of service, and worse depending on the sink). + Review n8n's backend and node packages against the linked rules. Think in + terms of blast radius, resource cost, and failure modes. Be pragmatic, + not pedantic. - Use `setSafeObjectProperty(target, key, value)` / - `isSafeObjectProperty(key)` from `n8n-workflow`, a `Map`, or a - null-prototype object (`Object.create(null)`) instead. - - - Detection Criteria: - Flag NEW code in `packages/nodes-base/**` or - `packages/@n8n/nodes-langchain/**` when a value that traces back to - `this.getNodeParameter(...)` (directly, via a variable, via a - `.map`/`.reduce`/`.forEach` callback param, or via a `getNodeParam` - function passed into a helper such as `createTableStruct`) is used as a - computed key to BUILD A NESTED OR CONTAINER STRUCTURE on a plain object - (an `IDataObject` / object literal — not a `Map`, not `Object.create(null)`): - 1. Container creation: `obj[key] = {}` or `obj[key] = []` - 2. Nested write where the untrusted value is a key: `obj[k1][k2] = value` - 3. The same via `obj[key] ??= {}` / `obj[key] ||= []` - This nested/container shape is what actually pollutes `Object.prototype` - (typically the dangerous pattern is building a shared accumulator, e.g. - `if (acc[table] === undefined) acc[table] = {}` then `acc[table][key] = []`). - - - Do NOT flag: - - Reads: `const x = obj[key]`, `if (obj[key] === undefined)` - (reads alone do not pollute) - - Single-level writes of a concrete (non-object) value, e.g. - `item.json[field] = value`, `item.binary[prop] = data`, - `body[target] = value`. Assigning a primitive to `__proto__` is a - no-op, and setting one property on a fresh per-item object is not - prototype pollution. This is the overwhelmingly common, benign case - in nodes — flagging it is noise. - - Keys that are string/number literals, or are validated by - `isSafeObjectProperty(key)` before the write - - Writes routed through `setSafeObjectProperty(...)` - - Targets that are a `Map` or created with `Object.create(null)` - - Existing unchanged code (review only new or modified lines) - - - Example Violation: - ```typescript - const table = this.getNodeParameter('table', i) as string; - const key = this.getNodeParameter('deleteKey', i) as string; - if (acc[table] === undefined) acc[table] = {}; // acc['__proto__'] = {} - if (acc[table][key] === undefined) acc[table][key] = []; - ``` - - - Example Allowed: - ```typescript - import { setSafeObjectProperty, isSafeObjectProperty } from 'n8n-workflow'; - - if (isSafeObjectProperty(table) && acc[table] === undefined) { - setSafeObjectProperty(acc, table, {}); - } - // or: const acc = new Map(); - ``` - - - Recommendation: - Prefer `setSafeObjectProperty` / `isSafeObjectProperty` from - `n8n-workflow`. See `nodes/Google/GSuiteAdmin/GSuiteAdmin.node.ts` and - `nodes/HttpRequest/V3/HttpRequestV3.node.ts` for reference usage. - - name: Use DTOs for Request Body Validation + The rules are drawn from real incidents and the conventions the team + enforces. Each file opens with the packages it applies to — skip one when + the changed code is out of its scope. If none match, say nothing. + - name: Frontend + file_paths: + - .agents/skills/design-system/SKILL.md + - .agents/review-rules/frontend/design-system.md + - .agents/review-rules/frontend/workflow-document-store.md + include: + - packages/frontend/** + exclude: + - '**/__tests__/**' + - '**/*.stories.ts' + - '**/*.spec.ts' description: |- - - Rule Statement: - Controller endpoints that accept request bodies must use the `@Body` decorator with a DTO class for runtime validation. TypeScript types alone provide no runtime validation - only `@Body` with a Zod-based DTO (extending `Z.class`) validates incoming data. - - - Detection Criteria: - Only flag in `*.controller.ts` files when there is positive evidence the developer intended to accept a body but didn't use the pattern: - - 1. Parameter named `payload`, `body`, or `data` without `@Body` decorator - 2. Direct `req.body` access in a controller method - 3. `@Body` decorator with a type not ending in `Dto` (indicates unawareness of the DTO pattern) - - Do NOT flag: - - `@Body` with a type ending in `Dto` (developer knows the pattern) - - POST/PUT/PATCH endpoints without body parameters (may legitimately have no body) - - Webhook controllers - - Existing unchanged code - - - Example Violation: - ```typescript - @Post('/') - async create(req: AuthenticatedRequest, payload: CreateUser) { - // payload is undefined - missing @Body decorator - } - - @Post('/') - async create(req: AuthenticatedRequest) { - const data = req.body; // No runtime validation - } - - @Post('/') - async create(@Body payload: CreateUser) { - // Type doesn't end in Dto - likely missing Zod validation - } - ``` - - - Example Allowed: - ```typescript - @Post('/') - async create(@Body payload: CreateUserDto) { ... } - - @Post('/activate') - async activate(req: AuthenticatedRequest) { - // Legitimately no body needed - } - ``` - - - Recommendation: - Use `@Body` decorator with a DTO class from `@n8n/api-types` or a local `dto/` directory. DTOs must extend `Z.class` from `zod-class` for runtime validation. - - **Input Validation & Injection Prevention:** - - Unsanitized user input flowing to file paths - - SQL nodes with expressions in query fields without parameterization - - Command execution nodes with unsanitized shell arguments - - Path traversal risks in file operation nodes - - Community package names with suspicious patterns - - **File System Access:** - - Changes that weaken file access restriction enforcement - - File operations that bypass allowlist/blocklist patterns - - Access to n8n internal directories - - **Database Security & Performance:** - - Missing indexes on frequently queried columns in migrations - - Resource-intensive queries without pagination or limits - - Missing encryption for sensitive fields beyond credentials - - Raw SQL queries that bypass TypeORM protections - - **HTTP, Webhooks & Network Security:** - - SSRF risks in user-controlled URLs - - CORS configuration that allows all origins - - CSP or iframe sandbox modifications that weaken protections - - Rate limiting configuration changes that reduce protection - - Disabled TLS certificate validation (`rejectUnauthorized: false`) - - **Audit Logging & Event Bus:** - - Missing logging for security-relevant events - - Sensitive data appearing in logs or error messages - - Incomplete audit trails for authentication and authorization events - - **Context-aware review:** - - Higher scrutiny for: expression engine, credential handling, code execution nodes, license enforcement, SSO integrations - - Consider n8n's security audit categories: credentials, database, nodes, instance, filesystem risks - - Community/custom nodes have higher risk profile than official nodes - - name: Quality & Performance Review + Enforce n8n's frontend conventions using the linked rules. The design + system skill is the source of truth for which token to reach for; the + rule files that follow set the enforcement level. + - name: QA & DX + file_paths: + - .agents/review-rules/qa-dx/docker-native-modules.md + - .agents/review-rules/qa-dx/docker-image-pinning.md + - .agents/review-rules/qa-dx/workflow-safety.md + - .agents/review-rules/qa-dx/ratchets-and-allowlists.md + include: + - .github/** + - docker/** + - scripts/** + - patches/** + - packages/testing/** + - packages/@n8n/eslint-config/** + - packages/@n8n/stylelint-config/** + - packages/@n8n/vitest-config/** + - packages/@n8n/typescript-config/** + - '**/eslint.config.mjs' + - '**/vitest.config.*' + - '**/turbo.json' + - .poutine.yml + - codecov.yml + - .code-health-baseline.json + - .boundaries-baseline.json + - '**/.janitor-baseline.json' description: |- - You are a Staff Quality Engineer reviewing this PR for production - readiness. You think in terms of blast radius, resource cost, and - failure modes. You are pragmatic, not pedantic. + Review the build, test, and CI surface against the linked rules. These + are lessons from builds that actually broke and from checks that turned + out to be bypassable, not general Docker or Actions advice. - Below is a curated list of known issues from real incidents and - review standards the team enforces. Review the diff against each - item. If none match, say nothing. If one matches, explain the - concrete risk and suggest a fix. Never flag existing unchanged code. + Most of this surface is already scanned — Poutine, Zizmor, and + `@n8n/code-health` fail CI for the well-known hazards. Report what those + cannot see: a documented invariant being undone, a guard being widened, + a gate that stops being able to fail. - ## Known Issues - - ### 1. Eager imports of heavy or native modules - Top-level `import` of modules only used in a specific code path loads - them into every process at startup, increasing baseline memory. - Native modules (e.g. `isolated-vm`) can crash instances that lack the - binary. Large parsers (e.g. `jsdom` at ~16 MB heap) waste memory when - the code path is rarely hit. - **Fix:** Use `await import()` at point of use. For barrel files, use - `export type` instead of value re-exports when consumers only need - the type. - - ### 2. Unreasonable test coverage expectations - Be pragmatic about test requirements. Pass if core functionality and - critical paths are tested at reasonable coverage. Do NOT require tests - for exports, types, configs, metadata files, or version files. Let - humans handle edge cases. - - name: Design System Tokens - description: |- - Follow `.claude/plugins/n8n/skills/design-system-rules/SKILL.md`. - - Apply balanced enforcement: - - Strong warning: hard-coded visual values, legacy token usage, and - deprecated style/component surfaces. - - Soft warning: token-to-token substitutions (request intent/rationale). - - name: Use workflowDocumentStore for Migrated Workflow Fields - description: |- - We are actively migrating workflow state from `workflowsStore` (Pinia store defined in - `packages/frontend/editor-ui/src/app/stores/workflows.store.ts`) to `workflowDocumentStore` - (defined in `packages/frontend/editor-ui/src/app/stores/workflowDocument.store.ts`). - - The `workflowDocumentStore` is the single source of truth for migrated fields. - When new frontend code reads or writes any of the fields listed below, it **must** use - `workflowDocumentStore` instead of `workflowsStore.workflow.*` or equivalent accessors. - - **Already-migrated fields (do NOT access via workflowsStore):** - - `active` - - `activeVersion` - - `activeVersionId` - - `checksum` - - `createdAt` - - `homeProject` - - `meta` - - `pinData` - - `settings` - - `tags` - - `updatedAt` - - **Detection criteria — flag when NEW code in `packages/frontend/` does any of:** - 1. Reads a migrated field from `workflowsStore.workflow.` (e.g. `workflowsStore.workflow.tags`) - 2. Reads a migrated field from a destructured `workflow` ref originating from `workflowsStore` - 3. Writes to a migrated field through `workflowsStore` (e.g. `workflowsStore.workflow.active = true`) - 4. Calls a `workflowsStore` setter/action that only mutates a migrated field when - `workflowDocumentStore` already exposes an equivalent method - - **Do NOT flag:** - - Existing unchanged code (only review new or modified lines) - - Access to non-migrated fields (e.g. `workflowsStore.workflow.nodes`, `workflowsStore.workflow.connections`) - - Backend code — this rule applies only to `packages/frontend/` - - Code inside `workflowDocument.store.ts` or its sub-modules (those *are* the source of truth) - - Code inside `workflows.store.ts` itself (the migration is still in progress there) - - **Recommendation:** - Use `useWorkflowDocumentStore(createWorkflowDocumentId(workflowId))` from - `@/app/stores/workflowDocument.store.ts` to access migrated fields. - - This is an active migration. The list of migrated fields will grow over time. - - name: Use the Shared sleep Utility - description: |- - - Rule Statement: - Code must ALWAYS use `sleep` from `@n8n/utils/sleep` instead of - hand-rolling a helper that only wraps `setTimeout` in a promise. One - canonical helper keeps call sites consistent and makes its abortable - form discoverable — `sleep(ms, abortSignal?)` rejects early when the - signal fires, so a local abort-aware waiter is a violation too. - - The `no-restricted-sleep-definition` and `no-restricted-sleep-import` - ESLint rules already catch helpers literally named `sleep` / - `sleepWithAbort` and named `sleep` imports from `n8n-workflow`. Do not - repeat those as review comments — this rule covers only what they - cannot see syntactically. - - - Detection Criteria: - Flag NEW or MODIFIED code in `packages/**` where a delay is - re-implemented under a name ESLint does not restrict (`wait`, `delay`, - `pause`, `waitFor`, …) and the entire body is a promise wrapping - `setTimeout` with no additional behaviour: - - `const delay = (ms) => new Promise((resolve) => setTimeout(resolve, ms))` - - `async function wait(ms) { await new Promise((r) => setTimeout(r, ms)); }` - The shape is what matters, not the name. Also flag an inline, awaited - `new Promise((resolve) => setTimeout(resolve, ms))`. - - - Do NOT flag: - - Wrappers that do more than wait: racing against another promise, - rejecting with a timeout error once the delay elapses, retry - backoff with jitter, debounce/throttle. (Rejecting on *abort* is a - violation — `sleep` takes the signal.) - - Bare `setTimeout` calls used for scheduling, not awaited as a delay - - Test-framework helpers such as `vi.advanceTimersByTime` or waiting - on fake timers - - `packages/@n8n/node-cli/**` and `packages/@n8n/typeorm/**`, which - cannot depend on `@n8n/utils` and are exempt from the ESLint rules - - Existing unchanged code (review only new or modified lines) - - - Recommendation: - Import `sleep` from `@n8n/utils/sleep` — `await sleep(500)`, or - `await sleep(500, abortSignal)` when the wait must be cancellable. + # cubic silently drops any rule past the fifth, so the last slot is a decision, + # not somewhere to append. Merge into an existing agent instead. pr_descriptions: generate: false - instructions: Each PR is supposed to have a limited scope. In your review, focus on changes made in the PR and avoid pointing out problems you found in the code that already existed. issues: fix_with_cubic_buttons: true diff --git a/package.json b/package.json index 7b5271ad6a7..20a6f9c9740 100644 --- a/package.json +++ b/package.json @@ -52,6 +52,7 @@ "sync:skill-links": "node scripts/sync-agent-skill-links.mjs", "check:skill-links": "node scripts/sync-agent-skill-links.mjs --check", "check:workspace-private-deps": "node scripts/check-workspace-private-deps.mjs", + "check:cubic-config": "node .github/scripts/quality/check-cubic-config.mjs", "optimize-svg": "find ./packages -name '*.svg' ! -name 'pipedrive.svg' -print0 | xargs -0 -P16 -L20 npx svgo", "n8n-module-sdk": "node packages/@n8n/module-cli/bin/n8n-module-sdk.mjs", "setup-backend-module": "node scripts/ensure-zx.mjs && zx scripts/backend-module/setup.mjs",