Repository: google/go-github
Pull request #1774 (2021-01-10 10:26:14)

Escape base and head in repos_commits.go
Fixes: #1642 as reported in #1654 

I added code to escape `base` and `head` part of the URL in `CompareCommits` and `CompareCommitsRaw` methods.
Also did changes in relevant test methods, which are now testing for the following 3 cases:


 ```java
         {base: "b", head: "h"},
		{base: "123base", head: "head123"},
		{base: "`~!@#$%^&*()_+-=[]\\{}|;':\",./<>?/*-+123base", head: "head123`~!@#$%^&*()_+-=[]\\{}|;':\",./<>?/*-+"},
```
P.S. - Here's the original [pull request](https://github.com/google/go-github/pull/1649) that had to be reverted.

codecov[bot]: # [Codecov](https://codecov.io/gh/google/go-github/pull/1774?src=pr&el=h1) Report
> Merging [#1774](https://codecov.io/gh/google/go-github/pull/1774?src=pr&el=desc) (a9d15b4) into [master](https://codecov.io/gh/google/go-github/commit/c2c4a6d477016a3e73a63d67b1933fb24f52fd99?el=desc) (c2c4a6d) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/google/go-github/pull/1774/graphs/tree.svg?width=650&height=150&src=pr&token=EgYjGjZlI0)](https://codecov.io/gh/google/go-github/pull/1774?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master    #1774   +/-   ##
=======================================
  Coverage   97.53%   97.53%           
=======================================
  Files          98       98           
  Lines        6373     6377    +4     
=======================================
+ Hits         6216     6220    +4     
  Misses         85       85           
  Partials       72       72           
```


| [Impacted Files](https://codecov.io/gh/google/go-github/pull/1774?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [github/repos\_commits.go](https://codecov.io/gh/google/go-github/pull/1774/diff?src=pr&el=tree#diff-Z2l0aHViL3JlcG9zX2NvbW1pdHMuZ28=) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/google/go-github/pull/1774?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/google/go-github/pull/1774?src=pr&el=footer). Last update [c2c4a6d...a9d15b4](https://codecov.io/gh/google/go-github/pull/1774?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

gmlewis: Thank you, @wesleimp !
Merging.

Patch:
diff --git a/github/repos_commits.go b/github/repos_commits.go
index 707e5cb1..b49a5afe 100644
--- a/github/repos_commits.go
+++ b/github/repos_commits.go
@@ -9,6 +9,7 @@ import (
 	"bytes"
 	"context"
 	"fmt"
+	"net/url"
 	"time"
 )
 
@@ -224,7 +225,10 @@ func (s *RepositoriesService) GetCommitSHA1(ctx context.Context, owner, repo, re
 //
 // GitHub API docs: https://docs.github.com/en/free-pro-team@latest/rest/reference/repos/#compare-two-commits
 func (s *RepositoriesService) CompareCommits(ctx context.Context, owner, repo string, base, head string) (*CommitsComparison, *Response, error) {
-	u := fmt.Sprintf("repos/%v/%v/compare/%v...%v", owner, repo, base, head)
+	escapedBase := url.QueryEscape(base)
+	escapedHead := url.QueryEscape(head)
+
+	u := fmt.Sprintf("repos/%v/%v/compare/%v...%v", owner, repo, escapedBase, escapedHead)
 
 	req, err := s.client.NewRequest("GET", u, nil)
 	if err != nil {
@@ -248,7 +252,11 @@ func (s *RepositoriesService) CompareCommits(ctx context.Context, owner, repo st
 //
 // GitHub API docs: https://docs.github.com/en/free-pro-team@latest/rest/reference/repos/#compare-two-commits
 func (s *RepositoriesService) CompareCommitsRaw(ctx context.Context, owner, repo, base, head string, opts RawOptions) (string, *Response, error) {
-	u := fmt.Sprintf("repos/%v/%v/compare/%v...%v", owner, repo, base, head)
+	escapedBase := url.QueryEscape(base)
+	escapedHead := url.QueryEscape(head)
+
+	u := fmt.Sprintf("repos/%v/%v/compare/%v...%v", owner, repo, escapedBase, escapedHead)
+
 	req, err := s.client.NewRequest("GET", u, nil)
 	if err != nil {
 		return "", nil, err


Test patch:
diff --git a/github/repos_commits_test.go b/github/repos_commits_test.go
index 0deeeef5..f2fde498 100644
--- a/github/repos_commits_test.go
+++ b/github/repos_commits_test.go
@@ -9,6 +9,7 @@ import (
 	"context"
 	"fmt"
 	"net/http"
+	"net/url"
 	"reflect"
 	"strings"
 	"testing"
@@ -366,12 +367,28 @@ func TestRepositoriesService_TrailingPercent_GetCommitSHA1(t *testing.T) {
 }
 
 func TestRepositoriesService_CompareCommits(t *testing.T) {
-	client, mux, _, teardown := setup()
-	defer teardown()
+	testCases := []struct {
+		base string
+		head string
+	}{
+		{base: "b", head: "h"},
+		{base: "123base", head: "head123"},
+		{base: "`~!@#$%^&*()_+-=[]\\{}|;':\",./<>?/*-+123base", head: "head123`~!@#$%^&*()_+-=[]\\{}|;':\",./<>?/*-+"},
+	}
 
-	mux.HandleFunc("/repos/o/r/compare/b...h", func(w http.ResponseWriter, r *http.Request) {
-		testMethod(t, r, "GET")
-		fmt.Fprintf(w, `{
+	for _, sample := range testCases {
+		client, mux, _, teardown := setup()
+
+		base := sample.base
+		head := sample.head
+		escapedBase := url.QueryEscape(base)
+		escapedHead := url.QueryEscape(head)
+
+		pattern := fmt.Sprintf("/repos/o/r/compare/%v...%v", base, head)
+
+		mux.HandleFunc(pattern, func(w http.ResponseWriter, r *http.Request) {
+			testMethod(t, r, "GET")
+			fmt.Fprintf(w, `{
 		  "base_commit": {
 		    "sha": "s",
 		    "commit": {
@@ -398,46 +415,28 @@ func TestRepositoriesService_CompareCommits(t *testing.T) {
 		    }
 		  ],
 		  "files": [ { "filename": "f" } ],
-		  "html_url":      "https://github.com/o/r/compare/b...h",
+		  "html_url":      "https://github.com/o/r/compare/%[1]v...%[2]v",
 		  "permalink_url": "https://github.com/o/r/compare/o:bbcd538c8e72b8c175046e27cc8f907076331401...o:0328041d1152db8ae77652d1618a02e57f745f17",
-		  "diff_url":      "https://github.com/o/r/compare/b...h.diff",
-		  "patch_url":     "https://github.com/o/r/compare/b...h.patch",
-		  "url":           "https://api.github.com/repos/o/r/compare/b...h"
-		}`)
-	})
-
-	ctx := context.Background()
-	got, _, err := client.Repositories.CompareCommits(ctx, "o", "r", "b", "h")
-	if err != nil {
-		t.Errorf("Repositories.CompareCommits returned error: %v", err)
-	}
+		  "diff_url":      "https://github.com/o/r/compare/%[1]v...%[2]v.diff",
+		  "patch_url":     "https://github.com/o/r/compare/%[1]v...%[2]v.patch",
+		  "url":           "https://api.github.com/repos/o/r/compare/%[1]v...%[2]v"
+		}`, escapedBase, escapedHead)
+		})
+
+		ctx := context.Background()
+		got, _, err := client.Repositories.CompareCommits(ctx, "o", "r", base, head)
+		if err != nil {
+			t.Errorf("Repositories.CompareCommits returned error: %v", err)
+		}
 
-	want := &CommitsComparison{
-		BaseCommit: &RepositoryCommit{
-			SHA: String("s"),
-			Commit: &Commit{
-				Author:    &CommitAuthor{Name: String("n")},
-				Committer: &CommitAuthor{Name: String("n")},
-				Message:   String("m"),
-				Tree:      &Tree{SHA: String("t")},
-			},
-			Author:    &User{Login: String("l")},
-			Committer: &User{Login: String("l")},
-			Parents: []*Commit{
-				{
-					SHA: String("s"),
-				},
-			},
-		},
-		Status:       String("s"),
-		AheadBy:      Int(1),
-		BehindBy:     Int(2),
-		TotalCommits: Int(1),
-		Commits: []*RepositoryCommit{
-			{
+		want := &CommitsComparison{
+			BaseCommit: &RepositoryCommit{
 				SHA: String("s"),
 				Commit: &Commit{
-					Author: &CommitAuthor{Name: String("n")},
+					Author:    &CommitAuthor{Name: String("n")},
+					Committer: &CommitAuthor{Name: String("n")},
+					Message:   String("m"),
+					Tree:      &Tree{SHA: String("t")},
 				},
 				Author:    &User{Login: String("l")},
 				Committer: &User{Login: String("l")},
@@ -447,109 +446,171 @@ func TestRepositoriesService_CompareCommits(t *testing.T) {
 					},
 				},
 			},
-		},
-		Files: []*CommitFile{
-			{
-				Filename: String("f"),
+			Status:       String("s"),
+			AheadBy:      Int(1),
+			BehindBy:     Int(2),
+			TotalCommits: Int(1),
+			Commits: []*RepositoryCommit{
+				{
+					SHA: String("s"),
+					Commit: &Commit{
+						Author: &CommitAuthor{Name: String("n")},
+					},
+					Author:    &User{Login: String("l")},
+					Committer: &User{Login: String("l")},
+					Parents: []*Commit{
+						{
+							SHA: String("s"),
+						},
+					},
+				},
 			},
-		},
-		HTMLURL:      String("https://github.com/o/r/compare/b...h"),
-		PermalinkURL: String("https://github.com/o/r/compare/o:bbcd538c8e72b8c175046e27cc8f907076331401...o:0328041d1152db8ae77652d1618a02e57f745f17"),
-		DiffURL:      String("https://github.com/o/r/compare/b...h.diff"),
-		PatchURL:     String("https://github.com/o/r/compare/b...h.patch"),
-		URL:          String("https://api.github.com/repos/o/r/compare/b...h"),
-	}
+			Files: []*CommitFile{
+				{
+					Filename: String("f"),
+				},
+			},
+			HTMLURL:      String(fmt.Sprintf("https://github.com/o/r/compare/%v...%v", escapedBase, escapedHead)),
+			PermalinkURL: String("https://github.com/o/r/compare/o:bbcd538c8e72b8c175046e27cc8f907076331401...o:0328041d1152db8ae77652d1618a02e57f745f17"),
+			DiffURL:      String(fmt.Sprintf("https://github.com/o/r/compare/%v...%v.diff", escapedBase, escapedHead)),
+			PatchURL:     String(fmt.Sprintf("https://github.com/o/r/compare/%v...%v.patch", escapedBase, escapedHead)),
+			URL:          String(fmt.Sprintf("https://api.github.com/repos/o/r/compare/%v...%v", escapedBase, escapedHead)),
+		}
 
-	if !reflect.DeepEqual(got, want) {
-		t.Errorf("Repositories.CompareCommits returned \n%+v, want \n%+v", got, want)
-	}
+		if !reflect.DeepEqual(got, want) {
+			t.Errorf("Repositories.CompareCommits returned \n%+v, want \n%+v", got, want)
+		}
 
-	const methodName = "CompareCommits"
-	testBadOptions(t, methodName, func() (err error) {
-		_, _, err = client.Repositories.CompareCommits(ctx, "\n", "\n", "\n", "\n")
-		return err
-	})
+		const methodName = "CompareCommits"
+		testBadOptions(t, methodName, func() (err error) {
+			_, _, err = client.Repositories.CompareCommits(ctx, "\n", "\n", "\n", "\n")
+			return err
+		})
 
-	testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
-		got, resp, err := client.Repositories.CompareCommits(ctx, "o", "r", "b", "h")
-		if got != nil {
-			t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
-		}
-		return resp, err
-	})
+		testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
+			got, resp, err := client.Repositories.CompareCommits(ctx, "o", "r", base, head)
+			if got != nil {
+				t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
+			}
+			return resp, err
+		})
+
+		teardown()
+	}
 }
 
 func TestRepositoriesService_CompareCommitsRaw_diff(t *testing.T) {
-	client, mux, _, teardown := setup()
-	defer teardown()
+	testCases := []struct {
+		base string
+		head string
+	}{
+		{base: "b", head: "h"},
+		{base: "123base", head: "head123"},
+		{base: "`~!@#$%^&*()_+-=[]\\{}|;':\",./<>?/*-+123base", head: "head123`~!@#$%^&*()_+-=[]\\{}|;':\",./<>?/*-+"},
+	}
+
+	for _, sample := range testCases {
+		client, mux, _, teardown := setup()
+
+		base := sample.base
+		head := sample.head
+		pattern := fmt.Sprintf("/repos/o/r/compare/%v...%v", base, head)
+		const rawStr = "@@diff content"
+
+		mux.HandleFunc(pattern, func(w http.ResponseWriter, r *http.Request) {
+			testMethod(t, r, "GET")
+			testHeader(t, r, "Accept", mediaTypeV3Diff)
+			fmt.Fprint(w, rawStr)
+		})
+
+		ctx := context.Background()
+		got, _, err := client.Repositories.CompareCommitsRaw(ctx, "o", "r", base, head, RawOptions{Type: Diff})
+		if err != nil {
+			t.Fatalf("Repositories.GetCommitRaw returned error: %v", err)
+		}
+		want := rawStr
+		if got != want {
+			t.Errorf("Repositories.GetCommitRaw returned %s want %s", got, want)
+		}
 
-	const rawStr = "@@diff content"
+		const methodName = "CompareCommitsRaw"
+		testBadOptions(t, methodName, func() (err error) {
+			_, _, err = client.Repositories.CompareCommitsRaw(ctx, "\n", "\n", "\n", "\n", RawOptions{Type: Diff})
+			return err
+		})
 
-	mux.HandleFunc("/repos/o/r/compare/b...h", func(w http.ResponseWriter, r *http.Request) {
-		testMethod(t, r, "GET")
-		testHeader(t, r, "Accept", mediaTypeV3Diff)
-		fmt.Fprint(w, rawStr)
-	})
+		testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
+			got, resp, err := client.Repositories.CompareCommitsRaw(ctx, "o", "r", base, head, RawOptions{Type: Diff})
+			if got != "" {
+				t.Errorf("testNewRequestAndDoFailure %v = %#v, want ''", methodName, got)
+			}
+			return resp, err
+		})
 
-	ctx := context.Background()
-	got, _, err := client.Repositories.CompareCommitsRaw(ctx, "o", "r", "b", "h", RawOptions{Type: Diff})
-	if err != nil {
-		t.Fatalf("Repositories.GetCommitRaw returned error: %v", err)
+		teardown()
 	}
-	want := rawStr
-	if got != want {
-		t.Errorf("Repositories.GetCommitRaw returned %s want %s", got, want)
-	}
-
-	const methodName = "CompareCommitsRaw"
-	testBadOptions(t, methodName, func() (err error) {
-		_, _, err = client.Repositories.CompareCommitsRaw(ctx, "\n", "\n", "\n", "\n", RawOptions{Type: Diff})
-		return err
-	})
-
-	testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
-		got, resp, err := client.Repositories.CompareCommitsRaw(ctx, "o", "r", "b", "h", RawOptions{Type: Diff})
-		if got != "" {
-			t.Errorf("testNewRequestAndDoFailure %v = %#v, want ''", methodName, got)
-		}
-		return resp, err
-	})
 }
 
 func TestRepositoriesService_CompareCommitsRaw_patch(t *testing.T) {
-	client, mux, _, teardown := setup()
-	defer teardown()
-
-	const rawStr = "@@patch content"
-
-	mux.HandleFunc("/repos/o/r/compare/b...h", func(w http.ResponseWriter, r *http.Request) {
-		testMethod(t, r, "GET")
-		testHeader(t, r, "Accept", mediaTypeV3Patch)
-		fmt.Fprint(w, rawStr)
-	})
+	testCases := []struct {
+		base string
+		head string
+	}{
+		{base: "b", head: "h"},
+		{base: "123base", head: "head123"},
+		{base: "`~!@#$%^&*()_+-=[]\\{}|;':\",./<>?/*-+123base", head: "head123`~!@#$%^&*()_+-=[]\\{}|;':\",./<>?/*-+"},
+	}
+
+	for _, sample := range testCases {
+		client, mux, _, teardown := setup()
+
+		base := sample.base
+		head := sample.head
+		pattern := fmt.Sprintf("/repos/o/r/compare/%v...%v", base, head)
+		const rawStr = "@@patch content"
+
+		mux.HandleFunc(pattern, func(w http.ResponseWriter, r *http.Request) {
+			testMethod(t, r, "GET")
+			testHeader(t, r, "Accept", mediaTypeV3Patch)
+			fmt.Fprint(w, rawStr)
+		})
+
+		ctx := context.Background()
+		got, _, err := client.Repositories.CompareCommitsRaw(ctx, "o", "r", base, head, RawOptions{Type: Patch})
+		if err != nil {
+			t.Fatalf("Repositories.GetCommitRaw returned error: %v", err)
+		}
+		want := rawStr
+		if got != want {
+			t.Errorf("Repositories.GetCommitRaw returned %s want %s", got, want)
+		}
 
-	ctx := context.Background()
-	got, _, err := client.Repositories.CompareCommitsRaw(ctx, "o", "r", "b", "h", RawOptions{Type: Patch})
-	if err != nil {
-		t.Fatalf("Repositories.GetCommitRaw returned error: %v", err)
-	}
-	want := rawStr
-	if got != want {
-		t.Errorf("Repositories.GetCommitRaw returned %s want %s", got, want)
+		teardown()
 	}
 }
 
 func TestRepositoriesService_CompareCommitsRaw_invalid(t *testing.T) {
-	client, _, _, teardown := setup()
-	defer teardown()
-
 	ctx := context.Background()
-	_, _, err := client.Repositories.CompareCommitsRaw(ctx, "o", "r", "s", "h", RawOptions{100})
-	if err == nil {
-		t.Fatal("Repositories.GetCommitRaw should return error")
+
+	testCases := []struct {
+		base string
+		head string
+	}{
+		{base: "b", head: "h"},
+		{base: "123base", head: "head123"},
+		{base: "`~!@#$%^&*()_+-=[]\\{}|;':\",./<>?/*-+123base", head: "head123`~!@#$%^&*()_+-=[]\\{}|;':\",./<>?/*-+"},
 	}
-	if !strings.Contains(err.Error(), "unsupported raw type") {
-		t.Error("Repositories.GetCommitRaw should return unsupported raw type error")
+
+	for _, sample := range testCases {
+		client, _, _, teardown := setup()
+		_, _, err := client.Repositories.CompareCommitsRaw(ctx, "o", "r", sample.base, sample.head, RawOptions{100})
+		if err == nil {
+			t.Fatal("Repositories.GetCommitRaw should return error")
+		}
+		if !strings.Contains(err.Error(), "unsupported raw type") {
+			t.Error("Repositories.GetCommitRaw should return unsupported raw type error")
+		}
+		teardown()
 	}
 }