feat: add SFTP and rclone backup destination support#3732
feat: add SFTP and rclone backup destination support#3732285729101 wants to merge 2 commits intoDokploy:canaryfrom
Conversation
Extends the backup system beyond S3-only storage to support SFTP servers and any rclone-compatible backend (Google Drive, OneDrive, Azure Blob, FTP, Dropbox, etc.). Changes: - Add destinationType enum (s3, sftp, rclone) to destination schema - Add SFTP fields (host, port, username, password, keyPath, remotePath) - Add generic rclone config fields (config, remoteName, remotePath) - Create unified helper functions that dispatch based on destination type - Update all backup handlers (postgres, mysql, mariadb, mongo, compose, web-server) to use destination-agnostic rclone commands - Update all restore handlers similarly - Update volume backup/cleanup utilities - Update destination API router with type-aware test connection - Update frontend with destination type selector and conditional forms - Add database migration for new columns and enum Closes Dokploy#416 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| if (sftpPassword) { | ||
| rcloneFlags.push(`--sftp-pass="$(rclone obscure '${sftpPassword}')"`); |
There was a problem hiding this comment.
shell injection vulnerability - sftpPassword not escaped before shell execution
if password contains quotes or special chars like '; rm -rf /;' will execute arbitrary commands
| rcloneFlags.push(`--sftp-pass="$(rclone obscure '${sftpPassword}')"`); | |
| rcloneFlags.push(`--sftp-pass="$(rclone obscure "${sftpPassword}")"`); |
| rcloneFlags.push(`--sftp-port="${sftpPort}"`); | ||
| } | ||
| if (sftpPassword) { | ||
| rcloneFlags.push(`--sftp-pass="$(rclone obscure '${sftpPassword}')"`); |
There was a problem hiding this comment.
same shell injection vulnerability as in utils.ts - password must be escaped
| rcloneFlags.push(`--sftp-pass="$(rclone obscure '${sftpPassword}')"`); | |
| rcloneFlags.push(`--sftp-pass="$(rclone obscure "${sftpPassword}")"`); |
| const configSetup = rcloneConfig | ||
| ? `RCLONE_CONFIG_FILE=$(mktemp /tmp/rclone-config-XXXXXX.conf) && cat > "$RCLONE_CONFIG_FILE" << 'RCLONE_EOF'\n${rcloneConfig}\nRCLONE_EOF\nexport RCLONE_CONFIG="$RCLONE_CONFIG_FILE" && ` | ||
| : ""; | ||
| const remoteName = rcloneRemoteName || "remote"; | ||
| const remotePath = (rcloneRemotePath || "").replace(/\/+$/, ""); | ||
| rcloneCommand = `${configSetup}rclone ls --retries 1 --low-level-retries 1 --timeout 10s --contimeout 5s "${remoteName}:${remotePath}"`; |
There was a problem hiding this comment.
temp config file created but never cleaned up - causes file leak in /tmp
the inline config setup doesn't include trap for cleanup. wrap in subshell with trap or reuse getRcloneConfigSetup()
| const listCommand = `${configSetup}rclone lsf ${rcloneFlags.join(" ")} --include "${volumeName}-*.tar" ${backupFilesPath}`; | ||
| const sortAndPick = `sort -r | tail -n +$((${keepLatestCount}+1)) | xargs -I{}`; | ||
| const deleteCommand = `rclone delete ${rcloneFlags.join(" ")} ${backupFilesPath}{}`; | ||
| const fullCommand = `${listCommand} | ${sortAndPick} ${deleteCommand}`; |
There was a problem hiding this comment.
same issue - rclone config not accessible in delete command
the delete command at line 94 won't have access to the config file or env vars set in configSetup
| return ""; | ||
| } | ||
|
|
||
| const configContent = destination.rcloneConfig.replace(/'/g, "'\\''"); |
There was a problem hiding this comment.
configContent variable created but never used - line 211 uses raw destination.rcloneConfig instead
not a bug (heredoc delimiter is quoted so safe) but unused variable
| `--sftp-host="${sftpHost || ""}"`, | ||
| `--sftp-user="${sftpUsername || ""}"`, |
There was a problem hiding this comment.
shell injection - sftpHost and sftpUsername need escaping
same issue as password - these are directly interpolated into commands
| } | ||
|
|
||
| if (sftpKeyPath) { | ||
| rcloneFlags.push(`--sftp-key-file="${sftpKeyPath}"`); |
There was a problem hiding this comment.
shell injection - sftpKeyPath needs escaping
|
|
||
| switch (destType) { | ||
| case "s3": | ||
| return `:s3:${destination.bucket}/${subPath}`; |
There was a problem hiding this comment.
shell injection - destination.bucket and subPath interpolated without escaping
these params are used in shell commands and could contain special chars
| /\/+$/, | ||
| "", | ||
| ); | ||
| return `:sftp:${remotePath}/${subPath}`; |
There was a problem hiding this comment.
shell injection - remotePath needs escaping
| const remoteName = destination.rcloneRemoteName || "remote"; | ||
| const remotePath = (destination.rcloneRemotePath || "").replace( | ||
| /\/+$/, | ||
| "", | ||
| ); | ||
| return `${remoteName}:${remotePath}/${subPath}`; |
There was a problem hiding this comment.
shell injection - remoteName and remotePath need escaping
remote name especially critical since it's user-controlled and appears before the colon in rclone paths
Additional Comments (2)
need to wrap entire pipeline in subshell or pass config to both commands
if |
|
@Siumauricio adds SFTP and rclone backup destination support alongside the existing S3 option. |
Add shellEscape() helper to properly escape all user-supplied values before they are interpolated into shell commands. This addresses shell injection vulnerabilities in: - SFTP password, host, username, key path (both utils.ts and destination.ts) - S3 access key, secret key, region, endpoint, provider - Rclone remote name, remote path, bucket, and subPath values - Rclone config content (use escaped configContent variable) Also fix: - Temp rclone config file now cleaned up via trap on EXIT - configSetup moved to wrap entire command in volume-backups/utils.ts so rclone config is accessible during both list and delete operations
|
@Siumauricio I've addressed all 10 shell injection vulnerabilities flagged by the greptile bot. Here's a summary of the fixes: Shell escaping (issues 1, 2, 6, 7, 8, 9, 10):
Temp file cleanup (issue 3):
Rclone config not accessible in delete command (issue 4):
Unused variable (issue 5):
|
|
@Siumauricio I've addressed all the review feedback: Security fixes:
Cleanup fixes:
Ready for review! |
1 participant
Summary
Resolves #416
This PR extends Dokploy's backup system beyond S3-only storage to support SFTP servers and any rclone-compatible backend (Google Drive, OneDrive, Azure Blob Storage, FTP, Dropbox, Backblaze B2, and 40+ more).
What changed
destinationTypeenum (s3,sftp,rclone) and new columns for SFTP credentials and generic rclone configurationgetRcloneFlags(),getRcloneDestinationPath(),getRcloneBasePath(), andgetRcloneConfigSetup()that dispatch based on destination type — replacing hardcoded S3 paths throughout the codebasetestConnectionmutation now handles S3, SFTP, and rclone destinations with appropriate test commands0145_backup_destinations.sqladds the enum and columnsHow it works
The existing codebase already uses
rclonefor S3 operations, so extending to other backends is natural:--s3-access-key-id,--s3-secret-access-key, etc. passed as rclone flags--sftp-host,--sftp-user,--sftp-passflags (password obfuscated viarclone obscure)Backward compatibility
destinationTypecolumn defaults tos3DEFAULT ''to allow nullable-like behavior for non-S3 destinationsgetS3Credentials()helper is preserved for backward compatibilityTest plan
keepLatestNBackupscleanup works for all destination types🤖 Generated with Claude Code
Greptile Summary
Extends Dokploy's backup system from S3-only to support SFTP and rclone-compatible backends (Google Drive, OneDrive, Azure, etc.). The implementation adds new database schema fields, unified helper functions that dispatch based on
destinationType, and updates all backup/restore handlers across postgres, mysql, mariadb, mongo, compose, and web-server.Critical Issues Found:
Shell injection vulnerabilities - Multiple user-controlled fields (SFTP password, host, username, key path, S3 credentials, rclone remote names/paths, bucket names) are interpolated directly into shell commands without escaping. An attacker could inject arbitrary shell commands through these fields.
Rclone config not accessible in cleanup operations - The backup and volume cleanup functions create rclone config files and export environment variables, but when the commands are piped through
xargs, the delete commands run in a separate subshell without access to the config file or environment. This will cause cleanup to fail for generic rclone destinations.Temp file leak in testConnection - The rclone test connection creates a temporary config file but doesn't set up cleanup trap, causing file accumulation in
/tmp.Architecture:
The refactoring follows a clean pattern -
getRcloneFlags(),getRcloneDestinationPath(),getRcloneBasePath(), andgetRcloneConfigSetup()provide type-based dispatch, and all backup/restore handlers were systematically updated. The database migration properly defaults existing rows to S3 type and adds appropriate nullable columns for new destination types.Confidence Score: 1/5
packages/server/src/utils/backups/utils.ts(shell injection and config scoping),apps/dokploy/server/api/routers/destination.ts(test connection issues),packages/server/src/utils/backups/index.tsandpackages/server/src/utils/volume-backups/utils.ts(cleanup failures)Last reviewed commit: 77a6967
(2/5) Greptile learns from your feedback when you react with thumbs up/down!