Internal subnets from 100.64.0.0/10 ratelimiting#33
Internal subnets from 100.64.0.0/10 ratelimiting#33InSelfControll wants to merge 2 commits intoT3-Content:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a TypeScript VPN-aware IP rate-limiting module with CIDR parsing and VPN/internal IP detection, and integrates it into the server request flow to apply stricter throttling for VPN/internal IPs (health-check endpoint excluded). Changes
Sequence DiagramsequenceDiagram
participant Client as Client/Request
participant Server as Server Handler
participant Detector as VPN Detector
participant Limiter as VpnAwareRateLimiter
participant Response as Response
Client->>Server: HTTP request (IP, path)
Server->>Detector: isVpnOrInternalIp(ip) / getVpnRateLimitConfig()
Detector-->>Server: isVpn: boolean
alt path != /healthz
Server->>Limiter: checkVpnRateLimit(ip, Limiter, log)
Limiter->>Limiter: select config (vpn vs standard)
Limiter->>Limiter: increment per-IP window counter / sweep expired
alt rate limit exceeded
Limiter-->>Server: Response(429)
Server-->>Client: 429 Too Many Requests
else allowed
Limiter-->>Server: null
Server->>Response: process request
Response-->>Client: 200 OK (or route response)
end
else /healthz
Server->>Response: process health check
Response-->>Client: 200 OK
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Add stricter 429 rate limiting for requests from internal/VPN subnets (including 100.64.0.0/10) in
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| private sweep(now: number, windowMs: number): void { | ||
| for (const [key, timestamps] of this.windows) { | ||
| const recent = timestamps.filter((timestamp) => now - timestamp <= windowMs); |
There was a problem hiding this comment.
🟠 High ratelimit.ts:222
The sweep method uses the caller's windowMs to filter all entries, but VPN and standard IPs have different windows. A VPN request (short window) would incorrectly purge valid timestamps from standard IP entries (longer window). Consider checking each key's prefix to apply the correct window size during sweep.
- private sweep(now: number, windowMs: number): void {
+ private sweep(now: number, _windowMs: number): void {
for (const [key, timestamps] of this.windows) {
+ const entryWindowMs = key.startsWith("vpn:") ? this.config.vpnWindowMs : this.config.standardWindowMs;
- const recent = timestamps.filter((timestamp) => now - timestamp <= windowMs);
+ const recent = timestamps.filter((timestamp) => now - timestamp <= entryWindowMs);🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file ratelimit.ts around lines 222-224:
The `sweep` method uses the caller's `windowMs` to filter all entries, but VPN and standard IPs have different windows. A VPN request (short window) would incorrectly purge valid timestamps from standard IP entries (longer window). Consider checking each key's prefix to apply the correct window size during sweep.
Evidence trail:
ratelimit.ts lines 183-192 (isRateLimited method showing different windowMs for VPN vs standard), lines 222-230 (sweep method applying single windowMs to all entries), lines 153-159 (config showing vpnWindowMs is configurable while standardWindowMs is hardcoded), lines 185, 212 (VPN and standard use different key prefixes but stored in same map)
|
|
||
| // Expand :: to full 8 groups | ||
| const expanded: string[] = []; | ||
| if (emptyIndex !== -1 && parts.length < 8) { |
There was a problem hiding this comment.
🟠 High ratelimit.ts:77
IPv6 addresses ending with :: (like fc00::) fail to parse. Splitting fc00:: produces ['fc00', '', ''], so the trailing empty string at index 2 is skipped, leaving only 7 groups. Consider filtering empty strings from the non-expansion path or handling trailing :: specially.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file ratelimit.ts around line 77:
IPv6 addresses ending with `::` (like `fc00::`) fail to parse. Splitting `fc00::` produces `['fc00', '', '']`, so the trailing empty string at index 2 is skipped, leaving only 7 groups. Consider filtering empty strings from the non-expansion path or handling trailing `::` specially.
Evidence trail:
ratelimit.ts lines 70-92 at commit a9d7b346d257a76be864164802ace2ce7b2b0a8f. The parseIPv6 function at line 72 splits by `:`, line 73 finds first empty index, lines 77-87 expand `::` but skip non-emptyIndex empty strings (line 84-86), resulting in only 7 groups for input `fc00::`.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
ratelimit.ts (2)
8-22:getBlockedSubnets()and CIDR parsing run on every request — precompute and cache at module load.
isVpnOrInternalIpcallsgetBlockedSubnets()(which splits/trims the env var) and then callsparseCidron each subnet for every incoming request. All five default subnets are re-parsed from scratch on every request. Since the subnet list and parsed CIDR structures are static (env vars don't change at runtime), they should be computed once at module initialization.♻️ Proposed refactor
-function getBlockedSubnets(): string[] { - const envSubnets = process.env.RATELIMIT_SUBNETS; - if (envSubnets) { - return envSubnets.split(",").map((s) => s.trim()); - } - return [ - "100.64.0.0/10", - "10.0.0.0/8", - "172.16.0.0/12", - "192.168.0.0/16", - "fc00::/7", - ]; -} +function getBlockedSubnets(): string[] { + const envSubnets = process.env.RATELIMIT_SUBNETS; + if (envSubnets) { + return envSubnets.split(",").map((s) => s.trim()); + } + return [ + "100.64.0.0/10", + "10.0.0.0/8", + "172.16.0.0/12", + "192.168.0.0/16", + "fc00::/7", + ]; +} + +// Pre-parse CIDRs once at module load to avoid per-request overhead +const BLOCKED_CIDRS = getBlockedSubnets() + .map((cidr) => ({ cidr, parsed: parseCidr(cidr) })) + .filter((entry) => entry.parsed !== null) as Array<{ cidr: string; parsed: NonNullable<ReturnType<typeof parseCidr>> }>;Then update
isVpnOrInternalIpto useBLOCKED_CIDRSdirectly instead of callinggetBlockedSubnets()andipInCidrper request:export function isVpnOrInternalIp(ip: string): boolean { if (!ip || ip === "unknown" || ip === "127.0.0.1" || ip === "::1") { return false; } - const subnets = getBlockedSubnets(); - for (const subnet of subnets) { - try { - if (ipInCidr(ip, subnet)) { + for (const { parsed } of BLOCKED_CIDRS) { + try { + const ipNum = parsed.isV6 ? parseIPv6(ip) : parseIPv4(ip); + if (ipNum !== null && (ipNum & parsed.mask) === parsed.network) { return true; } } catch { // skip } } return false; }Also applies to: 122-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ratelimit.ts` around lines 8 - 22, getBlockedSubnets currently returns raw strings and parseCidr/ipInCidr are being called per request in isVpnOrInternalIp; instead, compute a module-level constant (e.g., BLOCKED_CIDRS) at import time by calling getBlockedSubnets() once, splitting/trimming the env var if present, parsing each CIDR via parseCidr and filtering out invalid entries, then update isVpnOrInternalIp to iterate over BLOCKED_CIDRS and call ipInCidr against the pre-parsed structures; keep getBlockedSubnets for returning strings but stop re-parsing on each request.
183-205: Sweep uses the current request'swindowMsto clean up entries from both windows, causing premature eviction whenvpnWindowMs ≠ standardWindowMs.
sweepis called with eithervpnWindowMsorstandardWindowMsdepending on which type of request triggers it. The same sweep logic then filters all entries inthis.windowsregardless of their key prefix. IfvpnWindowMs < standardWindowMs, a VPN-triggered sweep will deletestd:*entries that are older thanvpnWindowMsbut still withinstandardWindowMs, effectively resetting a standard IP's request counter mid-window and allowing more requests than the limit permits.This is harmless in the default configuration (both windows = 60 s), but becomes a rate-limit bypass if the windows are set to different values via env vars.
♻️ Proposed fix: use per-key windowMs in
sweep- private sweep(now: number, windowMs: number): void { + private sweep(now: number): void { for (const [key, timestamps] of this.windows) { + const windowMs = key.startsWith("vpn:") + ? this.config.vpnWindowMs + : this.config.standardWindowMs; const recent = timestamps.filter((timestamp) => now - timestamp <= windowMs); if (recent.length === 0) { this.windows.delete(key); } else { this.windows.set(key, recent); } } this.lastSweep = now; }And update the two call sites in
isRateLimited:- this.sweep(now, windowMs); + this.sweep(now);Also applies to: 222-232
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ratelimit.ts` around lines 183 - 205, The sweep currently uses the caller's windowMs which causes cross-eviction between vpn and standard entries; change sweep to accept only the current timestamp (e.g., sweep(now)) and inside sweep iterate this.windows entries and, for each key (check prefixes "vpn:" vs "std:"), pick the appropriate windowMs from this.config (vpnWindowMs or standardWindowMs) and filter timestamps using that per-key window before re-setting or deleting the map entry; update both call sites in isRateLimited to call sweep(now) (remove windowMs arg) and keep lastSweep logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ratelimit.ts`:
- Around line 267-301: The function checkVpnRateLimit determines isVpn but
always uses getVpnRateLimitConfig().vpnWindowMs for the response; fix it by
selecting the correct windowMs based on isVpn and only calling config getters
once: fetch the VPN config via getVpnRateLimitConfig() and the standard config
via getRateLimitConfig() (or the appropriate standard config getter used in this
codebase) at the start of the rate-limit branch, set const windowMs = isVpn ?
vpnConfig.vpnWindowMs : standardConfig.windowMs, then use windowMs to compute
retryAfter in both the JSON body and the "Retry-After" header and remove the
duplicate getVpnRateLimitConfig() calls; update references in the Response
creation inside checkVpnRateLimit accordingly.
- Around line 70-101: The parseIPv6 function fails for leading/trailing ::
because it splits on ":" and drops adjacent empty segments; fix by detecting and
splitting on the literal "::" instead: if the IP contains "::", split into left
and right halves at "::", split each half by ":" (allowing empty halves),
compute how many zero groups to insert to reach 8 total groups, then concatenate
left groups + inserted "0" groups + right groups; validate that the final array
has exactly 8 hex groups and then parse each into a 16-bit value as before
(references: parseIPv6, expanded/result variables).
In `@server.ts`:
- Around line 104-110: The fallback default for vpnMaxRequests is inconsistent:
update getVpnRateLimitConfig() to use vpnMaxRequests: 10 (not 5) so it matches
the VpnAwareRateLimiter instantiation and the PR docs; locate the
getVpnRateLimitConfig function and change its default/fallback value for
VPN_RATELIMIT_MAX_REQ to 10 (and run quick tests or lint to ensure no other
callers expect 5).
- Around line 322-329: The VPN rate limiter is applied before static assets,
causing requests to /assets/* to consume the limited VPN budget; update the
early-exit check around checkVpnRateLimit(ip, vpnRateLimiter, log) to also skip
VPN limiting for requests whose url.pathname starts with "/assets/" (and keep
skipping "/healthz"), i.e. only call checkVpnRateLimit when url.pathname !==
"/healthz" AND !url.pathname.startsWith("/assets/"); modify the block that
invokes checkVpnRateLimit so static asset requests bypass the VPN limiter.
---
Nitpick comments:
In `@ratelimit.ts`:
- Around line 8-22: getBlockedSubnets currently returns raw strings and
parseCidr/ipInCidr are being called per request in isVpnOrInternalIp; instead,
compute a module-level constant (e.g., BLOCKED_CIDRS) at import time by calling
getBlockedSubnets() once, splitting/trimming the env var if present, parsing
each CIDR via parseCidr and filtering out invalid entries, then update
isVpnOrInternalIp to iterate over BLOCKED_CIDRS and call ipInCidr against the
pre-parsed structures; keep getBlockedSubnets for returning strings but stop
re-parsing on each request.
- Around line 183-205: The sweep currently uses the caller's windowMs which
causes cross-eviction between vpn and standard entries; change sweep to accept
only the current timestamp (e.g., sweep(now)) and inside sweep iterate
this.windows entries and, for each key (check prefixes "vpn:" vs "std:"), pick
the appropriate windowMs from this.config (vpnWindowMs or standardWindowMs) and
filter timestamps using that per-key window before re-setting or deleting the
map entry; update both call sites in isRateLimited to call sweep(now) (remove
windowMs arg) and keep lastSweep logic intact.
| function parseIPv6(ip: string): bigint | null { | ||
| // Handle :: abbreviation | ||
| const parts = ip.split(":"); | ||
| const emptyIndex = parts.findIndex((p) => p === ""); | ||
|
|
||
| // Expand :: to full 8 groups | ||
| const expanded: string[] = []; | ||
| if (emptyIndex !== -1 && parts.length < 8) { | ||
| const missing = 8 - parts.length + 1; | ||
| for (let i = 0; i < parts.length; i++) { | ||
| if (parts[i] === "" && i === emptyIndex) { | ||
| for (let j = 0; j < missing; j++) { | ||
| expanded.push("0"); | ||
| } | ||
| } else if (parts[i] !== "") { | ||
| expanded.push(parts[i]); | ||
| } | ||
| } | ||
| } else { | ||
| expanded.push(...parts.filter((p) => p !== "")); | ||
| } | ||
|
|
||
| if (expanded.length !== 8) return null; | ||
|
|
||
| let result = 0n; | ||
| for (const part of expanded) { | ||
| const num = parseInt(part, 16); | ||
| if (!Number.isFinite(num) || num < 0 || num > 0xffff) return null; | ||
| result = (result << 16n) | BigInt(num); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
parseIPv6 incorrectly handles :: at the start or end of an address, breaking the default fc00::/7 subnet.
When :: appears at the start or end (e.g., "fc00::", "::1"), "fc00::".split(":") produces ["fc00", "", ""]. The code tracks only emptyIndex (the first ""), fills zeros there, then silently drops the second adjacent empty string with else if (parts[i] !== ""). This produces 7 groups instead of 8, causing parseIPv6 to return null. As a result, parseCidr("fc00::/7") returns null, ipInCidr returns false, and IPv6 ULA addresses are never rate-limited — the entire fc00::/7 default subnet is a no-op.
The fix is to split on "::" (two characters) instead of ":":
🐛 Proposed fix for parseIPv6
function parseIPv6(ip: string): bigint | null {
- // Handle :: abbreviation
- const parts = ip.split(":");
- const emptyIndex = parts.findIndex((p) => p === "");
-
- // Expand :: to full 8 groups
- const expanded: string[] = [];
- if (emptyIndex !== -1 && parts.length < 8) {
- const missing = 8 - parts.length + 1;
- for (let i = 0; i < parts.length; i++) {
- if (parts[i] === "" && i === emptyIndex) {
- for (let j = 0; j < missing; j++) {
- expanded.push("0");
- }
- } else if (parts[i] !== "") {
- expanded.push(parts[i]);
- }
- }
- } else {
- expanded.push(...parts.filter((p) => p !== ""));
- }
-
- if (expanded.length !== 8) return null;
+ // Split on "::" to correctly handle leading, middle, and trailing abbreviations
+ const halves = ip.split("::");
+ if (halves.length > 2) return null; // More than one "::" is invalid
+
+ let groups: string[];
+ if (halves.length === 2) {
+ const left = halves[0] ? halves[0].split(":") : [];
+ const right = halves[1] ? halves[1].split(":") : [];
+ const zerosNeeded = 8 - left.length - right.length;
+ if (zerosNeeded < 0) return null;
+ groups = [...left, ...Array(zerosNeeded).fill("0"), ...right];
+ } else {
+ groups = ip.split(":");
+ }
+
+ if (groups.length !== 8) return null;
- let result = 0n;
- for (const part of expanded) {
+ let result = 0n;
+ for (const part of groups) {
const num = parseInt(part, 16);
if (!Number.isFinite(num) || num < 0 || num > 0xffff) return null;
result = (result << 16n) | BigInt(num);
}
return result;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function parseIPv6(ip: string): bigint | null { | |
| // Handle :: abbreviation | |
| const parts = ip.split(":"); | |
| const emptyIndex = parts.findIndex((p) => p === ""); | |
| // Expand :: to full 8 groups | |
| const expanded: string[] = []; | |
| if (emptyIndex !== -1 && parts.length < 8) { | |
| const missing = 8 - parts.length + 1; | |
| for (let i = 0; i < parts.length; i++) { | |
| if (parts[i] === "" && i === emptyIndex) { | |
| for (let j = 0; j < missing; j++) { | |
| expanded.push("0"); | |
| } | |
| } else if (parts[i] !== "") { | |
| expanded.push(parts[i]); | |
| } | |
| } | |
| } else { | |
| expanded.push(...parts.filter((p) => p !== "")); | |
| } | |
| if (expanded.length !== 8) return null; | |
| let result = 0n; | |
| for (const part of expanded) { | |
| const num = parseInt(part, 16); | |
| if (!Number.isFinite(num) || num < 0 || num > 0xffff) return null; | |
| result = (result << 16n) | BigInt(num); | |
| } | |
| return result; | |
| } | |
| function parseIPv6(ip: string): bigint | null { | |
| // Split on "::" to correctly handle leading, middle, and trailing abbreviations | |
| const halves = ip.split("::"); | |
| if (halves.length > 2) return null; // More than one "::" is invalid | |
| let groups: string[]; | |
| if (halves.length === 2) { | |
| const left = halves[0] ? halves[0].split(":") : []; | |
| const right = halves[1] ? halves[1].split(":") : []; | |
| const zerosNeeded = 8 - left.length - right.length; | |
| if (zerosNeeded < 0) return null; | |
| groups = [...left, ...Array(zerosNeeded).fill("0"), ...right]; | |
| } else { | |
| groups = ip.split(":"); | |
| } | |
| if (groups.length !== 8) return null; | |
| let result = 0n; | |
| for (const part of groups) { | |
| const num = parseInt(part, 16); | |
| if (!Number.isFinite(num) || num < 0 || num > 0xffff) return null; | |
| result = (result << 16n) | BigInt(num); | |
| } | |
| return result; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ratelimit.ts` around lines 70 - 101, The parseIPv6 function fails for
leading/trailing :: because it splits on ":" and drops adjacent empty segments;
fix by detecting and splitting on the literal "::" instead: if the IP contains
"::", split into left and right halves at "::", split each half by ":" (allowing
empty halves), compute how many zero groups to insert to reach 8 total groups,
then concatenate left groups + inserted "0" groups + right groups; validate that
the final array has exactly 8 hex groups and then parse each into a 16-bit value
as before (references: parseIPv6, expanded/result variables).
| export function checkVpnRateLimit( | ||
| ip: string, | ||
| limiter: VpnAwareRateLimiter, | ||
| log?: (level: string, component: string, message: string, meta?: Record<string, unknown>) => void, | ||
| ): Response | null { | ||
| const isVpn = isVpnOrInternalIp(ip); | ||
| const isLimited = limiter.isRateLimited(ip, isVpn); | ||
|
|
||
| if (isLimited) { | ||
| const count = limiter.getRequestCount(ip, isVpn); | ||
| if (log) { | ||
| log("WARN", "ratelimit", `Rate limited ${isVpn ? "VPN/internal" : "regular"} IP`, { | ||
| ip, | ||
| isVpn, | ||
| count, | ||
| }); | ||
| } | ||
| return new Response( | ||
| JSON.stringify({ | ||
| error: "Too Many Requests", | ||
| retryAfter: Math.ceil(getVpnRateLimitConfig().vpnWindowMs / 1000), | ||
| }), | ||
| { | ||
| status: 429, | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| "Retry-After": String(Math.ceil(getVpnRateLimitConfig().vpnWindowMs / 1000)), | ||
| "Cache-Control": "no-store", | ||
| }, | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
Retry-After always uses vpnWindowMs even when a standard IP is rate-limited, and getVpnRateLimitConfig() is called twice.
isVpn is already resolved in this function. The 429 response body and Retry-After header should reflect the actual window that applies to the IP type. If VPN_RATELIMIT_WINDOW_MS is configured shorter than WINDOW_MS, a standard IP that gets rate-limited will receive a Retry-After value that is too short and will keep retrying prematurely. The function also calls getVpnRateLimitConfig() redundantly on both lines 287 and 293.
♻️ Proposed fix
export function checkVpnRateLimit(
ip: string,
limiter: VpnAwareRateLimiter,
log?: (level: string, component: string, message: string, meta?: Record<string, unknown>) => void,
): Response | null {
const isVpn = isVpnOrInternalIp(ip);
const isLimited = limiter.isRateLimited(ip, isVpn);
if (isLimited) {
const count = limiter.getRequestCount(ip, isVpn);
+ const config = getVpnRateLimitConfig();
+ const windowMs = isVpn ? config.vpnWindowMs : config.standardWindowMs;
+ const retryAfterSecs = Math.ceil(windowMs / 1000);
if (log) {
log("WARN", "ratelimit", `Rate limited ${isVpn ? "VPN/internal" : "regular"} IP`, {
ip,
isVpn,
count,
});
}
return new Response(
JSON.stringify({
error: "Too Many Requests",
- retryAfter: Math.ceil(getVpnRateLimitConfig().vpnWindowMs / 1000),
+ retryAfter: retryAfterSecs,
}),
{
status: 429,
headers: {
"Content-Type": "application/json",
- "Retry-After": String(Math.ceil(getVpnRateLimitConfig().vpnWindowMs / 1000)),
+ "Retry-After": String(retryAfterSecs),
"Cache-Control": "no-store",
},
},
);
}
return null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function checkVpnRateLimit( | |
| ip: string, | |
| limiter: VpnAwareRateLimiter, | |
| log?: (level: string, component: string, message: string, meta?: Record<string, unknown>) => void, | |
| ): Response | null { | |
| const isVpn = isVpnOrInternalIp(ip); | |
| const isLimited = limiter.isRateLimited(ip, isVpn); | |
| if (isLimited) { | |
| const count = limiter.getRequestCount(ip, isVpn); | |
| if (log) { | |
| log("WARN", "ratelimit", `Rate limited ${isVpn ? "VPN/internal" : "regular"} IP`, { | |
| ip, | |
| isVpn, | |
| count, | |
| }); | |
| } | |
| return new Response( | |
| JSON.stringify({ | |
| error: "Too Many Requests", | |
| retryAfter: Math.ceil(getVpnRateLimitConfig().vpnWindowMs / 1000), | |
| }), | |
| { | |
| status: 429, | |
| headers: { | |
| "Content-Type": "application/json", | |
| "Retry-After": String(Math.ceil(getVpnRateLimitConfig().vpnWindowMs / 1000)), | |
| "Cache-Control": "no-store", | |
| }, | |
| }, | |
| ); | |
| } | |
| return null; | |
| } | |
| export function checkVpnRateLimit( | |
| ip: string, | |
| limiter: VpnAwareRateLimiter, | |
| log?: (level: string, component: string, message: string, meta?: Record<string, unknown>) => void, | |
| ): Response | null { | |
| const isVpn = isVpnOrInternalIp(ip); | |
| const isLimited = limiter.isRateLimited(ip, isVpn); | |
| if (isLimited) { | |
| const count = limiter.getRequestCount(ip, isVpn); | |
| const config = getVpnRateLimitConfig(); | |
| const windowMs = isVpn ? config.vpnWindowMs : config.standardWindowMs; | |
| const retryAfterSecs = Math.ceil(windowMs / 1000); | |
| if (log) { | |
| log("WARN", "ratelimit", `Rate limited ${isVpn ? "VPN/internal" : "regular"} IP`, { | |
| ip, | |
| isVpn, | |
| count, | |
| }); | |
| } | |
| return new Response( | |
| JSON.stringify({ | |
| error: "Too Many Requests", | |
| retryAfter: retryAfterSecs, | |
| }), | |
| { | |
| status: 429, | |
| headers: { | |
| "Content-Type": "application/json", | |
| "Retry-After": String(retryAfterSecs), | |
| "Cache-Control": "no-store", | |
| }, | |
| }, | |
| ); | |
| } | |
| return null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ratelimit.ts` around lines 267 - 301, The function checkVpnRateLimit
determines isVpn but always uses getVpnRateLimitConfig().vpnWindowMs for the
response; fix it by selecting the correct windowMs based on isVpn and only
calling config getters once: fetch the VPN config via getVpnRateLimitConfig()
and the standard config via getRateLimitConfig() (or the appropriate standard
config getter used in this codebase) at the start of the rate-limit branch, set
const windowMs = isVpn ? vpnConfig.vpnWindowMs : standardConfig.windowMs, then
use windowMs to compute retryAfter in both the JSON body and the "Retry-After"
header and remove the duplicate getVpnRateLimitConfig() calls; update references
in the Response creation inside checkVpnRateLimit accordingly.
| // VPN-aware rate limiter for stricter limits on VPN/internal IPs | ||
| const vpnRateLimiter = new VpnAwareRateLimiter({ | ||
| vpnWindowMs: parsePositiveInt(process.env.VPN_RATELIMIT_WINDOW_MS, 60_000), | ||
| vpnMaxRequests: parsePositiveInt(process.env.VPN_RATELIMIT_MAX_REQ, 10), | ||
| standardWindowMs: WINDOW_MS, | ||
| standardMaxRequests: HISTORY_LIMIT_PER_MIN, | ||
| }); |
There was a problem hiding this comment.
vpnMaxRequests default is inconsistent between server.ts (10) and ratelimit.ts's getVpnRateLimitConfig() (5).
getVpnRateLimitConfig() returns vpnMaxRequests: 5 as the fallback when VPN_RATELIMIT_MAX_REQ is unset, while server.ts passes 10 explicitly. The PR description documents the default as 10. Any code that calls new VpnAwareRateLimiter() with no arguments, or reads getVpnRateLimitConfig() directly, will get the inconsistent value of 5. Align the getVpnRateLimitConfig() default with the intended value.
🛠️ Proposed fix
In ratelimit.ts, line 156:
- vpnMaxRequests: parsePositiveInt(process.env.VPN_RATELIMIT_MAX_REQ, 5),
+ vpnMaxRequests: parsePositiveInt(process.env.VPN_RATELIMIT_MAX_REQ, 10),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server.ts` around lines 104 - 110, The fallback default for vpnMaxRequests is
inconsistent: update getVpnRateLimitConfig() to use vpnMaxRequests: 10 (not 5)
so it matches the VpnAwareRateLimiter instantiation and the PR docs; locate the
getVpnRateLimitConfig function and change its default/fallback value for
VPN_RATELIMIT_MAX_REQ to 10 (and run quick tests or lint to ensure no other
callers expect 5).
| // Check VPN/internal IP rate limiting (very strict limits for VPN/subnet IPs) | ||
| // Skip health check endpoint from VPN rate limiting | ||
| if (url.pathname !== "/healthz") { | ||
| const vpnRateLimitResponse = checkVpnRateLimit(ip, vpnRateLimiter, log); | ||
| if (vpnRateLimitResponse) { | ||
| return vpnRateLimitResponse; | ||
| } | ||
| } |
There was a problem hiding this comment.
Static asset requests (/assets/) are counted against the VPN rate limit budget.
The VPN limiter check runs at line 324 before the /assets/ handler at line 331. Every static asset fetch (JS, CSS, fonts, etc.) by a VPN/internal IP consumes one of the 10 allowed requests per window, which can exhaust the budget quickly for a normal page load and cause legitimate users to see 429s. Extend the exclusion to at least /assets/ (and possibly /healthz-style paths) before hitting the limiter.
🛠️ Suggested fix
- if (url.pathname !== "/healthz") {
+ if (url.pathname !== "/healthz" && !url.pathname.startsWith("/assets/")) {
const vpnRateLimitResponse = checkVpnRateLimit(ip, vpnRateLimiter, log);
if (vpnRateLimitResponse) {
return vpnRateLimitResponse;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check VPN/internal IP rate limiting (very strict limits for VPN/subnet IPs) | |
| // Skip health check endpoint from VPN rate limiting | |
| if (url.pathname !== "/healthz") { | |
| const vpnRateLimitResponse = checkVpnRateLimit(ip, vpnRateLimiter, log); | |
| if (vpnRateLimitResponse) { | |
| return vpnRateLimitResponse; | |
| } | |
| } | |
| // Check VPN/internal IP rate limiting (very strict limits for VPN/subnet IPs) | |
| // Skip health check endpoint from VPN rate limiting | |
| if (url.pathname !== "/healthz" && !url.pathname.startsWith("/assets/")) { | |
| const vpnRateLimitResponse = checkVpnRateLimit(ip, vpnRateLimiter, log); | |
| if (vpnRateLimitResponse) { | |
| return vpnRateLimitResponse; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server.ts` around lines 322 - 329, The VPN rate limiter is applied before
static assets, causing requests to /assets/* to consume the limited VPN budget;
update the early-exit check around checkVpnRateLimit(ip, vpnRateLimiter, log) to
also skip VPN limiting for requests whose url.pathname starts with "/assets/"
(and keep skipping "/healthz"), i.e. only call checkVpnRateLimit when
url.pathname !== "/healthz" AND !url.pathname.startsWith("/assets/"); modify the
block that invokes checkVpnRateLimit so static asset requests bypass the VPN
limiter.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server.ts (1)
119-125: Consider a dedicated env var / constant for the standard (non-VPN) rate limit.
HISTORY_LIMIT_PER_MINis semantically tied to the/api/historyendpoint, but here it doubles as the global per-IP budget applied to every non-VPN request that passes throughfetch(assets, WebSocket upgrades, Fossabot, admin APIs, etc.). If either value is tuned independently in the future, the coupling will be surprising.♻️ Suggested approach
+const STANDARD_RATELIMIT_PER_MIN = parsePositiveInt( + process.env.STANDARD_RATELIMIT_PER_MIN, + 120, +); const vpnRateLimiter = new VpnAwareRateLimiter({ vpnWindowMs: parsePositiveInt(process.env.VPN_RATELIMIT_WINDOW_MS, 60_000), vpnMaxRequests: parsePositiveInt(process.env.VPN_RATELIMIT_MAX_REQ, 10), standardWindowMs: WINDOW_MS, - standardMaxRequests: HISTORY_LIMIT_PER_MIN, + standardMaxRequests: STANDARD_RATELIMIT_PER_MIN, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.ts` around lines 119 - 125, The code uses HISTORY_LIMIT_PER_MIN for the standard (non-VPN) global rate limit which couples a history-endpoint constant to the VpnAwareRateLimiter; introduce a dedicated constant/env var (e.g., STANDARD_RATELIMIT_MAX_REQ parsed via parsePositiveInt and/or STANDARD_RATELIMIT_WINDOW_MS if desired) and replace HISTORY_LIMIT_PER_MIN in the VpnAwareRateLimiter instantiation (vpnRateLimiter) with that new constant; update any relevant environment loading/docs to include the new name and keep WINDOW_MS and vpn-specific vars unchanged so future tuning of history vs global limits is independent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@server.ts`:
- Around line 426-433: The VPN/internal IP rate-limit is applied before static
assets are handled, causing requests like url.pathname starting with "/assets/"
to consume the 10 req/min VPN budget; change the logic in the request handling
so checkVpnRateLimit(ip, vpnRateLimiter, log) is skipped for static asset paths
(e.g., when url.pathname.startsWith("/assets/")) or move the vpn rate-check to
after the assets branch that handles "/assets/" so that asset requests do not
decrement the VPN limiter (keep the existing health check skip for "/healthz"
intact).
- Around line 119-122: The fallback default for VPN rate limit is inconsistent
(server.ts uses 10 while getVpnRateLimitConfig() in ratelimit.ts uses 5); unify
them by introducing or using a single exported constant (e.g.,
DEFAULT_VPN_MAX_REQUESTS) from ratelimit.ts and reference that from server.ts
(replace the hard-coded 10 in the VpnAwareRateLimiter instantiation and the
parsePositiveInt fallback with the shared constant), or update both places to
the same numeric default (preferably via the shared constant) so vpnMaxRequests
and getVpnRateLimitConfig() return the identical default.
---
Nitpick comments:
In `@server.ts`:
- Around line 119-125: The code uses HISTORY_LIMIT_PER_MIN for the standard
(non-VPN) global rate limit which couples a history-endpoint constant to the
VpnAwareRateLimiter; introduce a dedicated constant/env var (e.g.,
STANDARD_RATELIMIT_MAX_REQ parsed via parsePositiveInt and/or
STANDARD_RATELIMIT_WINDOW_MS if desired) and replace HISTORY_LIMIT_PER_MIN in
the VpnAwareRateLimiter instantiation (vpnRateLimiter) with that new constant;
update any relevant environment loading/docs to include the new name and keep
WINDOW_MS and vpn-specific vars unchanged so future tuning of history vs global
limits is independent.
1 participant
Implementation Summary
New File: ratelimit.ts
A complete IP rate limiting module with VPN/internal subnet detection:
Features:
• CIDR-based IP detection - Detects IPs in configured subnets (100.64.0.0/10 CGNAT, 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, fc00::/7
Pv6)
• VpnAwareRateLimiter class - Separate rate limits for VPN/internal IPs vs regular IPs
• Configurable via environment variables
Environment Variables:
Variable Default Description
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
VPN_RATELIMIT_WINDOW_MS 60000 Time window for VPN IP limits (ms)
VPN_RATELIMIT_MAX_REQ 10 Max requests per window for VPN IPs
RATELIMIT_SUBNETS (built-in) Comma-separated CIDR blocks to block
Default blocked subnets:
• 100.64.0.0/10 - CGNAT (RFC 6598) ← your requested subnet
• 10.0.0.0/8 - RFC 1918 private
• 172.16.0.0/12 - RFC 1918 private
• 192.168.0.0/16 - RFC 1918 private
• fc00::/7 - IPv6 unique local
Updated server.ts
• Integrated VPN rate limiting in the main fetch handler
• VPN IPs get much stricter limits (default: 10 requests/minute vs 120 for regular IPs)
• Health check endpoint (/healthz) is excluded from VPN rate limiting
• Rate limited responses include Retry-After header
Usage Examples:
Use default CGNAT blocking (100.64.0.0/10)
bun server.ts
Custom stricter limits
VPN_RATELIMIT_MAX_REQ=5 VPN_RATELIMIT_WINDOW_MS=30000 bun server.ts
Custom subnets only
RATELIMIT_SUBNETS="100.64.0.0/10,10.0.0.0/8" bun server.ts
Summary by CodeRabbit