From 9b9c5df676f465af13d1a128105b0ef70afe5fea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=AD=E5=B7=9E=E6=98=8E=E5=A9=B3=E7=A7=91=E6=8A=80?= Date: Wed, 8 Jul 2026 12:21:13 +0800 Subject: [PATCH] fix storage safety edge cases --- storage/storage.go | 51 +++++++++++++++++++--- storage/storage_path_internal_test.go | 26 ++++++++++++ storage/storage_security_test.go | 61 ++++++++++++++++++++++++--- 3 files changed, 124 insertions(+), 14 deletions(-) diff --git a/storage/storage.go b/storage/storage.go index 96e00ca..a6b9d1f 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -41,8 +41,10 @@ var ( ErrPathTraversal = errors.New("path traversal detected") // ErrInvalidPath 路径非法（空、含 NUL 等）。 ErrInvalidPath = errors.New("invalid path") - // ErrUploadTooLarge 上传实际字节数超过 MaxSizeBytes（P0）。客户端声明的 file.Size - // 不可信，故除前置校验外，拷贝阶段按实际字节封顶，防止声明小体积却流式发送大 body 撑爆磁盘/OSS。 + // ErrInvalidFile 上传文件参数非法，例如 nil *multipart.FileHeader。 + ErrInvalidFile = errors.New("invalid file") + // ErrUploadTooLarge 上传声明大小或实际字节数超过 MaxSizeBytes（P0）。客户端声明的 file.Size + // 不可信，故除前置校验外，拷贝阶段也按实际字节封顶，防止声明小体积却流式发送大 body 撑爆磁盘/OSS。 ErrUploadTooLarge = errors.New("upload exceeds max size") ) @@ -71,7 +73,7 @@ func resolveMaxRead(n int64) int64 { // 在拷贝阶段按实际字节数二次把关（P0）。 func validateUploadSize(p config.UploadPolicy, size int64) error { if p.MaxSizeBytes > 0 && size > p.MaxSizeBytes { - return fmt.Errorf("文件大小 %d 超过上限 %d: %w", size, p.MaxSizeBytes, ErrInvalidPath) + return fmt.Errorf("文件大小 %d 超过上限 %d: %w", size, p.MaxSizeBytes, ErrUploadTooLarge) } return nil } @@ -186,6 +188,14 @@ func sanitizeObjectKey(key string) (string, error) { return path.Clean(key), nil } +func publicURL(baseURL, key string) string { + cleanKey, err := sanitizeObjectKey(key) + if err != nil { + return "" + } + return strings.TrimRight(baseURL, "/") + "/" + cleanKey +} + // LocalStorage 本地存储 type LocalStorage struct { rootAbs string // 绝对根路径（已 Clean），前缀锚定用（C4a） @@ -199,7 +209,12 @@ type LocalStorage struct { // 安全约束：Path 指向的根目录应为框架独占目录，不与用户可控内容混用。 // safeJoin 已防 `..` 路径穿越，但若根目录内已存在指向外部的符号链接， // Get 会跟随 symlink 读到根外内容——需保证攻击者无法在根目录内创建 symlink。 +// cfg 为 nil、Path 无法解析或根目录本身是 symlink 时，实例 fail-closed： +// 后续文件操作返回 ErrStorageNotInitialized。 func NewLocalStorage(cfg *config.LocalStorageConfig) *LocalStorage { + if cfg == nil { + return &LocalStorage{maxReadBytes: resolveMaxRead(0)} + } // 用绝对路径作根锚定，避免相对路径 + `..` 组合绕过前缀校验（C4a）。 var rootAbs string abs, err := filepath.Abs(cfg.Path) @@ -213,6 +228,12 @@ func NewLocalStorage(cfg *config.LocalStorageConfig) *LocalStorage { } else { rootAbs = filepath.Clean(abs) } + if rootAbs != "" { + if info, err := os.Lstat(rootAbs); err == nil && info.Mode()&os.ModeSymlink != 0 { + logger.Error("storage: local storage root must not be a symlink", zap.String("path", rootAbs)) + rootAbs = "" + } + } return &LocalStorage{ rootAbs: rootAbs, baseURL: cfg.BaseURL, @@ -245,6 +266,9 @@ func (s *LocalStorage) safeJoin(parts ...string) (string, error) { // Upload 上传文件 func (s *LocalStorage) Upload(file *multipart.FileHeader, subdir string) (ret string, err error) { + if file == nil { + return "", ErrInvalidFile + } // 上传安全策略校验（大小 / 扩展名）；file.Size 由 multipart 解析时填，无需打开文件（C4b）。 if err := validateUploadSize(s.policy, file.Size); err != nil { return "", err @@ -391,7 +415,7 @@ func (s *LocalStorage) UploadFromBytes(data []byte, filename, subdir string) (re // GetURL 获取文件访问 URL func (s *LocalStorage) GetURL(path string) string { - return fmt.Sprintf("%s/%s", s.baseURL, path) + return publicURL(s.baseURL, path) } // Delete 删除文件 @@ -462,6 +486,9 @@ type OSSStorage struct { // NewOSSStorage 创建 OSS 存储实例 func NewOSSStorage(cfg *config.OSSStorageConfig) (*OSSStorage, error) { + if cfg == nil { + return nil, ErrStorageNotInitialized + } client, err := oss.New(cfg.Endpoint, cfg.AccessKeyID, cfg.AccessKeySecret) if err != nil { return nil, fmt.Errorf("创建 OSS 客户端失败: %w", err) @@ -485,6 +512,9 @@ func NewOSSStorage(cfg *config.OSSStorageConfig) (*OSSStorage, error) { // Upload 上传文件到 OSS func (s *OSSStorage) Upload(file *multipart.FileHeader, subdir string) (string, error) { + if file == nil { + return "", ErrInvalidFile + } // 上传安全策略校验（C4b） if err := validateUploadSize(s.policy, file.Size); err != nil { return "", err @@ -576,10 +606,14 @@ func (s *OSSStorage) UploadFromBytes(data []byte, filename, subdir string) (stri // GetURL 获取文件访问 URL func (s *OSSStorage) GetURL(path string) string { - if s.baseURL != "" { - return fmt.Sprintf("%s/%s", s.baseURL, path) + cleanKey, err := sanitizeObjectKey(path) + if err != nil { + return "" } - return fmt.Sprintf("https://%s.%s/%s", s.bucketName, s.endpoint, path) + if s.baseURL != "" { + return strings.TrimRight(s.baseURL, "/") + "/" + cleanKey + } + return fmt.Sprintf("https://%s.%s/%s", s.bucketName, s.endpoint, cleanKey) } // GetSignedURL 获取带签名的临时访问 URL（用于私有文件） @@ -674,6 +708,9 @@ func SetDefaultStorageManager(m *StorageManager) { // Init 初始化存储 func (m *StorageManager) Init(cfg *config.StorageConfig) error { + if cfg == nil { + return ErrStorageNotInitialized + } m.mu.Lock() defer m.mu.Unlock() diff --git a/storage/storage_path_internal_test.go b/storage/storage_path_internal_test.go index 22e3787..c4656b4 100644 --- a/storage/storage_path_internal_test.go +++ b/storage/storage_path_internal_test.go @@ -1,6 +1,7 @@ package storage import ( + "errors" "strings" "testing" ) @@ -64,3 +65,28 @@ func TestResolveMaxRead(t *testing.T) { t.Errorf("resolveMaxRead(2048) = %d, want 2048", got) } } + +func TestNewOSSStorageNilConfig(t *testing.T) { + if _, err := NewOSSStorage(nil); !errors.Is(err, ErrStorageNotInitialized) { + t.Fatalf("NewOSSStorage(nil) err = %v, want ErrStorageNotInitialized", err) + } +} + +func TestOSSStorageGetURLSanitizesPath(t *testing.T) { + s := &OSSStorage{ + baseURL: "https://cdn.example.com/", + bucketName: "bucket", + endpoint: "oss.example.com", + } + if got := s.GetURL("docs//a.txt"); got != "https://cdn.example.com/docs/a.txt" { + t.Fatalf("GetURL clean path = %q", got) + } + if got := s.GetURL("../secret.txt"); got != "" { + t.Fatalf("GetURL traversal = %q, want empty", got) + } + + s.baseURL = "" + if got := s.GetURL("docs/a.txt"); got != "https://bucket.oss.example.com/docs/a.txt" { + t.Fatalf("GetURL endpoint fallback = %q", got) + } +} diff --git a/storage/storage_security_test.go b/storage/storage_security_test.go index 8c6c99a..737bd11 100644 --- a/storage/storage_security_test.go +++ b/storage/storage_security_test.go @@ -196,8 +196,8 @@ func TestLocalStorageGetReadLimit(t *testing.T) { func TestLocalStorageUploadSizeLimit(t *testing.T) { s := newLocalStorageWithPolicy(t, config.UploadPolicy{MaxSizeBytes: 5}, 0) fh := makeFileHeader(t, "file", "big.txt", "text/plain", []byte("0123456789")) // 10 字节 - if _, err := s.Upload(fh, "docs"); !errors.Is(err, storage.ErrInvalidPath) { - t.Errorf("Upload over size err = %v, want ErrInvalidPath", err) + if _, err := s.Upload(fh, "docs"); !errors.Is(err, storage.ErrUploadTooLarge) { + t.Errorf("Upload over size err = %v, want ErrUploadTooLarge", err) } } @@ -239,14 +239,14 @@ func TestLocalStorageUploadMIMESniff(t *testing.T) { // 回归 C4b：UploadFromBytes 同样受策略约束（大小 / 扩展名 / MIME）。 func TestLocalStorageUploadFromBytesPolicy(t *testing.T) { s := newLocalStorageWithPolicy(t, config.UploadPolicy{ - MaxSizeBytes: 100, - AllowedExts: []string{".txt"}, - AllowedMIMEs: []string{"text/plain"}, + MaxSizeBytes: 100, + AllowedExts: []string{".txt"}, + AllowedMIMEs: []string{"text/plain"}, }, 0) // 超限 - if _, err := s.UploadFromBytes(make([]byte, 200), "ok.txt", "docs"); !errors.Is(err, storage.ErrInvalidPath) { - t.Errorf("UploadFromBytes over size err = %v, want ErrInvalidPath", err) + if _, err := s.UploadFromBytes(make([]byte, 200), "ok.txt", "docs"); !errors.Is(err, storage.ErrUploadTooLarge) { + t.Errorf("UploadFromBytes over size err = %v, want ErrUploadTooLarge", err) } // 扩展名不符 if _, err := s.UploadFromBytes([]byte("hi"), "evil.php", "docs"); !errors.Is(err, storage.ErrInvalidPath) { @@ -295,3 +295,50 @@ func TestLocalStorageZeroPolicyAllowsAll(t *testing.T) { t.Errorf("Upload with zero policy err = %v (must remain unrestricted for backward compat)", err) } } + +func TestLocalStorageNilConfigFailsClosed(t *testing.T) { + s := storage.NewLocalStorage(nil) + if _, err := s.Get("x.txt"); !errors.Is(err, storage.ErrStorageNotInitialized) { + t.Fatalf("Get with nil config err = %v, want ErrStorageNotInitialized", err) + } +} + +func TestStorageInitNilConfigNoPanic(t *testing.T) { + if err := storage.NewStorageManager().Init(nil); !errors.Is(err, storage.ErrStorageNotInitialized) { + t.Fatalf("Init(nil) err = %v, want ErrStorageNotInitialized", err) + } +} + +func TestLocalStorageUploadNilFile(t *testing.T) { + s := newLocalStorageWithPolicy(t, config.UploadPolicy{}, 0) + if _, err := s.Upload(nil, "docs"); !errors.Is(err, storage.ErrInvalidFile) { + t.Fatalf("Upload(nil) err = %v, want ErrInvalidFile", err) + } +} + +func TestLocalStorageRejectsSymlinkRoot(t *testing.T) { + parent := t.TempDir() + target := filepath.Join(parent, "target") + if err := os.Mkdir(target, 0755); err != nil { + t.Fatalf("mkdir target: %v", err) + } + link := filepath.Join(parent, "link") + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlink not available on this system: %v", err) + } + + s := storage.NewLocalStorage(&config.LocalStorageConfig{Path: link, BaseURL: "http://localhost/uploads"}) + if _, err := s.Get("x.txt"); !errors.Is(err, storage.ErrStorageNotInitialized) { + t.Fatalf("Get with symlink root err = %v, want ErrStorageNotInitialized", err) + } +} + +func TestLocalStorageGetURLSanitizesPath(t *testing.T) { + s := newLocalStorageWithPolicy(t, config.UploadPolicy{}, 0) + if got := s.GetURL("docs//a.txt"); got != "http://localhost/uploads/docs/a.txt" { + t.Fatalf("GetURL clean path = %q", got) + } + if got := s.GetURL("../secret.txt"); got != "" { + t.Fatalf("GetURL traversal = %q, want empty", got) + } +}