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

Removal of files outside of password-store #300

Closed
annejan opened this Issue Mar 2, 2017 · 15 comments

Comments

Projects
None yet
3 participants
@annejan
Member

annejan commented Mar 2, 2017

Received via mail:

One day I was trying out password managers, I came across QtPass. It seems nice but once when I wanted to delete a file, it showed me "Delete /?", I assumed that it just meant to remove all the folders within the directory, that is, delete all my passwords. So I chose it, it deleted every file off my computer, didn't put it in trash, just gone.

Looking up the sender told me this was most probably on Ubuntu linux.
Have tried to reproduce, but couldn't so-far.

@annejan annejan added bug Linux labels Mar 2, 2017

@annejan annejan self-assigned this Mar 2, 2017

@annejan

This comment has been minimized.

Member

annejan commented Mar 2, 2017

The affected user sent me his QtPass.conf file, that only contained [mainwindow] layout related information.

It seems that QtPass was never properly initialised.
But that should not matter. . QtPass should never be able to manipulate files outside of the (current) password-store folder.

I'm keeping this ticket as a reminder to write some safeguards on the (native) handlers to never be able to operate outside of the store.

@jounathaen

This comment has been minimized.

Member

jounathaen commented Mar 2, 2017

Hmm, just wondering how/if it could remove files outside the home folder, as when running the program as non-su you wouldn't have permissions to do so. My guess would be, that the home folder was choosen as password-store. (Maybe qtpass should catch initializing the home folder as pwd-store with a pop up warning?)
But I'm just guessing.

@FiloSpaTeam

This comment has been minimized.

Contributor

FiloSpaTeam commented Mar 30, 2018

I think QtPass should check files to remove before cleanup. So even if you have photo or videos in this folder the delete method will skip those files.

Which types of file QtPass handles? Only gpg files?

UPDATE:
I check the code, we can handle this before exec the rm command, the message shown to the user can be rewritten to: "Every file related to QtPass in this directory will be deleted. Want you?" and if the directory is empty will be deleted too.

@annejan

This comment has been minimized.

Member

annejan commented Mar 30, 2018

There are two file types . . .gpg and .gpg-id

@FiloSpaTeam

This comment has been minimized.

Contributor

FiloSpaTeam commented Apr 3, 2018

Ok i found a way. The problem itself is "pass" because the command rm doesn't accept specific arguments like Test/*.gpg so i need to:

  • select all keys under that folder and delete singularly
  • check if dir is empty and delete them if needed

It that case no external pass file are deleted accidentally and if you mix something you are safe.

@annejan

This comment has been minimized.

Member

annejan commented Apr 3, 2018

My main question is, why would there ever be non pass related files in your .password-store folder?

@FiloSpaTeam

This comment has been minimized.

Contributor

FiloSpaTeam commented Apr 3, 2018

I have the same question LoL but i think that someone doesn't trouble if something goes into this folder (it's a folder anyway).
I imagine that: if i created my wallet in the home_path (i understand is not a good thing) but i don't know very well how it works or something else and i need to delete my 2 test passwords, i lose everything and it's unclear because in the UI those files (my files) are filtered out.

I think it's related to bad user experience more than i should do or i shouldn't do this.

@annejan

This comment has been minimized.

Member

annejan commented Apr 3, 2018

Perhaps scanning the folder for non pass files and warning would be a nice clean feature instead.

@FiloSpaTeam

This comment has been minimized.

Contributor

FiloSpaTeam commented Apr 3, 2018

I can move in 2 ways:

  • Change delete function as described above
  • Change dinamically the deletion message and warn the user that the folder contains something else

Which do you prefer?

@annejan

This comment has been minimized.

Member

annejan commented Apr 3, 2018

Since the second option doesn't involve changing the way pass handles things (or rather writing more code to bypass pass) and QtPass should just be a UI for pass (even though it does a lot more now) I think my preference goes towards the second option.

@FiloSpaTeam

This comment has been minimized.

Contributor

FiloSpaTeam commented Apr 3, 2018

Ok so goes to this way. Warn the user or change something to prevent errors are the same as result.
But the first is a more conscious choice. 👍

@annejan

This comment has been minimized.

Member

annejan commented Apr 3, 2018

Scanning the folder for non pass related files should be rather trivial.
Having the result trigger a different warning would be a nice addition.

@FiloSpaTeam

This comment has been minimized.

Contributor

FiloSpaTeam commented Apr 3, 2018

I thought we can exec an rm function with the specified folder and arguments like *.gpg and so on but as you said it's a GUI for Pass, so we should just have same pass experience with more "salt"

@jounathaen

This comment has been minimized.

Member

jounathaen commented Apr 9, 2018

My main question is, why would there ever be non pass related files in your .password-store folder?

It could happen, that if you sync the folder with other tools than git, these tools leave some lock files or other hidden files. I think I had this, when I was syncing to android with "Folder Sync" app...

On the other hand: These files are pass "related" and can be removed with the folder...

@annejan annejan closed this in 04c7c89 Apr 9, 2018

annejan added a commit that referenced this issue Apr 9, 2018

@annejan

This comment has been minimized.

Member

annejan commented Apr 9, 2018

@FiloSpaTeam changes have been merged

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