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

Add <script nomodule> to prevent script evaluation #2261

Merged
merged 4 commits into from Jan 31, 2017
Merged

Add <script nomodule> to prevent script evaluation #2261

merged 4 commits into from Jan 31, 2017

Conversation

domenic
Copy link
Member

@domenic domenic commented Jan 11, 2017

This allows selective delivery of classic scripts only to older user
agents. Fixes #1442.

/cc @whatwg/modules

This is meant to give something concrete to discuss. As @jakearchibald pointed out, it's important that if we do this, everyone does it, so let's try to get some sign-off from all vendors.

This allows selective delivery of classic scripts only to older user
agents. Fixes #1442.
@domenic domenic added addition/proposal New features or enhancements do not merge yet Pull request must not be merged per rationale in comment needs implementer interest Moving the issue forward requires implementers to express interest labels Jan 11, 2017
<span data-x="module script">module scripts</span>. This allows selective execution of <span
data-x="module script">module scripts</span> in modern user agents and <span data-x="classic
script">classic scripts</span> in older user agents, <a href="#script-nomodule-example">as shown
below</a>.</p>
Copy link
Member Author

@domenic domenic Jan 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably add document conformance requirements that this not be specified on module scripts.

content attribute, then abort these steps. The script is not executed.</p>

<p class="note">This applies even to scripts with their <code
data-x="attr-script-type">type</code> attribute set to "<code data-x="">module</code>".</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was surprising to me. Why make it apply to module scripts also?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I interpret "nomodule" as "do not run this in browsers that support modules", so that's why I went with this behavior. It should be a document conformance error anyway to use both, BTW.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, ok... I sort of expect a model where attributes are ignored in contexts where they don't make sense -- e.g. <input type="text" src="foo"> ignores the src attribute because type is not image (and it's non-conforming to specify src there). But maybe there are cases that I'm not thinking about that don't follow this model?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the attribute index and couldn't find another attribute that has an effect when it "shouldn't" apply. So I think we should check if it's a classic script here.

@Constellation
Copy link
Member

Constellation commented Jan 12, 2017

I think this is the right direction.
When defining <script type="module">, we do not select <module> tag due to the backward compatibility: <module> will be shown as the unknown HTML tag in the browsers not supporting modules.
I think the essence of this issue is the same. We should have the way to use <script type="module"> without ignoring the older browsers.

@rniwa Any thoughts?

@rniwa
Copy link
Collaborator

rniwa commented Jan 12, 2017

This feature & approach sane to me. Note that type=module is already in Safari Tech Preview 21.

@@ -57059,6 +57060,7 @@ interface <dfn>HTMLDialogElement</dfn> : <span>HTMLElement</span> {
interface <dfn>HTMLScriptElement</dfn> : <span>HTMLElement</span> {
[<span>CEReactions</span>] attribute USVString <span data-x="dom-script-src">src</span>;
[<span>CEReactions</span>] attribute DOMString <span data-x="dom-script-type">type</span>;
[<span>CEReactions</span>] attribute boolean <span data-x="dom-script-noModule">noModule</span>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this forward-compatible enough with a potential wasm dependency?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we’d end up adding noWASM as well? Maybe we want something more generic like nosupport="module wasm"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@domenic had nomodule=wasm in the original issue, but perhaps we shouldn't worry about it. Initial wasm usage needs to be deployed through JavaScript anyway and there's also the possibility of reviving content negotiation for it.

Copy link
Collaborator

@rniwa rniwa Jan 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might be a little shortsighted though. I’m sure the next step for WASM is to load it directly in browser, and presumably, we’d be using type=wasm or something.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if they use content negotiation as the opt-in the markup wouldn't have to change necessarily. But if we do add type=wasm or some such, we could add nowasm as @domenic suggested.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if we load the WASM code with type="module"? IIUC, ES6 modules will handle WASM as well (maybe, based on content type): For example, import A from "./A.wasm" will work well.
http://webassembly.org/docs/modules/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, what @Constellation alludes to is what I meant by content negotiation above. And that's the route Mozilla's wasm folks want to take.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One reason I didn't go with a more generic syntax is it opens up a bunch of new cases that need to be nailed down and tested. E.g.

<script nosupport=module type=module src="foo.js"></script>
<script nosupport=text/javascript type=module src="foo.js"></script>
<script nosupport=text/livescript type=module src="foo.js"></script>
<script nosupport=text/javascript src="foo.js"></script>
<script nosupport=text/x-handlebars src="foo.js"></script>
<script nosupport=wasm type="module" src="foo.wasm"></script>

You can think up reasonable answers for all of these, but it's extra work for everyone involved. And especially since wasm will likely use type=module anyway, the idea that nosupport should take a type is probably not going to work.

Even if we say that the "support" values are different from the "type" values, there's also the case that we probably would want to allow loading a script only if both wasm and modules are not supported, so now we need to parse space-delimited tokens or similar:

<script nosupport="module wasm" src="es5fallback.js"></script>
<script nosupport="wasm" type="module" src="es6fallback.js"></script>
<script type="module" src="whatireallywanttoload.wasm"></script> <!-- fails to load in non-wasm browsers due to unrecognized MIME type -->

Again, more complexity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New boolean attributes for each new thing seems simplest and best to me. I would expect the need for such new things to be pretty rare.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nosupport="module wasm" is definitely extensible

@domenic
Copy link
Member Author

domenic commented Jan 12, 2017

Updated with authoring requirements and to ignore the attribute if type="module", per @zcorpan's advice at #2261 (comment) .

@rniwa @Constellation I am going to tentatively proceed with nomodule instead of a more general syntax, as discussed in #2261 (comment) , and check the WebKit supports box. But let me know if you are not yet convinced and we can continue to discuss.

I will work on web platform tests next.

@zcorpan zcorpan added needs tests Moving the issue forward requires someone to write tests and removed needs implementer interest Moving the issue forward requires implementers to express interest labels Jan 12, 2017
@rniwa
Copy link
Collaborator

rniwa commented Jan 12, 2017

WebKit bug: https://bugs.webkit.org/show_bug.cgi?id=166987

@annevk
Copy link
Member

annevk commented Jan 13, 2017

I filed a Gecko bug indicating that the time to object is now: https://bugzilla.mozilla.org/show_bug.cgi?id=1330900.

@Constellation
Copy link
Member

Note that the patch is ready in WebKit now. https://bugs.webkit.org/show_bug.cgi?id=166987 Once we all get agreement, I'll land it.

@domenic domenic removed the do not merge yet Pull request must not be merged per rationale in comment label Jan 16, 2017
@zcorpan
Copy link
Member

zcorpan commented Jan 19, 2017

@Constellation it looks like the WebKit patch includes some tests; do you think you could convert them to testharness.js tests and submit to https://github.com/w3c/web-platform-tests ? This pull request is waiting on tests. 🙂

@Constellation
Copy link
Member

@zcorpan sounds fine!

@domenic
Copy link
Member Author

domenic commented Jan 24, 2017

@Constellation have you started work on porting the nomodule tests to web-platform-tests already? If not, I can do it tomorrow :). But I don't want to duplicate work if you have started already.

@Constellation
Copy link
Member

@domenic oh, sorry, I do not start working on that yet.
I appreciate it if you can work on that :D

@rniwa
Copy link
Collaborator

rniwa commented Jan 24, 2017

I'll do the conversion.

hubot pushed a commit to WebKit/WebKit-http that referenced this pull request Jan 24, 2017
https://bugs.webkit.org/show_bug.cgi?id=166987

Reviewed by Sam Weinig.

Source/WebCore:

As discussed on whatwg/html#2261, we should have
the way to suppress classic script execution when our user agent have
modules support. With such a feature, developers can write the code like,

    <script type="module" src="./app.js"></script>
    <script nomodule src="./bundled-app.js"></script>

In the above code, if the user agent does not support modules, the bundled-app.js
will be executed. On the other hand, if the user agent supports modules, we should
ignore the script tag which has the `nomodule` attribute.
This way allows us to support the legacy browsers while using modules.

In WebKit, we already support modules. Thus, we should ignore the classic script
attributed `nomodule`.

We also rename asyncAttributeValue and deferAttributeValue to hasAsyncAttribute and
hasDeferAttribute.

Tests: js/dom/modules/nomodule-has-no-effect-on-module-inline.html
       js/dom/modules/nomodule-has-no-effect-on-module-src.html
       js/dom/modules/nomodule-prevents-execution-classic-script-inline.html
       js/dom/modules/nomodule-prevents-execution-classic-script-src.html

* dom/ScriptElement.cpp:
(WebCore::ScriptElement::prepareScript):
* dom/ScriptElement.h:
* html/HTMLAttributeNames.in:
* html/HTMLScriptElement.cpp:
(WebCore::HTMLScriptElement::hasAsyncAttribute):
(WebCore::HTMLScriptElement::hasDeferAttribute):
(WebCore::HTMLScriptElement::hasNoModuleAttribute):
(WebCore::HTMLScriptElement::asyncAttributeValue): Deleted.
(WebCore::HTMLScriptElement::deferAttributeValue): Deleted.
* html/HTMLScriptElement.h:
* html/HTMLScriptElement.idl:
* svg/SVGScriptElement.cpp:
(WebCore::SVGScriptElement::hasAsyncAttribute):
(WebCore::SVGScriptElement::hasDeferAttribute):
(WebCore::SVGScriptElement::hasNoModuleAttribute):
(WebCore::SVGScriptElement::asyncAttributeValue): Deleted.
(WebCore::SVGScriptElement::deferAttributeValue): Deleted.
* svg/SVGScriptElement.h:

LayoutTests:

* js/dom/modules/nomodule-dynamic-classic-inline-expected.txt: Added.
* js/dom/modules/nomodule-dynamic-classic-inline.html: Added.
* js/dom/modules/nomodule-dynamic-classic-src-expected.txt: Added.
* js/dom/modules/nomodule-dynamic-classic-src.html: Added.
* js/dom/modules/nomodule-has-no-effect-on-module-inline-expected.txt: Added.
* js/dom/modules/nomodule-has-no-effect-on-module-inline.html: Added.
* js/dom/modules/nomodule-has-no-effect-on-module-src-expected.txt: Added.
* js/dom/modules/nomodule-has-no-effect-on-module-src.html: Added.
* js/dom/modules/nomodule-prevents-execution-classic-script-inline-expected.txt: Added.
* js/dom/modules/nomodule-prevents-execution-classic-script-inline.html: Added.
* js/dom/modules/nomodule-prevents-execution-classic-script-src-expected.txt: Added.
* js/dom/modules/nomodule-prevents-execution-classic-script-src.html: Added.
* js/dom/modules/nomodule-reflect-expected.txt: Added.
* js/dom/modules/nomodule-reflect.html: Added.
* js/dom/modules/script-tests/error-classic-script.js: Added.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@211078 268f45cc-cd09-0410-ab3c-d52691b4dbfc
rniwa added a commit to rniwa/web-platform-tests that referenced this pull request Jan 24, 2017
This feature is being added in whatwg/html#2261

Converted the tests in https://trac.webkit.org/r211078 to use testharness.js,
refined the test for IDL attribute reflecting content attribute, and re-organized tests.
rniwa added a commit to rniwa/web-platform-tests that referenced this pull request Jan 24, 2017
This feature is being added in whatwg/html#2261

Converted the tests in https://trac.webkit.org/r211078 to use testharness.js,
refined the test for IDL attribute reflecting content attribute, and re-organized tests.
rniwa added a commit to rniwa/web-platform-tests that referenced this pull request Jan 30, 2017
This feature is being added in whatwg/html#2261

Converted the tests in https://trac.webkit.org/r211078 to use testharness.js,
refined the test for IDL attribute reflecting content attribute, and re-organized tests.
rniwa added a commit to rniwa/web-platform-tests that referenced this pull request Jan 31, 2017
This feature is being added in whatwg/html#2261

Converted the tests in https://trac.webkit.org/r211078 to use testharness.js,
refined the test for IDL attribute reflecting content attribute, and re-organized tests.
@zcorpan zcorpan removed the needs tests Moving the issue forward requires someone to write tests label Jan 31, 2017
@zcorpan
Copy link
Member

zcorpan commented Jan 31, 2017

Tests are reviewed, I think we're all set.

domenic pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 31, 2017
This feature is being added in whatwg/html#2261

Converted the tests in https://trac.webkit.org/r211078 to use testharness.js,
refined the test for IDL attribute reflecting content attribute, and re-organized tests.
@domenic domenic merged commit a828019 into master Jan 31, 2017
@domenic domenic deleted the nomodule branch Jan 31, 2017
@Constellation
Copy link
Member

Great!

zcorpan added a commit to web-platform-tests/wpt that referenced this pull request Feb 1, 2017
zcorpan added a commit to web-platform-tests/wpt that referenced this pull request Feb 1, 2017
zcorpan added a commit to web-platform-tests/wpt that referenced this pull request Feb 2, 2017
zcorpan added a commit to web-platform-tests/wpt that referenced this pull request Feb 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements
Development

Successfully merging this pull request may close these issues.

None yet

6 participants