Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(Wait Node): Prevent waiting until invalid date #10523

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/core/src/NodeExecuteFunctions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ import { InstanceSettings } from './InstanceSettings';
import { ScheduledTaskManager } from './ScheduledTaskManager';
import { SSHClientsManager } from './SSHClientsManager';
import { binaryToBuffer } from './BinaryData/utils';
import { assertValidDate } from './assertions';

axios.defaults.timeout = 300000;
// Prevent axios from adding x-form-www-urlencoded headers by default
Expand Down Expand Up @@ -3776,7 +3777,9 @@ export function getExecuteFunctions(
);
return dataProxy.getDataProxy();
},
async putExecutionToWait(waitTill: Date): Promise<void> {
async putExecutionToWait(waitTill: unknown): Promise<void> {
assertValidDate(waitTill);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of putting this here (where we'll then validate all dates), we should put this check in the wait node only when using a user input date.


runExecutionData.waitTill = waitTill;
if (additionalData.setExecutionStatus) {
additionalData.setExecutionStatus('waiting');
Expand Down
7 changes: 7 additions & 0 deletions packages/core/src/assertions.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { ApplicationError } from 'n8n-workflow';

export function assertValidDate(candidate: unknown): asserts candidate is Date {
if (!(candidate instanceof Date) || isNaN(candidate.getTime())) {
throw new ApplicationError('Found invalid date', { extra: { date: candidate } });
}
}
22 changes: 22 additions & 0 deletions packages/core/test/assertions.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { assertValidDate } from '@/assertions';
import { ApplicationError } from 'n8n-workflow';

describe('assertions', () => {
describe('assertValidDate', () => {
it('should throw if the candidate is not a date', () => {
expect(() => assertValidDate(null)).toThrowError(
new ApplicationError('Found invalid date', { extra: { date: null } }),
);
});

it('should throw if the candidate is an invalid date', () => {
expect(() => assertValidDate(new Date('invalid'))).toThrowError(
new ApplicationError('Found invalid date', { extra: { date: new Date('invalid') } }),
);
});

it('should not throw if the candidate is a valid date', () => {
expect(() => assertValidDate(new Date())).not.toThrow();
});
});
});
9 changes: 9 additions & 0 deletions packages/nodes-base/nodes/Wait/Wait.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
IDisplayOptions,
IWebhookFunctions,
} from 'n8n-workflow';
import { WAIT_TIME_UNLIMITED } from 'n8n-workflow';

Check failure on line 10 in packages/nodes-base/nodes/Wait/Wait.node.ts

View workflow job for this annotation

GitHub Actions / Lint / Lint

'/home/runner/work/n8n/n8n/packages/workflow/dist/index.js' imported multiple times

import {
authenticationProperty,
Expand All @@ -31,6 +31,7 @@
import { formWebhook } from '../Form/utils';
import { updateDisplayOptions } from '../../utils/utilities';
import { Webhook } from '../Webhook/Webhook.node';
import { NodeOperationError } from 'n8n-workflow';

Check failure on line 34 in packages/nodes-base/nodes/Wait/Wait.node.ts

View workflow job for this annotation

GitHub Actions / Lint / Lint

'/home/runner/work/n8n/n8n/packages/workflow/dist/index.js' imported multiple times

const toWaitAmount: INodeProperties = {
displayName: 'Wait Amount',
Expand Down Expand Up @@ -346,6 +347,7 @@
},
default: '',
description: 'The date and time to wait for before continuing',
required: true,
Copy link
Contributor Author

@ivov ivov Aug 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A deeper fix would be having this param type fail when required: true and still using a '' default for all nodes.

Since this is a rare edge case and that deeper change might have unexpected side effects, I'd rather prevent this at the nodes-base and core levels for now.

},

// ----------------------------------
Expand Down Expand Up @@ -492,6 +494,13 @@
} else {
const dateTimeStr = context.getNodeParameter('dateTime', 0) as string;

if (dateTimeStr === '') {
throw new NodeOperationError(
context.getNode(),
'[Wait node] Cannot put execution to wait because `dateTime` parameter is empty. Please pick a specific date and time to wait until.',
);
}

netroy marked this conversation as resolved.
Show resolved Hide resolved
waitTill = DateTime.fromFormat(dateTimeStr, "yyyy-MM-dd'T'HH:mm:ss", {
zone: context.getTimezone(),
})
Expand Down
Loading