From b2ca0500311d85742ef8abf8f9f0d1436e6d9ba1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Mon, 13 Nov 2023 11:50:43 +0100 Subject: [PATCH] perf(core): Lazyload security audit reporters (#7696) Also converting to service. Followup to https://github.com/n8n-io/n8n/pull/7663 --- packages/cli/src/PublicApi/types.ts | 2 +- .../v1/handlers/audit/audit.handler.ts | 5 +- packages/cli/src/audit/index.ts | 69 ------ .../cli/src/audit/risks/credentials.risk.ts | 146 ------------ packages/cli/src/audit/risks/database.risk.ts | 106 --------- .../cli/src/audit/risks/filesystem.risk.ts | 35 --- packages/cli/src/audit/risks/instance.risk.ts | 202 ----------------- packages/cli/src/audit/risks/nodes.risk.ts | 116 ---------- packages/cli/src/commands/audit.ts | 11 +- .../security-audit/SecurityAudit.service.ts | 63 ++++++ .../{audit => security-audit}/constants.ts | 2 +- .../risk-reporters/CredentialsRiskReporter.ts | 159 +++++++++++++ .../risk-reporters/DatabaseRiskReporter.ts | 110 +++++++++ .../risk-reporters/FilesystemRiskReporter.ts | 39 ++++ .../risk-reporters/InstanceRiskReporter.ts | 209 ++++++++++++++++++ .../risk-reporters/NodesRiskReporter.ts | 123 +++++++++++ .../src/{audit => security-audit}/types.ts | 4 + .../src/{audit => security-audit}/utils.ts | 2 +- .../CredentialsRiskReporter.test.ts} | 16 +- .../DatabaseRiskReporter.test.ts} | 16 +- .../FilesystemRiskReporter.test.ts} | 12 +- .../InstanceRiskReporter.test.ts} | 28 ++- .../NodesRiskReporter.test.ts} | 18 +- .../{audit => security-audit}/utils.ts | 4 +- 24 files changed, 779 insertions(+), 718 deletions(-) delete mode 100644 packages/cli/src/audit/index.ts delete mode 100644 packages/cli/src/audit/risks/credentials.risk.ts delete mode 100644 packages/cli/src/audit/risks/database.risk.ts delete mode 100644 packages/cli/src/audit/risks/filesystem.risk.ts delete mode 100644 packages/cli/src/audit/risks/instance.risk.ts delete mode 100644 packages/cli/src/audit/risks/nodes.risk.ts create mode 100644 packages/cli/src/security-audit/SecurityAudit.service.ts rename packages/cli/src/{audit => security-audit}/constants.ts (98%) create mode 100644 packages/cli/src/security-audit/risk-reporters/CredentialsRiskReporter.ts create mode 100644 packages/cli/src/security-audit/risk-reporters/DatabaseRiskReporter.ts create mode 100644 packages/cli/src/security-audit/risk-reporters/FilesystemRiskReporter.ts create mode 100644 packages/cli/src/security-audit/risk-reporters/InstanceRiskReporter.ts create mode 100644 packages/cli/src/security-audit/risk-reporters/NodesRiskReporter.ts rename packages/cli/src/{audit => security-audit}/types.ts (95%) rename packages/cli/src/{audit => security-audit}/utils.ts (93%) rename packages/cli/test/integration/{audit/credentials.risk.test.ts => security-audit/CredentialsRiskReporter.test.ts} (91%) rename packages/cli/test/integration/{audit/database.risk.test.ts => security-audit/DatabaseRiskReporter.test.ts} (89%) rename packages/cli/test/integration/{audit/filesystem.risk.test.ts => security-audit/FilesystemRiskReporter.test.ts} (81%) rename packages/cli/test/integration/{audit/instance.risk.test.ts => security-audit/InstanceRiskReporter.test.ts} (87%) rename packages/cli/test/integration/{audit/nodes.risk.test.ts => security-audit/NodesRiskReporter.test.ts} (84%) rename packages/cli/test/integration/{audit => security-audit}/utils.ts (97%) diff --git a/packages/cli/src/PublicApi/types.ts b/packages/cli/src/PublicApi/types.ts index cae8e64b77..fe40fdf63a 100644 --- a/packages/cli/src/PublicApi/types.ts +++ b/packages/cli/src/PublicApi/types.ts @@ -9,7 +9,7 @@ import type { WorkflowEntity } from '@db/entities/WorkflowEntity'; import type { UserManagementMailer } from '@/UserManagement/email'; -import type { Risk } from '@/audit/types'; +import type { Risk } from '@/security-audit/types'; export type AuthlessRequest< RouteParams = {}, diff --git a/packages/cli/src/PublicApi/v1/handlers/audit/audit.handler.ts b/packages/cli/src/PublicApi/v1/handlers/audit/audit.handler.ts index 89e4479944..a6e4787134 100644 --- a/packages/cli/src/PublicApi/v1/handlers/audit/audit.handler.ts +++ b/packages/cli/src/PublicApi/v1/handlers/audit/audit.handler.ts @@ -1,14 +1,15 @@ import { authorize } from '@/PublicApi/v1/shared/middlewares/global.middleware'; -import { audit } from '@/audit'; import type { Response } from 'express'; import type { AuditRequest } from '@/PublicApi/types'; +import Container from 'typedi'; export = { generateAudit: [ authorize(['owner']), async (req: AuditRequest.Generate, res: Response): Promise => { try { - const result = await audit( + const { SecurityAuditService } = await import('@/security-audit/SecurityAudit.service'); + const result = await Container.get(SecurityAuditService).run( req.body?.additionalOptions?.categories, req.body?.additionalOptions?.daysAbandonedWorkflow, ); diff --git a/packages/cli/src/audit/index.ts b/packages/cli/src/audit/index.ts deleted file mode 100644 index 0d73b8a542..0000000000 --- a/packages/cli/src/audit/index.ts +++ /dev/null @@ -1,69 +0,0 @@ -import { separate } from '@/utils'; -import config from '@/config'; -import { RISK_CATEGORIES } from '@/audit/constants'; -import { toReportTitle } from '@/audit/utils'; -import { reportCredentialsRisk } from '@/audit/risks/credentials.risk'; -import { reportDatabaseRisk } from '@/audit/risks/database.risk'; -import { reportNodesRisk } from '@/audit/risks/nodes.risk'; -import { reportFilesystemRisk } from '@/audit/risks/filesystem.risk'; -import { reportInstanceRisk } from '@/audit/risks/instance.risk'; -import type { Risk } from '@/audit/types'; -import Container from 'typedi'; -import { WorkflowRepository } from '@db/repositories/workflow.repository'; - -export const SYNC_MAP: Record = { - database: reportDatabaseRisk, - filesystem: reportFilesystemRisk, -}; - -export const ASYNC_MAP: Record = { - credentials: reportCredentialsRisk, - nodes: reportNodesRisk, - instance: reportInstanceRisk, -}; - -export const isAsync = (c: Risk.Category) => Object.keys(ASYNC_MAP).includes(c); - -export async function audit( - categories: Risk.Category[] = RISK_CATEGORIES, - daysAbandonedWorkflow?: number, -) { - if (categories.length === 0) categories = RISK_CATEGORIES; - - const daysFromEnv = config.getEnv('security.audit.daysAbandonedWorkflow'); - - if (daysAbandonedWorkflow) { - config.set('security.audit.daysAbandonedWorkflow', daysAbandonedWorkflow); - } - - const workflows = await Container.get(WorkflowRepository).find({ - select: ['id', 'name', 'active', 'nodes', 'connections'], - }); - - const [asyncCategories, syncCategories] = separate(categories, isAsync); - - const reports: Risk.Report[] = []; - - if (asyncCategories.length > 0) { - const promises = asyncCategories.map(async (c) => ASYNC_MAP[c](workflows)); - const asyncReports = await Promise.all(promises); - asyncReports.forEach((r) => r !== null && reports.push(r)); - } - - if (syncCategories.length > 0) { - const syncReports = syncCategories.map((c) => SYNC_MAP[c](workflows)); - syncReports.forEach((r) => r !== null && reports.push(r)); - } - - if (daysAbandonedWorkflow) { - config.set('security.audit.daysAbandonedWorkflow', daysFromEnv); // restore env - } - - if (reports.length === 0) return []; // trigger empty state - - return reports.reduce((acc, cur) => { - acc[toReportTitle(cur.risk)] = cur; - - return acc; - }, {}); -} diff --git a/packages/cli/src/audit/risks/credentials.risk.ts b/packages/cli/src/audit/risks/credentials.risk.ts deleted file mode 100644 index 31df1f44fe..0000000000 --- a/packages/cli/src/audit/risks/credentials.risk.ts +++ /dev/null @@ -1,146 +0,0 @@ -import { In, MoreThanOrEqual } from 'typeorm'; -import { DateUtils } from 'typeorm/util/DateUtils'; -import { Container } from 'typedi'; -import type { IWorkflowBase } from 'n8n-workflow'; -import config from '@/config'; -import { CREDENTIALS_REPORT } from '@/audit/constants'; -import type { Risk } from '@/audit/types'; -import type { WorkflowEntity } from '@db/entities/WorkflowEntity'; -import { CredentialsRepository } from '@db/repositories/credentials.repository'; -import { ExecutionRepository } from '@db/repositories/execution.repository'; -import { ExecutionDataRepository } from '@db/repositories/executionData.repository'; - -async function getAllCredsInUse(workflows: WorkflowEntity[]) { - const credsInAnyUse = new Set(); - const credsInActiveUse = new Set(); - - workflows.forEach((workflow) => { - workflow.nodes.forEach((node) => { - if (!node.credentials) return; - - Object.values(node.credentials).forEach((cred) => { - if (!cred?.id) return; - - credsInAnyUse.add(cred.id); - - if (workflow.active) credsInActiveUse.add(cred.id); - }); - }); - }); - - return { - credsInAnyUse, - credsInActiveUse, - }; -} - -async function getAllExistingCreds() { - const credentials = await Container.get(CredentialsRepository).find({ select: ['id', 'name'] }); - - return credentials.map(({ id, name }) => ({ kind: 'credential' as const, id, name })); -} - -async function getExecutedWorkflowsInPastDays(days: number): Promise { - const date = new Date(); - - date.setDate(date.getDate() - days); - - const executionIds = await Container.get(ExecutionRepository) - .find({ - select: ['id'], - where: { - startedAt: MoreThanOrEqual(DateUtils.mixedDateToUtcDatetimeString(date) as Date), - }, - }) - .then((executions) => executions.map(({ id }) => id)); - - return Container.get(ExecutionDataRepository) - .find({ - select: ['workflowData'], - where: { - executionId: In(executionIds), - }, - }) - .then((executionData) => executionData.map(({ workflowData }) => workflowData)); -} - -/** - * Return IDs of credentials in workflows executed in the past n days. - */ -async function getCredsInRecentlyExecutedWorkflows(days: number) { - const executedWorkflows = await getExecutedWorkflowsInPastDays(days); - - return executedWorkflows.reduce>((acc, { nodes }) => { - nodes.forEach((node) => { - if (node.credentials) { - Object.values(node.credentials).forEach((c) => { - if (c.id) acc.add(c.id); - }); - } - }); - - return acc; - }, new Set()); -} - -export async function reportCredentialsRisk(workflows: WorkflowEntity[]) { - const days = config.getEnv('security.audit.daysAbandonedWorkflow'); - - const allExistingCreds = await getAllExistingCreds(); - const { credsInAnyUse, credsInActiveUse } = await getAllCredsInUse(workflows); - const recentlyExecutedCreds = await getCredsInRecentlyExecutedWorkflows(days); - - const credsNotInAnyUse = allExistingCreds.filter((c) => !credsInAnyUse.has(c.id)); - const credsNotInActiveUse = allExistingCreds.filter((c) => !credsInActiveUse.has(c.id)); - const credsNotRecentlyExecuted = allExistingCreds.filter((c) => !recentlyExecutedCreds.has(c.id)); - - const issues = [credsNotInAnyUse, credsNotInActiveUse, credsNotRecentlyExecuted]; - - if (issues.every((i) => i.length === 0)) return null; - - const report: Risk.StandardReport = { - risk: CREDENTIALS_REPORT.RISK, - sections: [], - }; - - const hint = 'Keeping unused credentials in your instance is an unneeded security risk.'; - const recommendation = 'Consider deleting these credentials if you no longer need them.'; - - const sentenceStart = ({ length }: { length: number }) => - length > 1 ? 'These credentials are' : 'This credential is'; - - if (credsNotInAnyUse.length > 0) { - report.sections.push({ - title: CREDENTIALS_REPORT.SECTIONS.CREDS_NOT_IN_ANY_USE, - description: [sentenceStart(credsNotInAnyUse), 'not used in any workflow.', hint].join(' '), - recommendation, - location: credsNotInAnyUse, - }); - } - - if (credsNotInActiveUse.length > 0) { - report.sections.push({ - title: CREDENTIALS_REPORT.SECTIONS.CREDS_NOT_IN_ACTIVE_USE, - description: [sentenceStart(credsNotInActiveUse), 'not used in active workflows.', hint].join( - ' ', - ), - recommendation, - location: credsNotInActiveUse, - }); - } - - if (credsNotRecentlyExecuted.length > 0) { - report.sections.push({ - title: CREDENTIALS_REPORT.SECTIONS.CREDS_NOT_RECENTLY_EXECUTED, - description: [ - sentenceStart(credsNotRecentlyExecuted), - `not used in recently executed workflows, i.e. workflows executed in the past ${days} days.`, - hint, - ].join(' '), - recommendation, - location: credsNotRecentlyExecuted, - }); - } - - return report; -} diff --git a/packages/cli/src/audit/risks/database.risk.ts b/packages/cli/src/audit/risks/database.risk.ts deleted file mode 100644 index c0f62438c3..0000000000 --- a/packages/cli/src/audit/risks/database.risk.ts +++ /dev/null @@ -1,106 +0,0 @@ -import { toFlaggedNode } from '@/audit/utils'; -import { - SQL_NODE_TYPES, - DATABASE_REPORT, - DB_QUERY_PARAMS_DOCS_URL, - SQL_NODE_TYPES_WITH_QUERY_PARAMS, -} from '@/audit/constants'; -import type { WorkflowEntity as Workflow } from '@db/entities/WorkflowEntity'; -import type { Risk } from '@/audit/types'; - -function getIssues(workflows: Workflow[]) { - return workflows.reduce<{ [sectionTitle: string]: Risk.NodeLocation[] }>( - (acc, workflow) => { - workflow.nodes.forEach((node) => { - if (!SQL_NODE_TYPES.has(node.type)) return; - if (node.parameters === undefined) return; - if (node.parameters.operation !== 'executeQuery') return; - - if (typeof node.parameters.query === 'string' && node.parameters.query.startsWith('=')) { - acc.expressionsInQueries.push(toFlaggedNode({ node, workflow })); - } - - if (!SQL_NODE_TYPES_WITH_QUERY_PARAMS.has(node.type)) return; - - if (!node.parameters.additionalFields) { - acc.unusedQueryParams.push(toFlaggedNode({ node, workflow })); - } - - if (typeof node.parameters.additionalFields !== 'object') return; - if (node.parameters.additionalFields === null) return; - - if (!('queryParams' in node.parameters.additionalFields)) { - acc.unusedQueryParams.push(toFlaggedNode({ node, workflow })); - } - - if ( - 'queryParams' in node.parameters.additionalFields && - typeof node.parameters.additionalFields.queryParams === 'string' && - node.parameters.additionalFields.queryParams.startsWith('=') - ) { - acc.expressionsInQueryParams.push(toFlaggedNode({ node, workflow })); - } - }); - - return acc; - }, - { expressionsInQueries: [], expressionsInQueryParams: [], unusedQueryParams: [] }, - ); -} - -export function reportDatabaseRisk(workflows: Workflow[]) { - const { expressionsInQueries, expressionsInQueryParams, unusedQueryParams } = - getIssues(workflows); - - const issues = [expressionsInQueries, expressionsInQueryParams, unusedQueryParams]; - - if (issues.every((i) => i.length === 0)) return null; - - const report: Risk.StandardReport = { - risk: DATABASE_REPORT.RISK, - sections: [], - }; - - const sentenceStart = ({ length }: { length: number }) => - length > 1 ? 'These SQL nodes have' : 'This SQL node has'; - - if (expressionsInQueries.length > 0) { - report.sections.push({ - title: DATABASE_REPORT.SECTIONS.EXPRESSIONS_IN_QUERIES, - description: [ - sentenceStart(expressionsInQueries), - 'an expression in the "Query" field of an "Execute Query" operation. Building a SQL query with an expression may lead to a SQL injection attack.', - ].join(' '), - recommendation: - 'Consider using the "Query Parameters" field to pass parameters to the query, or validating the input of the expression in the "Query" field.', - location: expressionsInQueries, - }); - } - - if (expressionsInQueryParams.length > 0) { - report.sections.push({ - title: DATABASE_REPORT.SECTIONS.EXPRESSIONS_IN_QUERY_PARAMS, - description: [ - sentenceStart(expressionsInQueryParams), - 'an expression in the "Query Parameters" field of an "Execute Query" operation. Building a SQL query with an expression may lead to a SQL injection attack.', - ].join(' '), - recommendation: - 'Consider validating the input of the expression in the "Query Parameters" field.', - location: expressionsInQueryParams, - }); - } - - if (unusedQueryParams.length > 0) { - report.sections.push({ - title: DATABASE_REPORT.SECTIONS.UNUSED_QUERY_PARAMS, - description: [ - sentenceStart(unusedQueryParams), - 'no "Query Parameters" field in the "Execute Query" operation. Building a SQL query with unsanitized data may lead to a SQL injection attack.', - ].join(' '), - recommendation: `Consider using the "Query Parameters" field to sanitize parameters passed to the query. See: ${DB_QUERY_PARAMS_DOCS_URL}`, - location: unusedQueryParams, - }); - } - - return report; -} diff --git a/packages/cli/src/audit/risks/filesystem.risk.ts b/packages/cli/src/audit/risks/filesystem.risk.ts deleted file mode 100644 index 6e1033d5a6..0000000000 --- a/packages/cli/src/audit/risks/filesystem.risk.ts +++ /dev/null @@ -1,35 +0,0 @@ -import { getNodeTypes } from '@/audit/utils'; -import { FILESYSTEM_INTERACTION_NODE_TYPES, FILESYSTEM_REPORT } from '@/audit/constants'; -import type { WorkflowEntity } from '@db/entities/WorkflowEntity'; -import type { Risk } from '@/audit/types'; - -export function reportFilesystemRisk(workflows: WorkflowEntity[]) { - const fsInteractionNodeTypes = getNodeTypes(workflows, (node) => - FILESYSTEM_INTERACTION_NODE_TYPES.has(node.type), - ); - - if (fsInteractionNodeTypes.length === 0) return null; - - const report: Risk.StandardReport = { - risk: FILESYSTEM_REPORT.RISK, - sections: [], - }; - - const sentenceStart = ({ length }: { length: number }) => - length > 1 ? 'These nodes read from and write to' : 'This node reads from and writes to'; - - if (fsInteractionNodeTypes.length > 0) { - report.sections.push({ - title: FILESYSTEM_REPORT.SECTIONS.FILESYSTEM_INTERACTION_NODES, - description: [ - sentenceStart(fsInteractionNodeTypes), - 'any accessible file in the host filesystem. Sensitive file content may be manipulated through a node operation.', - ].join(' '), - recommendation: - 'Consider protecting any sensitive files in the host filesystem, or refactoring the workflow so that it does not require host filesystem interaction.', - location: fsInteractionNodeTypes, - }); - } - - return report; -} diff --git a/packages/cli/src/audit/risks/instance.risk.ts b/packages/cli/src/audit/risks/instance.risk.ts deleted file mode 100644 index 0005047676..0000000000 --- a/packages/cli/src/audit/risks/instance.risk.ts +++ /dev/null @@ -1,202 +0,0 @@ -import axios from 'axios'; -import { Container } from 'typedi'; -import { InstanceSettings } from 'n8n-core'; -import config from '@/config'; -import { toFlaggedNode } from '@/audit/utils'; -import { separate } from '@/utils'; -import { - ENV_VARS_DOCS_URL, - INSTANCE_REPORT, - WEBHOOK_NODE_TYPE, - WEBHOOK_VALIDATOR_NODE_TYPES, -} from '@/audit/constants'; -import { getN8nPackageJson, inDevelopment } from '@/constants'; -import type { WorkflowEntity } from '@db/entities/WorkflowEntity'; -import type { Risk, n8n } from '@/audit/types'; -import { isApiEnabled } from '@/PublicApi'; - -function getSecuritySettings() { - if (config.getEnv('deployment.type') === 'cloud') return null; - - const settings: Record = {}; - - settings.features = { - communityPackagesEnabled: config.getEnv('nodes.communityPackages.enabled'), - versionNotificationsEnabled: config.getEnv('versionNotifications.enabled'), - templatesEnabled: config.getEnv('templates.enabled'), - publicApiEnabled: isApiEnabled(), - }; - - settings.auth = { - authExcludeEndpoints: config.getEnv('security.excludeEndpoints') || 'none', - }; - - settings.nodes = { - nodesExclude: config.getEnv('nodes.exclude') ?? 'none', - nodesInclude: config.getEnv('nodes.include') ?? 'none', - }; - - settings.telemetry = { - diagnosticsEnabled: config.getEnv('diagnostics.enabled'), - }; - - return settings; -} - -/** - * Whether a webhook node has a direct child assumed to validate its payload. - */ -function hasValidatorChild({ - node, - workflow, -}: { - node: WorkflowEntity['nodes'][number]; - workflow: WorkflowEntity; -}) { - const childNodeNames = workflow.connections[node.name]?.main[0].map((i) => i.node); - - if (!childNodeNames) return false; - - return childNodeNames.some((name) => - workflow.nodes.find((n) => n.name === name && WEBHOOK_VALIDATOR_NODE_TYPES.has(n.type)), - ); -} - -function getUnprotectedWebhookNodes(workflows: WorkflowEntity[]) { - return workflows.reduce((acc, workflow) => { - if (!workflow.active) return acc; - - workflow.nodes.forEach((node) => { - if ( - node.type === WEBHOOK_NODE_TYPE && - node.parameters.authentication === undefined && - !hasValidatorChild({ node, workflow }) - ) { - acc.push(toFlaggedNode({ node, workflow })); - } - }); - - return acc; - }, []); -} - -async function getNextVersions(currentVersionName: string) { - const BASE_URL = config.getEnv('versionNotifications.endpoint'); - const { instanceId } = Container.get(InstanceSettings); - - const response = await axios.get(BASE_URL + currentVersionName, { - // eslint-disable-next-line @typescript-eslint/naming-convention - headers: { 'n8n-instance-id': instanceId }, - }); - - return response.data; -} - -function removeIconData(versions: n8n.Version[]) { - return versions.map((version) => { - if (version.nodes.length === 0) return version; - - version.nodes.forEach((node) => delete node.iconData); - - return version; - }); -} - -function classify(versions: n8n.Version[], currentVersionName: string) { - const [pass, fail] = separate(versions, (v) => v.name === currentVersionName); - - return { currentVersion: pass[0], nextVersions: fail }; -} - -export async function getOutdatedState() { - let versions = []; - - const localVersion = getN8nPackageJson().version; - - try { - versions = await getNextVersions(localVersion).then(removeIconData); - } catch (error) { - if (inDevelopment) { - console.error('Failed to fetch n8n versions. Skipping outdated instance report...'); - } - return null; - } - - const { currentVersion, nextVersions } = classify(versions, localVersion); - - const nextVersionsNumber = nextVersions.length; - - if (nextVersionsNumber === 0) return null; - - const description = [ - `This n8n instance is outdated. Currently at version ${ - currentVersion.name - }, missing ${nextVersionsNumber} ${nextVersionsNumber > 1 ? 'updates' : 'update'}.`, - ]; - - const upcomingSecurityUpdates = nextVersions.some((v) => v.hasSecurityIssue || v.hasSecurityFix); - - if (upcomingSecurityUpdates) description.push('Newer versions contain security updates.'); - - return { - description: description.join(' '), - nextVersions, - }; -} - -export async function reportInstanceRisk(workflows: WorkflowEntity[]) { - const unprotectedWebhooks = getUnprotectedWebhookNodes(workflows); - const outdatedState = await getOutdatedState(); - const securitySettings = getSecuritySettings(); - - if (unprotectedWebhooks.length === 0 && outdatedState === null && securitySettings === null) { - return null; - } - - const report: Risk.InstanceReport = { - risk: INSTANCE_REPORT.RISK, - sections: [], - }; - - if (unprotectedWebhooks.length > 0) { - const sentenceStart = ({ length }: { length: number }) => - length > 1 ? 'These webhook nodes have' : 'This webhook node has'; - - const recommendedValidators = [...WEBHOOK_VALIDATOR_NODE_TYPES] - .filter((nodeType) => !nodeType.endsWith('function') || !nodeType.endsWith('functionItem')) - .join(','); - - report.sections.push({ - title: INSTANCE_REPORT.SECTIONS.UNPROTECTED_WEBHOOKS, - description: [ - sentenceStart(unprotectedWebhooks), - `the "Authentication" field set to "None" and ${ - unprotectedWebhooks.length > 1 ? 'are' : 'is' - } not directly connected to a node to validate the payload. Every unprotected webhook allows your workflow to be called by any third party who knows the webhook URL.`, - ].join(' '), - recommendation: `Consider setting the "Authentication" field to an option other than "None", or validating the payload with one of the following nodes: ${recommendedValidators}.`, - location: unprotectedWebhooks, - }); - } - - if (outdatedState !== null) { - report.sections.push({ - title: INSTANCE_REPORT.SECTIONS.OUTDATED_INSTANCE, - description: outdatedState.description, - recommendation: - 'Consider updating this n8n instance to the latest version to prevent security vulnerabilities.', - nextVersions: outdatedState.nextVersions, - }); - } - - if (securitySettings !== null) { - report.sections.push({ - title: INSTANCE_REPORT.SECTIONS.SECURITY_SETTINGS, - description: 'This n8n instance has the following security settings.', - recommendation: `Consider adjusting the security settings for your n8n instance based on your needs. See: ${ENV_VARS_DOCS_URL}`, - settings: securitySettings, - }); - } - - return report; -} diff --git a/packages/cli/src/audit/risks/nodes.risk.ts b/packages/cli/src/audit/risks/nodes.risk.ts deleted file mode 100644 index dca1dcee15..0000000000 --- a/packages/cli/src/audit/risks/nodes.risk.ts +++ /dev/null @@ -1,116 +0,0 @@ -import * as path from 'path'; -import glob from 'fast-glob'; -import { Container } from 'typedi'; -import config from '@/config'; -import { LoadNodesAndCredentials } from '@/LoadNodesAndCredentials'; -import { getNodeTypes } from '@/audit/utils'; -import { - OFFICIAL_RISKY_NODE_TYPES, - ENV_VARS_DOCS_URL, - NODES_REPORT, - COMMUNITY_NODES_RISKS_URL, - NPM_PACKAGE_URL, -} from '@/audit/constants'; -import type { WorkflowEntity } from '@db/entities/WorkflowEntity'; -import type { Risk } from '@/audit/types'; - -async function getCommunityNodeDetails() { - if (!config.getEnv('nodes.communityPackages.enabled')) return []; - - const { CommunityPackagesService } = await import('@/services/communityPackages.service'); - const installedPackages = await Container.get(CommunityPackagesService).getAllInstalledPackages(); - - return installedPackages.reduce((acc, pkg) => { - pkg.installedNodes.forEach((node) => - acc.push({ - kind: 'community', - nodeType: node.type, - packageUrl: [NPM_PACKAGE_URL, pkg.packageName].join('/'), - }), - ); - - return acc; - }, []); -} - -async function getCustomNodeDetails() { - const customNodeTypes: Risk.CustomNodeDetails[] = []; - - const nodesAndCredentials = Container.get(LoadNodesAndCredentials); - for (const customDir of nodesAndCredentials.getCustomDirectories()) { - const customNodeFiles = await glob('**/*.node.js', { cwd: customDir, absolute: true }); - - for (const nodeFile of customNodeFiles) { - const [fileName] = path.parse(nodeFile).name.split('.'); - customNodeTypes.push({ - kind: 'custom', - nodeType: ['CUSTOM', fileName].join('.'), - filePath: nodeFile, - }); - } - } - - return customNodeTypes; -} - -export async function reportNodesRisk(workflows: WorkflowEntity[]) { - const officialRiskyNodes = getNodeTypes(workflows, (node) => - OFFICIAL_RISKY_NODE_TYPES.has(node.type), - ); - - const [communityNodes, customNodes] = await Promise.all([ - getCommunityNodeDetails(), - getCustomNodeDetails(), - ]); - - const issues = [officialRiskyNodes, communityNodes, customNodes]; - - if (issues.every((i) => i.length === 0)) return null; - - const report: Risk.StandardReport = { - risk: NODES_REPORT.RISK, - sections: [], - }; - - const sentenceStart = (length: number) => (length > 1 ? 'These nodes are' : 'This node is'); - - if (officialRiskyNodes.length > 0) { - report.sections.push({ - title: NODES_REPORT.SECTIONS.OFFICIAL_RISKY_NODES, - description: [ - sentenceStart(officialRiskyNodes.length), - "part of n8n's official nodes and may be used to fetch and run any arbitrary code in the host system. This may lead to exploits such as remote code execution.", - ].join(' '), - recommendation: `Consider reviewing the parameters in these nodes, replacing them with app nodes where possible, and not loading unneeded node types with the NODES_EXCLUDE environment variable. See: ${ENV_VARS_DOCS_URL}`, - location: officialRiskyNodes, - }); - } - - if (communityNodes.length > 0) { - report.sections.push({ - title: NODES_REPORT.SECTIONS.COMMUNITY_NODES, - description: [ - sentenceStart(communityNodes.length), - `sourced from the n8n community. Community nodes are not vetted by the n8n team and have full access to the host system. See: ${COMMUNITY_NODES_RISKS_URL}`, - ].join(' '), - recommendation: - 'Consider reviewing the source code in any community nodes installed in this n8n instance, and uninstalling any community nodes no longer in use.', - location: communityNodes, - }); - } - - if (customNodes.length > 0) { - report.sections.push({ - title: NODES_REPORT.SECTIONS.CUSTOM_NODES, - description: [ - sentenceStart(communityNodes.length), - 'unpublished and located in the host system. Custom nodes are not vetted by the n8n team and have full access to the host system.', - ].join(' '), - recommendation: - 'Consider reviewing the source code in any custom node installed in this n8n instance, and removing any custom nodes no longer in use.', - location: customNodes, - }); - } - - return report; -} diff --git a/packages/cli/src/commands/audit.ts b/packages/cli/src/commands/audit.ts index 0c1555ccdf..082aab3a1b 100644 --- a/packages/cli/src/commands/audit.ts +++ b/packages/cli/src/commands/audit.ts @@ -1,8 +1,8 @@ import { flags } from '@oclif/command'; -import { audit } from '@/audit'; -import { RISK_CATEGORIES } from '@/audit/constants'; +import { SecurityAuditService } from '@/security-audit/SecurityAudit.service'; +import { RISK_CATEGORIES } from '@/security-audit/constants'; import config from '@/config'; -import type { Risk } from '@/audit/types'; +import type { Risk } from '@/security-audit/types'; import { BaseCommand } from './BaseCommand'; import { Container } from 'typedi'; import { InternalHooks } from '@/InternalHooks'; @@ -49,7 +49,10 @@ export class SecurityAudit extends BaseCommand { throw new Error([message, hint].join('. ')); } - const result = await audit(categories, auditFlags['days-abandoned-workflow']); + const result = await Container.get(SecurityAuditService).run( + categories, + auditFlags['days-abandoned-workflow'], + ); if (Array.isArray(result) && result.length === 0) { this.logger.info('No security issues found'); diff --git a/packages/cli/src/security-audit/SecurityAudit.service.ts b/packages/cli/src/security-audit/SecurityAudit.service.ts new file mode 100644 index 0000000000..469ea60332 --- /dev/null +++ b/packages/cli/src/security-audit/SecurityAudit.service.ts @@ -0,0 +1,63 @@ +import Container, { Service } from 'typedi'; + +import config from '@/config'; +import { WorkflowRepository } from '@/databases/repositories/workflow.repository'; + +import { RISK_CATEGORIES } from '@/security-audit/constants'; +import { toReportTitle } from '@/security-audit/utils'; +import type { Risk, RiskReporter } from '@/security-audit/types'; + +@Service() +export class SecurityAuditService { + constructor(private readonly workflowRepository: WorkflowRepository) {} + + private reporters: { + [name: string]: RiskReporter; + } = {}; + + async run(categories: Risk.Category[] = RISK_CATEGORIES, daysAbandonedWorkflow?: number) { + if (categories.length === 0) categories = RISK_CATEGORIES; + + await this.initReporters(categories); + + const daysFromEnv = config.getEnv('security.audit.daysAbandonedWorkflow'); + + if (daysAbandonedWorkflow) { + config.set('security.audit.daysAbandonedWorkflow', daysAbandonedWorkflow); + } + + const workflows = await this.workflowRepository.find({ + select: ['id', 'name', 'active', 'nodes', 'connections'], + }); + + const promises = categories.map(async (c) => this.reporters[c].report(workflows)); + + const reports = (await Promise.all(promises)).filter((r): r is Risk.Report => r !== null); + + if (daysAbandonedWorkflow) { + config.set('security.audit.daysAbandonedWorkflow', daysFromEnv); // restore env + } + + if (reports.length === 0) return []; // trigger empty state + + return reports.reduce((acc, cur) => { + acc[toReportTitle(cur.risk)] = cur; + + return acc; + }, {}); + } + + async initReporters(categories: Risk.Category[]) { + for (const category of categories) { + const className = category.charAt(0).toUpperCase() + category.slice(1) + 'RiskReporter'; + + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment + const RiskReporterModule = await import(`@/security-audit/risk-reporters/${className}`); + + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + const RiskReporterClass = RiskReporterModule[className] as { new (): RiskReporter }; + + this.reporters[category] = Container.get(RiskReporterClass); + } + } +} diff --git a/packages/cli/src/audit/constants.ts b/packages/cli/src/security-audit/constants.ts similarity index 98% rename from packages/cli/src/audit/constants.ts rename to packages/cli/src/security-audit/constants.ts index 9c2658158d..a47b39986c 100644 --- a/packages/cli/src/audit/constants.ts +++ b/packages/cli/src/security-audit/constants.ts @@ -1,4 +1,4 @@ -import type { Risk } from '@/audit/types'; +import type { Risk } from '@/security-audit/types'; /** * Risk categories diff --git a/packages/cli/src/security-audit/risk-reporters/CredentialsRiskReporter.ts b/packages/cli/src/security-audit/risk-reporters/CredentialsRiskReporter.ts new file mode 100644 index 0000000000..64adce7c88 --- /dev/null +++ b/packages/cli/src/security-audit/risk-reporters/CredentialsRiskReporter.ts @@ -0,0 +1,159 @@ +import { In, MoreThanOrEqual } from 'typeorm'; +import { DateUtils } from 'typeorm/util/DateUtils'; +import { Service } from 'typedi'; +import type { IWorkflowBase } from 'n8n-workflow'; +import config from '@/config'; +import { CREDENTIALS_REPORT } from '@/security-audit/constants'; +import type { RiskReporter, Risk } from '@/security-audit/types'; +import type { WorkflowEntity } from '@db/entities/WorkflowEntity'; +import { CredentialsRepository } from '@db/repositories/credentials.repository'; +import { ExecutionRepository } from '@db/repositories/execution.repository'; +import { ExecutionDataRepository } from '@db/repositories/executionData.repository'; + +@Service() +export class CredentialsRiskReporter implements RiskReporter { + constructor( + private readonly credentialsRepository: CredentialsRepository, + private readonly executionRepository: ExecutionRepository, + private readonly executionDataRepository: ExecutionDataRepository, + ) {} + + async report(workflows: WorkflowEntity[]) { + const days = config.getEnv('security.audit.daysAbandonedWorkflow'); + + const allExistingCreds = await this.getAllExistingCreds(); + const { credsInAnyUse, credsInActiveUse } = await this.getAllCredsInUse(workflows); + const recentlyExecutedCreds = await this.getCredsInRecentlyExecutedWorkflows(days); + + const credsNotInAnyUse = allExistingCreds.filter((c) => !credsInAnyUse.has(c.id)); + const credsNotInActiveUse = allExistingCreds.filter((c) => !credsInActiveUse.has(c.id)); + const credsNotRecentlyExecuted = allExistingCreds.filter( + (c) => !recentlyExecutedCreds.has(c.id), + ); + + const issues = [credsNotInAnyUse, credsNotInActiveUse, credsNotRecentlyExecuted]; + + if (issues.every((i) => i.length === 0)) return null; + + const report: Risk.StandardReport = { + risk: CREDENTIALS_REPORT.RISK, + sections: [], + }; + + const hint = 'Keeping unused credentials in your instance is an unneeded security risk.'; + const recommendation = 'Consider deleting these credentials if you no longer need them.'; + + const sentenceStart = ({ length }: { length: number }) => + length > 1 ? 'These credentials are' : 'This credential is'; + + if (credsNotInAnyUse.length > 0) { + report.sections.push({ + title: CREDENTIALS_REPORT.SECTIONS.CREDS_NOT_IN_ANY_USE, + description: [sentenceStart(credsNotInAnyUse), 'not used in any workflow.', hint].join(' '), + recommendation, + location: credsNotInAnyUse, + }); + } + + if (credsNotInActiveUse.length > 0) { + report.sections.push({ + title: CREDENTIALS_REPORT.SECTIONS.CREDS_NOT_IN_ACTIVE_USE, + description: [ + sentenceStart(credsNotInActiveUse), + 'not used in active workflows.', + hint, + ].join(' '), + recommendation, + location: credsNotInActiveUse, + }); + } + + if (credsNotRecentlyExecuted.length > 0) { + report.sections.push({ + title: CREDENTIALS_REPORT.SECTIONS.CREDS_NOT_RECENTLY_EXECUTED, + description: [ + sentenceStart(credsNotRecentlyExecuted), + `not used in recently executed workflows, i.e. workflows executed in the past ${days} days.`, + hint, + ].join(' '), + recommendation, + location: credsNotRecentlyExecuted, + }); + } + + return report; + } + + private async getAllCredsInUse(workflows: WorkflowEntity[]) { + const credsInAnyUse = new Set(); + const credsInActiveUse = new Set(); + + workflows.forEach((workflow) => { + workflow.nodes.forEach((node) => { + if (!node.credentials) return; + + Object.values(node.credentials).forEach((cred) => { + if (!cred?.id) return; + + credsInAnyUse.add(cred.id); + + if (workflow.active) credsInActiveUse.add(cred.id); + }); + }); + }); + + return { + credsInAnyUse, + credsInActiveUse, + }; + } + + private async getAllExistingCreds() { + const credentials = await this.credentialsRepository.find({ select: ['id', 'name'] }); + + return credentials.map(({ id, name }) => ({ kind: 'credential' as const, id, name })); + } + + private async getExecutedWorkflowsInPastDays(days: number): Promise { + const date = new Date(); + + date.setDate(date.getDate() - days); + + const executionIds = await this.executionRepository + .find({ + select: ['id'], + where: { + startedAt: MoreThanOrEqual(DateUtils.mixedDateToUtcDatetimeString(date) as Date), + }, + }) + .then((executions) => executions.map(({ id }) => id)); + + return this.executionDataRepository + .find({ + select: ['workflowData'], + where: { + executionId: In(executionIds), + }, + }) + .then((executionData) => executionData.map(({ workflowData }) => workflowData)); + } + + /** + * Return IDs of credentials in workflows executed in the past n days. + */ + private async getCredsInRecentlyExecutedWorkflows(days: number) { + const executedWorkflows = await this.getExecutedWorkflowsInPastDays(days); + + return executedWorkflows.reduce>((acc, { nodes }) => { + nodes.forEach((node) => { + if (node.credentials) { + Object.values(node.credentials).forEach((c) => { + if (c.id) acc.add(c.id); + }); + } + }); + + return acc; + }, new Set()); + } +} diff --git a/packages/cli/src/security-audit/risk-reporters/DatabaseRiskReporter.ts b/packages/cli/src/security-audit/risk-reporters/DatabaseRiskReporter.ts new file mode 100644 index 0000000000..2632917f8f --- /dev/null +++ b/packages/cli/src/security-audit/risk-reporters/DatabaseRiskReporter.ts @@ -0,0 +1,110 @@ +import { toFlaggedNode } from '@/security-audit/utils'; +import { + SQL_NODE_TYPES, + DATABASE_REPORT, + DB_QUERY_PARAMS_DOCS_URL, + SQL_NODE_TYPES_WITH_QUERY_PARAMS, +} from '@/security-audit/constants'; +import type { WorkflowEntity as Workflow } from '@db/entities/WorkflowEntity'; +import type { RiskReporter, Risk } from '@/security-audit/types'; +import { Service } from 'typedi'; + +@Service() +export class DatabaseRiskReporter implements RiskReporter { + async report(workflows: Workflow[]) { + const { expressionsInQueries, expressionsInQueryParams, unusedQueryParams } = + this.getIssues(workflows); + + const issues = [expressionsInQueries, expressionsInQueryParams, unusedQueryParams]; + + if (issues.every((i) => i.length === 0)) return null; + + const report: Risk.StandardReport = { + risk: DATABASE_REPORT.RISK, + sections: [], + }; + + const sentenceStart = ({ length }: { length: number }) => + length > 1 ? 'These SQL nodes have' : 'This SQL node has'; + + if (expressionsInQueries.length > 0) { + report.sections.push({ + title: DATABASE_REPORT.SECTIONS.EXPRESSIONS_IN_QUERIES, + description: [ + sentenceStart(expressionsInQueries), + 'an expression in the "Query" field of an "Execute Query" operation. Building a SQL query with an expression may lead to a SQL injection attack.', + ].join(' '), + recommendation: + 'Consider using the "Query Parameters" field to pass parameters to the query, or validating the input of the expression in the "Query" field.', + location: expressionsInQueries, + }); + } + + if (expressionsInQueryParams.length > 0) { + report.sections.push({ + title: DATABASE_REPORT.SECTIONS.EXPRESSIONS_IN_QUERY_PARAMS, + description: [ + sentenceStart(expressionsInQueryParams), + 'an expression in the "Query Parameters" field of an "Execute Query" operation. Building a SQL query with an expression may lead to a SQL injection attack.', + ].join(' '), + recommendation: + 'Consider validating the input of the expression in the "Query Parameters" field.', + location: expressionsInQueryParams, + }); + } + + if (unusedQueryParams.length > 0) { + report.sections.push({ + title: DATABASE_REPORT.SECTIONS.UNUSED_QUERY_PARAMS, + description: [ + sentenceStart(unusedQueryParams), + 'no "Query Parameters" field in the "Execute Query" operation. Building a SQL query with unsanitized data may lead to a SQL injection attack.', + ].join(' '), + recommendation: `Consider using the "Query Parameters" field to sanitize parameters passed to the query. See: ${DB_QUERY_PARAMS_DOCS_URL}`, + location: unusedQueryParams, + }); + } + + return report; + } + + private getIssues(workflows: Workflow[]) { + return workflows.reduce<{ [sectionTitle: string]: Risk.NodeLocation[] }>( + (acc, workflow) => { + workflow.nodes.forEach((node) => { + if (!SQL_NODE_TYPES.has(node.type)) return; + if (node.parameters === undefined) return; + if (node.parameters.operation !== 'executeQuery') return; + + if (typeof node.parameters.query === 'string' && node.parameters.query.startsWith('=')) { + acc.expressionsInQueries.push(toFlaggedNode({ node, workflow })); + } + + if (!SQL_NODE_TYPES_WITH_QUERY_PARAMS.has(node.type)) return; + + if (!node.parameters.additionalFields) { + acc.unusedQueryParams.push(toFlaggedNode({ node, workflow })); + } + + if (typeof node.parameters.additionalFields !== 'object') return; + if (node.parameters.additionalFields === null) return; + + if (!('queryParams' in node.parameters.additionalFields)) { + acc.unusedQueryParams.push(toFlaggedNode({ node, workflow })); + } + + if ( + 'queryParams' in node.parameters.additionalFields && + typeof node.parameters.additionalFields.queryParams === 'string' && + node.parameters.additionalFields.queryParams.startsWith('=') + ) { + acc.expressionsInQueryParams.push(toFlaggedNode({ node, workflow })); + } + }); + + return acc; + }, + { expressionsInQueries: [], expressionsInQueryParams: [], unusedQueryParams: [] }, + ); + } +} diff --git a/packages/cli/src/security-audit/risk-reporters/FilesystemRiskReporter.ts b/packages/cli/src/security-audit/risk-reporters/FilesystemRiskReporter.ts new file mode 100644 index 0000000000..f2cb5eb092 --- /dev/null +++ b/packages/cli/src/security-audit/risk-reporters/FilesystemRiskReporter.ts @@ -0,0 +1,39 @@ +import { getNodeTypes } from '@/security-audit/utils'; +import { FILESYSTEM_INTERACTION_NODE_TYPES, FILESYSTEM_REPORT } from '@/security-audit/constants'; +import type { WorkflowEntity } from '@db/entities/WorkflowEntity'; +import type { RiskReporter, Risk } from '@/security-audit/types'; +import { Service } from 'typedi'; + +@Service() +export class FilesystemRiskReporter implements RiskReporter { + async report(workflows: WorkflowEntity[]) { + const fsInteractionNodeTypes = getNodeTypes(workflows, (node) => + FILESYSTEM_INTERACTION_NODE_TYPES.has(node.type), + ); + + if (fsInteractionNodeTypes.length === 0) return null; + + const report: Risk.StandardReport = { + risk: FILESYSTEM_REPORT.RISK, + sections: [], + }; + + const sentenceStart = ({ length }: { length: number }) => + length > 1 ? 'These nodes read from and write to' : 'This node reads from and writes to'; + + if (fsInteractionNodeTypes.length > 0) { + report.sections.push({ + title: FILESYSTEM_REPORT.SECTIONS.FILESYSTEM_INTERACTION_NODES, + description: [ + sentenceStart(fsInteractionNodeTypes), + 'any accessible file in the host filesystem. Sensitive file content may be manipulated through a node operation.', + ].join(' '), + recommendation: + 'Consider protecting any sensitive files in the host filesystem, or refactoring the workflow so that it does not require host filesystem interaction.', + location: fsInteractionNodeTypes, + }); + } + + return report; + } +} diff --git a/packages/cli/src/security-audit/risk-reporters/InstanceRiskReporter.ts b/packages/cli/src/security-audit/risk-reporters/InstanceRiskReporter.ts new file mode 100644 index 0000000000..55aec055da --- /dev/null +++ b/packages/cli/src/security-audit/risk-reporters/InstanceRiskReporter.ts @@ -0,0 +1,209 @@ +import axios from 'axios'; +import { Service } from 'typedi'; +import { InstanceSettings } from 'n8n-core'; +import config from '@/config'; +import { toFlaggedNode } from '@/security-audit/utils'; +import { separate } from '@/utils'; +import { + ENV_VARS_DOCS_URL, + INSTANCE_REPORT, + WEBHOOK_NODE_TYPE, + WEBHOOK_VALIDATOR_NODE_TYPES, +} from '@/security-audit/constants'; +import { getN8nPackageJson, inDevelopment } from '@/constants'; +import type { WorkflowEntity } from '@db/entities/WorkflowEntity'; +import type { RiskReporter, Risk, n8n } from '@/security-audit/types'; +import { isApiEnabled } from '@/PublicApi'; + +@Service() +export class InstanceRiskReporter implements RiskReporter { + constructor(private readonly instanceSettings: InstanceSettings) {} + + async report(workflows: WorkflowEntity[]) { + const unprotectedWebhooks = this.getUnprotectedWebhookNodes(workflows); + const outdatedState = await this.getOutdatedState(); + const securitySettings = this.getSecuritySettings(); + + if (unprotectedWebhooks.length === 0 && outdatedState === null && securitySettings === null) { + return null; + } + + const report: Risk.Report = { + risk: INSTANCE_REPORT.RISK, + sections: [], + }; + + if (unprotectedWebhooks.length > 0) { + const sentenceStart = ({ length }: { length: number }) => + length > 1 ? 'These webhook nodes have' : 'This webhook node has'; + + const recommendedValidators = [...WEBHOOK_VALIDATOR_NODE_TYPES] + .filter((nodeType) => !nodeType.endsWith('function') || !nodeType.endsWith('functionItem')) + .join(','); + + report.sections.push({ + title: INSTANCE_REPORT.SECTIONS.UNPROTECTED_WEBHOOKS, + description: [ + sentenceStart(unprotectedWebhooks), + `the "Authentication" field set to "None" and ${ + unprotectedWebhooks.length > 1 ? 'are' : 'is' + } not directly connected to a node to validate the payload. Every unprotected webhook allows your workflow to be called by any third party who knows the webhook URL.`, + ].join(' '), + recommendation: `Consider setting the "Authentication" field to an option other than "None", or validating the payload with one of the following nodes: ${recommendedValidators}.`, + location: unprotectedWebhooks, + }); + } + + if (outdatedState !== null) { + report.sections.push({ + title: INSTANCE_REPORT.SECTIONS.OUTDATED_INSTANCE, + description: outdatedState.description, + recommendation: + 'Consider updating this n8n instance to the latest version to prevent security vulnerabilities.', + nextVersions: outdatedState.nextVersions, + }); + } + + if (securitySettings !== null) { + report.sections.push({ + title: INSTANCE_REPORT.SECTIONS.SECURITY_SETTINGS, + description: 'This n8n instance has the following security settings.', + recommendation: `Consider adjusting the security settings for your n8n instance based on your needs. See: ${ENV_VARS_DOCS_URL}`, + settings: securitySettings, + }); + } + + return report; + } + + private getSecuritySettings() { + if (config.getEnv('deployment.type') === 'cloud') return null; + + const settings: Record = {}; + + settings.features = { + communityPackagesEnabled: config.getEnv('nodes.communityPackages.enabled'), + versionNotificationsEnabled: config.getEnv('versionNotifications.enabled'), + templatesEnabled: config.getEnv('templates.enabled'), + publicApiEnabled: isApiEnabled(), + }; + + settings.auth = { + authExcludeEndpoints: config.getEnv('security.excludeEndpoints') || 'none', + }; + + settings.nodes = { + nodesExclude: config.getEnv('nodes.exclude') ?? 'none', + nodesInclude: config.getEnv('nodes.include') ?? 'none', + }; + + settings.telemetry = { + diagnosticsEnabled: config.getEnv('diagnostics.enabled'), + }; + + return settings; + } + + /** + * Whether a webhook node has a direct child assumed to validate its payload. + */ + private hasValidatorChild({ + node, + workflow, + }: { + node: WorkflowEntity['nodes'][number]; + workflow: WorkflowEntity; + }) { + const childNodeNames = workflow.connections[node.name]?.main[0].map((i) => i.node); + + if (!childNodeNames) return false; + + return childNodeNames.some((name) => + workflow.nodes.find((n) => n.name === name && WEBHOOK_VALIDATOR_NODE_TYPES.has(n.type)), + ); + } + + private getUnprotectedWebhookNodes(workflows: WorkflowEntity[]) { + return workflows.reduce((acc, workflow) => { + if (!workflow.active) return acc; + + workflow.nodes.forEach((node) => { + if ( + node.type === WEBHOOK_NODE_TYPE && + node.parameters.authentication === undefined && + !this.hasValidatorChild({ node, workflow }) + ) { + acc.push(toFlaggedNode({ node, workflow })); + } + }); + + return acc; + }, []); + } + + private async getNextVersions(currentVersionName: string) { + const BASE_URL = config.getEnv('versionNotifications.endpoint'); + const { instanceId } = this.instanceSettings; + + const response = await axios.get(BASE_URL + currentVersionName, { + // eslint-disable-next-line @typescript-eslint/naming-convention + headers: { 'n8n-instance-id': instanceId }, + }); + + return response.data; + } + + private removeIconData(versions: n8n.Version[]) { + return versions.map((version) => { + if (version.nodes.length === 0) return version; + + version.nodes.forEach((node) => delete node.iconData); + + return version; + }); + } + + private classify(versions: n8n.Version[], currentVersionName: string) { + const [pass, fail] = separate(versions, (v) => v.name === currentVersionName); + + return { currentVersion: pass[0], nextVersions: fail }; + } + + private async getOutdatedState() { + let versions = []; + + const localVersion = getN8nPackageJson().version; + + try { + versions = await this.getNextVersions(localVersion).then((v) => this.removeIconData(v)); + } catch (error) { + if (inDevelopment) { + console.error('Failed to fetch n8n versions. Skipping outdated instance report...'); + } + return null; + } + + const { currentVersion, nextVersions } = this.classify(versions, localVersion); + + const nextVersionsNumber = nextVersions.length; + + if (nextVersionsNumber === 0) return null; + + const description = [ + `This n8n instance is outdated. Currently at version ${ + currentVersion.name + }, missing ${nextVersionsNumber} ${nextVersionsNumber > 1 ? 'updates' : 'update'}.`, + ]; + + const upcomingSecurityUpdates = nextVersions.some( + (v) => v.hasSecurityIssue || v.hasSecurityFix, + ); + + if (upcomingSecurityUpdates) description.push('Newer versions contain security updates.'); + + return { + description: description.join(' '), + nextVersions, + }; + } +} diff --git a/packages/cli/src/security-audit/risk-reporters/NodesRiskReporter.ts b/packages/cli/src/security-audit/risk-reporters/NodesRiskReporter.ts new file mode 100644 index 0000000000..f98afb8a3d --- /dev/null +++ b/packages/cli/src/security-audit/risk-reporters/NodesRiskReporter.ts @@ -0,0 +1,123 @@ +import * as path from 'path'; +import glob from 'fast-glob'; +import { Service } from 'typedi'; +import config from '@/config'; +import { LoadNodesAndCredentials } from '@/LoadNodesAndCredentials'; +import { getNodeTypes } from '@/security-audit/utils'; +import { + OFFICIAL_RISKY_NODE_TYPES, + ENV_VARS_DOCS_URL, + NODES_REPORT, + COMMUNITY_NODES_RISKS_URL, + NPM_PACKAGE_URL, +} from '@/security-audit/constants'; +import type { WorkflowEntity } from '@db/entities/WorkflowEntity'; +import type { Risk, RiskReporter } from '@/security-audit/types'; +import { CommunityPackagesService } from '@/services/communityPackages.service'; + +@Service() +export class NodesRiskReporter implements RiskReporter { + constructor( + private readonly loadNodesAndCredentials: LoadNodesAndCredentials, + private readonly communityPackagesService: CommunityPackagesService, + ) {} + + async report(workflows: WorkflowEntity[]) { + const officialRiskyNodes = getNodeTypes(workflows, (node) => + OFFICIAL_RISKY_NODE_TYPES.has(node.type), + ); + + const [communityNodes, customNodes] = await Promise.all([ + this.getCommunityNodeDetails(), + this.getCustomNodeDetails(), + ]); + + const issues = [officialRiskyNodes, communityNodes, customNodes]; + + if (issues.every((i) => i.length === 0)) return null; + + const report: Risk.StandardReport = { + risk: NODES_REPORT.RISK, + sections: [], + }; + + const sentenceStart = (length: number) => (length > 1 ? 'These nodes are' : 'This node is'); + + if (officialRiskyNodes.length > 0) { + report.sections.push({ + title: NODES_REPORT.SECTIONS.OFFICIAL_RISKY_NODES, + description: [ + sentenceStart(officialRiskyNodes.length), + "part of n8n's official nodes and may be used to fetch and run any arbitrary code in the host system. This may lead to exploits such as remote code execution.", + ].join(' '), + recommendation: `Consider reviewing the parameters in these nodes, replacing them with app nodes where possible, and not loading unneeded node types with the NODES_EXCLUDE environment variable. See: ${ENV_VARS_DOCS_URL}`, + location: officialRiskyNodes, + }); + } + + if (communityNodes.length > 0) { + report.sections.push({ + title: NODES_REPORT.SECTIONS.COMMUNITY_NODES, + description: [ + sentenceStart(communityNodes.length), + `sourced from the n8n community. Community nodes are not vetted by the n8n team and have full access to the host system. See: ${COMMUNITY_NODES_RISKS_URL}`, + ].join(' '), + recommendation: + 'Consider reviewing the source code in any community nodes installed in this n8n instance, and uninstalling any community nodes no longer in use.', + location: communityNodes, + }); + } + + if (customNodes.length > 0) { + report.sections.push({ + title: NODES_REPORT.SECTIONS.CUSTOM_NODES, + description: [ + sentenceStart(communityNodes.length), + 'unpublished and located in the host system. Custom nodes are not vetted by the n8n team and have full access to the host system.', + ].join(' '), + recommendation: + 'Consider reviewing the source code in any custom node installed in this n8n instance, and removing any custom nodes no longer in use.', + location: customNodes, + }); + } + + return report; + } + + private async getCommunityNodeDetails() { + if (!config.getEnv('nodes.communityPackages.enabled')) return []; + + const installedPackages = await this.communityPackagesService.getAllInstalledPackages(); + + return installedPackages.reduce((acc, pkg) => { + pkg.installedNodes.forEach((node) => + acc.push({ + kind: 'community', + nodeType: node.type, + packageUrl: [NPM_PACKAGE_URL, pkg.packageName].join('/'), + }), + ); + + return acc; + }, []); + } + + private async getCustomNodeDetails() { + const customNodeTypes: Risk.CustomNodeDetails[] = []; + + for (const customDir of this.loadNodesAndCredentials.getCustomDirectories()) { + const customNodeFiles = await glob('**/*.node.js', { cwd: customDir, absolute: true }); + + for (const nodeFile of customNodeFiles) { + const [fileName] = path.parse(nodeFile).name.split('.'); + customNodeTypes.push({ + kind: 'custom', + nodeType: ['CUSTOM', fileName].join('.'), + filePath: nodeFile, + }); + } + } + + return customNodeTypes; + } +} diff --git a/packages/cli/src/audit/types.ts b/packages/cli/src/security-audit/types.ts similarity index 95% rename from packages/cli/src/audit/types.ts rename to packages/cli/src/security-audit/types.ts index aa88f21aa5..131eb0616d 100644 --- a/packages/cli/src/audit/types.ts +++ b/packages/cli/src/security-audit/types.ts @@ -84,3 +84,7 @@ export namespace n8n { securityIssueFixVersion: string; }; } + +export interface RiskReporter { + report(workflows: Workflow[]): Promise; +} diff --git a/packages/cli/src/audit/utils.ts b/packages/cli/src/security-audit/utils.ts similarity index 93% rename from packages/cli/src/audit/utils.ts rename to packages/cli/src/security-audit/utils.ts index 9a0bf208e6..14dd1ff9d1 100644 --- a/packages/cli/src/audit/utils.ts +++ b/packages/cli/src/security-audit/utils.ts @@ -1,5 +1,5 @@ import type { WorkflowEntity as Workflow } from '@db/entities/WorkflowEntity'; -import type { Risk } from '@/audit/types'; +import type { Risk } from '@/security-audit/types'; type Node = Workflow['nodes'][number]; diff --git a/packages/cli/test/integration/audit/credentials.risk.test.ts b/packages/cli/test/integration/security-audit/CredentialsRiskReporter.test.ts similarity index 91% rename from packages/cli/test/integration/audit/credentials.risk.test.ts rename to packages/cli/test/integration/security-audit/CredentialsRiskReporter.test.ts index 104ac00476..9a10e6e70c 100644 --- a/packages/cli/test/integration/audit/credentials.risk.test.ts +++ b/packages/cli/test/integration/security-audit/CredentialsRiskReporter.test.ts @@ -1,7 +1,7 @@ import { v4 as uuid } from 'uuid'; import config from '@/config'; -import { audit } from '@/audit'; -import { CREDENTIALS_REPORT } from '@/audit/constants'; +import { SecurityAuditService } from '@/security-audit/SecurityAudit.service'; +import { CREDENTIALS_REPORT } from '@/security-audit/constants'; import { getRiskSection } from './utils'; import * as testDb from '../shared/testDb'; import { generateNanoId } from '@db/utils/generators'; @@ -11,8 +11,12 @@ import { CredentialsRepository } from '@db/repositories/credentials.repository'; import { ExecutionRepository } from '@db/repositories/execution.repository'; import { ExecutionDataRepository } from '@db/repositories/executionData.repository'; +let securityAuditService: SecurityAuditService; + beforeAll(async () => { await testDb.init(); + + securityAuditService = new SecurityAuditService(Container.get(WorkflowRepository)); }); beforeEach(async () => { @@ -54,7 +58,7 @@ test('should report credentials not in any use', async () => { Container.get(WorkflowRepository).save(workflowDetails), ]); - const testAudit = await audit(['credentials']); + const testAudit = await securityAuditService.run(['credentials']); const section = getRiskSection( testAudit, @@ -99,7 +103,7 @@ test('should report credentials not in active use', async () => { await Container.get(WorkflowRepository).save(workflowDetails); - const testAudit = await audit(['credentials']); + const testAudit = await securityAuditService.run(['credentials']); const section = getRiskSection( testAudit, @@ -167,7 +171,7 @@ test('should report credential in not recently executed workflow', async () => { workflowData: workflow, }); - const testAudit = await audit(['credentials']); + const testAudit = await securityAuditService.run(['credentials']); const section = getRiskSection( testAudit, @@ -236,7 +240,7 @@ test('should not report credentials in recently executed workflow', async () => workflowData: workflow, }); - const testAudit = await audit(['credentials']); + const testAudit = await securityAuditService.run(['credentials']); expect(testAudit).toBeEmptyArray(); }); diff --git a/packages/cli/test/integration/audit/database.risk.test.ts b/packages/cli/test/integration/security-audit/DatabaseRiskReporter.test.ts similarity index 89% rename from packages/cli/test/integration/audit/database.risk.test.ts rename to packages/cli/test/integration/security-audit/DatabaseRiskReporter.test.ts index 7a496edc4b..63c50caad9 100644 --- a/packages/cli/test/integration/audit/database.risk.test.ts +++ b/packages/cli/test/integration/security-audit/DatabaseRiskReporter.test.ts @@ -1,18 +1,22 @@ import { v4 as uuid } from 'uuid'; -import { audit } from '@/audit'; +import { SecurityAuditService } from '@/security-audit/SecurityAudit.service'; import { DATABASE_REPORT, SQL_NODE_TYPES, SQL_NODE_TYPES_WITH_QUERY_PARAMS, -} from '@/audit/constants'; +} from '@/security-audit/constants'; import { getRiskSection, saveManualTriggerWorkflow } from './utils'; import * as testDb from '../shared/testDb'; import { generateNanoId } from '@db/utils/generators'; import { WorkflowRepository } from '@db/repositories/workflow.repository'; import Container from 'typedi'; +let securityAuditService: SecurityAuditService; + beforeAll(async () => { await testDb.init(); + + securityAuditService = new SecurityAuditService(Container.get(WorkflowRepository)); }); beforeEach(async () => { @@ -56,7 +60,7 @@ test('should report expressions in queries', async () => { await Promise.all(promises); - const testAudit = await audit(['database']); + const testAudit = await securityAuditService.run(['database']); const section = getRiskSection( testAudit, @@ -111,7 +115,7 @@ test('should report expressions in query params', async () => { await Promise.all(promises); - const testAudit = await audit(['database']); + const testAudit = await securityAuditService.run(['database']); const section = getRiskSection( testAudit, @@ -163,7 +167,7 @@ test('should report unused query params', async () => { await Promise.all(promises); - const testAudit = await audit(['database']); + const testAudit = await securityAuditService.run(['database']); const section = getRiskSection( testAudit, @@ -183,7 +187,7 @@ test('should report unused query params', async () => { test('should not report non-database node', async () => { await saveManualTriggerWorkflow(); - const testAudit = await audit(['database']); + const testAudit = await securityAuditService.run(['database']); expect(testAudit).toBeEmptyArray(); }); diff --git a/packages/cli/test/integration/audit/filesystem.risk.test.ts b/packages/cli/test/integration/security-audit/FilesystemRiskReporter.test.ts similarity index 81% rename from packages/cli/test/integration/audit/filesystem.risk.test.ts rename to packages/cli/test/integration/security-audit/FilesystemRiskReporter.test.ts index 440a4bdba0..a3e3f69cc9 100644 --- a/packages/cli/test/integration/audit/filesystem.risk.test.ts +++ b/packages/cli/test/integration/security-audit/FilesystemRiskReporter.test.ts @@ -1,13 +1,17 @@ import { v4 as uuid } from 'uuid'; -import { audit } from '@/audit'; -import { FILESYSTEM_INTERACTION_NODE_TYPES, FILESYSTEM_REPORT } from '@/audit/constants'; +import { SecurityAuditService } from '@/security-audit/SecurityAudit.service'; +import { FILESYSTEM_INTERACTION_NODE_TYPES, FILESYSTEM_REPORT } from '@/security-audit/constants'; import { getRiskSection, saveManualTriggerWorkflow } from './utils'; import * as testDb from '../shared/testDb'; import { WorkflowRepository } from '@db/repositories/workflow.repository'; import Container from 'typedi'; +let securityAuditService: SecurityAuditService; + beforeAll(async () => { await testDb.init(); + + securityAuditService = new SecurityAuditService(Container.get(WorkflowRepository)); }); beforeEach(async () => { @@ -48,7 +52,7 @@ test('should report filesystem interaction nodes', async () => { await Promise.all(promises); - const testAudit = await audit(['filesystem']); + const testAudit = await securityAuditService.run(['filesystem']); const section = getRiskSection( testAudit, @@ -68,7 +72,7 @@ test('should report filesystem interaction nodes', async () => { test('should not report non-filesystem-interaction node', async () => { await saveManualTriggerWorkflow(); - const testAudit = await audit(['filesystem']); + const testAudit = await securityAuditService.run(['filesystem']); expect(testAudit).toBeEmptyArray(); }); diff --git a/packages/cli/test/integration/audit/instance.risk.test.ts b/packages/cli/test/integration/security-audit/InstanceRiskReporter.test.ts similarity index 87% rename from packages/cli/test/integration/audit/instance.risk.test.ts rename to packages/cli/test/integration/security-audit/InstanceRiskReporter.test.ts index bdd25a6f29..102ff1c6d6 100644 --- a/packages/cli/test/integration/audit/instance.risk.test.ts +++ b/packages/cli/test/integration/security-audit/InstanceRiskReporter.test.ts @@ -1,6 +1,6 @@ import { v4 as uuid } from 'uuid'; -import { audit } from '@/audit'; -import { INSTANCE_REPORT, WEBHOOK_VALIDATOR_NODE_TYPES } from '@/audit/constants'; +import { SecurityAuditService } from '@/security-audit/SecurityAudit.service'; +import { INSTANCE_REPORT, WEBHOOK_VALIDATOR_NODE_TYPES } from '@/security-audit/constants'; import { getRiskSection, saveManualTriggerWorkflow, @@ -9,15 +9,19 @@ import { simulateUpToDateInstance, } from './utils'; import * as testDb from '../shared/testDb'; -import { toReportTitle } from '@/audit/utils'; +import { toReportTitle } from '@/security-audit/utils'; import config from '@/config'; import { generateNanoId } from '@db/utils/generators'; import { WorkflowRepository } from '@db/repositories/workflow.repository'; import Container from 'typedi'; +let securityAuditService: SecurityAuditService; + beforeAll(async () => { await testDb.init(); + securityAuditService = new SecurityAuditService(Container.get(WorkflowRepository)); + simulateUpToDateInstance(); }); @@ -56,7 +60,7 @@ test('should report webhook lacking authentication', async () => { await Container.get(WorkflowRepository).save(details); - const testAudit = await audit(['instance']); + const testAudit = await securityAuditService.run(['instance']); const section = getRiskSection( testAudit, @@ -103,10 +107,12 @@ test('should not report webhooks having basic or header auth', async () => { await Promise.all(promises); - const testAudit = await audit(['instance']); - if (Array.isArray(testAudit)) fail('audit is empty'); + const testAudit = await securityAuditService.run(['instance']); + + if (Array.isArray(testAudit)) fail('Audit is empty'); const report = testAudit[toReportTitle('instance')]; + if (!report) { fail('Expected test audit to have instance risk report'); } @@ -164,7 +170,7 @@ test('should not report webhooks validated by direct children', async () => { await Promise.all(promises); - const testAudit = await audit(['instance']); + const testAudit = await securityAuditService.run(['instance']); if (Array.isArray(testAudit)) fail('audit is empty'); const report = testAudit[toReportTitle('instance')]; @@ -180,7 +186,7 @@ test('should not report webhooks validated by direct children', async () => { test('should not report non-webhook node', async () => { await saveManualTriggerWorkflow(); - const testAudit = await audit(['instance']); + const testAudit = await securityAuditService.run(['instance']); if (Array.isArray(testAudit)) fail('audit is empty'); const report = testAudit[toReportTitle('instance')]; @@ -197,7 +203,7 @@ test('should not report non-webhook node', async () => { test('should report outdated instance when outdated', async () => { simulateOutdatedInstanceOnce(); - const testAudit = await audit(['instance']); + const testAudit = await securityAuditService.run(['instance']); const section = getRiskSection( testAudit, @@ -215,7 +221,7 @@ test('should report outdated instance when outdated', async () => { }); test('should not report outdated instance when up to date', async () => { - const testAudit = await audit(['instance']); + const testAudit = await securityAuditService.run(['instance']); if (Array.isArray(testAudit)) fail('audit is empty'); const report = testAudit[toReportTitle('instance')]; @@ -231,7 +237,7 @@ test('should not report outdated instance when up to date', async () => { test('should report security settings', async () => { config.set('diagnostics.enabled', true); - const testAudit = await audit(['instance']); + const testAudit = await securityAuditService.run(['instance']); const section = getRiskSection( testAudit, diff --git a/packages/cli/test/integration/audit/nodes.risk.test.ts b/packages/cli/test/integration/security-audit/NodesRiskReporter.test.ts similarity index 84% rename from packages/cli/test/integration/audit/nodes.risk.test.ts rename to packages/cli/test/integration/security-audit/NodesRiskReporter.test.ts index a486ac7780..f10dbee30e 100644 --- a/packages/cli/test/integration/audit/nodes.risk.test.ts +++ b/packages/cli/test/integration/security-audit/NodesRiskReporter.test.ts @@ -1,8 +1,8 @@ import { v4 as uuid } from 'uuid'; import { Container } from 'typedi'; -import { audit } from '@/audit'; -import { OFFICIAL_RISKY_NODE_TYPES, NODES_REPORT } from '@/audit/constants'; -import { toReportTitle } from '@/audit/utils'; +import { SecurityAuditService } from '@/security-audit/SecurityAudit.service'; +import { OFFICIAL_RISKY_NODE_TYPES, NODES_REPORT } from '@/security-audit/constants'; +import { toReportTitle } from '@/security-audit/utils'; import { LoadNodesAndCredentials } from '@/LoadNodesAndCredentials'; import { NodeTypes } from '@/NodeTypes'; import { CommunityPackagesService } from '@/services/communityPackages.service'; @@ -18,8 +18,12 @@ mockInstance(NodeTypes); const communityPackagesService = mockInstance(CommunityPackagesService); Container.set(CommunityPackagesService, communityPackagesService); +let securityAuditService: SecurityAuditService; + beforeAll(async () => { await testDb.init(); + + securityAuditService = new SecurityAuditService(Container.get(WorkflowRepository)); }); beforeEach(async () => { @@ -59,7 +63,7 @@ test('should report risky official nodes', async () => { await Promise.all(promises); - const testAudit = await audit(['nodes']); + const testAudit = await securityAuditService.run(['nodes']); const section = getRiskSection( testAudit, @@ -80,10 +84,12 @@ test('should not report non-risky official nodes', async () => { communityPackagesService.getAllInstalledPackages.mockResolvedValue(MOCK_PACKAGE); await saveManualTriggerWorkflow(); - const testAudit = await audit(['nodes']); + const testAudit = await securityAuditService.run(['nodes']); + if (Array.isArray(testAudit)) return; const report = testAudit[toReportTitle('nodes')]; + if (!report) return; for (const section of report.sections) { @@ -94,7 +100,7 @@ test('should not report non-risky official nodes', async () => { test('should report community nodes', async () => { communityPackagesService.getAllInstalledPackages.mockResolvedValue(MOCK_PACKAGE); - const testAudit = await audit(['nodes']); + const testAudit = await securityAuditService.run(['nodes']); const section = getRiskSection( testAudit, diff --git a/packages/cli/test/integration/audit/utils.ts b/packages/cli/test/integration/security-audit/utils.ts similarity index 97% rename from packages/cli/test/integration/audit/utils.ts rename to packages/cli/test/integration/security-audit/utils.ts index f9d8d71106..1ad445e950 100644 --- a/packages/cli/test/integration/audit/utils.ts +++ b/packages/cli/test/integration/security-audit/utils.ts @@ -1,9 +1,9 @@ import nock from 'nock'; import config from '@/config'; import { v4 as uuid } from 'uuid'; -import { toReportTitle } from '@/audit/utils'; +import { toReportTitle } from '@/security-audit/utils'; import * as constants from '@/constants'; -import type { Risk } from '@/audit/types'; +import type { Risk } from '@/security-audit/types'; import type { InstalledNodes } from '@db/entities/InstalledNodes'; import type { InstalledPackages } from '@db/entities/InstalledPackages'; import { WorkflowRepository } from '@db/repositories/workflow.repository';