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

Escape commit ref with percent encoding #1099

Merged
merged 2 commits into from
Jan 19, 2019
Merged

Conversation

bgpat
Copy link
Contributor

@bgpat bgpat commented Jan 16, 2019

If Repositories.GetCommitSHA1 was called with the branch name which includes non-alphabet character, it occurs an error. It looks to require escaping with percent encoding.

Below is sample code:

package main

import (
	"context"
	"fmt"

	"github.com/google/go-github/github"
)

func main() {
	client := github.NewClient(nil)
	sha1, _, err := client.Repositories.GetCommitSHA1(ctx, "bgpat", "test", "cpu100%", "")
	if err != nil {
		panic(err)
	}
	fmt.Println(sha1)
}

before: parse repos/bgpat/test/commits/cpu100%: invalid URL escape "%"
after: 46736bd30fe65160f7d21e49d8f23e4687abc41b

@googlebot googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Jan 16, 2019
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @bgpat!
LGTM.
Awaiting second LGTM before merging.

@gmlewis
Copy link
Collaborator

gmlewis commented Jan 16, 2019

I'm curious if there are other APIs in our repo where we have made the same mistake?

I'm betting that anywhere we pass a ref in the URL, we would need to escape it.

While you are here, @bgpat, do you want to take a look?

@bgpat
Copy link
Contributor Author

bgpat commented Jan 17, 2019

I found 9 same mistakes. I will fix it.

$ grep -n Sprintf *.go | grep ', ref\W'
checks.go:258:	u := fmt.Sprintf("repos/%v/%v/commits/%v/check-runs", owner, repo, ref)
checks.go:324:	u := fmt.Sprintf("repos/%v/%v/commits/%v/check-suites", owner, repo, ref)
git_refs.go:60:	u := fmt.Sprintf("repos/%v/%v/git/refs/%v", owner, repo, ref)
git_refs.go:91:	u := fmt.Sprintf("repos/%v/%v/git/refs/%v", owner, repo, ref)
git_refs.go:211:	u := fmt.Sprintf("repos/%v/%v/git/refs/%v", owner, repo, ref)
repos_commits.go:192:	u := fmt.Sprintf("repos/%v/%v/commits/%v", owner, repo, ref)
repos_statuses.go:47:	u := fmt.Sprintf("repos/%v/%v/commits/%v/statuses", owner, repo, ref)
repos_statuses.go:72:	u := fmt.Sprintf("repos/%v/%v/statuses/%v", owner, repo, ref)
repos_statuses.go:111:	u := fmt.Sprintf("repos/%v/%v/commits/%v/status", owner, repo, ref)

@bgpat
Copy link
Contributor Author

bgpat commented Jan 17, 2019

@gmlewis I escaped all ref. Please review.

@gmlewis
Copy link
Collaborator

gmlewis commented Jan 17, 2019

OK, this is true awesomeness... thank you so much, @bgpat!
LGTM.

Awaiting input from @gauntface as a sanity check to make sure my review makes sense... then we will get this in.

Copy link
Contributor

@gauntface gauntface left a comment

Choose a reason for hiding this comment

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

This looks great to me. We should look at adding some tests to make sure we don't regress on this in the future, but that can be done separately.

Thank you for fixing this :)

@gmlewis
Copy link
Collaborator

gmlewis commented Jan 19, 2019

Excellent point, @gauntface!
I've added issue #1101 to track this.
Merging.

@gmlewis gmlewis merged commit 56cb1dd into google:master Jan 19, 2019
@gmlewis
Copy link
Collaborator

gmlewis commented Jan 19, 2019

This is a bug fix. Bumping package version number to capture it.

vetinari added a commit to vetinari/go-github that referenced this pull request Feb 18, 2020
as given in the original google#1099
n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indication that the PR author has signed a Google Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants