Fix for filename with invalid chars on the SFTP server#245
Conversation
…s not allowed chars on destination windows server and you are not allowed to do a rename on the SFTP server
WalkthroughAdds filename sanitization to SFTP download operations by introducing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Frends.SFTP.DownloadFiles/Frends.SFTP.DownloadFiles/Definitions/SingleFileTransfer.cs (1)
141-167: Critical bug: Path corruption whenReplaceNotAllowedCharsInDownloadFileNameis false.When the option is
false,targetFilenameis assignedSourceFileDuringTransfer(a full path, not just a filename). Lines 166-167 then run unconditionally and corrupt both paths:// If SourceFileDuringTransfer = "/upload/test.txt" and SourceFile.Name = "test.txt" // targetFilename = "/upload/test.txt" (full path when option is false) // Line 166: "/upload/test.txt".Replace("test.txt", "/upload/test.txt") // = "/upload//upload/test.txt" ← CORRUPTEDThis breaks all file transfers when the new option is disabled.
🐛 Proposed fix - make path updates conditional
if (BatchContext.Options.ReplaceNotAllowedCharsInDownloadFileName) targetFilename = CleanWindowsFileName(SourceFile.Name, BatchContext.Options.ReplaceNotAllowedCharWithChar); else - targetFilename = SourceFileDuringTransfer; + targetFilename = Path.GetFileName(SourceFileDuringTransfer); SetCurrentState(TransferState.GetFile, $"Downloading temporary source file {Path.GetFileName(targetFilename)} to local temp folder {WorkFileInfo.WorkFileDir}"); using var fs = File.Open( Path.Combine(WorkFileInfo.WorkFileDir, Path.GetFileName(targetFilename)), FileMode.Create); var asynch = Client.BeginDownloadFile(SourceFileDuringTransfer, fs); var sftpAsynch = asynch as SftpDownloadAsyncResult; while (sftpAsynch != null && !sftpAsynch.IsCompleted) { if (cancellationToken.IsCancellationRequested) { sftpAsynch.IsDownloadCanceled = true; _logger.NotifyError(BatchContext, "Operation was cancelled from UI.", new OperationCanceledException()); cancellationToken.ThrowIfCancellationRequested(); } } - SourceFileDuringTransfer = SourceFileDuringTransfer.Replace(SourceFile.Name, targetFilename); - DestinationFileWithMacrosExpanded = Path.Combine(Path.GetDirectoryName(DestinationFileWithMacrosExpanded), targetFilename); + if (BatchContext.Options.ReplaceNotAllowedCharsInDownloadFileName) + { + SourceFileDuringTransfer = SourceFileDuringTransfer.Replace(SourceFile.Name, targetFilename); + DestinationFileWithMacrosExpanded = Path.Combine(Path.GetDirectoryName(DestinationFileWithMacrosExpanded), targetFilename); + } Client.EndDownloadFile(asynch);Frends.SFTP.DownloadFiles/Frends.SFTP.DownloadFiles.Tests/TransferTests.cs (2)
222-249: Test doesn't validate the sanitization functionality.This test uses
_source.FileNamewhich presumably doesn't contain invalid Windows characters. The assertion on line 248 checks for the original filename, but if sanitization actually occurred, the filename would be different.To properly test this feature:
- Create a file with invalid Windows characters (e.g.,
file:name?.txt) on the SFTP server- Assert that the destination file has the sanitized name (e.g.,
file_name_.txt)- Assert that the original invalid-named file does NOT exist locally
📝 Suggested test improvement
[Test] public async Task DownloadFiles_TestTransferWithNotAllowedCharsInFileName() { // Create a file with Windows-invalid characters on SFTP var invalidFileName = "file:name?.txt"; var expectedSanitizedName = "file_name_.txt"; Helpers.UploadTestFiles(_source.Directory, 1, null, new List<string> { invalidFileName }); var source = new Source { Directory = _source.Directory, FileName = invalidFileName, Action = SourceAction.Error, Operation = SourceOperation.Delete, }; var destination = new Destination { Directory = Path.Combine(_workDir, "destination"), Action = DestinationAction.Overwrite, }; var options = new Options { ThrowErrorOnFail = true, RenameSourceFileBeforeTransfer = false, RenameDestinationFileDuringTransfer = false, ReplaceNotAllowedCharsInDownloadFileName = true, CreateDestinationDirectories = true, PreserveLastModified = false, OperationLog = true }; var result = await SFTP.DownloadFiles(source, destination, _connection, options, _info, new CancellationToken()); Assert.IsTrue(result.Success); Assert.AreEqual(1, result.SuccessfulTransferCount); // Verify sanitized file exists, not the original invalid name Assert.IsTrue(File.Exists(Path.Combine(destination.Directory, expectedSanitizedName))); Assert.IsFalse(File.Exists(Path.Combine(destination.Directory, invalidFileName))); }
196-220: Test method name no longer matches its behavior.The test
DownloadFiles_TestTransferWithRenameSourceEnabledRenameDestinationDisablednow setsRenameDestinationFileDuringTransfer = true(line 209), contradicting the method name which says "RenameDestinationDisabled".Per Microsoft naming conventions, the test name should accurately describe what it tests.
📝 Proposed fix
Either rename the test method to match the new behavior:
-public async Task DownloadFiles_TestTransferWithRenameSourceEnabledRenameDestinationDisabled() +public async Task DownloadFiles_TestTransferWithRenameSourceAndDestinationEnabled()Or revert the option to match the test name:
-RenameDestinationFileDuringTransfer = true, +RenameDestinationFileDuringTransfer = false,
🤖 Fix all issues with AI agents
In `@Frends.SFTP.DownloadFiles/Frends.SFTP.DownloadFiles/Definitions/Options.cs`:
- Line 71: The XML documentation example in Options.cs is incorrect: update the
<example> value from ".8CO" to match the property's DefaultValue of "_" (i.e.,
change <example>.8CO</example> to <example>_</example>) so the doc comment
reflects the actual default; locate the property that has the
[DefaultValue("_")] attribute in the Options class and adjust its XML <example>
accordingly.
- Around line 65-66: The DefaultValue attribute for the property
ReplaceNotAllowedCharsInDownloadFileName conflicts with its initializer (=
true); update one so they match—either change the attribute to
[DefaultValue(true)] if true is the intended default, or set the property
initializer to false (public bool ReplaceNotAllowedCharsInDownloadFileName {
get; set; } = false;) if false is intended—ensure the DefaultValue attribute and
the property initializer are consistent.
In
`@Frends.SFTP.DownloadFiles/Frends.SFTP.DownloadFiles/Definitions/SingleFileTransfer.cs`:
- Around line 181-204: The CleanWindowsFileName method currently checks
ReservedNames against the whole filename which misses cases like "CON.txt";
modify it to extract the extension using Path.GetExtension(fileName) and the
base name via Path.GetFileNameWithoutExtension(fileName) after replacing invalid
chars and trimming, then perform the ReservedNames.Contains(baseName,
StringComparer.OrdinalIgnoreCase) check; if reserved, append "_" to the baseName
and then reassemble the cleaned filename as baseName + extension (preserving the
original extension), keep the existing trimming/unnamed fallback logic.
🧹 Nitpick comments (3)
Frends.SFTP.DownloadFiles/Frends.SFTP.DownloadFiles/Definitions/SingleFileTransfer.cs (2)
172-179: Consider extracting constants and the method to a utility class.
ReservedNamesandCleanWindowsFileNameare general-purpose Windows filename utilities. They could be moved to a sharedUtilclass for reusability and easier unit testing, sinceSingleFileTransferis aninternalclass.Additionally, consider adding
COM0andLPT0toReservedNamesas they are also reserved on some Windows versions.
349-353: Verify the logic forExecuteSourceOperationpath selection.The conditional ensures the original SFTP path is used when sanitization is enabled (since the sanitized name only exists locally). However, the comment clarifying this behavior would help future maintainers understand why the logic appears "inverted."
📝 Consider adding a clarifying comment
private async Task ExecuteSourceOperation(CancellationToken cancellationToken) { var filePath = string.Empty; + // When filename sanitization is enabled, SourceFileDuringTransfer contains the sanitized local name, + // but SFTP operations need the original path since the file wasn't renamed on the server. if(BatchContext.Options.ReplaceNotAllowedCharsInDownloadFileName) filePath = SourceFile.FullPath; else filePath = string.IsNullOrEmpty(SourceFileDuringTransfer) ? SourceFile.FullPath : SourceFileDuringTransfer;Frends.SFTP.DownloadFiles/Frends.SFTP.DownloadFiles.Tests/TransferTests.cs (1)
231-240: Missing test forReplaceNotAllowedCharWithCharcustom replacement character.The
ReplaceNotAllowedCharWithCharoption isn't set in this test, so it defaults to_. Consider adding a test that verifies custom replacement characters work correctly.
| [DefaultValue(false)] | ||
| public bool ReplaceNotAllowedCharsInDownloadFileName { get; set; } = true; |
There was a problem hiding this comment.
Inconsistency between [DefaultValue(false)] and initializer = true.
The [DefaultValue(false)] attribute conflicts with the initializer = true. This causes inconsistent behavior: objects instantiated in code default to true, while deserialization/UI frameworks using the attribute may default to false.
Per coding guidelines, ensure consistency by aligning the attribute and initializer.
🐛 Proposed fix
/// <example>true</example>
- [DefaultValue(false)]
- public bool ReplaceNotAllowedCharsInDownloadFileName { get; set; } = true;
+ [DefaultValue(false)]
+ public bool ReplaceNotAllowedCharsInDownloadFileName { get; set; } = false;Alternatively, if true is the desired default:
/// <example>true</example>
- [DefaultValue(false)]
+ [DefaultValue(true)]
public bool ReplaceNotAllowedCharsInDownloadFileName { get; set; } = true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [DefaultValue(false)] | |
| public bool ReplaceNotAllowedCharsInDownloadFileName { get; set; } = true; | |
| [DefaultValue(false)] | |
| public bool ReplaceNotAllowedCharsInDownloadFileName { get; set; } = false; |
🤖 Prompt for AI Agents
In `@Frends.SFTP.DownloadFiles/Frends.SFTP.DownloadFiles/Definitions/Options.cs`
around lines 65 - 66, The DefaultValue attribute for the property
ReplaceNotAllowedCharsInDownloadFileName conflicts with its initializer (=
true); update one so they match—either change the attribute to
[DefaultValue(true)] if true is the intended default, or set the property
initializer to false (public bool ReplaceNotAllowedCharsInDownloadFileName {
get; set; } = false;) if false is intended—ensure the DefaultValue attribute and
the property initializer are consistent.
| /// <summary> | ||
| /// Char to replace not allowed chars with when file is written to local server. | ||
| /// </summary> | ||
| /// <example>.8CO</example> |
There was a problem hiding this comment.
Incorrect XML example value.
The example shows .8CO but the actual [DefaultValue] is "_". The example should match the default.
📝 Proposed fix
/// <summary>
/// Char to replace not allowed chars with when file is written to local server.
/// </summary>
- /// <example>.8CO</example>
+ /// <example>_</example>
[DefaultValue("_")]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// <example>.8CO</example> | |
| /// <summary> | |
| /// Char to replace not allowed chars with when file is written to local server. | |
| /// </summary> | |
| /// <example>_</example> | |
| [DefaultValue("_")] |
🤖 Prompt for AI Agents
In `@Frends.SFTP.DownloadFiles/Frends.SFTP.DownloadFiles/Definitions/Options.cs`
at line 71, The XML documentation example in Options.cs is incorrect: update the
<example> value from ".8CO" to match the property's DefaultValue of "_" (i.e.,
change <example>.8CO</example> to <example>_</example>) so the doc comment
reflects the actual default; locate the property that has the
[DefaultValue("_")] attribute in the Options class and adjust its XML <example>
accordingly.
| public static string CleanWindowsFileName(string fileName, string replacement = "_") | ||
| { | ||
| // Step 1: Replace invalid characters | ||
| foreach (char c in Path.GetInvalidFileNameChars()) | ||
| { | ||
| fileName = fileName.Replace(c.ToString(), replacement); | ||
| } | ||
|
|
||
| // Step 2: Trim trailing periods and spaces | ||
| fileName = fileName.TrimEnd(' ', '.'); | ||
|
|
||
| // Step 3: Avoid reserved names (case-insensitive) | ||
| if (ReservedNames.Contains(fileName, StringComparer.OrdinalIgnoreCase)) | ||
| { | ||
| fileName += "_"; | ||
| } | ||
|
|
||
| if (string.IsNullOrWhiteSpace(fileName)) | ||
| { | ||
| return "unnamed"; | ||
| } | ||
|
|
||
| return fileName; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Windows reserved filenames CON PRN reserved with extension .txt
💡 Result:
Short answer: No — names like CON and PRN are reserved device names in Windows and you must not use them even with an extension (e.g., CON.txt or PRN.txt). They are reserved for files and directories. [1]
If you need to create such a name from another environment (WSL, Linux tools, etc.) it can produce files that Windows tools cannot handle properly. [2]
Sources:
- Microsoft: "Naming Files, Paths, and Namespaces" (reserved names; "Also avoid these names followed immediately by an extension"). [1]
- MS/WSL issue and community reports on files created with reserved names causing Windows-side problems. [2]
Fix reserved name handling for extensions — Windows reserves names like CON, PRN, etc., even with extensions (e.g., CON.txt). The current implementation checks the entire filename string against ReservedNames, which won't catch cases like CON.txt if ReservedNames only contains base names. Extract and check the base filename (before the extension) against reserved names to handle this properly.
🤖 Prompt for AI Agents
In
`@Frends.SFTP.DownloadFiles/Frends.SFTP.DownloadFiles/Definitions/SingleFileTransfer.cs`
around lines 181 - 204, The CleanWindowsFileName method currently checks
ReservedNames against the whole filename which misses cases like "CON.txt";
modify it to extract the extension using Path.GetExtension(fileName) and the
base name via Path.GetFileNameWithoutExtension(fileName) after replacing invalid
chars and trimming, then perform the ReservedNames.Contains(baseName,
StringComparer.OrdinalIgnoreCase) check; if reserved, append "_" to the baseName
and then reassemble the cleaned filename as baseName + extension (preserving the
original extension), keep the existing trimming/unnamed fallback logic.
1 participant
For an explanation of the issue please see issue
#244
Dear PR creator, please select one of the PR templates, then remove others and this text.
Default PR template
Please review my changes :)
Task Update PR template
Review Checklist
Task Harmonization PR template
Review Checklist
1. Frends Task Project File
Frends.*/Frends.*/*.csproj<TargetFramework>net8.0</TargetFramework><Version>x.0.0</Version><Authors>Frends</Authors><PackageLicenseExpression>MIT</PackageLicenseExpression><GenerateDocumentationFile>true</GenerateDocumentationFile><Description><RepositoryUrl>https://github.com/FrendsPlatform/Frends.SYSTEM/tree/main/Frends.SYSTEM.ACTION</RepositoryUrl><Nullable>disable</Nullable>StyleCop.Analyzers v1.2.0-beta.556FrendsTaskAnalyzers v1.*<Content Include="migration.json" PackagePath="/" Pack="true"/><Content Include="../CHANGELOG.md" PackagePath="/" Pack="true"/><AdditionalFiles Include="FrendsTaskMetadata.json" PackagePath="/" Pack="true"/>2. Frends Task Test Project File
Frends.*/Frends.*.Tests/*.Tests.csproj<TargetFramework>net8.0</TargetFramework><IsPackable>false</IsPackable><Nullable>disable</Nullable>StyleCop.Analyzers v1.2.0-beta.5563. Additional Files
LICENSEfile per repository.gitignorefile per repository.idea/foldersFrends.*/README.mdFrends.*/CHANGELOG.mdFrends.*/Frends.*/FrendsTaskMetadata.jsonFrends.System.Action.System.ActionFrends.*/Frends.*/migration.jsonFrends.*/Frends.*/GlobalSuppressions.csFrends.*/Frends.*.Tests/GlobalSuppressions.cs4. Source Code
5. GitHub Actions Workflows
.github/workflows/*.yml*_release.ymlfeed_api_key: ${{ secrets.TASKS_FEED_API_KEY }}*_test_on_main.ymlbadge_service_api_key: ${{ secrets.BADGE_SERVICE_API_KEY }}*_test_on_push.ymlbadge_service_api_key: ${{ secrets.BADGE_SERVICE_API_KEY }}test_feed_api_key: ${{ secrets.TASKS_TEST_FEED_API_KEY }}GITHUB_TOKENworkdir: Frends.SYSTEM.ACTIONstrict_analyzers: truedotnet_version: 8.0.xprebuild_command: docker-compose up -d)Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.