Skip to content

Commit

Permalink
fix: prevent validations on encrypted values
Browse files Browse the repository at this point in the history
  • Loading branch information
mdonnalley committed Jun 20, 2024
1 parent 80fde91 commit ecbe83d
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 15 deletions.
37 changes: 32 additions & 5 deletions src/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,16 @@ export class Config extends ConfigFile<ConfigFile.Options, ConfigProperties> {
return keyBy(Config.allowedProperties, 'key');
}

private static findProperty(key: string): ConfigPropertyMeta {
const property = Config.allowedProperties.find((allowedProp) => allowedProp.key === key);

if (!property) {
throw messages.createError('unknownConfigKey', [key]);
}

return property;
}

/**
* Read, assign, and return the config contents.
*/
Expand Down Expand Up @@ -493,11 +503,8 @@ export class Config extends ConfigFile<ConfigFile.Options, ConfigProperties> {
* @param value The value of the property.
*/
public set<K extends Key<ConfigProperties>>(key: K, value: ConfigProperties[K]): ConfigProperties {
const property = Config.allowedProperties.find((allowedProp) => allowedProp.key === key);
const property = Config.findProperty(key);

if (!property) {
throw messages.createError('unknownConfigKey', [key]);
}
if (property.deprecated && property.newKey) {
// you're trying to set a deprecated key, but we'll set the new key instead
void Lifecycle.getInstance().emitWarning(messages.getMessage('deprecatedConfigKey', [key, property.newKey]));
Expand Down Expand Up @@ -590,11 +597,31 @@ export class Config extends ConfigFile<ConfigFile.Options, ConfigProperties> {

this.forEach((key, value) => {
if (this.getPropertyConfig(key).encrypted && isString(value)) {
this.set(key, ensure(encrypt ? crypto.encrypt(value) : crypto.decrypt(value)));
if (encrypt) {
this.setEncryptedProperty(key, ensure(crypto.encrypt(value)));
} else {
this.set(key, ensure(crypto.decrypt(value)));
}
}
});
}
}

/**
* Set an encrypted property without rerunning the validator. Should only be used by `cryptProperties` method.
*/
private setEncryptedProperty<K extends Key<ConfigProperties>>(key: K, value: ConfigProperties[K]): ConfigProperties {
const property = Config.findProperty(key);

if (property.deprecated && property.newKey) {
// you're trying to set a deprecated key, but we'll set the new key instead
void Lifecycle.getInstance().emitWarning(messages.getMessage('deprecatedConfigKey', [key, property.newKey]));
return this.set(property.newKey, value);
}

super.set(property.key, value);
return this.getContents();
}
}

/**
Expand Down
64 changes: 54 additions & 10 deletions test/unit/config/configTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -260,12 +260,10 @@ describe('Config', () => {
it('calls ConfigFile.write with encrypted values contents', async () => {
const TEST_VAL = 'test';

const writeStub = stubMethod($$.SANDBOX, ConfigFile.prototype, ConfigFile.prototype.write.name).callsFake(
async function (this: Config) {
expect(ensureString(this.get('org-isv-debugger-sid')).length).to.be.greaterThan(TEST_VAL.length);
expect(ensureString(this.get('org-isv-debugger-sid'))).to.not.equal(TEST_VAL);
}
);
const writeStub = stubMethod($$.SANDBOX, ConfigFile.prototype, 'write').callsFake(async function (this: Config) {
expect(ensureString(this.get('org-isv-debugger-sid')).length).to.be.greaterThan(TEST_VAL.length);
expect(ensureString(this.get('org-isv-debugger-sid'))).to.not.equal(TEST_VAL);
});

const config = await Config.create(Config.getDefaultOptions(true));
config.set(OrgConfigProperties.ORG_ISV_DEBUGGER_SID, TEST_VAL);
Expand All @@ -275,8 +273,8 @@ describe('Config', () => {
});

it('calls ConfigFile.read with unknown key and does not throw on crypt', async () => {
stubMethod($$.SANDBOX, ConfigFile.prototype, ConfigFile.prototype.readSync.name).callsFake(async () => {});
stubMethod($$.SANDBOX, ConfigFile.prototype, ConfigFile.prototype.read.name).callsFake(async function () {
stubMethod($$.SANDBOX, ConfigFile.prototype, 'readSync').callsFake(async () => {});
stubMethod($$.SANDBOX, ConfigFile.prototype, 'read').callsFake(async function () {
// @ts-expect-error -> this is any
// eslint-disable-next-line @typescript-eslint/no-unsafe-call
this.setContentsFromFileContents({ unknown: 'unknown config key and value' });
Expand All @@ -285,18 +283,64 @@ describe('Config', () => {
const config = await Config.create({ isGlobal: true });
expect(config).to.exist;
});

describe('set', () => {
let originalAllowedProperties: ConfigPropertyMeta[];

beforeEach(() => {
// @ts-expect-error because allowedProperties is protected
originalAllowedProperties = Config.allowedProperties;
(Config as any).allowedProperties = [];
});

afterEach(() => {
// @ts-expect-error because allowedProperties is protected
Config.allowedProperties = originalAllowedProperties;
});

it('does not rerun validation when setting an encrypted value', async () => {
const validationSpy = $$.SANDBOX.stub().callsFake(
(value) => typeof value === 'string' && value.startsWith('123')
);
Config.addAllowedProperties([
{
key: 'encrypted-token',
description: 'encrypted token',
encrypted: true,
input: {
validator: validationSpy,
failedMessage: 'Token must start with 123',
},
},
]);
const config = await Config.create({ ...Config.getDefaultOptions(true), encryptedKeys: ['encrypted-token'] });
try {
config.set('encrypted-token', '123456');
// config.write will call cryptProperties(true). It's easier to call it directly than to stub the file system operations
// If it doesn't throw, then the validation was not rerun on the encrypted value
// @ts-expect-error because cryptProperties is private
await config.cryptProperties(true);
expect(validationSpy.calledOnce).to.be.true;
expect(config.get('encrypted-token')).to.be.ok;
} catch (err) {
expect(err, 'No error should have been thrown').to.be.undefined;
}
});
});
});

describe('allowed properties', () => {
let originalAllowedProperties: ConfigPropertyMeta[];

beforeEach(() => {
originalAllowedProperties = (Config as any).allowedProperties;
// @ts-expect-error because allowedProperties is protected
originalAllowedProperties = Config.allowedProperties;
(Config as any).allowedProperties = [];
});

afterEach(() => {
(Config as any).allowedProperties = originalAllowedProperties;
// @ts-expect-error because allowedProperties is protected
Config.allowedProperties = originalAllowedProperties;
});

it('has default properties assigned', () => {
Expand Down

3 comments on commit ecbe83d

@svc-cli-bot
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logger Benchmarks - ubuntu-latest

Benchmark suite Current: ecbe83d Previous: bd03190 Ratio
Child logger creation 479019 ops/sec (±0.52%) 470560 ops/sec (±1.34%) 0.98
Logging a string on root logger 769643 ops/sec (±9.76%) 804441 ops/sec (±7.49%) 1.05
Logging an object on root logger 586455 ops/sec (±7.80%) 617201 ops/sec (±6.29%) 1.05
Logging an object with a message on root logger 6342 ops/sec (±208.99%) 3822 ops/sec (±220.28%) 0.60
Logging an object with a redacted prop on root logger 442955 ops/sec (±5.90%) 423920 ops/sec (±9.46%) 0.96
Logging a nested 3-level object on root logger 374718 ops/sec (±8.41%) 365283 ops/sec (±8.22%) 0.97

This comment was automatically generated by workflow using github-action-benchmark.

@svc-cli-bot
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logger Benchmarks - windows-latest

Benchmark suite Current: ecbe83d Previous: bd03190 Ratio
Child logger creation 351661 ops/sec (±0.33%) 333243 ops/sec (±0.68%) 0.95
Logging a string on root logger 823379 ops/sec (±5.40%) 771857 ops/sec (±5.37%) 0.94
Logging an object on root logger 644119 ops/sec (±6.45%) 582351 ops/sec (±8.84%) 0.90
Logging an object with a message on root logger 1960 ops/sec (±243.21%) 5853 ops/sec (±212.30%) 2.99
Logging an object with a redacted prop on root logger 476110 ops/sec (±6.50%) 465146 ops/sec (±9.40%) 0.98
Logging a nested 3-level object on root logger 324421 ops/sec (±6.99%) 333364 ops/sec (±3.98%) 1.03

This comment was automatically generated by workflow using github-action-benchmark.

@svc-cli-bot
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Logger Benchmarks - windows-latest'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: ecbe83d Previous: bd03190 Ratio
Logging an object with a message on root logger 1960 ops/sec (±243.21%) 5853 ops/sec (±212.30%) 2.99

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.