Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
May 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/perf.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
6 changes: 6 additions & 0 deletions src/logger/filters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) => ({
Expand All @@ -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);
Expand Down
10 changes: 9 additions & 1 deletion src/org/authInfo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
Expand Down
89 changes: 69 additions & 20 deletions test/unit/logger/filterTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
});
});
});
});

Expand Down
Loading