Skip to content

fix: insert patchNodeField literal replacements verbatim, escaping JS replacement patterns (v2.71.1) - #1013

Merged
czlonkowski merged 2 commits into
mainfrom
fix/patch-node-field-literal-replace
Aug 18, 2026
Merged

fix: insert patchNodeField literal replacements verbatim, escaping JS replacement patterns (v2.71.1)#1013
czlonkowski merged 2 commits into
mainfrom
fix/patch-node-field-literal-replace

Conversation

@czlonkowski

@czlonkowski czlonkowski commented Aug 18, 2026

Copy link
Copy Markdown
Owner

Fixes #1012.

The bug

patchNodeField in literal mode (the default) passed the user's replace string directly to String.prototype.replace(), which interprets JS replacement patterns inside the replacement text:

  • $' — splices in everything after the match
  • $` — everything before the match
  • $& — the match itself
  • $1 — a capture group reference (or literal, depending)

Patching a Code node with something as ordinary as const money = '$' + amount.toFixed(2) duplicated the rest of the node's source into the middle of the insertion, saved the workflow in that corrupted state — live, if the workflow was active — and reported success: true. In the reported incident this took down the confirm/decline webhook path of an active payment workflow.

The fix

Literal-mode replacements are now inserted verbatim — patchNodeField uses a single split().join() path for single-occurrence and replaceAll alike (the ambiguity guard leaves exactly one occurrence in the single case), and the __patch_find_replace path, which replaces only the first occurrence, uses a function replacer:

Literal replaceAll already used split().join() and was safe. Regex mode is unchanged by design: with regex: true, replacement patterns ($1 for capture groups, $$ for a literal $) are standard JS regex-replace semantics and now locked in by a test. The tool documentation states the literal-vs-regex distinction explicitly.

Tests

  • The exact $' case from the issue (money-string insertion) — fails on the old code with the spliced corruption, passes now
  • All replacement patterns ($&, $`, $', $1, $$, $<name>) kept literal in literal mode
  • $ in literal replaceAll (locks the already-safe path)
  • $1 capture group reference working in regex mode (locks intended semantics)
  • $' kept literal in __patch_find_replace

302 tests pass across the diff-engine suites (after a simplification pass merged two overlapping literal-mode tests); typecheck and build clean.

Not in this PR

The issue also proposes syntax-validating patched code fields (jsCode/pythonCode) before saving, refusing patches that would write a syntactically invalid body. That's a behavior change deserving its own discussion and PR — see the issue thread.

Reported with root cause, repro, and both candidate fixes by @NextLevelManagementAdvisors — thank you for the exemplary report.

Conceived by Romuald Członkowski - www.aiadvisors.pl/en

🤖 Generated with Claude Code

… replacement patterns (v2.71.1)

In literal mode, patchNodeField handed the replacement string straight to
String.prototype.replace(), so JS replacement patterns inside it were
interpreted: $' splices in everything after the match, $` everything
before it, $& the match itself. A replacement as ordinary as
"const money = '$' + amount.toFixed(2)" duplicated the rest of the
node's source into the insertion, saved the corrupted workflow (live,
if active), and reported success.

Literal single-occurrence replacement and the __patch_find_replace path
in updateNode now use a function replacer, so replacements are inserted
verbatim. Literal replaceAll (split/join) was already safe. Regex mode
keeps standard replacement-pattern semantics ($1, $$) by design; the
tool docs now state the distinction.

Reported with root cause and fix by @NextLevelManagementAdvisors.

Fixes #1012

Conceived by Romuald Członkowski - www.aiadvisors.pl/en

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 18, 2026 21:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a workflow-corruption bug in the diff engine’s string patching paths by ensuring literal-mode replacements are inserted verbatim (so JavaScript $ replacement patterns are not interpreted), which is critical for safely patching code-node fields like parameters.jsCode.

Changes:

  • Updated patchNodeField (literal, single-occurrence path) and updateNode’s legacy __patch_find_replace path to use a function replacer (replace(..., () => text)) to prevent JS replacement-pattern parsing.
  • Added targeted unit tests covering the $' corruption case, all common JS replacement patterns staying literal in literal mode, and preserved regex-mode $1 semantics.
  • Clarified tool documentation about literal vs regex replacement semantics and bumped version/changelog for v2.71.1.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/services/workflow-diff-engine.ts Switches literal single-replace paths to function replacers to keep replacement text literal and prevent $-pattern corruption.
tests/unit/services/workflow-diff-engine.test.ts Adds regression tests for $ replacement-pattern cases in literal mode and confirms intended regex-mode $1 behavior.
src/mcp/tool-docs/workflow_management/n8n-update-partial-workflow.ts Documents literal-mode verbatim replacement vs regex-mode JS replacement-pattern behavior.
package.json Bumps package version to 2.71.1.
package.runtime.json Bumps runtime package version to 2.71.1.
package-lock.json Aligns lockfile package versions to 2.71.1.
CHANGELOG.md Adds a 2.71.1 entry describing the fix and intended semantics.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Simplification pass on the #1012 fix: patchNodeField's literal mode now
uses split().join() for the single-occurrence case as well — the
ambiguity guard above leaves exactly one occurrence, so it is equivalent
and removes the branch. The __patch_find_replace path keeps the function
replacer (it replaces only the first occurrence). Two overlapping
literal-mode tests merged into one covering the #1012 repro plus every
replacement pattern; doc and changelog wording tightened to match.

Conceived by Romuald Członkowski - www.aiadvisors.pl/en

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 18, 2026 21:11
@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Test Results Summary

📊 Artifacts


Generated at Tue, 18 Aug 2026 21:19:20 GMT
Commit: 5b476f7
Run: #1489

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/services/workflow-diff-engine.ts:1204

  • Literal-mode patchNodeField now always uses split/join (even when the ambiguity guard ensures exactly one occurrence). For large string fields this allocates an array of substrings and does a second full scan, which is a measurable memory/CPU regression vs a single String.replace call. Consider keeping split/join (or replaceAll) only for replaceAll: true, and using a function replacer for the single-occurrence case: current = current.replace(patch.find, () => patch.replace) to stay verbatim while avoiding the extra allocations.
        // split/join inserts the replacement verbatim; String.replace would read
        // "$&", "$'" and friends in it as JS replacement patterns (#1012). Safe
        // for the single-occurrence case too: the checks above leave exactly one.
        current = current.split(patch.find).join(patch.replace);

@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@czlonkowski
czlonkowski merged commit 2e03049 into main Aug 18, 2026
16 checks passed
@czlonkowski
czlonkowski deleted the fix/patch-node-field-literal-replace branch August 18, 2026 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

patchNodeField: unescaped $ in replace corrupts code nodes via JS replacement patterns, and still reports success

2 participants