Do not allow different storage configurations to point to the same directory #30169

wxiaoguang · 2024-03-29T07:41:34Z

wxiaoguang · 2024-03-29T07:42:50Z

Compare by ignoring spaces: https://github.com/go-gitea/gitea/pull/30169/files?diff=split&w=1

delvh · 2024-03-29T15:39:51Z

modules/setting/indexer.go

@@ -58,7 +58,7 @@ func loadIndexerFrom(rootCfg ConfigProvider) {
 		if !filepath.IsAbs(Indexer.IssuePath) {
 			Indexer.IssuePath = filepath.ToSlash(filepath.Join(AppWorkPath, Indexer.IssuePath))
 		}
-		fatalDuplicatedPath("issue_indexer", Indexer.IssuePath)
+		checkOverlappedPath("indexer.ISSUE_INDEXER_PATH", Indexer.IssuePath)


I think it's better if we notify the users explicitly what the section is:

Suggested change

checkOverlappedPath("indexer.ISSUE_INDEXER_PATH", Indexer.IssuePath)

checkOverlappedPath("[indexer].ISSUE_INDEXER_PATH", Indexer.IssuePath)

delvh · 2024-03-29T15:40:20Z

modules/setting/repository.go

@@ -286,7 +286,7 @@ func loadRepositoryFrom(rootCfg ConfigProvider) {
 		RepoRootPath = filepath.Clean(RepoRootPath)
 	}

-	fatalDuplicatedPath("repository.ROOT", RepoRootPath)
+	checkOverlappedPath("repository.ROOT", RepoRootPath)


Suggested change

checkOverlappedPath("repository.ROOT", RepoRootPath)

checkOverlappedPath("[repository].ROOT", RepoRootPath)

delvh · 2024-03-29T15:41:21Z

modules/setting/storage.go

@@ -240,7 +240,7 @@ func getStorageForLocal(targetSec, overrideSec ConfigSection, tp targetSecType,
 		}
 	}

-	fatalDuplicatedPath("storage."+name, storage.Path)
+	checkOverlappedPath("storage."+name+".PATH", storage.Path)


Suggested change

checkOverlappedPath("storage."+name+".PATH", storage.Path)

checkOverlappedPath("[storage."+name+"].PATH", storage.Path)

It is not right by your approach. [storage.xxx] is the section name. [storage] is not.

Yup, fixed my comment

delvh · 2024-03-29T15:41:58Z

modules/setting/session.go

@@ -46,7 +46,7 @@ func loadSessionFrom(rootCfg ConfigProvider) {
 	SessionConfig.ProviderConfig = strings.Trim(sec.Key("PROVIDER_CONFIG").MustString(filepath.Join(AppDataPath, "sessions")), "\" ")
 	if SessionConfig.Provider == "file" && !filepath.IsAbs(SessionConfig.ProviderConfig) {
 		SessionConfig.ProviderConfig = filepath.Join(AppWorkPath, SessionConfig.ProviderConfig)
-		fatalDuplicatedPath("session", SessionConfig.ProviderConfig)
+		checkOverlappedPath("session.PROVIDER_CONFIG", SessionConfig.ProviderConfig)


Suggested change

checkOverlappedPath("session.PROVIDER_CONFIG", SessionConfig.ProviderConfig)

checkOverlappedPath("[session].PROVIDER_CONFIG", SessionConfig.ProviderConfig)

delvh · 2024-03-29T16:11:48Z

modules/setting/setting.go

+	// TODO: some paths shouldn't overlap (storage.xxx.path), while some could (data path is the base path for storage path)
+	if targetName, ok := configuredPaths[path]; ok && targetName != name {
+		msg := fmt.Sprintf("Configured path %q is used by %q and %q at the same time. The paths must be unique to prevent data loss.", path, targetName, name)
+		log.Error("%s", msg)
+		DeprecatedWarnings = append(DeprecatedWarnings, msg)
 	}


Can't we fix this completely using

Suggested change

// TODO: some paths shouldn't overlap (storage.xxx.path), while some could (data path is the base path for storage path)

if targetName, ok := configuredPaths[path]; ok && targetName != name {

msg := fmt.Sprintf("Configured path %q is used by %q and %q at the same time. The paths must be unique to prevent data loss.", path, targetName, name)

log.Error("%s", msg)

DeprecatedWarnings = append(DeprecatedWarnings, msg)

}

path = gopath.Clean(path)

for existingPath, existingName := range configuredPaths {

if strings.HasPrefix(path, existingPath) || strings.HasPrefix(existingPath, path) {

var msg string

if path == existingPath {

msg = fmt.Sprintf("Path %q is used by %q and %q simultaneously. This may lead to data loss. To prevent this, the paths must be different and they must not overlap.", path, existingName, name)

} else {

msg = fmt.Sprintf("Path %q (used by %q) is contained within %q (used by %q). This may lead to data loss. To prevent this, the paths may not overlap.", existingPath, existingName, path, name)

}

log.Error("%s", msg)

DeprecatedWarnings = append(DeprecatedWarnings, msg)

}

(assuming we have an import gopath path above)
?

It is not right (incomplete). /a/b overlaps /a, meanwhile /a overlaps /a/b

If you would like to "complete" it, it also needs enough tests.

Yup, fixed my comment.

But your "msg" is still not right, and it doesn't have tests.

As I said: "I won't do any further change". So feel free to take it, or improve it, or discard it.

And one more thing, path = gopath.Clean(path) is also not right. It should use filepath.

delvh · 2024-03-29T16:14:43Z

modules/setting/server.go


 	EnableGzip = sec.Key("ENABLE_GZIP").MustBool()
 	EnablePprof = sec.Key("ENABLE_PPROF").MustBool(false)
 	PprofDataPath = sec.Key("PPROF_DATA_PATH").MustString(filepath.Join(AppWorkPath, "data/tmp/pprof"))
 	if !filepath.IsAbs(PprofDataPath) {
 		PprofDataPath = filepath.Join(AppWorkPath, PprofDataPath)
 	}
-	fatalDuplicatedPath("pprof_data_path", PprofDataPath)
+	checkOverlappedPath("server.PPROF_DATA_PATH", PprofDataPath)


Suggested change

checkOverlappedPath("server.PPROF_DATA_PATH", PprofDataPath)

checkOverlappedPath("[server].PPROF_DATA_PATH", PprofDataPath)

routers/web/admin/admin.go

wxiaoguang · 2024-03-30T16:02:04Z

This is one more time that I have to fix other's bugs because nobody listened to my suggestions.

So I have no patience or motivation to make any more change. Feel free to do what people like, take it, or improve it, or discard it.

delvh · 2024-03-30T18:09:30Z

@wxiaoguang If I'm not mistaken, my proposed change will fix the problem completely.
What do you think about it?
That way, we don't need to worry about this problem anymore.

wxiaoguang · 2024-03-30T18:21:44Z

@wxiaoguang If I'm not mistaken, my proposed change will fix the problem completely. What do you think about it? That way, we don't need to worry about this problem anymore.

OK, I managed to reply the suggestions as much as possible, there are more problems than this PR. As I said, I do not have motivation to do any further change because the old approach just doesn't look good to me and this PR only helps to avoid some serious buggy behaviors.

delvh

Then let's get this PR merged now, and I'll see that I improve it afterwards.
The current state is better than nothing although not perfect.

…rectory (go-gitea#30169) Replace go-gitea#29171

…rectory (#30169) (#30204) Backport #30169 by wxiaoguang Replace #29171 Co-authored-by: wxiaoguang <[email protected]>

* giteaofficial/main: Do not allow different storage configurations to point to the same directory (go-gitea#30169) Fix GPG subkey verify (go-gitea#30193) [skip ci] Updated translations via Crowdin Fix unclickable checkboxes (go-gitea#30195) Remove jQuery class from the issue author dropdown (go-gitea#30188) Remove jQuery class from the comment edit history (go-gitea#30186) Remove jQuery class from the repository branch settings (go-gitea#30184) [skip ci] Updated translations via Crowdin Use Crowdin action for translation sync (go-gitea#30054) Remove jQuery class from the project page (go-gitea#30183) Remove jQuery class from the comment context menu (go-gitea#30179)

wxiaoguang added the backport/v1.22 This PR should be backported to Gitea 1.22 label Mar 29, 2024

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 29, 2024

pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 29, 2024

github-actions bot added modifies/translation modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files labels Mar 29, 2024

wxiaoguang mentioned this pull request Mar 29, 2024

Revert "Disallow duplicate storage paths (#26484)" #29171

Closed

fix

ce64832

wxiaoguang force-pushed the fix-path-check branch from 8dc3257 to ce64832 Compare March 29, 2024 07:47

wxiaoguang added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Mar 29, 2024

wxiaoguang added this to the 1.23.0 milestone Mar 29, 2024

lunny approved these changes Mar 29, 2024

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 29, 2024

delvh reviewed Mar 29, 2024

View reviewed changes

delvh approved these changes Mar 30, 2024

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 30, 2024

delvh added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 30, 2024

Merge branch 'main' into fix-path-check

bbd966d

delvh changed the title ~~Refactor startup path check~~ Do not allow different storage configurations to point to the same directory Mar 30, 2024

GiteaBot added 5 commits March 31, 2024 06:09

Merge branch 'main' into fix-path-check

4729dac

Merge branch 'main' into fix-path-check

90322c5

Merge branch 'main' into fix-path-check

0975f2c

Merge branch 'main' into fix-path-check

00a656c

Merge branch 'main' into fix-path-check

3e5e2f5

wxiaoguang enabled auto-merge (squash) March 31, 2024 02:40

wxiaoguang merged commit 6d34ce2 into go-gitea:main Mar 31, 2024
26 checks passed

GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Mar 31, 2024

Do not allow different storage configurations to point to the same di…

af93c23

…rectory (go-gitea#30169) Replace go-gitea#29171

GiteaBot mentioned this pull request Mar 31, 2024

Do not allow different storage configurations to point to the same directory (#30169) #30204

Merged

GiteaBot added backport/done All backports for this PR have been created and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Mar 31, 2024

wxiaoguang added a commit that referenced this pull request Mar 31, 2024

Do not allow different storage configurations to point to the same di…

cd11786

…rectory (#30169) (#30204) Backport #30169 by wxiaoguang Replace #29171 Co-authored-by: wxiaoguang <[email protected]>

wxiaoguang deleted the fix-path-check branch March 31, 2024 10:25

go-gitea locked as resolved and limited conversation to collaborators Jun 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not allow different storage configurations to point to the same directory #30169

Do not allow different storage configurations to point to the same directory #30169

wxiaoguang commented Mar 29, 2024

wxiaoguang commented Mar 29, 2024 •

edited

Loading

delvh Mar 29, 2024

delvh Mar 29, 2024

delvh Mar 29, 2024 •

edited

Loading

wxiaoguang Mar 30, 2024

delvh Mar 30, 2024

delvh Mar 29, 2024

delvh Mar 29, 2024 •

edited

Loading

wxiaoguang Mar 30, 2024 •

edited

Loading

delvh Mar 30, 2024

wxiaoguang Mar 30, 2024

wxiaoguang Mar 30, 2024

delvh Mar 29, 2024

wxiaoguang commented Mar 30, 2024

delvh commented Mar 30, 2024

wxiaoguang commented Mar 30, 2024 •

edited

Loading

delvh left a comment

	checkOverlappedPath("indexer.ISSUE_INDEXER_PATH", Indexer.IssuePath)
	checkOverlappedPath("[indexer].ISSUE_INDEXER_PATH", Indexer.IssuePath)

	checkOverlappedPath("repository.ROOT", RepoRootPath)
	checkOverlappedPath("[repository].ROOT", RepoRootPath)

	checkOverlappedPath("storage."+name+".PATH", storage.Path)
	checkOverlappedPath("[storage."+name+"].PATH", storage.Path)

	checkOverlappedPath("session.PROVIDER_CONFIG", SessionConfig.ProviderConfig)
	checkOverlappedPath("[session].PROVIDER_CONFIG", SessionConfig.ProviderConfig)

-	// TODO: some paths shouldn't overlap (storage.xxx.path), while some could (data path is the base path for storage path)
-	if targetName, ok := configuredPaths[path]; ok && targetName != name {
-		msg := fmt.Sprintf("Configured path %q is used by %q and %q at the same time. The paths must be unique to prevent data loss.", path, targetName, name)
-		log.Error("%s", msg)
-		DeprecatedWarnings = append(DeprecatedWarnings, msg)
-	}
+	path = gopath.Clean(path)
+	for existingPath, existingName := range configuredPaths {
+		if strings.HasPrefix(path, existingPath) || strings.HasPrefix(existingPath, path) {
+			var msg string
+			if path == existingPath {
+				msg = fmt.Sprintf("Path %q is used by %q and %q simultaneously. This may lead to data loss. To prevent this, the paths must be different and they must not overlap.", path, existingName, name)
+			} else {
+				msg = fmt.Sprintf("Path %q (used by %q) is contained within %q (used by %q). This may lead to data loss. To prevent this, the paths may not overlap.", existingPath, existingName, path, name)
+			}
+		log.Error("%s", msg)
+		DeprecatedWarnings = append(DeprecatedWarnings, msg)
+	}

	checkOverlappedPath("server.PPROF_DATA_PATH", PprofDataPath)
	checkOverlappedPath("[server].PPROF_DATA_PATH", PprofDataPath)

Do not allow different storage configurations to point to the same directory #30169

Do not allow different storage configurations to point to the same directory #30169

Conversation

wxiaoguang commented Mar 29, 2024

wxiaoguang commented Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delvh Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delvh Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

wxiaoguang Mar 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wxiaoguang commented Mar 30, 2024

delvh commented Mar 30, 2024

wxiaoguang commented Mar 30, 2024 • edited Loading

delvh left a comment

Choose a reason for hiding this comment

wxiaoguang commented Mar 29, 2024 •

edited

Loading

delvh Mar 29, 2024 •

edited

Loading

delvh Mar 29, 2024 •

edited

Loading

wxiaoguang Mar 30, 2024 •

edited

Loading

wxiaoguang commented Mar 30, 2024 •

edited

Loading