-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use CleanPath instead of path.Clean #23371
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I just confirmed that you refactored every instance of it.
* 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
I was unable to create a backport for 1.19, please send one manually. 🍵 |
@@ -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, "\\", "/")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong and is going to result in it being possible to have rPath have a preceding "/" whereas previously it was impossible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If rPath has a /
prefix, it will be invoked with path.Clean
directly. I think it should be better than the current logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
util.CleanPath will result in rPath still having the / whereas previous it would always be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if util.CleanPath
here will not convert absolute path to relative path, but line 47 will ensure rPath is a relative path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that's not CleanPath
's problem. I sent #23446 to fix possible unclear places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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. |
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can result in a lockPath with a preceding "/". This is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and everywhere else too
@@ -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, "\\", "/")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
util.CleanPath will result in rPath still having the / whereas previous it would always be removed.
if strings.HasPrefix(p, "/") { | ||
return path.Clean(p) | ||
} | ||
return path.Clean("/" + p)[1:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
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
As title.