Robust Header Check #13

cpheps · 2018-04-09T19:23:16Z

Added a robust check when adding the Accept Header, if an Accept header already exists with the value application/octet-stream then don't add the additional Accept header.

Resolves #12

… 'application/octet-stream' we don't want to add another

cpheps · 2018-04-19T14:06:56Z

@bradleyfalzon any chance you could take a look at this? My company is using the fork in production now and we don't want to do that for to long.

bradleyfalzon · 2018-04-22T23:32:31Z

Sorry for the delay on this one. I'm confused, we need to send the Accept header with application/vnd.github.machine-man-preview+json, but you're saying if this is sent to a different endpoint that still requires the normal token authentication it fails to work?

And therefore you're choosing to not send application/vnd.github.machine-man-preview+json at all?

As I'm thinking that instead of adding another accept header, we check if one exists and append to that header?

cpheps · 2018-04-22T23:47:30Z

The behavior I'm seeing is regardless of authentication type when trying to download a single asset from the GitHub API (documentation) you can only have a single accept header application/octet-stream. If the application/vnd.github.machine-man-preview+json is present along with the application/octet-stream it will return JSON only and not start a download.

I was able to verify this behavior just making a curl command with a personal oauth token.

My solution is if we detect the application/octet-stream don't add the application/vnd.github.machine-man-preview+json.

dmitshur · 2018-04-23T00:45:09Z

At a high level, this makes sense to me. 👍 I hope the implementation can be made simpler though.

@bradleyfalzon See a relevant issue google/go-github#870 if you’d like more context.

cpheps · 2018-04-23T00:49:39Z

@shurcooL Agreed on the implementation, I wasn't sure if there was a more elegant want to handle it. I'd be happy to change it if we can find something better.

bradleyfalzon · 2018-04-23T02:46:07Z

OK, so I see, it's working around a suspected bug on GitHub's side.

So essentially, if the request already has an Accept header, and it's equal to application/octet-stream , don't send the application/vnd.github.machine-man-preview+json header?

I like the cleaner google/go-github#870 implementation because it just disables the header for that one endpoint.

If it's only one particular URL that's causing the issue. Then it's clearer what bug we're working around?

dmitshur · 2018-04-23T03:14:45Z

So essentially, if the request already has an Accept header, and it's equal to application/octet-stream, don't send the application/vnd.github.machine-man-preview+json header?

That's right.

OK, so I see, it's working around a suspected bug on GitHub's side.

I don't think it's a bug, I think it's working correctly.

The documentation for https://developer.github.com/v3/repos/releases/#get-a-single-release-asset endpoint says:

To download the asset's binary content, set the Accept header of the request to application/octet-stream.

Basically, that endpoint can respond either with a JSON payload, or binary data, depending on what the client prefers.

If you read the documentation at https://developer.github.com/v3/media/ and https://developer.github.com/v3/previews/:

All GitHub media types look like this:
application/vnd.github[.version].param[+json]

It becomes more clear how to parse the following Accept header value:

application/vnd.github.machine-man-preview+json

It means to use "machine-man-preview" preview version, and since +json is specified, to respond using the JSON representation of resources.

So, when both application/octet-stream and application/vnd.github.machine-man-preview+json accept headers are specified, that GitHub API endpoint sees a request saying "I accept both JSON and raw bytes". The server decides to pick one of them (JSON).

…single asset

gopherci · 2018-04-23T12:19:11Z

transport.go

 )

+//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+")


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

cpheps · 2018-04-23T12:19:13Z

@bradleyfalzon I refactored the code to specifically look for the case to download a single asset.

dmitshur · 2018-04-23T19:02:38Z

I would suggest you consider this more general approach:

Check if the current request accept header asks for JSON.
If so, add application/vnd.github.machine-man-preview+json as an extra accept header value, if it’s not already present.
Otherwise, do not modify the accept header.

What do you think?

cpheps · 2018-04-24T11:20:51Z

@shurcooL I like that idea, little confused on how I can tell if it's asking for JSON. Would it be as simple as checking the Accept header? If so I would think my original solution of checking all accept headers would be the only way to correctly check for a JSON header. I know we said that wasn't very elegant but if we're all comfortable with that I can make the change.

dmitshur · 2018-04-24T17:05:23Z

I like that idea, little confused on how I can tell if it's asking for JSON. Would it be as simple as checking the Accept header? If so I would think my original solution of checking all accept headers would be the only way to correctly check for a JSON header.

Yes, you'd have to check the Accept header values to see if JSON was requested.

I know we said that wasn't very elegant but if we're all comfortable with that I can make the change.

Please wait to hear back from @bradleyfalzon as he's the owner here. He should read my comments following his, because I think that the original analysis was not accurate, and I tried to provide helpful information to improve it. I don't think the same fix as in go-github is applicable here.

bradleyfalzon · 2018-04-25T04:16:43Z

If so, add application/vnd.github.machine-man-preview+json as an extra accept header value, if it’s not already present.
Otherwise, do not modify the accept header.
What do you think?

I do think this is the best method, although I am concerned about the breaking implications of this for some users not setting that header, and not using go-github.

@shurcooL from my understanding, go-github always sets the relevant accept headers? I.e. by default it sets application/vnd.github.v3+json or the approprite preview headers?

dmitshur · 2018-04-25T04:35:25Z

for some users not setting that header, and not using go-github.

Isn't this package for GitHub? How can users not be using go-github? Asking because I don't know.

@shurcooL from my understanding, go-github always sets the relevant accept headers? I.e. by default it sets application/vnd.github.v3+json or the approprite preview headers?

That's right.

The GitHub REST API v3 expects the media type to be provided in order to control the format of the data to be received. See https://developer.github.com/v3/media/.

You can confirm that go-github always sets an Accept header for its JSON endpoints here:

https://github.com/google/go-github/blob/ab468aa138652ac73874d3a924ac84b26b990142/github/github.go#L318

bradleyfalzon · 2018-04-25T05:32:04Z

Isn't this package for GitHub? How can users not be using go-github? Asking because I don't know.

It's for GitHub, but it doesn't necessarily require users to use the go-github package.

I question this because https://developer.github.com/v3/#schema mentions:

All data is sent and received as JSON.

Although, just previous to that is https://developer.github.com/v3/#current-version

We encourage you to explicitly request this version via the Accept header.

So, similar to RFC guidelines of the term SHOULD, if a client is not setting this, it's expected they understood the implications of this.

I'm then OK with the simpler approach by @shurcooL #13 (comment)

Check if the current request accept header asks for JSON.
If so, add application/vnd.github.machine-man-preview+json as an extra accept header value, if it’s not already present.
Otherwise, do not modify the accept header.

@cpheps will this be satisfactory for you also?

cpheps · 2018-04-25T11:31:10Z

@bradleyfalzon That seems appropriate for me, I'll try and get a modification in later today.

…. If either cases is true append machine-man-preview+json accpet header

bradleyfalzon

Tiny nits, but can you review my alternate implementation?

bradleyfalzon · 2018-04-26T03:09:00Z

appsTransport.go


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


nit: can you remove this extra newline?

bradleyfalzon · 2018-04-26T03:32:32Z

transport.go

@@ -134,3 +135,22 @@ func (t *Transport) refreshToken() error {

 	return nil
 }
+
+func addAcceptHeader(req *http.Request) {


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.

@bradleyfalzon Made the changes

dmitshur · 2018-04-26T14:35:40Z

transport.go

+		return
+	}
+
+	//Need to loop through all Accept headers incase there is more than one.


Minor style issue. Since this is a normal comment for humans rather than for the compiler, it should have a space after //, like so:

// Need to ...

See https://dmitri.shuralyov.com/idiomatic-go#comments-for-humans-always-have-a-single-space-after-the-slashes for rationale.

Also, “incase” should be two words I think.

@cpheps Did you see this comment?

Missed the second one, just pushed a fix.

What about the first part? I still see some comments that don't have a space after //.

dmitshur · 2018-04-26T14:37:12Z

transport.go

+
+	//Need to loop through all Accept headers incase there is more than one.
+	for _, header := range headers["Accept"] {
+		//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.


Here as well:

// Looks as though ...

You might also want to wrap the line at around 80-100 columns, so it’s more comfortable to read.

cpheps · 2018-04-30T13:47:15Z

@bradleyfalzon Fixed those issues a few days ago any other requests?

cpheps · 2018-05-10T11:49:13Z

@bradleyfalzon Just wanted to check on this and see if there was anything blocking the merge still?

dmitshur · 2018-05-10T16:28:55Z

transport.go

@@ -134,3 +135,20 @@ func (t *Transport) refreshToken() error {

 	return nil
 }
+
+func addAcceptHeader(headers http.Header) {


It would be nice to document what the function does. Perhaps something like this:

// addAcceptHeader adds acceptHeader value to "Accept" header, but only // if the current "Accept" header is not set, or if it already accepts JSON. func addAcceptHeader(headers http.Header) {

Copied it directly (seemed like a good description) and pushed.

cpheps · 2018-05-24T17:24:46Z

@shurcooL @bradleyfalzon any more requests, would like to get this merged if it looks good to you guys.

dmitshur · 2018-05-24T18:44:01Z

I still see 3 lines of comments in transport.go file that don't have a space after //, but that's a minor thing. I don't have any other comments, I think this is just waiting on a review from @bradleyfalzon.

cpheps · 2018-06-08T11:01:43Z

@bradleyfalzon I don't mean to be annoying on this but it's been open a while without any movement.

cpheps · 2018-09-14T12:05:16Z

@bradleyfalzon Checking in again seeing if we can get this merged.

bradleyfalzon · 2019-06-07T02:18:02Z

Sorry for the delays in this, I've had another look through, and tentatively it looks OK to me. I'd like someone else to confirm before I merge though.

Corbin Phelps added 3 commits April 9, 2018 15:21

Added a check before adding Accept header. If one already exists with…

844754d

… 'application/octet-stream' we don't want to add another

Added case insensitive check for existing accept headers

2ee8446

Added robust header check to AppsTransport. Ran fmt

8a5d177

Refactored header check to only consider binary download calls for a …

7c7f7b3

…single asset

gopherci bot reviewed Apr 23, 2018

View reviewed changes

Addressed gopherci concern

52eae4e

Removed print from header check

6531cce

Refactored to check for existing json header or missing accept header…

a58e813

…. If either cases is true append machine-man-preview+json accpet header

bradleyfalzon requested changes Apr 26, 2018

View reviewed changes

Cleaned up header check a bit and removed extra new line

9e23b94

dmitshur reviewed Apr 26, 2018

View reviewed changes

Split long comment up into several lines for readability

677e3d3

fixed spelling in comment

b18aaba

dmitshur reviewed May 10, 2018

View reviewed changes

Added description for addAcceptHeader method

3169c9a

Added space before some comments

e9f06a3

bradleyfalzon self-assigned this Jun 7, 2019

cpheps closed this Oct 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Robust Header Check #13

Robust Header Check #13

cpheps commented Apr 9, 2018

cpheps commented Apr 19, 2018

bradleyfalzon commented Apr 22, 2018 •

edited

Loading

cpheps commented Apr 22, 2018

dmitshur commented Apr 23, 2018 •

edited

Loading

cpheps commented Apr 23, 2018

bradleyfalzon commented Apr 23, 2018

dmitshur commented Apr 23, 2018

gopherci bot Apr 23, 2018

cpheps commented Apr 23, 2018

dmitshur commented Apr 23, 2018 •

edited

Loading

cpheps commented Apr 24, 2018

dmitshur commented Apr 24, 2018 •

edited

Loading

bradleyfalzon commented Apr 25, 2018

dmitshur commented Apr 25, 2018 •

edited

Loading

bradleyfalzon commented Apr 25, 2018

cpheps commented Apr 25, 2018

bradleyfalzon left a comment

bradleyfalzon Apr 26, 2018

bradleyfalzon Apr 26, 2018

cpheps Apr 26, 2018

dmitshur Apr 26, 2018

dmitshur Apr 30, 2018

cpheps Apr 30, 2018

dmitshur May 10, 2018

dmitshur Apr 26, 2018

cpheps commented Apr 30, 2018

cpheps commented May 10, 2018

dmitshur May 10, 2018 •

edited

Loading

cpheps May 10, 2018

cpheps commented May 24, 2018

dmitshur commented May 24, 2018

cpheps commented Jun 8, 2018

cpheps commented Sep 14, 2018

bradleyfalzon commented Jun 7, 2019

Robust Header Check #13

Robust Header Check #13

Conversation

cpheps commented Apr 9, 2018

cpheps commented Apr 19, 2018

bradleyfalzon commented Apr 22, 2018 • edited Loading

cpheps commented Apr 22, 2018

dmitshur commented Apr 23, 2018 • edited Loading

cpheps commented Apr 23, 2018

bradleyfalzon commented Apr 23, 2018

dmitshur commented Apr 23, 2018

gopherci bot Apr 23, 2018

Choose a reason for hiding this comment

cpheps commented Apr 23, 2018

dmitshur commented Apr 23, 2018 • edited Loading

cpheps commented Apr 24, 2018

dmitshur commented Apr 24, 2018 • edited Loading

bradleyfalzon commented Apr 25, 2018

dmitshur commented Apr 25, 2018 • edited Loading

bradleyfalzon commented Apr 25, 2018

cpheps commented Apr 25, 2018

bradleyfalzon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpheps commented Apr 30, 2018

cpheps commented May 10, 2018

dmitshur May 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpheps commented May 24, 2018

dmitshur commented May 24, 2018

cpheps commented Jun 8, 2018

cpheps commented Sep 14, 2018

bradleyfalzon commented Jun 7, 2019

bradleyfalzon commented Apr 22, 2018 •

edited

Loading

dmitshur commented Apr 23, 2018 •

edited

Loading

dmitshur commented Apr 23, 2018 •

edited

Loading

dmitshur commented Apr 24, 2018 •

edited

Loading

dmitshur commented Apr 25, 2018 •

edited

Loading

dmitshur May 10, 2018 •

edited

Loading