diff --git a/backend/windmill-queue/src/jobs.rs b/backend/windmill-queue/src/jobs.rs index a1bd8034f0..c7c119a5a7 100644 --- a/backend/windmill-queue/src/jobs.rs +++ b/backend/windmill-queue/src/jobs.rs @@ -4671,6 +4671,12 @@ pub async fn check_debouncing_within_limits( } } +/// Whether the tag's queue name is computed from the job's arguments, so that a caller holding +/// arguments it could not build knows the tag cannot be built either. +pub fn tag_reads_args(tag: &str) -> bool { + RE_ARG_TAG.is_match(tag) +} + pub fn interpolate_args(x: String, args: &PushArgs, workspace_id: &str) -> String { // Save this value to avoid parsing twice let workspaced = x.as_str().replace("$workspace", workspace_id).to_string(); diff --git a/backend/windmill-types/src/jobs.rs b/backend/windmill-types/src/jobs.rs index db20bc1251..5190efe380 100644 --- a/backend/windmill-types/src/jobs.rs +++ b/backend/windmill-types/src/jobs.rs @@ -632,6 +632,28 @@ pub enum JobPayload { }, } +impl JobPayload { + /// Whether the payload itself declares a dedicated worker, in which case `push` replaces + /// whatever tag it is handed and the caller's tag never reaches the queue. + /// + /// This reads what the payload carries, not what `push` will conclude: a `SingleStepFlow` + /// loads the flag from the script row at push time and reports `false` here. That only + /// matters to a caller reasoning about the tag, and for those the answer is the same either + /// way, since `push` replaces the tag in exactly the case this misses. + pub fn is_dedicated_worker(&self) -> bool { + let dedicated_worker = match self { + JobPayload::ScriptHash { dedicated_worker, .. } + | JobPayload::FlowScript { dedicated_worker, .. } + | JobPayload::Dependencies { dedicated_worker, .. } + | JobPayload::FlowDependencies { dedicated_worker, .. } + | JobPayload::Flow { dedicated_worker, .. } => dedicated_worker, + JobPayload::Code(raw) => &raw.dedicated_worker, + _ => &None, + }; + dedicated_worker.is_some_and(|x| x) + } +} + #[derive(Clone, Serialize, Deserialize, Debug)] pub struct SkipHandler { pub path: String, diff --git a/backend/windmill-worker/src/worker.rs b/backend/windmill-worker/src/worker.rs index 350f0c5a61..bc9cc42771 100644 --- a/backend/windmill-worker/src/worker.rs +++ b/backend/windmill-worker/src/worker.rs @@ -3532,7 +3532,13 @@ pub async fn run_worker( job.kind, JobKind::Script | JobKind::Preview | JobKind::FlowScript ) { - if !dedicated_workers.is_empty() { + // A job carrying a pre-run error never runs its code: it only has to be + // pulled so `handle_queued_job` can fail it. Both hand-off paths below + // dispatch by path and return before that check, so a job sent down them + // would run with whatever arguments survived the failure. + let fails_before_running = job.pre_run_error.is_some(); + + if !dedicated_workers.is_empty() && !fails_before_running { let dedicated_worker_tx = job.runnable_path.as_ref().and_then(|path| { // For flow steps inside branches/loops, runnable_path includes // nesting segments (e.g. f/flow/branchone-0/a) but the dedicated @@ -3577,7 +3583,7 @@ pub async fn run_worker( NextJob::Http(_) => None, }; - if let Some(flow_runners) = flow_runners { + if let Some(flow_runners) = flow_runners.filter(|_| !fails_before_running) { let key_o = job.flow_step_id.as_ref().map(|x| x.to_string()); if let Some(key) = key_o { if let Some(flow_runner_tx) = flow_runners.runners.get(&key) { diff --git a/backend/windmill-worker/src/worker_flow.rs b/backend/windmill-worker/src/worker_flow.rs index bd39afa8c7..34f46307cf 100644 --- a/backend/windmill-worker/src/worker_flow.rs +++ b/backend/windmill-worker/src/worker_flow.rs @@ -70,9 +70,9 @@ use windmill_queue::schedule::get_schedule_opt; use windmill_queue::{ add_completed_job, add_completed_job_error, append_logs, get_mini_pulled_job, insert_concurrency_key_capped, interpolate_args, - report_error_to_workspace_handler_or_critical_side_channel, try_schedule_next_job, CanceledBy, - FlowRunners, MiniCompletedJob, MiniPulledJob, PushArgs, PushIsolationLevel, SameWorkerPayload, - WrappedError, + report_error_to_workspace_handler_or_critical_side_channel, tag_reads_args, + try_schedule_next_job, CanceledBy, FlowRunners, MiniCompletedJob, MiniPulledJob, PushArgs, + PushIsolationLevel, SameWorkerPayload, WrappedError, }; use windmill_audit::audit_oss::audit_log; @@ -4403,6 +4403,23 @@ async fn push_next_flow_job( payload_tag.tag.as_deref(), ); + // `push_args` is empty once the input transforms failed, so a tag reading `$args[...]` + // interpolates to a queue nobody serves and the step sits there instead of reporting + // the error. Send it to the flow's tag, which a worker is provably serving right now. + // + // A step handed over by id, or one whose tag `push` replaces, never reaches a worker + // through its tag, so rewriting theirs would be noise. + let step_is_pulled_by_tag = !continue_on_same_worker + && !continue_with_runners + && !payload_tag.payload.is_dedicated_worker(); + let reroute_to_flow_tag = + err.is_some() && step_is_pulled_by_tag && tag.as_deref().is_some_and(tag_reads_args); + let tag = if reroute_to_flow_tag { + Some(flow_job.tag.clone()) + } else { + tag + }; + let (email, permissioned_as) = if let Some(on_behalf_of) = payload_tag.on_behalf_of.as_ref() { (&on_behalf_of.email, on_behalf_of.permissioned_as.clone()) @@ -4421,8 +4438,7 @@ async fn push_next_flow_job( .as_deref() .filter(|t| !t.is_empty() && *t != flow_job.tag.as_str()) { - let is_super_admin = - windmill_common::auth::is_super_admin_email(db, email).await?; + let is_super_admin = windmill_common::auth::is_super_admin_email(db, email).await?; check_tag_available_for_workspace_internal( db, &flow_job.workspace_id, @@ -6155,9 +6171,7 @@ pub async fn script_to_payload( .await? .prefetch_cached(&db) .await?; - let on_behalf_of = script_info - .on_behalf_of(&flow_job.workspace_id, db) - .await?; + let on_behalf_of = script_info.on_behalf_of(&flow_job.workspace_id, db).await?; let ScriptHashInfo { tag, cache_ttl, diff --git a/frontend/package-lock.json b/frontend/package-lock.json index 82f5133ddb..0eed58d7d8 100644 --- a/frontend/package-lock.json +++ b/frontend/package-lock.json @@ -29,6 +29,7 @@ "@windmill-labs/svelte-dnd-action": "^0.9.44", "@xterm/addon-fit": "^0.10.0", "@xyflow/svelte": "^1.0.0", + "acorn": "^8.15.0", "ag-charts-community": "^9.0.1", "ag-charts-enterprise": "^9.0.1", "ag-grid-community": "^31.3.4", @@ -1754,7 +1755,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1771,7 +1771,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1788,7 +1787,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1805,7 +1803,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1822,7 +1819,6 @@ "cpu": [ "arm" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1839,7 +1835,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1856,7 +1851,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1873,7 +1867,6 @@ "cpu": [ "ppc64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1890,7 +1883,6 @@ "cpu": [ "s390x" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1907,7 +1899,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1924,7 +1915,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1941,7 +1931,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1958,7 +1947,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1975,7 +1963,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -7582,7 +7569,7 @@ "version": "1.21.7", "resolved": "https://registry.npmjs.org/jiti/-/jiti-1.21.7.tgz", "integrity": "sha512-/imKNG4EbWNrVjoNC/1H5/9GFy+tqjGBHCaSsN+P2RnPqjsLmv6UD3Ej+Kj8nBWaRAwyk7kK5ZUc+OEatnTR3A==", - "dev": true, + "devOptional": true, "license": "MIT", "bin": { "jiti": "bin/jiti.js" @@ -8278,7 +8265,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "MPL-2.0", "optional": true, "os": [ @@ -8299,7 +8285,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "MPL-2.0", "optional": true, "os": [ @@ -8320,7 +8305,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "MPL-2.0", "optional": true, "os": [ @@ -8341,7 +8325,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "MPL-2.0", "optional": true, "os": [ @@ -8362,7 +8345,6 @@ "cpu": [ "arm" ], - "dev": true, "license": "MPL-2.0", "optional": true, "os": [ @@ -8383,7 +8365,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "MPL-2.0", "optional": true, "os": [ @@ -8404,7 +8385,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "MPL-2.0", "optional": true, "os": [ @@ -8425,7 +8405,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "MPL-2.0", "optional": true, "os": [ @@ -8446,7 +8425,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "MPL-2.0", "optional": true, "os": [ @@ -8467,7 +8445,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "MPL-2.0", "optional": true, "os": [ @@ -8488,7 +8465,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "MPL-2.0", "optional": true, "os": [ @@ -13194,21 +13170,6 @@ } } }, - "node_modules/svelte-check/node_modules/picomatch": { - "version": "4.0.5", - "resolved": "https://registry.npmjs.org/picomatch/-/picomatch-4.0.5.tgz", - "integrity": "sha512-RvwwcruNjI1ncT5xRakeyS9Lf8lcItv34KD+aif+VH9kduAyfYBipGh12274xtenIPZ119/R9BdTBa8gAwSh0A==", - "dev": true, - "license": "MIT", - "optional": true, - "peer": true, - "engines": { - "node": ">=12" - }, - "funding": { - "url": "https://github.com/sponsors/jonschlinkert" - } - }, "node_modules/svelte-eslint-parser": { "version": "0.43.0", "resolved": "https://registry.npmjs.org/svelte-eslint-parser/-/svelte-eslint-parser-0.43.0.tgz", @@ -13988,7 +13949,7 @@ "version": "5.9.3", "resolved": "https://registry.npmjs.org/typescript/-/typescript-5.9.3.tgz", "integrity": "sha512-jl1vZzPDinLr9eUt3J/t7V6FgNEw9QjvBPdysz9KfQDD41fQrC2Y4vKQdiaUpFT4bXlb1RHhLpp8wtm6M5TgSw==", - "dev": true, + "devOptional": true, "license": "Apache-2.0", "bin": { "tsc": "bin/tsc", diff --git a/frontend/package.json b/frontend/package.json index 5b6dd5c000..895f218ba7 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -105,6 +105,7 @@ "@windmill-labs/svelte-dnd-action": "^0.9.44", "@xterm/addon-fit": "^0.10.0", "@xyflow/svelte": "^1.0.0", + "acorn": "^8.15.0", "ag-charts-community": "^9.0.1", "ag-charts-enterprise": "^9.0.1", "ag-grid-community": "^31.3.4", diff --git a/frontend/src/lib/components/InputTransformForm.svelte b/frontend/src/lib/components/InputTransformForm.svelte index 630d052ce4..d766c06603 100644 --- a/frontend/src/lib/components/InputTransformForm.svelte +++ b/frontend/src/lib/components/InputTransformForm.svelte @@ -30,6 +30,7 @@ import type { InputTransform } from '$lib/gen' import TemplateEditor from './TemplateEditor.svelte' import { setInputCat as computeInputCat, isCodeInjection } from '$lib/utils' + import { escapeTemplateBackticks } from '$lib/utils/templateLiteral' import { FunctionSquare, InfoIcon } from 'lucide-svelte' import { getResourceTypes } from './resourceTypesStore' import type { FlowCopilotContext } from './copilot/flow' @@ -253,7 +254,7 @@ arg.expr = getDefaultExpr( argName, previousModuleId, - `\`${rawValue.toString().replaceAll('`', '\\`')}\`` + `\`${escapeTemplateBackticks(rawValue.toString())}\`` ) arg.type = 'javascript' propertyType = 'static' @@ -687,7 +688,7 @@ argName, previousModuleId, staticTemplate - ? `\`${arg?.value?.toString().replaceAll('`', '\\`') ?? ''}\`` + ? `\`${escapeTemplateBackticks(arg?.value?.toString() ?? '')}\`` : arg.value ? '(' + JSON.stringify(arg?.value, null, 4) + ')' : '' diff --git a/frontend/src/lib/components/ModuleTest.svelte b/frontend/src/lib/components/ModuleTest.svelte index 2adba428ee..50e915a4cb 100644 --- a/frontend/src/lib/components/ModuleTest.svelte +++ b/frontend/src/lib/components/ModuleTest.svelte @@ -50,14 +50,19 @@ let jobProgressReset: () => void = () => {} let stepHistoryLoader = getStepHistoryLoaderContext() + // Every explicit run re-evaluates the args with errors surfaced. The reactive evaluations + // that follow each flow edit stay quiet, so without this a failing expression is silently + // `undefined` in what the run is built from. Manually edited args are preserved across the + // refresh by `initializeFromSchema`. export function runTestWithStepArgs() { - const args = stepsInputArgs.getStepArgs(mod.id) - runTest(args) - } - - export function loadArgsAndRunTest() { - stepsInputArgs?.updateStepArgs(mod.id, flowStateStore.val, flowStore?.val, previewArgs?.val) - runTestWithStepArgs() + stepsInputArgs?.updateStepArgs( + mod.id, + flowStateStore.val, + flowStore?.val, + previewArgs?.val, + true + ) + runTest(stepsInputArgs.getStepArgs(mod.id)) } // A step's timeout is an InputTransform. Only a static numeric value can be applied diff --git a/frontend/src/lib/components/apps/components/helpers/InputValue.svelte b/frontend/src/lib/components/apps/components/helpers/InputValue.svelte index aba0569228..7ec2336d3b 100644 --- a/frontend/src/lib/components/apps/components/helpers/InputValue.svelte +++ b/frontend/src/lib/components/apps/components/helpers/InputValue.svelte @@ -19,6 +19,7 @@ import { computeGlobalContext, eval_like } from './eval' import { deepEqual } from 'fast-equals' import { deepMergeWithPriority, isCodeInjection, readFieldsRecursively } from '$lib/utils' + import { escapeTemplateBackticks } from '$lib/utils/templateLiteral' import sum from 'hash-sum' import { createDispatcherIfMounted } from '$lib/createDispatcherIfMounted' @@ -271,7 +272,7 @@ if ((input.type === 'template' || input.type == 'templatev2') && isCodeInjection(input.eval)) { try { const r = await eval_like( - '`' + input.eval.replaceAll('`', '\\`') + '`', + '`' + escapeTemplateBackticks(input.eval) + '`', computeGlobalContext($worldStore, id, fullContext), $stateStore, $mode == 'dnd', diff --git a/frontend/src/lib/components/flows/map/FlowModuleSchemaItem.svelte b/frontend/src/lib/components/flows/map/FlowModuleSchemaItem.svelte index 6aeaa14f35..e6d7223fcb 100644 --- a/frontend/src/lib/components/flows/map/FlowModuleSchemaItem.svelte +++ b/frontend/src/lib/components/flows/map/FlowModuleSchemaItem.svelte @@ -464,7 +464,7 @@ btnClasses="px-1 py-1.5 bg-surface" on:click={() => { outputPicker?.toggleOpen(true) - moduleTest?.loadArgsAndRunTest() + moduleTest?.runTestWithStepArgs() }} dropdownItems={[ { diff --git a/frontend/src/lib/components/flows/stepsInputArgs.svelte.ts b/frontend/src/lib/components/flows/stepsInputArgs.svelte.ts index 8aa7a1f69f..6db6a4eb23 100644 --- a/frontend/src/lib/components/flows/stepsInputArgs.svelte.ts +++ b/frontend/src/lib/components/flows/stepsInputArgs.svelte.ts @@ -125,12 +125,15 @@ export class StepsInputArgs { initializeFromSchema( mod: FlowModule, schema: { properties?: Record }, - pickableProperties: PickableProperties | undefined + pickableProperties: PickableProperties | undefined, + // Off for the reactive re-evaluations that follow every flow edit; on for an explicit + // run, where a failing expression would otherwise become `undefined` unseen. + showError: boolean = false ) { const args = Object.fromEntries( Object.keys(schema.properties ?? {}).map((k) => [ k, - evalValue(k, mod, pickableProperties, false) + evalValue(k, mod, pickableProperties, showError) ]) ) @@ -158,14 +161,16 @@ export class StepsInputArgs { id: string, flowState: FlowState | undefined, flow: OpenFlow | undefined, - previewArgs: Record | undefined + previewArgs: Record | undefined, + showError: boolean = false ) { if (id === 'failure' && flow && flow.value.failure_module && flowState) { const picker = getFailureStepPropPicker(flowState, flow, previewArgs) this.initializeFromSchema( flow.value.failure_module, flowState['failure']?.schema ?? {}, - picker.pickableProperties + picker.pickableProperties, + showError ) return } @@ -193,7 +198,12 @@ export class StepsInputArgs { false ) const pickableProperties = stepPropPicker.pickableProperties - this.initializeFromSchema(modules[0], flowState[id]?.schema ?? {}, pickableProperties) + this.initializeFromSchema( + modules[0], + flowState[id]?.schema ?? {}, + pickableProperties, + showError + ) } removeExtraKey(moduleId: string, keys: string[]) { diff --git a/frontend/src/lib/components/flows/utils.svelte.ts b/frontend/src/lib/components/flows/utils.svelte.ts index d62138868f..8d8e4366e5 100644 --- a/frontend/src/lib/components/flows/utils.svelte.ts +++ b/frontend/src/lib/components/flows/utils.svelte.ts @@ -12,6 +12,7 @@ import { } from '$lib/gen' import { workspaceStore } from '$lib/stores' import { cleanExpr, emptySchema } from '$lib/utils' +import { unescapeTemplateBackticks } from '$lib/utils/templateLiteral' import { get } from 'svelte/store' import type { FlowModuleState } from './flowState' import { type PickableProperties, dfs } from './previousResults' @@ -219,7 +220,7 @@ export function codeToStaticTemplate(code?: string): string | undefined { if (lines.length == 1) { const line = lines[0].trim() if (line[0] == '`' && line.charAt(line.length - 1) == '`') { - return line.slice(1, line.length - 1).replaceAll('\\`', '`') + return unescapeTemplateBackticks(line.slice(1, line.length - 1)) } else { return `\$\{${line}\}` } diff --git a/frontend/src/lib/utils/templateLiteral.test.ts b/frontend/src/lib/utils/templateLiteral.test.ts new file mode 100644 index 0000000000..3aa16ecf38 --- /dev/null +++ b/frontend/src/lib/utils/templateLiteral.test.ts @@ -0,0 +1,133 @@ +import { describe, it, expect } from 'vitest' +import { escapeTemplateBackticks, unescapeTemplateBackticks } from './templateLiteral' + +// Template mode stores its value as a JS template literal, so backticks in the text have to be +// escaped. Escaping them inside `${...}` too is what broke nested template literals: a backslash +// is a syntax error in expression position. +describe('escapeTemplateBackticks', () => { + const nested = + '--input ${flow_input.iter.value}${results.config ? ` --config ${results.config}` : ""} --host ${flow_input.hostname}' + + it('leaves a nested template literal inside an interpolation intact', () => { + const expr = '`' + escapeTemplateBackticks(nested) + '`' + expect(expr).not.toContain('\\`') + expect( + new Function('flow_input', 'results', 'return ' + expr)( + { iter: { value: 'data.csv' }, hostname: 'host1' }, + { config: '/tmp/cfg.json' } + ) + ).toBe('--input data.csv --config /tmp/cfg.json --host host1') + }) + + it('still escapes a backtick in the literal text', () => { + expect(escapeTemplateBackticks('a ` b')).toBe('a \\` b') + expect(escapeTemplateBackticks('a ` ${x} ` b')).toBe('a \\` ${x} \\` b') + }) + + it('leaves an escaped interpolation as literal text', () => { + // `\\${...}` is escaped in the template source, so the backticks inside it are literal + // text and still need escaping. + expect(escapeTemplateBackticks('\\${foo `bar`}')).toBe('\\${foo \\`bar\\`}') + expect(unescapeTemplateBackticks('\\${foo \\`bar\\`}')).toBe('\\${foo `bar`}') + expect( + () => new Function('return `' + escapeTemplateBackticks('\\${foo `bar`}') + '`') + ).not.toThrow() + }) + + // Braces, quotes, regex literals and comments all hide backticks and braces from anything + // short of a real lexer, which is why the parser decides. + it('handles text only a lexer can read correctly', () => { + const inputs = [ + '${ x["}"] } `', + "${ f({ a: '`' }) } `", + "${ x.replace(/'/g, '') } `", + '${ /* ` */ x } `', + '{"match": ${/{/.test(flow_input.x)}, "literal": "`x`"}' + ] + for (const v of inputs) { + expect(() => new Function('return `' + escapeTemplateBackticks(v) + '`')).not.toThrow() + expect(unescapeTemplateBackticks(escapeTemplateBackticks(v))).toBe(v) + } + }) + + // The failure that matters most is not a syntax error but literal text quietly becoming code: + // here the author's `+ flow_input.y +` must stay text rather than being evaluated. + it('never lets literal text escape into the expression', () => { + const v = '{"match": ${/{/.test(flow_input.x)}, "literal": "` + flow_input.y + `"}' + const evaluated = new Function('flow_input', 'return `' + escapeTemplateBackticks(v) + '`')({ + x: 'x', + y: 'LEAKED' + }) + expect(evaluated).not.toContain('LEAKED') + expect(evaluated).toContain('` + flow_input.y + `') + }) + + // An expression the old blanket rule broke — it escaped backticks inside `${...}`, which + // does not parse — comes back as the author typed it, instead of showing the backslashes + // and escaping them one deeper on every save. + it('heals an expression the old rule broke', () => { + const broken = '-p ${a}${b ? \\` --x ${c}\\` : ""}' + const clean = '-p ${a}${b ? ` --x ${c}` : ""}' + expect(unescapeTemplateBackticks(broken)).toBe(clean) + expect(escapeTemplateBackticks(clean)).toBe(clean) + }) + + // A text that already parses is left alone: an over-escaped legacy value and a backslash the + // author wrote are the same bytes, so healing on looks alone would drop a real character. + it('leaves an expression that already parses alone', () => { + const run = (body: string) => new Function('return `' + body + '`')() + for (const stored of ['${"\\`"}', '${"a\\\\`"}']) { + expect(unescapeTemplateBackticks(stored)).toBe(stored) + expect(run(escapeTemplateBackticks(unescapeTemplateBackticks(stored)))).toBe(run(stored)) + } + }) + + // ...but a backtick escaped inside a nested template belongs there and must survive. + it('leaves an escaped backtick that is inside a nested template alone', () => { + const stored = '${cond ? `a\\`b` : ""}' + expect(unescapeTemplateBackticks(stored)).toBe(stored) + }) + + // Escapes the author wrote inside a nested template are not the old rule's doing, and + // stripping them changes what the expression means — here into chained tagged templates, + // which throw. Only a text whose backticks are *all* escaped came from the old rule. + it('leaves escapes that belong to a nested template alone', () => { + const run = (body: string) => new Function('flag', 'value', 'return `' + body + '`')(true, 'X') + for (const stored of ['${flag ? `\\`\\`${value}\\`\\`` : ""}', '${flag ? `a\\`b` : ""}']) { + expect(unescapeTemplateBackticks(stored)).toBe(stored) + expect(escapeTemplateBackticks(unescapeTemplateBackticks(stored))).toBe(stored) + expect(run(stored)).toBe(run(escapeTemplateBackticks(unescapeTemplateBackticks(stored)))) + } + }) + + it('round-trips through unescapeTemplateBackticks', () => { + for (const v of [ + nested, + 'a ` b', + '${ x["}"] } `', + 'plain', + '${a}${b}', + '\\${a}', + '${cond ? `a\\`b` : ""}' + ]) { + expect(unescapeTemplateBackticks(escapeTemplateBackticks(v))).toBe(v) + } + }) + // The guarantee that matters: opening a flow in the editor and saving it back must not change + // the stored expression, including for a value that only the all-or-nothing fallback can + // handle. + it('never rewrites the stored expression on a view/save cycle', () => { + const inputs = [ + '{"match": ${/{/.test(flow_input.x)}, "literal": "`x`"}', + '${cond ? `a\\`b` : ""}', + "${ x.replace(/'/g, '') } `", + 'a ` b', + '${ /* ` */ x } `' + ] + for (const v of inputs) { + const stored = escapeTemplateBackticks(v) + expect(() => new Function('return `' + stored + '`')).not.toThrow() + expect(escapeTemplateBackticks(unescapeTemplateBackticks(stored))).toBe(stored) + } + }) +}) diff --git a/frontend/src/lib/utils/templateLiteral.ts b/frontend/src/lib/utils/templateLiteral.ts new file mode 100644 index 0000000000..388169ea95 --- /dev/null +++ b/frontend/src/lib/utils/templateLiteral.ts @@ -0,0 +1,59 @@ +import { parseExpressionAt } from 'acorn' + +/** + * Template mode stores what the author typed as a JS template literal, so the text is spliced + * between backticks. A backtick in the literal part has to be escaped or it ends the literal + * early — but one inside a `${...}` must not be, since a backslash is a syntax error in + * expression position and a nested template literal there is legitimate. + * + * Telling those apart means knowing where each `${...}` ends, which needs a real JS lexer: + * regex literals, comments and nested templates all hide braces and backticks from anything + * simpler. So rather than escaping selectively, ask the parser whether the text already reads as + * one template literal. If it does, it needs no escaping at all; if it does not, escape every + * backtick, which is what this did before nested templates were supported. + * + * Known limitation: a value mixing a bare literal backtick with a nested template cannot be + * expressed either way, and gets the all-or-nothing fallback. Escaping the literal one by hand + * makes the whole value parse and it is then kept verbatim. + */ +function isCompleteTemplateBody(text: string): boolean { + const source = '`' + text + '`' + try { + const node = parseExpressionAt(source, 0, { ecmaVersion: 'latest' }) + // The type is what rejects a body that closes its own literal early: `` ` + evil() + ` `` + // parses, but as a concatenation, and would evaluate the author's literal text. The span + // rejects a body that stops short, like `` `a` x ``. + return node.type === 'TemplateLiteral' && node.start === 0 && node.end === source.length + } catch { + return false + } +} + +/** Escape `text` so it can be wrapped in backticks and mean what the author typed. */ +export function escapeTemplateBackticks(text: string): string { + // No backtick means nothing to escape and nothing to decide, which is every ordinary value. + if (!text.includes('`')) { + return text + } + return isCompleteTemplateBody(text) ? text : text.replaceAll('`', '\\`') +} + +/** Inverse of {@link escapeTemplateBackticks}, for turning an expression back into a template. */ +export function unescapeTemplateBackticks(text: string): string { + if (!text.includes('`')) { + return text + } + const unescaped = text.replaceAll('\\`', '`') + if (escapeTemplateBackticks(unescaped) === text) { + return unescaped + } + // Only an expression the old blanket rule *broke* is healed: it escaped backticks inside + // `${...}` too, which does not parse, while the unescaped form does. A text that already + // parses is left alone even if it looks over-escaped, because the two are indistinguishable + // from the text alone and guessing changes what the expression means — `${"a\\\\`"}` is a + // backslash the author wrote, not one the old rule added. + if (!isCompleteTemplateBody(text) && isCompleteTemplateBody(unescaped)) { + return unescaped + } + return text +}