🐛(frontend) sanitize pasted and dropped content in document title#2210
🐛(frontend) sanitize pasted and dropped content in document title#2210
Conversation
Prevent rich-text formatting when pasting or dropping into document title.
Ovgodd
force-pushed
the
fix/pasting-in-title
branch
from
c2c0cb5 to
fbd7166
Compare
|
Size Change: -1 B (0%) Total Size: 4.26 MB 📦 View Changed
|
WalkthroughAdds handling to sanitize pasted and dropped content into the document title contentEditable element by inserting only plain text. Implements an Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/frontend/apps/impress/src/features/docs/doc-header/components/DocTitle.tsx`:
- Around line 162-168: After inserting the text node into the range (using
selection.getRangeAt(0), range.insertNode(document.createTextNode(plainText))),
explicitly move the caret to after the inserted node: obtain the node you
inserted (the new Text node), call range.setStartAfter(thatNode) and then
collapse(true), then update selection.removeAllRanges() and
selection.addRange(range) so the caret is positioned after the pasted text;
reference the existing variables selection, range, and plainText in DocTitle.tsx
when making this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 55242805-6bdf-4fc8-896a-fb1d9bd0ed6c
📒 Files selected for processing (2)
CHANGELOG.mdsrc/frontend/apps/impress/src/features/docs/doc-header/components/DocTitle.tsx
| const range = selection.getRangeAt(0); | ||
| range.deleteContents(); | ||
| range.insertNode(document.createTextNode(plainText)); | ||
| range.collapse(false); | ||
|
|
||
| selection.removeAllRanges(); | ||
| selection.addRange(range); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "DocTitle.tsx" -type f 2>/dev/null | head -5Repository: suitenumerique/docs
Length of output: 45
🏁 Script executed:
git ls-files | grep -i "doctitle"Repository: suitenumerique/docs
Length of output: 330
🏁 Script executed:
find . -type f -name "DocTitle.tsx"Repository: suitenumerique/docs
Length of output: 143
🏁 Script executed:
cat -n src/frontend/apps/impress/src/features/docs/doc-header/components/DocTitle.tsx | sed -n '150,190p'Repository: suitenumerique/docs
Length of output: 1459
🏁 Script executed:
cat -n src/frontend/apps/impress/src/features/docs/doc-header/components/DocTitle.tsx | sed -n '1,50p'Repository: suitenumerique/docs
Length of output: 1836
🏁 Script executed:
web_search
DOM Range insertNode collapse behavior caret position typingRepository: suitenumerique/docs
Length of output: 155
🌐 Web query:
DOM Range insertNode API caret position after insertion
💡 Result:
The DOM Range.insertNode(newNode) method inserts the specified Node at the start boundary point of the Range. According to official MDN documentation, it inserts at the start of the Range, splitting any text node at the insertion point if necessary, and the children of a DocumentFragment if provided. It returns undefined and does not automatically update or collapse the Range boundaries after insertion. The Range object itself remains unchanged in terms of its boundary points relative to the original positions, but since insertion modifies the DOM structure (e.g., splitting text nodes), the effective position of the boundaries may shift depending on the exact context. However, official specs and docs do not explicitly state that the Range is collapsed or moved after insertion. For the caret (cursor) position in contenteditable scenarios—which is controlled by the Selection object associated with the Range—the caret typically appears at the original start position after insertion if the Range/Selection is not updated. To position the caret after the inserted node (common requirement for user-friendly editing), you must manually adjust the Range using methods like setStartAfter(newNode), collapse(true), and update the Selection with removeAllRanges and addRange(range). Example code pattern from multiple sources (Stack Overflow, javascript.info, dev.to): var sel = window.getSelection; if (sel.rangeCount) { var range = sel.getRangeAt(0); range.deleteContents; // Optional: replace selection range.insertNode(newNode); range.setStartAfter(newNode); range.collapse(true); sel.removeAllRanges; sel.addRange(range); } This ensures the caret is placed immediately after the inserted node. If inserting a DocumentFragment, track the last child node for setStartAfter(lastChild). This behavior is consistent across modern browsers as per browser compatibility data (widely supported since 2015).
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Range.insertNode
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Range/insertNode
- 3: https://developer.mozilla.org/en-US/docs/Web/API/Range
- 4: https://stackoverflow.com/questions/72654386/javascript-range-after-inserting-a-node-how-to-set-the-caret-after-the-that-no
- 5: https://stackoverflow.com/questions/21822425/set-cursor-positionon-editable-div-after-insertnode-by-range
- 6: https://dev.to/saurabh_raj_afaabe1844a4c/mastering-selection-and-ranges-in-javascript-never-to-look-back-again-2mkk
- 7: https://stackoverflow.com/questions/9828623/inserting-caret-after-an-inserted-node
- 8: https://stackoverflow.com/questions/4834793/set-caret-position-right-after-the-inserted-element-in-a-contenteditable-div
🌐 Web query:
Range.collapse(false) vs collapse(true) cursor placement
💡 Result:
Range.collapse(true) collapses the Range to its start boundary point, placing the cursor at the beginning of the original range. Range.collapse(false) collapses the Range to its end boundary point, placing the cursor at the end of the original range. Calling collapse without a parameter defaults to collapse(false), moving to the end. A collapsed Range has start and end positions at the same point and contains no content, acting as an insertion cursor. Example from MDN: const range = document.createRange; const referenceNode = document.getElementsByTagName("div").item(0); range.selectNode(referenceNode); range.collapse(true); // Cursor now at start of div This behavior is standard across browsers since 2015 and defined in the DOM specification.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Range/collapse
- 2: https://developer.mozilla.org/docs/Web/API/Range/collapse
- 3: https://developer.mozilla.org/en-US/docs/Web/API/range.collapse
- 4: https://github.com/mdn/content/blob/main/files/en-us/web/api/range/collapse/index.md?plain=1
- 5: https://mdn2.netlify.app/en-us/docs/web/api/range/collapse/
Move the caret after the inserted text node.
After insertNode(), the Range boundaries are not automatically updated by the DOM API. This causes the caret to potentially remain at the original range position rather than after the pasted text, leading to typing appearing before the pasted content. Explicitly position the caret using setStartAfter() and then collapse(true):
🐛 Proposed fix
const range = selection.getRangeAt(0);
range.deleteContents();
- range.insertNode(document.createTextNode(plainText));
- range.collapse(false);
+ const textNode = document.createTextNode(plainText);
+ range.insertNode(textNode);
+ range.setStartAfter(textNode);
+ range.collapse(true);
selection.removeAllRanges();
selection.addRange(range);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const range = selection.getRangeAt(0); | |
| range.deleteContents(); | |
| range.insertNode(document.createTextNode(plainText)); | |
| range.collapse(false); | |
| selection.removeAllRanges(); | |
| selection.addRange(range); | |
| const range = selection.getRangeAt(0); | |
| range.deleteContents(); | |
| const textNode = document.createTextNode(plainText); | |
| range.insertNode(textNode); | |
| range.setStartAfter(textNode); | |
| range.collapse(true); | |
| selection.removeAllRanges(); | |
| selection.addRange(range); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/frontend/apps/impress/src/features/docs/doc-header/components/DocTitle.tsx`
around lines 162 - 168, After inserting the text node into the range (using
selection.getRangeAt(0), range.insertNode(document.createTextNode(plainText))),
explicitly move the caret to after the inserted node: obtain the node you
inserted (the new Text node), call range.setStartAfter(thatNode) and then
collapse(true), then update selection.removeAllRanges() and
selection.addRange(range) so the caret is positioned after the pasted text;
reference the existing variables selection, range, and plainText in DocTitle.tsx
when making this change.
Yes of course |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts (1)
147-173: Consider also covering the drop path.The PR introduces both
onPasteCaptureandonDropCapturesanitization, but only paste is exercised here. A sibling case dispatching adropevent withDataTransfercarryingtext/plain+text/htmlwould prevent future regressions in the drop handler.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts` around lines 147 - 173, Add a sibling test to exercise the drop path similar to the paste test: create a test (e.g., "it drops plain text in the title without keeping formatting") that reuses createDoc and the same Document title textbox selector ('[aria-label="Document title"]'), programmatically dispatches a 'drop' event on that element with a DataTransfer containing both 'text/plain' and 'text/html' entries, blurs the title and asserts the textbox text equals the plain text, and reference the onDropCapture sanitization alongside onPasteCapture to ensure drop handling is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts`:
- Around line 147-173: The test "it pastes plain text in the title without
keeping formatting" currently uses toHaveText which only checks textContent and
would pass even if HTML formatting was preserved; update the assertion to verify
the rendered markup instead—locate the test and the docTitle element (query
selector '[aria-label="Document title"]' / variable docTitle) and assert that
the element's innerHTML does not contain formatting tags (e.g., no <b> or <em>
descendants) or explicitly compare innerHTML to the expected plain text
representation so the paste handler is validated to strip HTML.
---
Nitpick comments:
In `@src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts`:
- Around line 147-173: Add a sibling test to exercise the drop path similar to
the paste test: create a test (e.g., "it drops plain text in the title without
keeping formatting") that reuses createDoc and the same Document title textbox
selector ('[aria-label="Document title"]'), programmatically dispatches a 'drop'
event on that element with a DataTransfer containing both 'text/plain' and
'text/html' entries, blurs the title and asserts the textbox text equals the
plain text, and reference the onDropCapture sanitization alongside
onPasteCapture to ensure drop handling is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c1debf8e-e73b-4e26-9b6d-84890676c347
📒 Files selected for processing (1)
src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts
| test('it pastes plain text in the title without keeping formatting', async ({ | ||
| page, | ||
| browserName, | ||
| }) => { | ||
| await createDoc(page, 'doc-title-paste', browserName, 1); | ||
|
|
||
| const docTitle = page.getByRole('textbox', { name: 'Document title' }); | ||
| await docTitle.click(); | ||
| await page.keyboard.press('Control+a'); | ||
|
|
||
| await page.evaluate(() => { | ||
| const el = document.querySelector('[aria-label="Document title"]'); | ||
| if (!el) { | ||
| return; | ||
| } | ||
|
|
||
| const dt = new DataTransfer(); | ||
| dt.setData('text/plain', 'Pasted plain text'); | ||
| dt.setData('text/html', '<b><em>Pasted plain text</em></b>'); | ||
| el.dispatchEvent( | ||
| new ClipboardEvent('paste', { clipboardData: dt, bubbles: true }), | ||
| ); | ||
| }); | ||
|
|
||
| await docTitle.blur(); | ||
| await expect(docTitle).toHaveText('Pasted plain text'); | ||
| }); |
There was a problem hiding this comment.
Assertion does not actually verify plain-text sanitization.
toHaveText compares against textContent, and the HTML payload <b><em>Pasted plain text</em></b> has the exact same textContent as the plain-text payload. The test would pass even if the handler inserted the rich HTML verbatim, defeating its purpose. Assert the rendered markup instead (e.g., no <b>/<em> descendants, or verify innerHTML).
🔧 Proposed assertion tightening
await docTitle.blur();
await expect(docTitle).toHaveText('Pasted plain text');
+ // Ensure formatting tags from text/html were not inserted.
+ await expect(docTitle.locator('b, em, strong, i')).toHaveCount(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts` around lines
147 - 173, The test "it pastes plain text in the title without keeping
formatting" currently uses toHaveText which only checks textContent and would
pass even if HTML formatting was preserved; update the assertion to verify the
rendered markup instead—locate the test and the docTitle element (query selector
'[aria-label="Document title"]' / variable docTitle) and assert that the
element's innerHTML does not contain formatting tags (e.g., no <b> or <em>
descendants) or explicitly compare innerHTML to the expected plain text
representation so the paste handler is validated to strip HTML.
AntoLC
left a comment
AntoLC
left a comment
There was a problem hiding this comment.
You should fix this comments before merging, I think it is right what the rabbit says, the test has no purpose as it is:
#2210 (comment)
2 participants
Purpose
Fix document title input behavior so pasted or dragged content is treated as plain text only.
titlepaste.mp4
Proposal
DocTitleand inserttext/plaininstead of rich content.DocTitleand inserttext/plainonly.