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

Prevent from removing whole password-store directory and hidden directories and files #400

Closed
SergeyBear opened this Issue Jul 3, 2018 · 11 comments

Comments

Projects
None yet
4 participants
@SergeyBear
Copy link

SergeyBear commented Jul 3, 2018

Clicking Delete button when there is no focus on any treeview item (when opening QtPass or clicking on already selected item, etc) will lead to complete removing of password-store directory including .git repository, so there will be no way to restore password-store if you don't have backups or remote origin.

QtPass version 1.2.3

@FiloSpaTeam

This comment has been minimized.

Copy link
Contributor

FiloSpaTeam commented Jul 3, 2018

Thank you! I was trying to reproduce but i didn't tried without focus! 👍
I'm sorry for your folder :(:(

@annejan

This comment has been minimized.

Copy link
Member

annejan commented Jul 3, 2018

I am sorry for your loss.

I'll try and fix this tomorrow (not around computer today) to prevent future incidents.

Weird thing is that I can remember this kind of thing (disabling delete button) used to work correctly in the past.

@SergeyBear

This comment has been minimized.

Copy link

SergeyBear commented Jul 3, 2018

Don't worry, I have backups - useful habit :-)
Just don't want to someone shoot himself in a leg.

P.S Thank you for quick response and great QtPass!

@annejan

This comment has been minimized.

Copy link
Member

annejan commented Jul 5, 2018

Hi @SergeyBear I'm trying to reproduce this . . but the red X delete button gets disabled as soon as I don't have focus on any file or folder and the right click context menu doesn't show delete either in those cases for me . . on macOS at-least.

Can you help me reproduce this error?

@SergeyBear

This comment has been minimized.

Copy link

SergeyBear commented Jul 5, 2018

Hello @annejan !

To reproduce: click on any item in treeview twice with some delay (not doubleclick). After that focus disappears but Delete button stays enabled and offers to remove /
2018-07-05 14-50-09

Found annother issue with focus:
If I switch from one passwordstore to another without selecting item on treeview of openned passwordstore, focus stays on previous passwordstore and toolbar items perform actions also on previous passwordstore.

To reproduce: create two passwordstores Test1 and Test2. Select item on Test1 and switch to Test2 and press Create button on toolbar - it will offer to create item on Test1 onstead of Test2.

OS: Fedora 27/28 (all packages uptodate)
PACKAGE: qtpass-1.2.3-1.fc27.x86_64/qtpass-1.2.3-1.fc28.x86_64

@annejan

This comment has been minimized.

Copy link
Member

annejan commented Jul 5, 2018

Thank you for this very complete report, will get on it when I find the time . .

The "funny" thing is that I didn't test this (probably) because it feels un-natural for me to click twice to deselect . . interesting . .

I can now reproduce and will fix ASAP.

@FiloSpaTeam

This comment has been minimized.

Copy link
Contributor

FiloSpaTeam commented Aug 18, 2018

With last pull #407 i fixed this too. I check for selectionModel when possible and rowCount of proxyModel to disable accordingly button and bypass Enter event 👍

FiloSpaTeam added a commit to UnitooTeam/QtPass that referenced this issue Aug 18, 2018

@rdoeffinger

This comment has been minimized.

Copy link
Contributor

rdoeffinger commented Aug 25, 2018

Maybe it would be good to not rely on the button being disabled, but also double-check in the actual delete function though? Or was that already done in some of those fixes?

@FiloSpaTeam

This comment has been minimized.

Copy link
Contributor

FiloSpaTeam commented Aug 25, 2018

The problem is that i cannot check in the delete function because in some way a valid path is passed but the tree source is on / or different path. At the moment I'm checking how the model is used with the proxy to solve definitively.

@rdoeffinger

This comment has been minimized.

Copy link
Contributor

rdoeffinger commented Aug 25, 2018

I was more thinking of things like refusing to delete files/directories starting with ".", refusing to delete non-empty directories (at least without extra confirmation), refusing anything that isn't actually below the password store repo or similar measures...

@FiloSpaTeam

This comment has been minimized.

Copy link
Contributor

FiloSpaTeam commented Aug 29, 2018

Yep this should be but in some way this is not working due to different parameters checking. I think i'll find a way checking the model or the proxyModel 👍

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