diff --git a/cubic.yaml b/cubic.yaml index 44fc469e132..db0a5df7886 100644 --- a/cubic.yaml +++ b/cubic.yaml @@ -99,6 +99,72 @@ reviews: - 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 + 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). + + 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 description: |- - Rule Statement: diff --git a/packages/nodes-base/AGENTS.md b/packages/nodes-base/AGENTS.md index bfac3874975..2a15ac6cc28 100644 --- a/packages/nodes-base/AGENTS.md +++ b/packages/nodes-base/AGENTS.md @@ -94,6 +94,37 @@ Change parameter type to `'resourceLocator'`, define modes (list, id, url), add - Use `NodeApiError` for API-related errors - Support `continueOnFail` option when appropriate +### Security + +User input is untrusted. In nodes it arrives mainly through +`this.getNodeParameter(...)` (and incoming `item.json`), and a workflow author +controls these values. + +**Never use an untrusted value as a computed object key in an assignment.** A +value such as `__proto__`, `constructor`, or `prototype` pollutes the prototype +chain: + +```ts +// UNSAFE — `table`/`key` come from this.getNodeParameter(...) +if (acc[table] === undefined) acc[table] = {}; +acc[table][key] = value; +``` + +Route dynamic-key writes through the `n8n-workflow` helpers, or build the +accumulator as a `Map` / `Object.create(null)`: + +```ts +import { setSafeObjectProperty, isSafeObjectProperty } from 'n8n-workflow'; + +if (isSafeObjectProperty(table) && acc[table] === undefined) { + setSafeObjectProperty(acc, table, {}); +} +``` + +This only applies to dynamic-key **writes** (grouping/aggregating rows by a +user-chosen column is the common case). Reads like `const x = obj[key]` are +safe. Reference usage: `nodes/Google/GSuiteAdmin/GSuiteAdmin.node.ts`. + ### Code Organization - Separate operation/field descriptions into separate files - Create reusable API request helpers in GenericFunctions