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.

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

Always return valid go-get meta, even if unauthorized #2010

Merged
merged 2 commits into from
Jun 26, 2017

Conversation

bkcsoft
Copy link
Member

@bkcsoft bkcsoft commented Jun 19, 2017

(Not leaking information, returns valid metadata for ALL requests... even broken ones...)

This is required because go get doesn't know about .netrc

@bkcsoft bkcsoft added the type/enhancement An improvement of existing functionality label Jun 19, 2017
@tboerger
Copy link
Member

LGTM

@tboerger tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jun 19, 2017
branchName := "master"

owner, err := models.GetUserByName(ownerName)
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

It will leak users/organizations, it should skip searching for repository by name if there is error and never return 500 or any other error

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems to be fetching the default branch-name, so if err { branchName = "master" } ?

Copy link
Member

Choose a reason for hiding this comment

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

I would say something like that:

			owner, err := models.GetUserByName(ownerName)
			if err == nil {
				repo, err := models.GetRepositoryByName(owner.ID, repoName)
				if err == nil && len(repo.DefaultBranch) > 0 {
					branchName = repo.DefaultBranch
				}
			}

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just return meta tag with no GetUserByName and GetRepositoryByName.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done :)

@lunny
Copy link
Member

lunny commented Jun 21, 2017

Just return even if repo is not exist.

@lunny lunny added this to the 1.2.0 milestone Jun 23, 2017
branchName := "master"

owner, err := models.GetUserByName(ownerName)
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just return meta tag with no GetUserByName and GetRepositoryByName.

@bkcsoft
Copy link
Member Author

bkcsoft commented Jun 25, 2017

Now it should be done :) @lunny

@lafriks
Copy link
Member

lafriks commented Jun 25, 2017

LGTM

@tboerger tboerger 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 Jun 25, 2017
@lunny lunny merged commit 5db8cf3 into go-gitea:master Jun 26, 2017
@bkcsoft
Copy link
Member Author

bkcsoft commented Jun 26, 2017

@lunny that would break default branches 🙄

@lunny
Copy link
Member

lunny commented Jun 26, 2017

Where?

@bkcsoft
Copy link
Member Author

bkcsoft commented Jun 27, 2017

@lunny

Maybe just return meta tag with no GetUserByName and GetRepositoryByName.

☝️ that 😄

@bkcsoft bkcsoft deleted the go-get-meta-fix branch June 27, 2017 18:48
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants