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

Add toggle for unsafe paste #798

Merged
merged 3 commits into from
Nov 4, 2024
Merged

Add toggle for unsafe paste #798

merged 3 commits into from
Nov 4, 2024

Conversation

teamcons
Copy link
Contributor

@teamcons teamcons commented Nov 3, 2024

No description provided.

@teamcons teamcons changed the title OH THIS WORKS Add toggle for unsafe paste Nov 3, 2024
@teamcons
Copy link
Contributor Author

teamcons commented Nov 3, 2024

This is pretty much this. My apologies - @jeremypw pretty much mentioned there was no official supported mean to toggle it back, and for once i thought it was easy enough to offer.

I am quite new to all this, and not so sure about the lint - I only touched the one file.

Bildschirmfoto von 2024-11-03 18 41 28

@jeremypw
Copy link
Collaborator

jeremypw commented Nov 3, 2024

Code looks good! To be picky, the unsafe dialog also now appears with "doas" in the command and (will) also work with dropped text so a more general description would be something like
Show a warning dialog before pasting or dropping text that may be trying to gain administrative access or which contains multiple commands. Might be a bit too verbose though. @danirabbit thoughts?

@teamcons
Copy link
Contributor Author

teamcons commented Nov 3, 2024

Valid nitpick.
To be transparent, i went with the description from Pantheon-Tweaks, which expose the setting.

Would "inserting" work instead of "pasting or dropping" ? It isnt typed manually but "inserted"
Maybe instead of "commands that may be trying to gain administrative access" that would be "administration commands" ? Since doas and sudo are used pretty much for administration - you dont really go anywhere in administration without.

"Show a warning dialog when inserting administration commands or multiple commands at once"

@@ -113,6 +113,11 @@ public sealed class Terminal.SettingsPopover : Gtk.Popover {
active = Application.settings.get_boolean ("natural-copy-paste")
};

var unsafe_paste_alert_button = new Granite.SwitchModelButton (_("Unsafe Paste Alert")) {
description = _("Show a warning dialog when pasting a command with 'sudo' or multiple lines"),
Copy link
Member

@danirabbit danirabbit Nov 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe something like this? I don’t think we need to say “dialog”. Since there’s multiple ways to enter text from somewhere else, maybe we focus on the copying instead of the pasting/dragging. And the kind of person who needs this probably doesn’t understand what “sudo” or “doas” means so try to use language they would understand. Not sure if “administrative” or “advanced” or something else is better here. Might need to look at language from like Polkit agent or something else to see what we usually use here

Suggested change
description = _("Show a warning dialog when pasting a command with 'sudo' or multiple lines"),
description = _("Warn when pasted text contains administrative commands or multiple lines"),

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree about "dialog". "Copied text" is a bit ambiguous as you can also copy from the terminal. Again not sure what "advanced" means in this context (and what it might be translated to). It is difficult to be precise, unambiguous and succinct all at the same time!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay updated my suggestion to use "administrative commands" and stick to "paste" here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Warn when pasted text contains multiple or administrative commands"

short. May be of the vague.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is good - short and clear!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's pretty good. In combination with seeing/interacting with the actual warning dialog I think folks should be able to work out what this setting refers to

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. By default it is "on". So if they disable it, theyll likely have already interacted with it in the past and connected the dots with the dialog and pasting "doas"

(unrelated but does it also react to Wget outputted to bash ? Ive seen too "oh heres a one liner to download and run this script that fixes your problem")

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At present it just looks for the words "sudo" or "doas" so it is pretty dumb. Maybe raise an issue if you feel there is a further security threat not covered.

@jeremypw
Copy link
Collaborator

jeremypw commented Nov 3, 2024

Would "inserting" work instead of "pasting or dropping" ?

"Inserting" was my first choice but I was worried that the reader might think the dialog would appear when entering administrative commands by typing as well. Its probably OK together with TRANSLATORS note to make it clear that it only means pasting or dropping.

@danirabbit
Copy link
Member

danirabbit commented Nov 3, 2024

I think it would be better to use "paste" than "insert" personally. A dnd operation could still be considered a "paste" but I think "insert" implies typing would also trigger it

@danirabbit
Copy link
Member

"Administrative commands" could be good. I kinda like that! Seems concise but clear

@jeremypw
Copy link
Collaborator

jeremypw commented Nov 3, 2024

Another possibility is to use the word "privileged".

"Warn when pasted text contains multiple or administrative commands"
Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets go!

@jeremypw jeremypw merged commit 970ce17 into elementary:master Nov 4, 2024
4 checks passed
@teamcons teamcons deleted the main branch November 4, 2024 10:04
@teamcons
Copy link
Contributor Author

teamcons commented Nov 4, 2024

My first Pull Request :')

(sorry i know adding a toggle is the easiest thing in the world but i am proud)

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.

3 participants