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

Fix Go 1.13 private repository go get issue #8100

Merged
merged 3 commits into from
Sep 6, 2019
Merged

Fix Go 1.13 private repository go get issue #8100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c598f7d
Fix Go 1.13 invalid import path creation
rutgerbrf Sep 5, 2019
275c833
Apply suggested changes from #8100
rutgerbrf Sep 5, 2019
9270a74
Merge branch 'release/v1.9' into fix-go1.13-invalid-import-path
sapk Sep 6, 2019
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
15 changes: 14 additions & 1 deletion modules/context/context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,19 @@ func Contexter() macaron.Handler {
if ctx.Query("go-get") == "1" {
ownerName := c.Params(":username")
repoName := c.Params(":reponame")
trimmedRepoName := strings.TrimSuffix(repoName, ".git")

if ownerName == "" || trimmedRepoName == "" {
_, _ = c.Write([]byte(`<!doctype html>
Copy link
Member

Choose a reason for hiding this comment

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

if returned error, you should give a 500 I think.

Copy link
Member

Choose a reason for hiding this comment

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

Why not see the problem the other way and simply ignore go-get params instead of erroring ?

if ctx.Query("go-get") == "1" && ownerName != "" && repoName != "" {

}

Copy link
Member

Choose a reason for hiding this comment

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

This could be break down in two if obviously.

if ctx.Query("go-get") == "1" 
  ownerName := c.Params(":username")
  repoName := c.Params(":reponame")
  if ownerName != "" && repoName != "" {
	... Do go-get work
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

But else we still need return 404 but not continue the other work.

Copy link
Member

Choose a reason for hiding this comment

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

Once ctx.Query("go-get") == "1" , it should return anyway here.

Copy link
Author

@rutgerbrf rutgerbrf Sep 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think a 400 should be returned because in this case the client has not provided adequate information for the server to provide an import URL from the its request.

Copy link
Author

@rutgerbrf rutgerbrf Sep 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

gitea/routers/repo/http.go

Line 58 in 85202d4

reponame := strings.TrimSuffix(ctx.Params(":reponame"), ".git")
I suppose this should also be applied in this case.

Copy link
Author

Choose a reason for hiding this comment

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

Or perhaps in trimmedRepoName, such that this is not interfered with:

gitea/modules/context/context.go

Line 258 in bb609ca

prefix := setting.AppURL + path.Join(url.PathEscape(ownerName), url.PathEscape(repoName), "src", "branch", util.PathEscapeSegments(branchName))

Copy link
Member

@sapk sapk Sep 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Why we should return an error ? For exemple github doesn't trigger an error on an profile if you use go-get param. Futhermore triggering an error from a supervision point could be misleading. I will not block as it need a quick fix.

<html>
<body>
invalid import path
</body>
</html>
`))
c.WriteHeader(400)
return
}
branchName := "master"

repo, err := models.GetRepositoryByOwnerAndName(ownerName, repoName)
Expand Down Expand Up @@ -276,7 +289,7 @@ func Contexter() macaron.Handler {
</body>
</html>
`, map[string]string{
"GoGetImport": ComposeGoGetImport(ownerName, strings.TrimSuffix(repoName, ".git")),
"GoGetImport": ComposeGoGetImport(ownerName, trimmedRepoName),
"CloneLink": models.ComposeHTTPSCloneURL(ownerName, repoName),
"GoDocDirectory": prefix + "{/dir}",
"GoDocFile": prefix + "{/dir}/{file}#L{line}",
Expand Down
8 changes: 6 additions & 2 deletions modules/context/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,10 +201,14 @@ func ComposeGoGetImport(owner, repo string) string {
// .netrc file.
func EarlyResponseForGoGetMeta(ctx *Context) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here we can check previously to calling this method and maybe pass username and reponame as args since they are already extracted and cleaned from context before calling:

gitea/routers/repo/http.go

Line 58 in 85202d4

reponame := strings.TrimSuffix(ctx.Params(":reponame"), ".git")

Copy link
Author

Choose a reason for hiding this comment

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

Would it not be preferable to add this for username as well in case only .git is appended to the Gitea instance URL?

Copy link
Author

Choose a reason for hiding this comment

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

I guess not because it seems possible to have the suffix .git in a username or organization name.

username := ctx.Params(":username")
reponame := ctx.Params(":reponame")
reponame := strings.TrimSuffix(ctx.Params(":reponame"), ".git")
if username == "" || reponame == "" {
ctx.PlainText(400, []byte("invalid repository path"))
return
}
ctx.PlainText(200, []byte(com.Expand(`<meta name="go-import" content="{GoGetImport} git {CloneLink}">`,
map[string]string{
"GoGetImport": ComposeGoGetImport(username, strings.TrimSuffix(reponame, ".git")),
"GoGetImport": ComposeGoGetImport(username, reponame),
"CloneLink": models.ComposeHTTPSCloneURL(username, reponame),
})))
}
Expand Down