Conversation
…rializeNode function
🦋 Changeset detectedLatest commit: 73d2cb1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 98 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| const serializeNode = (node: TNode): Html => { | ||
| if (Text.isText(node)) { | ||
| let string = escapeHtml(node.text); | ||
| let string = node.text; |
There was a problem hiding this comment.
Not being familiar with the code and the context, but at first glance this change seems to open the flood-gates. It looks to me now as if a user could pass any html. Can you verify this is not the case?
Did you just check if a-tags pass now, or did you also check with malicious content (e.g. script-tags).
It might be a good idea to add tests for this 2 behaviors as well (allows tags, prevents malicious html).
There was a problem hiding this comment.
Oh good catch! I was already going to discuss this with the team. After raising the PR, I am beginning to see that destroying the tags was a deliberate decision. Maybe we can select some allowed tags to be passed at least.
There was a problem hiding this comment.
I handled it in this commit. To only allow these tags, any other tags will be destroyed.
There was a problem hiding this comment.
I think so too. Ideally on a case by case basis. Wether or not its worth it for ui-kit is another question though.
…ges in serialized output
…ttps://github.com/commercetools/ui-kit into CRAFT-2040-rich-text-input-destroys-hyperlink-tag
…on and update tests back to initial state
| import { canUseDOM } from '@commercetools-uikit/utils'; | ||
|
|
||
| type Html = string; | ||
| const ALLOWED_HTML_TAGS = [ |
There was a problem hiding this comment.
We need to have a conversation about this list here. So far, these are the use cases we want to take care of that I can think of, also these ones seem to serialise.
ByronDWall
left a comment
ByronDWall
left a comment
There was a problem hiding this comment.
Outside of 'should we allow links at all', I have some questions about what the desired end state of this pr should be, is it:
- users can enter html tags as plaintext and they won't get destroyed, or
- users should be able to format text into these tags using the WYSIWYG controls, similar to how they can format headings, lists, etc?
I was out last week - is this in response to a customer request / issue?
| 'tbody', | ||
| 'table', |
There was a problem hiding this comment.
Is there a usecase for allowing table elements? Right now there is no way to visually format a table in the editor, and adding one seems way outside the scope of this PR.
If we are allowing plaintext table elements, should we whitelist all the necessary tags?
thead
tfoot
tr
th
td
colgroup
col
caption
| const serializeNode = (node: TNode): Html => { | ||
| if (Text.isText(node)) { | ||
| let string = escapeHtml(node.text); | ||
| let string = node.text; |
There was a problem hiding this comment.
Assuming we are going to allow all these tags, do we also want to add controls to apply them? In this implementation, you have to manually type out the <a> tag and attributes, e.g. <a href="test.com">test</a>.
From what I can tell, when you input an a tag, the editor doesn't display the a tag as a link, but as the full html text - is this the intended result?
There was a problem hiding this comment.
@ByronDWall per our last meeting, I whitelisted the anchor tag and covered some common security concerns in how its used on this commit 471cfd0
Thank you.
…ping to enhance security
ddouglasz
force-pushed
the
CRAFT-2040-rich-text-input-destroys-hyperlink-tag
branch
from
5478e6c to
46f507d
Compare
…ich-text-utils and yarn.lock
|
[preview_deployment] |
|
Release workflow succeeded ✅\nSee details: Workflow Run |
…ests to ensure accurate HTML representation
ddouglasz
deleted the
CRAFT-2040-rich-text-input-destroys-hyperlink-tag
branch
4 participants
Summary
Update the Slate HTML serializer to stop escaping text nodes so </> stay as-is when the editor value is serialized, which allows tags to pass through.