feat: Add token to sendAndWait operations links to walidate in webhook #17566

michael-radency · 2025-07-23T03:58:47Z

Summary

Links in sendAndWait operations will include a token, which will be validated upon receipt in the webhook method

Related Linear tickets, Github issues, and Community forum posts

https://linear.app/n8n/issue/NODE-3300/unprotected-human-in-the-loop-webhooks

Review / Merge checklist

PR title and summary are descriptive. (conventions)
Docs updated or follow-up ticket created.
Tests included.
PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

…unprotected-human-in-the-loop-webhooks

cubic-dev-ai

cubic analysis

2 issues found across 21 files • Review in cubic

_{React with 👍 or 👎 to teach cubic. You can also tag @cubic-dev-ai to give feedback, ask questions, or re-run the review.}

packages/core/src/execution-engine/node-execution-context/webhook-context.ts

packages/nodes-base/nodes/Slack/V2/GenericFunctions.ts

codecov · 2025-07-23T04:10:45Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
9234	1	9233	0

View the top 1 failed test(s) by shortest run time

ActiveExecutions shutdown Should cancel all executions when cancelAll is true

Stack Traces | 0.002s run time

Error: expect(received).toHaveLength(expected)

Expected length: 4
Received length: 3
Received array:  [{"id": "1547", "mode": "manual", "retryOf": undefined, "startedAt": 2025-07-29T08:08:42.892Z, "status": "new", "workflowId": "123"}, {"id": "1727", "mode": "manual", "retryOf": undefined, "startedAt": 2025-07-29T08:08:42.892Z, "status": "waiting", "workflowId": "123"}, {"id": "1873", "mode": "manual", "retryOf": undefined, "startedAt": 2025-07-29T08:08:42.892Z, "status": "waiting", "workflowId": "123"}]
    at Object.<anonymous> (.../src/__tests__/active-executions.test.ts:342:51)
    at Promise.then.completed (.../n8n/node_modules/.pnpm/[email protected]..../jest-circus/build/utils.js:300:28)
    at new Promise (<anonymous>)
    at callAsyncCircusFn (.../n8n/node_modules/.pnpm/[email protected]..../jest-circus/build/utils.js:233:10)
    at _callCircusTest (.../n8n/node_modules/.pnpm/[email protected]..../jest-circus/build/run.js:315:40)
    at _runTest (.../n8n/node_modules/.pnpm/[email protected]..../jest-circus/build/run.js:251:3)
    at _runTestsForDescribeBlock (.../n8n/node_modules/.pnpm/[email protected]..../jest-circus/build/run.js:125:9)
    at _runTestsForDescribeBlock (.../n8n/node_modules/.pnpm/[email protected]..../jest-circus/build/run.js:120:9)
    at _runTestsForDescribeBlock (.../n8n/node_modules/.pnpm/[email protected]..../jest-circus/build/run.js:120:9)
    at run (.../n8n/node_modules/.pnpm/[email protected]..../jest-circus/build/run.js:70:3)
    at runAndTransformResultsToJestFormat (.../n8n/node_modules/.pnpm/[email protected]..../build/legacy-code-todo-rewrite/jestAdapterInit.js:122:21)
    at jestAdapter (.../n8n/node_modules/.pnpm/[email protected]..../build/legacy-code-todo-rewrite/jestAdapter.js:79:19)
    at runTestInternal (.../n8n/node_modules/.pnpm/[email protected]..../jest-runner/build/runTest.js:367:16)
    at runTest (.../n8n/node_modules/.pnpm/[email protected]..../jest-runner/build/runTest.js:444:34)
    at Object.worker (.../n8n/node_modules/.pnpm/[email protected]..../jest-runner/build/testWorker.js:106:12)

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

csuermann · 2025-07-23T06:36:25Z

packages/core/src/execution-engine/node-execution-context/webhook-context.ts

+
+		const expectedToken = this.getExecutionWaitingToken();
+
+		const valid = crypto.timingSafeEqual(Buffer.from(token), Buffer.from(expectedToken));


Can we make this backwards compatible so that executions created before the introduction of the HMAC mechanism can still be resumed?

I'll check, but for now it doesn't look like it will be possible

I think it could be doable by storing the current largest exec id (eg using a migration) and require the signature only for executions that are newer than the oldest one before this change. Requires some effort but prolly doable

or we persist the expected signature as part of execution data and compare on resume…

maybe also by using timestamps of execution

question is do we want have code for that included in n8n and affect every single execution farther
breaking will be only single execution of workflow started before instance upgrade, so probably not a lot

tomi · 2025-07-23T21:22:11Z

packages/core/src/constants.ts

@@ -19,3 +19,5 @@ export const CREDENTIAL_ERRORS = {
 	INVALID_JSON: 'Decrypted credentials data is not valid JSON.',
 	INVALID_DATA: 'Credentials data is not in a valid format.',
 };
+
+export const WAITING_TOKEN_QUERY_PARAM = 'n8nwaitingtoken';


We should call the query param sig or signature which is commonly used name for a param like this

tomi · 2025-07-23T21:26:52Z

packages/core/src/execution-engine/node-execution-context/node-execution-context.ts

@@ -178,6 +180,22 @@ export abstract class NodeExecutionContext implements Omit<FunctionsBase, 'getCr
 		return this.instanceSettings.instanceId;
 	}

+	protected getExecutionWaitingToken() {
+		const token = crypto
+			.createHmac('sha256', this.instanceSettings.encryptionKey)


We should not use the instance encryption key directly for this as it’s not safe. Instead we should let it be configurable and by default derive a nee key from the encryption key. See eg the binary file signing key

n8n/packages/core/src/binary-data/binary-data.config.ts

Line 30 in f30cc7b

* Secret for creating publicly-accesible signed URLs for binary data.

tomi · 2025-07-23T21:35:52Z

packages/core/src/execution-engine/node-execution-context/node-execution-context.ts

+	protected getExecutionWaitingToken() {
+		const token = crypto
+			.createHmac('sha256', this.instanceSettings.encryptionKey)
+			.update(this.getExecutionId())


Signing only the execution id is not secure. It allows changing any query param and the signature will still be valid. It also makes any subsequent wait URL have the same signature. Instead we need to sugn all critical parts, at least path and all query params.

we could not include query params, as we expect them be different, depending of users choice
I think we could add stringified node's parameters to update, this would be unique per waiting node and we could decode it later, WDYT?

Oh so users can provide arbitrary input to the hitl nodes using query params? I was thinking of signing the URL but I guess we could also sign the details that identify the 1) execution 2) node 3) input (unless arbitrary)

currently it's execution id + stringified node(INode), I think this would be unique and secure

tomi · 2025-07-23T21:38:36Z

packages/core/src/execution-engine/node-execution-context/webhook-context.ts

+
+		const expectedToken = this.getExecutionWaitingToken();
+
+		const valid = crypto.timingSafeEqual(Buffer.from(token), Buffer.from(expectedToken));


The comment from cubic is valid. Don’t ignore it

yes, had to wrapped in trycatch block, thought I did this, thanks

…unprotected-human-in-the-loop-webhooks

csuermann · 2025-07-24T07:23:49Z

packages/core/src/instance-settings/instance-settings.ts

@@ -225,6 +228,14 @@ export class InstanceSettings {
 			.digest('hex');
 	}

+	private getOrGenerateHmacSignatureSecret() {
+		const hmacSignatureSecretFromEnv = process.env.N8N_HMAC_SIGNATURE_SECRET;


just for my understanding, why would using the encryptionKey (or a hash of it) be considered insecure? Introducing new env vars adds configuration complexity and I think we should avoid it unless really necessary.

having separate hmacSignatureSecret make sense to me
I would say we could skip adding new env and always derive hmacSignatureSecret from encryption key
@tomi what do you think?

Yeah ideally I would like us to have only single signing secret, but we have named the binary one very specifically, so we can’t reuse it (without breaking change). As long as we derive a nee key from encryption key it should be okay.

Using it directly is not a good idea because

They have different purposes and different risks. It’s used for encryption while signing key only for signing messages. If one is leaked the other still remains secret

Using the same key can be used to probe details (eg bits) about the key by crafting input to the hmac validation and observing the answer.

More details here

…unprotected-human-in-the-loop-webhooks

elsmr · 2025-07-24T10:22:48Z

packages/nodes-base/utils/sendAndWait/utils.ts

+	const isTokenValid = this.validateExecutionWaitingToken();
+
+	if (!isTokenValid) {
+		throw new Error('Invalid waiting token');


Should this be a NodeOperationError or another class?
Could validateExecutionWaitingToken do the throw?

Should this be a NodeOperationError or another class?
Yes, maybe a UserError or NodeApiErro would be more appropriate.

Could validateExecutionWaitingToken handle the throw?
I considered that, but I think it's better if this helper simply returns whether the token is valid. The calling logic can then decide how to proceed. Maybe we should rename it to isExecutionWaitingTokenValid to reflect that behavior.

UserError probably makes more sense here as it's invalid input from the user, tho it should never be invalid unless there's a bug or it's malicious input from a user, so NodeApiError might do as well. Don't really mind which one it is, as long as it results in a 403 response for the webhook.

elsmr · 2025-07-24T10:28:02Z

packages/nodes-base/utils/sendAndWait/utils.ts

@@ -357,6 +357,12 @@ export async function sendAndWaitWebhook(this: IWebhookFunctions) {
 	const res = this.getResponseObject();
 	const req = this.getRequestObject();

+	const isTokenValid = this.validateExecutionWaitingToken();


Would it be possible to do this in core, maybe in waiting-webhooks, for any waiting execution that is resumed?

as for now doing this will break other waiting execution, because updates in nodes will be needed, this PR deals only with HiTL operations

Ideally we should do this in core and provide necessary methods for nodes. I think it’s ok to do it for hitl nodes first

It feels wrong to have this in the core.

Currently, this is a complete feature that allows obtaining a signing token and validating it at the node level.
If we move it to the core, we would need to check what kind of node it is and what is current operation. This logic would likely need to be modified later.

We should also consider community nodes - there’s already interest in adding send and wait operations. In my opinion, it’s better to provide tools for node developers rather than handling this at the core level.

I was thinking if we could always add the signature to $execution.resumeUrl and verify the signature of all (not specific nodes) incoming requests on waiting-webhook.

Then node code wouldn't have to be changed and $execution.resumeUrl would keep working for existing workflows.

we could, but it would increase the scope
we’d have to ensure that the signature is always included in all possible waiting executions

changes to the nodes would still be required - for example like in this PR for including url in service's message

Exactly what Elias said. Every node using resume URL should have this. But like I said, let's keep this scoped to HITL nodes for now.

elsmr · 2025-07-24T10:29:34Z

packages/nodes-base/utils/sendAndWait/utils.ts

@@ -480,8 +486,6 @@ export function getSendAndWaitConfig(context: IExecuteFunctions): SendAndWaitCon
 		.replace(/\\n/g, '\n')
 		.replace(/<br>/g, '\n');
 	const subject = escapeHtml(context.getNodeParameter('subject', 0, '') as string);
-	const resumeUrl = context.evaluateExpression('{{ $execution?.resumeUrl }}', 0) as string;


Does this mean that $execution.resumeUrl will no longer work because it would be missing the signature?

yes, unfortunately, this is breaking change for responses for executions started before update, still looking if it's possible to avoid

But wouldn't it also be breaking for new executions, if your workflow uses $execution.resumeUrl in an expression?

resumeUrl is only used in the Wait node for webhook mode

In other waiting nodes, such as sendAndWait operations and forms, we provide the FE with a way to resume execution

it would not be breaking, this PR does not modify resumeUrl
sendAndWait operations currently use resumeUrl + nodeId
Forms use resumeFormUrl

…unprotected-human-in-the-loop-webhooks

tomi · 2025-07-31T11:07:27Z

packages/nodes-base/utils/sendAndWait/utils.ts

@@ -357,6 +357,12 @@ export async function sendAndWaitWebhook(this: IWebhookFunctions) {
 	const res = this.getResponseObject();
 	const req = this.getRequestObject();

+	const isTokenValid = this.validateExecutionWaitingToken();


Exactly what Elias said. Every node using resume URL should have this. But like I said, let's keep this scoped to HITL nodes for now.

tomi · 2025-07-31T11:34:08Z

packages/nodes-base/utils/sendAndWait/utils.ts

+	const isTokenValid = this.validateExecutionWaitingToken();
+
+	if (!isTokenValid) {
+		throw new Error('Invalid waiting token');


UserError probably makes more sense here as it's invalid input from the user, tho it should never be invalid unless there's a bug or it's malicious input from a user, so NodeApiError might do as well. Don't really mind which one it is, as long as it results in a 403 response for the webhook.

tomi · 2025-07-31T12:15:26Z

packages/core/src/execution-engine/node-execution-context/node-execution-context.ts

+		if (this.runExecutionData) {
+			this.runExecutionData.validateSignature = true;
+		}


It's a bit odd that a get* method does a side effect. Could we do this somewhere else?

Also, what if there is no execution data?

It's a bit odd that a get* method does a side effect. Could we do this somewhere else?
moved to constructor
Also, what if there is no execution data?
then we do not need to set anything as context not used in execution

…unprotected-human-in-the-loop-webhooks

michael-radency · 2025-08-04T06:41:47Z

@tomi updated as suggested, could you please review?

tomi

Looks mostly good! Few minor comments

tomi · 2025-08-04T12:43:39Z

packages/core/src/execution-engine/node-execution-context/node-execution-context.ts

+		if (this.runExecutionData) this.runExecutionData.validateSignature = true;
+	}
+
+	getSignedResumeUrl(parameters: Record<string, string> = {}) {


Can we add docs for this

tomi · 2025-08-04T12:44:55Z

packages/core/src/execution-engine/node-execution-context/node-execution-context.ts

+		const { webhookWaitingBaseUrl, executionId } = this.additionalData;
+
+		if (typeof executionId !== 'string') {
+			throw new ApplicationError('Execution id is missing');


ApplicationError is deprecated. Is this an expected situation? If not, we could use UnexpectedError here

tomi · 2025-08-04T12:45:25Z

packages/core/src/execution-engine/node-execution-context/node-execution-context.ts

+		const baseURL = new URL(`${webhookWaitingBaseUrl}/${executionId}/${this.node.id}`);
+
+		for (const [key, value] of Object.entries(parameters)) {
+			baseURL.searchParams.set(key, value);
+		}
+
+		const token = this.getExecutionWaitingToken(baseURL.toString());
+
+		baseURL.searchParams.set(WAITING_TOKEN_QUERY_PARAM, token);
+
+		return baseURL.toString();


tomi · 2025-08-04T12:45:59Z

packages/core/src/execution-engine/node-execution-context/node-execution-context.ts

@@ -200,6 +202,38 @@ export abstract class NodeExecutionContext implements Omit<FunctionsBase, 'getCr
 		return this.instanceSettings.instanceId;
 	}

+	getExecutionWaitingToken(url: string) {


Is this used outside this class? Should make it private/protected if not

tomi · 2025-08-04T12:46:12Z

packages/core/src/execution-engine/node-execution-context/webhook-context.ts

@@ -172,4 +174,28 @@ export class WebhookContext extends NodeExecutionContext implements IWebhookFunc
 			itemIndex,
 		);
 	}
+
+	validateExecutionWaitingToken() {


Let's document this method

tomi · 2025-08-04T12:47:34Z

packages/core/src/execution-engine/node-execution-context/webhook-context.ts

+
+			if (typeof token !== 'string') return false;
+
+			const parsedUrl = new URL(req.url, `http://${req.headers.host}`);


Can we assume the URL is http? Couldn't it be https as well? Do we need to define the base?

tomi · 2025-08-04T12:51:37Z

packages/nodes-base/nodes/Google/Chat/GenericFunctions.ts

@@ -163,7 +163,7 @@ export function createSendAndWaitMessageBody(context: IExecuteFunctions) {
 	const config = getSendAndWaitConfig(context);

 	const buttons: string[] = config.options.map(
-		(option) => `*<${`${config.url}?approved=${option.value}`}|${option.label}>*`,
+		(option) => `*<${`${option.url}`}|${option.label}>*`,


These are actually cleaner now that they don't build URLs anymore 👍

michael-radency added 2 commits July 23, 2025 06:55

fix, tests update

3174ff8

Merge branch 'master' of https://github.com/n8n-io/n8n into node-3300-…

407ae05

…unprotected-human-in-the-loop-webhooks

michael-radency added node/improvement New feature or request core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team node/issue Issue with a node labels Jul 23, 2025

cubic-dev-ai bot reviewed Jul 23, 2025

View reviewed changes

packages/core/src/execution-engine/node-execution-context/webhook-context.ts Outdated Show resolved Hide resolved

packages/nodes-base/nodes/Slack/V2/GenericFunctions.ts Outdated Show resolved Hide resolved

dana-gill requested a review from elsmr July 23, 2025 04:03

michael-radency added 2 commits July 23, 2025 07:17

tests

9b78648

tests

14fdaa5

csuermann reviewed Jul 23, 2025

View reviewed changes

tomi reviewed Jul 23, 2025

View reviewed changes

michael-radency added 2 commits July 24, 2025 08:24

Merge branch 'master' of https://github.com/n8n-io/n8n into node-3300-…

e3dc0d6

…unprotected-human-in-the-loop-webhooks

review update

4308d1c

michael-radency requested a review from tomi July 24, 2025 06:55

csuermann reviewed Jul 24, 2025

View reviewed changes

michael-radency added 3 commits July 24, 2025 11:27

Merge branch 'master' of https://github.com/n8n-io/n8n into node-3300-…

e881c5c

…unprotected-human-in-the-loop-webhooks

test harness fix

fd4963c

Merge branch 'master' of https://github.com/n8n-io/n8n into node-3300-…

ff48c45

…unprotected-human-in-the-loop-webhooks

elsmr reviewed Jul 24, 2025

View reviewed changes

michael-radency added 4 commits July 29, 2025 09:00

Merge branch 'master' of https://github.com/n8n-io/n8n into node-3300-…

1ccbf56

…unprotected-human-in-the-loop-webhooks

Merge branch 'master' of https://github.com/n8n-io/n8n into node-3300-…

756cf25

…unprotected-human-in-the-loop-webhooks

prevent breaking change for already started waiting executions

b4e062b

Merge branch 'master' of https://github.com/n8n-io/n8n into node-3300-…

f4057ae

…unprotected-human-in-the-loop-webhooks

tomi reviewed Jul 31, 2025

View reviewed changes

michael-radency added 4 commits August 1, 2025 08:36

Merge branch 'master' of https://github.com/n8n-io/n8n into node-3300-…

3ee7b34

…unprotected-human-in-the-loop-webhooks

review update, wip

f453713

Merge branch 'master' of https://github.com/n8n-io/n8n into node-3300-…

b9cd6e0

…unprotected-human-in-the-loop-webhooks

Merge branch 'master' of https://github.com/n8n-io/n8n into node-3300-…

55f3d88

…unprotected-human-in-the-loop-webhooks

michael-radency added 2 commits August 4, 2025 09:22

review update

22abca8

Merge branch 'master' of https://github.com/n8n-io/n8n into node-3300-…

c1ac415

…unprotected-human-in-the-loop-webhooks

michael-radency requested a review from tomi August 4, 2025 06:41

tomi reviewed Aug 4, 2025

View reviewed changes


		const expectedToken = this.getExecutionWaitingToken();

		const valid = crypto.timingSafeEqual(Buffer.from(token), Buffer.from(expectedToken));


		if (typeof token !== 'string') return false;

		const parsedUrl = new URL(req.url, `http://${req.headers.host}`);

feat: Add token to sendAndWait operations links to walidate in webhook #17566

Are you sure you want to change the base?

feat: Add token to sendAndWait operations links to walidate in webhook #17566

Conversation

michael-radency commented Jul 23, 2025

Summary

Related Linear tickets, Github issues, and Community forum posts

Review / Merge checklist

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

cubic analysis

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ 1 Tests Failed:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov bot commented Jul 23, 2025 •

edited

Loading