From b8336fe910f18d3bdf53ffcb804cc039acd90fb1 Mon Sep 17 00:00:00 2001 From: Marcus Andersson Date: Fri, 5 May 2023 15:50:40 +0200 Subject: [PATCH] PluginExtensions: Make context read only with a proxy instead of object freeze (#67781) * quick poc on how we could use proxies instead of object freeze for the extension context. * wip * wip * added tests for readOnlyProxy. * Changed so we use the read only proxy for the context instead of the frozen object. * updated names according to feedback. --- .../extensions/getPluginExtensions.test.ts | 15 ++-- .../plugins/extensions/getPluginExtensions.ts | 10 ++- .../plugins/extensions/utils.test.tsx | 90 ++++++++++++++++++- .../app/features/plugins/extensions/utils.tsx | 49 ++++++++++ 4 files changed, 154 insertions(+), 10 deletions(-) diff --git a/public/app/features/plugins/extensions/getPluginExtensions.test.ts b/public/app/features/plugins/extensions/getPluginExtensions.test.ts index 54d5b63b838..5002fa16c30 100644 --- a/public/app/features/plugins/extensions/getPluginExtensions.test.ts +++ b/public/app/features/plugins/extensions/getPluginExtensions.test.ts @@ -2,6 +2,7 @@ import { PluginExtensionLinkConfig, PluginExtensionTypes } from '@grafana/data'; import { createPluginExtensionRegistry } from './createPluginExtensionRegistry'; import { getPluginExtensions } from './getPluginExtensions'; +import { isReadOnlyProxy } from './utils'; import { assertPluginExtensionLink } from './validators'; describe('getPluginExtensions()', () => { @@ -96,19 +97,19 @@ describe('getPluginExtensions()', () => { expect(link2.configure).toHaveBeenCalledTimes(1); expect(extensions).toHaveLength(0); }); - test('should pass a frozen copy of the context to the configure() function', () => { + test('should pass a read only context to the configure() function', () => { const context = { title: 'New title from the context!' }; const registry = createPluginExtensionRegistry([{ pluginId, extensionConfigs: [link2] }]); const { extensions } = getPluginExtensions({ registry, context, extensionPointId: extensionPoint2 }); const [extension] = extensions; - const frozenContext = (link2.configure as jest.Mock).mock.calls[0][0]; + const readOnlyContext = (link2.configure as jest.Mock).mock.calls[0][0]; assertPluginExtensionLink(extension); expect(link2.configure).toHaveBeenCalledTimes(1); - expect(Object.isFrozen(frozenContext)).toBe(true); + expect(isReadOnlyProxy(readOnlyContext)).toBe(true); expect(() => { - frozenContext.title = 'New title'; + readOnlyContext.title = 'New title'; }).toThrow(); expect(context.title).toBe('New title from the context!'); }); @@ -247,7 +248,7 @@ describe('getPluginExtensions()', () => { expect(global.console.warn).toHaveBeenCalledWith('[Plugin Extensions] Something went wrong!'); }); - test('should pass a frozen copy of the context to the onClick() function', () => { + test('should pass a read only context to the onClick() function', () => { const context = { title: 'New title from the context!' }; link2.path = undefined; @@ -263,13 +264,13 @@ describe('getPluginExtensions()', () => { const helpers = (link2.onClick as jest.Mock).mock.calls[0][1]; expect(link2.configure).toHaveBeenCalledTimes(1); - expect(Object.isFrozen(helpers.context)).toBe(true); + expect(isReadOnlyProxy(helpers.context)).toBe(true); expect(() => { helpers.context.title = 'New title'; }).toThrow(); }); - test('should should not freeze the original context', () => { + test('should should not make original context read only', () => { const context = { title: 'New title from the context!', nested: { title: 'title' }, diff --git a/public/app/features/plugins/extensions/getPluginExtensions.ts b/public/app/features/plugins/extensions/getPluginExtensions.ts index 7440528c4bc..071a67028b4 100644 --- a/public/app/features/plugins/extensions/getPluginExtensions.ts +++ b/public/app/features/plugins/extensions/getPluginExtensions.ts @@ -6,7 +6,13 @@ import { } from '@grafana/data'; import type { PluginExtensionRegistry } from './types'; -import { isPluginExtensionLinkConfig, deepFreeze, logWarning, generateExtensionId, getEventHelpers } from './utils'; +import { + isPluginExtensionLinkConfig, + getReadOnlyProxy, + logWarning, + generateExtensionId, + getEventHelpers, +} from './utils'; import { assertIsNotPromise, assertLinkPathIsValid, assertStringProps, isPromise } from './validators'; type GetExtensions = ({ @@ -21,7 +27,7 @@ type GetExtensions = ({ // Returns with a list of plugin extensions for the given extension point export const getPluginExtensions: GetExtensions = ({ context, extensionPointId, registry }) => { - const frozenContext = context ? deepFreeze(context) : {}; + const frozenContext = context ? getReadOnlyProxy(context) : {}; const registryItems = registry[extensionPointId] ?? []; // We don't return the extensions separated by type, because in that case it would be much harder to define a sort-order for them. const extensions: PluginExtension[] = []; diff --git a/public/app/features/plugins/extensions/utils.test.tsx b/public/app/features/plugins/extensions/utils.test.tsx index 7c20bf2f2df..f814bd1df3b 100644 --- a/public/app/features/plugins/extensions/utils.test.tsx +++ b/public/app/features/plugins/extensions/utils.test.tsx @@ -1,6 +1,6 @@ import { PluginExtensionLinkConfig, PluginExtensionTypes } from '@grafana/data'; -import { deepFreeze, isPluginExtensionLinkConfig, handleErrorsInFn } from './utils'; +import { deepFreeze, isPluginExtensionLinkConfig, handleErrorsInFn, getReadOnlyProxy } from './utils'; describe('Plugin Extensions / Utils', () => { describe('deepFreeze()', () => { @@ -219,4 +219,92 @@ describe('Plugin Extensions / Utils', () => { }).not.toThrow(); }); }); + + describe('getReadOnlyProxy()', () => { + it('should not be possible to modify values in proxied object', () => { + const proxy = getReadOnlyProxy({ a: 'a' }); + + expect(() => { + proxy.a = 'b'; + }).toThrowError(TypeError); + }); + + it('should not be possible to modify values in proxied array', () => { + const proxy = getReadOnlyProxy([1, 2, 3]); + + expect(() => { + proxy[0] = 2; + }).toThrowError(TypeError); + }); + + it('should not be possible to modify nested objects in proxied object', () => { + const proxy = getReadOnlyProxy({ + a: { + c: 'c', + }, + b: 'b', + }); + + expect(() => { + proxy.a.c = 'testing'; + }).toThrowError(TypeError); + }); + + it('should not be possible to modify nested arrays in proxied object', () => { + const proxy = getReadOnlyProxy({ + a: { + c: ['c', 'd'], + }, + b: 'b', + }); + + expect(() => { + proxy.a.c[0] = 'testing'; + }).toThrowError(TypeError); + }); + + it('should be possible to modify source object', () => { + const source = { a: 'b' }; + + getReadOnlyProxy(source); + source.a = 'c'; + + expect(source.a).toBe('c'); + }); + + it('should be possible to modify source array', () => { + const source = ['a', 'b']; + + getReadOnlyProxy(source); + source[0] = 'c'; + + expect(source[0]).toBe('c'); + }); + + it('should be possible to modify nedsted objects in source object', () => { + const source = { a: { b: 'c' } }; + + getReadOnlyProxy(source); + source.a.b = 'd'; + + expect(source.a.b).toBe('d'); + }); + + it('should be possible to modify nedsted arrays in source object', () => { + const source = { a: { b: ['c', 'd'] } }; + + getReadOnlyProxy(source); + source.a.b[0] = 'd'; + + expect(source.a.b[0]).toBe('d'); + }); + + it('should be possible to call functions in proxied object', () => { + const proxy = getReadOnlyProxy({ + a: () => 'testing', + }); + + expect(proxy.a()).toBe('testing'); + }); + }); }); diff --git a/public/app/features/plugins/extensions/utils.tsx b/public/app/features/plugins/extensions/utils.tsx index dd7de44c70a..8e02ff40c84 100644 --- a/public/app/features/plugins/extensions/utils.tsx +++ b/public/app/features/plugins/extensions/utils.tsx @@ -1,3 +1,4 @@ +import { isArray, isObject } from 'lodash'; import React from 'react'; import { @@ -110,3 +111,51 @@ export function generateExtensionId(pluginId: string, extensionConfig: PluginExt .reduce((s, c) => (Math.imul(31, s) + c.charCodeAt(0)) | 0, 0) .toString(); } + +const _isProxy = Symbol('isReadOnlyProxy'); + +/** + * Returns a proxy that wraps the given object in a way that makes it read only. + * If you try to modify the object a TypeError exception will be thrown. + * + * @param obj The object to make read only + * @returns A new read only object, does not modify the original object + */ +export function getReadOnlyProxy(obj: T): T { + if (!obj || typeof obj !== 'object' || isReadOnlyProxy(obj)) { + return obj; + } + + const cache = new WeakMap(); + + return new Proxy(obj, { + defineProperty: () => false, + deleteProperty: () => false, + isExtensible: () => false, + set: () => false, + get(target, prop, receiver) { + if (prop === _isProxy) { + return true; + } + + const value = Reflect.get(target, prop, receiver); + + if (isObject(value) || isArray(value)) { + if (!cache.has(value)) { + cache.set(value, getReadOnlyProxy(value)); + } + return cache.get(value); + } + + return value; + }, + }); +} + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null; +} + +export function isReadOnlyProxy(value: unknown): boolean { + return isRecord(value) && value[_isProxy] === true; +}