[WIP] prosemirror tables #3243
[WIP] prosemirror tables #3243
Conversation
DavidAkkerman
force-pushed
the
poc-prosemirror-tables
branch
2 times, most recently
from
4bbe3f8 to
e0714ef
Compare
|
@DavidAkkerman, just an FYI: I've assigned you to this, because it's on the Feature Team board, and we have the workflow that the person that made the PR remains assigned when the PR is put in review, in order to keep some sense of ownership from the developer who made the PR. Then the reviewer also assigns themself to the issue. So, in the review column (on our board), one assignee means it's ready for review, two assignees means it's being reviewed 😊 |
| setDOMAttr: (value: string, attrs: Record<string, string>) => { | ||
| if (value) { | ||
| attrs.style = | ||
| (attrs.style || '') + `background-color: ${value};`; |
There was a problem hiding this comment.
We probably need to check that we aren't opening up a vulnerability to script injection here. We might need to sanitise the value before using it.
There was a problem hiding this comment.
I did a quick test, and it seems like any tags are encoded if not explicitly allowed
Also tried to research a bit, and it seems that the prosemirror developers are generally quite on top of protecting against these kind of things, such as discussed in this forum post https://discuss.prosemirror.net/t/heads-up-xss-risk-in-domserializer/6572.
I do feel a bit out of my comfort zone with this topic though, so I would appreciate a second opinion :)
There was a problem hiding this comment.
Yeah, it sounds very reasonable that prosemirror would come with good protections built-in, or at least possible to easily enable (in which case I hope we've done so). What I'm concerned about is if directly manipulating DOM attributes like this, in a plugin, bypasses the built-in protections? I'm a bit unsure where the value of the value parameter comes from? It from the pasted content originally, of course, but has that all first gone through sanitising first, or are we reading it straight from the raw input?
You don't have to be the one who can answer these questions @DavidAkkerman. I'm guessing that @john-traas may have already looked into stuff like this in his previous work on the text editor, and if not, I, John, or someone else from our team can look it up 🙂
There was a problem hiding this comment.
⚡ See comment below about updating the prosemirror-model version.
src/components/text-editor/prosemirror-adapter/plugins/table-plugin.ts
Outdated
Show resolved
Hide resolved
| }, | ||
| setDOMAttr: (value: string, attrs: Record<string, string>) => { | ||
| if (value) { | ||
| attrs.style = (attrs.style || '') + `color: ${value};`; |
There was a problem hiding this comment.
Same here, we need to consider injection attacks.
There was a problem hiding this comment.
⚡ Just adding this for clarity in case anyone is reading in future. We have updated prosemirror-model to version 1.22.1 as per this thread here
| import { createImageRemoverPlugin } from './plugins/image-remover-plugin'; | ||
| import { createMenuStateTrackingPlugin } from './plugins/menu-state-tracking-plugin'; | ||
| import { createActionBarInteractionPlugin } from './plugins/menu-action-interaction-plugin'; | ||
| import { getTableNodes, tableEditingPlugin } from './plugins/table-plugin'; |
There was a problem hiding this comment.
From what I've seen so far, this looks like a very lightweight and well-made addition.
What I'm a little concerned over is whether hardcoding in this plugin might prove problematic as we go forward with making limel-text-editor pluggable. Since this would add a very rudimentary support for tables, it seems fairly likely that, if we make it possible to use your own plugins with limel-text-editor, someone will eventually want to use a plugin that gives more comprehensive support for tables.
- Do we have any idea if this plugin might be causing conflicts in a scenario like that?
- Will it be possible to make the use of this plugin the default, will still making it possible to disable it to avoid conflicts with a consumer-provided plugin?
Since adding this now would provide this support by default from that point on. That means that if we need to later make it non-default, and have consumers register the plugin, that would be a breaking change, which we want to avoid if possible.
I don't view this as a total show-stopper, but I want to lift the concern, so we will at least considered it before moving ahead.
@john-traas Your input would be valuable here 🙏
There was a problem hiding this comment.
@DavidAkkerman @adrianschmidt I'll take another look at the start of cooldown.
I'm focused on the cycle work at the moment.
adrianschmidt
force-pushed
the
poc-prosemirror-tables
branch
from
5211671 to
151b20f
Compare
john-traas
force-pushed
the
poc-prosemirror-tables
branch
from
151b20f to
4ed1c9d
Compare
john-traas
force-pushed
the
poc-prosemirror-tables
branch
2 times, most recently
from
c027904 to
78c6aba
Compare
john-traas
force-pushed
the
poc-prosemirror-tables
branch
from
78c6aba to
cfb3135
Compare
|
FYI - SonarCloud Scan will fail as long as the PR is created in a fork of this repo. We don't allow that (sharing the SONARCLOUD_TOKEN with the fork). I suggest recreating the branch in this repo instead.
|
|
@john-traas I don't have permission to create a branch in this repo, could you do this? Then we can merge to that branch instead |
john-traas
changed the base branch from
main
to
add-tables-plugin-to-text-editor
john-traas
merged commit 705a049
into
Lundalogik:add-tables-plugin-to-text-editor
4 participants
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: