fix(backup): redact unquoted and malformed rclone S3 credentials - #4951
Draft
MickaelM03 wants to merge 2 commits into
Draft
fix(backup): redact unquoted and malformed rclone S3 credentials#4951MickaelM03 wants to merge 2 commits into
MickaelM03 wants to merge 2 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
redactRcloneCredentials()covers only the double-quoted form--flag="value".getS3Credentials()passes values throughshell-quote, which emits "safe" credential strings (e.g. hex keys without spaces) unquoted.FAKE_R2_*constants).Root cause
--flag="value"form.--flag=value), single-quoted (--flag='value'), space-separated (--flag value) and malformed (unterminated quote) forms are not covered.ExecErrorobjects — whosemessageembeds the full command line and which carry the cleartextcommandproperty — can also reach Pino,consoleand scheduled task handlers.Additional exposure paths
console.log(error)in four database backup modules (the stack includes the full command).ExecErrorvia Pino, which serializeserr.message,err.stackand the enumerablecommand/stdout/stderrproperties (verified againstpino@9.4.0).node-schedulebackup callback receives raw rethrows (unhandled rejection prints the full message).testConnection,listBackupFiles, restore subscriptions), database backup failure notifications, restoreemit()streams and volume-backup retention paths.Fix
Executed commands, retention, scheduling and notification triggers are unchanged — only logged/emitted/thrown strings are sanitized.
Tests
ExecError.commandleak and its resolution via the sanitized rethrow.--stricton the rewritten module and Biome checks pass.Known limitation
The patch does not remove S3 credentials from the
rcloneprocess argv (visible inpsduring execution). A follow-up could pass credentials through the officially documentedRCLONE_S3_ACCESS_KEY_ID/RCLONE_S3_SECRET_ACCESS_KEYenvironment 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.