document.currentScript from a script in a shadow tree. #477

Closed
hayatoito opened this Issue Apr 1, 2016 · 32 comments

Projects

None yet

8 participants

@hayatoito
Member

Regarding #377,

I thought that document.currentScript should return null if a running script is in a shadow tree,
however, given that "document.currentScript" is always executed by the script, this situation looks like : "the script is just accessing the script element which is running me".

It's like a this pointer... This does not seem to violate an encapsulation.

Thus, what document.currentScript should be? Is it okay to return a script element in a shadow tree?

@hayatoito
Member

Per https://html.spec.whatwg.org/multipage/dom.html#dom-document-currentscript,

The currentScript attribute, on getting, must return the value to which it was most recently initialised. When the Document is created, the currentScript must be initialised to null.

Wait... As per the definition, I might be wrong. I forgot the case of an event listener.

1 A 'click' event listener is registered in a script in a document tree
2. <script> tag is added to a shadow tree
3. A user clicks something
4. In the click event listener, document.currentScript is accessed. => It returns the script in a shadow tree.

In this case, it looks that we would violate an encapsulation.

@hayatoito
Member

Hmm. Per the Note:

Returns null if the Document is not currently executing a script or SVG script element (e.g., because the running script is an event handler, or a timeout).

In the event handler, it looks that document.currentScript should return null, anyway. So it does not violate an encapsulation.

Am I correct?

@annevk
Member
annevk commented Apr 1, 2016

What about the corresponding events?

Also, this would mean that if the executing script does anything within the parent, like call a function, it exposes the shadow tree in a rather trivial matter.

@hayatoito
Member

Also, this would mean that if the executing script does anything within the parent, like call a function, it exposes the shadow tree in a rather trivial matter.

I see. That would cause an unintentional leak by calling a function such as:

var script;
void f() {
  script = document.currentScript;
  //...
}

I thought that we should let DocumentOrShadowRoot have currentScript, however, I am afraid that would not help at all. In most cases, the script needs to get the script element without any reference to its shadow root.

@annevk
Member
annevk commented Apr 1, 2016

That is a very good point, the script does not really know where it is, that is why we have this feature in the first place.

Maybe it's okay to have this and scripts just need to be careful. This is no isolation after all.

The events would still be troublesome however, but only Firefox implements those at the moment. @smaug----, thoughts?

@smaug----

The outer world shouldn't know there is a shadow script being executed so document.currentScript should never point to a shadow <script>. And the events should be scoped.
Otherwise we just leak everything about shadow dom to the outside world.

I don't see any good way to support currentScript for shadow scripts, unless we execute shadow scripts somehow in a special scope where shadowCurrentScript or some such is explicitly exposed.

(yet another case where I think XBL2 had things right)

@annevk
Member
annevk commented Apr 1, 2016

@smaug---- it only points to the script while the script is executing though, no? So how can the outer world observe it?

@annevk
Member
annevk commented Apr 1, 2016

Also, what kind of feature would give the shadow script a pointer to itself?

@smaug----

it only points to the script while the script is executing though, no? So how can the outer world observe it?

Just what was explained earlier. Shadow script calling some non-shadow function which accesses document.currentScript. That is accidental exposure, and it can happen when explicitly calling non-shadow functions or dispatching events from shadow script and handling them elsewhere.
The event handling case may even end up exposing shadow DOM to other shadow DOMs etc.

@annevk
Member
annevk commented Apr 1, 2016

So what is your solution?

@smaug----

Also, what kind of feature would give the shadow script a pointer to itself?
Well, script execution would need to happen rather differently in shadow scripts, and we possibly don't want to do that. It would do effectively something like
(function(currentShadowScript) { /* the contents of the script would be here*/ } )(shadowScriptElement);

Hmm, but that brings in another question. What should happen to the variables defined in shadow scripts. Perhaps we should actually execute those scripts inside some anonymous scope.
(That would be similar to what has happened to Gecko internal frame scripts where we initially had scripts executing in the same scope but then moved to per script scope by default. The change was done when it became clear that different scripts were defining same variables and thus affecting to each others somewhat accidentally.)

@smaug----

So what is your solution?

My current solution is to not expose shadow-script elements in document.currentScript and making events scoped. And let scripts to live with that limitation.

@hayatoito
Member

I understand the point. However, I am afraid that we are worrying too much about this kind of exposure.
Unless the script (in a shadow tree) is calling such a trap function, this exposure never happens.

For me, it sounds "document.currentScript" is something like a thread-local variable. We do not have to stress on document used here. The point is that we have a global function which allows a script to access a thread-local variable.

It's difficult to decide where we should draw the line. However, in this particular case, I think it's okay that we have a global function which returns the script element which is currently running.

Due to the historical reason, this global function is defined in a "document".

I'm okay to restrict document.currentScript, however, we need an alternative global function for a script in a shadow tree, such as window.currentScript. WDYT?

My preferences:

[Preferred]

  1. Allow document.currentScript to return a script in a shadow tree (as long as the script is in a shadow-including document)
  2. Disallow document.currentScript to return a script in a shadow tree, but have an alternative, such as window.currentScript, to rescue a script in a shadow tree
  3. Disallow document.currentScript to return a script in a shadow tree. That's all.
    [Not preferred]
@domenic
Contributor
domenic commented Apr 4, 2016

Worrying about escape via shadow scripts calling non-shadow scripts seems similar to the many worries people have about closed shadow roots not being closed enough. (E.g., you can modify the running of script in those by overriding getters or setters on Object.prototype to inject your own logic.) The way to solve those kind of concerns is some kind of isolated scripting environment, in some subsequent "isolated shadow trees" proposal. But for open and closed shadow trees, I don't think it's worth trying to restrict. So I also support 1.

@rniwa
Contributor
rniwa commented Apr 4, 2016

What are use cases in which scripts inside a shadow DOM would need to use document.currentScript?

Until such use cases are presented, I'd assume such a feature is not needed. So I support option 3 since adding such a feature retroactively or reverting the behavior back to option 1 is easy while shipping option 1 today and changing it later to option 2 or option 3 would be backwards incompatible.

@hayatoito
Member

The use case for a script in a shadow tree should be the same to a use case for a script in a document tree. Is there any difference between them?

@rniwa
Contributor
rniwa commented Apr 4, 2016

@hayatoito what are those use cases?

@hayatoito
Member

document.currentScript.ownerDocument ? (as far as I spotted in the past...)

It looks there are a lot of other use cases, per http://stackoverflow.com/search?q=document.currentScript

@rniwa
Contributor
rniwa commented Apr 4, 2016

I'm not certain things listed there would apply to shadow DOM.

For example, http://stackoverflow.com/questions/32466974/using-document-currentscript-to-append-data-to-divs talks about appending data using attributes specified in the script element. Since there is no declarative syntax for shadow DOM, this wouldn't be a useful technique in practice. It would be much better to do the work using a custom element in the new web components world.

http://stackoverflow.com/questions/24986178/get-script-element-in-dom-tree/24986240, again, talks about loading JSON data from the server and generating a table out of it. That just doesn't happen with shadow DOM which lacks declarative syntax at the moment. Even if you had a declarative syntax, you wouldn't put data and script inside a shadow tree. You'd instead use a custom element with data inside of it, and then use shadowRoot.host to get that data instead.

http://stackoverflow.com/questions/19936425/dynamically-create-iframe-and-append-to-unknown-parent/19939766 is about creating an iframe when a script is loaded. Again, there is no need to insert a script element, then have it insert an iframe when creating a shadow tree. You should just do that upfront when you're creating a shadow tree. Alternatively, use a custom element to do it.

This post about "protecting" a JS object seems completely misguided: http://stackoverflow.com/questions/32739099/protecting-a-javascript-object-against-external-scripts/32739197

I don't think http://stackoverflow.com/questions/35608095/having-the-same-widget-on-a-page-multiple-time-variable-clashes/35608462#35608462 is anything to do with having to use document.currentScript. It's just badly written code overriding its own variables when ran multiple times. Using let for scoping would solve the problem, not document.currentScript.

http://stackoverflow.com/questions/403967/how-may-i-reference-the-script-tag-that-loaded-the-currently-executing-script/3695686 is about on-demand loading of other scripts within a script element, and I don't think you want to be doing that inside a shadow DOM either (it can result in each shadow DOM loading its own copy of the script). A better approach would be for whatever script creating a shadow tree to take care of it as its dependency. This is also a problem we ought be solving with ES6 modules and a new HTML-import replacement.

I've looked at a few other posts listed there but I don't think any of them benefit the use inside a shadow tree. They're either useless or actively harmful inside a shadow tree.

@smaug----

We would we let document.currentScript to return anything in shadow DOM when we don't let event.target to do that. And in case of event dispatching, shadow script doesn't need to call
any "trap" function explicitly, but just dispatch an event in such way that light DOM can get access to it.

To be honest, I'm rather surprised anyone even questions the ''document.currentScript shouldn't return shadow script elements."

@annevk
Member
annevk commented Apr 4, 2016

@smaug---- isn't event.deepPath() exactly the same? If a shadow tree event listener invokes a function in the light tree that invokes event.deepPath() won't it similarly leak?

@smaug----

That is quite different. event isn't any global object, so one needs to explicitly pass event in that state to some light DOM function. And no need to use deepPath() there. light DOM could just use event.currentTarget, or shadow listener could just pass some node to light dom function.

@annevk
Member
annevk commented Apr 4, 2016

@smaug---- quite a few implementations have window.event and that might yet leak to Gecko.

@smaug----

I would expect window.event to be null when event.currentTarget points to some shadow DOM node.

@hayatoito
Member

I'm wondering how a script in a shadow tree can access its root node (DocumentOrShadowRoot), if there is no way to access the script element.

If they can get the script element, they can access the root node easily, via currentScipt.rootNode, where a lot of useful APIs are available.

Thus, I'm wondering that window.currentRoot is what we should have? Just as a naive idea.
Is it okay to have such a context-aware API in Web? I am assuming that it's okay because we already have document.currentScript.

@esprehn
esprehn commented Apr 4, 2016

The use cases for this are the same for the main document, ex. reading data off the <script> element.

<script src="..." data-option1="..." data-option2="..."></script>

if you appendChild that to your shadowRoot, document.currentScript is needed for the script to be able to read back option attributes. I don't think we can invent a new or fancier context aware property, that requires tagging every function to see what script created it.

@sjmiles
sjmiles commented Apr 4, 2016

[Sorry I had verbiage here about the context of currentScript, but it was already covered, my bad].

The issue if I understand it then is primarily dispatching an event from shadow-root?

IMO, the leakage risk is less worrisome than unintended consequences of changing the behavior of currentScript from the traditional policy because of use-cases like Elliott mentions.

@rniwa
Contributor
rniwa commented Apr 4, 2016

The use cases for this are the same for the main document, ex. reading data off the <script> element.

<script src="..." data-option1="..." data-option2="..."></script>

Again, I don't think that's a useful thing to do inside a shadow DOM since that would mean that every instance of such a shadow DOM would try to fetch the script. Unless the script is cached and doesn't require validation, it would end up issuing a network request each time. A better approach would be using a custom element.

@travisleithead
Member

I support not restricting this in any way (it may be a source of closed shadow root leakage, but that is not really a problem).

@domenic domenic removed the needs consensus label Apr 5, 2016
@domenic
Contributor
domenic commented Apr 5, 2016

Telecon consensus: return null always (both open and closed shadow trees)

@domenic
Contributor
domenic commented Apr 5, 2016

See also whatwg/html#997. We also discussed the desire for a new API in modules that introduces some kind of currentScript variable into the module scope, but doesn't put it on the global which everyone can access.

@hayatoito hayatoito added html-dom and removed v1 labels Apr 7, 2016
@hayatoito hayatoito closed this in 08793be Apr 8, 2016
@rniwa
Contributor
rniwa commented May 5, 2016

I'm adding a test for this in w3c/web-platform-tests#2934.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment