Claude finished @Yeraze's task —— View job

PR Review: fix(ratelimit): exempt GET requests from apiLimiter

• Read changed files
• Review code quality and best practices
• Check for bugs or issues
• Evaluate security concerns
• Assess test coverage

Summary

This is a clean, well-reasoned fix. The change is minimal (one condition added to the skip predicate), the rationale is documented thoroughly, and the tests are comprehensive. Here are a few observations:

Code Quality

rateLimiters.ts:88 — The skip predicate change is correct and idiomatic:

skip: (req) => env.rateLimitApi === 0 || isPrivateNetworkIp(req.ip ?? '') || req.method === 'GET',

The ordering is good: cheap checks first (disabled flag, then string match), method check last.

Startup logging (rateLimiters.ts:64) — The new log line is accurate and helpful:

- API GET requests: always exempt (navigation-driven reads)

Potential Issues

HEAD and OPTIONS methods — The fix exempts only GET, but HEAD requests (which are semantically safe, side-effect-free reads per RFC 9110) and OPTIONS (preflight CORS) are still rate-limited. In practice this is unlikely to matter for MeshMonitor's self-hosted use case, but it's worth noting. If the React app ever issues OPTIONS preflights in volume (e.g. with non-simple CORS headers), those would count against the limit. This is not a blocker — just something to be aware of if CORS issues ever surface.

No PATCH consideration — PATCH is a mutating method and is correctly left rate-limited. This is intentional and correct.

Security

The PR's security rationale is sound:

• authLimiter (5 req/15 min) still protects login endpoints
• messageLimiter (30/min) still protects message sends
• meshcoreDeviceLimiter (10/min) still protects device operations
• Only read operations are exempted, which carry minimal abuse risk on a self-hosted dashboard

No security concerns with this approach.

Test Coverage

Tests are thorough:

• rateLimiters.test.ts:129 — Correctly updated the enforcement test to use POST (since GET is now exempt) with an explicit comment explaining why (// Use POST — GET is exempt from the API limiter (#3735))
• rateLimiters.test.ts:150 — New regression test explicitly named #3735 verifies GET requests are not blocked even when the rate limit would be exceeded
• Startup logging test (rateLimiters.test.ts:299) verifies the new GET requests: always exempt log line is emitted

One minor observation: the new GET exemption test at line 150 sends 5 requests against a rateLimitApi: 2 limit, which is sufficient to demonstrate the bypass. This is good coverage.

Minor Nit (non-blocking)

The test at line 150 sets app.set('trust proxy', 1) and uses X-Forwarded-For: 1.2.3.4 for the GET test — consistent with the POST enforcement test. This is correct and ensures the GET bypass isn't accidentally caused by the private-IP exemption.

Overall: LGTM. The fix is minimal, well-documented, correctly scoped, and backed by targeted regression tests. Ready to merge.

Superseded by #3748 (merged). Rather than exempting all GET requests from the API rate limiter — which removes back-pressure protection for low-powered / heavily-utilized servers — #3748 cuts the request volume that caused the 429s: per-source dashboard reads are now bundled into aggregate endpoints (/api/sources/:id/dashboard and /api/unified/dashboard), taking the unified view from ~4N+N to ~1+N requests per poll, and the source-toggle refresh is debounced. The limiter stays fully in place. Closing in favor of that approach.