search

YunoHost-Apps / vaultwarden_ynh

Open source password management solutions for YunoHost
https://bitwarden.com/
GNU General Public License v3.0
57 stars 17 forks source link

Old resources are not cleaned up on update #68

Closed ghost closed 4 years ago

ghost commented 4 years ago

When updating Bitwarden (following the regular Yunohost upgrade process), the complete app is rebuilt. New resources are then created, but the old resources (produced with the initial install or the previous updates) are not removed.

These resources can be found in /var/www/bitwarden/live/build /var/www/bitwarden/live/deps

Checking these folders, it can easily be observed that files and directories are duplicated each time an update is applied (the "modified date" corresponds to the update time)

These leads the app to get a lot bigger each time an update is applied (which is kind of a big issue when having a periodic backup process)

ghost commented 4 years ago

Having a closer look on this, I think I understand why this happens. In the install/upgrade scripts we do:

# Install bitwarden_rs
cp -a "$final_path"/build/target/release/. "$final_path"/live/.
# Remove build files and rustup
ynh_secure_remove --file="$final_path/build"

Which means next time we build the app (next time we update), cargo have lost track on generated files, and cannot do the cleaning. When copying the new generated files to the live folder, we then duplicate these files instead of replacing them.

An easy fix I can suggest is to add:

ynh_secure_remove --file="$final_path/live/build"
ynh_secure_remove --file="$final_path/live/deps"
ynh_secure_remove --file="$final_path/live/.fingerprint"

before cp -a "$final_path"/build/target/release/. "$final_path"/live/. in the upgrade script

yalh76 commented 4 years ago 
ynh_secure_remove --file="$final_path/live/build"
ynh_secure_remove --file="$final_path/live/deps"
ynh_secure_remove --file="$final_path/live/.fingerprint

Or maybe we should delete the $final_path/live before copying the files ?

ghost commented 4 years ago

I don't think this is a good idea. That would actually break Bitwarden because it would erase the data directory containing the password database and the session key. Also, I'm not sure about the web-vault: i'm not sure it is always re-downloaded when upgrading the app. More generally speaking, any file which wouldn't have been generated on build time would be permanently lost.

yalh76 commented 4 years ago

seems like all data are stored in the data/folder https://github.com/YunoHost-Apps/bitwarden_ynh/blob/master/conf/bitwarden_rs.env

web-vault is upgraded during upgrades

Tested that on https://github.com/YunoHost-Apps/bitwarden_ynh/tree/fix_upgrade

ghost commented 4 years ago

Saving bitwarden_rs.env is good, it should be done indeed :ok_hand: But I'm no big fan of the approach of removing everything (but the data folder). Let's consider a use case:

In general I think it is safer to delete only what we know can be with no risk, because anyway the command cp -a "$final_path"/build/target/release/. "$final_path"/live/. will overwrite all the other files.

I've updated the PR #69 to backup/restore bitwarden_rs.env

yalh76 commented 4 years ago

Saving bitwarden_rs.env is good, it should be done indeed 👌 But I'm no big fan of the approach of removing everything (but the data folder). Let's consider a use case:

* Someone want to use a folder named `secret-data` instead of `data`

* Changes have been made in `bitwarden_rs.env` accordingly

* On upgrade, the file `bitwarden_rs.env` will be correctly restored with the changes, but all the data won't be because the `secret-data` folder is lost

The secret-data folder is not lost, it will be in the backup done at the beginning of the upgrade. https://github.com/YunoHost-Apps/bitwarden_ynh/blob/c67bb939aed9a24a96eb8c754ff0e3ee01236ce1/scripts/upgrade#L55-L68

In general I think it is safer to delete only what we know can be with no risk, because anyway the command cp -a "$final_path"/build/target/release/. "$final_path"/live/. will overwrite all the other files.

cp -a is enough to overwrite the files ? We don't need cp -af ?

I've updated the PR #69 to backup/restore bitwarden_rs.env

I'm currently checking if it's needed. not sure that there is a bitwarden_rs.env file in $final_path"/build/target/release/ at the end of the build

yalh76 commented 4 years ago

Seems like there is no bitwarden_rs.env in $final_path/build/target/release/ so maybe we can remove the backup part of bitwarden_rs.env during upgrade ?

ghost commented 4 years ago

Seems like there is no bitwarden_rs.env in $final_path/build/target/release/ so maybe we can remove the backup part of bitwarden_rs.env during upgrade ?

You are right, that's useless. Everything is already handled correctly in the "MODIFY A CONFIG FILE" section. I've removed it.

cp -a is enough to overwrite the files ? We don't need cp -af ?

By default "cp" overwrite files, but you are right we can add the "--force" option. I did it.