Clean up various use of escape/unescape functions for URL generation #6334
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| // Copyright 2019 The Gitea Authors. All rights reserved. | ||
| // Use of this source code is governed by a MIT-style | ||
| // license that can be found in the LICENSE file. | ||
|
|
||
| package util | ||
|
There was a problem hiding this comment. A package named There was a problem hiding this comment. util does feel right for something like this but I assume the advice is to not re-use common stdlib names in general? What namespace would be preferred here? I'm not very good at naming things, perhaps just a generic 'gitea' for these utility type functions that are local to this project Not very clever but at least obvious to where it comes from. Lemme know what makes sense and I can update this PR. If the name needs more thought/feedback, I think it would be better to add this PR 'as is' and create a second PR for changing the name of the currently used modules/util since this will fix a few existing bugs. There was a problem hiding this comment. @lunny I think it's reasonable to continue using the util module here. We should refactor the module later as a separate pr. There was a problem hiding this comment. I think we could create a new package named |
||
|
|
||
| import ( | ||
| "net/url" | ||
| "path" | ||
| "strings" | ||
|
|
||
| "code.gitea.io/gitea/modules/log" | ||
| "code.gitea.io/gitea/modules/setting" | ||
| ) | ||
|
|
||
| // PathEscapeSegments escapes segments of a path while not escaping forward slash | ||
| func PathEscapeSegments(path string) string { | ||
| slice := strings.Split(path, "/") | ||
| for index := range slice { | ||
| slice[index] = url.PathEscape(slice[index]) | ||
| } | ||
| escapedPath := strings.Join(slice, "/") | ||
| return escapedPath | ||
| } | ||
|
|
||
| // URLJoin joins url components, like path.Join, but preserving contents | ||
| func URLJoin(base string, elems ...string) string { | ||
| if !strings.HasSuffix(base, "/") { | ||
| base += "/" | ||
| } | ||
| baseURL, err := url.Parse(base) | ||
| if err != nil { | ||
| log.Error(4, "URLJoin: Invalid base URL %s", base) | ||
| return "" | ||
| } | ||
| joinedPath := path.Join(elems...) | ||
| argURL, err := url.Parse(joinedPath) | ||
| if err != nil { | ||
| log.Error(4, "URLJoin: Invalid arg %s", joinedPath) | ||
| return "" | ||
| } | ||
| joinedURL := baseURL.ResolveReference(argURL).String() | ||
| if !baseURL.IsAbs() && !strings.HasPrefix(base, "/") { | ||
| return joinedURL[1:] // Removing leading '/' if needed | ||
| } | ||
| return joinedURL | ||
| } | ||
|
|
||
| // IsExternalURL checks if rawURL points to an external URL like http://example.com | ||
| func IsExternalURL(rawURL string) bool { | ||
| parsed, err := url.Parse(rawURL) | ||
| if err != nil { | ||
| return true | ||
| } | ||
| if len(parsed.Host) != 0 && strings.Replace(parsed.Host, "www.", "", 1) != strings.Replace(setting.Domain, "www.", "", 1) { | ||
| return true | ||
| } | ||
| return false | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Just check that this doesn't break. I put the PathEscape in recently and it worked for branches with subpaths.
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.
Yes, this still works the same and as a bonus makes the logging a tiny bit more clear (though both should work fine here). With just PathEscape, you end up with a log like:
And with PathEscapeSegments you end up with:
Since we tolerate the forward slashes in a URL anyway the second choice seems preferable for clarity and consistency with how we escape the branchName in other places now.