From 49891122ece421f3d81ab80cf9601590a330d7eb Mon Sep 17 00:00:00 2001 From: Hanno Hecker Date: Mon, 17 Feb 2020 15:01:00 +0100 Subject: [PATCH 1/4] do not encode / in ref names fixes #1432 Signed-off-by: Hanno Hecker --- github/checks.go | 5 ++--- github/git_refs.go | 14 +++++++++++--- github/repos_commits.go | 3 +-- github/repos_statuses.go | 7 +++---- 4 files changed, 17 insertions(+), 12 deletions(-) diff --git a/github/checks.go b/github/checks.go index 55adf14ea16..67b7e770217 100644 --- a/github/checks.go +++ b/github/checks.go @@ -8,7 +8,6 @@ package github import ( "context" "fmt" - "net/url" ) // ChecksService provides access to the Checks API in the @@ -258,7 +257,7 @@ type ListCheckRunsResults struct { // // GitHub API docs: https://developer.github.com/v3/checks/runs/#list-check-runs-for-a-specific-ref func (s *ChecksService) ListCheckRunsForRef(ctx context.Context, owner, repo, ref string, opts *ListCheckRunsOptions) (*ListCheckRunsResults, *Response, error) { - u := fmt.Sprintf("repos/%v/%v/commits/%v/check-runs", owner, repo, url.QueryEscape(ref)) + u := fmt.Sprintf("repos/%v/%v/commits/%v/check-runs", owner, repo, refURLEscape(ref)) u, err := addOptions(u, opts) if err != nil { return nil, nil, err @@ -324,7 +323,7 @@ type ListCheckSuiteResults struct { // // GitHub API docs: https://developer.github.com/v3/checks/suites/#list-check-suites-for-a-specific-ref func (s *ChecksService) ListCheckSuitesForRef(ctx context.Context, owner, repo, ref string, opts *ListCheckSuiteOptions) (*ListCheckSuiteResults, *Response, error) { - u := fmt.Sprintf("repos/%v/%v/commits/%v/check-suites", owner, repo, url.QueryEscape(ref)) + u := fmt.Sprintf("repos/%v/%v/commits/%v/check-suites", owner, repo, refURLEscape(ref)) u, err := addOptions(u, opts) if err != nil { return nil, nil, err diff --git a/github/git_refs.go b/github/git_refs.go index 18494dae33e..68b668be44f 100644 --- a/github/git_refs.go +++ b/github/git_refs.go @@ -58,7 +58,7 @@ type updateRefRequest struct { // GitHub API docs: https://developer.github.com/v3/git/refs/#get-a-reference func (s *GitService) GetRef(ctx context.Context, owner string, repo string, ref string) (*Reference, *Response, error) { ref = strings.TrimPrefix(ref, "refs/") - u := fmt.Sprintf("repos/%v/%v/git/refs/%v", owner, repo, url.QueryEscape(ref)) + u := fmt.Sprintf("repos/%v/%v/git/refs/%v", owner, repo, refURLEscape(ref)) req, err := s.client.NewRequest("GET", u, nil) if err != nil { return nil, nil, err @@ -79,6 +79,14 @@ func (s *GitService) GetRef(ctx context.Context, owner string, repo string, ref return r, resp, nil } +func refURLEscape(ref string) string { + parts := strings.Split(ref, "/") + for i, s := range parts { + parts[i] = url.QueryEscape(s) + } + return strings.Join(parts, "/") +} + // GetRefs fetches a slice of Reference objects for a given Git ref. // If there is an exact match, only that ref is returned. // If there is no exact match, GitHub returns all refs that start with ref. @@ -92,7 +100,7 @@ func (s *GitService) GetRef(ctx context.Context, owner string, repo string, ref // GitHub API docs: https://developer.github.com/v3/git/refs/#get-a-reference func (s *GitService) GetRefs(ctx context.Context, owner string, repo string, ref string) ([]*Reference, *Response, error) { ref = strings.TrimPrefix(ref, "refs/") - u := fmt.Sprintf("repos/%v/%v/git/refs/%v", owner, repo, url.QueryEscape(ref)) + u := fmt.Sprintf("repos/%v/%v/git/refs/%v", owner, repo, refURLEscape(ref)) req, err := s.client.NewRequest("GET", u, nil) if err != nil { return nil, nil, err @@ -212,7 +220,7 @@ func (s *GitService) UpdateRef(ctx context.Context, owner string, repo string, r // GitHub API docs: https://developer.github.com/v3/git/refs/#delete-a-reference func (s *GitService) DeleteRef(ctx context.Context, owner string, repo string, ref string) (*Response, error) { ref = strings.TrimPrefix(ref, "refs/") - u := fmt.Sprintf("repos/%v/%v/git/refs/%v", owner, repo, url.QueryEscape(ref)) + u := fmt.Sprintf("repos/%v/%v/git/refs/%v", owner, repo, refURLEscape(ref)) req, err := s.client.NewRequest("DELETE", u, nil) if err != nil { return nil, err diff --git a/github/repos_commits.go b/github/repos_commits.go index a4ff2be4b9d..d6960124d7a 100644 --- a/github/repos_commits.go +++ b/github/repos_commits.go @@ -9,7 +9,6 @@ import ( "bytes" "context" "fmt" - "net/url" "time" ) @@ -198,7 +197,7 @@ func (s *RepositoriesService) GetCommitRaw(ctx context.Context, owner string, re // // GitHub API docs: https://developer.github.com/v3/repos/commits/#get-the-sha-1-of-a-commit-reference func (s *RepositoriesService) GetCommitSHA1(ctx context.Context, owner, repo, ref, lastSHA string) (string, *Response, error) { - u := fmt.Sprintf("repos/%v/%v/commits/%v", owner, repo, url.QueryEscape(ref)) + u := fmt.Sprintf("repos/%v/%v/commits/%v", owner, repo, refURLEscape(ref)) req, err := s.client.NewRequest("GET", u, nil) if err != nil { diff --git a/github/repos_statuses.go b/github/repos_statuses.go index 5543265880c..5576d311255 100644 --- a/github/repos_statuses.go +++ b/github/repos_statuses.go @@ -8,7 +8,6 @@ package github import ( "context" "fmt" - "net/url" "time" ) @@ -46,7 +45,7 @@ func (r RepoStatus) String() string { // // GitHub API docs: https://developer.github.com/v3/repos/statuses/#list-statuses-for-a-specific-ref func (s *RepositoriesService) ListStatuses(ctx context.Context, owner, repo, ref string, opts *ListOptions) ([]*RepoStatus, *Response, error) { - u := fmt.Sprintf("repos/%v/%v/commits/%v/statuses", owner, repo, url.QueryEscape(ref)) + u := fmt.Sprintf("repos/%v/%v/commits/%v/statuses", owner, repo, refURLEscape(ref)) u, err := addOptions(u, opts) if err != nil { return nil, nil, err @@ -71,7 +70,7 @@ func (s *RepositoriesService) ListStatuses(ctx context.Context, owner, repo, ref // // GitHub API docs: https://developer.github.com/v3/repos/statuses/#create-a-status func (s *RepositoriesService) CreateStatus(ctx context.Context, owner, repo, ref string, status *RepoStatus) (*RepoStatus, *Response, error) { - u := fmt.Sprintf("repos/%v/%v/statuses/%v", owner, repo, url.QueryEscape(ref)) + u := fmt.Sprintf("repos/%v/%v/statuses/%v", owner, repo, refURLEscape(ref)) req, err := s.client.NewRequest("POST", u, status) if err != nil { return nil, nil, err @@ -110,7 +109,7 @@ func (s CombinedStatus) String() string { // // GitHub API docs: https://developer.github.com/v3/repos/statuses/#get-the-combined-status-for-a-specific-ref func (s *RepositoriesService) GetCombinedStatus(ctx context.Context, owner, repo, ref string, opts *ListOptions) (*CombinedStatus, *Response, error) { - u := fmt.Sprintf("repos/%v/%v/commits/%v/status", owner, repo, url.QueryEscape(ref)) + u := fmt.Sprintf("repos/%v/%v/commits/%v/status", owner, repo, refURLEscape(ref)) u, err := addOptions(u, opts) if err != nil { return nil, nil, err From 63583852818dda78e3b21f60dfac70d923c9f529 Mon Sep 17 00:00:00 2001 From: Hanno Hecker Date: Mon, 17 Feb 2020 16:20:26 +0100 Subject: [PATCH 2/4] unit test for refURLEscape this is not visible in the original tests, because the muxter uses url.Path and not url.RawPath to match the route. --- github/git_refs.go | 3 ++- github/git_refs_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/github/git_refs.go b/github/git_refs.go index 68b668be44f..ec945b719cd 100644 --- a/github/git_refs.go +++ b/github/git_refs.go @@ -79,10 +79,11 @@ func (s *GitService) GetRef(ctx context.Context, owner string, repo string, ref return r, resp, nil } +// refURLEscape escapes every path segment of the given ref. func refURLEscape(ref string) string { parts := strings.Split(ref, "/") for i, s := range parts { - parts[i] = url.QueryEscape(s) + parts[i] = url.PathEscape(s) } return strings.Join(parts, "/") } diff --git a/github/git_refs_test.go b/github/git_refs_test.go index fb91c043316..22dc8eb6247 100644 --- a/github/git_refs_test.go +++ b/github/git_refs_test.go @@ -11,6 +11,7 @@ import ( "fmt" "net/http" "reflect" + "strings" "testing" ) @@ -444,3 +445,30 @@ func TestGitService_DeleteRef(t *testing.T) { t.Errorf("Git.DeleteRef returned error: %v", err) } } + +func TestGitService_GetRef_pathEscape(t *testing.T) { + client, mux, _, teardown := setup() + defer teardown() + + mux.HandleFunc("/repos/o/r/git/refs/heads/b", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + if strings.Contains(r.URL.RawPath, "%2F") { + t.Errorf("RawPath still contains escaped / as %%2F: %v", r.URL.RawPath) + } + fmt.Fprint(w, ` + { + "ref": "refs/heads/b", + "url": "https://api.github.com/repos/o/r/git/refs/heads/b", + "object": { + "type": "commit", + "sha": "aa218f56b14c9653891f9e74264a383fa43fefbd", + "url": "https://api.github.com/repos/o/r/git/commits/aa218f56b14c9653891f9e74264a383fa43fefbd" + } + }`) + }) + + _, _, err := client.Git.GetRef(context.Background(), "o", "r", "refs/heads/b") + if err != nil { + t.Fatalf("Git.GetRef returned error: %v", err) + } +} From 3f357278b94c198c5f59652cd7f5179a03c48388 Mon Sep 17 00:00:00 2001 From: Hanno Hecker Date: Mon, 17 Feb 2020 16:32:58 +0100 Subject: [PATCH 3/4] update comment --- github/git_refs.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/github/git_refs.go b/github/git_refs.go index ec945b719cd..a6269e5a9e4 100644 --- a/github/git_refs.go +++ b/github/git_refs.go @@ -79,7 +79,8 @@ func (s *GitService) GetRef(ctx context.Context, owner string, repo string, ref return r, resp, nil } -// refURLEscape escapes every path segment of the given ref. +// refURLEscape escapes every path segment of the given ref. Those must +// not contain escaped "/" - as "%2F" - or github will not recognize it. func refURLEscape(ref string) string { parts := strings.Split(ref, "/") for i, s := range parts { From 3f8549841d88e7494dc32640fb0070459800ccf0 Mon Sep 17 00:00:00 2001 From: Hanno Hecker Date: Tue, 18 Feb 2020 07:43:03 +0100 Subject: [PATCH 4/4] add test for traling % as given in the original #1099 --- github/repos_commits_test.go | 39 ++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/github/repos_commits_test.go b/github/repos_commits_test.go index 54a60147856..1c4fd77462c 100644 --- a/github/repos_commits_test.go +++ b/github/repos_commits_test.go @@ -262,6 +262,45 @@ func TestRepositoriesService_NonAlphabetCharacter_GetCommitSHA1(t *testing.T) { } } +func TestRepositoriesService_TrailingPercent_GetCommitSHA1(t *testing.T) { + client, mux, _, teardown := setup() + defer teardown() + const sha1 = "01234abcde" + + mux.HandleFunc("/repos/o/r/commits/comm%", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + testHeader(t, r, "Accept", mediaTypeV3SHA) + + fmt.Fprintf(w, sha1) + }) + + got, _, err := client.Repositories.GetCommitSHA1(context.Background(), "o", "r", "comm%", "") + if err != nil { + t.Errorf("Repositories.GetCommitSHA1 returned error: %v", err) + } + + if want := sha1; got != want { + t.Errorf("Repositories.GetCommitSHA1 = %v, want %v", got, want) + } + + mux.HandleFunc("/repos/o/r/commits/tag", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + testHeader(t, r, "Accept", mediaTypeV3SHA) + testHeader(t, r, "If-None-Match", `"`+sha1+`"`) + + w.WriteHeader(http.StatusNotModified) + }) + + got, _, err = client.Repositories.GetCommitSHA1(context.Background(), "o", "r", "tag", sha1) + if err == nil { + t.Errorf("Expected HTTP 304 response") + } + + if want := ""; got != want { + t.Errorf("Repositories.GetCommitSHA1 = %v, want %v", got, want) + } +} + func TestRepositoriesService_CompareCommits(t *testing.T) { client, mux, _, teardown := setup() defer teardown()