ci: Rework the cubic security review rules (no-changelog) (#37417)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

.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.

.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

.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

.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

.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.

.agents/review-rules/security/data-and-infrastructure.md

@@ -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

.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<string, ...>();
-```
-
-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

.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`)

.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

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/**