Fix Go 1.13 private repository go get issue by rutgerbrf · Pull Request #8100 · go-gitea/gitea

rutgerbrf · 2019-09-05T10:36:06Z

Signed-off-by: Rutger Broekhoff <rutger@viasalix.nl>

lunny · 2019-09-05T11:18:00Z

 			ownerName := c.Params(":username")
 			repoName := c.Params(":reponame")
+			if ownerName == "" || repoName == "" {
+				_, _ = c.Write([]byte(`<!doctype html>


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

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 != "" { }

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 } }

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

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

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.

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.

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))

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.

lunny · 2019-09-05T11:18:41Z

 	username := ctx.Params(":username")
 	reponame := ctx.Params(":reponame")
+	if username == "" || reponame == "" {
+		ctx.PlainText(404, []byte("invalid repository path"))


sapk · 2019-09-05T12:17:14Z

@@ -202,6 +202,9 @@ func ComposeGoGetImport(owner, repo string) string {
 func EarlyResponseForGoGetMeta(ctx *Context) {


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")

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

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

Signed-off-by: Rutger Broekhoff <rutger@viasalix.nl>

techknowlogick

This should be sent to master branch first. I'm going to mark this as blocked so we don't merge this prematurely.

lunny · 2019-09-06T03:46:20Z

@rutgerbrf please send a PR to master also.

Signed-off-by: Rutger Broekhoff <rutger@viasalix.nl>

* Fix Go 1.13 invalid import path creation Signed-off-by: Rutger Broekhoff <rutger@viasalix.nl> * Apply suggested changes from #8100 Signed-off-by: Rutger Broekhoff <rutger@viasalix.nl>

lunny · 2019-09-06T13:47:07Z

Since #8112 merged, I have removed the blocked label. @techknowlogick

lafriks · 2019-09-06T13:58:15Z

@techknowlogick it is been merged to master please approve

merged to master

Fix Go 1.13 invalid import path creation

c598f7d

Signed-off-by: Rutger Broekhoff <rutger@viasalix.nl>

lunny added the type/bug label Sep 5, 2019

lunny added this to the 1.10.0 milestone Sep 5, 2019

lunny added the backport/v1.9 label Sep 5, 2019

lunny requested changes Sep 5, 2019

View reviewed changes

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 5, 2019

sapk reviewed Sep 5, 2019

View reviewed changes

Apply suggested changes from #8100

275c833

Signed-off-by: Rutger Broekhoff <rutger@viasalix.nl>

lunny approved these changes Sep 5, 2019

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 5, 2019

lunny mentioned this pull request Sep 6, 2019

Go get from a git site will not read git config insteadOf any more on go1.13 but go1.12 golang/go#34102

Closed

guillep2k approved these changes Sep 6, 2019

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 Sep 6, 2019

Merge branch 'release/v1.9' into fix-go1.13-invalid-import-path

9270a74

techknowlogick previously requested changes Sep 6, 2019

View reviewed changes

techknowlogick added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Sep 6, 2019

lunny removed the backport/v1.9 label Sep 6, 2019

lunny modified the milestones: 1.10.0, 1.9.3 Sep 6, 2019

lunny pushed a commit to lunny/gitea that referenced this pull request Sep 6, 2019

Apply suggested changes from go-gitea#8100

cea91f6

Signed-off-by: Rutger Broekhoff <rutger@viasalix.nl>

lunny mentioned this pull request Sep 6, 2019

Fix Go 1.13 private repository go get issue #8112

Merged

lunny removed the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Sep 6, 2019

techknowlogick merged commit 89c5748 into go-gitea:release/v1.9 Sep 6, 2019

rutgerbrf deleted the fix-go1.13-invalid-import-path branch December 24, 2019 09:23

go-gitea locked and limited conversation to collaborators Nov 24, 2020

		@@ -202,6 +202,9 @@ func ComposeGoGetImport(owner, repo string) string {
		func EarlyResponseForGoGetMeta(ctx *Context) {

Uh oh!

Conversation

rutgerbrf commented Sep 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rutgerbrf Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rutgerbrf Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sapk Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

techknowlogick left a comment

Choose a reason for hiding this comment

Uh oh!

lunny commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lunny commented Sep 6, 2019

Uh oh!

lafriks commented Sep 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rutgerbrf Sep 5, 2019 •

edited

Loading

rutgerbrf Sep 5, 2019 •

edited

Loading

sapk Sep 5, 2019 •

edited

Loading

lunny commented Sep 6, 2019 •

edited

Loading