Skip to content
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.

Sign up for GitHub

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

escape filename when assemble URL #22850

Merged
merged 7 commits into from
Feb 12, 2023
Merged

escape filename when assemble URL #22850

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
92a7602
escape filename when assemble URL
sillyguodong Feb 10, 2023
1b766ad
use util.PathEscapeSegments
sillyguodong Feb 10, 2023
ec07809
More escaping in content.go
zeripath Feb 11, 2023
5734c4c
fix escaping in endpoint.go
zeripath Feb 11, 2023
b03c2b9
Merge remote-tracking branch 'origin/main' into bugfix/issue_22843
zeripath Feb 11, 2023
0cc708d
Merge branch 'main' into bugfix/issue_22843
zeripath Feb 11, 2023
f035728
Merge branch 'main' into bugfix/issue_22843
zeripath Feb 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions services/repository/files/content.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/setting"
api "code.gitea.io/gitea/modules/structs"
"code.gitea.io/gitea/modules/util"
)

// ContentType repo content type
Expand Down Expand Up @@ -158,7 +159,7 @@ func GetContents(ctx context.Context, repo *repo_model.Repository, treePath, ref
return nil, fmt.Errorf("no commit found for the ref [ref: %s]", ref)
}

selfURL, err := url.Parse(fmt.Sprintf("%s/contents/%s?ref=%s", repo.APIURL(), treePath, origRef))
selfURL, err := url.Parse(fmt.Sprintf("%s/contents/%s?ref=%s", repo.APIURL(), util.PathEscapeSegments(treePath), origRef))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

origRef needs to be url.QueryEscape()

Suggested change
selfURL, err := url.Parse(fmt.Sprintf("%s/contents/%s?ref=%s", repo.APIURL(), util.PathEscapeSegments(treePath), origRef))
selfURL, err := url.Parse(fmt.Sprintf("%s/contents/%s?ref=%s", repo.APIURL(), util.PathEscapeSegments(treePath), url.QueryEscape(origRef)))

if err != nil {
return nil, err
}
Expand Down Expand Up @@ -217,15 +218,15 @@ func GetContents(ctx context.Context, repo *repo_model.Repository, treePath, ref
}
// Handle links
if entry.IsRegular() || entry.IsLink() {
downloadURL, err := url.Parse(fmt.Sprintf("%s/raw/%s/%s/%s", repo.HTMLURL(), refType, ref, treePath))
downloadURL, err := url.Parse(fmt.Sprintf("%s/raw/%s/%s/%s", repo.HTMLURL(), refType, ref, util.PathEscapeSegments(treePath)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
downloadURL, err := url.Parse(fmt.Sprintf("%s/raw/%s/%s/%s", repo.HTMLURL(), refType, ref, util.PathEscapeSegments(treePath)))
downloadURL, err := url.Parse(fmt.Sprintf("%s/raw/%s/%s/%s", repo.HTMLURL(), url.PathEscape(refType), util.PathEscapeSegments(ref), util.PathEscapeSegments(treePath)))

Copy link
Contributor

@wxiaoguang wxiaoguang Feb 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work if ref contains / ? eg: ref="a/b", treePath="c/d", then there will be "/a/b/c/d", which is the ref part. Or, if actually the endpoint doesn't care about it, then there would be no problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ref should be escape path segments.

The code does the magic of handling the ref

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yup, that's getRefNameFromPath. By design if ref /a exists, there should be no ref /a/b, so it's safe.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the ref name should always be URL safe but... It's really much better to just escape the thing

if err != nil {
return nil, err
}
downloadURLString := downloadURL.String()
contentsResponse.DownloadURL = &downloadURLString
}
if !entry.IsSubModule() {
htmlURL, err := url.Parse(fmt.Sprintf("%s/src/%s/%s/%s", repo.HTMLURL(), refType, ref, treePath))
htmlURL, err := url.Parse(fmt.Sprintf("%s/src/%s/%s/%s", repo.HTMLURL(), refType, ref, util.PathEscapeSegments(treePath)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
htmlURL, err := url.Parse(fmt.Sprintf("%s/src/%s/%s/%s", repo.HTMLURL(), refType, ref, util.PathEscapeSegments(treePath)))
htmlURL, err := url.Parse(fmt.Sprintf("%s/src/%s/%s/%s", repo.HTMLURL(), url.PathEscape(refType), util.PathEscapeSegments(ref), util.PathEscapeSegments(treePath)))

if err != nil {
return nil, err
}
Expand Down