-
Notifications
You must be signed in to change notification settings - Fork 576
fix terminal paste code #2596
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix terminal paste code #2596
Conversation
…the pasting html on linux bug)
WalkthroughRefactors clipboard handling in frontend/app/view/term/termutil.ts. Adds exported type GenClipboardItem = { text?: string; image?: Blob }, reworks extractClipboardData to return GenClipboardItem or null with mode-based prioritization (image, plain text, HTML, empty-type strings), introduces extractDataTransferItems to process DataTransferItemList across modes (all images, first text/plain-like, first HTML → text, first empty-type string), adds findFirstDataTransferItem/findAllDataTransferItems helpers, and updates extractAllClipboardData to return GenClipboardItem[] with clipboard.items handling and Clipboard API fallback. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
|
fix for #2595 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/app/view/term/termutil.ts (1)
153-196: Helper search utilities are clear; consider narrowing thekindtype
findFirstDataTransferItem/findAllDataTransferItemsare straightforward and make the main logic more readable. If you want stricter typing, you could narrowkindto a union like'file' | 'string'instead ofstring, but that’s optional and not blocking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app/view/term/termutil.ts(3 hunks)
🧰 Additional context used
🪛 ast-grep (0.40.0)
frontend/app/view/term/termutil.ts
[warning] 136-136: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: tempDiv.innerHTML = html
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 264-264: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: tempDiv.innerHTML = html
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 136-136: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: tempDiv.innerHTML = html
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 264-264: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: tempDiv.innerHTML = html
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (3)
frontend/app/view/term/termutil.ts (3)
10-11: Unified clipboard payload type looks good
GenClipboardItemcleanly models the two supported payloads (text and image) and fits the new helpers’ return types without overcomplicating the shape.
101-151: ClipboardItem mode ordering and HTML-to-text extraction look correctThe prioritized modes (image → plain text → HTML → generic) align with the described heuristic, and converting HTML via a detached
<div>plustextContentensures terminals receive plain text instead of markup. Static analysis flags theinnerHTMLassignment, but because the element is never attached back into the DOM and you only readtextContent, this is not an XSS sink in this context.
292-321: Top-level paste orchestration and fallbacks look robustThe ordering—DataTransferItemList first, then Clipboard API, then a final plain‑text fallback on the event—matches the intended heuristic and gives a reasonable safety net for older or restricted environments. The control flow and error handling are clear and appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/app/view/term/termutil.ts (2)
128-140: Consider factoring HTML→text extraction into a small helperBoth branches use the same pattern (
createElement("div")→innerHTML = html→textContent) to derive plain text from HTML. A tiny helper likehtmlToPlainText(html: string): stringwould remove duplication and centralize any future tweaks (e.g., trimming rules) without changing behavior.Also applies to: 251-268
161-173: DataTransferItem helper utilities are correct; minor typing tweak possibleThe indexed iteration and
kind/typefiltering are straightforward and correct. You might tighten thekindparameter type toDataTransferItem["kind"](or"file" | "string") to make misuse harder at call sites, but that’s purely a nicety.Also applies to: 183-196
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app/view/term/termutil.ts(3 hunks)
🧰 Additional context used
🪛 ast-grep (0.40.0)
frontend/app/view/term/termutil.ts
[warning] 136-136: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: tempDiv.innerHTML = html
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 264-264: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: tempDiv.innerHTML = html
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 136-136: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: tempDiv.innerHTML = html
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 264-264: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: tempDiv.innerHTML = html
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
frontend/app/view/term/termutil.ts (4)
10-11: GenClipboardItem type alias fits the new clipboard flowThe minimal
{ text?: string; image?: Blob }shape lines up cleanly with all new extraction paths and keeps downstream handling straightforward. No changes needed here.
101-151: ClipboardItem mode ordering and fallbacks look correctThe four-mode heuristic (images → plain text → HTML-as-text → generic empty-type string) matches the PR description and avoids the previous HTML-preference issue for terminal pastes. The control flow is clear and returns early on the first supported mode, which is appropriate for a single
ClipboardItem.
198-283: Revised DataTransferItemList heuristic correctly prioritizes images and preserves text fallbacksMode 1 now explicitly targets
image/*file items, so non-image files no longer suppress text/HTML extraction, addressing the earlier gating problem. Modes 2–4 correctly prefer plain text, then HTML-derived text, then a generic empty-type string, each returning either a single{ text }item or all available{ image }items. The Promise wrapping aroundgetAsStringis also sound.
292-320: extractAllClipboardData routing and fallbacks are consistent with new APIDelegating event-based pastes to
extractDataTransferItemsfirst, then falling back tonavigator.clipboard.read(), and finally toclipboardData.getData("text/plain")on error gives a clear, layered strategy while always returningGenClipboardItem[]. This is a sensible evolution of the previous API.
lots of work to handle DataTransferItems with a better heuristic:
this should fix the html => to the terminal issue.