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

document.all is not virtualizable #2

Open
Jack-Works opened this issue Mar 20, 2020 · 24 comments
Open

document.all is not virtualizable #2

Jack-Works opened this issue Mar 20, 2020 · 24 comments

Comments

@Jack-Works
Copy link
Member

No description provided.

@erights
Copy link
Collaborator

erights commented Mar 21, 2020

Yes @Jack-Works we need to explicitly state the limits on the virtualizability we intend to support. We should explicitly state that we will not support emulating document.all. Thanks!

@Jack-Works
Copy link
Member Author

Instead I suggest to remove the magic behavior on document.all. Let it be truthy can solve this problem.

@erights
Copy link
Collaborator

erights commented Mar 21, 2020

You mean in the main spec?
I do not know what the impact of that would be.

@Jack-Works
Copy link
Member Author

Jack-Works commented Mar 21, 2020

Yes, I'm mean by deleting it in the main spec.

Falsy value of document.all is used to prevent feature detection on the document.all, but there are better ways to do it. Like the browser can only make it falsy when using the literal to infer to it like what we do on the eval.

e.g.:

if (document.all) { // this direct access evaluate to false
}
var x = document.all
if (x) { // this indirect access evaluate to true
}

Make the indirect access to document.all evaluate to truthy, most of the security issues in sandboxes can be resolved (because they never direct access document.all).

@ljharb
Copy link
Member

ljharb commented Mar 21, 2020

It would almost certainly break the web to change that behavior at this point; otherwise we wouldn’t have added the IsHTMLDDA slot to the spec.

@Jack-Works
Copy link
Member Author

Maybe we should try to make it falsy meanwhile less harmful to code that doesn't using document.all

@ljharb
Copy link
Member

ljharb commented Mar 21, 2020

I'm not sure what you mean; the weird part is that it's a falsy object. Making it truthy would break websites; making it not an object would break websites. What change are you suggesting?

@Jack-Works
Copy link
Member Author

I'm suggesting that it still be a falsy object but only when the website is really using it.

@caridy
Copy link
Collaborator

caridy commented Mar 22, 2020

IMO, there are 3 things to look at:

  1. how to virtualize the bizarre case when typeof x returns undefined if x uses [[IsHTMLDDA]] internal slot.
  2. how to virtualize the bizarre case when x is falsy even though it is an object if x uses [[IsHTMLDDA]] internal slot.
  3. Can [[IsHTMLDDA]] case be reformed?

For us, at salesforce, we are virtualizing this by setting document.all to undefined inside the sandbox, that helps with 1, and part of 2, it seems to work for most libraries out there, and will only fail if a piece of code actually attempts to use the HTMLAllCollection object in any meaningful way.

@Jack-Works
Copy link
Member Author

Let the typeof check the isHTMLDDL slot recursively through the Proxy object that can resolve the visualizability.

However, what I really mean is the falsy evaluation is harmful to web security. It's better to disable this behavior on newer websites. (for example in module context or use a "legacy site list")

@erights
Copy link
Collaborator

erights commented Mar 22, 2020

Let the typeof check the isHTMLDDL slot recursively through the Proxy object that can resolve the visualizability.

I @Jack-Works I do not want to add any complexity to the proxy mechanism to deal with this ugly special case.

However, what I really mean is the falsy evaluation is harmful to web security. It's better to disable this behavior on newer websites. (for example in module context or use a "legacy site list")

I agree it is harmful, and I agree that we should find a way to disable it, or change it to some behavior that can be expressed in JS. I am not familiar with how it is currently used and what the compat issues are in trying to change its behavior. I do encourage you and all of us to continue searching for a way out of the current mess.

@Jack-Works
Copy link
Member Author

Jack-Works commented Mar 22, 2020

if (document.all) {
  // code that uses `document.all`, for ancient browsers
} else if (document.getElementById) {
  // code that uses `document.getElementById`, for “modern” browsers
}

// Internet Explorer
if (document.all) {
    useActiveX()
}
// Netscape Navigator
else {
    useOldButStillWorkingCode()
}

It's the common pattern that why we need document.all falsy.

If we can change the spec like:

If the document.all literal (important) is used to do the ToBoolean operation e.g. if (document.all), Boolean(document.all), or var isIE = !!document.all, it must return a falsy value

If the document.all is not used by literal (or used in an ESModule context?), it must be a normal object (truthy). e.g. const x = document.all; some_lib(!!x)

// literal usages
if (document.all) // still falsy
Boolean(document.all) // still falsy
!!document.all // still falsy
typeof document.all // "undefined"

// non literal usages
function typeof_(x) { return typeof x }
typeof_(document.all) // "object"

Then the old code will still be working, and newer code won't be hacked by the document.all (e.g. salesforce/near-membrane#76) because those code doesn't use the document.all literal.

This way is somewhat like eval behavior. I think it's acceptable to have one more literal detect to prevent a more hacky solution.

+function x() {
    let y = 1
    eval('y = 2')
    console.log(y) // logs 2
}()
+function x() {
    let y = 1
    let indirectEval = eval
    indirectEval('y = 2')
    console.log(y) // logs 1
}()

@caridy
Copy link
Collaborator

caridy commented Mar 22, 2020

Let the typeof check the isHTMLDDL slot recursively through the Proxy object that can resolve the visualizability.

@Jack-Works unfortunately that does not work in a membrane implementation because the target of the proxy is not the real object/function, but a shadow of it, what we call the shadow target, in order to preserve the language semantics.

@Jack-Works
Copy link
Member Author

See 12.3.6.1 step 6 for how direct eval is specced. If you want to do the same thing for document.all, you need to add similar logic in the runtime semantics of:

the typeof operator;
operators based on boolean logic: ||, &&, ? :, !;
operator allowing comparison with null: ==;
statements making use of boolean logic: if, while, do/while, for.
(Fortunately, the newer ?. and ?? operators do not have special treatment of document.all.)

By claudepache tc39/ecma262#673 (comment)

The current PR was specifically to sanction what browsers and the HTML spec were already implementing. That is, they felt it was better that the ES spec provided the necessary hook for the HTML spec, rather than the HTML spec monkey-patched the ES spec.

What you are proposing, is modifying the semantics of document.all in a nontrivial way, which implies implementation work and BC concern. If you feel that it is important, it should be a separate issue, and probably indeed a full-fledged proposal.

By claudepache tc39/ecma262#673 (comment)

I think it is a good thing to do. Change the meaning of document.all literal to make it Web compatible, and prevent it evaluates to false accidentally, which can improve security.

(See the code example above)

// literal usages
typeof document.all // "undefined"

// non literal usages
function typeof_(x) { return typeof x }
typeof_(document.all) // "object"

@ljharb
Copy link
Member

ljharb commented May 12, 2020

I’d be very surprised if that would be web compatible - but itd be worth you making the case to web browsers to gather telemetry about it.

@claudepache
Copy link

See 12.3.6.1 step 6 for how direct eval is specced. If you want to do the same thing for document.all, you need to add similar logic in the runtime semantics of:

the typeof operator;
operators based on boolean logic: ||, &&, ? :, !;
operator allowing comparison with null: ==;
statements making use of boolean logic: if, while, do/while, for.
(Fortunately, the newer ?. and ?? operators do not have special treatment of document.all.)

By claudepache tc39/ecma262#673 (comment)

I correct myself: Copying how direct eval is specified would not work (unless you’re very optimistic about what is needed for web compatibility), because
(somethingThatEvaluatesToFalse || eval)() is not a direct eval (*), while
if (somethingThatEvaluatesToFalse || document.all) should probably test as false.

Another interesting case: should we “support” if (da = document.all)?


(*) The technical reason is that a || b does not evaluate to a Reference, because it applies GetValue() before returning the result.

@Jack-Works
Copy link
Member Author

browsers to gather telemetry about it

curious about it. do browsers already do that? does the telemetry data for this public or visible in some way?

@claudepache
Copy link

I think it is a good thing to do. Change the meaning of document.all literal to make it Web compatible, and prevent it evaluates to false accidentally, which can improve security.

FYI, 16 years ago, hacking document.all literal only wasn’t web-compatible, see:

https://bugzilla.mozilla.org/show_bug.cgi?id=259935

@Jack-Works
Copy link
Member Author

function foo() {
    this.ie = document.all;
}
var f = new foo();
if (f.ie) // ...

Damn! It's impossible to do that in the end

Hmm but, does the percent of usage changed now? Just like how we delete other web platform features.

@mmis1000
Copy link

mmis1000 commented May 13, 2020

Just to notice, Symbol polyfills rewrite typeof to helper functions (so typeof xxx === 'symbol' works), so literals will no longer be literal

Before

if (typeof document.all === 'undefined') {...}

After

function _typeof(val) { /* helper function body... */}

if (_typeof(document.all) === 'undefined') {...}

@Jack-Works
Copy link
Member Author

The code that using a transpiler, won't use document.all IMO

@mmis1000
Copy link

The npm dependencies that use document.all may still be transpiled in some condition. (e.g. some dependency used new syntax that old browser do not support, so they got transpiled all together)

@ljharb
Copy link
Member

ljharb commented May 13, 2020

@Jack-Works now that people are sometimes unwisely transpiling node_modules - code they didn't write - that's not necessarily the case.

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

No branches or pull requests

6 participants