Fix storage path logic especially for relative paths (go-gitea#26441)

This PR rewrites the function `getStorage` and make it more clear.

Include tests from go-gitea#26435, thanks @earl-warren

---------

Co-authored-by: Earl Warren <[email protected]>

Author: 2 people

modules/setting/storage.go

@@ -91,134 +91,172 @@ func getStorage(rootCfg ConfigProvider, name, typ string, sec ConfigSection) (*S
 		return nil, errors.New("no name for storage")
 	}
 
-	var targetSec ConfigSection
-	// check typ first
-	if typ != "" {
-		var err error
-		targetSec, err = rootCfg.GetSection(storageSectionName + "." + typ)
-		if err != nil {
-			if !IsValidStorageType(StorageType(typ)) {
-				return nil, fmt.Errorf("get section via storage type %q failed: %v", typ, err)
-			}
+	targetSec, tp, err := getStorageTargetSection(rootCfg, name, typ, sec)
+	if err != nil {
+		return nil, err
+	}
+
+	overrideSec := getStorageOverrideSection(rootCfg, targetSec, sec, tp, name)
+
+	targetType := targetSec.Key("STORAGE_TYPE").String()
+	switch targetType {
+	case string(LocalStorageType):
+		return getStorageForLocal(targetSec, overrideSec, tp, name)
+	case string(MinioStorageType):
+		return getStorageForMinio(targetSec, overrideSec, tp, name)
+	default:
+		return nil, fmt.Errorf("unsupported storage type %q", targetType)
+	}
+}
+
+type targetSecType int
+
+const (
+	targetSecIsTyp             targetSecType = iota // target section is [storage.type] which the type from parameter
+	targetSecIsStorage                              // target section is [storage]
+	targetSecIsDefault                              // target section is the default value
+	targetSecIsStorageWithName                      // target section is [storage.name]
+	targetSecIsSec                                  // target section is from the name seciont [name]
+)
+
+func getStorageSectionByType(rootCfg ConfigProvider, typ string) (ConfigSection, targetSecType, error) {
+	targetSec, err := rootCfg.GetSection(storageSectionName + "." + typ)
+	if err != nil {
+		if !IsValidStorageType(StorageType(typ)) {
+			return nil, 0, fmt.Errorf("get section via storage type %q failed: %v", typ, err)
 		}
-		if targetSec != nil {
-			targetType := targetSec.Key("STORAGE_TYPE").String()
-			if targetType == "" {
-				if !IsValidStorageType(StorageType(typ)) {
-					return nil, fmt.Errorf("unknow storage type %q", typ)
-				}
-				targetSec.Key("STORAGE_TYPE").SetValue(typ)
-			} else if !IsValidStorageType(StorageType(targetType)) {
-				return nil, fmt.Errorf("unknow storage type %q for section storage.%v", targetType, typ)
-			}
+		// if typ is a valid storage type, but there is no [storage.local] or [storage.minio] section
+		// it's not an error
+		return nil, 0, nil
+	}
+
+	targetType := targetSec.Key("STORAGE_TYPE").String()
+	if targetType == "" {
+		if !IsValidStorageType(StorageType(typ)) {
+			return nil, 0, fmt.Errorf("unknow storage type %q", typ)
 		}
+		targetSec.Key("STORAGE_TYPE").SetValue(typ)
+	} else if !IsValidStorageType(StorageType(targetType)) {
+		return nil, 0, fmt.Errorf("unknow storage type %q for section storage.%v", targetType, typ)
 	}
 
-	if targetSec == nil && sec != nil {
-		secTyp := sec.Key("STORAGE_TYPE").String()
-		if IsValidStorageType(StorageType(secTyp)) {
-			targetSec = sec
-		} else if secTyp != "" {
-			targetSec, _ = rootCfg.GetSection(storageSectionName + "." + secTyp)
+	return targetSec, targetSecIsTyp, nil
+}
+
+func getStorageTargetSection(rootCfg ConfigProvider, name, typ string, sec ConfigSection) (ConfigSection, targetSecType, error) {
+	// check typ first
+	if typ == "" {
+		if sec != nil { // check sec's type secondly
+			typ = sec.Key("STORAGE_TYPE").String()
+			if IsValidStorageType(StorageType(typ)) {
+				if targetSec, _ := rootCfg.GetSection(storageSectionName + "." + typ); targetSec == nil {
+					return sec, targetSecIsSec, nil
+				}
+			}
 		}
 	}
 
-	targetSecIsStoragename := false
-	storageNameSec, _ := rootCfg.GetSection(storageSectionName + "." + name)
-	if targetSec == nil {
-		targetSec = storageNameSec
-		targetSecIsStoragename = storageNameSec != nil
+	if typ != "" {
+		targetSec, tp, err := getStorageSectionByType(rootCfg, typ)
+		if targetSec != nil || err != nil {
+			return targetSec, tp, err
+		}
 	}
 
-	if targetSec == nil {
-		targetSec = getDefaultStorageSection(rootCfg)
-	} else {
+	// check stoarge name thirdly
+	targetSec, _ := rootCfg.GetSection(storageSectionName + "." + name)
+	if targetSec != nil {
 		targetType := targetSec.Key("STORAGE_TYPE").String()
 		switch {
 		case targetType == "":
-			if targetSec != storageNameSec && storageNameSec != nil {
-				targetSec = storageNameSec
-				targetSecIsStoragename = true
-				if targetSec.Key("STORAGE_TYPE").String() == "" {
-					return nil, fmt.Errorf("storage section %s.%s has no STORAGE_TYPE", storageSectionName, name)
-				}
-			} else {
-				if targetSec.Key("PATH").String() == "" {
-					targetSec = getDefaultStorageSection(rootCfg)
-				} else {
-					targetSec.Key("STORAGE_TYPE").SetValue("local")
-				}
+			if targetSec.Key("PATH").String() == "" { // both storage type and path are empty, use default
+				return getDefaultStorageSection(rootCfg), targetSecIsDefault, nil
 			}
+
+			targetSec.Key("STORAGE_TYPE").SetValue("local")
 		default:
-			newTargetSec, _ := rootCfg.GetSection(storageSectionName + "." + targetType)
-			if newTargetSec == nil {
-				if !IsValidStorageType(StorageType(targetType)) {
-					return nil, fmt.Errorf("invalid storage section %s.%q", storageSectionName, targetType)
-				}
-			} else {
-				targetSec = newTargetSec
-				if IsValidStorageType(StorageType(targetType)) {
-					tp := targetSec.Key("STORAGE_TYPE").String()
-					if tp == "" {
-						targetSec.Key("STORAGE_TYPE").SetValue(targetType)
-					}
-				}
+			targetSec, tp, err := getStorageSectionByType(rootCfg, targetType)
+			if targetSec != nil || err != nil {
+				return targetSec, tp, err
 			}
 		}
+
+		return targetSec, targetSecIsStorageWithName, nil
 	}
 
-	targetType := targetSec.Key("STORAGE_TYPE").String()
-	if !IsValidStorageType(StorageType(targetType)) {
-		return nil, fmt.Errorf("invalid storage type %q", targetType)
+	return getDefaultStorageSection(rootCfg), targetSecIsDefault, nil
+}
+
+// getStorageOverrideSection override section will be read SERVE_DIRECT, PATH, MINIO_BASE_PATH, MINIO_BUCKET to override the targetsec when possible
+func getStorageOverrideSection(rootConfig ConfigProvider, targetSec, sec ConfigSection, targetSecType targetSecType, name string) ConfigSection {
+	if targetSecType == targetSecIsSec {
+		return nil
 	}
 
-	// extra config section will be read SERVE_DIRECT, PATH, MINIO_BASE_PATH, MINIO_BUCKET to override the targetsec when possible
-	extraConfigSec := sec
-	if extraConfigSec == nil {
-		extraConfigSec = storageNameSec
+	if sec != nil {
+		return sec
 	}
 
-	var storage Storage
-	storage.Type = StorageType(targetType)
+	if targetSecType != targetSecIsStorageWithName {
+		nameSec, _ := rootConfig.GetSection(storageSectionName + "." + name)
+		return nameSec
+	}
+	return nil
+}
 
-	switch targetType {
-	case string(LocalStorageType):
-		targetPath := ConfigSectionKeyString(targetSec, "PATH", "")
-		if targetPath == "" {
-			targetPath = AppDataPath
-		} else if !filepath.IsAbs(targetPath) {
-			targetPath = filepath.Join(AppDataPath, targetPath)
-		}
+func getStorageForLocal(targetSec, overrideSec ConfigSection, tp targetSecType, name string) (*Storage, error) {
+	storage := Storage{
+		Type: StorageType(targetSec.Key("STORAGE_TYPE").String()),
+	}
 
-		var fallbackPath string
-		if targetSecIsStoragename {
-			fallbackPath = targetPath
-		} else {
+	targetPath := ConfigSectionKeyString(targetSec, "PATH", "")
+	var fallbackPath string
+	if targetPath == "" { // no path
+		fallbackPath = filepath.Join(AppDataPath, name)
+	} else {
+		if tp == targetSecIsStorage || tp == targetSecIsDefault {
 			fallbackPath = filepath.Join(targetPath, name)
+		} else {
+			fallbackPath = targetPath
+		}
+		if !filepath.IsAbs(fallbackPath) {
+			fallbackPath = filepath.Join(AppDataPath, fallbackPath)
 		}
+	}
 
-		if extraConfigSec == nil {
+	if overrideSec == nil { // no override section
+		storage.Path = fallbackPath
+	} else {
+		storage.Path = ConfigSectionKeyString(overrideSec, "PATH", "")
+		if storage.Path == "" { // overrideSec has no path
 			storage.Path = fallbackPath
-		} else {
-			storage.Path = ConfigSectionKeyString(extraConfigSec, "PATH", fallbackPath)
-			if !filepath.IsAbs(storage.Path) {
+		} else if !filepath.IsAbs(storage.Path) {
+			if targetPath == "" {
+				storage.Path = filepath.Join(AppDataPath, storage.Path)
+			} else {
 				storage.Path = filepath.Join(targetPath, storage.Path)
 			}
 		}
+	}
 
-	case string(MinioStorageType):
-		if err := targetSec.MapTo(&storage.MinioConfig); err != nil {
-			return nil, fmt.Errorf("map minio config failed: %v", err)
-		}
+	return &storage, nil
+}
 
-		storage.MinioConfig.BasePath = name + "/"
+func getStorageForMinio(targetSec, overrideSec ConfigSection, tp targetSecType, name string) (*Storage, error) {
+	var storage Storage
+	storage.Type = StorageType(targetSec.Key("STORAGE_TYPE").String())
+	if err := targetSec.MapTo(&storage.MinioConfig); err != nil {
+		return nil, fmt.Errorf("map minio config failed: %v", err)
+	}
 
-		if extraConfigSec != nil {
-			storage.MinioConfig.ServeDirect = ConfigSectionKeyBool(extraConfigSec, "SERVE_DIRECT", storage.MinioConfig.ServeDirect)
-			storage.MinioConfig.BasePath = ConfigSectionKeyString(extraConfigSec, "MINIO_BASE_PATH", storage.MinioConfig.BasePath)
-			storage.MinioConfig.Bucket = ConfigSectionKeyString(extraConfigSec, "MINIO_BUCKET", storage.MinioConfig.Bucket)
-		}
+	if storage.MinioConfig.BasePath == "" {
+		storage.MinioConfig.BasePath = name + "/"
 	}
 
+	if overrideSec != nil {
+		storage.MinioConfig.ServeDirect = ConfigSectionKeyBool(overrideSec, "SERVE_DIRECT", storage.MinioConfig.ServeDirect)
+		storage.MinioConfig.BasePath = ConfigSectionKeyString(overrideSec, "MINIO_BASE_PATH", storage.MinioConfig.BasePath)
+		storage.MinioConfig.Bucket = ConfigSectionKeyString(overrideSec, "MINIO_BUCKET", storage.MinioConfig.Bucket)
+	}
 	return &storage, nil
 }