Skip to content
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

Implicit copy to system clipboard doesn't obey law of least surprise and poses potential security hazard #759

Closed
Tastaturtaste opened this issue Feb 27, 2024 · 10 comments
Labels
bug Something isn't working

Comments

@Tastaturtaste
Copy link
Contributor

Certain operations, such as alt + d, cut content and put the content in the cut buffer. With the system_clipboard feature enabled, the cut buffer is replaced by the system clipboard. This can lead to surprising behaviour changes for the user and pose a security hazard in case a user cuts sensitive information into the system clipboard without knowing about it.

For more context see nushell/nushell#11907.

Steps to reproduce

  1. Type a top secret password, eg. '1234'
  2. Move the cursor back to the start of the line
  3. Press alt+d, invoking CutWordRight
  4. Switch to another program entirely, such as a text editor, and paste

Possible Solution

I am currently looking into this and think it would be best if reedline always has a local clipboard (which it calls the cut buffer) and additionally with the system_clipboard feature enabled a system clipboard. Most commands would always use the local clipboard, only some explicitly named commands interact with the system clipboard. I would also like to avoid changing which clipboard gets used by a command depending on the feature flag used to compile. Commands should either use the cut buffer, which is always available, or the system clipboard.

Commands that copy to the system clipboard would be CopySelection (default Ctrl+Shift+C) and CutSelection (default Ctrl+Shift+X). If permitted with regard to backward compatibility of reedline, I would also like to rename them to CopySelectionToSystem and CutSelectionToSystem to be more explicit about what they do. Changing the way the clipboard works is a semantic breaking change either way.

In addition to PasteCutBufferBefore (default Ctrl+Shift+V and emacs Ctrl+Y), I would introduce a new Command PasteSystemClipboard, which does what PasteCutBufferBefore currently does with the system clipboard enabled. I would then change PasteCutBufferBefore to always paste from the local clipboard, matching its name. The default key binding for Ctrl+Shift+V would then be changed to invoke the new PasteSystemClipboard.

Open Questions

  1. Should the EditCommand enum variants concerning the system_clipboard feature (CutSelectionToSystem, ...) be conditionally included only? This would statically ensure that those commands can only come in when there is actually a system clipboard available. In this case, the enum should also be marked non_exhaustiv, otherwise this would break exhaustive matches for crates when somewhere in their dependency graph the system_clipboard feature gets newly enabled in the future.
  2. Are there commands where it does make sense to change the clipboard used depending on the feature flag?
@Tastaturtaste Tastaturtaste added the bug Something isn't working label Feb 27, 2024
@sholderbach
Copy link
Member

sholderbach commented Feb 27, 2024

Yes isolating the system clipboard to deliberate operations seems to be the only reasonable thing to do here.

Regarding nushell/nushell#11907 currently the feature is not deliberately enabled?

@Tastaturtaste
Copy link
Contributor Author

The feature is enabled in nushell by default with the system-clipboard feature of nushell, which is a default feature.

@fdncred fdncred changed the title Implicit copy to system clipboard doesn't obay law of least surprise and poses potential security hazard Implicit copy to system clipboard doesn't obey law of least surprise and poses potential security hazard Feb 27, 2024
@sholderbach
Copy link
Member

Ah I was just checking on the reedline imports directly.

There could be a debate on the nushell side if we should even include system clipboard access among the default features thanks to the patchy support accross the platforms. For the basic operations it is not necessary

@fdncred
Copy link
Collaborator

fdncred commented Feb 27, 2024

Thanks for the issue @Tastaturtaste. I'm excited to see what you come up with. I don't know the answer to your questions, sorry.

@Tastaturtaste
Copy link
Contributor Author

Ah I was just checking on the reedline imports directly.

There could be a debate on the nushell side if we should even include system clipboard access among the default features thanks to the patchy support accross the platforms. For the basic operations it is not necessary

Is the support really that patchy? As far as I know, there are mainly problems when running on linux without an x11 server. I would guess this is a pretty rare thing to run into, outside of pure server instances. But I am also not that knowledgable outside of windows.
For me at least being able to access the system clipboard in the terminal is an important feature.

@sholderbach
Copy link
Member

I think this is potentially pretty distribution dependent how everything is set up.

Some distros still ship with a traditional X11 others migrated to Wayland more extensively (where I am no expert on the support for clipboards). nushell/nushell#11907 (comment) indicates that whatever support we get from the arboard library depends on what exactly is installed on such a system.
On the BSD side of things I am unfamiliar how things are handled there. WSL2 with a Ubuntu distribution, seems to work (even though there you would not necessarily expect a feature-complete X server at least pre Win11)


But that is a separate discussion for Nushell from how we should go about on the reedline side.

I would say the suggestion in #380

All CutXXX commands like "CutFromStart" or "CutWordLeft" should use the OS copy/paste buffer not an internal buffer

was shortsighted and should be replaced by explicit ops as you suggested. (limiting this to the selection for cutting would be a compromise to keep the number of commands in check)

We can probably discard the Clipboard trait or atleast all the "feature overloading" to split things up neatly.

@Tastaturtaste
Copy link
Contributor Author

I would say the suggestion in #380

All CutXXX commands like "CutFromStart" or "CutWordLeft" should use the OS copy/paste buffer not an internal buffer

was shortsighted and should be replaced by explicit ops as you suggested. (limiting this to the selection for cutting would be a compromise to keep the number of commands in check)

My suggestion would be to only provide access to the OS clipboard with three commands, CopySelectionToSystem, CutSelectionToSystem and PasteFromSystem. That would still keep the number of commands in check while providing all the functionality at least I would expect from an integration with the OS clipboard. These commands were just very recently introduced with the commit that also brought the selection feature and since then always interacted with the OS clipboard. Do they need to be duplicated for the local clipboard? If yes, that would mean three additional commands compared to now. If not, only one new command for PasteFromSystem is necessary.

Another possibility would be to parameterize those commands over the clipboard where both clipboard choices possibly make sense. That would also keep the number of commands in check.

We can probably discard the Clipboard trait or atleast all the "feature overloading" to split things up neatly.

I take it that this means the answer to

  1. Are there commands where it does make sense to change the clipboard used depending on the feature flag?

is No?

@sholderbach
Copy link
Member

Mhh when we implement visual mode for the vi-emulation we will probably want to be able to express those selection ops for the local cut buffer as well but that is Zukunftsmusik.

I would be fine with first making the ones you mentioned system only.

I think we should not make runtime behavior dependent on the feature flag anymore. This could be reasonably be made at runtime, and the feature flag should just control what is available.

@fdncred
Copy link
Collaborator

fdncred commented Feb 27, 2024

@Tastaturtaste I also agree with your suggestion #759 (comment) and the point @sholderbach is making about feature flags.

@Tastaturtaste
Copy link
Contributor Author

I just made a PR to fix this. I implemented the three commands CutSelection, CopySelection and Paste with a parameter to select either the cut buffer or the system clipboard. When implementing the visual mode for the vi-emulation it should be as simple as emitting those commands with system_clipboard: false to use the local cut buffer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants