From 3d196292396588366c9ac9f347f3c2cd6167a765 Mon Sep 17 00:00:00 2001 From: Steve Hetzel Date: Tue, 14 May 2024 11:07:27 -0600 Subject: [PATCH 1/6] fix: add redacted oauth options as error data --- src/org/authInfo.ts | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/org/authInfo.ts b/src/org/authInfo.ts index 15f29537e..2ebf4bba2 100644 --- a/src/org/authInfo.ts +++ b/src/org/authInfo.ts @@ -1106,7 +1106,10 @@ export class AuthInfo extends AsyncOptionalCreatable { 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 error = SfError.wrap(err); + error.message = messages.getMessage('authCodeExchangeError', [error.message]); + error.setData(getRedactedErrData(options)); + throw error; } const { orgId } = parseIdUrl(authFields.id); @@ -1242,6 +1245,21 @@ export class AuthInfo extends AsyncOptionalCreatable { } } +const getRedactedErrData = (options: JwtOAuth2Config): AnyJson => { + const keysToRedact = ['privateKey', 'privateKeyFile', 'authCode', 'refreshToken', 'username', 'clientSecret']; + const oauth2OptionsKeys = Object.getOwnPropertyNames(options); + return oauth2OptionsKeys.map((k) => { + if (keysToRedact.includes(k)) { + // @ts-expect-error no index signature with a parameter of type 'string' was found on type JwtOAuth2Config + return options[k] ? `${k}:''` : `${k}:''`; + } else if (k === 'clientId') { + return options[k] === 'PlatformCLI' ? `${k}:${options[k]}` : `${k}:''`; + } + // @ts-expect-error no index signature with a parameter of type 'string' was found on type JwtOAuth2Config + return `${k}:${options[k]}`; + }); +}; + export namespace AuthInfo { /** * Constructor options for AuthInfo. From 6aba25dbc06e9d7d573f586a938af0aa5f21464f Mon Sep 17 00:00:00 2001 From: Steve Hetzel Date: Tue, 14 May 2024 11:28:25 -0600 Subject: [PATCH 2/6] fix: add redacted oauth options as error data --- src/org/authInfo.ts | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/org/authInfo.ts b/src/org/authInfo.ts index 2ebf4bba2..c3c136582 100644 --- a/src/org/authInfo.ts +++ b/src/org/authInfo.ts @@ -1106,8 +1106,20 @@ export class AuthInfo extends AsyncOptionalCreatable { this.logger.info(`Exchanging auth code for access token using loginUrl: ${options.loginUrl}`); authFields = await oauth2.requestToken(ensure(options.authCode)); } catch (err) { - const error = SfError.wrap(err); - error.message = messages.getMessage('authCodeExchangeError', [error.message]); + let error: SfError; + let errorMsg: string; + if (err instanceof Error) { + errorMsg = `${err.name}::${err.message}`; + error = SfError.create({ + message: errorMsg, + name: 'AuthCodeExchangeError', + cause: err, + }); + } else { + error = SfError.wrap(err); + errorMsg = error.message; + } + error.message = messages.getMessage('authCodeExchangeError', [errorMsg]); error.setData(getRedactedErrData(options)); throw error; } From bd304079e68a79a2f62e2d7a1f68421aae51e645 Mon Sep 17 00:00:00 2001 From: mshanemc Date: Tue, 14 May 2024 18:28:06 -0500 Subject: [PATCH 3/6] test: avoid running dev-scripts on perf tests --- .github/workflows/perf.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/perf.yml b/.github/workflows/perf.yml index f6f6ddf7f..4e5911400 100644 --- 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 From 0ea16f088fc91f600805efa5f053d5e1c55bbb45 Mon Sep 17 00:00:00 2001 From: Steve Hetzel Date: Wed, 15 May 2024 10:40:22 -0600 Subject: [PATCH 4/6] fix: use logger filtering primarily --- src/logger/filters.ts | 1 + src/org/authInfo.ts | 44 +++++++++++++--------------------- test/unit/logger/filterTest.ts | 9 +++++++ 3 files changed, 26 insertions(+), 28 deletions(-) diff --git a/src/logger/filters.ts b/src/logger/filters.ts index 1bace000b..06ef49dee 100644 --- 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) => ({ diff --git a/src/org/authInfo.ts b/src/org/authInfo.ts index c3c136582..3a968645b 100644 --- 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,22 +1107,13 @@ export class AuthInfo extends AsyncOptionalCreatable { this.logger.info(`Exchanging auth code for access token using loginUrl: ${options.loginUrl}`); authFields = await oauth2.requestToken(ensure(options.authCode)); } catch (err) { - let error: SfError; - let errorMsg: string; - if (err instanceof Error) { - errorMsg = `${err.name}::${err.message}`; - error = SfError.create({ - message: errorMsg, - name: 'AuthCodeExchangeError', - cause: err, - }); - } else { - error = SfError.wrap(err); - errorMsg = error.message; - } - error.message = messages.getMessage('authCodeExchangeError', [errorMsg]); - error.setData(getRedactedErrData(options)); - throw error; + const msg = err instanceof Error ? `${err.name}::${err.message}` : typeof err === 'string' ? err : 'UNKNOWN'; + throw SfError.create({ + message: messages.getMessage('authCodeExchangeError', [msg]), + name: 'AuthCodeExchangeError', + ...(err instanceof Error ? { cause: err } : {}), + data: getRedactedErrData(options), + }); } const { orgId } = parseIdUrl(authFields.id); @@ -1258,18 +1250,14 @@ export class AuthInfo extends AsyncOptionalCreatable { } const getRedactedErrData = (options: JwtOAuth2Config): AnyJson => { - const keysToRedact = ['privateKey', 'privateKeyFile', 'authCode', 'refreshToken', 'username', 'clientSecret']; - const oauth2OptionsKeys = Object.getOwnPropertyNames(options); - return oauth2OptionsKeys.map((k) => { - if (keysToRedact.includes(k)) { - // @ts-expect-error no index signature with a parameter of type 'string' was found on type JwtOAuth2Config - return options[k] ? `${k}:''` : `${k}:''`; - } else if (k === 'clientId') { - return options[k] === 'PlatformCLI' ? `${k}:${options[k]}` : `${k}:''`; - } - // @ts-expect-error no index signature with a parameter of type 'string' was found on type JwtOAuth2Config - return `${k}:${options[k]}`; - }); + const filteredData = filterSecrets(options) as JwtOAuth2Config; + // we need an object but it probably returned an array + const fData = (isArray(filteredData) ? filteredData[0] : filteredData) as JwtOAuth2Config; + + if (fData?.clientId?.trim() !== 'PlatformCLI') { + fData.clientId = ''; + } + return fData; }; export namespace AuthInfo { diff --git a/test/unit/logger/filterTest.ts b/test/unit/logger/filterTest.ts index 7914dba22..638f0c5f3 100644 --- a/test/unit/logger/filterTest.ts +++ b/test/unit/logger/filterTest.ts @@ -19,6 +19,7 @@ describe('filters', () => { })}`; const obj1 = { accessToken: `${sid}`, refreshToken: `${sid}` }; const obj2 = { key: 'Access Token', value: `${sid}` }; + const obj3 = { authCode: 'authcode value' }; const arr1 = [ { key: 'ACCESS token ', value: `${sid}` }, { key: 'refresh TOKEN', value: `${sid}` }, @@ -59,6 +60,14 @@ describe('filters', () => { expect(bigString).to.contain('REDACTED ACCESS TOKEN'); }); + it('filters obj3 correctly', () => { + const result = getUnwrapped(obj3); + assert(result); + const bigString = JSON.stringify(result); + expect(bigString).to.not.contain('authCode value'); + expect(bigString).to.contain('authcode - HIDDEN'); + }); + it('filters arr1 correctly', () => { const result = getUnwrapped(arr1); assert(result); From 72288ab5640e0d38e971a91f8dace30762277c2b Mon Sep 17 00:00:00 2001 From: mshanemc Date: Wed, 15 May 2024 22:52:55 -0500 Subject: [PATCH 5/6] refactor: conditional regex w/ custom replacer function --- src/logger/filters.ts | 5 ++ src/org/authInfo.ts | 14 +----- test/unit/logger/filterTest.ts | 84 ++++++++++++++++++++++++---------- 3 files changed, 68 insertions(+), 35 deletions(-) diff --git a/src/logger/filters.ts b/src/logger/filters.ts index 06ef49dee..0b4d8013f 100644 --- a/src/logger/filters.ts +++ b/src/logger/filters.ts @@ -61,6 +61,11 @@ const replacementFunctions = FILTERED_KEYS_FOR_PROCESSING.flatMap( input .replace(new RegExp(accessTokenRegex, 'g'), '') .replace(new RegExp(sfdxAuthUrlRegex, 'g'), ''), + // 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}:""` + ), ]); const fullReplacementChain = compose(...replacementFunctions); diff --git a/src/org/authInfo.ts b/src/org/authInfo.ts index 3a968645b..a70f67069 100644 --- a/src/org/authInfo.ts +++ b/src/org/authInfo.ts @@ -1108,11 +1108,12 @@ export class AuthInfo extends AsyncOptionalCreatable { authFields = await oauth2.requestToken(ensure(options.authCode)); } catch (err) { 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: getRedactedErrData(options), + data: (isArray(redacted) ? redacted[0] : redacted) as JwtOAuth2Config, }); } @@ -1249,17 +1250,6 @@ export class AuthInfo extends AsyncOptionalCreatable { } } -const getRedactedErrData = (options: JwtOAuth2Config): AnyJson => { - const filteredData = filterSecrets(options) as JwtOAuth2Config; - // we need an object but it probably returned an array - const fData = (isArray(filteredData) ? filteredData[0] : filteredData) as JwtOAuth2Config; - - if (fData?.clientId?.trim() !== 'PlatformCLI') { - fData.clientId = ''; - } - return fData; -}; - export namespace AuthInfo { /** * Constructor options for AuthInfo. diff --git a/test/unit/logger/filterTest.ts b/test/unit/logger/filterTest.ts index 638f0c5f3..de8e68fd7 100644 --- a/test/unit/logger/filterTest.ts +++ b/test/unit/logger/filterTest.ts @@ -17,19 +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 obj3 = { authCode: 'authcode value' }; - 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); @@ -43,8 +30,8 @@ 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); @@ -52,24 +39,54 @@ describe('filters', () => { 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 obj3 correctly', () => { - const result = getUnwrapped(obj3); + 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'); }); - it('filters arr1 correctly', () => { - const result = getUnwrapped(arr1); + 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)', () => { + 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); @@ -79,8 +96,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); @@ -110,6 +131,23 @@ 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)', () => { + const input = { client_id: 'Platform CLI' }; + const result = getUnwrapped(input); + expect(result).to.deep.equal(input); + }); + }); }); }); From 44fce1d8b96b115239962b37d748098368d233ec Mon Sep 17 00:00:00 2001 From: mshanemc Date: Wed, 15 May 2024 22:54:02 -0500 Subject: [PATCH 6/6] style: non camel-case tests --- test/unit/logger/filterTest.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/unit/logger/filterTest.ts b/test/unit/logger/filterTest.ts index de8e68fd7..b39ffd21b 100644 --- a/test/unit/logger/filterTest.ts +++ b/test/unit/logger/filterTest.ts @@ -73,6 +73,7 @@ describe('filters', () => { }); it('filters clientId correctly (separator)', () => { + // eslint-disable-next-line camelcase const result = getUnwrapped({ Client_Id: 'clientIdValue' }); assert(result); const bigString = JSON.stringify(result); @@ -143,6 +144,7 @@ describe('filters', () => { 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);