Skip to content

fix(clipboard): restore cleanup when legacyCopy's select() throws - #1186

Open
AmirF194 wants to merge 1 commit into
mozilla-ai:mainfrom
AmirF194:fix/1149-legacycopy-textarea-leak
Open

AmirF194 wants to merge 1 commit into
mozilla-ai:mainfrom
AmirF194:fix/1149-legacycopy-textarea-leak

Conversation

@AmirF194

@AmirF194 AmirF194 commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Description

legacyCopy (the execCommand fallback used on plain-HTTP LAN origins, where navigator.clipboard is undefined) builds an offscreen textarea holding the copied text, often an API key since CopyField routes through this helper. It calls document.getSelection(), reads document.activeElement, and calls source.select() before entering the only try/catch in the function, which guards execCommand alone. source.remove() runs after that, on the happy path only. select() is documented to throw in a detached-document/cross-document situation, and if it does, the textarea is never removed: the plaintext stays in the DOM for the life of the page.

Moved the removal, and the selection/focus restore that already existed to protect the operator's tab position, into a finally so they run regardless of what throws once the textarea is in the DOM.

How to test it locally

pnpm --dir web vitest run src/design-system/helpers/clipboard.test.ts. The new test stubs HTMLTextAreaElement.prototype.select to throw and asserts no <textarea> is left behind; it fails on main (1 leaked node) and passes on this branch.

PR Type

  • Bug Fix

Relevant issues

Fixes #1149

Checklist

  • I understand the code I am submitting.
  • I have added or updated tests that cover my change (tests/unit, tests/integration).
  • I ran the Definition of Done checks locally (make lint, make typecheck, make test).
  • Documentation was updated where necessary.
  • If the API contract changed, I regenerated the OpenAPI spec (uv run python scripts/generate_openapi.py).

AI Usage

  • This is fully AI-generated.

AI Model/Tool used: Claude

Any additional AI details you'd like to share:

Summary

  • Updated legacyCopy to always remove its temporary textarea and restore selection and focus.
  • Added a regression test for cleanup when select() throws.
  • Preserved the existing error behavior.

This prevents copied credentials from remaining in the document after a setup failure.

legacyCopy() appended the scratch textarea and read the operator's
selection before entering the try/catch that guards execCommand, and
called source.remove() only on the happy path after that block. If
getSelection() or select() throws (a detached document, per the
issue report), the textarea holding the copied secret is left in the
DOM for the life of the page.

Move the whole sequence into a single try/finally so the textarea is
always removed and the selection/focus restored, whatever throws.
The propagated error is unchanged; only the cleanup is now
unconditional.

Fixes mozilla-ai#1149
@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 7029601d-8e95-4cd3-98ca-1eeafa8b8d93

📥 Commits

Reviewing files that changed from the base of the PR and between 957909e and 25e0f26.

📒 Files selected for processing (2)
  • web/src/design-system/helpers/clipboard.test.ts
  • web/src/design-system/helpers/clipboard.ts

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.


Walkthrough

Changes

legacyCopy now runs textarea removal, selection restoration, and focus restoration in a finally block when clipboard operations throw. A regression test verifies cleanup and error propagation when select() fails.

Changes

Clipboard cleanup

Layer / File(s) Summary
Exception-safe copy cleanup
web/src/design-system/helpers/clipboard.ts, web/src/design-system/helpers/clipboard.test.ts
legacyCopy performs cleanup and state restoration from finally. The test verifies that a thrown select() error is propagated and that no temporary textarea remains.

Priority: ⬆️ High

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: High

Suggested reviewers: khaledosman

Merge Risk: ⚪ Minimal · up to 25e0f

The temporary clipboard textarea is removed when selection setup fails, preventing copied content from remaining in the page DOM. The change is ready to merge.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the cleanup fix, uses the Conventional Commit fix prefix with a scope, uses imperative mood, and is 65 characters long.
Description check ✅ Passed The description is complete and relevant. It explains the bug and fix, provides a local test command and expected result, identifies the change as a bug fix, references issue #1149, includes the check…
Linked Issues check ✅ Passed Issue #1149 requires unconditional cleanup after the temporary textarea is appended, restoration of selection and focus, retention of execCommand error handling, and a regression test for select()
Out of Scope Changes check ✅ Passed The changes are limited to legacyCopy cleanup and its regression test. Both changes directly support issue #1149. No unrelated product behavior or files are changed.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified code

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

legacyCopy can leave a textarea holding a credential in the DOM

1 participant