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?

}
23 changes: 20 additions & 3 deletions transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,21 @@ import (
"fmt"
"io/ioutil"
"net/http"
"regexp"
"sync"
"time"
)

const (
// acceptHeader is the GitHub Integrations Preview Accept header.
acceptHeader = "application/vnd.github.machine-man-preview+json"
apiBaseURL = "https://api.github.com"
acceptHeader = "application/vnd.github.machine-man-preview+json"
defaultMediaType = "application/octet-stream"
apiBaseURL = "https://api.github.com"
)

//Used to detect if a single asset request is being made. https://developer.github.com/v3/repos/releases/#get-a-single-release-asset
var assetPathRegex = regexp.MustCompile("/repos/.+/.+/releases/assets/\\d+")
Copy link

Choose a reason for hiding this comment

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

gosimple: should use raw string (...) with regexp.MustCompile to avoid having to escape twice (S1007)


// Transport provides a http.RoundTripper by wrapping an existing
// http.RoundTripper and provides GitHub Apps authentication as an
// installation.
Expand Down Expand Up @@ -90,7 +95,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 +139,15 @@ 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

//Check to see if we're making a single asset GET request
fmt.Println(req.URL.Path)
if req.Method == http.MethodGet && assetPathRegex.MatchString(req.URL.Path) {
if req.Header.Get("Accept") != defaultMediaType {
req.Header.Add("Accept", acceptHeader) // We add to "Accept" header to avoid overwriting existing req headers.
}
} else {
req.Header.Add("Accept", acceptHeader) // We add to "Accept" header to avoid overwriting existing req headers.
}
}
49 changes: 49 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,51 @@ 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", defaultMediaType)

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 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")
}
}