fix: process undefined values before creating URLSearchParams (#2029)

Author: miguelvelezsa

src/auth/oauth2client.ts

@@ -23,6 +23,7 @@ import * as querystring from 'querystring';
 import * as stream from 'stream';
 import * as formatEcdsa from 'ecdsa-sig-formatter';
 
+import {removeUndefinedValuesInObject} from '../util';
 import {createCrypto, JwkCertificate, hasBrowserCrypto} from '../crypto/crypto';
 
 import {
@@ -754,7 +755,7 @@ export class OAuth2Client extends AuthClient {
       ...OAuth2Client.RETRY_CONFIG,
       method: 'POST',
       url,
-      data: new URLSearchParams(values as {}),
+      data: new URLSearchParams(removeUndefinedValuesInObject(values) as {}),
       headers,
     };
     AuthClient.setMethodName(opts, 'getTokenAsync');
@@ -820,7 +821,7 @@ export class OAuth2Client extends AuthClient {
         ...OAuth2Client.RETRY_CONFIG,
         method: 'POST',
         url,
-        data: new URLSearchParams(data),
+        data: new URLSearchParams(removeUndefinedValuesInObject(data)),
       };
       AuthClient.setMethodName(opts, 'refreshTokenNoCache');
 

src/auth/stscredentials.ts

@@ -22,6 +22,8 @@ import {
   getErrorFromOAuthErrorResponse,
 } from './oauth2common';
 
+import {removeUndefinedValuesInObject} from '../util';
+
 /**
  * Defines the interface needed to initialize an StsCredentials instance.
  * The interface does not directly map to the spec and instead is converted
@@ -203,20 +205,12 @@ export class StsCredentials extends OAuthClientAuthHandler {
       options: options && JSON.stringify(options),
     };
 
-    // Keep defined fields.
-    const payload: Record<string, string> = {};
-    Object.entries(values).forEach(([key, value]) => {
-      if (value !== undefined) {
-        payload[key] = value;
-      }
-    });
-
     const opts: GaxiosOptions = {
       ...StsCredentials.RETRY_CONFIG,
       url: this.#tokenExchangeEndpoint.toString(),
       method: 'POST',
       headers,
-      data: new URLSearchParams(payload),
+      data: new URLSearchParams(removeUndefinedValuesInObject(values)),
     };
     AuthClient.setMethodName(opts, 'exchangeToken');
 

src/util.ts

@@ -239,3 +239,15 @@ export class LRUCache<T> {
     }
   }
 }
+
+// Given and object remove fields where value is undefined.
+export function removeUndefinedValuesInObject(object: {[key: string]: any}): {
+  [key: string]: any;
+} {
+  Object.entries(object).forEach(([key, value]) => {
+    if (value === undefined || value === 'undefined') {
+      delete object[key];
+    }
+  });
+  return object;
+}

test/test.oauth2.ts

@@ -1370,6 +1370,52 @@ describe('oauth2', () => {
       assert.strictEqual(params.get('code_verifier'), 'its_verified');
     });
 
+    it('getToken should ignore undefined code verifier', async () => {
+      const scope = nock(baseUrl, {
+        reqheaders: {
+          'content-type': 'application/x-www-form-urlencoded;charset=UTF-8',
+        },
+      })
+        .post('/token')
+        .reply(200, {
+          access_token: 'abc',
+          refresh_token: '123',
+          expires_in: 10,
+        });
+      const res = await client.getToken({
+        code: 'code here',
+        codeVerifier: undefined,
+      });
+      scope.done();
+      assert(res.res);
+
+      const params = new URLSearchParams(res.res.config.data || '');
+      assert.strictEqual(params.get('code_verifier'), null);
+    });
+
+    it('getToken should ignore undefined string code verifier', async () => {
+      const scope = nock(baseUrl, {
+        reqheaders: {
+          'content-type': 'application/x-www-form-urlencoded;charset=UTF-8',
+        },
+      })
+        .post('/token')
+        .reply(200, {
+          access_token: 'abc',
+          refresh_token: '123',
+          expires_in: 10,
+        });
+      const res = await client.getToken({
+        code: 'code here',
+        codeVerifier: 'undefined',
+      });
+      scope.done();
+      assert(res.res);
+
+      const params = new URLSearchParams(res.res.config.data || '');
+      assert.strictEqual(params.get('code_verifier'), null);
+    });
+
     it('getToken should set redirect_uri if not provided in options', async () => {
       const scope = nock(baseUrl, {
         reqheaders: {

test/test.util.ts

@@ -15,7 +15,7 @@
 import {strict as assert} from 'assert';
 import * as sinon from 'sinon';
 
-import {LRUCache} from '../src/util';
+import {LRUCache, removeUndefinedValuesInObject} from '../src/util';
 
 describe('util', () => {
   let sandbox: sinon.SinonSandbox;
@@ -81,3 +81,24 @@ describe('util', () => {
     });
   });
 });
+
+describe('util removeUndefinedValuesInObject', () => {
+  it('remove undefined type values in object', () => {
+    const object: {[key: string]: any} = {
+      undefined: undefined,
+      number: 1,
+    };
+    assert.deepEqual(removeUndefinedValuesInObject(object), {
+      number: 1,
+    });
+  });
+  it('remove undefined string values in object', () => {
+    const object: {[key: string]: any} = {
+      undefined: 'undefined',
+      number: 1,
+    };
+    assert.deepEqual(removeUndefinedValuesInObject(object), {
+      number: 1,
+    });
+  });
+});