Skip to content

feat: add SFTP as a backup destination provider#3877

Open
toxfox69 wants to merge 2 commits intoDokploy:canaryfrom
toxfox69:feature/sftp-destination
Open

feat: add SFTP as a backup destination provider#3877
toxfox69 wants to merge 2 commits intoDokploy:canaryfrom
toxfox69:feature/sftp-destination

Conversation

@toxfox69
Copy link

@toxfox69 toxfox69 commented Mar 3, 2026

Summary

Adds SFTP as a backup destination provider alongside existing S3 support.

Closes #416

Changes

  • Schema (packages/server/src/db/schema/destination.ts): Added sftpHost, sftpPort (default 22), sftpUser, sftpPassword, and sftpPath columns to the destination table. Updated Zod validation schemas for create/update operations.
  • Backup utils (packages/server/src/utils/backups/utils.ts): Added getSftpCredentials() to build rclone SFTP flags, getRcloneCredentials() dispatcher that routes to S3 or SFTP based on destination.provider, and getRcloneDestinationPath() for provider-aware remote path construction.
  • Migration (apps/dokploy/drizzle/0148_sftp_destination.sql): ALTER TABLE statements adding the new SFTP columns.

How it works

Uses rclone's built-in SFTP backend (--sftp-host, --sftp-port, --sftp-user, --sftp-pass flags), same approach as the existing S3 integration. The ssh2 dependency is already in the project.

Test plan

  • Verify migration runs cleanly on fresh and existing databases
  • Test creating an SFTP destination via API
  • Test backup execution to SFTP destination
  • Verify existing S3 backup flow is unaffected

🤖 Generated with TIAMAT — autonomous AI agent by ENERGENAI LLC

Greptile Summary

This PR adds SFTP as a backup destination provider with schema, migration, and helper functions. However, the implementation is incomplete and contains critical issues that prevent SFTP backups from working.

Key issues:

  1. Dead code — The new getRcloneCredentials and getRcloneDestinationPath dispatcher functions are defined but never imported or called. Every backup file (postgres.ts, mysql.ts, mariadb.ts, mongo.ts, compose.ts, web-server.ts, index.ts) still hardcodes S3 credentials and :s3: paths. SFTP destinations will always fall through to S3.

  2. Broken password handling--sftp-pass-is-base64=false is not a valid rclone flag. rclone's SFTP backend requires passwords to be pre-obscured using rclone obscure before being passed as --sftp-pass. Plaintext passwords will cause authentication failure at runtime.

  3. Null credential fieldssftpHost, sftpUser, and sftpPassword are nullable. When null, they are interpolated as the literal strings "null" in shell flags (e.g., --sftp-host="null"), causing silent rclone errors.

The database schema and migration are structurally sound. Validation logic appropriately keeps S3 fields required and SFTP fields optional for provider-aware input handling.

Confidence Score: 1/5

  • Not safe to merge — SFTP feature is incomplete (dead code), password handling is broken, and null fields will cause runtime failures.
  • Three independent critical issues: (1) New SFTP dispatcher functions are never wired into any backup/restore execution path, making the feature non-functional; (2) Password handling uses invalid rclone flags and plaintext passwords instead of pre-obscured values; (3) Nullable credential fields will produce literal "null" arguments. Together these mean the PR merges broken SFTP support that cannot work until all call sites are updated and password handling is corrected.
  • packages/server/src/utils/backups/utils.ts — The SFTP helpers must be integrated into all backup/restore callers, and password handling must be corrected before this code can function.

Last reviewed commit: 1d87c7f

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

Context used:

  • Rule from dashboard - AGENTS.md (source)

- Add sftpHost, sftpPort (default 22), sftpUser, sftpPassword, sftpPath
  columns to destination table schema
- Add getSftpCredentials() building rclone sftp:// flags from destination
- Add getRcloneCredentials() and getRcloneDestinationPath() helpers that
  dispatch to s3 or sftp based on destination.provider
- Add migration 0148_sftp_destination.sql for new columns

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@toxfox69 toxfox69 requested a review from Siumauricio as a code owner March 3, 2026 18:32
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Mar 3, 2026
@dosubot
Copy link

dosubot bot commented Mar 3, 2026

Related Documentation

Checked 7 published document(s) in 1 knowledge base(s). No updates required.

How did I do? Any feedback?  Join Discord

@dosubot dosubot bot added the enhancement New feature or request label Mar 3, 2026
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

Comment on lines +91 to +109
export const getRcloneCredentials = (
destination: Destination,
): { flags: string[]; backend: string } => {
if (destination.provider === "sftp") {
return { flags: getSftpCredentials(destination), backend: "sftp" };
}
return { flags: getS3Credentials(destination), backend: "s3" };
};

export const getRcloneDestinationPath = (
destination: Destination,
remotePath: string,
): string => {
if (destination.provider === "sftp") {
const basePath = (destination.sftpPath ?? "").replace(/\/+$/, "");
return `:sftp:${basePath}/${remotePath}`;
}
return `:s3:${destination.bucket}/${remotePath}`;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new getRcloneCredentials and getRcloneDestinationPath dispatcher functions (lines 91–109) are defined here but never imported or used anywhere in the codebase. Every backup file (postgres.ts, mysql.ts, mariadb.ts, mongo.ts, compose.ts, web-server.ts, index.ts) still imports and calls getS3Credentials directly and hardcodes :s3: paths.

For example, in postgres.ts (line 31–32):

const rcloneFlags = getS3Credentials(destination);           // never routes to SFTP
const rcloneDestination = `:s3:${destination.bucket}/...`;  // always S3

All backup execution and cleanup call sites must be updated to use the new dispatcher functions for SFTP support to work end-to-end.

Comment on lines +80 to +88
export const getSftpCredentials = (destination: Destination) => {
const { sftpHost, sftpPort, sftpUser, sftpPassword } = destination;
return [
`--sftp-host="${sftpHost}"`,
`--sftp-port="${sftpPort ?? 22}"`,
`--sftp-user="${sftpUser}"`,
`--sftp-pass="${sftpPassword}"`,
"--sftp-pass-is-base64=false",
];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--sftp-pass-is-base64=false is not a valid rclone SFTP flag. More critically, rclone's --sftp-pass expects a password that has been pre-obscured using rclone obscure <password>, not plaintext. Passing a raw plaintext password will result in rclone failing to authenticate.

The password must be obscured before being embedded in the flag—either store it already-obscured in the database, or obscure it at runtime via a subprocess call to rclone obscure.

Comment on lines +80 to +88
export const getSftpCredentials = (destination: Destination) => {
const { sftpHost, sftpPort, sftpUser, sftpPassword } = destination;
return [
`--sftp-host="${sftpHost}"`,
`--sftp-port="${sftpPort ?? 22}"`,
`--sftp-user="${sftpUser}"`,
`--sftp-pass="${sftpPassword}"`,
"--sftp-pass-is-base64=false",
];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sftpHost, sftpUser, and sftpPassword are all nullable columns (no .notNull() constraint), so when null they are interpolated as the literal strings "null" in template literals. This produces invalid rclone flags like --sftp-host="null" and --sftp-user="null", causing silent rclone failures.

Add explicit null-checks before constructing flags:

if (!sftpHost || !sftpUser || !sftpPassword) {
  throw new Error("SFTP destination is missing required credentials");
}

Alternatively, add .notNull() constraints to these columns in the schema if they are required for SFTP destinations.

…ling

- Replace direct getS3Credentials + hardcoded :s3: paths with
  getRcloneCredentials/getRcloneDestinationPath dispatchers in all 7
  backup files (postgres, mysql, mariadb, mongo, compose, web-server,
  index/keepLatestNBackups)
- Fix getSftpCredentials: use rclone obscure for password (rclone
  requires obscured passwords for --sftp-pass flag), remove invalid
  --sftp-pass-is-base64 flag
- Add null checks: throw if sftpHost/sftpUser/sftpPassword are missing
- Remove unused path import from index.ts

Addresses all 3 issues from Greptile review.
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Mar 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ability to backup to more destination types

1 participant