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.

Sign up for GitHub

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(editor): Prevent infinite loop in expressions crashing the browser #12732

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+268 −135
Open

fix(editor): Prevent infinite loop in expressions crashing the browser #12732

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
22fba6c
Prevent infinite loop in expressions crashing the browser
elsmr Jan 20, 2025
a033ae1
Remove console.logs
elsmr Jan 21, 2025
1fb70de
Remove last console.log
elsmr Jan 21, 2025
c6f25fb
Remove .only
elsmr Jan 21, 2025
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
65 changes: 18 additions & 47 deletions packages/editor-ui/src/components/AssignmentCollection/Assignment.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@ import InputTriple from '@/components/InputTriple/InputTriple.vue';
import ParameterInputFull from '@/components/ParameterInputFull.vue';
import ParameterInputHint from '@/components/ParameterInputHint.vue';
import ParameterIssues from '@/components/ParameterIssues.vue';
import { useWorkflowHelpers } from '@/composables/useWorkflowHelpers';
import { useWorkflowsStore } from '@/stores/workflows.store';
import { isExpression, stringifyExpressionResult } from '@/utils/expressions';
import type { AssignmentValue, INodeProperties, Result } from 'n8n-workflow';
import { useResolvedExpression } from '@/composables/useResolvedExpression';
import useEnvironmentsStore from '@/stores/environments.ee.store';
import { useNDVStore } from '@/stores/ndv.store';
import type { AssignmentValue, INodeProperties } from 'n8n-workflow';
import { computed, ref } from 'vue';
import TypeSelect from './TypeSelect.vue';
import { useNDVStore } from '@/stores/ndv.store';
import { useRouter } from 'vue-router';
import { N8nIconButton } from 'n8n-design-system';

interface Props {
path: string;
Expand All @@ -32,8 +31,7 @@ const emit = defineEmits<{
}>();

const ndvStore = useNDVStore();
const router = useRouter();
const { resolveExpression } = useWorkflowHelpers({ router });
const environmentsStore = useEnvironmentsStore();

const assignmentTypeToNodeProperty = (
type: string,
Expand Down Expand Up @@ -76,47 +74,20 @@ const valueParameter = computed<INodeProperties>(() => {
};
});

const hint = computed(() => {
const { value } = assignment.value;
if (typeof value !== 'string' || !value.startsWith('=')) {
return '';
}

let result: Result<unknown, Error>;
try {
const resolvedValue = resolveExpression(
value,
undefined,
ndvStore.isInputParentOfActiveNode
? {
targetItem: ndvStore.expressionTargetItem ?? undefined,
inputNodeName: ndvStore.ndvInputNodeName,
inputRunIndex: ndvStore.ndvInputRunIndex,
inputBranchIndex: ndvStore.ndvInputBranchIndex,
}
: {},
) as unknown;

result = { ok: true, result: resolvedValue };
} catch (error) {
result = { ok: false, error };
}

const hasRunData =
!!useWorkflowsStore().workflowExecutionData?.data?.resultData?.runData[
ndvStore.activeNode?.name ?? ''
];
const value = computed(() => assignment.value.value);

return stringifyExpressionResult(result, hasRunData);
const resolvedAdditionalExpressionData = computed(() => {
return { $vars: environmentsStore.variablesAsObject };
});

const highlightHint = computed(() => Boolean(hint.value && ndvStore.getHoveringItem));
const { resolvedExpressionString, isExpression } = useResolvedExpression({
expression: value,
additionalData: resolvedAdditionalExpressionData,
});

const valueIsExpression = computed(() => {
const { value } = assignment.value;
const hint = computed(() => resolvedExpressionString.value);

return typeof value === 'string' && isExpression(value);
});
const highlightHint = computed(() => Boolean(hint.value && ndvStore.getHoveringItem));

const onAssignmentNameChange = (update: IUpdateInformation): void => {
assignment.value.name = update.value as string;
Expand All @@ -125,7 +96,7 @@ const onAssignmentNameChange = (update: IUpdateInformation): void => {
const onAssignmentTypeChange = (update: string): void => {
assignment.value.type = update;

if (update === 'boolean' && !valueIsExpression.value) {
if (update === 'boolean' && !isExpression.value) {
assignment.value.value = false;
}
};
Expand Down Expand Up @@ -160,7 +131,7 @@ const onBlur = (): void => {
icon="grip-vertical"
:class="[$style.iconButton, $style.defaultTopPadding, 'drag-handle']"
></N8nIconButton>
<n8n-icon-button
<N8nIconButton
v-if="!isReadOnly"
type="tertiary"
text
Expand All @@ -169,7 +140,7 @@ const onBlur = (): void => {
data-test-id="assignment-remove"
:class="[$style.iconButton, $style.extraTopPadding]"
@click="onRemove"
></n8n-icon-button>
></N8nIconButton>

<div :class="$style.inputs">
<InputTriple middle-width="100px">
Expand Down
41 changes: 23 additions & 18 deletions packages/editor-ui/src/components/ParameterInputList.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import type {
NodeParameterValueType,
} from 'n8n-workflow';
import { deepCopy, ADD_FORM_NOTICE } from 'n8n-workflow';
import { computed, defineAsyncComponent, onErrorCaptured, ref, watch } from 'vue';
import { computed, defineAsyncComponent, onErrorCaptured, ref, watch, type WatchSource } from 'vue';

import type { IUpdateInformation } from '@/Interface';

Expand Down Expand Up @@ -37,6 +37,7 @@ import { useRouter } from 'vue-router';
import { captureException } from '@sentry/vue';
import { N8nNotice, N8nIconButton, N8nInputLabel, N8nText, N8nIcon } from 'n8n-design-system';
import { useI18n } from '@/composables/useI18n';
import { computedWithControl } from '@vueuse/core';

const LazyFixedCollectionParameter = defineAsyncComponent(
async () => await import('./FixedCollectionParameter.vue'),
Expand Down Expand Up @@ -98,22 +99,29 @@ const nodeType = computed(() => {
return null;
});

const filteredParameters = computed(() => {
const parameters = props.parameters.filter((parameter: INodeProperties) =>
displayNodeParameter(parameter),
);
const filteredParameters = computedWithControl(
[() => props.parameters, () => props.nodeValues] as WatchSource[],
() => {
const parameters = props.parameters.filter((parameter: INodeProperties) =>
displayNodeParameter(parameter),
);

const activeNode = ndvStore.activeNode;
const activeNode = ndvStore.activeNode;

if (activeNode && activeNode.type === FORM_TRIGGER_NODE_TYPE) {
return updateFormTriggerParameters(parameters, activeNode.name);
}
if (activeNode && activeNode.type === WAIT_NODE_TYPE && activeNode.parameters.resume === 'form') {
return updateWaitParameters(parameters, activeNode.name);
}
if (activeNode && activeNode.type === FORM_TRIGGER_NODE_TYPE) {
return updateFormTriggerParameters(parameters, activeNode.name);
}
if (
activeNode &&
activeNode.type === WAIT_NODE_TYPE &&
activeNode.parameters.resume === 'form'
) {
return updateWaitParameters(parameters, activeNode.name);
}

return parameters;
});
return parameters;
},
);

const filteredParameterNames = computed(() => {
return filteredParameters.value.map((parameter) => parameter.name);
Expand Down Expand Up @@ -610,10 +618,7 @@ function getParameterValue<T extends NodeParameterValueType = NodeParameterValue
:is-read-only="isReadOnly"
@value-changed="valueChanged"
/>
<div
v-else-if="displayNodeParameter(parameter) && credentialsParameterIndex !== index"
class="parameter-item"
>
<div v-else-if="credentialsParameterIndex !== index" class="parameter-item">
<N8nIconButton
v-if="hideDelete !== true && !isReadOnly && !parameter.isNodeSetting"
type="tertiary"
Expand Down
11 changes: 7 additions & 4 deletions packages/editor-ui/src/components/ParameterInputWrapper.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@ import { createTestingPinia } from '@pinia/testing';
import ParameterInputWrapper from './ParameterInputWrapper.vue';
import { STORES } from '@/constants';
import { cleanupAppModals, createAppModals, SETTINGS_STORE_DEFAULT_STATE } from '@/__tests__/utils';
import { waitFor } from '@testing-library/vue';

vi.mock('@/composables/useWorkflowHelpers', () => {
return { useWorkflowHelpers: vi.fn(() => ({ resolveExpression: vi.fn(() => 'topSecret') })) };
});

describe('ParameterInputWrapper.vue', () => {
beforeEach(() => {
Expand All @@ -12,6 +17,7 @@ describe('ParameterInputWrapper.vue', () => {
afterEach(() => {
cleanupAppModals();
});

test('should resolve expression', async () => {
const { getByTestId } = renderComponent(ParameterInputWrapper, {
pinia: createTestingPinia({
Expand All @@ -34,16 +40,13 @@ describe('ParameterInputWrapper.vue', () => {
},
global: {
mocks: {
$workflowHelpers: {
resolveExpression: vi.fn(() => 'topSecret'),
},
$ndvStore: {
activeNode: vi.fn(() => ({ test: 'test' })),
},
},
},
});

expect(getByTestId('parameter-input-hint')).toHaveTextContent('[ERROR: ]');
await waitFor(() => expect(getByTestId('parameter-input-hint')).toHaveTextContent('topSecret'));
});
});
85 changes: 20 additions & 65 deletions packages/editor-ui/src/components/ParameterInputWrapper.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,16 @@ import {
type INodePropertyMode,
type IParameterLabel,
type NodeParameterValueType,
type Result,
} from 'n8n-workflow';

import { useWorkflowHelpers } from '@/composables/useWorkflowHelpers';
import { useResolvedExpression } from '@/composables/useResolvedExpression';
import useEnvironmentsStore from '@/stores/environments.ee.store';
import { useExternalSecretsStore } from '@/stores/externalSecrets.ee.store';
import { useNDVStore } from '@/stores/ndv.store';
import { stringifyExpressionResult } from '@/utils/expressions';
import { isValueExpression, parseResourceMapperFieldName } from '@/utils/nodeTypesUtils';
import type { EventBus } from 'n8n-design-system/utils';
import { createEventBus } from 'n8n-design-system/utils';
import { computed } from 'vue';
import { useRouter } from 'vue-router';
import { useWorkflowsStore } from '@/stores/workflows.store';

type Props = {
parameter: INodeProperties;
Expand Down Expand Up @@ -62,9 +58,6 @@ const emit = defineEmits<{
textInput: [value: IUpdateInformation];
}>();

const router = useRouter();
const workflowHelpers = useWorkflowHelpers({ router });

const ndvStore = useNDVStore();
const externalSecretsStore = useExternalSecretsStore();
const environmentsStore = useEnvironmentsStore();
Expand All @@ -73,8 +66,6 @@ const isExpression = computed(() => {
return isValueExpression(props.parameter, props.modelValue);
});

const activeNode = computed(() => ndvStore.activeNode);

const selectedRLMode = computed(() => {
if (
typeof props.modelValue !== 'object' ||
Expand Down Expand Up @@ -107,60 +98,9 @@ const targetItem = computed(() => ndvStore.expressionTargetItem);

const isInputParentOfActiveNode = computed(() => ndvStore.isInputParentOfActiveNode);

const evaluatedExpression = computed<Result<unknown, Error>>(() => {
const value = isResourceLocatorValue(props.modelValue)
? props.modelValue.value
: props.modelValue;

if (!activeNode.value || !isExpression.value || typeof value !== 'string') {
return { ok: false, error: new Error() };
}

try {
let opts: Parameters<typeof workflowHelpers.resolveExpression>[2] = {
isForCredential: props.isForCredential,
};
if (ndvStore.isInputParentOfActiveNode) {
opts = {
...opts,
targetItem: targetItem.value ?? undefined,
inputNodeName: ndvStore.ndvInputNodeName,
inputRunIndex: ndvStore.ndvInputRunIndex,
inputBranchIndex: ndvStore.ndvInputBranchIndex,
additionalKeys: resolvedAdditionalExpressionData.value,
};
}

if (props.isForCredential) opts.additionalKeys = resolvedAdditionalExpressionData.value;
const stringifyObject = props.parameter.type !== 'multiOptions';
return {
ok: true,
result: workflowHelpers.resolveExpression(value, undefined, opts, stringifyObject),
};
} catch (error) {
return { ok: false, error };
}
});

const evaluatedExpressionValue = computed(() => {
const evaluated = evaluatedExpression.value;
return evaluated.ok ? evaluated.result : null;
});

const evaluatedExpressionString = computed(() => {
const hasRunData =
!!useWorkflowsStore().workflowExecutionData?.data?.resultData?.runData[
ndvStore.activeNode?.name ?? ''
];
return stringifyExpressionResult(evaluatedExpression.value, hasRunData);
});

const expressionOutput = computed(() => {
if (isExpression.value && evaluatedExpressionString.value) {
return evaluatedExpressionString.value;
}

return null;
const expression = computed(() => {
if (!isExpression.value) return '';
return isResourceLocatorValue(props.modelValue) ? props.modelValue.value : props.modelValue;
});

const resolvedAdditionalExpressionData = computed(() => {
Expand All @@ -173,6 +113,21 @@ const resolvedAdditionalExpressionData = computed(() => {
};
});

const { resolvedExpression, resolvedExpressionString } = useResolvedExpression({
expression,
additionalData: resolvedAdditionalExpressionData,
isForCredential: props.isForCredential,
stringifyObject: props.parameter.type !== 'multiOptions',
});

const expressionOutput = computed(() => {
if (isExpression.value && resolvedExpressionString.value) {
return resolvedExpressionString.value;
}

return null;
});

const parsedParameterName = computed(() => {
return parseResourceMapperFieldName(props.parameter?.name ?? '');
});
Expand Down Expand Up @@ -216,7 +171,7 @@ function onTextInput(parameterData: IUpdateInformation) {
:error-highlight="errorHighlight"
:is-for-credential="isForCredential"
:event-source="eventSource"
:expression-evaluated="evaluatedExpressionValue"
:expression-evaluated="resolvedExpression"
:additional-expression-data="resolvedAdditionalExpressionData"
:label="label"
:rows="rows"
Expand Down
Loading
Loading