diff --git a/.agents/review-rules/README.md b/.agents/review-rules/README.md index 28140c51a3d..b95b5250268 100644 --- a/.agents/review-rules/README.md +++ b/.agents/review-rules/README.md @@ -13,7 +13,7 @@ clear, when to stay silent, what never to say — lives once in | Directory | Agent | Scope | |-------------|----------|-----------------------------------------------------| -| `security/` | Security | backend packages + nodes | +| `security/` | Security | backend packages, nodes, and the frontend render surface | | `backend/` | Backend | `cli`, `@n8n/db`, `core`, `workflow`, node packages | | `db-migrations/` | DB migrations | `@n8n/db` migrations + their tests in `cli` | | `frontend/` | Frontend | `packages/frontend` | @@ -59,6 +59,9 @@ enforces them in CI: files — a backend PR still loads the node rules, since include globs are per-agent. 3. Add its path to every `file_paths` that should load it in `cubic.yaml`. + Keep each agent's list ordered by how often the class it covers actually + shows up — the ceiling truncates from the end, so the last file listed is the + one that silently disappears first. 4. 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. diff --git a/.agents/review-rules/backend/license-enforcement.md b/.agents/review-rules/backend/license-enforcement.md new file mode 100644 index 00000000000..33405474063 --- /dev/null +++ b/.agents/review-rules/backend/license-enforcement.md @@ -0,0 +1,14 @@ +# License enforcement + +Applies to: `packages/cli`, `*.ee.ts` files, licensed modules. + +Nothing fails the build when a paid feature ships ungated, so this is review-only. + +- Endpoints in `*.ee.ts` reaching licensed features without `@Licensed` — a + scope decorator is a permission check, not a license check +- `@Licensed` not matching the feature (`LICENSE_FEATURES` in `@n8n/constants`) +- Bypassed quota checks, or missing `FeatureNotLicensedError` on an unlicensed + path +- Custom licensing middleware where the `@Licensed` decorator fits +- Decorator order: route decorator → `@Licensed` → scope decorators +- Enterprise code reachable outside `*.ee.ts` files or licensed modules diff --git a/.agents/review-rules/security/auth-and-access-control.md b/.agents/review-rules/security/auth-and-access-control.md index eb677f783b6..cf996ee2a46 100644 --- a/.agents/review-rules/security/auth-and-access-control.md +++ b/.agents/review-rules/security/auth-and-access-control.md @@ -1,36 +1,18 @@ -# Authentication, authorization, and licensing +# Authentication and access control 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 +- JWT validation or expiration weakened; cookie flags (HttpOnly, Secure, SameSite) missing or disabled; MFA bypassed; SSO/SAML/OIDC validation gaps; OAuth 2.0 without PKCE or a weak redirect check ## 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. +`@n8n/code-health`'s `endpoint-scope-coverage` rule is disabled, so an undecorated route ships 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 +- Every authenticated route needs `@GlobalScope` or `@ProjectScope`, unless it declares `skipAuth`, `allowUnauthenticated`, or `apiKeyAuth` +- A decorator proves the caller may do this somewhere, not that the row is theirs. Ownership belongs in the query — a `find…ForUser` finder, or the project filter alongside the id — never a comparison afterwards +- An endpoint querying a repository directly while its siblings use a project-aware service — the public API reaching past it is the usual shape +- Middleware treating an authenticated session as sufficient (`if (req.user) return next()`), skipping the ownership check behind it +- A scope parameter defaulting to the weakest value, or a role map gaining write or execute: mutating call sites must ask for the stronger scope +- Subworkflow execution bypassing caller policies diff --git a/.agents/review-rules/security/code-execution-and-sandboxing.md b/.agents/review-rules/security/code-execution-and-sandboxing.md index c84ffd34744..32fdb08aae8 100644 --- a/.agents/review-rules/security/code-execution-and-sandboxing.md +++ b/.agents/review-rules/security/code-execution-and-sandboxing.md @@ -1,15 +1,17 @@ # Code execution and sandboxing -Applies to: `packages/cli`, `packages/core`, `packages/workflow`. +Applies to: `packages/cli`, `packages/core`, `packages/workflow`, Code and Merge nodes. + +Every sandbox n8n has shipped was eventually escaped, so the bar is reachability: if sandboxed code can reach it, assume it can climb out. 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 +- Give a sandboxed context new access to Node.js builtins, `process.env`, dynamic `require`/`import`, the filesystem, or the network +- Add a global, helper, or bridge callable from inside the sandbox that hands back a host-realm value — one returned object, thrown error, or callback is enough to walk `constructor` back out +- Weaken a prototype sanitizer, a blocklist, or the allowed globals +- Weaken file access restriction enforcement, or let unsanitized input reach a file path or an n8n internal directory -Higher scrutiny applies to the expression engine and the code execution nodes — -a regression there is reachable by any workflow author. +Two guards that did not hold before: + +- `typeof value === 'string'` before a blocklist check: `['__proto__']` reports `'object'` yet coerces to `__proto__` as a key. Compare `String(value)` +- An AST check covering computed member expressions only — the same name also arrives as a call argument or a template literal, neither a string literal diff --git a/.agents/review-rules/security/credentials-and-secrets.md b/.agents/review-rules/security/credentials-and-secrets.md index 2af151273d8..f0467cd0832 100644 --- a/.agents/review-rules/security/credentials-and-secrets.md +++ b/.agents/review-rules/security/credentials-and-secrets.md @@ -4,15 +4,10 @@ Applies to: all backend packages. Flag: -- Credentials logged or exposed in error messages +- Credential values reaching logs, error messages, response headers, or a REST response — a decrypted field returned "because the UI needs it" is the shape - 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 +- Credential encryption weakened, or `N8N_ENCRYPTION_KEY` reused for token signing, OAuth state, or anything but credential encryption +- OAuth state/CSRF validation bypassed, or webhook requests keeping auth cookies +- Credential access ignoring scope boundaries (instance/project/user), or a credential resolved by name and type with no ownership filter -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. +A caught integration error interpolated into a message is a common way credential material reaches a status field or an API response. diff --git a/.agents/review-rules/security/data-and-infrastructure.md b/.agents/review-rules/security/data-and-infrastructure.md deleted file mode 100644 index 5d6c6f4a34c..00000000000 --- a/.agents/review-rules/security/data-and-infrastructure.md +++ /dev/null @@ -1,32 +0,0 @@ -# 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 index cfef69795b4..8a99e467770 100644 --- a/.agents/review-rules/security/node-input-safety.md +++ b/.agents/review-rules/security/node-input-safety.md @@ -3,75 +3,32 @@ 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 here and `pnpm lint` runs with `--quiet`. -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)`): +Flag NEW code where a value from `this.getNodeParameter(...)` — directly or via a variable, a callback param, or a helper — is a computed key BUILDING A NESTED OR CONTAINER STRUCTURE on a plain object (`IDataObject` or object literal, not a `Map` or `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. +The carrier is usually a shared accumulator; a deep merge of user options into a config object is the same defect. 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)` +- Single-level writes of a concrete (non-object) value, e.g. `item.json[field] = value` — a primitive assigned to `__proto__` is a no-op. This is the common, benign case in nodes; flagging it is noise +- Keys that are 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. +Use `setSafeObjectProperty` / `isSafeObjectProperty` from `n8n-workflow`, or a `Map`, and coerce with `String(key)` before any check. ## Injection through node parameters -Flag NEW code where a node parameter reaches a sink without escaping or -parameterisation: +Flag NEW code where a parameter reaches a sink without escaping: -- SQL query fields built by string concatenation or expression interpolation - instead of bound parameters +- SQL values concatenated or expression-interpolated instead of bound. An expression is interpolation, not parameterisation +- SQL identifiers — table, column, `ORDER BY`, a cast suffix after `name:`. They cannot be bound, so they need an allowlist; numeric fields a numeric cast +- A SQL dialect reaching JavaScript (AlaSQL in the Merge node) gaining a new construct that can call a function - 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/.agents/review-rules/security/outbound-requests.md b/.agents/review-rules/security/outbound-requests.md new file mode 100644 index 00000000000..ce4a0f1d522 --- /dev/null +++ b/.agents/review-rules/security/outbound-requests.md @@ -0,0 +1,15 @@ +# Outbound requests + +Applies to: `packages/cli`, `packages/core`, `packages/nodes-base`, +`packages/@n8n/nodes-langchain`. + +A URL is user-controlled unless the code shows otherwise. The recurring finding is not a weak allowlist but a working check the new path never calls: `n8n-workflow` exports `assertUrlAllowed`, `assertCredentialAllowsUrl` and `getCredentialAllowedDomains`, and a request surface reaching none of them is the finding. + +Flag NEW code where: + +- A request built from user input reaches an HTTP client unchecked +- A later hop inherits the first hop's approval — pagination `next` links, redirects, `.well-known` discovery, webhook callbacks. Per-hop checks need the allowlist `getCredentialAllowedDomains` returns +- Credential material (`Authorization`, cookies, signed headers) reaches a host the check did not cover +- The URL arrives in a field that reads like configuration but the user writes: `requestTokenUrl`, `accessTokenUrl`, OAuth `serverUrl`, SAML `metadataUrl`, a region or host fragment reaching an SDK endpoint +- A URL is checked by regex rather than `new URL()`; regexes miss `@`-userinfo, `#` fragments and IPv6 literals +- Certificate validation is dropped (`rejectUnauthorized: false`) diff --git a/.agents/review-rules/security/rendering-untrusted-content.md b/.agents/review-rules/security/rendering-untrusted-content.md new file mode 100644 index 00000000000..8ee53e4f5c5 --- /dev/null +++ b/.agents/review-rules/security/rendering-untrusted-content.md @@ -0,0 +1,15 @@ +# Rendering untrusted content + +Applies to: `packages/frontend`, and backend paths serving HTML or user bytes. + +Workflow names, node parameters, and execution data are author-controlled +strings, rendered in a session with full REST access. + +Flag NEW code that: + +- Passes user-derived content to `v-html`, `innerHTML`, `insertAdjacentHTML`, or a render function concatenating markup, with no sanitizer in front of it +- Widens a sanitizer allowlist — tags, attributes, `on*` handlers, `srcdoc` — or drops a sanitize call from an existing render path +- Puts a user-supplied value into `href`, `src`, or a `window.open` target without rejecting non-`http(s)` schemes. `javascript:` and `data:` survive naive checks; parse with `new URL()`, never a regex with the `m` flag, which makes `^` match any line start +- Renders markdown or HTML returned by a node, an AI response, or a webhook body as trusted markup +- Serves user-controlled bytes from the n8n origin without `Content-Disposition: attachment` or an equivalent inline-render guard +- Loosens an iframe `sandbox` attribute or a CSP directive on a view or response carrying user content diff --git a/cubic.yaml b/cubic.yaml index 6548178eef6..aca4d1c4a3c 100644 --- a/cubic.yaml +++ b/cubic.yaml @@ -104,9 +104,10 @@ reviews: - 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/outbound-requests.md - .agents/review-rules/security/auth-and-access-control.md - - .agents/review-rules/security/data-and-infrastructure.md + - .agents/review-rules/security/rendering-untrusted-content.md + - .agents/review-rules/security/credentials-and-secrets.md - .agents/review-rules/security/node-input-safety.md include: - packages/cli/** @@ -115,17 +116,17 @@ reviews: - packages/workflow/** - packages/nodes-base/** - packages/@n8n/nodes-langchain/** + - packages/frontend/** exclude: - '**/__tests__/**' - '**/*.test.ts' - '**/*.spec.ts' + - '**/*.stories.ts' - '**/test/**' description: |- - Flag security defects introduced by this PR, using the linked rules. - - 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. + Flag security defects introduced by this PR, using the linked rules, + ordered by how often each class shows up. Community and custom nodes + carry a higher risk profile than official ones. - name: Backend file_paths: - .agents/review-rules/backend/controller-request-validation.md @@ -133,6 +134,7 @@ reviews: - .agents/review-rules/backend/explicit-any.md - .agents/review-rules/backend/lazy-load-heavy-modules.md - .agents/review-rules/backend/hand-rolled-delays.md + - .agents/review-rules/backend/license-enforcement.md - .agents/review-rules/testing/coverage.md include: - packages/cli/**