[MAGNOLIA-3805] Page titles are not HTML encoded if a user cancels the inline editing in the tree Created: 22/Aug/11  Updated: 16/Sep/11  Resolved: 07/Sep/11

Status: Closed
Project: Magnolia
Component/s: admininterface
Affects Version/s: 4.4.4
Fix Version/s: 4.4.5

Type: Bug Priority: Critical
Reporter: Rory Gibson Assignee: Ondrej Chytil
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:

 Description   

Steps to reproduce:

  • Access the Magnolia Admin and create a new page within the Website structure with the title set to '<b>test</b>'
  • Select the title attribut for the page so that it can be edited and then select another section of the page
  • At this stage the title is shown with the html code being executed.

This could be potentially harmful to the system.



 Comments   
Comment by Magnolia International [ 23/Aug/11 ]

Are we talking about STK templates ? Is this reproducible with all templates ?

Comment by Rory Gibson [ 23/Aug/11 ]

Hi Gregory
This isn't in a template - it's the Admin UI.

  • Go to the website tree
  • right click on root and add new page
  • double-click on the page title
  • enter '<em>MyNewPage</em>'
  • press enter
  • title is rendered as "<em>MyNewPage</em>" (i.e. correct, you see the escaped HTML)
  • double-click on 'MyNewPage'
  • change to '<strong>BrokenTitle</strong>'
  • now - don't press enter - instead, click away to somewhere else in the tree (blur the focus from the rendered input field)
  • title will be rendered in bold, instead of escaped

I've also tried entering '<strong onclick='alert()'>some text</strong>' and this results in an alert box appear upon click on the title.

It looks like validation is different on blur than submit.

Also - if you enter two HTML'ed titles, pressing enter for each one (so that they are rendered normally, escaped), then, when you break one of them by blurring focus, and go back into the other one, the behaviour upon pressing Enter is now incorrect. Looks like there may be something being stored statically that's being affected.

Comment by Rory Gibson [ 23/Aug/11 ]

Note: i've just replicated this in the demoauthor instance - it should be there for the next few minutes.

Comment by Magnolia International [ 23/Aug/11 ]

Oh wow ok, I get it now.
Probably too late to get it into 4.4.5, but I'll try to push this in the next bugfix release.
Thanks for the report !

Comment by Magnolia International [ 23/Aug/11 ]

Tentatively scheduled for 4.4.5 but might be bumped to next version.
Rory, if you had a patch, that'd of course change the plans

Comment by Jan Haderka [ 07/Sep/11 ]

don't use escape ... there must be a better way.

Comment by Richard Unger [ 16/Sep/11 ]

Hi Gregory, hi Jan,

We're having some serious discussions regarding security that are related to this bug:

In a recent internal security check (we're currently using 4.4.2 EE) we also noticed that it is possible to insert javascripts into the title fields in the admin trees. The inserted scripts are executed within the backend, already a problem in itself, but are also executed in the front-ends, when the title fields are rendered in the web-pages (this, of course depends on how the templates are written, admittedly).

One question is, of course, whether it makes sense to secure the back-end against the editors, who are after all authorized users, and should be trusted.

But setting aside this question, which is more philosophical than technical, I have the following technical question:

Examining the subversion commits related to this bug it seems the only modification associated with this bug-fix is to the file "/magnolia-module-admininterface/src/main/resources/mgnl-resources/admin-js/tree.js".

If this is the case, then I contend that this is not a fix, but merely a "cosmetic change".

If we're assuming "evil editors", attacking the system with javascript, they can simply use firebug (or similar tools) to modify the behaviour of tree.js, and disable your angle-bracket removal code. Or they can skip past the all checks by intercepting the communication resulting from a tree-edit, and supplying modified values after all checks have been perfomed.

To really fix this bug there needs to be a change on the server side, and inputs need to be escaped by the server, before storing to JCR.

I'd appreciate feedback on whether the subversion commits attached to this bug are "complete", and I am correct in my assumtion that the only change that was made is in the java-scripts, i.e. client-side...

Thanks!

Regards from Vienna,

Richard

Comment by Jan Haderka [ 16/Sep/11 ]

Hi Richard,

even if you subvert the javascript in the browser you can do so only for your browser, since script is executed even when reading values from server to display them in the tree. This way you are able to store the html in the node value when necessary, but it would still not execute for other users in admin central.

That being said, we are still preparing solution on the server side, but since sometimes (e.g. html paragraph) it is perfectly valid to store the pure html, we can's just blindly switch it off for all.

Regards,
Jan

Comment by Richard Unger [ 16/Sep/11 ]

Hi Jan,

ok, thank you very much - I understand.

Thanks also for the information on this and related issues today, that was very helpful,

Regards from Vienna,

Richard

Generated at Mon Feb 12 03:49:50 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.