fix: prevent SSH private key from leaking into build-failure notifications#63
Open
AminDhouib wants to merge 4 commits into
Open
fix: prevent SSH private key from leaking into build-failure notifications#63AminDhouib wants to merge 4 commits into
AminDhouib wants to merge 4 commits into
Conversation
…d of leaking SSH key Deployments run as a single shell command whose output is redirected into the log file ((...) >> logPath 2>&1). When that command fails, Node's exec error.message is the entire command string, which for git/ssh sources contains the echo "<private key>" used to write the deploy key. That raw message was passed straight to build-error notifications, so Discord (and other channels) showed the SSH private key instead of the actual build error. Add getDeploymentErrorMessage to read the tail of the deployment log (the real build output, which never contains the key) and use it as the notification error message for both application and compose deployments. (cherry picked from commit e10ac33)
Adds unit tests verifying the deployment error message is read from the deployment log (the real build error) rather than the raw command string, so the SSH private key can no longer leak into notifications. Covers local and remote reads, line limiting, and fallback behaviour. (cherry picked from commit be56959)
Discord, Lark and Teams capped the error message with errorMessage.substring(0, 800), which keeps the FIRST 800 characters. Since the deployment log tail is sent as the error and the actual build error is at the bottom, this showed the top of the log and cut off the real error. Extract a truncateErrorMessage helper that keeps the LAST `limit` characters (prefixed with "...") and use it in all three channels. (cherry picked from commit afb128b)
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
2 participants
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.
Ports Dokploy#4630 by @jsheely (3 commits, cherry-picked with
-x, original authorship preserved).Vulnerability class
Build-failure notifications embedded the raw exec
error.message, which contains the entire failed command line. For deployments cloned over SSH with a custom key, that command string can include the SSH private key material, so the key was forwarded to every configured notification channel.Fix
getDeploymentErrorMessage()(new, inpackages/server/src/services/deployment.ts): notifications now report the tail (last 50 lines) of the deployment log — which never contains the key — with a safe generic fallback, both for local and remote (serverId) deployments.deployApplication/deployComposepass that log-tail message instead oferror?.message.truncateErrorMessage()(new, inbuild-error.ts): channels with length limits (Discord, Lark, Teams) now truncate from the END of the message, so the real error at the bottom of the log stays visible.Fork notifier audit (Gotify / Ntfy / custom webhook)
Audited the fork's
build-error.ts: it is structurally identical to upstream's (Gotify, Ntfy, and the custom webhook notifier exist upstream too — no fork-only drift beyondrendervsrenderAsync). Since the fix sanitizes the message at the callers (application.ts/compose.ts, the onlysendBuildErrorNotificationscall sites in the fork), all channels — including Gotify, Ntfy, Pushover, and the custom webhook — receive the sanitized log-tail message. No fork-extension changes were required.Adaptations
None — all 3 commits applied cleanly onto fork canary.
Test evidence
vitest run __test__/deploy/deployment-error-message.test.ts __test__/deploy/truncate-error-message.test.ts: 13/13 passed (Windows local).pnpm --filter=dokploy run typecheck: clean.Overlap notes
packages/server/src/services/application.ts. Hunk analysis: this PR edits the catch block (~L250) and the./deploymentimport; fix(deploy): notify on container crash loop after deployment #72 edits the try block (~L222/L323) and thedocker/utilsimport — change-disjoint, either merge order is clean; re-run CI on whichever merges second. Once both land, crash-loop failures detected by fix(deploy): notify on container crash loop after deployment #72 get notified through this PR's log-tail sanitizer, which is the desired combined behavior.