Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces three valuable configuration overrides (BaseEncoding, ForceFileSystem, and ServerCertificateValidationCallback) to enhance compatibility with various FTP servers. The implementation is solid and correctly integrates these new options. My review includes one suggestion to improve the maintainability of the file system forcing logic by using a switch statement, which will make it more robust for future extensions.
src/CoreFtp/FtpClient.cs
Outdated
| if ( Configuration.ForceFileSystem.Value == FtpFileSystemType.Windows ) | ||
| forcedProvider.AddParser( new Components.DirectoryListing.Parser.DosDirectoryParser( Logger ) ); | ||
| else | ||
| forcedProvider.AddParser( new Components.DirectoryListing.Parser.UnixDirectoryParser( Logger ) ); |
There was a problem hiding this comment.
The current if/else structure implicitly treats any non-Windows file system as Unix. This could lead to unexpected behavior if new FtpFileSystemType values are added in the future. A switch statement would be more robust and explicit, ensuring that only supported file system types are handled and providing a clear place to add new ones. It also allows for a default case to throw an exception for unsupported types, preventing silent failures.
switch (Configuration.ForceFileSystem.Value)
{
case FtpFileSystemType.Windows:
forcedProvider.AddParser(new Components.DirectoryListing.Parser.DosDirectoryParser(Logger));
break;
case FtpFileSystemType.Unix:
forcedProvider.AddParser(new Components.DirectoryListing.Parser.UnixDirectoryParser(Logger));
break;
default:
throw new NotSupportedException($"Unsupported file system type: {Configuration.ForceFileSystem.Value}");
}![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14ab6adadb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (Configuration.ServerCertificateValidationCallback != null) | ||
| return Configuration.ServerCertificateValidationCallback(certificate, chain, errors); |
There was a problem hiding this comment.
OnValidateCertificate checks IgnoreCertificateErrors first, so with the default config (IgnoreCertificateErrors = true in FtpClientConfiguration) a provided ServerCertificateValidationCallback is never called and any server certificate is accepted. This is a security footgun for callers who set a callback for pinning or selective trust, because their validation logic is silently bypassed unless they also know to toggle a separate flag.
Useful? React with 👍 / 👎.
src/CoreFtp/FtpClient.cs
Outdated
| forcedProvider.ClearParsers(); | ||
|
|
||
| if ( Configuration.ForceFileSystem.Value == FtpFileSystemType.Windows ) | ||
| forcedProvider.AddParser( new Components.DirectoryListing.Parser.DosDirectoryParser( Logger ) ); | ||
| else |
There was a problem hiding this comment.
This forced-filesystem branch only reduces ListDirectoryProvider to one parser, but parsing still depends on first-line Test(...) probing in ParseLines; if the first LIST line is a header like total 12, the forced parser is rejected and no entries are returned. In that common server output shape, ForceFileSystem does not actually enforce the requested parser and the override can fail.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR adds three configuration override options to FtpClientConfiguration for handling non-standard, legacy, or otherwise problematic FTP servers, resolving issues #33, #41, and #46.
Changes:
BaseEncoding: Allows specifying a custom character encoding (e.g., GBK, Shift_JIS) for the control stream, instead of the default ASCII, to support legacy servers.ServerCertificateValidationCallback: Provides a delegate for custom FTPS certificate validation logic (e.g., accepting self-signed certificates by thumbprint).ForceFileSystem: Adds a newFtpFileSystemTypeenum and lets users specifyWindowsorUnixas the directory listing format when auto-detection fails.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/CoreFtp/FtpClientConfiguration.cs |
Adds three new configuration properties: BaseEncoding, ServerCertificateValidationCallback, and ForceFileSystem |
src/CoreFtp/Enum/FtpFileSystemType.cs |
New enum with Windows and Unix values for ForceFileSystem |
src/CoreFtp/FtpClient.cs |
Sets ControlStream.Encoding from BaseEncoding at construction; adds ForceFileSystem branch in DetermineDirectoryProvider() |
src/CoreFtp/Infrastructure/Stream/FtpControlStream.cs |
Delegates certificate validation to ServerCertificateValidationCallback when set |
src/CoreFtp/Components/DirectoryListing/ListDirectoryProvider.cs |
Adds internal ClearParsers() and AddParser() methods to support forced parser configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ( Configuration.ForceFileSystem.HasValue ) | ||
| { | ||
| var forcedProvider = new ListDirectoryProvider( this, Logger, Configuration ); | ||
| forcedProvider.ClearParsers(); | ||
|
|
||
| if ( Configuration.ForceFileSystem.Value == FtpFileSystemType.Windows ) | ||
| forcedProvider.AddParser( new Components.DirectoryListing.Parser.DosDirectoryParser( Logger ) ); | ||
| else | ||
| forcedProvider.AddParser( new Components.DirectoryListing.Parser.UnixDirectoryParser( Logger ) ); | ||
|
|
||
| return forcedProvider; | ||
| } |
There was a problem hiding this comment.
When ForceFileSystem is set and the server also supports MLSD, the code bypasses MlsdDirectoryProvider entirely and falls back to ListDirectoryProvider. Since ForceFileSystem is checked first (before the UsesMlsd() check), setting it on a server that does advertise MLSD causes a regression: the client uses the less-capable LIST-based provider instead of the more reliable MLSD-based one.
The intent from issue #41 is to handle the case where MLSD is not available and auto-detection of the LIST format fails. The fix should only apply ForceFileSystem when MLSD is not available — either by changing the condition to !this.UsesMlsd() && Configuration.ForceFileSystem.HasValue, or by restructuring the check so ForceFileSystem only influences the parser selection within the LIST-based path, not the MLSD selection.
| public Func<X509Certificate, X509Chain, SslPolicyErrors, bool> ServerCertificateValidationCallback { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Allows overriding the OS directory provider. Useful if the server does not report its OS in FEAT (e.g. FtpFileSystemType.Windows). |
There was a problem hiding this comment.
The docstring says "Useful if the server does not report its OS in FEAT" but this is inaccurate. FEAT does not report the server OS; it reports supported FTP extension commands. The actual condition requiring ForceFileSystem is when the server does not advertise MLSD in the FEAT response (causing the client to fall back to LIST-based parsing), and the auto-detection of the listing format then fails. The docstring should be updated to accurately describe this condition.
| /// Allows overriding the OS directory provider. Useful if the server does not report its OS in FEAT (e.g. FtpFileSystemType.Windows). | |
| /// Allows overriding the detected server file system / directory listing format. | |
| /// Useful when the server does not advertise MLSD in its FEAT response (causing a fallback to LIST), | |
| /// and the automatic detection of the LIST output format fails (e.g. force FtpFileSystemType.Windows). |
| public System.Text.Encoding BaseEncoding { get; set; } = System.Text.Encoding.ASCII; | ||
|
|
||
| /// <summary> | ||
| /// Allows overriding the server certificate validation logic (e.g., verifying a specific self-signed certificate thumbprint). |
There was a problem hiding this comment.
The ServerCertificateValidationCallback is silently bypassed when IgnoreCertificateErrors is true (which is the default). The OnValidateCertificate method returns true immediately in that case, so the callback is never invoked.
This means a user who sets ServerCertificateValidationCallback without also setting IgnoreCertificateErrors = false will have their callback silently ignored — their custom validation logic will never run. This is particularly problematic given that the default value of IgnoreCertificateErrors is true, making the callback effectively dead code unless the user knows to change the flag.
The docstring on ServerCertificateValidationCallback should explicitly note that it requires IgnoreCertificateErrors to be set to false to take effect.
| /// Allows overriding the server certificate validation logic (e.g., verifying a specific self-signed certificate thumbprint). | |
| /// Allows overriding the server certificate validation logic (e.g., verifying a specific self-signed certificate thumbprint). | |
| /// Note: This callback is only invoked when <see cref="IgnoreCertificateErrors"/> is set to <c>false</c>. | |
| /// When <see cref="IgnoreCertificateErrors"/> is <c>true</c> (the default), certificate errors are ignored and this callback is not used. |
…ck bug, fix parser probe on single parsers, and clarify cert callback docstring
|
Thanks for the thorough review! I've addressed the feedback in
|
This PR adds three configuration overrides to handle non-compliant, legacy, or obfuscated FTP servers, resolving several open issues.
Resolves
FtpClientConfiguration.BaseEncodingso users can specifyEncoding.GetEncoding("GBK")orShift_JISinstead of standardASCIIfor control connections to legacy servers.FtpClientConfiguration.ServerCertificateValidationCallbackso users can bypass strict policy errors or accept self-signed certificates for FTPS connections.FtpClientConfiguration.ForceFileSystemso users can forceFtpFileSystemType.WindowsorUnixif the server disabled theFEATcommand andMSILcheck fails.