fix: add oauth options data to auth code exchange errors (#1068)

* fix: add redacted oauth options as error data * fix: add redacted oauth options as error data * test: avoid running dev-scripts on perf tests * fix: use logger filtering primarily * refactor: conditional regex w/ custom replacer function * style: non camel-case tests --------- Co-authored-by: mshanemc <[email protected]>
forcedotcom · May 16, 2024 · ea859ed · ea859ed
1 parent 63f120d
commit ea859ed
Show file tree

Hide file tree

Showing 4 changed files with 86 additions and 21 deletions.
diff --git a/.github/workflows/perf.yml b/.github/workflows/perf.yml
@@ -22,6 +22,8 @@ jobs:
         with:
           cache: yarn
       - uses: salesforcecli/github-workflows/.github/actions/yarnInstallWithRetries@main
+        with:
+          ignore-scripts: true
       - run: yarn build
       - run: npm run test:perf | tee test/perf/output.txt
 

diff --git a/src/logger/filters.ts b/src/logger/filters.ts
@@ -35,6 +35,7 @@ const FILTERED_KEYS: FilteredKeyDefinition[] = [
   // Any json attribute that contains the words "refresh" and "token" will have the attribute/value hidden
   { name: 'refresh_token', regex: 'refresh[^\'"]*token' },
   { name: 'clientsecret' },
+  { name: 'authcode' },
 ];
 
 const FILTERED_KEYS_FOR_PROCESSING: FilteredKeyForProcessing[] = FILTERED_KEYS.map((key) => ({
@@ -60,6 +61,11 @@ const replacementFunctions = FILTERED_KEYS_FOR_PROCESSING.flatMap(
     input
       .replace(new RegExp(accessTokenRegex, 'g'), '<REDACTED ACCESS TOKEN>')
       .replace(new RegExp(sfdxAuthUrlRegex, 'g'), '<REDACTED AUTH URL TOKEN>'),
+  // conditional replacement for clientId: leave the value if it's the Platform CLI, otherwise redact it
+  (input: string): string =>
+    input.replace(/(['"]client.*Id['"])\s*:\s*(['"][^'"]*['"])/gi, (all, key: string, value: string) =>
+      value.includes('Platform CLI') ? `${key}:${value}` : `${key}:"<REDACTED CLIENT ID>"`
+    ),
 ]);
 
 const fullReplacementChain = compose(...replacementFunctions);

diff --git a/src/org/authInfo.ts b/src/org/authInfo.ts
@@ -35,6 +35,7 @@ import { Logger } from '../logger/logger';
 import { SfError } from '../sfError';
 import { matchesAccessToken, trimTo15 } from '../util/sfdc';
 import { StateAggregator } from '../stateAggregator/stateAggregator';
+import { filterSecrets } from '../logger/filters';
 import { Messages } from '../messages';
 import { getLoginAudienceCombos, SfdcUrl } from '../util/sfdcUrl';
 import { Connection, SFDX_HTTP_HEADERS } from './connection';
@@ -1106,7 +1107,14 @@ export class AuthInfo extends AsyncOptionalCreatable<AuthInfo.Options> {
       this.logger.info(`Exchanging auth code for access token using loginUrl: ${options.loginUrl}`);
       authFields = await oauth2.requestToken(ensure(options.authCode));
     } catch (err) {
-      throw messages.createError('authCodeExchangeError', [(err as Error).message]);
+      const msg = err instanceof Error ? `${err.name}::${err.message}` : typeof err === 'string' ? err : 'UNKNOWN';
+      const redacted = filterSecrets(options);
+      throw SfError.create({
+        message: messages.getMessage('authCodeExchangeError', [msg]),
+        name: 'AuthCodeExchangeError',
+        ...(err instanceof Error ? { cause: err } : {}),
+        data: (isArray(redacted) ? redacted[0] : redacted) as JwtOAuth2Config,
+      });
     }
 
     const { orgId } = parseIdUrl(authFields.id);

diff --git a/test/unit/logger/filterTest.ts b/test/unit/logger/filterTest.ts
@@ -17,18 +17,6 @@ describe('filters', () => {
     // eslint-disable-next-line camelcase
     access_token: sid,
   })}`;
-  const obj1 = { accessToken: `${sid}`, refreshToken: `${sid}` };
-  const obj2 = { key: 'Access Token', value: `${sid}` };
-  const arr1 = [
-    { key: 'ACCESS token ', value: `${sid}` },
-    { key: 'refresh  TOKEN', value: `${sid}` },
-    { key: 'Sfdx Auth Url', value: `${sid}` },
-  ];
-  const arr2 = [
-    { key: ' AcCESS 78token', value: ` ${sid} ` },
-    { key: ' refresh  _TOKEn ', value: ` ${sid} ` },
-    { key: ' SfdX__AuthUrl  ', value: ` ${sid} ` },
-  ];
 
   it(`filters ${simpleString} correctly`, () => {
     const result = getUnwrapped(simpleString);
@@ -42,25 +30,64 @@ describe('filters', () => {
     expect(result).to.contain('REDACTED ACCESS TOKEN');
   });
 
-  it('filters obj1 correctly', () => {
-    const result = getUnwrapped(obj1);
+  it('filters regular object correctly', () => {
+    const result = getUnwrapped({ accessToken: `${sid}`, refreshToken: `${sid}` });
     assert(result);
     const bigString = JSON.stringify(result);
     expect(bigString).to.not.contain(sid);
     expect(bigString).to.contain('REDACTED ACCESS TOKEN');
     expect(bigString).to.contain('refresh_token - HIDDEN');
   });
 
-  it('filters obj2 correctly', () => {
-    const result = getUnwrapped(obj2);
+  it('filters key/value object correctly', () => {
+    const result = getUnwrapped({ key: 'Access Token', value: `${sid}` });
     assert(result);
     const bigString = JSON.stringify(result);
     expect(bigString).to.not.contain(sid);
     expect(bigString).to.contain('REDACTED ACCESS TOKEN');
   });
 
-  it('filters arr1 correctly', () => {
-    const result = getUnwrapped(arr1);
+  it('filters auth code correctly', () => {
+    const result = getUnwrapped({ authCode: 'authcode value' });
+    assert(result);
+    const bigString = JSON.stringify(result);
+    expect(bigString).to.not.contain('authCode value');
+    expect(bigString).to.contain('authcode - HIDDEN');
+  });
+
+  describe('client id', () => {
+    it('filters clientId correctly', () => {
+      const result = getUnwrapped({ clientId: 'clientIdValue' });
+      assert(result);
+      const bigString = JSON.stringify(result);
+      expect(bigString).to.not.contain('clientIdValue');
+      expect(bigString).to.contain('REDACTED CLIENT ID');
+    });
+
+    it('filters clientId correctly (case insensitive)', () => {
+      const result = getUnwrapped({ ClientId: 'clientIdValue' });
+      assert(result);
+      const bigString = JSON.stringify(result);
+      expect(bigString).to.not.contain('clientIdValue');
+      expect(bigString).to.contain('REDACTED CLIENT ID');
+    });
+
+    it('filters clientId correctly (separator)', () => {
+      // eslint-disable-next-line camelcase
+      const result = getUnwrapped({ Client_Id: 'clientIdValue' });
+      assert(result);
+      const bigString = JSON.stringify(result);
+      expect(bigString).to.not.contain('clientIdValue');
+      expect(bigString).to.contain('REDACTED CLIENT ID');
+    });
+  });
+
+  it('filters array correctly', () => {
+    const result = getUnwrapped([
+      { key: 'ACCESS token ', value: `${sid}` },
+      { key: 'refresh  TOKEN', value: `${sid}` },
+      { key: 'Sfdx Auth Url', value: `${sid}` },
+    ]);
     assert(result);
     assert(Array.isArray(result));
     const bigString = JSON.stringify(result);
@@ -70,8 +97,12 @@ describe('filters', () => {
     expect(bigString).to.contain('refresh_token - HIDDEN');
   });
 
-  it('filters arr2 correctly', () => {
-    const result = getUnwrapped(arr2);
+  it('filters another array correctly', () => {
+    const result = getUnwrapped([
+      { key: ' AcCESS 78token', value: ` ${sid} ` },
+      { key: ' refresh  _TOKEn ', value: ` ${sid} ` },
+      { key: ' SfdX__AuthUrl  ', value: ` ${sid} ` },
+    ]);
     assert(result);
     assert(Array.isArray(result));
     const bigString = JSON.stringify(result);
@@ -101,6 +132,24 @@ describe('filters', () => {
       expect(result).to.have.property('foo', 'bar');
       expect(result).to.have.property('accessToken').contains('REDACTED ACCESS TOKEN');
     });
+    describe('clientId', () => {
+      it('default connected app', () => {
+        const input = { clientId: 'Platform CLI' };
+        const result = getUnwrapped(input);
+        expect(result).to.deep.equal(input);
+      });
+      it('default connected app (case insensitive)', () => {
+        const input = { ClientID: 'Platform CLI' };
+        const result = getUnwrapped(input);
+        expect(result).to.deep.equal(input);
+      });
+      it('default connected app (case insensitive)', () => {
+        // eslint-disable-next-line camelcase
+        const input = { client_id: 'Platform CLI' };
+        const result = getUnwrapped(input);
+        expect(result).to.deep.equal(input);
+      });
+    });
   });
 });