diff --git a/changes.md b/changes.md new file mode 100644 index 0000000..71749f5 --- /dev/null +++ b/changes.md @@ -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 ./...` 编译验证。 diff --git a/handler/file.go b/handler/file.go index 893c844..02eb559 100644 --- a/handler/file.go +++ b/handler/file.go @@ -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 } diff --git a/handler/proxy.go b/handler/proxy.go index ee56386..de24c1a 100644 --- a/handler/proxy.go +++ b/handler/proxy.go @@ -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") diff --git a/server/middleware.go b/server/middleware.go index a9dca35..fdbab6f 100644 --- a/server/middleware.go +++ b/server/middleware.go @@ -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,29 +200,18 @@ 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 { - http.Error(w, "Forbidden", http.StatusForbidden) - return - } + 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 { - http.Error(w, "Forbidden", http.StatusForbidden) - return - } + if len(allowedIPs) > 0 && !slices.Contains(allowedIPs, clientIP) { + http.Error(w, "Forbidden", http.StatusForbidden) + return } } else { http.Error(w, "Server Config Error", http.StatusInternalServerError) diff --git a/server/server.go b/server/server.go index 9f87d84..b67677c 100644 --- a/server/server.go +++ b/server/server.go @@ -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 }