[4.0] Make sure the renderer does not manipulate the inline CSS and JS by zero-24 · Pull Request #28719 · joomla/joomla-cms

zero-24 · 2020-04-18T09:55:37Z

Pull Request for Issue #28557

Summary of Changes

Make sure there renderer does not manipulate the inline CSS and JS

Testing Instructions

Add the following lines to the index.php of the cassiopea template:

// Add inline JavaScript
Factory::getDocument()->addScriptDeclaration('
    window.event("domready", function() {
        alert("An inline JavaScript Declaration");
    });
');

// Add inline Style
Factory::getDocument()->addStyleDeclaration('
	body {
		background: #00ff00;
		color: rgb(0,0,255);
	}
');

Enable CSP (System -> Content-Security-Policy -> Options) and configure like this:

visit the frontend
notice that the styles and scripts are blocked before the patch
apply the patch
notice it is working now

Expected result

inline script and inline style tags are not modified by the renderer and can be whitelisted via an csp.

Actual result

the style and script renderer add some spaces and line endings that breaks the CSP hash generation.

Documentation Changes Required

none

cc @wilsonge @SharkyKZ

zero-24 · 2020-04-18T10:21:32Z

Here is the backport to 3.x: #28720

Bodge-IT · 2020-04-18T16:27:52Z

Pls overlook my ignorance, isn't it cassiopea on J4?

zero-24 · 2020-04-18T16:40:00Z

Yes thanks fixed. I worked in parallel on protostar too :D

wilsonge · 2020-04-18T16:58:53Z

This is going to make the source look much worse when debugging. But then again given everyone uses dev tools in browsers these days I guess it doesn't matter much either?

zero-24 · 2020-04-18T17:24:27Z

This is going to make the source look much worse when debugging. But then again given everyone uses dev tools in browsers these days I guess it doesn't matter much either?

I have not noticed any difference in the dev tools.

zero-24 · 2020-04-18T17:25:09Z

BTW you should not use any inline JS or Inline CSS anyway so one more reason to not use them :D

Bodge-IT · 2020-04-18T18:13:17Z

Yes thans fixed. I worked in parallel on protostar too :D

Thanks Tobias, I need a bit more info as running into some wierdness.
With security policy enabled or disabled, I get this warning:
( ! ) Warning: hash() expects parameter 2 to be string, array given in F:\xampp\htdocs\j4test\plugins\system\httpheaders\httpheaders.php on line 178
Can I ignore?
Also, I'm testing this locally, that OK for this PR?

zero-24 · 2020-04-18T18:22:25Z

Yes you can ignore it. That is an issue that has been patched here too when you apply the patch that Warning should be gone too.

wilsonge · 2020-04-21T13:16:52Z

libraries/src/Document/Renderer/Html/ScriptsRenderer.php

 		// This is for full XHTML support.
 		if ($this->_doc->_mime !== 'text/html')
 		{
 			$buffer .= $tab . $tab . '//<![CDATA[' . $lnEnd;


Will this CData tag still work without the line break above? Honestly don't know enough here to be sure.

How to trigger that stuff anyway? Does joomla actually support something different than html and json?

In the template of your choice something like $this->setMimeEncoding('application/xhtml+xml')

Technically it does support these things - but I think it dates back to XHTML support. We can probably get rid of it (we should try and track back it's introduction first). But if we leave it in it should work

It seems to be there since more than 11 years: https://github.com/joomla/joomla-cms/blame/ac1822ffb8368d1e6be4c356d45088fd196c5e81/libraries/joomla/document/html/renderer/head.php#L105

Do you know how to go deeper than that commit here: 83939b8?

I mean in terms of out of scope I assume if you're running an xhtml mime type it won't work with csp ;)

Does this mean it can be removed?

@wilsonge @zero-24 ^^

I honestly have no clue. It’s probably safe. But I’m not certain either. I certainly have no clue if the CDATA tag has any use anymore - again I suspect it’s legacy. But I’m not certain

I mean in terms of out of scope I assume if you're running an xhtml mime type it won't work with csp ;)

Right we would not try to add hashes there we only do for HTML: https://github.com/joomla/joomla-cms/blob/4.0-dev/plugins/system/httpheaders/httpheaders.php#L140

vlaucht · 2020-08-04T08:22:58Z

I have tested this item ✅ successfully on 169e33e

Tested with Chrome on Win64

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28719.}

jmeintrup · 2020-08-04T08:49:12Z

I have tested this item ✅ successfully on 169e33e

Tested successfully.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28719.}

richard67 · 2020-08-05T07:53:43Z

@wilsonge @zero-24 I'm waiting with RTC until your above discussion is resolved.

Fedik · 2020-08-11T07:51:05Z

CDATA for xhtml/xml document, and as long as Document may be used for render XML I would keep it. Even if we never used it 😄

https://stackoverflow.com/a/66865/1000711

zero-24 · 2020-08-11T18:03:43Z

Ok right now to me this PR looks ready than given that I do not touch the CDATA thing and that we only apply hashes on HTML sites anyway.

wilsonge · 2020-08-11T18:11:39Z

I'm still not 100% we're getting this right - but let's give it a go

zero-24 · 2020-08-11T18:13:47Z

Thanks please merge the backport too: #28720

…om_templates * '4.0-dev' of github.com:joomla/joomla-cms: Add closing quote (joomla#30353) [4.0] Make sure the renderer does not manipulate the inline CSS and JS (joomla#28719) [4.0] Atum and Cassiopea Change alerts styling - space saving + (joomla#30294) Update editor versions (joomla#30340) [4.0][mod_menu] Add space for 'menu class' (joomla#30341) Optimize code for aria-current (joomla#30328) [4.0] Composer and npm updates 10 Aug (joomla#30334) [4.0] fix js code style (joomla#30335)

joomla#28719)

zero-24 added 2 commits April 18, 2020 11:44

make sure there rendere does not manipulate the inline CSS and JS

53d4538

extend the array patch for styles too

e2bf53a

joomla-cms-bot added the PR-4.0-dev label Apr 18, 2020

zero-24 mentioned this pull request Apr 18, 2020

[4.0] Script/Style hash validation broken #28557

Closed

zero-24 added the Release Blocker label Apr 18, 2020

zero-24 added this to the Joomla 4.0 milestone Apr 18, 2020

zero-24 added 3 commits April 18, 2020 12:07

fix auto mode

da87782

we need to check both

c3a0c3a

styles

b50d48f

zero-24 added a commit to zero-24/joomla-cms that referenced this pull request Apr 18, 2020

backport joomla#28719 to 3.x

1e51b54

zero-24 mentioned this pull request Apr 18, 2020

[3.x] Backport Make sure the renderer does not manipulate the inline CSS and JS #28720

Closed

richard67 changed the title ~~[4.0] Make sure there rendere does not manipulate the inline CSS and JS~~ [4.0] Make sure there renderer does not manipulate the inline CSS and JS Apr 18, 2020

richard67 changed the title ~~[4.0] Make sure there renderer does not manipulate the inline CSS and JS~~ [4.0] Make sure the renderer does not manipulate the inline CSS and JS Apr 18, 2020

wilsonge reviewed Apr 21, 2020

View reviewed changes

joomla-cms-bot removed the Release Blocker label Jun 12, 2020

joomla-cms-bot removed this from the Joomla 4.0 milestone Jun 12, 2020

jwaisner added the Release Blocker label Jun 12, 2020

Merge branch '4.0-dev' into fix_style_script_renderer

169e33e

zero-24 added this to the Joomla 4.0 milestone Jun 26, 2020

roland-d added the Information Required label Aug 5, 2020

wilsonge merged commit f64ddd2 into joomla:4.0-dev Aug 11, 2020

zero-24 deleted the fix_style_script_renderer branch August 11, 2020 18:12

wilsonge removed the Release Blocker label Sep 22, 2020

sakiss pushed a commit to sakiss/joomla-cms that referenced this pull request Oct 16, 2020

[4.0] Make sure the renderer does not manipulate the inline CSS and JS (

faade61

joomla#28719)

Uh oh!

Comments

Conversation

zero-24 commented Apr 18, 2020 • edited by richard67 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

Uh oh!

zero-24 commented Apr 18, 2020

Uh oh!

Bodge-IT commented Apr 18, 2020

Uh oh!

zero-24 commented Apr 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilsonge commented Apr 18, 2020

Uh oh!

zero-24 commented Apr 18, 2020

Uh oh!

zero-24 commented Apr 18, 2020

Uh oh!

Bodge-IT commented Apr 18, 2020

Uh oh!

zero-24 commented Apr 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vlaucht commented Aug 4, 2020

Uh oh!

jmeintrup commented Aug 4, 2020

Uh oh!

richard67 commented Aug 5, 2020

Uh oh!

Fedik commented Aug 11, 2020

Uh oh!

zero-24 commented Aug 11, 2020

Uh oh!

wilsonge commented Aug 11, 2020

Uh oh!

zero-24 commented Aug 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

zero-24 commented Apr 18, 2020 •

edited by richard67

Loading

zero-24 commented Apr 18, 2020 •

edited

Loading