Skip to content

fix: File Mount with nested File Path created as directory instead of file - #5153

Open
vaelu wants to merge 2 commits into
Dokploy:canaryfrom
vaelu:fix/file-mount-nested-path-directory-bug
Open

fix: File Mount with nested File Path created as directory instead of file#5153
vaelu wants to merge 2 commits into
Dokploy:canaryfrom
vaelu:fix/file-mount-nested-path-directory-bug

Conversation

@vaelu

@vaelu vaelu commented Aug 21, 2026

Copy link
Copy Markdown

Summary

Fixes #5152 — a File Mount whose File Path contains a / (e.g. nginx/nginx.conf.template) gets created as an empty directory instead of a file, both on Create and on Edit → Save, with the error silently swallowed.

Root cause

  • updateFileMount (packages/server/src/services/mount.ts) never ran mkdir -p on the parent directory, and never checked whether fullPath already existed as a directory before running echo ... > fullPath. If it did (typically because Docker's own bind-mount fallback created an empty directory there first, when the compose service's volumes: declaration referenced the path before any File Mount content existed for it), the shell redirection failed with Is a directory — and the bare catch { console.log(...) } swallowed it, so the UI reported success regardless.
  • createFileMountgetCreateFileCommand (packages/server/src/utils/docker/utils.ts) already did mkdir -p the parent directory, but had the same blind spot for fullPath itself.

Fix

  • getCreateFileCommand and createFile now clear a pre-existing directory at fullPath before writing (rm -rf/fs.rmSync guarded by an existence+type check), so both the remote (shell) and local (fs) code paths self-heal instead of failing silently.
  • updateFileMount now reuses getCreateFileCommand instead of duplicating a broken inline command — both create and update share the same fixed logic, and the duplicate encodeBase64 construction is gone.
  • updateFileMount's catch block now logs the actual error instead of swallowing it, to make any future failure here visible in logs instead of silent.

Test plan

Added apps/dokploy/__test__/deploy/file-mount-nested-directory.test.ts, following the existing pattern in apps/dokploy/__test__/deploy/env-file-literals-dockerfile.test.ts (build the shell command, execute it with execFileSync, assert on the resulting file).

  • Verified the new tests fail against the pre-fix code with the exact errors hit in production (Is a directory from the shell command, EISDIR from createFile) — confirmed by temporarily stashing the fix and re-running.
  • Verified the new tests pass against the fixed code.
  • pnpm --filter=dokploy exec vitest run --config __test__/vitest.config.ts __test__/deploy — same pre-existing failures in application.real.test.ts (network/Docker-build dependent "REAL Execution Tests") occur identically with or without this change; nothing in the affected file's test suite regressed.
  • pnpm exec biome check on the three changed files — clean (pre-existing, unrelated warnings elsewhere in utils.ts left untouched, out of scope for this fix).
  • tsc --noEmit on packages/server — clean.

Ran on a local clone (not a full server + UI click-through, since this fix is at the file-system/shell-command layer and is covered by the regression test above) — happy to also verify end-to-end against a running Dokploy instance if useful.

Greptile Summary

The PR fixes nested file mounts that were mistakenly created as directories and incorporates safeguards requested in the previous review.

  • Reuses the shared file-creation command when updating mounts.
  • Rejects lexical paths outside the application’s files directory.
  • Removes only empty directories occupying a file target, preserving populated directories.
  • Adds regression tests for nested mounts, traversal attempts, and populated-directory preservation.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Reviews (2): Last reviewed commit: "fix: address review — path containment +..." | Re-trigger Greptile

… file

updateFileMount never ensured the parent directory existed and never
checked whether fullPath already existed as a directory before writing,
so `echo ... > fullPath` failed with "Is a directory" whenever something
(most commonly Docker's own bind-mount fallback, when a compose service's
volumes: declaration references the path before any File Mount exists for
it) had already created a directory there. The error was silently
swallowed (bare catch, console.log only), so the UI reported success
regardless.

createFileMount's getCreateFileCommand had the same blind spot: it
mkdir -p's the parent directory but never checked fullPath itself.

Fixes Dokploy#5152

- getCreateFileCommand and createFile now clear a pre-existing directory
  at fullPath before writing, so both the remote (shell) and local (fs)
  code paths self-heal instead of failing silently.
- updateFileMount now reuses getCreateFileCommand instead of duplicating
  a broken inline command, so both create and update share the same
  fixed logic.
- Adds a regression test that reproduces the bug (fails on the
  pre-fix code with the exact "Is a directory" / EISDIR errors we hit
  in production) and verifies the fix.
@vaelu
vaelu requested a review from Siumauricio as a code owner August 21, 2026 12:31
@dosubot dosubot Bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Aug 21, 2026
const encodedContent = encodeBase64(content);
return `
mkdir -p ${quote([directory])};
if [ -d ${quote([fullPath])} ]; then rm -rf ${quote([fullPath])}; fi;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 security Uncontained recursive path deletion

When a file mount uses enough ../ segments to resolve to an existing directory outside its files root, the unrestricted filePath reaches the new rm -rf or fs.rmSync branch, recursively deleting unrelated host data before writing the file. How this was verified: The mount schema accepts an unconstrained filePath, which is joined to the base path and deleted without a canonical containment check.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good catch — fixed in 7121209 by adding assertPathIsContained, which resolves the path and throws if it escapes the base files directory. Added a regression test for it too.

const encodedContent = encodeBase64(content);
return `
mkdir -p ${quote([directory])};
if [ -d ${quote([fullPath])} ]; then rm -rf ${quote([fullPath])}; fi;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Populated directories are destroyed

When the configured file target is a populated directory rather than Docker's stale empty fallback directory, this branch recursively removes all of its contents and replaces it with one file. The guard checks only whether the target is a directory, so ordinary mount creation or save can destroy data stored beneath that path.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Also fixed in 7121209 — the cleanup now only removes the target if it's an empty directory (rmdir/fs.rmdirSync, which fail on non-empty), matching the actual bug scenario (Docker's bind-mount fallback always creates an empty one) without being able to touch a directory that holds real data. A populated directory now survives the write failing loudly instead. Added a regression test for this too.

Greptile flagged two real issues in the initial fix:
- filePath is unconstrained user input; the rm -rf/fs.rmSync additions
  had no check that the resolved path stayed inside the app's files
  directory, so a crafted "../../..." filePath could delete arbitrary
  host paths.
- The stale-directory removal didn't check the directory was actually
  empty, so a File Mount pointed at a real, populated directory would
  have its contents wiped rather than the write failing loudly.

Adds assertPathIsContained (resolves and checks the target stays under
the base files directory, throws otherwise) and switches the cleanup
from a recursive force-delete to rmdir/fs.rmdirSync, which only ever
removes an empty directory — matching the actual bug scenario (Docker's
bind-mount fallback always creates an empty one) without being able to
touch anything with real content in it.

Extends the regression test with both properties: a crafted escaping
filePath is rejected, and a populated directory at the target path
survives a failed write untouched.
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Aug 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: File Mount with nested File Path (containing "/") is created as an empty directory instead of a file

1 participant