Claude finished @Yeraze's task —— View job

PR Review: fix(security): RE2 for user-supplied regexes

Todo List:

• Gather context and understand the PR
• Read new files (safeRegex.ts, safeRegex.test.ts)
• Review modified files (server.ts, database.ts, meshtasticManager.ts, scriptRoutes.ts, Dockerfile)
• Analyze test coverage
• Provide comprehensive review feedback

Review Summary

This is a solid, well-scoped security fix. The approach is correct and the changes are conservative. A few observations worth addressing:

Issues

1. autoAckRegex.ts and settingsRoutes.ts still use new RegExp() — inconsistency

src/server/utils/autoAckRegex.ts:69 still calls new RegExp(pattern, 'i') at execution time:

return { regex: new RegExp(pattern, 'i') };

And src/server/routes/settingsRoutes.ts:215 uses new RegExp(pattern, 'i') for validation:

new RegExp(pattern, 'i');

These are the autoAckRegex validation/compilation code paths that meshtasticManager.ts bypasses (the manager now uses compileUserRegex via the cache, so at execution time it's safe). But:

• compileAutoAckRegex in autoAckRegex.ts is exported and may be called directly — callers get a native RegExp not a RE2 instance
• The heuristic filter in both files (charset allowlist + DANGEROUS_SHAPE_RE) reduces but doesn't eliminate the risk for the compile step in compileAutoAckRegex, which uses native new RegExp
• CodeQL may or may not flag these depending on taint flow, but they represent residual surface if compileAutoAckRegex is ever called outside the manager

Recommendation: Update autoAckRegex.ts:69 to use compileUserRegex and settingsRoutes.ts:215 to attempt compileUserRegex for validation. The existing charset allowlist can remain as a fast pre-rejection step.

2. safeRegex.ts — as unknown as RegExp cast is fragile

return new RE2(pattern, flags) as unknown as RegExp;

The RE2 class implements test, exec, etc., but it is not a true RegExp subclass. The double cast hides this. If any downstream code passes the returned value to something that does instanceof RegExp or accesses RegExp-specific internals (e.g. source, flags, lastIndex), it will silently fail or return unexpected values.

Recommendation: Either keep the as unknown as RegExp cast (acceptable given the explicit comment in the file) but add a note that instanceof RegExp checks will return false, or return a union type RE2 | RegExp so TypeScript surfaces incompatibilities at call sites. At minimum, the test suite should assert that re.source and re.flags return expected values to document the behaviour.

Minor Observations

3. src/utils/safeRegex.ts — no input validation before RE2 construction

Unlike the autoAckRegex.ts path, compileUserRegex applies no length cap or charset filter before handing the pattern to RE2. This is fine for security (RE2 is the mitigation), but a very long pattern could consume CPU during compilation. The existing call sites do have length checks upstream (e.g. scriptRoutes.ts:421 caps at 2000 chars), but compileUserRegex itself is silent on this. Worth documenting that length validation is the caller's responsibility.

4. Dockerfile — build-base python3 in builder is correct, but python3 was already added

The runtime stage (FROM node:24.15.0-alpine3.22 at line 48) already installs python3 via apk add. The builder now adds python3 too (line 11). This is harmless — they're independent stages — but the comment could clarify it's specifically build-base (the C compiler toolchain) that's new, not python3.

5. Test: (a+)+$ ReDoS timing test

The test at safeRegex.test.ts:19-27 asserts elapsed < 100ms. This is correct in intent but timing assertions in unit tests can be flaky in resource-constrained CI environments (e.g. under load or on slow runners). Consider increasing the threshold to 500ms or using a larger repeat(100) input to make the native RegExp case definitively slow while keeping RE2 well under the cap.

What's Done Well

• Correct, comprehensive conversion. All user-regex execution sites (validation and execution) are updated to use RE2. The PR author correctly identified that CodeQL flags validation but the real risk is execution — and both are fixed.
• Dockerfile is correct. build-base python3 in the builder stage only is the right pattern; the runtime image stays lean.
• Test coverage is good. The test suite covers: basic matching, capture groups, ReDoS immunity (timing), invalid patterns, and unsupported RE2 constructs (backreferences, lookahead).
• Drop-in interface maintained. RE2 exposes test/exec/match/replace, so call-site changes are zero.

Summary

The two issues worth fixing before merge:

1. autoAckRegex.ts:69 — replace new RegExp(pattern, 'i') with compileUserRegex(pattern, 'i') (and update the import)
2. settingsRoutes.ts:215 — replace new RegExp(pattern, 'i') with compileUserRegex(pattern, 'i') for consistent validation

Everything else is optional polish. The core security fix is sound.

Thanks for the thorough review — addressed in 3e933426:

• "Claude PR Assistant workflow" #1 autoAckRegex.ts:69 → compileUserRegex (charset/length/shape guards kept as a fast pre-check).
• feat: traceroute highlighting and UI improvements #2 settingsRoutes.ts:215 → compileUserRegex for consistent RE2-compatible validation.
• "Update Claude PR Assistant workflow" #3 cast/instanceof → documented in safeRegex.ts that the result is not instanceof RegExp and that length bounding is the caller's responsibility; added a test asserting .source/.flags.
• feat: traceroute request tracking and auth documentation #4 ReDoS timing test → threshold raised to 1000ms + larger input so it can't flake on slow runners.
• feat: add telemetry graphs, node sorting, and fix duplication #5 Dockerfile comment → clarified that build-base is the new compiler; python3 is node-gyp's other requirement.

Note: the prior CI run had two transient failures unrelated to this change — Test Suite (24.x) hit a vitest worker-teardown race (EnvironmentTeardownError; all 463 files passed) and ClamAV Scan hit npm install ECONNRESET. This push re-runs them clean.

Claude finished @Yeraze's task —— View job

PR Review: fix(security): RE2 for user-supplied regexes

Todo List:

• Gather context and understand the PR
• Read updated files (safeRegex.ts, safeRegex.test.ts, autoAckRegex.ts, settingsRoutes.ts)
• Review remaining modified files
• Verify all previous review items addressed
• Provide updated review feedback

Review of Updated Changes

All five items from the previous review have been addressed. This is a clean, well-scoped security fix ready to merge. A few minor observations on the current state:

What Was Addressed

#1 autoAckRegex.ts:69 — compileUserRegex is now used at autoAckRegex.ts:71. The charset/length/shape guards remain as a fast pre-check — the layered approach is sound.

#2 settingsRoutes.ts:215 — Updated to compileUserRegex(pattern, 'i') for consistent RE2-compatible validation.

#3 instanceof RegExp cast — Well-documented in safeRegex.ts with explicit caveats about instanceof, lastIndex, and length bounding as caller responsibility. The new test at safeRegex.test.ts:32-38 asserts .source and .flags behave as expected.

#4 ReDoS timing test — Threshold raised to 1000ms at safeRegex.test.ts:29 with a note about CI flakiness.

#5 Dockerfile comment — Comment at line 10 now clearly distinguishes build-base (the new C/C++ compiler toolchain) from python3 (which existed before).

Minor Observations (non-blocking)

new RegExp in test files — meshtasticManager.autoack-channels.test.ts lines 286, 317, 347, 377, 422, 432, 448, 461 still use new RegExp(autoAckRegex, 'i') for inline assertions. This is acceptable in tests since the values are hardcoded strings like '^(test|ping)', not user-supplied patterns.

new RegExp in meshtasticManager.ts:9964,10183,10306 — These three remaining instances use the pattern `\\{${key}\\}` where key is a param name from admin-defined trigger templates (e.g. {location}). The key is a known extracted identifier, not user-controlled at runtime, so these are safe template-substitution usages and don't need RE2.

src/components/auto-responder/utils.ts:269,291 — These use new RegExp in frontend utility code that processes auto-responder patterns. Since this is frontend-only (preview/display logic, not execution), it carries no server-side ReDoS risk. Worth noting in case CodeQL scans frontend code and flags it.

src/server/mqttPacketFilter.ts:325 — new RegExp('^' + expanded + '$') constructs a regex from an expanded MQTT topic pattern. This doesn't appear to be user-supplied input (MQTT topic filtering from config), but worth a review to confirm the expanded value is sanitized before this call.

Summary

The core fix is correct and comprehensive. All user-regex execution paths — validation and runtime matching — now go through RE2. The documentation, test coverage, and Dockerfile are all in good shape. The remaining new RegExp instances in the codebase are either test-only, template substitution with bounded keys, or frontend display code — none are ReDoS-risky user-controlled paths.

Verdict: Approved — no blocking issues remain.