An Open angle bracket < in custom field within Manager Form crashes the save process

[donShakespeare]

An open angle bracket < in custom field (from MODx.onDocFormRender or however) within Manager Form crashes the save process, but the content is still saved on the server without error

MODX 2.6.0 and below

To Reproduce

1. Please insert custom field using this tut
   https://docs.modx.com/revolution/2.x/case-studies-and-tutorials/adding-custom-fields-to-manager-forms
2. Then alter the content of said field to have an open angle bracket.
3. Then save the resource

Quick Reproduce

1. Open a native MODX resource in the Manager
2. Run code in browser's JS dev console
3. Save the resource

 var modxForm = document.getElementsByTagName('form')[0];
  var newTextarea = document.createElement("textarea");
  newTextarea.setAttribute('id', 'mynewTextArea');
  newTextarea.setAttribute('name', 'mynewTextArea');
  newTextarea.value='<THIS__WILL__CRASH'; //crashes when you save the resource
  modxForm.appendChild(newTextarea);

[donShakespeare]

My current solution is

JS

document.getElementById("myEl").value = content.replace(/(?:<)/g, "&lt++;");
 //snuff out all opening angle brackets with custom entity

PHP

$content = str_replace("&lt++;", "<", $content);
//remove your custom entity before saving resource

[gadgetto]

Problem is related to this:
https://forums.modx.com/thread/95636/mxfb---success-message-rte-saving-error

and this:
https://forums.modx.com/thread/103273/solved-extjs-problem-while-saving-content-of-textarea-contains-html-tags

[Finetuned]

After looking into this in modxbughunt#3, the issue is due to Ext.data.Connection truncating the json which arrives intact from modconnectorresponseclass->outputContent().

So it is already invalid before it arrives into utilities.js -> Ext.override(Ext.form.Action.Submit.... handleResponse() where the error is raised.

The ExtJS docs for Ext.data.Connection state that:

Characters which are significant to an HTML parser must be sent as HTML entities...

So the content from custom fields must be processed before it gets sent to the server.

The error is not encountered if a closing bracket is present.

[OptimusCrime]

Nice detective work. Is there any way to solve this issue, or is the issue in Extjs itself?

[Finetuned]

It's in Ext. It creates a iframe and drops the response text into the body. I am thinking to override the class to add a <textarea> into the iframe. The response would be put into that and not be parsed as html.

[OptimusCrime]

Sounds tricky. Perhaps this is impossible to deal with properly until we can get rid of extjs or bump its version?

[Finetuned]

It shouldn't be that complicated although it will need some checking for side-effects.

[OptimusCrime]

@Finetuned What do you mean? If the problem is caused by Extjs and only Extjs, there is no way for us to fix it currently.

[Mark-H]

As there's no more updates, I think we can make a bugfix in the depths of extjs if needed. Or maybe there's a way to override it in a different way. Perhaps someone already has a fix for it somewhere on the web?

[Finetuned]

I wouldn't change the library itself as Ext.override() is available for this.
Other manager overrides to Ext can be found in manager/assets/modext/util/utilities.js

[Finetuned]

Ok, so I've taken a different approach to this:

When the JSON response comes back and is truncated by the browser, we catch this in ExtJS and replace the truncated responseText with an empty fallback response:

Truncated json:
{"success":true,"message":"","total":0,"data":[],"object":{"id":1,"type":"document","contentType":"text\/html","alias":"index","published":true,"pub_date":0,"unpub_date":0,"parent":0,"isfolder":false,"richtext":true,"template":1,"menuindex":0,"searchable":true,"cacheable":true,"createdby":1,"createdon":"2018-03-07 22:07:38","editedby":1,"editedon":"2018-03-13 09:29:30","deleted":false,"deletedon":0,"deletedby":0,"publishedon":0,"publishedby":0,"donthit":false,"privateweb":false,"privatemgr":false,"content_dispo":0,"hidemenu":false,"class_key":"modDocument","context_key":"web","content_type":1,"uri":"index.html","uri_override":0,"hide_children_in_tree":0,"show_in_tree":1,"create-resource-token":"5aa79528bc5228.77657389","reloaded":"0","parent-original":"0","parent-cmb":"","syncsite":1,"mynewTextArea":"

Fallback Response:

{ "success":true, "message":"", "total":0, "data":[], "object":{"error": "e.g. The response object was truncated due to unescaped html entities"} },

This is all written and working but I'd like some input on the UX and error handling

It would be possible to repair the damaged object thus providing a partial response but I wonder if this is worth the effort/overhead?

The alternatives are to

1. notify the user that although a successful response was returned from the server, ExtJS messed it up.
2. add the above object.error value to the error log.

Given that this process is not performed using a real xmlhttprequest, the faked response wrapper never returns a failure. We don't know at this point whether the save was performed successfully so I suggest we use the error log and let the response pass through to any listeners that can deal with it as they wish.

As far as the UX goes, I initially thought to use the following lexicon entries but they don't really completely explain the problem (due to the fake ajax explained above) and the successful save of data on the server side:

MODx.lang.warning > 'Warning!'
MODx.lang.resource_err_save > 'An error occurred while trying to save the resource.'
MODx.lang.invalid_data > 'Invalid data'

[Jako]

I think it is possible to add some code in ,listeners: { 'beforeSubmit': of the form handling ExtJS code that encodes all 'htmlspecialchars' in those custom fields before they are submitted.

[Mark-H]

If I understand correctly @Jako - the problem is in the response handling, not sending the request. Encoding all entities is also going to break saving basic things like chunks/templates which should not be encoded.

[Jako]

Sorry, I missed the two ++ after the &lt in the code of @donShakespeare and thought he solved it by encoding the custom fields with a sort of 'htmlspecialchars'.

If I read the modConnectorResponse code right, the work (htmlspecialchars on the custom fields) has to be done in the processor called with runProcessor. Since @donShakespeare wants to save the the field in a modResource, the custom field could be processed with htmlspecialchars in onDocFormSave, which happens after the resource is saved.

[Finetuned]

@Jako Mark is right, the submitted form is processed fine by the server and a correct response is returned. It's the way Ext.data.Connection implements this that is causing the issue: in Don's case, the unmatched < is truncated by the browser when it renders the response. Their documentation details this in the introduction to that class.

[Finetuned]

I've pushed the changes on my [branch] (2.x...Finetuned:bug-13711).
It's the last 4 commits from today's date (14th March) that provide the fix.

Hopefully it will make things easier to discuss

[Jako]

So the question is: What is different between the default resource fields (that could contain this < sign to) and the custom field. The answer is quite simple: The resource fields are not returned and the custom field is returned. So the custom field has to be stripped away (unset) in onDocFormSave from the processor response. @gadgetto did the same in his code.

[Finetuned]

@Jako, the difference, for me at least, is that the standard resource fields are known in advance (hardcoded into update.class.php in the cleanup function ). This problem extends beyond Don's specific use case: an unknown number of custom fields with unknown names are possible.

This is why I'm suggesting passing through a warning in place of a truncated response. The warning makes escaping html entities the CMP developer's responsibility. They know best what their specific CMP is trying to achieve and can react according to the context of their CMP.

[donShakespeare]

@gadgetto can you remember/share some of our experiments on that day? I failed to logged them here. Might help in the thorough solving of this issue.
I believe you extended the update class and used the cleanup function to straighten your own specific issues.

This cleanup method is the 'official' one, but one seemingly too advanced for certain users doing simple things as found in the MODX tutorial.

[gadgetto]

@donShakespeare please read this: https://forums.modx.com/thread/103273/solved-extjs-problem-while-saving-content-of-textarea-contains-html-tags

Should explain what I did!

[Mark-H]

I may have found a simpler fix than #13810 for this.

The problem remains that ExtJS uses an iframe to submit requests. The response in that iframe is processed by the browser, causing HTML to break the (otherwise valid) JSON response. JSON can contain HTML, even invalid HTML, just fine, it's only ExtJS' iframe-based request handling that breaks it.

I found this post about ext4 and how it has a new check for pre tags that the browser inserts when the response has a content type of text/json.

That code looked remarkably close to what @Finetuned tried to do in #13810, by wrapping the result in a textarea, which unfortunately would break a lot of things. However, setting a Content-Type header would not be quite as invasive to extras/non-mgr usage of connectors, as only really silly ways of submitting a request (like an iframe) would get a slightly different response.

So, back-porting that ext4 change into our ancient ext3, and updating modConnectorResponse to issue a text/json (or application/json) content type header, solves the problem!

I reformatted ext-all.js as a quick test and added

if ((contentNode = A.body.firstChild) && /pre/i.test(contentNode.tagName)) {
       w.responseText = z.textContent;
}

before the handling for textarea.

In modConnectorResponse there's some logic that only sets an application/json header when the request came from an xmlhttprequest. I just added header("Content-Type: application/json; charset=UTF-8"); before that if to always set it as a test.

And voila, my test case for this bug was fixed! :D

I think this would be a safer fix than #13810 as it doesn't change the actual content, just a header, but still fixes it. Would need some cleaning up and more testing, but this may be a way for us to get proper json support...