search

ttionya / vaultwarden-backup

Backup vaultwarden (formerly known as bitwarden_rs) SQLite3/PostgreSQL/MySQL/MariaDB database by rclone. (Docker)
MIT License
1.04k stars 119 forks source link

escaped every parameter in all scripts #10

Closed mustaphazorgati closed 3 years ago

mustaphazorgati commented 3 years ago

Hey @ttionya,

so sorry for creating so many PRs lately.

Since we've introduced BACKUP_FILE_DATE_SUFFIX the user can create a backup file which may contain a space character. This will cause some issues because the scripts are not completely escaped.

In order to fix this and prevent further additions to break the current code I escaped every variable in all scripts. Furthermore I replaced the scripts with a bourne shell in order to use the bash arrays. We need bash arrays in order to delete backup files which contain a space character.

Again: I didn't test the changes, but I am confident that this won't break anything.

ttionya commented 3 years ago

Thank you for the PRs.

I tested the code for efd91f5c50b2010249031efb4e584d5514663db9 and found that the space character caused a lot of unexpected problems.

I think the easiest way to deal with this is to limit the character set of the BACKUP_FILE_DATE_SUFFIX environment variable, since it was introduced for the sole purpose of creating unique backup file names.

I would REVERT this PR and restrict BACKUP_FILE_DATE_SUFFIX to allow only [0-9a-zA-Z_%-].

That's the easiest way to do it, isn't it?

mustaphazorgati commented 3 years ago

Hey @ttionya! Thank you for investing this.

Personally I think the restriction is fine.

What kind of issues came up? How are you testing? The :nerd_face: in me wants to fix the issues and make sure nothing breaks and will break (in the future) if any character needs to be escaped. I am willing to invest some time and get rid of all issues.