diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0a70f890d88..1ce2b557119 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -5,37 +5,47 @@ Great that you are here and you want to contribute to n8n ## Contents - [Contributing to n8n](#contributing-to-n8n) - - [Contents](#contents) - - [Code of conduct](#code-of-conduct) - - [Directory structure](#directory-structure) - - [Development setup](#development-setup) - - [Dev Container](#dev-container) - - [Requirements](#requirements) - - [Node.js](#nodejs) - - [pnpm](#pnpm) - - [pnpm workspaces](#pnpm-workspaces) - - [Build tools](#build-tools) - - [Actual n8n setup](#actual-n8n-setup) - - [Start](#start) - - [Development cycle](#development-cycle) - - [Stacked pull requests](#stacked-pull-requests) - - [Enabling `gh stack`](#enabling-gh-stack) - - [Splitting work into a stack yourself](#splitting-work-into-a-stack-yourself) - - [Asking an agent to do it](#asking-an-agent-to-do-it) - - [Community PR Guidelines](#community-pr-guidelines) - - [**1. Change Request/Comment**](#1-change-requestcomment) - - [**2. General Requirements**](#2-general-requirements) - - [**3. PR Specific Requirements**](#3-pr-specific-requirements) - - [**4. Workflow Summary for Non-Compliant PRs**](#4-workflow-summary-for-non-compliant-prs) - - [Test suite](#test-suite) - - [Unit tests](#unit-tests) - - [Code Coverage](#code-coverage) - - [E2E tests](#e2e-tests) - - [Releasing](#releasing) - - [Create custom nodes](#create-custom-nodes) - - [Extend documentation](#extend-documentation) - - [Contribute workflow templates](#contribute-workflow-templates) - - [Contributor License Agreement](#contributor-license-agreement) + - [Contents](#contents) + - [Code of conduct](#code-of-conduct) + - [Directory structure](#directory-structure) + - [Development setup](#development-setup) + - [Dev Container](#dev-container) + - [Requirements](#requirements) + - [Node.js](#nodejs) + - [pnpm](#pnpm) + - [pnpm workspaces](#pnpm-workspaces) + - [corepack](#corepack) + - [Build tools](#build-tools) + - [actionlint (for GitHub Actions workflow development)](#actionlint-for-github-actions-workflow-development) + - [tbls (for database schema docs)](#tbls-for-database-schema-docs) + - [Actual n8n setup](#actual-n8n-setup) + - [For external contributors](#for-external-contributors) + - [For everyone](#for-everyone) + - [Start](#start) + - [Environment variables (optional)](#environment-variables-optional) + - [Development cycle](#development-cycle) + - [Basic Development Workflow Example (most used within n8n)](#basic-development-workflow-example-most-used-within-n8n) + - [Selective Package Development](#selective-package-development) + - [Available Filtered Commands](#available-filtered-commands) + - [Custom Selective Development](#custom-selective-development) + - [Running the BE server with a clean database](#running-the-be-server-with-a-clean-database) + - [Hot Reload for Nodes (N8N\_DEV\_RELOAD)](#hot-reload-for-nodes-n8n_dev_reload) + - [Run local instances in different configurations](#run-local-instances-in-different-configurations) + - [Work locally with syslog](#work-locally-with-syslog) + - [Performance Considerations](#performance-considerations) + - [Community PR Guidelines](#community-pr-guidelines) + - [**1. Change Request/Comment**](#1-change-requestcomment) + - [**2. General Requirements**](#2-general-requirements) + - [**3. PR Specific Requirements**](#3-pr-specific-requirements) + - [**4. Workflow Summary for Non-Compliant PRs**](#4-workflow-summary-for-non-compliant-prs) + - [Test suite](#test-suite) + - [Unit tests](#unit-tests) + - [Code Coverage](#code-coverage) + - [E2E tests](#e2e-tests) + - [Create custom nodes](#create-custom-nodes) + - [Extend documentation](#extend-documentation) + - [Contribute workflow templates](#contribute-workflow-templates) + - [Contributor License Agreement](#contributor-license-agreement) ## Code of conduct diff --git a/packages/@n8n/config/src/configs/__tests__/security.config.test.ts b/packages/@n8n/config/src/configs/__tests__/security.config.test.ts index b877f3d8288..cb271bcc4d1 100644 --- a/packages/@n8n/config/src/configs/__tests__/security.config.test.ts +++ b/packages/@n8n/config/src/configs/__tests__/security.config.test.ts @@ -62,11 +62,13 @@ describe('SecurityConfig', () => { // serving every HTML page. It has to warn and leave the declared default instead. const unservable = `script-src 'self'${String.fromCharCode(10)}X-Injected: yes`; - test('warns and enforces nothing when the enforced policy is unservable', () => { + test("warns and keeps n8n's own policy when the enforced policy is unservable", () => { const consoleWarnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); process.env = { N8N_CONTENT_SECURITY_POLICY: unservable }; - expect(Container.get(SecurityConfig).contentSecurityPolicy).toBeUndefined(); + expect(Container.get(SecurityConfig).contentSecurityPolicy).toBe( + DEFAULT_CONTENT_SECURITY_POLICY, + ); expect(consoleWarnSpy).toHaveBeenCalledWith( expect.stringContaining('N8N_CONTENT_SECURITY_POLICY'), ); @@ -74,13 +76,11 @@ describe('SecurityConfig', () => { consoleWarnSpy.mockRestore(); }); - test("warns and keeps n8n's own policy when the report-only policy is unservable", () => { + test('warns and reports on nothing when the report-only policy is unservable', () => { const consoleWarnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); process.env = { N8N_CONTENT_SECURITY_POLICY_REPORT_ONLY: unservable }; - expect(Container.get(SecurityConfig).contentSecurityPolicyReportOnly).toBe( - DEFAULT_CONTENT_SECURITY_POLICY, - ); + expect(Container.get(SecurityConfig).contentSecurityPolicyReportOnly).toBeUndefined(); expect(consoleWarnSpy).toHaveBeenCalledWith( expect.stringContaining('N8N_CONTENT_SECURITY_POLICY_REPORT_ONLY'), ); diff --git a/packages/@n8n/config/src/configs/content-security-policy.ts b/packages/@n8n/config/src/configs/content-security-policy.ts index 45ef0762d33..b977e22acad 100644 --- a/packages/@n8n/config/src/configs/content-security-policy.ts +++ b/packages/@n8n/config/src/configs/content-security-policy.ts @@ -2,7 +2,7 @@ import { NONCE_PLACEHOLDER } from '@n8n/constants'; import { z } from 'zod'; /** - * The [Level 3](https://web.dev/articles/strict-csp) policy n8n reports on by default. + * The [Level 3](https://web.dev/articles/strict-csp) policy n8n enforces by default. * * {@link NONCE_PLACEHOLDER} takes the response's nonce when n8n serves the header. */ diff --git a/packages/@n8n/config/src/configs/security.config.ts b/packages/@n8n/config/src/configs/security.config.ts index e24831f5086..0dc0b177c29 100644 --- a/packages/@n8n/config/src/configs/security.config.ts +++ b/packages/@n8n/config/src/configs/security.config.ts @@ -81,33 +81,32 @@ export class SecurityConfig { * Write `` where the per-request nonce should go to keep n8n's own scripts working, * e.g. `script-src 'strict-dynamic'`. * - * Set to `default` to enforce n8n's own policy without transcribing it. - * - * Empty by default: n8n enforces nothing until a policy is set here. See - * `N8N_CONTENT_SECURITY_POLICY_REPORT_ONLY` for the policy it reports on. + * Defaults to n8n's Level 3 policy, which the instance enforces. Set it to `default` for + * that policy explicitly, or to `{}` to enforce nothing. See + * `N8N_CONTENT_SECURITY_POLICY_REPORT_ONLY` to try a policy out before enforcing it. * * Parsed on read, so this holds the policy to send, or `undefined` to send no header. - * A value that cannot be read warns and leaves this `undefined`: a policy n8n cannot - * parse must not be enforced. + * A value that cannot be read warns and leaves the default policy in place: a policy n8n + * cannot parse must not be enforced. */ @Env('N8N_CONTENT_SECURITY_POLICY', contentSecurityPolicySchema) - contentSecurityPolicy: ContentSecurityPolicySetting = undefined; + contentSecurityPolicy: ContentSecurityPolicySetting = DEFAULT_CONTENT_SECURITY_POLICY; /** * The policy n8n serves as `Content-Security-Policy-Report-Only`, in the same two formats * `N8N_CONTENT_SECURITY_POLICY` accepts. This header blocks nothing, so use it to try a * policy out first. Both headers report violations, but only the enforced one blocks. * - * Defaults to n8n's Level 3 policy. Set it to `default` for that policy explicitly, or to - * `{}` to send no report-only header. + * Empty by default: n8n enforces its own policy instead, and an enforced header already + * reports what it blocks. Set this to `default` for n8n's policy, or to the policy you + * want to try out before you enforce it. * * Parsed on read, as `N8N_CONTENT_SECURITY_POLICY` is. The variable held a boolean until * it took a policy, so a boolean parses to `{ legacyBoolean }` for the caller to honor * with a deprecation warning. */ @Env('N8N_CONTENT_SECURITY_POLICY_REPORT_ONLY', contentSecurityPolicyReportOnlySchema) - contentSecurityPolicyReportOnly: ContentSecurityPolicyReportOnlySetting = - DEFAULT_CONTENT_SECURITY_POLICY; + contentSecurityPolicyReportOnly: ContentSecurityPolicyReportOnlySetting = undefined; /** * Configuration for the `Cross-Origin-Opener-Policy` header. @@ -120,8 +119,8 @@ export class SecurityConfig { * Whether to disable the `sandbox` directive in the CSP header for webhooks. * The sandboxing mechanism uses CSP headers now, but the name is kept for backwards compatibility. * - * To disable the entire CSP, use `N8N_CONTENT_SECURITY_POLICY_REPORT_ONLY` or override the policy with - * `N8N_CONTENT_SECURITY_POLICY`. + * To disable the entire CSP, set `N8N_CONTENT_SECURITY_POLICY` to `{}`. To change it, set + * that variable to your own policy. */ @Env('N8N_INSECURE_DISABLE_WEBHOOK_IFRAME_SANDBOX') disableWebhookHtmlSandboxing: boolean = false; @@ -134,8 +133,8 @@ export class SecurityConfig { * The correct way to prevent this is to configure forms to be served from a different * (sub)domain instead of disabling the sandbox. * - * To disable the entire CSP, use `N8N_CONTENT_SECURITY_POLICY_REPORT_ONLY` or override the policy with - * `N8N_CONTENT_SECURITY_POLICY`. + * To disable the entire CSP, set `N8N_CONTENT_SECURITY_POLICY` to `{}`. To change it, set + * that variable to your own policy. */ @Env('N8N_INSECURE_DISABLE_FORM_HTML_SANDBOX') disableFormHtmlSandboxing: boolean = false; diff --git a/packages/@n8n/config/test/config.test.ts b/packages/@n8n/config/test/config.test.ts index 5279d5cf154..7d471750e63 100644 --- a/packages/@n8n/config/test/config.test.ts +++ b/packages/@n8n/config/test/config.test.ts @@ -537,8 +537,8 @@ describe('GlobalConfig', () => { blockFileAccessToN8nFiles: true, blockFilePatterns: '^(?:[^/]*/)*\\.git(?:/.*)?$', daysAbandonedWorkflow: 90, - contentSecurityPolicy: undefined, - contentSecurityPolicyReportOnly: DEFAULT_CONTENT_SECURITY_POLICY, + contentSecurityPolicy: DEFAULT_CONTENT_SECURITY_POLICY, + contentSecurityPolicyReportOnly: undefined, crossOriginOpenerPolicy: 'same-origin-allow-popups', disableWebhookHtmlSandboxing: false, disableFormHtmlSandboxing: false, diff --git a/packages/cli/BREAKING-CHANGES.md b/packages/cli/BREAKING-CHANGES.md index 9e8d63e9f76..94b5b83ffc4 100644 --- a/packages/cli/BREAKING-CHANGES.md +++ b/packages/cli/BREAKING-CHANGES.md @@ -28,6 +28,14 @@ The Execute Sub-workflow node no longer supports the "Local File" and "URL" sour If your workflows use an Execute Sub-workflow node with the "Local File" or "URL" source. Save the sub-workflow on the instance and use the "Database" source, or paste the workflow JSON into the "Define Below" source. The migration report on v2 lists every affected node. +### What changed? + +n8n enforces a Content-Security-Policy on its own HTML pages. The policy was served as `Content-Security-Policy-Report-Only` before, which reported violations but blocked nothing. The enforced policy is `script-src 'strict-dynamic' 'unsafe-eval'; object-src 'none'; base-uri 'none'`: only scripts that carry the response nonce run, `` and `` are blocked, and a `` tag cannot repoint relative URLs. `N8N_CONTENT_SECURITY_POLICY_REPORT_ONLY` no longer sends a header by default. Webhook and form pages keep their own sandbox policy and are not affected. + +### When is action necessary? + +If you inject your own scripts into n8n's pages. Test the instance before you update, because a script without the response nonce no longer runs. Set `N8N_CONTENT_SECURITY_POLICY` to `{}` to enforce nothing, or to your own policy. To try a policy before you enforce it, put it in `N8N_CONTENT_SECURITY_POLICY_REPORT_ONLY` instead. + # 2.0.0 ### What changed? diff --git a/packages/cli/src/security/__tests__/content-security-policy.test.ts b/packages/cli/src/security/__tests__/content-security-policy.test.ts index e01138703e4..3126fb068b8 100644 --- a/packages/cli/src/security/__tests__/content-security-policy.test.ts +++ b/packages/cli/src/security/__tests__/content-security-policy.test.ts @@ -28,8 +28,11 @@ describe('resolveContentSecurityPolicies', () => { ) => resolveContentSecurityPolicies(policy, reportOnly, logger); /** What each setting holds when the operator sets neither variable. */ - const UNSET_POLICY = undefined; - const UNSET_REPORT_ONLY = DEFAULT_CONTENT_SECURITY_POLICY; + const UNSET_POLICY = DEFAULT_CONTENT_SECURITY_POLICY; + const UNSET_REPORT_ONLY = undefined; + + /** What the enforced setting holds when the operator turns that header off with `{}`. */ + const NO_POLICY = undefined; const A = "script-src 'strict-dynamic' 'unsafe-eval'"; const B = "script-src 'strict-dynamic'"; @@ -44,34 +47,41 @@ describe('resolveContentSecurityPolicies', () => { const rows: Row[] = [ [ - 'neither set: report on the default policy, enforce nothing', + 'neither set: enforce the default policy, send no report-only header', UNSET_POLICY, UNSET_REPORT_ONLY, - { enforced: undefined, reportOnly: DEFAULT_CONTENT_SECURITY_POLICY }, + { enforced: DEFAULT_CONTENT_SECURITY_POLICY, reportOnly: undefined }, ], [ - 'report-only set: report on that policy, enforce nothing', + 'report-only set: enforce the default policy, report on the other', UNSET_POLICY, B, + { enforced: DEFAULT_CONTENT_SECURITY_POLICY, reportOnly: B }, + ], + [ + 'policy set: enforce it, report on nothing', + A, + UNSET_REPORT_ONLY, + { enforced: A, reportOnly: undefined }, + ], + ['both set: enforce one, report on the other', A, B, { enforced: A, reportOnly: B }], + [ + // The operator asked for both headers, so both go out. + 'the same policy in both: send it twice', + A, + A, + { enforced: A, reportOnly: A }, + ], + [ + 'enforcement off, report-only set: try the policy out, block nothing', + NO_POLICY, + B, { enforced: undefined, reportOnly: B }, ], [ - 'policy set: enforce it, keep reporting on the default', - A, + 'enforcement off: no CSP headers at all', + NO_POLICY, UNSET_REPORT_ONLY, - { enforced: A, reportOnly: DEFAULT_CONTENT_SECURITY_POLICY }, - ], - ['both set: enforce one, report on the other', A, B, { enforced: A, reportOnly: B }], - [ - 'policy set, reporting off: enforce only', - A, - undefined, - { enforced: A, reportOnly: undefined }, - ], - [ - 'both off: no CSP headers at all', - UNSET_POLICY, - undefined, { enforced: undefined, reportOnly: undefined }, ], [ @@ -81,16 +91,28 @@ describe('resolveContentSecurityPolicies', () => { { enforced: undefined, reportOnly: A }, ], [ - 'legacy `true` with no policy configured: report on the default', + 'legacy `true` with no policy configured: report on the default, enforce nothing', UNSET_POLICY, { legacyBoolean: true }, { enforced: undefined, reportOnly: DEFAULT_CONTENT_SECURITY_POLICY }, ], + [ + 'legacy `true` with enforcement off: no CSP headers at all', + NO_POLICY, + { legacyBoolean: true }, + { enforced: undefined, reportOnly: undefined }, + ], [ 'legacy `false`: the configured policy is enforced', A, { legacyBoolean: false }, - { enforced: A, reportOnly: DEFAULT_CONTENT_SECURITY_POLICY }, + { enforced: A, reportOnly: undefined }, + ], + [ + 'legacy `false` with no policy configured: enforce the default policy', + UNSET_POLICY, + { legacyBoolean: false }, + { enforced: DEFAULT_CONTENT_SECURITY_POLICY, reportOnly: undefined }, ], ]; @@ -127,6 +149,13 @@ describe('resolveContentSecurityPolicies', () => { expect(enforced).not.toBe('true'); expect(reportOnly).not.toBe('true'); }); + + // The variable was the only way to keep the policy report-only, so honoring `true` + // has to beat the enforced default an upgrade now brings in. + it('should enforce nothing for `true`, whatever the enforced setting holds', () => { + expect(resolve(UNSET_POLICY, { legacyBoolean: true }).enforced).toBeUndefined(); + expect(resolve(A, { legacyBoolean: true }).enforced).toBeUndefined(); + }); }); }); diff --git a/packages/cli/src/security/content-security-policy.ts b/packages/cli/src/security/content-security-policy.ts index 1825a671cd7..08390796dff 100644 --- a/packages/cli/src/security/content-security-policy.ts +++ b/packages/cli/src/security/content-security-policy.ts @@ -3,7 +3,7 @@ import type { ContentSecurityPolicyReportOnlySetting, ContentSecurityPolicySetting, } from '@n8n/config'; -import { DEFAULT_CONTENT_SECURITY_POLICY, isLegacyBooleanSetting } from '@n8n/config'; +import { isLegacyBooleanSetting } from '@n8n/config'; import { NONCE_PLACEHOLDER } from '@n8n/constants'; export type ContentSecurityPolicies = { @@ -18,8 +18,8 @@ export type ContentSecurityPolicies = { * already read each variable on its own; the only decision left is the one that needs * both, namely the boolean the report-only variable used to hold. * - * Only the report-only variable carries a policy by default, so a new instance reports - * violations but cannot break on them. + * Only the enforced variable carries a policy by default, so a new instance enforces + * n8n's policy and sends no report-only header. */ export const resolveContentSecurityPolicies = ( policy: ContentSecurityPolicySetting, @@ -31,11 +31,13 @@ export const resolveContentSecurityPolicies = ( 'N8N_CONTENT_SECURITY_POLICY_REPORT_ONLY is deprecated as a boolean: the variable now holds the policy to report on, in the same formats as N8N_CONTENT_SECURITY_POLICY. Honoring the old meaning for now - set it to a policy, or to `{}` to report on nothing.', ); - // Read as a policy, `true` would start enforcing a policy that the instance - // deliberately kept report-only. - return reportOnly.legacyBoolean - ? { reportOnly: policy ?? DEFAULT_CONTENT_SECURITY_POLICY } - : { enforced: policy, reportOnly: DEFAULT_CONTENT_SECURITY_POLICY }; + // `true` used to mean "report, never block". Enforcing the default policy here + // would break an instance that deliberately asked for report-only. + if (reportOnly.legacyBoolean) return { reportOnly: policy }; + + // `false` meant "enforce". The boolean took the place of a report-only policy, so + // there is none to send. + return { enforced: policy }; } return { enforced: policy, reportOnly };