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

fixes #76: document.all == null breaks the membrane for valid targets #77

Merged
merged 4 commits into from Mar 17, 2020

Conversation

@caridy
Copy link
Owner

caridy commented Mar 16, 2020

This PR fixes #76

  • tests (pending)
@caridy caridy requested a review from jdalton Mar 16, 2020
@@ -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)) {

This comment has been minimized.

Copy link
@caridy

caridy Mar 16, 2020

Author Owner

using existing local abtraction

Copy link
Collaborator

jdalton left a comment

This is interesting!

I'm a fan of basics like foo == null so this is a good case to remember for sure!

@mmis1000

This comment has been minimized.

Copy link

mmis1000 commented Mar 16, 2020

It looks like it will cause typeof document.all being function now?
Because document.all is actually a callable function.
Just the engine decide not to call it a function and call it undefined instead

@caridy

This comment has been minimized.

Copy link
Owner Author

caridy commented Mar 16, 2020

wow:

document.all
> HTMLAllCollection(23) [html, head, title, link, meta, meta, body, x-external, iframe, iframe, iframe, iframe, iframe, p, iframe, iframe, iframe, iframe, iframe, iframe, iframe, iframe, iframe, viewport: meta]
var a = document.all
> undefined
typeof a
> "undefined"
@mmis1000

This comment has been minimized.

Copy link

mmis1000 commented Mar 16, 2020

https://tc39.es/ecma262/#sec-IsHTMLDDA-internal-slot

So... You can't mimic the actual behavior of the document.all collection because it has a special internal slot that can't be written by normal js code.

caridy added 2 commits Mar 17, 2020
@@ -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();

This comment has been minimized.

Copy link
@caridy

caridy Mar 17, 2020

Author Owner

we had some checks dispersed in different places to check the distortion when retrieved for usage, that had the side effect that only if used, you will see those errors. That check was also testing if a distortion is needed before extracting the distortion, this is why this is related to this PR, because that check was broken due to document.all issue.

Nevertheless, we now validate each and every one of the distortion entries during construction phase, resulting on early errors.

if (o !== d) {
throw ErrorCreate(`Invalid distortion ${value}.`);
}
WeakMapSet(this.distortionMap, key, value);

This comment has been minimized.

Copy link
@caridy

caridy Mar 17, 2020

Author Owner

probably we can optimize more here by avoiding the this lookup.

@@ -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)) {

This comment has been minimized.

Copy link
@caridy

caridy Mar 17, 2020

Author Owner

moved to construction check

@@ -285,6 +274,15 @@ export function reverseProxyFactory(env: MembraneBroker) {
ReflectSetPrototypeOf(ReverseProxyHandler.prototype, null);

function getRawValue(sec: SecureValue): RawValue {
if (isNullOrUndefined(sec)) {

This comment has been minimized.

Copy link
@caridy

caridy Mar 17, 2020

Author Owner

reorganizing the check here, using the information to decide what to do to avoid double checking based on the type.

@@ -152,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)) {

This comment has been minimized.

Copy link
@caridy

caridy Mar 17, 2020

Author Owner

moved to construction

expect(typeof document.all).toBe("undefined");
evalScript(`
// observable difference between a regular dom and a sandboxed dom
expect(typeof document.all).toBe("object");

This comment has been minimized.

Copy link
@caridy

caridy Mar 17, 2020

Author Owner

this is the new observable difference between real window and sandbox

}

function getSecureValue(raw: RawValue): SecureValue {
if (isNullOrUndefined(raw)) {

This comment has been minimized.

Copy link
@caridy

caridy Mar 17, 2020

Author Owner

same as the getRawValue refactor, using the type check to decide what to do.

// * 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') {

This comment has been minimized.

Copy link
@caridy

caridy Mar 17, 2020

Author Owner

this is the condition that includes typeof document.all === 'undefined' considering that if the value is truly undefined, it will exit in the block above not here.

@caridy

This comment has been minimized.

Copy link
Owner Author

caridy commented Mar 17, 2020

@jdalton this is ready for another pass.

Copy link
Collaborator

jdalton left a comment

Looks ok! Would be interesting to see if we could hack around the observable typeof difference for document.all (maybe by monkey-patching its prototype.

@caridy caridy merged commit ca2349f into master Mar 17, 2020
@caridy caridy deleted the caridy/issue-76 branch Mar 17, 2020
@mmis1000

This comment has been minimized.

Copy link

mmis1000 commented Mar 18, 2020

@jdalton I tried that, but it looks like all props of document.all is magic non-configurable property (just like UInt8Array::length)

So hack the prototype won't help, because you will still leak elements.

@caridy

This comment has been minimized.

Copy link
Owner Author

caridy commented Mar 18, 2020

@mmis1000 honestly, I'm ok with that observable difference of the typeof document.all being "object" inside the sandbox, I think that's not going to break a lot of existing code anyways.

@mmis1000

This comment has been minimized.

Copy link

mmis1000 commented Mar 18, 2020

The reason of typeof document.all is undefined is compatibility reason for ancient code created before the document.getElementById completely rollout to all browser.

During that transition period. Both of them exist and some code made bad assumption to try document.all first

This magic behavior lies to the code that document.all does not exist, try document.getElementById instead.

I guess it is even ok to make it actually undefined because code nowadays is unlikely to use it.

@jdalton

This comment has been minimized.

Copy link
Collaborator

jdalton commented Mar 18, 2020

I guess it is even ok to make it actually undefined because code nowadays is unlikely to use it.

Ohhh! If possible undefined would rock too.

@caridy

This comment has been minimized.

Copy link
Owner Author

caridy commented Mar 18, 2020

@jdalton I prefer to no have that logic here in this library, and instead, developers using this lib can add a simple distortion to make the all getter to return undefined, and close out the access to that feature entirely. Makes sense?

@jdalton

This comment has been minimized.

Copy link
Collaborator

jdalton commented Mar 18, 2020

Ah yes!

if (isNullOrUndefined(raw)) {
return raw as SecureValue;
}
const t = typeof raw;

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 18, 2020

i believe caching the typeof value like this can hurt performance; i've been pushed to avoid this pattern in the es6-shim for that reason (but don't have benchmarks). iow, the faster pattern (at the time, at least) was to always repeat typeof raw === inline, because that's what engines optimized for.

This comment has been minimized.

Copy link
@caridy

caridy Mar 18, 2020

Author Owner

@ljharb I can see why! let me run some benchmarks, and update that in another PR since we do it in multiple places!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.