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

Check for unsafe drop #794

Merged
merged 11 commits into from
Nov 7, 2024
Merged

Check for unsafe drop #794

merged 11 commits into from
Nov 7, 2024

Conversation

jeremypw
Copy link
Collaborator

@jeremypw jeremypw commented Oct 13, 2024

Fixes #793

In order to use the same code to check for unsafe input, both paste and drop end up feeding the text to the terminal child. This actually saves some lines.

@jeremypw jeremypw requested a review from a team October 13, 2024 12:36
@teamcons
Copy link
Contributor

The issue was marked critical, but the PR seems out of date - Any ETA for merging ?

@jeremypw
Copy link
Collaborator Author

jeremypw commented Nov 1, 2024

I marked it critical because the issue has theoretical security implications, but its not a new issue. I guess folks are focused on getting OS8 released atm.

Copy link
Contributor

@ryonakano ryonakano left a comment

Choose a reason for hiding this comment

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

The clipboard or dragged text are no longer pasted if we once uncheck "Show paste protection warnings" in the UnsafePasteDialog.

@jeremypw
Copy link
Collaborator Author

jeremypw commented Nov 3, 2024

@ryonakano Oops! Thanks for spotting that. Now fixed. It occurred to me that if unsafe paste alert is turned off in the dialog, there is no way of turning it back on through the UI. Am I right? If so that needs fixing but in another PR.

@jeremypw
Copy link
Collaborator Author

jeremypw commented Nov 3, 2024

I noticed that this PR was pasting the stripped text into the terminal whereas in master, the text was refetched from the clipboard. Now the stripped text is only used for the dialog message as before.

@jeremypw jeremypw requested a review from ryonakano November 3, 2024 17:32
@teamcons
Copy link
Contributor

teamcons commented Nov 3, 2024

@ryonakano Oops! Thanks for spotting that. Now fixed. It occurred to me that if unsafe paste alert is turned off in the dialog, there is no way of turning it back on through the UI. Am I right? If so that needs fixing but in another PR.

following your comment i did a PR for that.
Pantheon-tweaks always offered that option, im not sure why there was no toggle-back in first instance.

Copy link
Contributor

@ryonakano ryonakano left a comment

Choose a reason for hiding this comment

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

The clipboard or dragged text are no longer pasted if we once uncheck "Show paste protection warnings" in the UnsafePasteDialog.

Confirmed this is fixed, thank you!

Leaving another two tiny comments.

src/Widgets/TerminalWidget.vala Show resolved Hide resolved
src/Widgets/TerminalWidget.vala Outdated Show resolved Hide resolved
@ryonakano ryonakano dismissed their stale review November 5, 2024 14:49

Changes addressed

Copy link
Contributor

@ryonakano ryonakano left a comment

Choose a reason for hiding this comment

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

Code looks good and fixes the linked issue. Thank you!

@jeremypw jeremypw enabled auto-merge (squash) November 7, 2024 16:46
@jeremypw jeremypw merged commit 28fd418 into master Nov 7, 2024
4 checks passed
@jeremypw jeremypw deleted the jeremypw/safe-drop branch November 7, 2024 16:47
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.

Drag-drop command bypasses sudo warning
3 participants