From 3a3212cc9842d3ca2cdab86c6fc0611eaf5606f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Caridy=20Pati=C3=B1o?= Date: Mon, 16 Mar 2020 00:49:01 -0400 Subject: [PATCH 1/4] fixes #76: document.all == null breaks the membrane for valid targets --- src/raw-value.ts | 2 +- src/secure-value.ts | 9 ++++++--- src/shared.ts | 9 ++++++--- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/raw-value.ts b/src/raw-value.ts index 4d7bce63..3b392977 100644 --- a/src/raw-value.ts +++ b/src/raw-value.ts @@ -183,7 +183,7 @@ export function reverseProxyFactory(env: MembraneBroker) { } construct(shadowTarget: ReverseShadowTarget, rawArgArray: RawValue[], rawNewTarget: RawObject): RawObject { const { target: SecCtor } = this; - if (rawNewTarget === undefined) { + if (isUndefined(rawNewTarget)) { throw TypeError(); } const secArgArray = env.getSecureValue(rawArgArray); diff --git a/src/secure-value.ts b/src/secure-value.ts index 9dc4fe8d..d72f990b 100644 --- a/src/secure-value.ts +++ b/src/secure-value.ts @@ -102,13 +102,16 @@ export const serializedSecureEnvSourceText = (function secureEnvFactory(rawEnv: return obj === undefined; } + function isNull(obj: any): obj is null { + return obj === null; + } + function isFunction(obj: any): obj is Function { return typeof obj === 'function'; } - function isNullish(obj: any): obj is null { - // eslint-disable-next-line eqeqeq - return obj == null; + function isNullish(obj: any): obj is (null | undefined) { + return isNull(obj) || isUndefined(obj); } function getSecureValue(raw: RawValue): SecureValue { diff --git a/src/shared.ts b/src/shared.ts index ea114649..488fdf35 100644 --- a/src/shared.ts +++ b/src/shared.ts @@ -62,9 +62,12 @@ export function isUndefined(obj: any): obj is undefined { return obj === undefined; } -export function isNullish(obj: any): obj is null { - // eslint-disable-next-line eqeqeq - return obj == null; +export function isNull(obj: any): obj is null { + return obj === null; +} + +export function isNullish(obj: any): obj is (null | undefined) { + return isNull(obj) || isUndefined(obj); } export function isTrue(obj: any): obj is true { From 527ad739ac62449f25055ee89c5d2fd91614a6d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Caridy=20Pati=C3=B1o?= Date: Mon, 16 Mar 2020 21:03:08 -0400 Subject: [PATCH 2/4] covering the case when HTMLAllCollection reports typeof equal undefined --- src/environment.ts | 75 +++++++++++++++++++++++---------------------- src/raw-value.ts | 24 +++++++-------- src/secure-value.ts | 32 +++++++++---------- src/shared.ts | 2 +- 4 files changed, 65 insertions(+), 68 deletions(-) diff --git a/src/environment.ts b/src/environment.ts index ccb9ff2b..3636e2de 100644 --- a/src/environment.ts +++ b/src/environment.ts @@ -2,7 +2,7 @@ import { apply, assign, isUndefined, - isNullish, + isNullOrUndefined, ObjectCreate, isFunction, hasOwnProperty, @@ -31,7 +31,6 @@ import { RawValue, SecureValue, RawConstructor, - SecureConstructor, MembraneBroker, DistortionMap, SecureProxy, @@ -39,17 +38,6 @@ import { ReverseProxyTarget, } from './types'; -// it means it does have identity and should be proxified. -function isProxyTarget(o: RawValue | SecureValue): - o is (RawFunction | RawConstructor | RawObject | SecureFunction | SecureConstructor | SecureObject) { - // hire-wired for the common case - if (isNullish(o)) { - return false; - } - const t = typeof o; - return t === 'object' || t === 'function'; -} - interface SecureEnvironmentOptions { // Base global object used by the raw environment rawGlobalThis: RawObject & typeof globalThis; @@ -72,7 +60,16 @@ export class SecureEnvironment implements MembraneBroker { throw ErrorCreate(`Missing SecureEnvironmentOptions options bag.`); } const { rawGlobalThis, secureGlobalThis, distortionMap } = options; - this.distortionMap = WeakMapCreate(isUndefined(distortionMap) ? [] : distortionMap.entries()); + this.distortionMap = WeakMapCreate(); + // validating distortion entries + distortionMap?.forEach((value, key) => { + const o = typeof key; + const d = typeof value; + if (o !== d) { + throw ErrorCreate(`Invalid distortion ${value}.`); + } + WeakMapSet(this.distortionMap, key, value); + }); // getting proxy factories ready per environment so we can produce // the proper errors without leaking instances into a sandbox const secureEnvFactory = secureGlobalThis.eval(`(${serializedSecureEnvSourceText})`); @@ -152,10 +149,6 @@ export class SecureEnvironment implements MembraneBroker { let currentGetter = () => undefined; if (isFunction(originalGetter)) { const originalOrDistortedGetter: () => any = WeakMapGet(this.distortionMap, originalGetter) || originalGetter; - if (!isProxyTarget(originalOrDistortedGetter)) { - // TODO: needs to be resilient, cannot just throw, what should we do instead? - throw ErrorCreate(`Invalid distortion.`); - } currentGetter = function(this: any): SecureValue { const value: RawValue = apply(originalOrDistortedGetter, env.getRawValue(this), emptyArray); return env.getSecureValue(value); @@ -178,39 +171,47 @@ export class SecureEnvironment implements MembraneBroker { if (!isUndefined(secureDescriptor) && hasOwnProperty(secureDescriptor, 'configurable') && secureDescriptor.configurable === false) { + const securePropertyValue = secureValue[key]; + if (isNullOrUndefined(securePropertyValue)) { + continue; + } + const t = typeof securePropertyValue; // this is the case where the secure env has a descriptor that was supposed to be // overrule but can't be done because it is a non-configurable. Instead we try to // fallback to some more advanced gymnastics - if (hasOwnProperty(secureDescriptor, 'value') && isProxyTarget(secureDescriptor.value)) { - const { value: secureDescriptorValue } = secureDescriptor; - if (!WeakMapHas(this.som, secureDescriptorValue)) { - // remapping the value of the secure object graph to the outer realm graph - const { value: rawDescriptorValue } = rawDescriptor; - if (secureValue !== rawDescriptorValue) { - if (this.getRawValue(secureValue) !== rawValue) { - console.error('need remapping: ', key, rawValue, rawDescriptor); + if (hasOwnProperty(secureDescriptor, 'value')) { + // valid proxy target (intentionally ignoring the case of document.all since it is not a value descriptor) + if (t === 'function' || t === 'object') { + if (!WeakMapHas(this.som, securePropertyValue)) { + // remapping the value of the secure object graph to the outer realm graph + const { value: rawDescriptorValue } = rawDescriptor; + if (secureValue !== rawDescriptorValue) { + if (this.getRawValue(secureValue) !== rawValue) { + console.error('need remapping: ', key, rawValue, rawDescriptor); + } else { + // it was already mapped + } } else { - // it was already mapped + // window.top is the classic example of a descriptor that leaks access to the outer + // window reference, and there is no containment for that case yet. + console.error('leaking: ', key, rawValue, rawDescriptor); } } else { - // window.top is the classic example of a descriptor that leaks access to the outer - // window reference, and there is no containment for that case yet. - console.error('leaking: ', key, rawValue, rawDescriptor); + // an example of this is circular window.window ref + console.info('circular: ', key, rawValue, rawDescriptor); } - } else { - // an example of this is circular window.window ref - console.info('circular: ', key, rawValue, rawDescriptor); } } else if (hasOwnProperty(secureDescriptor, 'get')) { - const secureDescriptorValue = secureValue[key]; - if (isProxyTarget(secureDescriptorValue)) { - if (secureDescriptorValue === secureValue[key]) { + // internationally ignoring the case of (typeof document.all === 'undefined') because + // it is specified as configurable, you never get one of those exotic objects in this branch + if (t === 'function' || t === 'object') { + if (securePropertyValue === secureValue[key]) { // this is the case for window.document which is identity preserving getter // const rawDescriptorValue = rawValue[key]; // this.setRefMapEntries(secureDescriptorValue, rawDescriptorValue); // this.installDescriptors(secureDescriptorValue, rawDescriptorValue, getOwnPropertyDescriptors(rawDescriptorValue)); console.error('need remapping: ', key, rawValue, rawDescriptor); - if (ReflectIsExtensible(secureDescriptorValue)) { + if (ReflectIsExtensible(securePropertyValue)) { // remapping proto chain // ReflectSetPrototypeOf(secureDescriptorValue, this.getSecureValue(ReflectGetPrototypeOf(secureDescriptorValue))); console.error('needs prototype remapping: ', key, rawValue); diff --git a/src/raw-value.ts b/src/raw-value.ts index 3b392977..adf3a567 100644 --- a/src/raw-value.ts +++ b/src/raw-value.ts @@ -14,7 +14,7 @@ import { ReflectGet, ReflectSet, map, - isNullish, + isNullOrUndefined, unconstruct, ownKeys, ReflectIsExtensible, @@ -34,7 +34,6 @@ import { RawArray, SecureValue, MembraneBroker, - SecureObject, } from './types'; function renameFunction(provider: (...args: any[]) => any, receiver: (...args: any[]) => any) { @@ -52,16 +51,6 @@ function renameFunction(provider: (...args: any[]) => any, receiver: (...args: a } } -// it means it does have identity and should be proxified. -function isProxyTarget(o: RawValue): o is (SecureFunction | SecureConstructor | SecureObject) { - // hire-wired for the common case - if (isNullish(o)) { - return false; - } - const t = typeof o; - return t === 'object' || t === 'function'; -} - const ProxyRevocable = Proxy.revocable; const ProxyCreate = unconstruct(Proxy); const { isArray: isArrayOrNotOrThrowForRevoked } = Array; @@ -285,6 +274,15 @@ export function reverseProxyFactory(env: MembraneBroker) { ReflectSetPrototypeOf(ReverseProxyHandler.prototype, null); function getRawValue(sec: SecureValue): RawValue { + if (isNullOrUndefined(sec)) { + return sec as RawValue; + } + const t = typeof sec; + // internationally ignoring the case of (typeof document.all === 'undefined') because + // in the reserve membrane, you never get one of those exotic objects + if (t === 'function') { + return getRawFunction(sec); + } let isSecArray = false; try { isSecArray = isArrayOrNotOrThrowForRevoked(sec); @@ -293,7 +291,7 @@ export function reverseProxyFactory(env: MembraneBroker) { } if (isSecArray) { return getRawArray(sec); - } else if (isProxyTarget(sec)) { + } else if (t === 'object') { const raw = env.getRawRef(sec); if (isUndefined(raw)) { return createReverseProxy(sec); diff --git a/src/secure-value.ts b/src/secure-value.ts index d72f990b..57d0b0dd 100644 --- a/src/secure-value.ts +++ b/src/secure-value.ts @@ -20,7 +20,6 @@ import { RawConstructor, RawFunction, RawValue, - RawObject, RawArray, TargetMeta, MembraneBroker, @@ -110,11 +109,24 @@ export const serializedSecureEnvSourceText = (function secureEnvFactory(rawEnv: return typeof obj === 'function'; } - function isNullish(obj: any): obj is (null | undefined) { + function isNullOrUndefined(obj: any): obj is (null | undefined) { return isNull(obj) || isUndefined(obj); } function getSecureValue(raw: RawValue): SecureValue { + if (isNullOrUndefined(raw)) { + return raw as SecureValue; + } + const t = typeof raw; + // NOTE: internationally checking for typeof 'undefined' for the case of + // `typeof document.all === 'undefined'`, which is an exotic object with + // a bizarre behavior described here: + // * https://tc39.es/ecma262/#sec-IsHTMLDDA-internal-slot + // This check cover that case, but doesn't affect other unidefined values + // because those are covered by the previous condition anyways. + if (t === 'function' || t === 'undefined') { + return getSecureFunction(raw); + } let isRawArray = false; try { isRawArray = isArrayOrNotOrThrowForRevoked(raw); @@ -124,7 +136,7 @@ export const serializedSecureEnvSourceText = (function secureEnvFactory(rawEnv: } if (isRawArray) { return getSecureArray(raw); - } else if (isProxyTarget(raw)) { + } else if (t === 'object') { const sec: SecureValue | undefined = WeakMapGet(rom, raw); if (isUndefined(sec)) { return createSecureProxy(raw); @@ -155,23 +167,9 @@ export const serializedSecureEnvSourceText = (function secureEnvFactory(rawEnv: } // if a distortion entry is found, it must be a valid proxy target const distortedTarget = WeakMapGet(distortionMap, target) as SecureProxyTarget; - if (!isProxyTarget(distortedTarget)) { - // TODO: needs to be resilient, cannot just throw, what should we do instead? - throw ErrorCreate(`Invalid distortion.`); - } return distortedTarget; } - // it means it does have identity and should be proxified. - function isProxyTarget(o: RawValue | SecureValue): o is (RawFunction | RawConstructor | RawObject) { - // hire-wired for the common case - if (isNullish(o)) { - return false; - } - const t = typeof o; - return t === 'object' || t === 'function'; - } - function renameFunction(rawProvider: (...args: any[]) => any, receiver: (...args: any[]) => any) { let nameDescriptor: PropertyDescriptor | undefined; try { diff --git a/src/shared.ts b/src/shared.ts index 488fdf35..19fa299a 100644 --- a/src/shared.ts +++ b/src/shared.ts @@ -66,7 +66,7 @@ export function isNull(obj: any): obj is null { return obj === null; } -export function isNullish(obj: any): obj is (null | undefined) { +export function isNullOrUndefined(obj: any): obj is (null | undefined) { return isNull(obj) || isUndefined(obj); } From 1259e38f6048a56f5bec59ceaefb7e5d2c7617af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Caridy=20Pati=C3=B1o?= Date: Mon, 16 Mar 2020 21:03:40 -0400 Subject: [PATCH 3/4] adding tests --- test/membrane/document-all.spec.js | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 test/membrane/document-all.spec.js diff --git a/test/membrane/document-all.spec.js b/test/membrane/document-all.spec.js new file mode 100644 index 00000000..dee02aec --- /dev/null +++ b/test/membrane/document-all.spec.js @@ -0,0 +1,22 @@ +import createSecureEnvironment from '../../lib/browser-realm.js'; + +describe('document.all', () => { + it('should change the value of its yellow typeof from "undefined" to "object"', function() { + // expect.assertions(2); + const evalScript = createSecureEnvironment(); + expect(typeof document.all).toBe("undefined"); + evalScript(` + // observable difference between a regular dom and a sandboxed dom + expect(typeof document.all).toBe("object"); + `); + }); + it('should work throughout the membrane', function() { + // expect.assertions(3); + const evalScript = createSecureEnvironment(); + evalScript(` + expect(document.all.length > 1).toBeTrue(); + expect(document.all[0].ownerDocument).toBe(document); // comparison in red + expect(document.all[0].ownerDocument === document).toBeTrue(); // comparison in yellow + `); + }); +}); From dd82723d7cda5ffbe93f8362c9e737b7c37281b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Caridy=20Pati=C3=B1o?= Date: Mon, 16 Mar 2020 21:36:16 -0400 Subject: [PATCH 4/4] typos --- src/secure-value.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/secure-value.ts b/src/secure-value.ts index 57d0b0dd..97f7ac2f 100644 --- a/src/secure-value.ts +++ b/src/secure-value.ts @@ -122,7 +122,7 @@ export const serializedSecureEnvSourceText = (function secureEnvFactory(rawEnv: // `typeof document.all === 'undefined'`, which is an exotic object with // a bizarre behavior described here: // * https://tc39.es/ecma262/#sec-IsHTMLDDA-internal-slot - // This check cover that case, but doesn't affect other unidefined values + // This check covers that case, but doesn't affect other undefined values // because those are covered by the previous condition anyways. if (t === 'function' || t === 'undefined') { return getSecureFunction(raw);