Skip to content

fix(backup): redact unquoted and malformed rclone S3 credentials - #4951

Draft
MickaelM03 wants to merge 2 commits into
Dokploy:canaryfrom
MickaelM03:fix/rclone-credential-redaction
Draft

fix(backup): redact unquoted and malformed rclone S3 credentials#4951
MickaelM03 wants to merge 2 commits into
Dokploy:canaryfrom
MickaelM03:fix/rclone-credential-redaction

Conversation

@MickaelM03

Copy link
Copy Markdown

Summary

  • The existing redactRcloneCredentials() covers only the double-quoted form --flag="value".
  • getS3Credentials() passes values through shell-quote, which emits "safe" credential strings (e.g. hex keys without spaces) unquoted.
  • As a result, credentials remain visible in structured logs, notifications, API error responses and UI restore streams. This was reproduced with 10 failing test cases on v0.29.13.
  • All reproduction values used in tests and in this PR are entirely fictitious (FAKE_R2_* constants).

Root cause

  • The existing expression only matches the double-quoted --flag="value" form.
  • Unquoted (--flag=value), single-quoted (--flag='value'), space-separated (--flag value) and malformed (unterminated quote) forms are not covered.
  • Raw ExecError objects — whose message embeds the full command line and which carry the cleartext command property — can also reach Pino, console and scheduled task handlers.

Additional exposure paths

  • Raw errors logged with console.log(error) in four database backup modules (the stack includes the full command).
  • The scheduled-jobs worker logs the rethrown ExecError via Pino, which serializes err.message, err.stack and the enumerable command/stdout/stderr properties (verified against pino@9.4.0).
  • The node-schedule backup callback receives raw rethrows (unhandled rejection prints the full message).
  • API routers (testConnection, listBackupFiles, restore subscriptions), database backup failure notifications, restore emit() streams and volume-backup retention paths.

Fix

  • Robust, idempotent credential redaction covering quoted, unquoted, space-separated, shell-escaped and malformed (truncated/unterminated) forms, with a last-resort pass that skips already-redacted placeholders.
  • Redaction applied before every log, notification, API and UI exposure point.
  • Sanitized rethrow from the backup runners, so upstream loggers (Pino worker, node-schedule callback, tRPC dev handler) never see the raw command.

Executed commands, retention, scheduling and notification triggers are unchanged — only logged/emitted/thrown strings are sanitized.

Tests

  • Reproduction tests using fictitious values (10 red cases on the unpatched code, all green after the fix).
  • Malformed input tests (unterminated quotes, trailing backslash).
  • Idempotence, determinism, empty/malformed input safety.
  • Performance/large input (1 MiB string unchanged, 100k malformed chars, 1000 sensitive args — no algorithmic blow-up).
  • Pino serialization proof demonstrating the ExecError.command leak and its resolution via the sanitized rethrow.
  • TypeScript --strict on the rewritten module and Biome checks pass.

Known limitation

The patch does not remove S3 credentials from the rclone process argv (visible in ps during execution). A follow-up could pass credentials through the officially documented RCLONE_S3_ACCESS_KEY_ID / RCLONE_S3_SECRET_ACCESS_KEY environment variables instead of command-line flags.

Related issue

Refs #4621 — the initial fix covers only the double-quoted form; this PR completes it and demonstrates the gap with reproducible tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant