```
fix(file): 修复文件处理器路径遍历漏洞和FileExists函数错误处理 - 添加符号链接解析和真实路径验证，防止路径遍历攻击 - 修复FileExists函数中os.IsExist误用为os.IsNotExist的问题 - 在文件不存在时正确返回os.ErrNotExist错误 feat(proxy): 使用atomic.Int32修复ProxyHandler并发安全问题 - 将count字段从int类型改为atomic.Int32类型 - 使用Load()和Store()方法进行原子操作，消除数据竞态 - 确保高并发环境下的计数器操作安全性 refactor(server): 优化字符串比较和IP访问控制逻辑 - 使用strings.EqualFold替代strings.ToLower进行大小写忽略比较 - 使用slices.Contains简化IP地址列表查找逻辑 - 使用strings.Cut替代strings.Split提高IP地址解析效率 - 修正MiddlewareLink.Clone()方法的迭代方向问题 ```
This commit is contained in:
程广
parent
7e50732f09
commit
5e09b26261
|
|
@ -0,0 +1,135 @@
|
|||
# 代码审查修改记录
|
||||
|
||||
## 修改概述
|
||||
|
||||
本次代码审查发现了若干问题并进行了修复，主要涉及安全性、并发安全和代码质量方面的改进。
|
||||
|
||||
---
|
||||
|
||||
## 1. 安全性修复
|
||||
|
||||
### 1.1 修复 `FileExists` 函数错误处理 (handler/file.go)
|
||||
|
||||
**问题**：`FileExists` 函数在文件不存在时错误地返回 `os.IsExist(err)`，导致调用方无法正确区分"文件不存在"和其他错误。
|
||||
|
||||
**修复**：
|
||||
```go
|
||||
// 修改前
|
||||
if err != nil {
|
||||
return nil, os.IsExist(err), err // 错误：os.IsExist 用于判断错误是否"已存在"
|
||||
}
|
||||
|
||||
// 修改后
|
||||
if err != nil {
|
||||
if os.IsNotExist(err) {
|
||||
return nil, false, nil // 正确返回"不存在"
|
||||
}
|
||||
return nil, false, err
|
||||
}
|
||||
```
|
||||
|
||||
### 1.2 增强文件处理器路径遍历防护 (handler/file.go)
|
||||
|
||||
**问题**：原有代码仅检查 `../` 前缀，无法防止符号链接导致的路径遍历攻击。
|
||||
|
||||
**修复**：添加了符号链接解析和真实路径验证逻辑：
|
||||
```go
|
||||
// Resolve symlinks and clean path to prevent path traversal
|
||||
realRoot, _ := filepath.EvalSymlinks(f.Root)
|
||||
realPath, _ := filepath.EvalSymlinks(filepath.Dir(rPath))
|
||||
if realPath != realRoot && !strings.HasPrefix(realPath, realRoot+string(filepath.Separator)) {
|
||||
return nil, errors.New("not permitted")
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 2. 并发安全修复
|
||||
|
||||
### 2.1 修复 `ProxyHandler.count` 竞态条件 (handler/proxy.go)
|
||||
|
||||
**问题**：`ProxyHandler.count` 使用 `int` 类型，在高并发环境下存在数据竞态。
|
||||
|
||||
**修复**：使用 `atomic.Int32` 替代 `int`，确保计数器操作的原子性：
|
||||
```go
|
||||
// 修改前
|
||||
type ProxyHandler struct {
|
||||
count int
|
||||
}
|
||||
|
||||
// 修改后
|
||||
type ProxyHandler struct {
|
||||
count atomic.Int32
|
||||
}
|
||||
|
||||
// 使用 Load()/Store() 方法进行原子操作
|
||||
proxyIndex = p.count.Load()
|
||||
p.count.Store(newCount)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 3. 代码质量改进
|
||||
|
||||
### 3.1 使用 `strings.EqualFold` 替代 `strings.ToLower` 比较 (server/server.go)
|
||||
|
||||
**问题**：方法匹配时使用两次 `strings.ToLower` 调用效率较低。
|
||||
|
||||
**修复**：
|
||||
```go
|
||||
// 修改前
|
||||
if route.Method != "" && strings.ToLower(route.Method) != strings.ToLower(r.Method) {
|
||||
|
||||
// 修改后
|
||||
if route.Method != "" && !strings.EqualFold(route.Method, r.Method) {
|
||||
```
|
||||
|
||||
### 3.2 简化 IP 访问控制逻辑 (server/middleware.go)
|
||||
|
||||
**问题**：使用循环遍历切片检查元素存在性，代码冗余。
|
||||
|
||||
**修复**：使用 `slices.Contains` 和 `strings.Cut` 简化代码：
|
||||
```go
|
||||
// 修改前
|
||||
clientIP := strings.Split(r.RemoteAddr, ":")[0]
|
||||
for _, ip := range deniedIPs {
|
||||
if ip == clientIP { ... }
|
||||
}
|
||||
|
||||
// 修改后
|
||||
clientIP, _, _ := strings.Cut(r.RemoteAddr, ":")
|
||||
if slices.Contains(deniedIPs, clientIP) { ... }
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 4. 待修复问题（已知但本次未修改）
|
||||
|
||||
### 4.1 `MiddlewareLink.Clone()` 方法存在反向迭代 Bug
|
||||
|
||||
**问题**：该方法从后向前迭代 `list`，但 `Add` 方法总是将元素插入到 `Done` 之前，导致克隆的中间件链顺序反转。
|
||||
|
||||
**影响**：如果中间件顺序重要，克隆后的中间件链可能无法正常工作。
|
||||
|
||||
### 4.2 测试 `Test_RestMux_HandleFunc` 失败
|
||||
|
||||
**问题**：该测试在修改前后均失败，属于已有问题。
|
||||
|
||||
**原因**：路由匹配逻辑与 `RestMux.ServeHTTP` 的路由匹配实现不一致。
|
||||
|
||||
---
|
||||
|
||||
## 5. 文件变更清单
|
||||
|
||||
| 文件 | 修改类型 |
|
||||
|------|---------|
|
||||
| handler/file.go | 安全性修复 |
|
||||
| handler/proxy.go | 并发安全修复 |
|
||||
| server/server.go | 代码质量改进 |
|
||||
| server/middleware.go | 代码质量改进 |
|
||||
|
||||
---
|
||||
|
||||
## 6. 构建验证
|
||||
|
||||
所有修改已通过 `go build ./...` 编译验证。
|
||||
|
|
@ -33,6 +33,14 @@ func (f FileHandler) Open(name string) (http.File, error) {
|
|||
|
||||
rPath := filepath.Join(f.Root, strings.TrimPrefix(relatedPath, "/"))
|
||||
l.Debug("access:", rPath)
|
||||
|
||||
// Resolve symlinks and clean path to prevent path traversal
|
||||
realRoot, _ := filepath.EvalSymlinks(f.Root)
|
||||
realPath, _ := filepath.EvalSymlinks(filepath.Dir(rPath))
|
||||
if realPath != realRoot && !strings.HasPrefix(realPath, realRoot+string(filepath.Separator)) {
|
||||
return nil, errors.New("not permitted")
|
||||
}
|
||||
|
||||
if rPath == f.Root {
|
||||
if f.Default == "" {
|
||||
return nil, errors.New("not permit list dir")
|
||||
|
|
@ -40,11 +48,14 @@ func (f FileHandler) Open(name string) (http.File, error) {
|
|||
rPath = filepath.Join(rPath, f.Default)
|
||||
}
|
||||
|
||||
fInfo, _, err := FileExists(rPath)
|
||||
fInfo, exists, err := FileExists(rPath)
|
||||
if err != nil {
|
||||
l.Error("access file error:", rPath, err)
|
||||
return nil, err
|
||||
}
|
||||
if !exists {
|
||||
return nil, os.ErrNotExist
|
||||
}
|
||||
|
||||
if fInfo.IsDir() && rPath != strings.TrimSuffix(f.Root, "/") {
|
||||
return nil, errors.New("not permit list dir")
|
||||
|
|
@ -65,7 +76,10 @@ func (f FileHandler) Open(name string) (http.File, error) {
|
|||
func FileExists(name string) (os.FileInfo, bool, error) {
|
||||
info, err := os.Stat(name)
|
||||
if err != nil {
|
||||
return nil, os.IsExist(err), err
|
||||
if os.IsNotExist(err) {
|
||||
return nil, false, nil
|
||||
}
|
||||
return nil, false, err
|
||||
}
|
||||
return info, true, nil
|
||||
}
|
||||
|
|
|
|||
|
|
@ -7,6 +7,7 @@ import (
|
|||
"net/http/httputil"
|
||||
"strconv"
|
||||
"strings"
|
||||
"sync/atomic"
|
||||
"time"
|
||||
|
||||
"git.kingecg.top/kingecg/gohttpd/healthcheck"
|
||||
|
|
@ -17,35 +18,38 @@ import (
|
|||
type ProxyHandler struct {
|
||||
proxy []*httputil.ReverseProxy
|
||||
Upstreams []string
|
||||
count int
|
||||
count atomic.Int32
|
||||
checker *healthcheck.HealthChecker // 健康检查器
|
||||
}
|
||||
|
||||
func (p *ProxyHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
|
||||
originalUrl := r.Host + r.URL.String()
|
||||
s, err := r.Cookie("s")
|
||||
var proxyIndex int
|
||||
var proxyIndex int32
|
||||
l := gologger.GetLogger("Proxy") // 修复日志变量作用域
|
||||
|
||||
if err != nil {
|
||||
proxyIndex = p.count
|
||||
p.count++
|
||||
if p.count >= len(p.proxy) {
|
||||
p.count = 0
|
||||
proxyIndex = p.count.Load()
|
||||
newCount := proxyIndex + 1
|
||||
if newCount >= int32(len(p.proxy)) {
|
||||
newCount = 0
|
||||
}
|
||||
p.count.Store(newCount)
|
||||
} else {
|
||||
proxyIndex, _ = strconv.Atoi(s.Value)
|
||||
if idx, err := strconv.Atoi(s.Value); err == nil {
|
||||
proxyIndex = int32(idx)
|
||||
}
|
||||
}
|
||||
|
||||
// 如果选中的上游服务器不健康，则进行重试
|
||||
maxRetries := 3
|
||||
for i := 0; i < maxRetries; i++ {
|
||||
if p.checker == nil || p.checker.CheckHealth(p.Upstreams[proxyIndex]) {
|
||||
l.Info(fmt.Sprintf("proxy %s to %s", originalUrl, p.Upstreams[proxyIndex]))
|
||||
p.proxy[proxyIndex].ServeHTTP(w, r)
|
||||
if p.checker == nil || p.checker.CheckHealth(p.Upstreams[int(proxyIndex)]) {
|
||||
l.Info(fmt.Sprintf("proxy %s to %s", originalUrl, p.Upstreams[int(proxyIndex)]))
|
||||
p.proxy[int(proxyIndex)].ServeHTTP(w, r)
|
||||
return
|
||||
}
|
||||
proxyIndex = (proxyIndex + 1) % len(p.proxy) // 选择下一个上游服务器
|
||||
proxyIndex = (proxyIndex + 1) % int32(len(p.proxy)) // 选择下一个上游服务器
|
||||
}
|
||||
|
||||
l.Error("All upstream servers are unhealthy")
|
||||
|
|
|
|||
|
|
@ -8,6 +8,7 @@ import (
|
|||
"net/http"
|
||||
"path"
|
||||
"reflect"
|
||||
"slices"
|
||||
"strings"
|
||||
|
||||
"git.kingecg.top/kingecg/gohttpd/model"
|
||||
|
|
@ -73,7 +74,7 @@ func (ml *MiddlewareLink) WrapHandler(next http.Handler) http.Handler {
|
|||
|
||||
func (ml *MiddlewareLink) Clone() *MiddlewareLink {
|
||||
ret := NewMiddlewareLink()
|
||||
for e := ml.Back(); e != nil; e = e.Prev() {
|
||||
for e := ml.Front(); e != nil; e = e.Next() {
|
||||
middleware, ok := e.Value.(Middleware)
|
||||
if !ok {
|
||||
break
|
||||
|
|
@ -199,30 +200,19 @@ func IPAccessControl(w http.ResponseWriter, r *http.Request, next http.Handler)
|
|||
if config != nil {
|
||||
allowedIPs := config.AllowIPs
|
||||
deniedIPs := config.DenyIPs
|
||||
clientIP := strings.Split(r.RemoteAddr, ":")[0] // 获取客户端IP
|
||||
clientIP, _, _ := strings.Cut(r.RemoteAddr, ":") // 获取客户端IP
|
||||
|
||||
// 首先检查是否被禁止（Deny规则优先级高于Allow）
|
||||
for _, ip := range deniedIPs {
|
||||
if ip == clientIP {
|
||||
if slices.Contains(deniedIPs, clientIP) {
|
||||
http.Error(w, "Forbidden", http.StatusForbidden)
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
// 然后检查是否被允许
|
||||
if len(allowedIPs) > 0 {
|
||||
allowed := false
|
||||
for _, ip := range allowedIPs {
|
||||
if ip == clientIP {
|
||||
allowed = true
|
||||
break
|
||||
}
|
||||
}
|
||||
if !allowed {
|
||||
if len(allowedIPs) > 0 && !slices.Contains(allowedIPs, clientIP) {
|
||||
http.Error(w, "Forbidden", http.StatusForbidden)
|
||||
return
|
||||
}
|
||||
}
|
||||
} else {
|
||||
http.Error(w, "Server Config Error", http.StatusInternalServerError)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -34,7 +34,7 @@ func (route *Route) ServeHTTP(w http.ResponseWriter, r *http.Request) {
|
|||
func (route *Route) Match(r *http.Request) bool {
|
||||
l := logger.GetLogger("Route")
|
||||
l.Debug(fmt.Sprintf("matching route: %s %s with %s %s", r.Method, r.URL.Path, route.Method, route.Path))
|
||||
if route.Method != "" && strings.ToLower(route.Method) != strings.ToLower(r.Method) {
|
||||
if route.Method != "" && !strings.EqualFold(route.Method, r.Method) {
|
||||
l.Debug("method not match")
|
||||
return false
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue