diff --git a/src/server/server.ts b/src/server/server.ts index f73f522d8..582e44e2b 100644 --- a/src/server/server.ts +++ b/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, diff --git a/src/server/utils/filterNameRegex.test.ts b/src/server/utils/filterNameRegex.test.ts new file mode 100644 index 000000000..649659ca7 --- /dev/null +++ b/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); + }); +}); diff --git a/src/server/utils/filterNameRegex.ts b/src/server/utils/filterNameRegex.ts new file mode 100644 index 000000000..71e76cfa4 --- /dev/null +++ b/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 (`(?=`, `(?<=`, `(?!`, `(? 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 }; +}