fix(settings): don't let a stored bad filterNameRegex brick automations (#3934)

src/server/server.ts

@@ -11,7 +11,7 @@ import meshtasticManager from './meshtasticManager.js';
 import { MeshtasticManager } from './meshtasticManager.js';
 import { sourceManagerRegistry } from './sourceManagerRegistry.js';
 import { resolveSourceManager } from './utils/resolveSourceManager.js';
-import { compileUserRegex } from '../utils/safeRegex.js';
+import { validateFilterNameRegexOnSave } from './utils/filterNameRegex.js';
 import { canonicalMessageTime, messageReceivedAt } from './utils/messageTime.js';
 import { pivotPositionHistory } from './utils/positionHistoryPivot.js';
 import protobufService from './protobufService.js';
@@ -3554,26 +3554,30 @@ apiRouter.post('/settings/traceroute-nodes', requirePermission('settings', 'writ
       return res.status(400).json({ error: (error as Error).message });
     }
 
-    // Validate regex if provided
+    // Current stored settings — needed to decide whether the regex must be
+    // hard-validated (see validateFilterNameRegexOnSave / #3934).
+    const traceNodesPostSourceId = (req.query.sourceId as string | undefined) || (req.body?.sourceId as string | undefined);
+    const currentTraceSettings = await databaseService.getTracerouteFilterSettingsAsync(traceNodesPostSourceId);
+
+    // Validate regex if provided — only hard-validate (RE2) when it will actually
+    // be applied or the pattern changed, so a stored RE2-incompatible pattern
+    // can't permanently brick the automation (#3934, mirrors #3806).
     let validatedRegex = '.*';
     if (filterNameRegex !== undefined && filterNameRegex !== null) {
       if (typeof filterNameRegex !== 'string') {
         return res.status(400).json({ error: 'Invalid filterNameRegex value. Must be a string.' });
       }
-      // Length cap + catastrophic-backtracking pattern check to prevent ReDoS
-      if (filterNameRegex.length > 200) {
-        return res.status(400).json({ error: 'filterNameRegex too long (max 200 characters).' });
-      }
-      if (/(\.\*){2,}|(\+.*\+)|(\*.*\*)|(\{[0-9]{3,}\})|(\{[0-9]+,\})/.test(filterNameRegex)) {
-        return res.status(400).json({ error: 'filterNameRegex too complex or may cause performance issues.' });
-      }
-      // Test that regex is valid
-      try {
-        compileUserRegex(filterNameRegex);
-        validatedRegex = filterNameRegex;
-      } catch {
-        return res.status(400).json({ error: 'Invalid filterNameRegex value. Must be a valid regular expression.' });
+      const regexWillBeApplied =
+        enabled &&
+        (filterRegexEnabled !== undefined ? filterRegexEnabled === true : currentTraceSettings.filterRegexEnabled);
+      const regexResult = validateFilterNameRegexOnSave(filterNameRegex, {
+        willBeApplied: regexWillBeApplied,
+        storedRegex: currentTraceSettings.filterNameRegex,
+      });
+      if ('error' in regexResult) {
+        return res.status(400).json({ error: regexResult.error });
       }
+      validatedRegex = regexResult.regex;
     }
 
     // Validate individual filter enabled flags (optional booleans, default to true)
@@ -3655,8 +3659,7 @@ apiRouter.post('/settings/traceroute-nodes', requirePermission('settings', 'writ
       return res.status(400).json({ error: 'filterHopsMin cannot be greater than filterHopsMax.' });
     }
 
-    // Update all settings (scoped to source when provided)
-    const traceNodesPostSourceId = (req.query.sourceId as string | undefined) || (req.body?.sourceId as string | undefined);
+    // Update all settings (scoped to source when provided; sourceId resolved above)
     await databaseService.setTracerouteFilterSettingsAsync({
       enabled,
       nodeNums,
@@ -3744,24 +3747,33 @@ apiRouter.post('/settings/remote-localstats-nodes', requirePermission('settings'
       return res.status(400).json({ error: (error as Error).message });
     }
 
-    // Validate regex (ReDoS-guarded, mirrors the traceroute route)
+    // sourceId is required here; resolve it up-front so we can read the current
+    // stored settings for the regex guard below.
+    const sourceId = (req.query.sourceId as string | undefined) || (req.body?.sourceId as string | undefined);
+    if (!sourceId) {
+      return res.status(400).json({ error: 'sourceId is required for remote LocalStats filter settings.' });
+    }
+    const currentRemoteSettings = await databaseService.getRemoteLocalStatsFilterSettingsAsync(sourceId);
+
+    // Validate regex — only hard-validate (RE2) when it will actually be applied
+    // or the pattern changed, so a stored RE2-incompatible pattern can't
+    // permanently brick the automation (#3934, mirrors #3806).
     let validatedRegex = '.*';
     if (filterNameRegex !== undefined && filterNameRegex !== null) {
       if (typeof filterNameRegex !== 'string') {
         return res.status(400).json({ error: 'Invalid filterNameRegex value. Must be a string.' });
       }
-      if (filterNameRegex.length > 200) {
-        return res.status(400).json({ error: 'filterNameRegex too long (max 200 characters).' });
-      }
-      if (/(\.\*){2,}|(\+.*\+)|(\*.*\*)|(\{[0-9]{3,}\})|(\{[0-9]+,\})/.test(filterNameRegex)) {
-        return res.status(400).json({ error: 'filterNameRegex too complex or may cause performance issues.' });
-      }
-      try {
-        compileUserRegex(filterNameRegex);
-        validatedRegex = filterNameRegex;
-      } catch {
-        return res.status(400).json({ error: 'Invalid filterNameRegex value. Must be a valid regular expression.' });
+      const regexWillBeApplied =
+        enabled &&
+        (filterRegexEnabled !== undefined ? filterRegexEnabled === true : currentRemoteSettings.filterRegexEnabled);
+      const regexResult = validateFilterNameRegexOnSave(filterNameRegex, {
+        willBeApplied: regexWillBeApplied,
+        storedRegex: currentRemoteSettings.filterNameRegex,
+      });
+      if ('error' in regexResult) {
+        return res.status(400).json({ error: regexResult.error });
       }
+      validatedRegex = regexResult.regex;
     }
 
     const validateOptionalBoolean = (value: unknown, name: string): boolean | undefined => {
@@ -3795,11 +3807,6 @@ apiRouter.post('/settings/remote-localstats-nodes', requirePermission('settings'
       validatedFilterLastHeardHours = filterLastHeardHours;
     }
 
-    const sourceId = (req.query.sourceId as string | undefined) || (req.body?.sourceId as string | undefined);
-    if (!sourceId) {
-      return res.status(400).json({ error: 'sourceId is required for remote LocalStats filter settings.' });
-    }
-
     await databaseService.setRemoteLocalStatsFilterSettingsAsync({
       enabled,
       nodeNums,

src/server/utils/filterNameRegex.test.ts

@@ -0,0 +1,54 @@
+import { describe, it, expect } from 'vitest';
+import { validateFilterNameRegexOnSave } from './filterNameRegex.js';
+
+// #3934: a stored RE2-incompatible filterNameRegex (client validates with native
+// RegExp, which allows lookaround/backrefs) must not permanently brick the
+// traceroute / remote-LocalStats automations. The guard hard-validates only when
+// the regex will be applied OR the pattern changed.
+describe('validateFilterNameRegexOnSave', () => {
+  const LOOKAROUND = '^(?!.*Mobile).*$'; // valid native RegExp, rejected by RE2
+
+  it('accepts a valid pattern when applied', () => {
+    const r = validateFilterNameRegexOnSave('^Node', { willBeApplied: true, storedRegex: '.*' });
+    expect(r).toEqual({ regex: '^Node' });
+  });
+
+  it('rejects a NEW RE2-incompatible pattern (lookaround) when it will be applied', () => {
+    const r = validateFilterNameRegexOnSave(LOOKAROUND, { willBeApplied: true, storedRegex: '.*' });
+    expect('error' in r).toBe(true);
+  });
+
+  it('rejects a NEW RE2-incompatible pattern even when NOT applied (changed → validated)', () => {
+    // Supplying a brand-new invalid pattern is always rejected, per #3934 expected behavior.
+    const r = validateFilterNameRegexOnSave(LOOKAROUND, { willBeApplied: false, storedRegex: '.*' });
+    expect('error' in r).toBe(true);
+  });
+
+  it('ALLOWS re-saving an unchanged stored bad pattern while the filter is disabled (unsticks the section)', () => {
+    // The core #3934 recovery path: pattern unchanged + not applied → no RE2 check.
+    const r = validateFilterNameRegexOnSave(LOOKAROUND, { willBeApplied: false, storedRegex: LOOKAROUND });
+    expect(r).toEqual({ regex: LOOKAROUND });
+  });
+
+  it('still rejects an unchanged stored bad pattern while the filter STAYS applied', () => {
+    const r = validateFilterNameRegexOnSave(LOOKAROUND, { willBeApplied: true, storedRegex: LOOKAROUND });
+    expect('error' in r).toBe(true);
+  });
+
+  it('ALLOWS clearing a stored bad pattern to a valid one', () => {
+    const r = validateFilterNameRegexOnSave('.*', { willBeApplied: true, storedRegex: LOOKAROUND });
+    expect(r).toEqual({ regex: '.*' });
+  });
+
+  it('enforces the length cap only when validating', () => {
+    const long = 'a'.repeat(201);
+    expect('error' in validateFilterNameRegexOnSave(long, { willBeApplied: true, storedRegex: '.*' })).toBe(true);
+    // unchanged + not applied → skipped, so an over-long stored value can still be re-saved to unstick
+    expect(validateFilterNameRegexOnSave(long, { willBeApplied: false, storedRegex: long })).toEqual({ regex: long });
+  });
+
+  it('flags catastrophic-backtracking patterns when validating', () => {
+    const r = validateFilterNameRegexOnSave('.*.*.*', { willBeApplied: true, storedRegex: '.*' });
+    expect('error' in r).toBe(true);
+  });
+});

src/server/utils/filterNameRegex.ts

@@ -0,0 +1,55 @@
+/**
+ * Shared `filterNameRegex` save-time validation for the auto-traceroute and
+ * remote-LocalStats node-filter endpoints (#3934).
+ *
+ * `filterNameRegex` is validated with RE2 (`compileUserRegex`), which — unlike
+ * the browser's native `RegExp` the client validates with — rejects lookaround
+ * and backreferences (`(?=`, `(?<=`, `(?!`, `(?<!`, `\1`, …). Without the guard
+ * here, an install that previously persisted such a pattern is permanently
+ * stuck: the settings form re-POSTs the stored regex on every save, so the whole
+ * request 400s and the user can't even toggle the filter/automation OFF (the
+ * same failure mode #3806 fixed for auto-ack, never ported to these endpoints).
+ *
+ * We therefore only hard-validate when the regex will actually be applied (the
+ * automation AND its regex sub-filter are enabled) OR the incoming pattern
+ * differs from the stored one. Disabling the automation/filter — or re-saving an
+ * unchanged bad pattern — is always allowed, so the section can be recovered.
+ * The ReDoS length/complexity caps ride along on the same condition.
+ */
+import { compileUserRegex } from '../../utils/safeRegex.js';
+
+// Length cap + catastrophic-backtracking pattern check (ReDoS guard).
+const REDOS_PATTERN = /(\.\*){2,}|(\+.*\+)|(\*.*\*)|(\{[0-9]{3,}\})|(\{[0-9]+,\})/;
+const MAX_REGEX_LENGTH = 200;
+
+export interface FilterNameRegexOptions {
+  /** The regex will actually be applied after this save (automation && regex sub-filter both enabled). */
+  willBeApplied: boolean;
+  /** The currently-persisted pattern, to detect an unchanged re-save. */
+  storedRegex: string;
+}
+
+/**
+ * Validate an incoming `filterNameRegex` for a save.
+ * @returns the pattern to persist, or an `error` string (caller → HTTP 400).
+ */
+export function validateFilterNameRegexOnSave(
+  incoming: string,
+  opts: FilterNameRegexOptions,
+): { regex: string } | { error: string } {
+  const changed = incoming !== opts.storedRegex;
+  if (opts.willBeApplied || changed) {
+    if (incoming.length > MAX_REGEX_LENGTH) {
+      return { error: `filterNameRegex too long (max ${MAX_REGEX_LENGTH} characters).` };
+    }
+    if (REDOS_PATTERN.test(incoming)) {
+      return { error: 'filterNameRegex too complex or may cause performance issues.' };
+    }
+    try {
+      compileUserRegex(incoming);
+    } catch {
+      return { error: 'Invalid filterNameRegex value. Must be a valid regular expression.' };
+    }
+  }
+  return { regex: incoming };
+}