Skip to content

feat(server): 增加 /p 及 /ap 代理接口的单 IP 并发数限制#2375

Open
Roberta001 wants to merge 2 commits intoOpenListTeam:mainfrom
Roberta001:main
Open

feat(server): 增加 /p 及 /ap 代理接口的单 IP 并发数限制#2375
Roberta001 wants to merge 2 commits intoOpenListTeam:mainfrom
Roberta001:main

Conversation

@Roberta001
Copy link
Copy Markdown

Description / 描述

在/@manage/settings/traffic页下增加了一个配置项proxy_max_concurrent_requests_per_ip,用于控制单个IP请求/p//ap/的并发数

Motivation and Context / 背景

解决启用web代理时被多线程下载器刷爆的场景

How Has This Been Tested? / 测试

将Proxy max concurrent requests per ip配置为-1(无限制)或0(完全禁用/p//ap/接口)以外的数,即可限制并发

Checklist / 检查清单

  • I have read the CONTRIBUTING document.
    我已阅读 CONTRIBUTING 文档。
  • I have formatted my code with go fmt or prettier.
    我已使用 go fmtprettier 格式化提交的代码。
  • I have added appropriate labels to this PR (or mentioned needed labels in the description if lacking permissions).
    我已为此 PR 添加了适当的标签(如无权限或需要的标签不存在,请在描述中说明,管理员将后续处理)。
  • [x I have requested review from relevant code authors using the "Request review" feature when applicable.
    我已在适当情况下使用"Request review"功能请求相关代码作者进行审查。
  • I have updated the repository accordingly (If it’s needed).
    我已相应更新了相关仓库(若适用)。

@Roberta001 Roberta001 changed the title 增加了/p/和/ap/代理接口的并发数控制 feat(server): 增加 /p 及 /ap 代理接口的单 IP 并发数限制 Apr 18, 2026
Copy link
Copy Markdown

@utafrali utafrali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The core logic is sound: the mutex-protected per-IP counter, deferred decrement, and sentinel values (-1 for unlimited, 0 for disabled) are all correct. The two substantive concerns are IP spoofability via unvalidated proxy headers and the undocumented per-process scoping of the counter in multi-instance setups.

"message": "Proxy is disabled",
"data": nil,
})
return
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c.ClientIP() resolves the client address from X-Forwarded-For / X-Real-IP headers, which a client can freely forge if Gin's trusted proxy list hasn't been configured. An attacker could rotate spoofed IPs in that header on every request and bypass the concurrency limit entirely. Either add a code comment (and a note to the setting description) that engine.SetTrustedProxies(...) must be configured for this limit to have any teeth, or expose a separate admin option to use c.Request.RemoteAddr for strict enforcement (accepting that it breaks deployments behind CDNs/reverse-proxies).

"github.com/OpenListTeam/OpenList/v4/internal/conf"
"github.com/OpenListTeam/OpenList/v4/internal/setting"
"github.com/gin-gonic/gin"
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The counters are stored in package-level variables, so in a horizontally-scaled deployment each process maintains its own independent counts. A single IP effectively gets configured_limit * N concurrent requests across N instances. This is not necessarily a bug, but it will surprise operators who set a tight limit and still see it exceeded. Add a comment here (or in the setting's description string in bootstrap/data/setting.go) calling this out, e.g. // NOTE: counts are per-process; in multi-instance deployments the effective limit is limit*N.

Comment thread server/router.go Outdated
signCheck := middlewares.Down(sign.Verify)
g.GET("/d/*path", middlewares.PathParse, signCheck, downloadLimiter, handles.Down)
g.GET("/p/*path", middlewares.PathParse, signCheck, downloadLimiter, handles.Proxy)
g.GET("/p/*path", middlewares.PathParse, signCheck, downloadLimiter, proxyConcurrencyLimiter, handles.Proxy)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the GET proxy routes, proxyConcurrencyLimiter is placed after downloadLimiter. Requests that are already over the IP limit will be rejected with 429, but only after downloadLimiter has already wrapped the response writer. Swapping the two so the order becomes signCheck → proxyConcurrencyLimiter → downloadLimiter → handler short-circuits rejected requests one step earlier with no change in correctness or security.

ip := c.ClientIP()

proxyIPCountsMu.Lock()
count := proxyIPCounts[ip]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 429 response body is hand-rolled as gin.H{"code": ..., "message": ..., "data": nil}. Double-check that this matches the project's shared error-response helper (if one exists, e.g. common.ErrorResp or similar). Using a different shape here would make client-side error handling inconsistent with every other endpoint in the API.

@Roberta001 Roberta001 requested a review from utafrali April 18, 2026 10:10
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a configurable per-IP concurrency limit for the server-side proxy endpoints (/p/*path and /ap/*path) to mitigate scenarios where web proxy mode is overwhelmed by multi-threaded downloaders.

Changes:

  • Added a new Gin middleware to enforce per-IP concurrent request limits for proxy endpoints.
  • Wired the new middleware into the /p and /ap GET/HEAD routes.
  • Introduced new settings keys (and default setting items) to configure the limit and optionally choose which request header to use for client IP extraction.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
server/router.go Applies the new per-IP proxy concurrency limiter middleware to /p and /ap routes.
server/middlewares/ip_limit.go Implements the per-process, per-IP concurrent request limiter for proxy requests.
internal/conf/const.go Adds new setting key constants for proxy per-IP concurrency limiting and client-IP header selection.
internal/bootstrap/data/setting.go Registers default values/help text for the new settings in the Traffic settings group.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

{Key: conf.TaskDecompressDownloadThreadsNum, Value: strconv.Itoa(conf.Conf.Tasks.Decompress.Workers), Type: conf.TypeNumber, Group: model.TRAFFIC, Flag: model.PRIVATE},
{Key: conf.TaskDecompressUploadThreadsNum, Value: strconv.Itoa(conf.Conf.Tasks.DecompressUpload.Workers), Type: conf.TypeNumber, Group: model.TRAFFIC, Flag: model.PRIVATE},
{Key: conf.ProxyMaxConcurrentRequestsPerIP, Value: "-1", Type: conf.TypeNumber, Group: model.TRAFFIC, Flag: model.PRIVATE, Help: "Limit the maximum number of concurrent proxy requests per IP. -1 means unlimited, 0 means disabled. NOTE: This limit relies on the client IP address. To prevent IP spoofing via headers, ensure your reverse proxy correctly overwrites X-Forwarded-For and X-Real-IP headers. Also, these counts are per-process; in multi-instance deployments, the effective limit is limit * N instances."},
{Key: conf.ProxyClientIPHeader, Value: "", Type: conf.TypeString, Group: model.TRAFFIC, Flag: model.PRIVATE, Help: "Custom HTTP header to extract the client IP for proxy limits (e.g., 'X-Forwarded-For', 'CF-Connecting-IP'). Leave empty to use strict strict remote address (c.Request.RemoteAddr) which prevents IP spoofing but breaks behind reverse proxies without transparent IP capabilities."},
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The help text contains a duplicated word: "strict strict remote address". Please fix to "strict remote address" (or rephrase) to avoid confusing admins reading the setting description.

Suggested change
{Key: conf.ProxyClientIPHeader, Value: "", Type: conf.TypeString, Group: model.TRAFFIC, Flag: model.PRIVATE, Help: "Custom HTTP header to extract the client IP for proxy limits (e.g., 'X-Forwarded-For', 'CF-Connecting-IP'). Leave empty to use strict strict remote address (c.Request.RemoteAddr) which prevents IP spoofing but breaks behind reverse proxies without transparent IP capabilities."},
{Key: conf.ProxyClientIPHeader, Value: "", Type: conf.TypeString, Group: model.TRAFFIC, Flag: model.PRIVATE, Help: "Custom HTTP header to extract the client IP for proxy limits (e.g., 'X-Forwarded-For', 'CF-Connecting-IP'). Leave empty to use strict remote address (c.Request.RemoteAddr) which prevents IP spoofing but breaks behind reverse proxies without transparent IP capabilities."},

Copilot uses AI. Check for mistakes.
Comment on lines +42 to +46
// Extract IP based on user configuration to prevent spoofing
if ipHeader != "" {
ip = c.Request.Header.Get(ipHeader)
if idx := strings.Index(ip, ","); idx != -1 {
ip = ip[:idx]
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says this header-based extraction is "to prevent spoofing", but the code will trust whatever value is in the configured header. To reduce spoofing/misconfiguration impact, validate/normalize the extracted value (e.g., net.ParseIP after trimming/splitting) and fall back to RemoteAddr when invalid/empty; also update the comment to reflect the actual trust model.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants