Skip to content

Prompt before terminal clipboard writes by default - #138

Open
lawrencecchen wants to merge 2 commits into
mainfrom
fix-clipboard-consent
Open

Prompt before terminal clipboard writes by default#138
lawrencecchen wants to merge 2 commits into
mainfrom
fix-clipboard-consent

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Jul 24, 2026

Copy link
Copy Markdown

Summary

  • default terminal clipboard reads and writes to ask
  • clarify that callback confirmation flags must be honored before access completes

Testing

  • added the default-setting regression test in the first commit
  • passed focused and broader clipboard tests with Zig 0.15.2

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@lawrencecchen, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 55 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 15e38e4a-38f9-467b-86e9-8bf96dec76bc

📥 Commits

Reviewing files that changed from the base of the PR and between 80d6551 and 21f089e.

📒 Files selected for processing (3)
  • include/ghostty.h
  • src/apprt/embedded.zig
  • src/config/Config.zig
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-clipboard-consent

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.

@lawrencecchen
lawrencecchen marked this pull request as ready for review July 24, 2026 23:10
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@greptile-apps

greptile-apps Bot commented Jul 24, 2026

Copy link
Copy Markdown

Greptile Summary

This PR tightens clipboard security by changing the clipboard-write default from allow to ask, so OSC 52 writes now prompt the user before touching the system clipboard, matching the existing behaviour for clipboard-read. Documentation in the C header and Zig interface is updated to describe what the embedder must do when the confirm flag is true, and a regression test pins the new defaults.

  • src/config/Config.zig: clipboard-write default changed to .ask, doc comment updated, and a new test verifies both defaults are .ask.
  • include/ghostty.h / src/apprt/embedded.zig: Added comments clarifying that confirm=true requires the embedder to obtain approval and leave the clipboard unchanged on denial — however, neither comment notes that the ghostty_clipboard_content_s* data pointer is only valid during the callback, which matters for the async confirmation flows this default now makes common.

Confidence Score: 3/5

The config default change and test are clean, but the new documentation contract omits the pointer-lifetime constraint for the write callback, which is critical for any embedder that handles the confirmation dialog asynchronously.

The ghostty_clipboard_content_s* data passed to write_clipboard_cb is stack-allocated in Surface.zig and freed immediately after the callback returns. With the old allow default, embedders wrote synchronously and this was never an issue. With ask now the default, embedders are expected to show an async confirmation UI — but the newly added comments don't warn them to copy the content before returning. An embedder that doesn't copy will read freed memory when the user confirms, leading to UB or a crash.

Files Needing Attention: include/ghostty.h and src/apprt/embedded.zig — the write_clipboard_cb contract comment should note that ghostty_clipboard_content_s data is only valid for the lifetime of the callback.

Important Files Changed

Filename Overview
include/ghostty.h Added a comment clarifying the confirm contract for write_clipboard_cb, but omits the critical pointer-lifetime constraint for async confirmation flows.
src/apprt/embedded.zig Doc comment on write_clipboard updated to describe the confirm=true obligation; same lifetime omission as in ghostty.h.
src/config/Config.zig Default for clipboard-write changed from .allow to .ask, doc comment updated, and a regression test added — all consistent and correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[OSC 52 write request from terminal program] --> B{clipboard-write config}
    B -->|deny| C[Log and return — clipboard unchanged]
    B -->|allow / old default| D[call setClipboard with confirm=false]
    B -->|ask / new default| E[call setClipboard with confirm=true]
    D --> F[Embedder writes clipboard directly]
    E --> G[Embedder shows confirmation dialog - must copy content before returning]
    G -->|User approves| H[Embedder writes clipboard]
    G -->|User denies| I[Embedder leaves clipboard unchanged]
Loading

Comments Outside Diff (1)

  1. include/ghostty.h, line 1156-1162 (link)

    P1 Clipboard content pointer lifetime undocumented for async confirmation

    The new comment tells embedders they must obtain explicit approval before writing, but does not mention that the ghostty_clipboard_content_s* pointer (and the data/mime C strings it contains) is only valid for the duration of the callback. In Surface.zig, buf is freed via defer immediately after setClipboard returns. An embedder showing a confirmation dialog asynchronously (the common GUI pattern now that the default is ask) must copy the content before returning from the callback or it will read freed memory when the user confirms. The same observation applies to the parallel documentation added in embedded.zig.

Reviews (1): Last reviewed commit: "fix: prompt for terminal clipboard write..." | Re-trigger Greptile

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.

1 participant