WIP: add support for ESI floating window rate limits#469
WIP: add support for ESI floating window rate limits#469joaomlneto wants to merge 1 commit intomainfrom
Conversation
joaomlneto
added
enhancement
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| }; | ||
|
|
||
| const normalizeEsiUrl = (value: string) => { | ||
| if (!value.startsWith(ESI_BASE_URL)) return value; |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
In general, the fix is to stop treating the URL as a plain string for trust decisions and instead parse it with URL, then compare the parsed origin (protocol + hostname + port) against a set of allowed ESI origins. We want to preserve existing behavior (normalizing ESI URLs to the “latest” base and ensuring trailing slashes) while making sure that only URLs genuinely targeting the configured ESI host are normalized.
Concretely, we should:
- Parse
valueinnormalizeEsiUrlusing theURLconstructor. - Similarly parse
ESI_BASE_URLandESI_LATEST_BASE_URLonce and derive theiroriginvalues. - Replace the
startsWith(ESI_BASE_URL)/startsWith(ESI_LATEST_BASE_URL)string checks with origin comparisons using the parsed URL objects. - When we rebuild the normalized URL, use the original pathname/query/search, but force the host/protocol from the trusted ESI base.
This preserves functionality: any URL whose origin matches the trusted ESI origin gets normalized to the latest base and trailing slash; all other URLs are left untouched. We only need modifications in packages/esi-client/src/client.ts around the helper functions and without adding new external dependencies; the standard URL class is already being used (parseUrlPath), so no new imports are needed.
| @@ -306,13 +306,32 @@ | ||
| }; | ||
|
|
||
| const normalizeEsiUrl = (value: string) => { | ||
| if (!value.startsWith(ESI_BASE_URL)) return value; | ||
| let url = value; | ||
| if (!url.startsWith(ESI_LATEST_BASE_URL)) { | ||
| const rest = url.slice(ESI_BASE_URL.length); | ||
| url = `${ESI_LATEST_BASE_URL}${rest.startsWith("/") ? "" : "/"}${rest}`; | ||
| let target: URL; | ||
| let esiBase: URL; | ||
| let esiLatestBase: URL; | ||
| try { | ||
| target = new URL(value); | ||
| esiBase = new URL(ESI_BASE_URL); | ||
| esiLatestBase = new URL(ESI_LATEST_BASE_URL); | ||
| } catch { | ||
| // If parsing fails for any of the URLs, do not attempt normalization. | ||
| return value; | ||
| } | ||
| return ensureTrailingSlash(url); | ||
|
|
||
| // Only normalize URLs that actually point at the configured ESI origin. | ||
| if (target.origin !== esiBase.origin && target.origin !== esiLatestBase.origin) { | ||
| return value; | ||
| } | ||
|
|
||
| // Build a normalized URL using the "latest" ESI base as origin. | ||
| const normalized = new URL(target.toString()); | ||
| normalized.protocol = esiLatestBase.protocol; | ||
| normalized.hostname = esiLatestBase.hostname; | ||
| normalized.port = esiLatestBase.port; | ||
| // Ensure path + query are preserved, but with a trailing slash on the path. | ||
| normalized.pathname = ensureTrailingSlash(normalized.pathname); | ||
|
|
||
| return normalized.toString(); | ||
| }; | ||
|
|
||
| const normalizeEsiRequestConfig = (config: RequestConfig): RequestConfig => { |
| if (normalizedUrl === config.url) return config; | ||
| return { ...config, url: normalizedUrl }; | ||
| } | ||
| if (config.baseURL && config.baseURL.startsWith(ESI_BASE_URL)) { |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
In general, the fix is to stop using string prefix checks (startsWith) to decide whether a URL belongs to the trusted ESI endpoint and instead parse the URL and compare its origin/hostname against a trusted value. This avoids cases where an attacker can prepend a trusted prefix to an untrusted hostname (e.g., https://esi.evetech.net.evil.com) and still pass the check.
Concretely in packages/esi-client/src/client.ts, the problematic checks are:
normalizeEsiUrl:if (!value.startsWith(ESI_BASE_URL)) return value;normalizeEsiRequestConfig:if (config.baseURL && config.baseURL.startsWith(ESI_BASE_URL)) { ... }
We should introduce a small helper that, given a URL string, parses it with the standard URL class and verifies that its origin (or at least protocol + hostname) exactly matches the expected ESI base origin. Then:
- In
normalizeEsiUrl, replace thestartsWithtest with a call to this helper, and computeESI_LATEST_BASE_URLconversion based on the parsed path instead of substring slicing againstESI_BASE_URL. This prevents a malicious URL that merely starts with the same characters from being handled as an ESI URL. - In
normalizeEsiRequestConfig, similarly replace theconfig.baseURL.startsWith(ESI_BASE_URL)condition with the helper so only URLs whose actual origin matches ESI are treated as ESI URLs.
We can implement this entirely within client.ts using the built‑in URL class (available in modern Node and browser environments) without new external libraries or changes to existing imports. The new helper will be defined near the existing URL utility functions (isAbsoluteUrl, ensureTrailingSlash, etc.) to keep the code cohesive. The rest of the logic—ensuring trailing slashes, preserving query strings, and handling ESI version prefixes—will remain the same for valid ESI URLs.
| @@ -305,12 +305,45 @@ | ||
| return query ? `${withSlash}?${query}` : withSlash; | ||
| }; | ||
|
|
||
| /** | ||
| * Returns true if the given absolute URL has the same origin as ESI_BASE_URL. | ||
| * Falls back to the previous string-based behavior only for origin comparison, | ||
| * not for arbitrary prefixes. | ||
| */ | ||
| const isEsiOrigin = (value: string): boolean => { | ||
| try { | ||
| const esi = new URL(ESI_BASE_URL); | ||
| const url = new URL(value); | ||
| return esi.protocol === url.protocol && esi.host === url.host; | ||
| } catch { | ||
| // If parsing fails, treat as non-ESI URL. | ||
| return false; | ||
| } | ||
| }; | ||
|
|
||
| const normalizeEsiUrl = (value: string) => { | ||
| if (!value.startsWith(ESI_BASE_URL)) return value; | ||
| if (!isAbsoluteUrl(value) || !isEsiOrigin(value)) return value; | ||
| let url = value; | ||
| if (!url.startsWith(ESI_LATEST_BASE_URL)) { | ||
| const rest = url.slice(ESI_BASE_URL.length); | ||
| url = `${ESI_LATEST_BASE_URL}${rest.startsWith("/") ? "" : "/"}${rest}`; | ||
| try { | ||
| const esiLatest = new URL(ESI_LATEST_BASE_URL); | ||
| const current = new URL(value); | ||
| // If already using the latest base URL (same origin + pathname prefix), keep as is. | ||
| if (!current.href.startsWith(ESI_LATEST_BASE_URL)) { | ||
| // Preserve the path/query after the base ESI path. | ||
| const esiBase = new URL(ESI_BASE_URL); | ||
| let restPath = current.pathname; | ||
| if (restPath.startsWith(esiBase.pathname)) { | ||
| restPath = restPath.slice(esiBase.pathname.length); | ||
| } | ||
| if (!restPath.startsWith("/")) { | ||
| restPath = `/${restPath}`; | ||
| } | ||
| const search = current.search ?? ""; | ||
| url = `${esiLatest.origin}${esiLatest.pathname.replace(/\/?$/, "")}${restPath}${search}`; | ||
| } | ||
| } catch { | ||
| // On any parsing issues, fall back to the original value. | ||
| url = value; | ||
| } | ||
| return ensureTrailingSlash(url); | ||
| }; | ||
| @@ -322,7 +353,7 @@ | ||
| if (normalizedUrl === config.url) return config; | ||
| return { ...config, url: normalizedUrl }; | ||
| } | ||
| if (config.baseURL && config.baseURL.startsWith(ESI_BASE_URL)) { | ||
| if (config.baseURL && isAbsoluteUrl(config.baseURL) && isEsiOrigin(config.baseURL)) { | ||
| const normalizedBase = normalizeEsiUrl(config.baseURL); | ||
| const normalizedUrl = ensureTrailingSlash(config.url); | ||
| return { |
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive ESI (EVE Swagger Interface) rate limiting support to the codebase, including a floating window rate limit implementation, real-time monitoring dashboard, and status indicators. The changes update the ESI client to use the new OpenAPI endpoint with compatibility date handling, implement client-side request throttling with token bucket algorithm, and provide UI components for observability.
Changes:
- Updated ESI client code generation to use the new
/meta/openapi.jsonendpoint with compatibility date parameter - Implemented comprehensive rate limiting logic with floating window algorithm, token reservation, and bucket management
- Added React hooks for monitoring rate limit state and schema
- Created a detailed rate limit dashboard UI component with live metrics and configuration display
- Integrated ESI status indicator in the main header with modal dashboard access
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/esi-client/kubb.config.ts | Updated OpenAPI input URL to use new meta endpoint with compatibility date |
| packages/esi-client/src/client.ts | Added comprehensive rate limiting implementation with token buckets, reservation system, and request throttling |
| packages/esi-client/src/index.ts | Exported client and infinite query hooks for external use |
| apps/web/lib/useEsiRateLimitState.ts | Created React hook for subscribing to real-time rate limit state with polling |
| apps/web/lib/useEsiRateLimitSchema.ts | Created React hook for fetching and registering rate limit schema from OpenAPI spec |
| apps/web/components/RateLimits/RateLimitDashboard.tsx | Built comprehensive dashboard UI for monitoring rate limits, buckets, and schema configuration |
| apps/web/components/EsiStatus/EsiStatusIndicator.tsx | Created status indicator component that displays ESI health and opens dashboard modal |
| apps/web/components/EsiStatus/index.ts | Added barrel export for ESI status components |
| apps/web/pages/status.tsx | Added rate limits button to open dashboard modal on status page |
| apps/web/app/rate-limits/page.tsx | Created dedicated rate limits page |
| apps/web/app/rate-limits/layout.tsx | Added layout for rate limits route |
| apps/web/layouts/MainLayout/HeaderWithMegaMenus/HeaderWithMegaMenus.tsx | Integrated ESI status indicator in main header |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| import type { RateLimitSchemaEntry } from "@jitaspace/esi-client"; | ||
| import { getRateLimitRouteKey, registerRateLimitConfig } from "@jitaspace/esi-client"; | ||
|
|
There was a problem hiding this comment.
The ESI compatibility date "2025-12-16" is duplicated between packages/esi-client/kubb.config.ts (line 8) and apps/web/lib/useEsiRateLimitSchema.ts (line 8). This creates a maintenance issue where the two values could diverge. Consider extracting this constant to a shared location or configuration file that both files can import, or at minimum, add comments indicating that these values must be kept in sync.
| // NOTE: This compatibility date must be kept in sync with the value in | |
| // packages/esi-client/kubb.config.ts (see CodeQL rule about duplicated ESI compatibility date). |
|
|
||
| const pruneBucket = (bucket: RateLimitBucket, now: number) => { | ||
| const cutoff = now - bucket.windowMs; | ||
| while (bucket.entries.length > 0 && bucket.entries[0].timestamp <= cutoff) { |
There was a problem hiding this comment.
The function pruneBucket removes expired entries based on entry.timestamp <= cutoff. This condition uses less-than-or-equal-to, which means entries exactly at the cutoff boundary (timestamp equals now - windowMs) are removed. Since the window should typically include entries from (now - windowMs, now] (exclusive on the left, inclusive on the right), the condition should be strictly less-than (<) instead of less-than-or-equal-to (<=) to avoid prematurely removing entries that are still within the window.
| while (bucket.entries.length > 0 && bucket.entries[0].timestamp <= cutoff) { | |
| while (bucket.entries.length > 0 && bucket.entries[0].timestamp < cutoff) { |
| useEffect(() => { | ||
| if (refreshMs <= 0) return; | ||
| const timer = setInterval(() => { | ||
| refreshSnapshot(); | ||
| setTick((tick) => tick + 1); | ||
| }, refreshMs); | ||
| return () => clearInterval(timer); | ||
| }, [refreshMs]); |
There was a problem hiding this comment.
The hook useEsiRateLimitState uses both useSyncExternalStore for subscription-based updates and a separate setInterval for polling (lines 41-48). However, the polling mechanism only calls refreshSnapshot() and setTick(), which doesn't trigger the external store's subscribers. The setTick state update forces a re-render, but this creates an inconsistent pattern. The snapshot returned from useSyncExternalStore (line 34-38) won't be updated by the polling interval - only by the subscription. Either the polling should be removed (relying only on subscriptions), or the polling should call the subscribers via notifyRateLimitSubscribers() instead of using a separate state update mechanism.
| return () => controller.abort(); | ||
| }, []); | ||
|
|
||
| return useMemo(() => state, [state]); |
There was a problem hiding this comment.
The useEsiRateLimitSchema hook returns useMemo(() => state, [state]) on line 191. Since state is already a stable object from useState, wrapping it in useMemo with state as a dependency doesn't provide any benefit - it will return a new memoized value every time state changes anyway. This line should just return state directly, or if memoization of derived values is needed, it should memoize specific computed properties rather than the entire state object.
| const reserveTokens = ( | ||
| bucket: RateLimitBucket, | ||
| expectedTokens: number, | ||
| now: number, | ||
| ) => { | ||
| if (expectedTokens <= 0) return { reservation: undefined }; | ||
| if (bucket.windowMs <= 0 || bucket.maxTokens <= 0) { | ||
| return { reservation: undefined }; | ||
| } | ||
|
|
||
| pruneBucket(bucket, now); | ||
|
|
||
| const effectiveMaxTokens = getEffectiveMaxTokens(bucket.maxTokens); | ||
| const available = effectiveMaxTokens - bucket.usedTokens; | ||
| let retryAt = now; | ||
|
|
||
| if (available < expectedTokens) { | ||
| let needed = expectedTokens - available; | ||
| let freed = 0; | ||
| for (const entry of bucket.entries) { | ||
| freed += entry.tokens; | ||
| if (freed >= needed) { | ||
| retryAt = Math.max(retryAt, entry.timestamp + bucket.windowMs); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const spacingMs = | ||
| bucket.windowMs > 0 | ||
| ? (expectedTokens / effectiveMaxTokens) * bucket.windowMs | ||
| : 0; | ||
| const spacingAt = Math.max(bucket.nextFreeAt, now); | ||
| retryAt = Math.max(retryAt, spacingAt); | ||
|
|
||
| if (retryAt > now) { | ||
| return { retryAfterMs: retryAt - now }; | ||
| } | ||
|
|
||
| const entry: RateLimitEntry = { | ||
| id: (rateLimitEntryId += 1), | ||
| timestamp: now, | ||
| tokens: expectedTokens, | ||
| }; | ||
| bucket.entries.push(entry); | ||
| bucket.usedTokens += expectedTokens; | ||
| bucket.nextFreeAt = Math.max(bucket.nextFreeAt, now) + spacingMs; | ||
| return { reservation: { bucket, entry, expectedTokens } }; |
There was a problem hiding this comment.
The rate limiting logic uses global shared state (rateLimitBuckets, rateLimitGroups, rateLimitRoutes) without any concurrency control mechanism. In a concurrent environment (e.g., when multiple requests are made simultaneously), this can lead to race conditions. For example, two requests could both call reserveTokens at nearly the same time, both see available tokens, and both reserve tokens, potentially exceeding the effective limit. Similarly, the pruneBucket and token adjustment operations could have race conditions. Consider using a locking mechanism or restructuring the code to handle concurrent access safely, or document that this implementation is only safe for single-threaded JavaScript environments.
| const getTokenCostForStatus = (status: number) => { | ||
| if (status >= 200 && status < 300) return 2; | ||
| if (status >= 300 && status < 400) return 1; | ||
| if (status >= 400 && status < 500) return status === 429 ? 0 : 5; | ||
| return 0; | ||
| }; |
There was a problem hiding this comment.
The token cost function getTokenCostForStatus returns hardcoded values (2 for 2xx, 1 for 3xx, 5 for 4xx except 429, 0 for 5xx and 429). These values appear to be assumptions about ESI's rate limiting cost per status code. However, there's no documentation or comment explaining why these specific values are used, and whether they are based on ESI's actual behavior or are estimates. If ESI provides actual token cost information in response headers, that should be used instead of these hardcoded values. If these are intentional defaults, they should be documented with a comment explaining the rationale.
| ); | ||
|
|
||
| export default function RateLimitDashboard() { | ||
| const schema = useEsiRateLimitSchema({}); |
There was a problem hiding this comment.
The useEsiRateLimitSchema hook is called with an empty object {} as an argument on line 66, but looking at the hook definition, it doesn't accept any parameters (it's defined as useEsiRateLimitSchema(): RateLimitSchemaState). This empty object argument is unnecessary and should be removed.
| const schema = useEsiRateLimitSchema({}); | |
| const schema = useEsiRateLimitSchema(); |
| </UnstyledButton> | ||
| </Tooltip> | ||
| ); | ||
| }); |
There was a problem hiding this comment.
The EsiStatusIndicator component is wrapped in memo() but doesn't have a display name set. This makes debugging more difficult as React DevTools will show it as Memo(Component) instead of a meaningful name. Consider adding EsiStatusIndicator.displayName = 'EsiStatusIndicator' after the component definition.
| }); | |
| }); | |
| EsiStatusIndicator.displayName = "EsiStatusIndicator"; |
| const [path, query] = value.split("?"); | ||
| if (path.endsWith("/")) return value; | ||
| const withSlash = `${path}/`; | ||
| return query ? `${withSlash}?${query}` : withSlash; |
There was a problem hiding this comment.
The ensureTrailingSlash function splits on ? to separate path and query string, but doesn't handle URLs with hash fragments (#). If a URL contains a hash fragment (e.g., /path#fragment or /path?query#fragment), the fragment will be included in the query variable or lost. While this might not be an issue for ESI API calls (which typically don't use fragments), it could lead to unexpected behavior if used with such URLs. Consider handling hash fragments explicitly or documenting that this function doesn't support them.
| const [path, query] = value.split("?"); | |
| if (path.endsWith("/")) return value; | |
| const withSlash = `${path}/`; | |
| return query ? `${withSlash}?${query}` : withSlash; | |
| const hashIndex = value.indexOf("#"); | |
| const hash = hashIndex >= 0 ? value.slice(hashIndex) : ""; | |
| const withoutHash = hashIndex >= 0 ? value.slice(0, hashIndex) : value; | |
| const [path, query] = withoutHash.split("?"); | |
| if (path.endsWith("/")) return value; | |
| const withSlash = `${path}/`; | |
| const withSlashAndQuery = query ? `${withSlash}?${query}` : withSlash; | |
| return `${withSlashAndQuery}${hash}`; |
|
|
||
| if ( | ||
| response.status === 429 && | ||
| context.reservation && | ||
| context.reservation.bucket.group === resolvedGroup | ||
| ) { | ||
| applyServerRetryAfter(context.reservation.bucket, response.headers, now); | ||
| } |
There was a problem hiding this comment.
In applyRateLimitResponse, lines 959-965 check if response.status === 429 and the bucket group matches to apply server retry-after. However, this code is redundant because the same check is already performed on lines 954-956 when the reservation bucket matches the resolved group. The condition on line 962 (context.reservation.bucket.group === resolvedGroup) was already true in the if-block at line 911-924, so this additional check will never execute any new logic. This redundant code should be removed.
| if ( | |
| response.status === 429 && | |
| context.reservation && | |
| context.reservation.bucket.group === resolvedGroup | |
| ) { | |
| applyServerRetryAfter(context.reservation.bucket, response.headers, now); | |
| } |
This pull request introduces a new ESI (EVE Swagger Interface) status indicator and rate limit dashboard to the web app, along with the underlying hooks and schema logic to support real-time ESI rate limit and endpoint status monitoring. It also updates the ESI client code generation to use the latest OpenAPI specification and exposes additional hooks for infinite queries. The most important changes are grouped below.
ESI Client Code Generation and Exports:
/meta/openapi.jsonendpoint with a compatibility date, ensuring up-to-date schema and rate limit metadata.ESI Status Indicator and Dashboard Integration:
EsiStatusIndicatorcomponent that displays ESI health status and opens a modal with theRateLimitDashboardon click, and integrated it into the main header (HeaderWithMegaMenus). [1] [2] [3] [4]/rate-limitsroute and page which renders theRateLimitDashboardwithin the main layout. [1] [2]/statuspage to open theRateLimitDashboardin a modal for quick access. [1] [2] [3] [4]ESI Rate Limit Monitoring Hooks and Schema:
useEsiRateLimitSchemato fetch and parse ESI rate limit information from the OpenAPI spec, and register it with the client for use in dashboards and monitoring.useEsiRateLimitStatehook to provide a real-time snapshot of current ESI rate limit state, using a subscription and polling mechanism for up-to-date UI.Closes #468