LPD-33910 Add sanitization for codeMirror editor #212
Conversation
| var ALERT_REGEX = /alert\((.*?)\)/; | ||
| var INNER_HTML_REGEX = /innerHTML\s*=\s*.*?/; | ||
| var PHP_CODE_REGEX = /<\?[\s\S]*?\?>/g; | ||
| var ASP_CODE_REGEX = /<%[\s\S]*?%>/g; | ||
| var ASP_NET_CODE_REGEX = /(<asp:[^]+>[\s|\S]*?<\/asp:[^]+>)|(<asp:[^]+\/>)/gi; | ||
| var HTML_TAG_WITH_ON_ATTRIBUTE_REGEX = /<[^>]+?(\s+\bon\w+=(?:'[^']*'|"[^"]*"|[^'"\s>]+))*\s*\/?>/gi; | ||
| var ON_ATTRIBUTE_REGEX = /(\s+\bon\w+=(?:'[^']*'|"[^"]*"|[^'"\s>]+))/gi; |
There was a problem hiding this comment.
Where did you get these from? Do we already do something similar in liferay-portal somewhere?
There was a problem hiding this comment.
Hello @bryceosterhaus
I took this from here: https://liferay.atlassian.net/browse/LPD-2161.
This is done for Rich Text Fields, and I asked the team to see if there component could cover this particular issue, but they came back saying it couldn't.
https://liferay.atlassian.net/browse/LPP-55225?focusedCommentId=2721890
Let me know if there are any further questions or comments about this.
There was a problem hiding this comment.
Okay good! Just wanted to make sure these regexes were at least being used in some other place so that we are testing so many new ones at once.
|
Thank you @bryceosterhaus! Thank you! |
There was a problem hiding this comment.
@fortunatomaldonado The sanitation looks good, but a question inline on code removal in PR.
| _sanitizeHTML: function (html) { | ||
| var sanitizedHtml = html | ||
| .replace(HTML_TAG_WITH_ON_ATTRIBUTE_REGEX, function (match) { | ||
| return match.replace(ON_ATTRIBUTE_REGEX, ''); | ||
| }) | ||
| .replace(ALERT_REGEX, '') | ||
| .replace(INNER_HTML_REGEX, '') | ||
| .replace(PHP_CODE_REGEX, '') | ||
| .replace(ASP_CODE_REGEX, '') | ||
| .replace(ASP_NET_CODE_REGEX, ''); | ||
|
|
||
| return sanitizedHtml; |
There was a problem hiding this comment.
Are we intentionally removing _handleCodeMirrorChange? It seems to update preview iframe. Won't this change result in preview never updating?
There was a problem hiding this comment.
Hello @markocikos,
Are you referring to this: 799b5af#diff-8570f7b77fa9c9666a3f866cf502ba43abeb941c958aaf14683e3177eb60ecfeL186-L197
I found there were two _handleCodeMirrorChange methods that were exactly the same in the file. So I decided to remove one of them.
Let me know if I am misunderstanding.
There was a problem hiding this comment.
Oh wow, that's a nice cleanup then 😄
|
@fortunatomaldonado you don't need to generate artifacts on each PR, that's what caused the conflicts after I merged #213. We only need to generate them once before release. Historically there was rarely more then one PR per release, so it was custom to add them immediately in PR. But it's not necessary, especially if you expect multiple fixes soon. |
|
@markocikos, oh I see. I'll remove the artifacts here and resend. |
fortunatomaldonado
force-pushed
the
LPD-33910
branch
from
43fde8f
to
799b5af
Compare
https://liferay.atlassian.net/browse/LPD-33910
https://liferay.atlassian.net/browse/LPP-55225
CodeMirror editor was not being sanitized and so adding sensitization prevents any unexpected behavior.
I tried several examples and was not able to get any unexpected behavior.
Please let me know if there are any questions or comments about this.
Thank you!