perf: ip check#6850
Conversation
Coverage Report
File CoverageNo changed files found. |
There was a problem hiding this comment.
Pull request overview
This PR replaces request-ip with a centralized, proxy-aware client IP resolver and updates rate limiting / logging / internal endpoints to use the new logic, aiming to prevent spoofed forwarding headers and improve correctness/performance.
Changes:
- Added
packages/service/common/security/clientIp.ts(proxy-addr + normalization + trusted proxy allowlist viaTRUSTED_PROXY_IPS) and comprehensive tests. - Updated multiple API/middleware locations to use
getClientIpFromRequest/normalizeClientIpinstead ofrequest-ipor raw headers. - Removed
request-ipdependencies and introducedproxy-addr(+ types); updated docs to mention the stronger IP checks.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/app/src/pages/api/support/user/account/loginByPassword.ts | Uses new client IP resolver when creating user sessions. |
| projects/app/src/pages/api/core/sandbox/proxyCSPassword.ts | Normalizes socket IP before internal-only allowlist check. |
| projects/app/package.json | Removes request-ip and its types from app deps. |
| pnpm-workspace.yaml | Adds proxy-addr (+ types) to catalog; request-ip catalog entries still present. |
| pnpm-lock.yaml | Adds proxy-addr (+ types) resolutions; request-ip entries still present. |
| packages/service/type/env.ts | Adds TRUSTED_PROXY_IPS env typing. |
| packages/service/env.ts | Adds USE_IP_LIMIT and TRUSTED_PROXY_IPS to env schema/metadata. |
| packages/service/common/system/frequencyLimit/utils.ts | Fixes rate-limit signaling by throwing/rethrowing tooManyRequest. |
| packages/service/common/security/clientIp.ts | New trusted-proxy-aware client IP parsing + normalization utilities. |
| packages/service/common/middle/reqFrequencyLimit.ts | Switches IP extraction to new resolver and changes enable/disable gating logic. |
| packages/service/common/middle/entry.ts | Uses new resolver for request logging IP field. |
| packages/service/common/geo/index.ts | Uses new resolver for geo IP extraction. |
| packages/service/test/common/security/clientIp.test.ts | Adds unit tests for normalization, proxy trust, and header parsing behavior. |
| packages/service/test/common/middle/reqFrequencyLimit.test.ts | Adds tests ensuring rate limiting uses non-spoofable client IP keys. |
| packages/service/test/common/geo/index.test.ts | Updates/adds tests to reflect trusted-proxy behavior and spoof prevention. |
| document/data/doc-last-modified.json | Updates doc last-modified timestamp. |
| document/content/self-host/upgrading/4-15/4150.mdx | Notes stronger IP detection/spoofing prevention in release notes. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!env.CHECK_INTERNAL_IP || !force) { | ||
| return; | ||
| } | ||
|
|
||
| const ip = getClientIpFromRequest(req) ?? 'unknown'; |
There was a problem hiding this comment.
The early-return condition disables IP frequency limiting in almost all cases: if (!env.CHECK_INTERNAL_IP || !force) return; will return whenever force is false (the default), and it also ties rate limiting to CHECK_INTERNAL_IP rather than the IP-limit flag.
This looks like a regression from the previous USE_IP_LIMIT/force semantics. Consider switching back to a guard like “skip only when IP limiting is disabled AND not forced” (using env.USE_IP_LIMIT), so forced call sites (e.g. password login) still work while respecting the runtime toggle.
| const getTrustProxyFn = () => { | ||
| const trustedProxyIpEnv = env.TRUSTED_PROXY_IPS; | ||
| const nodeEnv = process.env.NODE_ENV; | ||
| if (trustedProxyIpEnv === cachedTrustedProxyIpEnv && nodeEnv === cachedNodeEnv) { | ||
| return cachedTrustProxyFn; | ||
| } |
There was a problem hiding this comment.
getTrustProxyFn can return the default cachedTrustProxyFn compiled with [] without ever compiling the intended default trust list when both process.env.NODE_ENV and env.TRUSTED_PROXY_IPS are undefined on the first call (because the cache keys also start as undefined).
This breaks the documented behavior (“non-production defaults to trusting loopback”) when NODE_ENV isn’t set. Consider initializing the cache keys to a sentinel value (or null) so the first call always compiles, or initializing cachedTrustProxyFn by calling getTrustProxyFn() lazily instead of compiling [] up front.
| const validAddresses: string[] = []; | ||
| const invalidAddresses: string[] = []; | ||
|
|
||
| (trustedProxyIpEnv ?? '') | ||
| .split(/[,\s]+/) | ||
| .filter(Boolean) | ||
| .forEach((item) => { | ||
| if (isValidTrustedProxyAddress(item)) { | ||
| validAddresses.push(item); | ||
| } else { | ||
| invalidAddresses.push(item); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
The comment says TRUSTED_PROXY_IPS entries are “去重” (deduplicated), but validAddresses/invalidAddresses are only pushed into arrays without any deduplication.
Either dedupe the parsed entries (e.g., via a Set) or update the comment to match the current behavior.
| const validAddresses: string[] = []; | |
| const invalidAddresses: string[] = []; | |
| (trustedProxyIpEnv ?? '') | |
| .split(/[,\s]+/) | |
| .filter(Boolean) | |
| .forEach((item) => { | |
| if (isValidTrustedProxyAddress(item)) { | |
| validAddresses.push(item); | |
| } else { | |
| invalidAddresses.push(item); | |
| } | |
| }); | |
| const validAddressSet = new Set<string>(); | |
| const invalidAddressSet = new Set<string>(); | |
| (trustedProxyIpEnv ?? '') | |
| .split(/[,\s]+/) | |
| .filter(Boolean) | |
| .forEach((item) => { | |
| if (isValidTrustedProxyAddress(item)) { | |
| validAddressSet.add(item); | |
| } else { | |
| invalidAddressSet.add(item); | |
| } | |
| }); | |
| const validAddresses = Array.from(validAddressSet); | |
| const invalidAddresses = Array.from(invalidAddressSet); |
| ) => { | ||
| return { | ||
| headers: { | ||
| ...(req.headers ?? {}), |
There was a problem hiding this comment.
createProxyAddrRequest’s comment says it “仅保留…XFF…避免外部 header 干扰判定”, but the implementation spreads all original headers into the new request.
To match the intent, consider only setting the single validated x-forwarded-for header (and omitting the spread), or adjust the comment if keeping other headers is intentional.
| ...(req.headers ?? {}), |
| "@types/proxy-addr": 2.0.3 | ||
| "@types/react": ^18 | ||
| "@types/react-dom": ^18 | ||
| "@types/request-ip": ^0.0.38 |
There was a problem hiding this comment.
request-ip is no longer imported anywhere in the repo (and was removed from the app/service package.json files), but the workspace catalog still includes @types/request-ip (and request-ip is still listed further down in this same catalog).
Please remove the remaining request-ip / @types/request-ip catalog entries to avoid carrying unused dependencies, and regenerate the lockfile after the cleanup.
| '@types/request-ip': | ||
| specifier: ^0.0.38 | ||
| version: 0.0.38 |
There was a problem hiding this comment.
The lockfile catalog section still contains @types/request-ip (and request-ip elsewhere) even though the dependency appears to have been removed from all importers and there are no remaining request-ip imports in the codebase.
After removing the catalog entries, please regenerate pnpm-lock.yaml so these obsolete entries are dropped as well.
| '@types/request-ip': | |
| specifier: ^0.0.38 | |
| version: 0.0.38 |
|
✅ Docs Preview Deployed! 🔗 👀 Click here to visit preview |
|
✅ Build Successful - Preview code-sandbox Image for this PR: |
|
✅ Build Successful - Preview fastgpt Image for this PR: |
|
✅ Build Successful - Preview mcp_server Image for this PR: |
|
✅ Admin Preview Image Ready! |
Made-with: Cursor
No description provided.