mirror of
https://github.com/n8n-io/n8n.git
synced 2026-10-17 05:29:44 +02:00
feat(core): Lock credential decryption to available node types (no-changelog) (#38988)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
0f3c8f0841
commit
e530b79b5a
|
|
@ -1,8 +1,8 @@
|
|||
# Node type availability policies
|
||||
|
||||
Rules that say which node types a project may use. An admin writes the rules. The policy
|
||||
infrastructure enforces them at the points that handle a workflow: save, publish, start,
|
||||
transfer and import.
|
||||
infrastructure enforces them at the points that handle a workflow — save, publish, start,
|
||||
transfer and import — and at the point where a node asks for a credential.
|
||||
|
||||
This module is the first tenant of that infrastructure. It adds a check and a store, and
|
||||
nothing else: no enforcement path of its own, no error shape, no audit line. Read
|
||||
|
|
@ -27,13 +27,14 @@ The break-glass levers stay the ones the substrate documents: remove this module
|
|||
|
||||
## What each point decides
|
||||
|
||||
| Point | Scope it reads | Verdict |
|
||||
| ------------------ | -------------------------------- | ------------------------------------------- |
|
||||
| `workflowSave` | the workflow's project | violations for node **types** the save adds |
|
||||
| `workflowPublish` | the workflow's project | every blocked type in the workflow |
|
||||
| `workflowStart` | the workflow's project | every blocked type in the workflow |
|
||||
| `workflowTransfer` | the **target** project | every blocked type in the workflow |
|
||||
| `contentImport` | the project the content lands in | every blocked type in the workflow |
|
||||
| Point | Scope it reads | Verdict |
|
||||
| ------------------- | -------------------------------- | ---------------------------------------------- |
|
||||
| `workflowSave` | the workflow's project | violations for node **types** the save adds |
|
||||
| `workflowPublish` | the workflow's project | every blocked type in the workflow |
|
||||
| `workflowStart` | the workflow's project | every blocked type in the workflow |
|
||||
| `workflowTransfer` | the **target** project | every blocked type in the workflow |
|
||||
| `contentImport` | the project the content lands in | every blocked type in the workflow |
|
||||
| `credentialDecrypt` | the executing project | the type of the node asking for the credential |
|
||||
|
||||
The check reads the license again per decision. A license that lapses while the instance runs
|
||||
stops enforcement, which matches the routes that author the policy: an admin who can no longer
|
||||
|
|
@ -66,6 +67,26 @@ does not travel into a project whose rules the content has never met.
|
|||
A node counts even when it is disabled. Exempting disabled nodes would open a two-step path
|
||||
around the save diff: add the node disabled, then enable it once its type is stored.
|
||||
|
||||
## The credential lock
|
||||
|
||||
`credentialDecrypt` is the one point that reads no workflow. A node asks for a credential, and
|
||||
the verdict is about **the node asking**: a Slack credential is refused to a blocked Slack node
|
||||
and handed to an allowed HTTP Request. The credential's own type is never evaluated. Deriving
|
||||
availability from "some usable node accepts this type" is close to vacuous, because HTTP
|
||||
Request accepts almost any credential, and it would let installing an unrelated node package
|
||||
change which credentials work.
|
||||
|
||||
`workflowStart` already fails a run that carries a blocked type, so this point is there for the
|
||||
decryptions no run covers:
|
||||
|
||||
- a parameter dropdown loading its options, which decrypts without starting an execution;
|
||||
- a node the workflow points cannot see, such as one inside a workflow carried in another
|
||||
node's parameters.
|
||||
|
||||
A refusal fails the decryption with the same `node-type-unavailable` violation the workflow
|
||||
points report, naming the **node type** as `subject`. The audit line carries the credential's
|
||||
id and type from the context.
|
||||
|
||||
## What a violation looks like
|
||||
|
||||
One violation per blocked type, deduplicated, in the order the types first appear:
|
||||
|
|
@ -144,6 +165,11 @@ through a sealed repository method, and the lint rule that guards that has no al
|
|||
deny `n8n-nodes-base.gmailTool`. The registry holds them as two types.
|
||||
- **A workflow carried inside a node's parameters is not read.** The check reads
|
||||
`workflow.nodes`. Node types inside an inline sub-workflow definition are invisible to it.
|
||||
The credential lock still catches such a node once it asks for a credential.
|
||||
- **A decryption with no consumer is not policed.** `consumer` is `null` wherever no node is
|
||||
asking — an OAuth authorize or revoke, the agents adapter, a log-streaming destination. There
|
||||
is no node type to evaluate there, and the credential's type is deliberately not a stand-in,
|
||||
so the decryption goes through.
|
||||
- **Cache staleness is bounded by the TTL, not eliminated.** A write drops the cached scope it
|
||||
changed, which is enough wherever the processes share one Redis cache — which is every
|
||||
deployment that has more than one, unless `N8N_CACHE_BACKEND=memory` is set by hand in queue
|
||||
|
|
@ -176,7 +202,7 @@ through a sealed repository method, and the lint rule that guards that has no al
|
|||
|
||||
| File | Role |
|
||||
| ------------------------------------------------- | ------------------------------------------------------------------------------- |
|
||||
| `node-type-policy.check.ts` | The `@PolicyCheck()` class: the five points, the save diff, the violations |
|
||||
| `node-type-policy.check.ts` | The `@PolicyCheck()` class: the six points, the save diff, the violations |
|
||||
| `policy-evaluator.ts` | Pure evaluation: first match per scope, then the instance ∩ project composition |
|
||||
| `policy-shadow-lint.ts` | Warns at write time about rules an earlier rule already covers |
|
||||
| `type-availability-policy.service.ts` | Reads and writes the store, with versioning and row locks |
|
||||
|
|
|
|||
|
|
@ -1,5 +1,9 @@
|
|||
import type { LicenseState } from '@n8n/backend-common';
|
||||
import type { ContentImportTransport, PolicedWorkflow } from '@n8n/decorators';
|
||||
import type {
|
||||
ContentImportTransport,
|
||||
CredentialDecryptContext,
|
||||
PolicedWorkflow,
|
||||
} from '@n8n/decorators';
|
||||
import type { INode } from 'n8n-workflow';
|
||||
import { mock } from 'vitest-mock-extended';
|
||||
|
||||
|
|
@ -13,6 +17,14 @@ import type {
|
|||
const SLACK = 'n8n-nodes-base.slack';
|
||||
const GMAIL = 'n8n-nodes-base.gmail';
|
||||
const SET = 'n8n-nodes-base.set';
|
||||
const HTTP_REQUEST = 'n8n-nodes-base.httpRequest';
|
||||
|
||||
const slackCredentialFor = (nodeType: string | null): CredentialDecryptContext => ({
|
||||
credentialType: 'slackApi',
|
||||
credentialId: 'cred-1',
|
||||
consumer: nodeType === null ? null : { nodeType },
|
||||
projectId: 'project-1',
|
||||
});
|
||||
|
||||
const node = (type: string, name = type, disabled = false): INode =>
|
||||
({ id: name, name, type, typeVersion: 1, position: [0, 0], parameters: {}, disabled }) as INode;
|
||||
|
|
@ -193,6 +205,51 @@ describe('NodeTypePolicyCheck', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('onCredentialDecrypt', () => {
|
||||
it('vetoes the decryption when the node asking is a denied type', async () => {
|
||||
const result = await check.onCredentialDecrypt(slackCredentialFor(SLACK));
|
||||
|
||||
expect(result.violations[0]).toStrictEqual({
|
||||
kind: 'node-type-unavailable',
|
||||
checkId: 'node-type-availability',
|
||||
message: `Node type "${SLACK}" is blocked by an instance policy`,
|
||||
subject: SLACK,
|
||||
subjectType: 'nodeType',
|
||||
scope: 'instance',
|
||||
matchedRuleId: 'rule-7',
|
||||
});
|
||||
expect(service.evaluateComposedTypesFor).toHaveBeenCalledWith('node-types', 'project-1', [
|
||||
SLACK,
|
||||
]);
|
||||
});
|
||||
|
||||
it('hands the same credential to a node whose type is available', async () => {
|
||||
const result = await check.onCredentialDecrypt(slackCredentialFor(HTTP_REQUEST));
|
||||
|
||||
expect(result.violations).toEqual([]);
|
||||
expect(service.evaluateComposedTypesFor).toHaveBeenCalledWith('node-types', 'project-1', [
|
||||
HTTP_REQUEST,
|
||||
]);
|
||||
});
|
||||
|
||||
it('reports nothing when no node is asking, and never reads the policy', async () => {
|
||||
const result = await check.onCredentialDecrypt(slackCredentialFor(null));
|
||||
|
||||
expect(result.violations).toEqual([]);
|
||||
expect(service.evaluateComposedTypesFor).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('evaluates instance-only when the execution has no project', async () => {
|
||||
const result = await check.onCredentialDecrypt({
|
||||
...slackCredentialFor(SLACK),
|
||||
projectId: null,
|
||||
});
|
||||
|
||||
expect(service.evaluateComposedTypesFor).toHaveBeenCalledWith('node-types', null, [SLACK]);
|
||||
expect(result.violations).toHaveLength(1);
|
||||
});
|
||||
});
|
||||
|
||||
describe('scope resolution', () => {
|
||||
it('evaluates instance-only when no project applies', async () => {
|
||||
const result = await check.onWorkflowStart({
|
||||
|
|
@ -269,6 +326,15 @@ describe('NodeTypePolicyCheck', () => {
|
|||
expect(service.evaluateComposedTypesFor).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('reports nothing on a credential decrypt without a license', async () => {
|
||||
licenseState.isLicensed.mockReturnValue(false);
|
||||
|
||||
const result = await check.onCredentialDecrypt(slackCredentialFor(SLACK));
|
||||
|
||||
expect(result.violations).toEqual([]);
|
||||
expect(service.evaluateComposedTypesFor).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('reports nothing for a workflow with no nodes', async () => {
|
||||
const result = await check.onWorkflowSave({
|
||||
workflow: workflow([], null),
|
||||
|
|
|
|||
|
|
@ -2,6 +2,7 @@ import { LicenseState } from '@n8n/backend-common';
|
|||
import { LICENSE_FEATURES } from '@n8n/constants';
|
||||
import type {
|
||||
ContentImportContext,
|
||||
CredentialDecryptContext,
|
||||
PolicedWorkflow,
|
||||
PolicyCheckResult,
|
||||
PolicyViolation,
|
||||
|
|
@ -106,20 +107,40 @@ export class NodeTypePolicyCheck implements RegisteredPolicyCheck {
|
|||
}
|
||||
|
||||
/**
|
||||
* No `signal` is threaded: nothing below accepts one (TypeORM does not), and the deadline
|
||||
* race in the decision service already turns an overrun into a check failure.
|
||||
* Locks on the node asking, never on the credential's type: the same credential is refused
|
||||
* to a blocked Slack node and handed to an allowed HTTP Request. This catches what a run
|
||||
* cannot — a dropdown loading its options, a node inside an inline workflow definition.
|
||||
*/
|
||||
async onCredentialDecrypt({
|
||||
consumer,
|
||||
projectId,
|
||||
}: CredentialDecryptContext): Promise<PolicyCheckResult> {
|
||||
// No node is asking — an OAuth flow, a credential test — so there is no consumer to
|
||||
// police. Deriving a verdict from the credential's type instead is what the RFC rejects.
|
||||
if (consumer === null) return NO_VIOLATIONS;
|
||||
|
||||
return await this.checkTypes([consumer.nodeType], projectId);
|
||||
}
|
||||
|
||||
private async check(
|
||||
workflow: PolicedWorkflow,
|
||||
projectId: string | null,
|
||||
grandfathered: ReadonlySet<string>,
|
||||
): Promise<PolicyCheckResult> {
|
||||
const types = distinctTypes(workflow.nodes).filter((type) => !grandfathered.has(type));
|
||||
|
||||
return await this.checkTypes(types, projectId);
|
||||
}
|
||||
|
||||
/**
|
||||
* No `signal` is threaded: nothing below accepts one (TypeORM does not), and the deadline
|
||||
* race in the decision service already turns an overrun into a check failure.
|
||||
*/
|
||||
private async checkTypes(types: string[], projectId: string | null): Promise<PolicyCheckResult> {
|
||||
// An expired license stops enforcing, matching the `@Licensed` routes that author the
|
||||
// policy: a customer who cannot edit the policy must not keep being blocked by it.
|
||||
if (!this.licenseState.isLicensed(LICENSE_FEATURES.NODE_TYPE_POLICIES)) return NO_VIOLATIONS;
|
||||
|
||||
const types = distinctTypes(workflow.nodes).filter((type) => !grandfathered.has(type));
|
||||
|
||||
if (types.length === 0) return NO_VIOLATIONS;
|
||||
|
||||
const { verdicts, versions } = await this.service.evaluateComposedTypesFor(
|
||||
|
|
|
|||
|
|
@ -0,0 +1,132 @@
|
|||
/**
|
||||
* Pins the credential lock on the real path: the real store behind the real `@PolicyCheck()`,
|
||||
* with `CredentialsHelper.getDecrypted` as the host. The pair of cases is the point — a lock
|
||||
* keyed on the credential's type would pass the refusal and fail the hand-over.
|
||||
*/
|
||||
import { testDb } from '@n8n/backend-test-utils';
|
||||
import { LICENSE_FEATURES } from '@n8n/constants';
|
||||
import { ProjectRepository, type User } from '@n8n/db';
|
||||
import { PolicyCheckMetadata } from '@n8n/decorators';
|
||||
import { Container } from '@n8n/di';
|
||||
import type { IExecuteData, IWorkflowExecuteAdditionalData } from 'n8n-workflow';
|
||||
|
||||
import { CredentialsHelper } from '@/credentials-helper';
|
||||
|
||||
import { saveCredential } from '../shared/db/credentials';
|
||||
import { createOwner } from '../shared/db/users';
|
||||
import type { SuperAgentTest } from '../shared/types';
|
||||
import * as utils from '../shared/utils/';
|
||||
import { clearPolicyCache } from './shared/policy-cache';
|
||||
|
||||
const CHECK_ID = 'node-type-availability';
|
||||
|
||||
const SLACK = 'n8n-nodes-base.slack';
|
||||
const HTTP_REQUEST = 'n8n-nodes-base.httpRequest';
|
||||
|
||||
const testServer = utils.setupTestServer({
|
||||
endpointGroups: ['type-availability-policies'],
|
||||
modules: ['policy-infrastructure', 'type-availability-policies'],
|
||||
enabledFeatures: [LICENSE_FEATURES.NODE_TYPE_POLICIES],
|
||||
});
|
||||
|
||||
let owner: User;
|
||||
let ownerAgent: SuperAgentTest;
|
||||
let additionalData: IWorkflowExecuteAdditionalData;
|
||||
|
||||
/** Only `node` is read: the helper passes it on as the check's consumer. */
|
||||
const askingAs = (nodeType: string) => ({ node: { type: nodeType } }) as IExecuteData;
|
||||
|
||||
/**
|
||||
* `raw` returns the stored data before overwrites and defaults, which keeps the credential type
|
||||
* out of the node registry — the check reads the asking node's type, never the credential's.
|
||||
*/
|
||||
async function decryptAskingAs(nodeType?: string) {
|
||||
const credential = await saveCredential(
|
||||
{ name: 'Slack account', type: 'slackApi', data: { accessToken: 'secret' } },
|
||||
{ user: owner, role: 'credential:owner' },
|
||||
);
|
||||
|
||||
return await Container.get(CredentialsHelper).getDecrypted(
|
||||
additionalData,
|
||||
{ id: credential.id, name: credential.name },
|
||||
credential.type,
|
||||
'internal',
|
||||
nodeType === undefined ? undefined : askingAs(nodeType),
|
||||
true,
|
||||
);
|
||||
}
|
||||
|
||||
async function denySlackAtInstanceScope() {
|
||||
const response = await ownerAgent.put('/node-type-policies/instance').send({
|
||||
rules: [{ id: 'deny-slack', action: 'deny', selector: { kind: 'name', value: SLACK } }],
|
||||
defaultAction: 'allow',
|
||||
version: 0,
|
||||
});
|
||||
|
||||
expect(response.statusCode).toBe(200);
|
||||
}
|
||||
|
||||
beforeAll(async () => {
|
||||
// Asserted by id, not by importing the class: the import would register the check itself,
|
||||
// and the refusal below would pass with the module no longer registering it.
|
||||
const registeredIds = Container.get(PolicyCheckMetadata)
|
||||
.getClasses()
|
||||
.map((checkClass) => Container.get(checkClass).id);
|
||||
expect(registeredIds).toContain(CHECK_ID);
|
||||
|
||||
owner = await createOwner();
|
||||
ownerAgent = testServer.authAgentFor(owner);
|
||||
|
||||
const personalProject = await Container.get(ProjectRepository).getPersonalProjectForUserOrFail(
|
||||
owner.id,
|
||||
);
|
||||
additionalData = { projectId: personalProject.id } as IWorkflowExecuteAdditionalData;
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
await testDb.truncate([
|
||||
'TypeAvailabilityPolicyAttachment',
|
||||
'TypeAvailabilityPolicyScope',
|
||||
'TypeAvailabilityPolicy',
|
||||
'SharedCredentials',
|
||||
'CredentialsEntity',
|
||||
]);
|
||||
await clearPolicyCache();
|
||||
});
|
||||
|
||||
describe('credential lock', () => {
|
||||
test('refuses to decrypt for a node type the policy blocks', async () => {
|
||||
await denySlackAtInstanceScope();
|
||||
|
||||
await expect(decryptAskingAs(SLACK)).rejects.toMatchObject({
|
||||
httpStatusCode: 403,
|
||||
violations: [
|
||||
{
|
||||
kind: 'node-type-unavailable',
|
||||
checkId: CHECK_ID,
|
||||
message: `Node type "${SLACK}" is blocked by an instance policy`,
|
||||
subject: SLACK,
|
||||
subjectType: 'nodeType',
|
||||
scope: 'instance',
|
||||
matchedRuleId: 'deny-slack',
|
||||
},
|
||||
],
|
||||
});
|
||||
});
|
||||
|
||||
test('hands the same credential to a node type the policy allows', async () => {
|
||||
await denySlackAtInstanceScope();
|
||||
|
||||
await expect(decryptAskingAs(HTTP_REQUEST)).resolves.toEqual({ accessToken: 'secret' });
|
||||
});
|
||||
|
||||
test('decrypts when no node is asking, because the lock has no consumer to read', async () => {
|
||||
await denySlackAtInstanceScope();
|
||||
|
||||
await expect(decryptAskingAs()).resolves.toEqual({ accessToken: 'secret' });
|
||||
});
|
||||
|
||||
test('decrypts for any node type while no rule denies one', async () => {
|
||||
await expect(decryptAskingAs(SLACK)).resolves.toEqual({ accessToken: 'secret' });
|
||||
});
|
||||
});
|
||||
Loading…
Reference in New Issue
Block a user