From f78d73dadbb6f464995d2cadadd421fd80dc178b Mon Sep 17 00:00:00 2001 From: Colin Grant Date: Wed, 1 Feb 2023 12:49:18 -0700 Subject: [PATCH 1/4] Allow plugins to access language overrides with bracket syntax --- .../src/plugin/preference-registry.spec.ts | 71 ++++++----------- .../src/plugin/preference-registry.ts | 78 ++++++++----------- 2 files changed, 58 insertions(+), 91 deletions(-) diff --git a/packages/plugin-ext/src/plugin/preference-registry.spec.ts b/packages/plugin-ext/src/plugin/preference-registry.spec.ts index 1218a30efcfec..c8c951caf0d22 100644 --- a/packages/plugin-ext/src/plugin/preference-registry.spec.ts +++ b/packages/plugin-ext/src/plugin/preference-registry.spec.ts @@ -23,7 +23,7 @@ import { URI } from './types-impl'; const expect = chai.expect; /* eslint-disable @typescript-eslint/no-explicit-any */ -describe('PreferenceRegistryExtImpl:', () => { +describe.only('PreferenceRegistryExtImpl:', () => { const workspaceRoot = URI.parse('/workspace-root'); let preferenceRegistryExtImpl: PreferenceRegistryExtImpl; const getProxy = (proxyId: ProxyIdentifier) => { }; @@ -41,36 +41,6 @@ describe('PreferenceRegistryExtImpl:', () => { preferenceRegistryExtImpl = new PreferenceRegistryExtImpl(mockRPC, mockWorkspace); }); - it('should parse configuration data without overrides', () => { - const value: Record = { - 'my.key1.foo': 'value1', - 'my.key1.bar': 'value2', - }; - const result = preferenceRegistryExtImpl['parseConfigurationData'](value); - expect(result.contents.my).to.be.an('object'); - expect(result.contents.my.key1).to.be.an('object'); - - expect(result.contents.my.key1.foo).to.be.an('string'); - expect(result.contents.my.key1.foo).to.equal('value1'); - - expect(result.contents.my.key1.bar).to.be.an('string'); - expect(result.contents.my.key1.bar).to.equal('value2'); - expect(result.keys).deep.equal(['my.key1.foo', 'my.key1.bar']); - }); - - it('should parse configuration with overrides', () => { - const value: Record = { - 'editor.tabSize': 2, - '[typescript].editor.tabSize': 4, - }; - const result = preferenceRegistryExtImpl['parseConfigurationData'](value); - expect(result.contents.editor.tabSize).to.equal(2); - const tsOverride = result.overrides[0]; - expect(tsOverride.contents.editor.tabSize).to.equal(4); - expect(tsOverride.identifiers).deep.equal(['typescript']); - expect(tsOverride.keys).deep.equal(['editor.tabSize']); - }); - describe('Prototype pollution', () => { it('Ignores key `__proto__`', () => { const value: Record = { @@ -80,16 +50,18 @@ describe('PreferenceRegistryExtImpl:', () => { '__proto__': {}, '[typescript].someKey.foo': 'value', '[typescript].__proto__.injectedParsedPrototype': true, + 'b': { '__proto__.injectedParsedPrototype': true }, + 'c': { '__proto__': { 'injectedParsedPrototype': true } } }; - const result = preferenceRegistryExtImpl['parseConfigurationData'](value); - expect(result.contents.my).to.be.an('object'); - expect(result.contents.__proto__).to.be.an('undefined'); - expect(result.contents.my.key1.foo).to.equal('value1'); - expect(result.overrides[0].contents.__proto__).to.be.an('undefined'); + const configuration = preferenceRegistryExtImpl['getConfigurationModel']('test', value); + const result = configuration['_contents']; + expect(result.my, 'Safe keys are preserved.').to.be.an('object'); + expect(result.__proto__, 'Keys containing __proto__ are ignored').to.be.an('undefined'); + expect(result.my.key1.foo, 'Safe keys are dendrified.').to.equal('value1'); const prototypeObject = Object.prototype as any; - expect(prototypeObject.injectedParsedPrototype).to.be.an('undefined'); + expect(prototypeObject.injectedParsedPrototype, 'Object.prototype is unaffected').to.be.an('undefined'); const rawObject = {} as any; - expect(rawObject.injectedParsedPrototype).to.be.an('undefined'); + expect(rawObject.injectedParsedPrototype, 'Instantiated objects are unaffected.').to.be.an('undefined'); }); it('Ignores key `constructor.prototype`', () => { @@ -98,17 +70,19 @@ describe('PreferenceRegistryExtImpl:', () => { 'a.constructor.prototype.injectedParsedConstructorPrototype': true, 'constructor.prototype.injectedParsedConstructorPrototype': true, '[python].some.key.foo': 'value', - '[python].a.constructor.prototype.injectedParsedConstructorPrototype': true + '[python].a.constructor.prototype.injectedParsedConstructorPrototype': true, + 'constructor': { 'prototype.injectedParsedConstructorPrototype': true }, + 'b': { 'constructor': { 'prototype': { 'injectedParsedConstructorPrototype': true } } } }; - const result = preferenceRegistryExtImpl['parseConfigurationData'](value); - expect(result.contents.my).to.be.an('object'); - expect(result.contents.__proto__).to.be.an('undefined'); - expect(result.contents.my.key1.foo).to.equal('value1'); + const configuration = preferenceRegistryExtImpl['getConfigurationModel']('test', value); + const result = configuration['_contents']; + expect(result.my, 'Safe keys are preserved').to.be.an('object'); + expect(result.__proto__, 'Keys containing __proto__ are ignored').to.be.an('undefined'); + expect(result.my.key1.foo, 'Safe keys are dendrified.').to.equal('value1'); const prototypeObject = Object.prototype as any; - expect(prototypeObject.injectedParsedConstructorPrototype).to.be.an('undefined'); - expect(result.overrides[0].contents.__proto__).to.be.an('undefined'); + expect(prototypeObject.injectedParsedConstructorPrototype, 'Object.prototype is unaffected').to.be.an('undefined'); const rawObject = {} as any; - expect(rawObject.injectedParsedConstructorPrototype).to.be.an('undefined'); + expect(rawObject.injectedParsedConstructorPrototype, 'Instantiated objects are unaffected.').to.be.an('undefined'); }); }); @@ -250,6 +224,11 @@ describe('PreferenceRegistryExtImpl:', () => { const valuesRetrieved = preferenceRegistryExtImpl.getConfiguration(undefined, { uri: workspaceRoot, languageId: 'python' }).get('editor') as Record; expect(valuesRetrieved.tabSize).equal(4); }); + it('Allows access to language overrides in bracket form', () => { + const pythonOverrides = preferenceRegistryExtImpl.getConfiguration().get>('[python]'); + expect(pythonOverrides).not.to.be.undefined; + expect(pythonOverrides?.['editor.renderWhitespace']).equal('all'); + }); }); describe('Proxy Behavior', () => { diff --git a/packages/plugin-ext/src/plugin/preference-registry.ts b/packages/plugin-ext/src/plugin/preference-registry.ts index 63cc7efa4ecb9..332670c65c1fe 100644 --- a/packages/plugin-ext/src/plugin/preference-registry.ts +++ b/packages/plugin-ext/src/plugin/preference-registry.ts @@ -20,8 +20,8 @@ import { Emitter, Event } from '@theia/core/lib/common/event'; import { isOSX, isWindows } from '@theia/core/lib/common/os'; import { URI } from '@theia/core/shared/vscode-uri'; import { ResourceMap } from '@theia/monaco-editor-core/esm/vs/base/common/map'; -import { IConfigurationOverrides, IOverrides } from '@theia/monaco-editor-core/esm/vs/platform/configuration/common/configuration'; -import { Configuration, ConfigurationModel } from '@theia/monaco-editor-core/esm/vs/platform/configuration/common/configurationModels'; +import { IConfigurationOverrides } from '@theia/monaco-editor-core/esm/vs/platform/configuration/common/configuration'; +import { Configuration, ConfigurationModel, ConfigurationModelParser } from '@theia/monaco-editor-core/esm/vs/platform/configuration/common/configurationModels'; import { Workspace, WorkspaceFolder } from '@theia/monaco-editor-core/esm/vs/platform/workspace/common/workspace'; import * as theia from '@theia/plugin'; import { v4 } from 'uuid'; @@ -234,12 +234,12 @@ export class PreferenceRegistryExtImpl implements PreferenceRegistryExt { } private parse(data: PreferenceData): Configuration { - const defaultConfiguration = this.getConfigurationModel(data[PreferenceScope.Default]); - const userConfiguration = this.getConfigurationModel(data[PreferenceScope.User]); - const workspaceConfiguration = this.getConfigurationModel(data[PreferenceScope.Workspace]); + const defaultConfiguration = this.getConfigurationModel('Default', data[PreferenceScope.Default]); + const userConfiguration = this.getConfigurationModel('User', data[PreferenceScope.User]); + const workspaceConfiguration = this.getConfigurationModel('Workspace', data[PreferenceScope.Workspace]); const folderConfigurations = new ResourceMap(); Object.keys(data[PreferenceScope.Folder]).forEach(resource => { - folderConfigurations.set(URI.parse(resource), this.getConfigurationModel(data[PreferenceScope.Folder][resource])); + folderConfigurations.set(URI.parse(resource), this.getConfigurationModel(`Folder: ${resource}`, data[PreferenceScope.Folder][resource])); }); return new Configuration( defaultConfiguration, @@ -252,53 +252,41 @@ export class PreferenceRegistryExtImpl implements PreferenceRegistryExt { ); } - private getConfigurationModel(data: { [key: string]: any }): ConfigurationModel { - if (!data) { - return new ConfigurationModel(); - } - const configData = this.parseConfigurationData(data); - return new ConfigurationModel(configData.contents, configData.keys, configData.overrides); + private getConfigurationModel(label: string, data: { [key: string]: any }): ConfigurationModel { + const parser = new ConfigurationModelParser(label); + const sanitized = this.sanitize(data); + parser.parseRaw(sanitized); + return parser.configurationModel; } - private readonly OVERRIDE_PROPERTY = '^\\[(.*)\\]$'; - private readonly OVERRIDE_PROPERTY_PATTERN = new RegExp(this.OVERRIDE_PROPERTY); - private readonly OVERRIDE_KEY_TEST = /^\[([^\]]+)\]\./; - - private parseConfigurationData(data: { [key: string]: any }): Omit & { overrides: IOverrides[] } { - const keys = new Array(); - const overrides: Record = Object.create(null); - const contents = Object.keys(data).reduce((result: any, key: string) => { - if (injectionRe.test(key)) { - return result; - } - const parts = key.split('.'); - let branch = result; - const isOverride = this.OVERRIDE_KEY_TEST.test(key); - if (!isOverride) { - keys.push(key); - } - for (let i = 0; i < parts.length; i++) { - if (i === 0 && isOverride) { - const identifier = this.OVERRIDE_PROPERTY_PATTERN.exec(parts[i])![1]; - if (!overrides[identifier]) { - overrides[identifier] = { keys: [], identifiers: [identifier], contents: Object.create(null) }; + /** + * Creates a new object and assigns those keys of raw to it that are not likely to cause prototype polution. + * Also preprocesses override identifiers so that they take the form [identifier]: {...contents}. + */ + private sanitize(raw: T): T { + if (!isObject(raw)) { return raw; } + const asObject = raw as Record; + const sanitized = Object.create(null); + for (const key of Object.keys(asObject)) { + if (!injectionRe.test(key)) { + const override = this.OVERRIDE_KEY_TEST.exec(key); + if (override) { + const overrideKey = `[${override[1]}]`; + const remainder = key.slice(override[0].length); + if (!isObject(sanitized[overrideKey])) { + sanitized[overrideKey] = Object.create(null); } - branch = overrides[identifier].contents; - overrides[identifier].keys.push(key.slice(parts[i].length + 1)); - } else if (i === parts.length - 1) { - branch[parts[i]] = data[key]; + sanitized[overrideKey][remainder] = this.sanitize(asObject[key]); } else { - if (!branch[parts[i]]) { - branch[parts[i]] = Object.create(null); - } - branch = branch[parts[i]]; + sanitized[key] = this.sanitize(asObject[key]); } } - return result; - }, Object.create(null)); - return { contents, keys, overrides: Object.values(overrides) }; + } + return sanitized; } + private readonly OVERRIDE_KEY_TEST = /^\[([^\]]+)\]\./; + private toConfigurationChangeEvent(eventData: PreferenceChangeExt[]): theia.ConfigurationChangeEvent { return Object.freeze({ affectsConfiguration: (section: string, scope?: theia.ConfigurationScope): boolean => { From 310b32690944dd54ef285af93f3c9a3fa91555bc Mon Sep 17 00:00:00 2001 From: Colin Grant Date: Wed, 1 Feb 2023 13:09:45 -0700 Subject: [PATCH 2/4] Add guard for 12043 --- packages/plugin-ext/src/plugin/preference-registry.spec.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/plugin-ext/src/plugin/preference-registry.spec.ts b/packages/plugin-ext/src/plugin/preference-registry.spec.ts index c8c951caf0d22..7cbd2cb6c559a 100644 --- a/packages/plugin-ext/src/plugin/preference-registry.spec.ts +++ b/packages/plugin-ext/src/plugin/preference-registry.spec.ts @@ -229,6 +229,11 @@ describe.only('PreferenceRegistryExtImpl:', () => { expect(pythonOverrides).not.to.be.undefined; expect(pythonOverrides?.['editor.renderWhitespace']).equal('all'); }); + // https://github.com/eclipse-theia/theia/issues/12043 + it('Allows access preferences without specifying the section', () => { + const inspection = preferenceRegistryExtImpl.getConfiguration().inspect('editor.fontSize'); + expect(inspection?.defaultValue).equal(14); + }); }); describe('Proxy Behavior', () => { From 33b62395956f06ff0f8c86d3941956ba21b5a3e4 Mon Sep 17 00:00:00 2001 From: Colin Grant Date: Wed, 1 Feb 2023 13:12:48 -0700 Subject: [PATCH 3/4] typo --- packages/plugin-ext/src/plugin/preference-registry.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/plugin-ext/src/plugin/preference-registry.spec.ts b/packages/plugin-ext/src/plugin/preference-registry.spec.ts index 7cbd2cb6c559a..8f4d5db9c46d1 100644 --- a/packages/plugin-ext/src/plugin/preference-registry.spec.ts +++ b/packages/plugin-ext/src/plugin/preference-registry.spec.ts @@ -230,7 +230,7 @@ describe.only('PreferenceRegistryExtImpl:', () => { expect(pythonOverrides?.['editor.renderWhitespace']).equal('all'); }); // https://github.com/eclipse-theia/theia/issues/12043 - it('Allows access preferences without specifying the section', () => { + it('Allows access to preferences without specifying the section', () => { const inspection = preferenceRegistryExtImpl.getConfiguration().inspect('editor.fontSize'); expect(inspection?.defaultValue).equal(14); }); From deaa6d576a0c334cd72bf8c5b5fff9fb3b504440 Mon Sep 17 00:00:00 2001 From: Colin Grant Date: Wed, 1 Feb 2023 13:13:40 -0700 Subject: [PATCH 4/4] Not .only --- packages/plugin-ext/src/plugin/preference-registry.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/plugin-ext/src/plugin/preference-registry.spec.ts b/packages/plugin-ext/src/plugin/preference-registry.spec.ts index 8f4d5db9c46d1..e5e1143386fe2 100644 --- a/packages/plugin-ext/src/plugin/preference-registry.spec.ts +++ b/packages/plugin-ext/src/plugin/preference-registry.spec.ts @@ -23,7 +23,7 @@ import { URI } from './types-impl'; const expect = chai.expect; /* eslint-disable @typescript-eslint/no-explicit-any */ -describe.only('PreferenceRegistryExtImpl:', () => { +describe('PreferenceRegistryExtImpl:', () => { const workspaceRoot = URI.parse('/workspace-root'); let preferenceRegistryExtImpl: PreferenceRegistryExtImpl; const getProxy = (proxyId: ProxyIdentifier) => { };