S&R improved filename sanitizing (#2784)

Co-authored-by: typpos <28550406+typpos@users.noreply.github.com>

src/extensions/core/saveImageExtraOutput.ts

@@ -1,6 +1,7 @@
 // @ts-strict-ignore
+import { applyTextReplacements } from '@/utils/searchAndReplace'
+
 import { app } from '../../scripts/app'
-import { applyTextReplacements } from '../../scripts/utils'
 
 // Use widget values and dates in output filenames
 
@@ -21,7 +22,7 @@ app.registerExtension({
 
         const widget = this.widgets.find((w) => w.name === 'filename_prefix')
         widget.serializeValue = () => {
-          return applyTextReplacements(app, widget.value)
+          return applyTextReplacements(app.graph.nodes, widget.value)
         }
 
         return r

src/extensions/core/widgetInputs.ts

@@ -14,10 +14,11 @@ import type { CanvasMouseEvent } from '@comfyorg/litegraph/dist/types/events'
 
 import type { InputSpec } from '@/schemas/nodeDefSchema'
 import { app } from '@/scripts/app'
-import { applyTextReplacements, clone } from '@/scripts/utils'
+import { clone } from '@/scripts/utils'
 import { ComfyWidgets, addValueControlWidgets } from '@/scripts/widgets'
 import { useNodeDefStore } from '@/stores/nodeDefStore'
 import { useSettingStore } from '@/stores/settingStore'
+import { applyTextReplacements } from '@/utils/searchAndReplace'
 import { isPrimitiveNode } from '@/utils/typeGuardUtil'
 
 const CONVERTED_TYPE = 'converted-widget'
@@ -73,7 +74,7 @@ export class PrimitiveNode extends LGraphNode {
     ]
     let v = this.widgets?.[0].value
     if (v && this.properties[replacePropertyName]) {
-      v = applyTextReplacements(app, v as string)
+      v = applyTextReplacements(app.graph.nodes, v as string)
     }
 
     // For each output link copy our value over the original widget value

src/scripts/utils.ts

@@ -1,33 +1,10 @@
 // @ts-strict-ignore
+import { applyTextReplacements as _applyTextReplacements } from '@/utils/searchAndReplace'
+
 import { api } from './api'
 import type { ComfyApp } from './app'
 import { $el } from './ui'
 
-// Simple date formatter
-const parts = {
-  d: (d) => d.getDate(),
-  M: (d) => d.getMonth() + 1,
-  h: (d) => d.getHours(),
-  m: (d) => d.getMinutes(),
-  s: (d) => d.getSeconds()
-}
-const format =
-  Object.keys(parts)
-    .map((k) => k + k + '?')
-    .join('|') + '|yyy?y?'
-
-function formatDate(text: string, date: Date) {
-  return text.replace(new RegExp(format, 'g'), (text: string): string => {
-    if (text === 'yy') return (date.getFullYear() + '').substring(2)
-    if (text === 'yyyy') return date.getFullYear().toString()
-    if (text[0] in parts) {
-      const p = parts[text[0]](date)
-      return (p + '').padStart(text.length, '0')
-    }
-    return text
-  })
-}
-
 export function clone<T>(obj: T): T {
   try {
     if (typeof structuredClone !== 'undefined') {
@@ -40,49 +17,12 @@ export function clone<T>(obj: T): T {
   return JSON.parse(JSON.stringify(obj))
 }
 
+/**
+ * @deprecated Use `applyTextReplacements` from `@/utils/searchAndReplace` instead
+ * There are external callers to this function, so we need to keep it for now
+ */
 export function applyTextReplacements(app: ComfyApp, value: string): string {
-  return value.replace(/%([^%]+)%/g, function (match, text) {
-    const split = text.split('.')
-    if (split.length !== 2) {
-      // Special handling for dates
-      if (split[0].startsWith('date:')) {
-        return formatDate(split[0].substring(5), new Date())
-      }
-
-      if (text !== 'width' && text !== 'height') {
-        // Dont warn on standard replacements
-        console.warn('Invalid replacement pattern', text)
-      }
-      return match
-    }
-
-    // Find node with matching S&R property name
-    let nodes = app.graph.nodes.filter(
-      (n) => n.properties?.['Node name for S&R'] === split[0]
-    )
-    // If we cant, see if there is a node with that title
-    if (!nodes.length) {
-      nodes = app.graph.nodes.filter((n) => n.title === split[0])
-    }
-    if (!nodes.length) {
-      console.warn('Unable to find node', split[0])
-      return match
-    }
-
-    if (nodes.length > 1) {
-      console.warn('Multiple nodes matched', split[0], 'using first match')
-    }
-
-    const node = nodes[0]
-
-    const widget = node.widgets?.find((w) => w.name === split[1])
-    if (!widget) {
-      console.warn('Unable to find widget', split[1], 'on node', split[0], node)
-      return match
-    }
-
-    return ((widget.value ?? '') + '').replaceAll(/\/|\\/g, '_')
-  })
+  return _applyTextReplacements(app.graph.nodes, value)
 }
 
 export async function addStylesheet(

src/utils/formatUtil.ts

@@ -272,3 +272,28 @@ export function parseFilePath(filepath: string): {
     subfolder: normalizedPath.slice(0, lastSlashIndex)
   }
 }
+
+// Simple date formatter
+const parts = {
+  d: (d: Date) => d.getDate(),
+  M: (d: Date) => d.getMonth() + 1,
+  h: (d: Date) => d.getHours(),
+  m: (d: Date) => d.getMinutes(),
+  s: (d: Date) => d.getSeconds()
+}
+const format =
+  Object.keys(parts)
+    .map((k) => k + k + '?')
+    .join('|') + '|yyy?y?'
+
+export function formatDate(text: string, date: Date) {
+  return text.replace(new RegExp(format, 'g'), (text: string): string => {
+    if (text === 'yy') return (date.getFullYear() + '').substring(2)
+    if (text === 'yyyy') return date.getFullYear().toString()
+    if (text[0] in parts) {
+      const p = parts[text[0] as keyof typeof parts](date)
+      return (p + '').padStart(text.length, '0')
+    }
+    return text
+  })
+}

src/utils/searchAndReplace.ts

@@ -0,0 +1,54 @@
+import type { LGraphNode } from '@comfyorg/litegraph'
+
+import { formatDate } from '@/utils/formatUtil'
+
+export function applyTextReplacements(
+  allNodes: LGraphNode[],
+  value: string
+): string {
+  return value.replace(/%([^%]+)%/g, function (match, text) {
+    const split = text.split('.')
+    if (split.length !== 2) {
+      // Special handling for dates
+      if (split[0].startsWith('date:')) {
+        return formatDate(split[0].substring(5), new Date())
+      }
+
+      if (text !== 'width' && text !== 'height') {
+        // Dont warn on standard replacements
+        console.warn('Invalid replacement pattern', text)
+      }
+      return match
+    }
+
+    // Find node with matching S&R property name
+    let nodes = allNodes.filter(
+      (n) => n.properties?.['Node name for S&R'] === split[0]
+    )
+    // If we cant, see if there is a node with that title
+    if (!nodes.length) {
+      nodes = allNodes.filter((n) => n.title === split[0])
+    }
+    if (!nodes.length) {
+      console.warn('Unable to find node', split[0])
+      return match
+    }
+
+    if (nodes.length > 1) {
+      console.warn('Multiple nodes matched', split[0], 'using first match')
+    }
+
+    const node = nodes[0]
+
+    const widget = node.widgets?.find((w) => w.name === split[1])
+    if (!widget) {
+      console.warn('Unable to find widget', split[1], 'on node', split[0], node)
+      return match
+    }
+    return ((widget.value ?? '') + '').replaceAll(
+      // eslint-disable-next-line no-control-regex
+      /[/?<>\\:*|"\x00-\x1F\x7F]/g,
+      '_'
+    )
+  })
+}

tests-ui/tests/utils/serachAndReplace.test.ts

@@ -0,0 +1,73 @@
+import type { LGraphNode } from '@comfyorg/litegraph'
+import { describe, expect, it } from 'vitest'
+
+import { applyTextReplacements } from '@/utils/searchAndReplace'
+
+describe('applyTextReplacements', () => {
+  // Test specifically the filename sanitization part
+  describe('filename sanitization', () => {
+    it('should replace invalid filename characters with underscores', () => {
+      // Mock the minimal app structure needed
+      const mockNodes = [
+        {
+          title: 'TestNode',
+          widgets: [
+            {
+              name: 'testWidget',
+              value:
+                'file/name?with<invalid>chars\\:*|"control\x00chars\x1F\x7F'
+            }
+          ]
+        } as LGraphNode
+      ]
+
+      const result = applyTextReplacements(mockNodes, '%TestNode.testWidget%')
+
+      // The expected result should have all invalid characters replaced with underscores
+      expect(result).toBe('file_name_with_invalid_chars_____control_chars__')
+    })
+
+    it('should handle various invalid filename characters individually', () => {
+      const testCases = [
+        { input: '/', expected: '_' },
+        { input: '?', expected: '_' },
+        { input: '<', expected: '_' },
+        { input: '>', expected: '_' },
+        { input: '\\', expected: '_' },
+        { input: ':', expected: '_' },
+        { input: '*', expected: '_' },
+        { input: '|', expected: '_' },
+        { input: '"', expected: '_' },
+        { input: '\x00', expected: '_' }, // NULL character
+        { input: '\x1F', expected: '_' }, // Unit separator
+        { input: '\x7F', expected: '_' } // Delete character
+      ]
+
+      for (const { input, expected } of testCases) {
+        const mockNodes = [
+          {
+            title: 'TestNode',
+            widgets: [{ name: 'testWidget', value: input }]
+          } as LGraphNode
+        ]
+
+        const result = applyTextReplacements(mockNodes, '%TestNode.testWidget%')
+        expect(result).toBe(expected)
+      }
+    })
+
+    it('should not modify valid filename characters', () => {
+      const validChars = 'abcABC123.-_ '
+
+      const mockNodes = [
+        {
+          title: 'TestNode',
+          widgets: [{ name: 'testWidget', value: validChars }]
+        } as LGraphNode
+      ]
+
+      const result = applyTextReplacements(mockNodes, '%TestNode.testWidget%')
+      expect(result).toBe(validChars)
+    })
+  })
+})