feat: add SFTP and FTP backup destination support by ljapptest-art · Pull Request #4108 · Dokploy/dokploy

ljapptest-art · 2026-03-30T05:09:44Z

Summary

This PR adds support for SFTP and FTP backup destinations, addressing issue #416.

Features

New destination types: SFTP and FTP in addition to existing S3
Secure password handling: Passwords are passed via rclone obscure + environment variables
Full backward compatibility: Existing S3 destinations continue to work without changes

Implementation Details

Schema Changes

Added destinationType field (s3 | sftp | ftp)
Added SFTP/FTP connection fields: host, port, username, password, remotePath
Made S3-specific fields optional to support non-S3 destinations

Security Improvements

Environment variable-based password handling: Prevents shell injection attacks
Input escaping: All user inputs are properly escaped for shell safety
No direct string interpolation: Sensitive data never directly interpolated in shell commands

Why This Approach is Better

I noticed other PRs for this issue have shell injection vulnerabilities. For example, if a password contains '; rm -rf /; echo '`, it could break out of the shell quoting and execute arbitrary commands.

This PR avoids that by:

Using rclone obscure to encode the password
Passing the encoded password via environment variables (RCLONE_SFTP_PASS, RCLONE_FTP_PASS)
Never interpolating passwords directly into shell command strings

Files Changed

packages/server/src/db/schema/destination.ts - Schema with discriminated union
packages/server/src/utils/backups/utils.ts - getRcloneConfig() with secure handling
packages/server/src/utils/backups/*.ts - Updated all backup operations
packages/server/src/utils/restore/*.ts - Updated all restore operations
apps/dokploy/server/api/routers/destination.ts - Updated testConnection
apps/dokploy/drizzle/0156_add_destination_types.sql - Database migration

Testing

Type checking passes
All backup/restore operations updated to use new config

Closes #416

Greptile Summary

This PR adds SFTP and FTP as new backup destination types alongside the existing S3-compatible backend. The implementation is well-structured: a central getRcloneConfig() helper replaces the S3-only getS3Credentials(), and all backup/restore call sites are updated in a uniform, mechanical way. The password-handling approach — running rclone obscure in a shell preamble and exposing the result via an environment variable — is the correct way to avoid both shell injection and plaintext credential exposure on the command line. The single-quote escaping in the preamble is implemented correctly.

Key observations:

No frontend UI is included. SFTP and FTP destinations cannot be created or managed through the web interface with this PR alone. A follow-up UI PR will be required before the feature is usable by end users.
Hardcoded "S3" strings remain in web-server.ts log messages ("Uploaded backup to S3 ✅", "Downloading backup from S3...") even though the paths now support all three destination types.
The testConnection handler in destination.ts has redundant conditional spreading (the ...input spread already covers all fields) and uses an as any cast to suppress the resulting type mismatch.
The update mutation's catch block still emits "Error connecting to bucket".
The port field is a free-form string with no numeric range validation (1–65535).
SFTP/FTP passwords are stored as plaintext in the database, consistent with how S3 keys are stored today, but worth revisiting given that these are often reused personal credentials.

Confidence Score: 4/5

Safe to merge after addressing the missing frontend UI and minor stale-message/code-quality issues; the core security approach is sound.

All remaining findings are P2: misleading log messages, redundant code in testConnection, a stale error string, and no port-range validation. The core password-handling logic (rclone obscure + env-var preamble with correct single-quote escaping) is implemented correctly and the migration is idempotent. The main gap is the absence of any frontend UI, which makes the feature inaccessible to end users via the web interface.

apps/dokploy/server/api/routers/destination.ts (redundant spreading + stale error message), packages/server/src/utils/backups/web-server.ts (stale S3 log messages), packages/server/src/db/schema/destination.ts (port validation, plaintext password)

Important Files Changed

Filename	Overview
packages/server/src/utils/backups/utils.ts	Core logic change replacing getS3Credentials with getRcloneConfig. SFTP/FTP passwords are securely obscured via rclone obscure and passed through an env-var preamble; single-quote escaping is correct. S3 values now go through escapeShellValue. Implementation is sound.
apps/dokploy/server/api/routers/destination.ts	testConnection updated to use getRcloneConfig but contains redundant conditional spreading and an as any cast. The update mutation error message still says 'Error connecting to bucket'. create/update paths work correctly.
packages/server/src/db/schema/destination.ts	Schema extended with SFTP/FTP fields and a destinationType discriminator. S3 fields made nullable. Discriminated union Zod schemas are well structured. Port has no numeric range validation; password is stored in plaintext.
apps/dokploy/drizzle/0156_add_destination_types.sql	Migration uses IF NOT EXISTS guards and DROP NOT NULL on S3 columns. Safe and idempotent.
packages/server/src/utils/backups/web-server.ts	Mechanical swap from getS3Credentials to getRcloneConfig. Log messages 'Running command to upload backup to S3' and 'Uploaded backup to S3 ✅' are now misleading for SFTP/FTP destinations.

Comments Outside Diff (4)

packages/server/src/utils/backups/web-server.ts, line 606-608 (link)

Stale S3-specific log messages

These log messages still say "S3" even though the function now supports SFTP and FTP destinations. Users will see misleading messages like "Uploaded backup to S3 ✅" when they're actually uploading to an SFTP or FTP server.

The same issue exists in the restore path at packages/server/src/utils/restore/web-server.ts (~line 807: "Downloading backup from S3...").
packages/server/src/db/schema/destination.ts, line 163 (link)

SFTP/FTP password stored as plaintext

The password column persists the SFTP/FTP password in plaintext. Unlike S3 access keys (which are generally purpose-created API credentials), SFTP/FTP passwords are often shared credentials that users reuse across systems. A database dump would expose these credentials directly.

Consider storing an encrypted form at rest or at minimum documenting this risk clearly. This is consistent with how S3 secrets are handled today, but worth reconsidering now that personal passwords are involved.
packages/server/src/db/schema/destination.ts, line 210-220 (link)

Port accepted as a free-form string with no range validation

port is a z.string() with no further constraints. A user could supply "0", "99999", or any non-numeric string. The error they get back from rclone will be cryptic rather than a friendly validation message.

Consider adding a Zod refinement to enforce a valid port number (1–65535). The same applies to the FTP schema.
apps/dokploy/server/api/routers/destination.ts, line 193 (link)

Stale error message in update handler

The error message still reads "Error connecting to bucket" even though this handler now deals with SFTP and FTP destinations as well.

_{Reviews (1): Last reviewed commit: "feat: add SFTP and FTP backup destinatio..." | Re-trigger Greptile}

Greptile also left 1 inline comment on this PR.

_{(2/5) Greptile learns from your feedback when you react with thumbs up/down!}

- Add destinationType field to schema (s3, sftp, ftp) - Add SFTP/FTP connection fields (host, port, username, password, remotePath) - Implement secure rclone configuration with environment variable-based password handling - Fix shell injection vulnerability in existing implementations - Update all backup and restore operations to support new destination types - Add database migration for new fields Security improvements: - Passwords are passed via rclone obscure and environment variables - All user inputs are properly escaped for shell safety - No direct string interpolation in shell commands for sensitive data Addresses: Dokploy#416

greptile-apps · 2026-03-30T05:15:33Z

apps/dokploy/server/api/routers/destination.ts

+				// Build a temporary destination object from input
+				const tempDestination = {
+					...input,
+					destinationId: "temp",
+					createdAt: new Date(),
+					organizationId: "temp",
+					destinationType: input.destinationType,
+					// Map input fields based on type
+					...(input.destinationType === "s3" ? {
+						accessKey: input.accessKey,
+						secretAccessKey: input.secretAccessKey,
+						bucket: input.bucket,
+						region: input.region,
+						endpoint: input.endpoint,
+						provider: input.provider,
+						additionalFlags: input.additionalFlags,
+					} : {}),
+					...(input.destinationType === "sftp" ? {
+						host: input.host,
+						port: input.port,
+						username: input.username,
+						password: input.password,
+						remotePath: input.remotePath,
+					} : {}),
+					...(input.destinationType === "ftp" ? {
+						host: input.host,
+						port: input.port,
+						username: input.username,
+						password: input.password,
+						remotePath: input.remotePath,
+					} : {}),
+				} as any;


Redundant field spreading and unsafe as any cast

The ...input spread on line 55 already copies every field from the discriminated union onto tempDestination. The three subsequent conditional spreads are therefore no-ops — they just re-assign the same properties. The as any cast is needed only to silence the resulting type mismatch, hiding legitimate type errors.

Both issues can be fixed by constructing the object once with explicit nullable defaults for fields that might be absent in a given union branch.

ljapptest-art requested a review from Siumauricio as a code owner March 30, 2026 05:09

dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Mar 30, 2026

github-actions bot closed this Mar 30, 2026

dosubot bot added the enhancement New feature or request label Mar 30, 2026

greptile-apps bot reviewed Mar 30, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: add SFTP and FTP backup destination support#4108

feat: add SFTP and FTP backup destination support#4108
ljapptest-art wants to merge 1 commit intoDokploy:canaryfrom
ljapptest-art:feat/secure-sftp-ftp-backup

ljapptest-art commented Mar 30, 2026 •

edited by greptile-apps bot

Loading

Uh oh!

greptile-apps bot Mar 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

ljapptest-art commented Mar 30, 2026 • edited by greptile-apps bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Features

Implementation Details

Schema Changes

Security Improvements

Why This Approach is Better

Files Changed

Testing

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Comments Outside Diff (4)

Uh oh!

greptile-apps bot Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

ljapptest-art commented Mar 30, 2026 •

edited by greptile-apps bot

Loading