feat: add SFTP, FTP, and Google Drive backup destinations#4096
feat: add SFTP, FTP, and Google Drive backup destinations#4096marcoberutti wants to merge 1 commit intoDokploy:canaryfrom
Conversation
Closes Dokploy#416 - Add destinationType (text, default s3) and providerConfig (jsonb) columns to destination table - Migration: 0155_backup_multi_destination.sql with defaults on S3 credential columns - New getRcloneConfig() utility returning {preamble, flags, remotePath} for all types - All backup/restore executors migrated from getS3Credentials to getRcloneConfig - testConnection router handler supports sftp/ftp/gdrive/s3 - handle-destinations.tsx UI rewritten with dynamic provider-specific form fields
dosubot
bot
added
the
size:XXL
|
Related Documentation 1 document(s) may need updating based on files changed in this PR: Dokploy's Space README
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| `rclone copyto ${rclone.flags.join(" ")} "${backupPath}" "${tempDir}/${backupFile}"`, | ||
| ); |
There was a problem hiding this comment.
rclone.flags for SFTP includes --sftp-pass="$RCLONE_SFTP_PASS" and for FTP --ftp-pass="$RCLONE_FTP_PASS", but the preamble that defines those variables (RCLONE_SFTP_PASS=$(rclone obscure "...")) is never executed here. The shell variable will expand to an empty string and rclone will attempt authentication with an empty password, causing all SFTP/FTP restores to fail.
The same bug is present in every other restore file: restore/compose.ts, restore/libsql.ts, restore/mariadb.ts, restore/mongo.ts, restore/mysql.ts, and restore/postgres.ts — none of them prepend rclone.preamble before executing the rclone command.
Compare to the backup path in getBackupCommand (backups/utils.ts) which correctly injects the preamble into the shell script via the rclonePreamble parameter.
| const sortAndPick = `sort -r | tail -n +$((${keepLatestCount}+1)) | xargs -I{}`; | ||
| const deleteCommand = `rclone delete ${rcloneFlags.join(" ")} ${backupFilesPath}{}`; | ||
| const deleteCommand = `rclone delete ${rclone.flags.join(" ")} ${backupFilesPath}{}`; | ||
| const fullCommand = `${listCommand} | ${sortAndPick} ${deleteCommand}`; |
There was a problem hiding this comment.
fullCommand uses rclone.flags but never runs rclone.preamble first. For SFTP/FTP destinations the password environment variable (RCLONE_SFTP_PASS / RCLONE_FTP_PASS) will be unset when rclone lsf and rclone delete execute, causing the cleanup to fail silently (error is caught and swallowed). Note that keepLatestNBackups in backups/index.ts correctly prepends the preamble.
| const port = cfg?.port ?? "22"; | ||
| const remotePath = cfg?.remotePath ?? "/"; | ||
| return { | ||
| preamble: `RCLONE_SFTP_PASS=$(rclone obscure "${cfg?.password ?? ""}")`, |
There was a problem hiding this comment.
The password is interpolated directly into a double-quoted shell substitution. A password such as x"$(id)" would execute arbitrary commands. The same issue exists on line 145 for FTP, and in testConnection (destination.ts router lines 60 and 66). Use single-quoting with escaped single-quotes to prevent command substitution.
1 participant
What is this PR about?
Adds SFTP, FTP, and Google Drive as backup destination types alongside existing S3 support. Previously only S3 was supported. This allows users to backup databases and volumes to SFTP servers, FTP servers, or Google Drive using a service account.
Checklist
Issues related
closes #416
Changes
/claim #416
Greptile Summary
This PR adds SFTP, FTP, and Google Drive as backup destinations alongside existing S3 support. The core architecture is sound, but three P1 defects were found: shell injection via unescaped passwords in SFTP/FTP preamble commands, and missing preamble in all restore files and volume-backup cleanup (causing SFTP/FTP restores and cleanup to always fail).
Confidence Score: 4/5
Not safe to merge: SFTP/FTP restores are broken at runtime and passwords are vulnerable to shell injection
Three P1 issues requiring fixes before merge; the backup path is correct but restore and cleanup paths were overlooked
packages/server/src/utils/restore/ (all 7 files), packages/server/src/utils/volume-backups/utils.ts, packages/server/src/utils/backups/utils.ts lines 112+145, apps/dokploy/server/api/routers/destination.ts lines 60+66
Reviews (1): Last reviewed commit: "feat: add SFTP, FTP, and Google Drive ba..." | Re-trigger Greptile
(5/5) You can turn off certain types of comments like style here!