feat: add configurable sign-in attempts limit and IP allow/deny lists#2776
feat: add configurable sign-in attempts limit and IP allow/deny lists#2776qiuxiuya wants to merge 7 commits into
Conversation
* fix: replace strings.HasPrefix with utils.IsSubPath for path validation Signed-off-by: MadDogOwner <xiaoran@xrgzs.top> * fix: re-validate shared paths to ensure they remain within the creator's base path Signed-off-by: MadDogOwner <xiaoran@xrgzs.top> --------- Signed-off-by: MadDogOwner <xiaoran@xrgzs.top> Co-authored-by: MadDogOwner <xiaoran@xrgzs.top>
…m#2757) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…eam#2756) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…eam#2754) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…tTeam#2739) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…istTeam#2743) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
|
|
||
| // login lock settings | ||
| {Key: conf.LoginLockDuration, Value: "5", Type: conf.TypeNumber, Group: model.SITE, Flag: model.PRIVATE}, | ||
| {Key: conf.LoginMaxRetries, Value: "5", Type: conf.TypeNumber, Group: model.SITE, Flag: model.PRIVATE}, |
There was a problem hiding this comment.
为了保证兼容性应该设置-1 但是给真正默认值确实更安全,这个需要团队内其他开发看下考虑下
| } | ||
|
|
||
| // matchIPList checks if the given IP matches any entry in the list. | ||
| func matchIPList(ip string, list []string) bool { |
There was a problem hiding this comment.
这里直接用字符串比较可能不太合适:前面解析 ip 时做了 TrimSpace,但这里比较的仍是原始 ip,带空白时会出现匹配失败;另外,IPv6 的不同等价表示也无法通过字符串比较匹配。
建议将 entry 同样解析为 IP,并使用 parsedEntry.Equal(parsedIP) 比较,这样逻辑会更一致。
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc16024c43
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // check IP blacklist | ||
| if model.IsIPBlacklisted(ip, model.ParseIPList(setting.GetStr(conf.LoginIPBlacklist))) { | ||
| return nil, errors.New(model.TooManyAttempts) |
There was a problem hiding this comment.
Apply the blacklist to all SFTP auth paths
Because the blacklist is checked only inside PasswordAuth, it is skipped for SFTP public-key logins and the no-client-auth guest path. In deployments that set login_ip_blacklist to block an address, any user with a registered SSH key (or an enabled guest no-auth login) can still authenticate from that blacklisted IP, so the deny list does not actually protect the SFTP endpoint unless every non-password auth method is disabled.
Useful? React with 👍 / 👎.
| // check IP blacklist | ||
| if model.IsIPBlacklisted(ip, model.ParseIPList(setting.GetStr(conf.LoginIPBlacklist))) { | ||
| return nil, errors.New(model.TooManyAttempts) |
There was a problem hiding this comment.
Strip the FTP peer port before IP matching
For FTP clients, cc.RemoteAddr().String() is a host:port value, but matchIPList calls net.ParseIP on the whole string, so configured single-IP and CIDR entries never match this blacklist check. In deployments relying on login_ip_blacklist for FTP, a blocked address can still authenticate normally because the deny list is effectively ignored for every new TCP connection.
Useful? React with 👍 / 👎.
| // check IP whitelist (bypasses lock) | ||
| whitelist := model.ParseIPList(setting.GetStr(conf.LoginIPWhitelist)) | ||
| if model.IsIPWhitelisted(ip, whitelist) { | ||
| // whitelisted IP, skip lock check | ||
| } else if model.CheckLoginLocked(ip, maxRetries, lockDuration) { |
There was a problem hiding this comment.
Avoid trusting spoofable headers for the whitelist
When an admin configures login_ip_whitelist, this branch can be reached with a forged client IP on direct HTTP deployments or proxies that do not sanitize X-Forwarded-For: I checked internal/bootstrap/run.go and the Gin engine is created with gin.New() without any SetTrustedProxies call, so Gin's default trusts forwarded IP headers. A client can set X-Forwarded-For to a whitelisted address and skip both the lock check and failure recording.
Useful? React with 👍 / 👎.
| // check IP blacklist | ||
| if model.IsIPBlacklisted(ip, model.ParseIPList(setting.GetStr(conf.LoginIPBlacklist))) { | ||
| return nil, errors.New(model.TooManyAttempts) |
There was a problem hiding this comment.
Strip the SFTP peer port before IP matching
Even on the password-auth path, conn.RemoteAddr().String() is a host:port value, so IsIPBlacklisted cannot parse it as an IP and configured single-IP/CIDR deny entries never match. In SFTP deployments that rely on login_ip_blacklist, a blacklisted address can still authenticate with a password because this check is effectively bypassed before credential validation.
Useful? React with 👍 / 👎.
| // RecordLoginAttempt records a failed login attempt for the IP and returns the new count. | ||
| func RecordLoginAttempt(ip string) int { | ||
| count, _ := LoginCache.Get(ip) | ||
| LoginCache.Set(ip, count+1) | ||
| return count + 1 |
There was a problem hiding this comment.
Stop recording attempts while locking is disabled
login_max_retries <= 0 is documented by CheckLoginLocked as disabling the lock feature, but failed logins still call this helper and accumulate counters in LoginCache. If an admin temporarily sets login_max_retries to -1, failed attempts during that period can immediately lock the same IP as soon as the setting is restored, even though locking was supposed to be disabled at the time.
Useful? React with 👍 / 👎.

Summary / 摘要
设置-站点登录限流的配置
设置-站点新增 4 个配置项:
login_lock_duration、login_max_retries、login_ip_whitelist、login_ip_blacklistlogin_max_retries设为-1可完全禁用登录锁定功能This PR has breaking changes.
/ 此 PR 包含破坏性变更。
This PR changes public API, config, storage format, or migration behavior.
/ 此 PR 修改了公开 API、配置、存储格式或迁移行为。
This PR requires corresponding changes in related repositories.
/ 此 PR 需要关联仓库同步修改。
Related repository PRs / 关联仓库 PR:
Related Issues / 关联 Issue
无
Testing / 测试
Checklist / 检查清单
/ 我已阅读 CONTRIBUTING。
/ 我确认此贡献符合仓库许可证、贡献规范和行为准则。
gofmt,go fmt, orprettierwhere applicable./ 我已按适用情况使用
gofmt、go fmt或prettier格式化变更代码。/ 我已在适用情况下请求相关维护者或代码所有者审查。
AI Disclosure / AI 使用声明
/ 此 PR 包含 AI 辅助内容。
Tools used / 使用工具:
Usage scope / 使用范围:
Code generation / 代码生成
Refactoring / 重构
Documentation / 文档
Tests / 测试
Translation / 翻译
Review assistance / 审查辅助
I have reviewed and validated all AI-assisted content included in this PR.
/ 我已审核并验证此 PR 中的所有 AI 辅助内容。
[] I have ensured that all AI-assisted commits include
Co-Authored-Byattribution./ 我已确保所有 AI 辅助提交都包含
Co-Authored-By归属信息。I can reproduce all AI-assisted content included in this PR without any AI tools.
/ 我可以在没有任何 AI 工具的情况下重现此 PR 中包含的所有 AI 辅助内容。