fix: headers were incorrectly set to application/x-www-form-urlencoded #362
Conversation
This is a long standing bug that prevents us from setting content-type in gaxios. Refs: googleapis/google-api-nodejs-client#2003 Refs: googleapis/gaxios#263
google-cla
bot
added
the
cla: yes
| @@ -610,16 +610,14 @@ describe('.getToken()', () => { | |||
| }); | |||
|
|
|||
| function createGetTokenMock(code = 200, body?: {}) { | |||
| return nock(GOOGLE_TOKEN_URLS[0]) | |||
| return nock(GOOGLE_TOKEN_URLS[0], { | |||
| reqheaders: {'Content-Type': 'application/json'}, | |||
There was a problem hiding this comment.
we had reqheaders in the wrong place, and it was not actually being asserted against.
application/x-www-form-urlencoded was already being replaced by gaxios with application/json.
There was a problem hiding this comment.
I see that the docs do call out that headers are specified in this outer function, but doesn't it have to have some way to define header assertions on a per-route basis? Like it makes sense to be that these could be defined at the top level for the host, but I would expect .get and .post to accept this parameter as well. I am le nervous here. If nock isn't validating headers here, when the types accept Options in InterceptorFunction, that means there's a bug in their types as well :(
Codecov Report
@@ Coverage Diff @@
## master #362 +/- ##
=======================================
Coverage 94.22% 94.22%
=======================================
Files 3 3
Lines 398 398
Branches 64 64
=======================================
Hits 375 375
Misses 23 23
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
We need to make sure the documentation and this are doing the same thing. If the docs are wrong, let's reach out to those folks and verify first. Also - I would feel soooooo much better if I knew this was being properly covered with an end to end test ( I don't know that it isn't)
| @@ -327,7 +327,7 @@ export class GoogleToken { | |||
| grant_type: 'urn:ietf:params:oauth:grant-type:jwt-bearer', | |||
| assertion: signedJWT, | |||
| }, | |||
| headers: {'Content-Type': 'application/x-www-form-urlencoded'}, | |||
There was a problem hiding this comment.
I am worried this is not correct. I'm reading this doc:
https://developers.google.com/identity/protocols/oauth2/service-account#httprest
It's pretty specific:
POST /token HTTP/1.1
Host: oauth2.googleapis.com
Content-Type: application/x-www-form-urlencoded
grant_type=urn%3Aietf%3Aparams%3Aoauth%3Agrant-type%3Ajwt-bearer&assertion=eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiI3NjEzMjY3OTgwNjktcjVtbGpsbG4xcmQ0bHJiaGc3NWVmZ2lncDM2bTc4ajVAZGV2ZWxvcGVyLmdzZXJ2aWNlYWNjb3VudC5jb20iLCJzY29wZSI6Imh0dHBzOi8vd3d3Lmdvb2dsZWFwaXMuY29tL2F1dGgvcHJlZGljdGlvbiIsImF1ZCI6Imh0dHBzOi8vYWNjb3VudHMuZ29vZ2xlLmNvbS9vL29hdXRoMi90b2tlbiIsImV4cCI6MTMyODU3MzM4MSwiaWF0IjoxMzI4NTY5NzgxfQ.ixOUGehweEVX_UKXv5BbbwVEdcz6AYS-6uQV6fGorGKrHf3LIJnyREw9evE-gs2bmMaQI5_UbabvI4k-mQE4kBqtmSpTzxYBL1TCd7Kv5nTZoUC1CmwmWCFqT9RE6D7XSgPUh_jF1qskLa2w0rxMSjwruNKbysgRNctZPln7cqQ
Do we have an end to end test checking this out?
There was a problem hiding this comment.
If gaxios sees an object it turns it into application/json for content-type, I'm 99% sure that we were never setting the form encoded header.
There was a problem hiding this comment.
Oh I totally believe you. I'm specifically scared that POST-ing JSON happens to work on the endpoint you're using, but that it isn't fully supported.
| @@ -610,16 +610,14 @@ describe('.getToken()', () => { | |||
| }); | |||
|
|
|||
| function createGetTokenMock(code = 200, body?: {}) { | |||
| return nock(GOOGLE_TOKEN_URLS[0]) | |||
| return nock(GOOGLE_TOKEN_URLS[0], { | |||
| reqheaders: {'Content-Type': 'application/json'}, | |||
There was a problem hiding this comment.
I see that the docs do call out that headers are specified in this outer function, but doesn't it have to have some way to define header assertions on a per-route basis? Like it makes sense to be that these could be defined at the top level for the host, but I would expect .get and .post to accept this parameter as well. I am le nervous here. If nock isn't validating headers here, when the types accept Options in InterceptorFunction, that means there's a bug in their types as well :(
This is a long standing bug that prevents us from setting content-type in
gaxios.
Refs: googleapis/google-api-nodejs-client#2003
Refs: googleapis/gaxios#263