Skip to content

Fix destination directory/filename split in file copier - #143

Merged
fdevans merged 2 commits into
rundeck-plugins:mainfrom
skywalkw3r:fix-copier-destination
Oct 20, 2026
Merged

fdevans merged 2 commits into
rundeck-plugins:mainfrom
skywalkw3r:fix-copier-destination

Conversation

@skywalkw3r

Copy link
Copy Markdown
Contributor

Change

  • Add common.split_destination(destination, source), which splits the destination with ntpath.split (accepts both \ and /), strips only the last path component, and falls back to the source file's basename when the destination ends in a separator.
  • Use it in winrm-filecopier.py in place of the basename + str.replace logic (and the unreachable else branch).
  • Add tests/test_common.py with unit tests for the helper.

Why

str.replace(filename, '') removed every occurrence of the filename from the path, so destinations like C:\Users\admin\Desktop\a or C:\deploy\app\app were uploaded into a wrong, newly created folder. A destination ending in \ gave an empty filename and the upload failed instead of using the source name.

The returned directory keeps its trailing separator, so ordinary paths such as C:\Windows\TEMP\123-dispatch.ps1 produce exactly the same remote_path and filename as before, and the format stays compatible with winrm_upload(). check_is_file / get_file in common.py are no longer called; I left them in place rather than remove public helpers in a bug fix.

Verification

  • python3 -m unittest discover -s tests: all pass, including 8 new tests covering a normal dispatch path, a filename that also appears in a parent directory, a single-character filename, trailing \ and /, forward and mixed separators, and a file in the drive root.
  • Not tested against a live Windows node.

End-to-end on Rundeck: I also ran this on a local Rundeck 6.4.0-SNAPSHOT server with the plugin built from main and from this branch, against a local stub WinRM endpoint (plain HTTP, basic auth, implements the WS-Man shell calls and emulates the handful of commands the plugin sends). It is not a real Windows host.

  • Copy File step to C:\Users\admin\Desktop\a: main writes the file to C:\Users\dmin\Desktop\a; this branch writes it to C:\Users\admin\Desktop\a.
  • Inline script steps, which go through the copier to C:\WINDOWS\TEMP\...-dispatch-script.tmp.bat, still work.

Fixes #142

winrm-filecopier.py split the destination by taking ntpath.basename()
and then calling str.replace(filename, '') on the whole path. replace()
removes every occurrence of the filename, not just the last component,
so the remote directory was corrupted whenever the filename also
appeared earlier in the path:

  C:\Users\admin\Desktop\a  -> directory C:\Users\dmin\Desktop\
  C:\deploy\app\app         -> directory C:\deploy\\

The file was then written into a wrong, newly created folder. A
destination ending in a separator (a directory) produced an empty
filename, so the upload targeted the directory path itself and failed
instead of using the source file's name. The fallback branch using
check_is_file/get_file could never run, because basename() never
returns None and its result is always a substring of the path.

Move the split into common.split_destination(), which uses ntpath.split
(accepts both \ and /), strips only the last component, and falls back
to the source basename when the destination ends in a separator. The
directory keeps its trailing separator, so ordinary paths such as
C:\Windows\TEMP\123-dispatch.ps1 produce exactly the same remote_path
and filename as before.

Add unit tests for the helper.
@skywalkw3r
skywalkw3r requested review from a team and a balanced review from Copilot October 18, 2026 20:46

Copilot AI left a comment

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.

🟢 Approval recommended

The focused fix preserves existing upload formatting, covers the reported regressions, and has no identified blocking issues.

No open issues 🎉

What changed in this PR

Fixes #142 by correcting destination splitting in the WinRM file copier without changing normal script-dispatch path formatting.

Changes:

  • Preserves parent directories when the filename also appears earlier in the path.
  • Uses the source filename for destinations ending in a separator.
  • Adds eight regression tests.
File Description
tests/​test_common.py Tests destination splitting and separator handling.
contents/​winrm-filecopier.py Replaces filename replacement logic with the shared helper.
contents/​common.py Adds Windows-aware destination splitting.

Review effort: Balanced

@fdevans
fdevans merged commit fa761d9 into rundeck-plugins:main Oct 20, 2026
1 of 2 checks passed
@skywalkw3r
skywalkw3r deleted the fix-copier-destination branch October 20, 2026 18:23
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.

File copier writes to the wrong folder when the filename also appears earlier in the destination path

3 participants