This was not taken care of in #1174756: Convert <head> markup to HTML5 to limit the scope of work, but the attribute is not needed and should be removed. See http://dev.w3.org/html5/spec/Overview.html#attr-script-type and http://www.whatwg.org/specs/web-apps/current-work/multipage/scripting-1....

CommentFileSizeAuthor
#28 no-mail-test-1362974-28.patch10.44 KBdcam
#24 remove-text-js-reroll-1362974-24.patch12.25 KBdcam
#24 no-xss-test-1362974-24.patch11.28 KBdcam
#14 remove_text_js_reroll-1362974-14.patch8.75 KBtkrajcar
#8 remove_text_js.patch9.38 KBdroplet
#5 remove_text_javascript.patch9.06 KBdroplet
#1 text.javascript.patch9.09 KBJacine
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented
Status: Active » Needs review
FileSize
text.javascript.patch9.09 KB

Here's a first pass at a patch. Hopefully it passes. :P

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented 
+++ b/core/includes/common.incundefined
@@ -4205,9 +4205,7 @@ function drupal_pre_render_scripts($elements) {
+    '#attributes' => array(),

BTW, I'm not sure whether or not we need this line. It's there because I remember hearing something about it needing an empty array.... Need to come back to this and make sure.

rupl’s picture

rupl
he/him
Primary language English
CreditAttribution: rupl commented

@Jacine, yes it needs to be there. Relevant comment from other issue: http://drupal.org/node/1174756#comment-4759566

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented

Ha, thanks for tracking that down @rupl! I knew I read it somewhere ;)

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs review » Needs work
FileSize
remove_text_javascript.patch9.06 KB

It looks good. but what happen if we remove it:

+++ b/core/includes/common.incundefined
@@ -4205,9 +4205,7 @@ function drupal_pre_render_scripts($elements) {
+    '#attributes' => array(),

testBot!! Please tell me :)

-3 days to next Drupal core point release.

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, remove_text_javascript.patch, failed testing.

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs work » Needs review
FileSize
remove_text_js.patch9.38 KB
cosmicdreams’s picture

cosmicdreams CreditAttribution: cosmicdreams commented

looks like a nice patch. Does it need manual testing? I'll see if a reroll is necessary but this looks like it should be on a fast track to RTBC

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs review » Needs work

I think no manual testing needed.

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented

@droplet Did you mean to set this to NW?

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs work » Needs review

oh, no. dedtior script changed :(

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented
Status: Needs review » Needs work
Issue tags: +Novice

This patch needs a reroll. Tagging novice to do a simple rebase:

git checkout -b 1362974-script-html5 25ef3c917fc3c8daf502
git apply [the-patch-from-#8]
git commit -a
git rebase 8.x
git diff 8.x > [rerolled-patch-file-name]

Other than that, having:

+++ b/core/modules/system/system.testundefined
@@ -1355,10 +1355,10 @@ class PageTitleFiltering extends DrupalWebTestCase {
-    $slogan = '<script type="text/javascript">alert("Slogan XSS!");</script>';
+    $slogan = '<script>alert("Slogan XSS!");</script>';

Not sure if removing type="text/javascript" from the XSS tests is applicable - but that shouldn't make a difference anyway. Should we have the html corrector remove type="text/javascript" as a follow-up?

tkrajcar’s picture

tkrajcar CreditAttribution: tkrajcar commented
Status: Needs work » Needs review
FileSize
remove_text_js_reroll-1362974-14.patch8.75 KB

Rerolled.

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented

Thank you @tkrajcar. I'd totally RTBC this after this is clear: Do we now need #attributes => array() for anything, or not?

tkrajcar’s picture

tkrajcar CreditAttribution: tkrajcar commented

@Niklas Fiekas, see #3 for the link to the convo where it is needed. I'm not savvy enough about the theme layer to know whether that's accurate or not, but that's what was referenced earlier. :-)

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented

Yeah ... me neither. The patch is green - so I wonder whether this is not true for our case.

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs review » Reviewed & tested by the community

@#13,

keep the XSS is not a bad idea. but I think somewhere we already have tests covered those attributes.

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented

Yeah. RTBC seconded.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

@Niklas Fiekas We might be needing #attributes later on, just not at this level.
agree with rtbc.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs work 
Not sure if removing type="text/javascript" from the XSS tests is applicable - but that shouldn't make a difference anyway. Should we have the html corrector remove type="text/javascript" as a follow-up?

Yeah I don't think this is the place to do that, XSS attacks might include type="text/javascript" so if anything I'd add to the existing tests rather than changing them. Either way can we split that to a new issue?

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs work » Reviewed & tested by the community

XSS tests in CORE are so weak. only tested "SCRIPT" tag in title & path.

It used check_plain to compare the result, remove the type attribute is no harms. and a separated issue to enhance XSS test cases is required.

back to RTBC. ( welcome switch back to needs work, and I will reroll it again )

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs work

Yeah sorry I'd really prefer to do that in another issue, even if it's harmless it is going to make for weird commit history.

dcam’s picture

dcam CreditAttribution: dcam commented
Status: Needs work » Needs review
FileSize
no-xss-test-1362974-24.patch11.28 KB
remove-text-js-reroll-1362974-24.patch12.25 KB

If I'm understanding the discussion correctly, the changes to testTitleXSS() needed to be removed and handled in a separate issue.

The first patch is a reroll of #14 since the test files were moved. Plus, I removed type='text/javascript' from a few more lines that had been added to JavaScriptTest.php.

The second patch is the same as the first, but with the changes to the XSS tests in PageTitleFilteringTest.php removed.

dcam’s picture

dcam CreditAttribution: dcam commented

#24: no-xss-test-1362974-24.patch queued for re-testing.

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs review » Reviewed & tested by the community

no-xss-test-1362974-24.patch reviewed.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs work

Sorry same for this test, no need to remove the text/javascript there either:

--- a/core/modules/system/lib/Drupal/system/Tests/Mail/HtmlToTextTest.php
+++ b/core/modules/system/lib/Drupal/system/Tests/Mail/HtmlToTextTest.php
@@ -153,7 +153,7 @@ class HtmlToTextTest extends WebTestBase {
       // Tests some unsupported HTML tags.
       '<html>Drupal</html>' => "Drupal\n",
       // @todo Perhaps the contents of <script> tags should be dropped.
-      '<script type="text/javascript">Drupal</script>' => "Drupal\n",
+      '<script>Drupal</script>' => "Drupal\n",
       // A couple of tests for Unicode characters.
       '<q>I <em>will</em> be back…</q>' => "I /will/ be back…\n",
       'FrançAIS is ÜBER-åwesome' => "FrançAIS is ÜBER-åwesome\n",
dcam’s picture

dcam CreditAttribution: dcam commented
Status: Needs work » Needs review
FileSize
no-mail-test-1362974-28.patch10.44 KB

Removed the Mail module test.

dcam’s picture

dcam CreditAttribution: dcam commented

#28: no-mail-test-1362974-28.patch queued for re-testing.

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs review » Reviewed & tested by the community

Confirm that removed.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Less bytes, wooo!

Committed/pushed to 8.x.

Automatically closed -- issue fixed for 2 weeks with no activity.