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

Robust Header Check #13

Closed
wants to merge 12 commits into from
3 changes: 2 additions & 1 deletion appsTransport.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,9 @@ func (t *AppsTransport) RoundTrip(req *http.Request) (*http.Response, error) {
}

req.Header.Set("Authorization", "Bearer "+ss)
req.Header.Set("Accept", acceptHeader)
addAcceptHeader(req)

resp, err := t.tr.RoundTrip(req)
return resp, err

Copy link
Owner

Choose a reason for hiding this comment

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

nit: can you remove this extra newline?

}
22 changes: 21 additions & 1 deletion transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"io/ioutil"
"net/http"
"strings"
"sync"
"time"
)
Expand Down Expand Up @@ -90,7 +91,7 @@ func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) {
}

req.Header.Set("Authorization", "token "+token)
req.Header.Add("Accept", acceptHeader) // We add to "Accept" header to avoid overwriting existing req headers.
addAcceptHeader(req)
resp, err := t.tr.RoundTrip(req)
return resp, err
}
Expand Down Expand Up @@ -134,3 +135,22 @@ func (t *Transport) refreshToken() error {

return nil
}

func addAcceptHeader(req *http.Request) {
Copy link
Owner

Choose a reason for hiding this comment

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

Could this be slightly rewritten to return early when possible, what's your thoughts on this? I haven't tested it at all.

// addAcceptIfJSON adds the preview accept headers if the request does not already specify an
// accept header, or if it's already specifying acceptance of json type as defined by
// https://developer.github.com/v3/media/.
func addAcceptIfJSON(headers http.Header) {
	if headers.Get("Accept") == "" {
		headers.Set("Accept", acceptHeader)
                return
	}

	for _, header := range headers["Accept"] {
		if strings.HasSuffix(header, "json") {
			headers.Add("Accept", acceptHeader)
			return
		}
	}
}

Just accepting a http.Header also simplifies some of the tests, as the function just requires what it checks and modifies, nothing more. But again, I haven't tested this at all.

Copy link
Author

Choose a reason for hiding this comment

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

@bradleyfalzon Made the changes

if req.Header.Get("Accept") != "" {
//Need to loop through all Accept headers incase there is more than one.
for headerName, headers := range req.Header {
if strings.ToLower(headerName) == "accept" {
for _, header := range headers {
//Looks as though all media types (https://developer.github.com/v3/media/) that can accept json end with "json". Only doing a suffix check to see if a json header already exists.
if strings.HasSuffix(header, "json") {
req.Header.Add("Accept", acceptHeader) // We add to "Accept" header to avoid overwriting existing req headers.
return
}
}
}
}
} else {
req.Header.Add("Accept", acceptHeader) // We add to "Accept" header to avoid overwriting existing req headers.
}
}
76 changes: 76 additions & 0 deletions transport_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"net/http"
"net/http/httptest"
"os"
"strings"
"testing"
"time"
)
Expand Down Expand Up @@ -173,3 +174,78 @@ func TestNew_appendHeader(t *testing.T) {
t.Errorf("could not find %v in request's accept headers: %v", myheader, headers["Accept"])
}
}

func TestSingleAssetBinary_AddAcceptHeader(t *testing.T) {
req, err := http.NewRequest(http.MethodGet, "https://api.github.com/repos/bradleyfalzon/ghinstallation/releases/assets/1", http.NoBody)
if err != nil {
t.Fatalf("Failed to create test request: %s", err.Error())
}

req.Header.Set("Accept", "application/octet-stream")

addAcceptHeader(req)

for headerName, headers := range req.Header {
if strings.ToLower(headerName) == "accept" {
for _, header := range headers {
if header == acceptHeader {
fmt.Printf("Header Name: %s, Header: %s", headerName, header)
t.Error("Set Accept header improperly for single asset call")
}
}
}
}
}

func TestNormal_AddAcceptHeader(t *testing.T) {
req, err := http.NewRequest(http.MethodGet, "https://api.github.com/repos", http.NoBody)
if err != nil {
t.Fatalf("Failed to create test request: %s", err.Error())
}

addAcceptHeader(req)

if req.Header.Get("Accept") != acceptHeader {
t.Error("Did not correctly set 'Accept' header")
}
}

func TestAlreadyExists_AddAcceptHeader(t *testing.T) {
req, err := http.NewRequest(http.MethodGet, "https://api.github.com/repos", http.NoBody)
if err != nil {
t.Fatalf("Failed to create test request: %s", err.Error())
}

req.Header.Set("Accept", "application/vnd.github.v3+json")

addAcceptHeader(req)

found := false
for headerName, headers := range req.Header {
if strings.ToLower(headerName) == "accept" {
for _, header := range headers {
if header == acceptHeader {
found = true
break
}
}
}
}

if !found {
t.Errorf("Did not correctly append %s header", acceptHeader)
}
}

func TestSingleAssetJSON_AddAcceptHeader(t *testing.T) {
req, err := http.NewRequest(http.MethodGet, "https://api.github.com/repos/bradleyfalzon/ghinstallation/releases/assets/1", http.NoBody)
if err != nil {
t.Fatalf("Failed to create test request: %s", err.Error())
}

addAcceptHeader(req)

if req.Header.Get("Accept") != acceptHeader {
t.Error("Did not correctly set 'Accept' header")
}
}