Use CleanPath instead of path.Clean #23371

lunny · 2023-03-08T07:21:39Z

As title.

delvh

LGTM. I just confirmed that you refactored every instance of it.

modules/options/repo.go

* giteaofficial/main: Test renderReadmeFile (go-gitea#23185) [skip ci] Updated translations via Crowdin Set `X-Gitea-Debug` header once (go-gitea#23361) Improve cache context (go-gitea#23330) add user visibility in dashboard navbar (go-gitea#22747) Fix panic when getting notes by ref (go-gitea#23372) Use CleanPath instead of path.Clean (go-gitea#23371) Reduce duplicate and useless code in options (go-gitea#23369) Clean Path in Options (go-gitea#23006) Do not recognize text files as audio (go-gitea#23355) Fix incorrect display for comment context menu (go-gitea#23343) # Conflicts: # templates/repo/issue/view_content/context_menu.tmpl

GiteaBot · 2023-03-09T22:55:48Z

I was unable to create a backport for 1.19, please send one manually. 🍵

zeripath · 2023-03-13T11:47:45Z

routers/web/base.go

@@ -44,7 +45,7 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
 				routing.UpdateFuncInfo(req.Context(), funcInfo)

 				rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/")
-				rPath = path.Clean("/" + strings.ReplaceAll(rPath, "\\", "/"))[1:]
+				rPath = util.CleanPath(strings.ReplaceAll(rPath, "\\", "/"))


This is wrong and is going to result in it being possible to have rPath have a preceding "/" whereas previously it was impossible.

If rPath has a / prefix, it will be invoked with path.Clean directly. I think it should be better than the current logic.

util.CleanPath will result in rPath still having the / whereas previous it would always be removed.

Even if util.CleanPath here will not convert absolute path to relative path, but line 47 will ensure rPath is a relative path.

I think we need clear definitions for every case, instead of using unpredictable CleanPath behavior.

The unstable part is :

CleanPath("/path") => /path

CleanPath("path") => path

But in many cases, we the caller only wants path.

But that's not CleanPath's problem. I sent #23446 to fix possible unclear places

I think the problem is that CleanPath's behavior is not that stable in many cases.

Callers should always know what they need - absolute or relative - no matter what path has been passed in the util function.

absolute or relative should not be the responsibility of CleanPath. It should be decided out of the function. Or we can rename CleanPath to CleanAndEnsureRelativePath.

Hmm .. that's a disagreement: I do not think so about "absolute or relative should not be the responsibility of CleanPath. It should be decided out of the function."

IMO maybe it could be (like #23441)

rel := util.PathRelJoin(....) // `/path` => `path`, `path` => `path` abs := path.Join("/", util.PathRelJoin(....)) // if necessary

zeripath · 2023-03-13T11:50:05Z

There are many issues with this PR. In many places the previous code absolutely ensured that a path could not start with a "/" which is this PR will no longer protect against.

zeripath · 2023-03-13T11:48:53Z

routers/web/repo/lfs.go

@@ -207,7 +207,7 @@ func LFSLockFile(ctx *context.Context) {
 		ctx.Redirect(ctx.Repo.RepoLink + "/settings/lfs/locks")
 		return
 	}
-	lockPath = path.Clean("/" + lockPath)[1:]
+	lockPath = util.CleanPath(lockPath)


This can result in a lockPath with a preceding "/". This is wrong.

and everywhere else too

zeripath · 2023-03-13T12:08:09Z

routers/web/base.go

@@ -44,7 +45,7 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
 				routing.UpdateFuncInfo(req.Context(), funcInfo)

 				rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/")
-				rPath = path.Clean("/" + strings.ReplaceAll(rPath, "\\", "/"))[1:]
+				rPath = util.CleanPath(strings.ReplaceAll(rPath, "\\", "/"))


util.CleanPath will result in rPath still having the / whereas previous it would always be removed.

delvh · 2023-03-13T12:57:08Z

modules/util/path.go

+	if strings.HasPrefix(p, "/") {
+		return path.Clean(p)
+	}
+	return path.Clean("/" + p)[1:]


I just read through the documentation of the method, this whole block is unnecessary and can be replaced with return path.Clean("/" + p)[1:].
That way, we ensure that the path is cleaned and that it is a relative path.
If we always want relative paths, we can just use that instead.
So the question is rather: Is there any use case where we need to clean an absolute path?

I just read through the documentation of the method, this whole block is unnecessary and can be replaced with return path.Clean("/" + p)[1:]. That way, we ensure that the path is cleaned and that it is a relative path. If we always want relative paths, we can just use that instead. So the question is rather: Is there any use case where we need to clean an absolute path?

I don't think so except we rename the function to CleanAndEnsureRelativePath.

This reverts commit b116418.

GiteaBot · 2023-03-16T10:09:53Z

I was unable to create a backport for 1.19, please send one manually. 🍵

Since #23493 has conflicts with latest commits, this PR is my proposal for fixing #23371 Details are in the comments And refactor the `modules/options` module, to make it always use "filepath" to access local files. Benefits: * No need to do `util.CleanPath(strings.ReplaceAll(p, "\\", "/"))), "/")` any more (not only one before) * The function behaviors are clearly defined

Use CleanPath instead of path.Clean

46aaa2e

lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Mar 8, 2023

delvh approved these changes Mar 8, 2023

View reviewed changes

modules/options/repo.go Outdated Show resolved Hide resolved

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 8, 2023

delvh added the outdated/backport/v1.19 This PR should be backported to Gitea 1.19 label Mar 8, 2023

merge main branch

c9371bc

wolfogre approved these changes Mar 8, 2023

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 8, 2023

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

lunny added this to the 1.20.0 milestone Mar 8, 2023

lunny merged commit b116418 into go-gitea:main Mar 8, 2023

lunny deleted the lunny/clean_path branch March 8, 2023 12:17

lunny removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 8, 2023

wolfogre mentioned this pull request Mar 9, 2023

add path prefix to ObjectStorage.Iterator #23332

Merged

GiteaBot added the backport/manual No power to the bots! Create your backport yourself! label Mar 9, 2023

This was referenced Mar 13, 2023

Support SafeJoinPath and SafeJoinFilepath #23441

Closed

Use CleanPath instead of path.Clean (#23371) #23442

Closed

wolfogre added backport/done All backports for this PR have been created and removed backport/manual No power to the bots! Create your backport yourself! labels Mar 13, 2023

zeripath reviewed Mar 13, 2023

View reviewed changes

delvh reviewed Mar 13, 2023

View reviewed changes

lunny mentioned this pull request Mar 13, 2023

Make sure lfs will only support relative path #23446

Closed

lunny added a commit that referenced this pull request Mar 15, 2023

Revert "Use CleanPath instead of path.Clean (#23371)"

d5c2743

This reverts commit b116418.

lunny mentioned this pull request Mar 15, 2023

Revert "Use CleanPath instead of path.Clean" #23493

Closed

wxiaoguang mentioned this pull request Mar 15, 2023

Introduce path Clean/Join helper functions #23495

Merged

wolfogre removed the backport/done All backports for this PR have been created label Mar 16, 2023

GiteaBot added the backport/manual No power to the bots! Create your backport yourself! label Mar 16, 2023

lunny removed this from the 1.20.0 milestone Mar 16, 2023

delvh removed outdated/backport/v1.19 This PR should be backported to Gitea 1.19 backport/manual No power to the bots! Create your backport yourself! labels Mar 17, 2023

delvh added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Apr 3, 2023

delvh added this to the 1.20.0 milestone Apr 3, 2023

go-gitea locked and limited conversation to collaborators May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use CleanPath instead of path.Clean #23371

Use CleanPath instead of path.Clean #23371

lunny commented Mar 8, 2023

delvh left a comment

GiteaBot commented Mar 9, 2023

zeripath Mar 13, 2023

lunny Mar 13, 2023

zeripath Mar 13, 2023

lunny Mar 13, 2023

wxiaoguang Mar 13, 2023 •

edited

lunny Mar 13, 2023

wxiaoguang Mar 13, 2023

lunny Mar 13, 2023

wxiaoguang Mar 13, 2023

zeripath commented Mar 13, 2023

zeripath Mar 13, 2023

zeripath Mar 13, 2023

zeripath Mar 13, 2023

delvh Mar 13, 2023

lunny Mar 13, 2023

GiteaBot commented Mar 16, 2023

Use CleanPath instead of path.Clean #23371

Use CleanPath instead of path.Clean #23371

Conversation

lunny commented Mar 8, 2023

delvh left a comment

Choose a reason for hiding this comment

GiteaBot commented Mar 9, 2023

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 Mar 13, 2023 • edited

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

zeripath commented Mar 13, 2023

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

GiteaBot commented Mar 16, 2023

wxiaoguang Mar 13, 2023 •

edited