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 focuses on modernizing the CoreFTP library by upgrading its target .NET framework and all its dependencies to the latest versions. It also addresses several critical bug fixes related to connection stability, error handling during logout and disposal, and robust parsing of FTP server responses. The changes aim to provide a more reliable and up-to-date FTP client, backed by new integration and unit tests to ensure stability. Highlights
Changelog
Ignored Files
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 upgrades the project to target newer .NET frameworks (including a preview of .NET 10) and updates all dependencies. It also includes several important bug fixes, such as more robust FTP response parsing, ensuring resources are properly disposed, and making logout operations more resilient to network errors, with new integration tests covering these fixes. However, a security audit identified two significant vulnerabilities in the network stream handling logic: a potential Denial of Service (DoS) due to unbounded memory allocation in ReadLine, and an FTP command injection vulnerability due to missing CRLF sanitization in WriteLineAsync. Both issues must be addressed to ensure the security and stability of the library. Additionally, my review suggests removing a committed log file and improving exception logging in the Dispose method.
| protected async Task WriteLineAsync(string buf) | ||
| { | ||
| var data = Encoding.GetBytes( $"{buf}\r\n" ); | ||
| await WriteAsync( data, 0, data.Length, CancellationToken.None ); | ||
| var data = Encoding.GetBytes($"{buf}\r\n"); | ||
| await WriteAsync(data, 0, data.Length, CancellationToken.None); | ||
| } |
There was a problem hiding this comment.
The WriteLineAsync method appends \r\n to the provided buffer and sends it to the server without checking if the buffer itself contains CRLF sequences. This allows an attacker who can control the parameters of FTP commands (e.g., directory names, filenames) to inject arbitrary FTP commands into the stream. This is a classic CRLF injection vulnerability in the context of the FTP protocol.
To remediate this, sanitize all user-supplied inputs to remove or encode CRLF characters before they are used in FTP commands, or validate that the input does not contain these characters.
protected async Task WriteLineAsync(string buf)
{
if (buf.Contains("\r") || buf.Contains("\n"))
throw new ArgumentException("Command parameters cannot contain CRLF characters", nameof(buf));
var data = Encoding.GetBytes($"{buf}\r\n");
await WriteAsync(data, 0, data.Length, CancellationToken.None);
}
restore_log.txt
Outdated
| @@ -0,0 +1,64 @@ | |||
| Determining projects to restore... | |||
There was a problem hiding this comment.
This log file appears to be a build artifact from a failed dotnet restore. Such files should not be committed to the version control system. Please remove this file from the pull request and add *.log or restore_log.txt to your .gitignore file to prevent this from happening in the future.
| protected string ReadLine(Encoding encoding, CancellationToken token) | ||
| { | ||
| if ( encoding == null ) | ||
| throw new ArgumentNullException( nameof( encoding ) ); | ||
| if (encoding == null) | ||
| throw new ArgumentNullException(nameof(encoding)); | ||
|
|
||
| var data = new List<byte>(); | ||
| var buffer = new byte[ 1 ]; | ||
| Span<byte> buffer = stackalloc byte[1]; | ||
| string line = null; | ||
|
|
||
| token.ThrowIfCancellationRequested(); | ||
|
|
||
| while ( Read( buffer, 0, buffer.Length ) > 0 ) | ||
| while (Read(buffer) > 0) | ||
| { | ||
| token.ThrowIfCancellationRequested(); | ||
| data.Add( buffer[ 0 ] ); | ||
| if ( (char) buffer[ 0 ] != '\n' ) | ||
| data.Add(buffer[0]); | ||
| if ((char)buffer[0] != '\n') | ||
| continue; | ||
| line = encoding.GetString( data.ToArray() ).Trim( '\r', '\n' ); | ||
| line = encoding.GetString(data.ToArray()).Trim('\r', '\n'); | ||
| break; | ||
| } | ||
|
|
||
| return line; | ||
| } |
There was a problem hiding this comment.
The ReadLine method reads bytes from the network stream and adds them to a List<byte> until a newline character (\n) is encountered. If a malicious or misconfigured FTP server sends an extremely long line without a newline, the client will continue to allocate memory until it runs out of memory (OOM), leading to a crash. This is a potential Denial of Service (DoS) vulnerability.
To remediate this, implement a maximum line length limit. If the limit is exceeded, throw an exception or truncate the line.
protected string ReadLine(Encoding encoding, CancellationToken token)
{
if (encoding == null)
throw new ArgumentNullException(nameof(encoding));
var data = new List<byte>();
Span<byte> buffer = stackalloc byte[1];
string line = null;
token.ThrowIfCancellationRequested();
const int maxLineLength = 4096;
while (Read(buffer) > 0)
{
token.ThrowIfCancellationRequested();
data.Add(buffer[0]);
if ((char)buffer[0] != '\n')
{
if (data.Count > maxLineLength)
throw new FtpException("Line length limit exceeded");
continue;
}
line = encoding.GetString(data.ToArray()).Trim('\r', '\n');
break;
}
return line;
}
src/CoreFtp/FtpClient.cs
Outdated
| LogOutAsync().GetAwaiter().GetResult(); | ||
| } | ||
| } | ||
| catch { } |
There was a problem hiding this comment.
Swallowing exceptions silently with an empty catch block can hide underlying issues. While Dispose methods should not throw exceptions, it's good practice to log any exceptions that occur during cleanup to aid in debugging. This is especially important here since LogOutAsync can throw exceptions (e.g., from IgnoreStaleData).
catch (Exception ex)
{
Logger?.LogWarning(0, ex, "Exception during logout on dispose.");
}There was a problem hiding this comment.
Pull request overview
This PR modernizes CoreFtp for a v2 release by upgrading the solution to newer .NET targets/tooling and adding targeted regression tests for recent bug fixes (logout/dispose robustness, PWD parsing, control-stream reads), while removing the older end-to-end integration suite.
Changes:
- Upgrade CoreFtp + test project to newer target frameworks and update package/tooling versions (including global.json and CI workflows).
- Fix FTP client robustness around logout/dispose and parsing unquoted
PWDresponses. - Rework tests: remove legacy integration tests and add focused tests using a lightweight fake FTP server and control-stream unit tests.
Reviewed changes
Copilot reviewed 37 out of 38 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/CoreFtp.Tests.Integration/Program.cs | Updates logging initialization (but still contains Main, which conflicts with test SDK entrypoint). |
| tests/CoreFtp.Tests.Integration/FtpControlStreamTests/When_reading_control_stream.cs | New unit tests for FtpControlStream.ReadLine / ReadByte behavior. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/LogOutTests.cs | New fake-server test covering LogOutAsync on dropped connection. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/DisposeTests.cs | New fake-server test covering Dispose() when logout fails. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/ChangeWorkingDirectoryTests.cs | New fake-server test covering PWD responses without quotes + inline xUnit logger helper. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/When_using_a_uri_as_hostname.cs | Removes legacy integration test. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/When_sending_a_custom_command.cs | Removes legacy integration test. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/When_renaming_a_node.cs | Removes legacy integration test. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/When_opening_datastream_with_explicit_encryption.cs | Removes legacy integration test. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/When_connecting_to_an_ftp_server.cs | Removes legacy integration test. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/When_connecting_to_a_tls_encrypted_ftp_server.cs | Removes legacy integration test. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/When_changing_transfer_type.cs | Removes legacy integration test. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/TestBase.cs | Removes old shared integration test base. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/Files/When_uploading_file_to_deep_folder.cs | Removes legacy integration test. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/Files/When_opening_file_write_stream.cs | Removes legacy integration test. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/Files/When_opening_a_file_for_upload.cs | Removes legacy integration test. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/Files/When_opening_a_file_for_download.cs | Removes legacy integration test. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/Files/When_listing_files.cs | Removes legacy integration test. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/Files/When_getting_the_size_of_a_file.cs | Removes legacy integration test. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/Files/When_deleting_a_file.cs | Removes legacy integration test. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/Directories/When_providing_a_base_directory.cs | Removes legacy integration test. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/Directories/When_listing_directories.cs | Removes legacy integration test. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/Directories/When_deleting_directories.cs | Removes legacy integration test. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/Directories/When_creating_directories.cs | Removes legacy integration test. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/Directories/When_creating_a_deep_folder_from_root.cs | Removes legacy integration test. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/Directories/When_changing_working_directories.cs | Removes legacy integration test. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/Directories/When_Encountering_Differing_Directory_Formats.cs | Removes legacy integration test. |
| tests/CoreFtp.Tests.Integration/FtpClientTests/CustomExamples/When_connecting_to_bom_gov_au.cs | Removes example test. |
| tests/CoreFtp.Tests.Integration/CoreFtp.Tests.Integration.csproj | Upgrades test project to net10.0 + updates dependencies. |
| src/CoreFtp/Infrastructure/Stream/FtpControlStream.cs | Adds buffered ReadByte + adjusts read path used by ReadLine; improves disconnect behavior. |
| src/CoreFtp/FtpClientConfiguration.cs | Defaults SslProtocols to None (defer to OS defaults). |
| src/CoreFtp/FtpClient.cs | Makes logout/dispose more resilient; adds PWD parsing fallback. |
| src/CoreFtp/CoreFtp.csproj | Updates TFMs + dependencies; bumps version to 2.0.0. |
| global.json | Pins repository to .NET SDK 10.0.103. |
| .github/workflows/build.yml | Updates CI build to install .NET 10. |
| .github/workflows/publish.yml | Adds NuGet publish workflow on version tags. |
| restore_log.txt | Adds restore output log (likely an artifact). |
| build_log.txt | Adds build output log (likely an artifact). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
restore_log.txt
Outdated
| Determining projects to restore... | ||
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : warning NU1903: Package 'Microsoft.NETCore.App' 1.0.5 has a known high severity vulnerability, https://github.com/advisories/GHSA-7mfr-774f-w5r9 | ||
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : warning NU1903: Package 'Microsoft.NETCore.App' 1.0.5 has a known high severity vulnerability, https://github.com/advisories/GHSA-8884-xcr4-r68p | ||
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: Package xunit.runner.visualstudio 3.0.0 is not compatible with netcoreapp1.0 (.NETCoreApp,Version=v1.0). Package xunit.runner.visualstudio 3.0.0 supports: | ||
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net472 (.NETFramework,Version=v4.7.2) | ||
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net6.0 (.NETCoreApp,Version=v6.0) | ||
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: Package Microsoft.Extensions.Configuration 9.0.0 is not compatible with netcoreapp1.0 (.NETCoreApp,Version=v1.0). Package Microsoft.Extensions.Configuration 9.0.0 supports: | ||
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net462 (.NETFramework,Version=v4.6.2) | ||
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net8.0 (.NETCoreApp,Version=v8.0) | ||
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net9.0 (.NETCoreApp,Version=v9.0) | ||
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - netstandard2.0 (.NETStandard,Version=v2.0) | ||
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: Package Microsoft.Extensions.Configuration.FileExtensions 9.0.0 is not compatible with netcoreapp1.0 (.NETCoreApp,Version=v1.0). Package Microsoft.Extensions.Configuration.FileExtensions 9.0.0 supports: | ||
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net462 (.NETFramework,Version=v4.6.2) | ||
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net8.0 (.NETCoreApp,Version=v8.0) | ||
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net9.0 (.NETCoreApp,Version=v9.0) | ||
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - netstandard2.0 (.NETStandard,Version=v2.0) | ||
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: Package Microsoft.Extensions.Configuration.Json 9.0.0 is not compatible with netcoreapp1.0 (.NETCoreApp,Version=v1.0). Package Microsoft.Extensions.Configuration.Json 9.0.0 supports: | ||
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net462 (.NETFramework,Version=v4.6.2) | ||
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net8.0 (.NETCoreApp,Version=v8.0) | ||
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net9.0 (.NETCoreApp,Version=v9.0) | ||
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - netstandard2.0 (.NETStandard,Version=v2.0) |
There was a problem hiding this comment.
restore_log.txt appears to be a local build artifact (absolute paths, transient restore output). It’s usually better not to commit build/restore logs to the repo; consider removing this file (or moving to a docs/troubleshooting location if it’s intentionally retained).
| Determining projects to restore... | |
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : warning NU1903: Package 'Microsoft.NETCore.App' 1.0.5 has a known high severity vulnerability, https://github.com/advisories/GHSA-7mfr-774f-w5r9 | |
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : warning NU1903: Package 'Microsoft.NETCore.App' 1.0.5 has a known high severity vulnerability, https://github.com/advisories/GHSA-8884-xcr4-r68p | |
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: Package xunit.runner.visualstudio 3.0.0 is not compatible with netcoreapp1.0 (.NETCoreApp,Version=v1.0). Package xunit.runner.visualstudio 3.0.0 supports: | |
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net472 (.NETFramework,Version=v4.7.2) | |
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net6.0 (.NETCoreApp,Version=v6.0) | |
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: Package Microsoft.Extensions.Configuration 9.0.0 is not compatible with netcoreapp1.0 (.NETCoreApp,Version=v1.0). Package Microsoft.Extensions.Configuration 9.0.0 supports: | |
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net462 (.NETFramework,Version=v4.6.2) | |
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net8.0 (.NETCoreApp,Version=v8.0) | |
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net9.0 (.NETCoreApp,Version=v9.0) | |
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - netstandard2.0 (.NETStandard,Version=v2.0) | |
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: Package Microsoft.Extensions.Configuration.FileExtensions 9.0.0 is not compatible with netcoreapp1.0 (.NETCoreApp,Version=v1.0). Package Microsoft.Extensions.Configuration.FileExtensions 9.0.0 supports: | |
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net462 (.NETFramework,Version=v4.6.2) | |
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net8.0 (.NETCoreApp,Version=v8.0) | |
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net9.0 (.NETCoreApp,Version=v9.0) | |
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - netstandard2.0 (.NETStandard,Version=v2.0) | |
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: Package Microsoft.Extensions.Configuration.Json 9.0.0 is not compatible with netcoreapp1.0 (.NETCoreApp,Version=v1.0). Package Microsoft.Extensions.Configuration.Json 9.0.0 supports: | |
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net462 (.NETFramework,Version=v4.6.2) | |
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net8.0 (.NETCoreApp,Version=v8.0) | |
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net9.0 (.NETCoreApp,Version=v9.0) | |
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - netstandard2.0 (.NETStandard,Version=v2.0) | |
| This file is intentionally *not* a live `dotnet restore` log. | |
| It previously contained transient restore output with machine-specific absolute | |
| paths (for example, `d:\...`) and package compatibility errors. That content | |
| has been removed to avoid committing local build artifacts and environment | |
| details to the repository. | |
| If you need a restore log for troubleshooting: | |
| 1. Run `dotnet restore` locally in your working copy. | |
| 2. Capture the output to a temporary file, for example: | |
| dotnet restore > restore_log_local.txt | |
| 3. Use that temporary file for debugging instead of committing it. | |
| You may repurpose or relocate this file (for example, into `docs/`) if you | |
| want to maintain more detailed, curated troubleshooting notes. | |
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - net9.0 (.NETCoreApp,Version=v9.0) | |
| d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj : error NU1202: - netstandard2.0 (.NETStandard,Version=v2.0) |
build_log.txt
Outdated
| Build started 28/02/2026 17:34:03. | ||
| 1>Project "d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj" on node 1 (Restore target(s)). | ||
| 1>_GetAllRestoreProjectPathItems: | ||
| Determining projects to restore... | ||
| 1>Project "d:\code\oss\CoreFTP\tests\CoreFtp.Tests.Integration\CoreFtp.Tests.Integration.csproj" (1) is building "d:\code\oss\CoreFTP\src\CoreFtp\CoreFtp.csproj" (2:10) on node 1 (_GenerateProjectRestoreGraph target(s)). | ||
| 2:10>Project "d:\code\oss\CoreFTP\src\CoreFtp\CoreFtp.csproj" (2:10) is building "d:\code\oss\CoreFTP\src\CoreFtp\CoreFtp.csproj" (2:14) on node 1 (_GenerateProjectRestoreGraphPerFramework target(s)). |
There was a problem hiding this comment.
build_log.txt looks like a local MSBuild output capture (machine-specific paths and timestamps). If this is only for debugging the PR locally, it shouldn’t be checked in—please remove it (or replace with a short note in the PR description / docs).
|
|
||
| // Quick xunit logger provider | ||
| public class XunitLoggerProvider : ILoggerProvider | ||
| { | ||
| private readonly ITestOutputHelper _output; | ||
|
|
||
| public XunitLoggerProvider(ITestOutputHelper output) | ||
| { | ||
| _output = output; | ||
| } | ||
|
|
||
| public ILogger CreateLogger(string categoryName) => new XunitLogger(_output); | ||
|
|
||
| public void Dispose() | ||
| { | ||
| } | ||
| } | ||
|
|
||
| public class XunitLogger : ILogger | ||
| { | ||
| private readonly ITestOutputHelper _output; | ||
|
|
||
| public XunitLogger(ITestOutputHelper output) | ||
| { | ||
| _output = output; | ||
| } | ||
|
|
||
| public IDisposable BeginScope<TState>(TState state) => null; | ||
| public bool IsEnabled(LogLevel logLevel) => true; | ||
|
|
||
| public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception exception, | ||
| Func<TState, Exception, string> formatter) | ||
| { | ||
| _output.WriteLine(formatter(state, exception)); | ||
| } | ||
| } |
There was a problem hiding this comment.
XunitLoggerProvider/XunitLogger are defined inline here but used by other test classes (DisposeTests/LogOutTests). That creates an implicit coupling between test files and also duplicates the existing CoreFtp.Tests.Integration.Logger providers. Consider moving these logger helpers into a dedicated shared file (or reusing the existing XUnitConsole logger implementation) so tests don’t depend on this particular test file being present.
| // Quick xunit logger provider | |
| public class XunitLoggerProvider : ILoggerProvider | |
| { | |
| private readonly ITestOutputHelper _output; | |
| public XunitLoggerProvider(ITestOutputHelper output) | |
| { | |
| _output = output; | |
| } | |
| public ILogger CreateLogger(string categoryName) => new XunitLogger(_output); | |
| public void Dispose() | |
| { | |
| } | |
| } | |
| public class XunitLogger : ILogger | |
| { | |
| private readonly ITestOutputHelper _output; | |
| public XunitLogger(ITestOutputHelper output) | |
| { | |
| _output = output; | |
| } | |
| public IDisposable BeginScope<TState>(TState state) => null; | |
| public bool IsEnabled(LogLevel logLevel) => true; | |
| public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception exception, | |
| Func<TState, Exception, string> formatter) | |
| { | |
| _output.WriteLine(formatter(state, exception)); | |
| } | |
| } |
| .AddXunitConsole( outputHelper, LogLevel.Debug ) | ||
| .AddDebug( LogLevel.Error ); | ||
| LoggerFactory = Microsoft.Extensions.Logging.LoggerFactory.Create(builder => | ||
| { |
There was a problem hiding this comment.
The test project is now being built with an SDK-provided entry point (via Microsoft.NET.Test.Sdk), but this file still defines its own Main(). That results in multiple entry points (CS0017). Consider removing Main from Program (keep Initialise), or otherwise ensure only one entry point is compiled for the test assembly.
| } | ||
|
|
||
| [Fact] | ||
| public async Task ChangeWorkingDirectoryAsync_ThrowsIndexOutOfRangeException_WhenPwdResponseLacksQuotes() |
There was a problem hiding this comment.
Test name says it "ThrowsIndexOutOfRangeException", but the assertions below expect the call to succeed and set WorkingDirectory. Rename the test to reflect the intended behavior (e.g., "does not throw when PWD response lacks quotes") so failures are diagnosable.
| public async Task ChangeWorkingDirectoryAsync_ThrowsIndexOutOfRangeException_WhenPwdResponseLacksQuotes() | |
| public async Task ChangeWorkingDirectoryAsync_DoesNotThrow_WhenPwdResponseLacksQuotes() |
| await writer.WriteLineAsync("230 Logged in"); | ||
| await reader.ReadLineAsync(); // FEAT | ||
| await writer.WriteLineAsync("211 End"); | ||
| await reader.ReadLineAsync(); // OPTS UTF8 | ||
| await writer.WriteLineAsync("200 OK"); | ||
| await reader.ReadLineAsync(); // TYPE I |
There was a problem hiding this comment.
This fake server replies to FEAT with only 211 End, which makes DetermineFeaturesAsync() return no features. The client will then not send OPTS UTF8 ON, but the server currently blocks waiting for it (// OPTS UTF8). Update the FEAT response to include a UTF8 line (e.g., 211-Features: + UTF8 + 211 End) or make the server accept the absence of the OPTS command.
| await writer.WriteLineAsync("230 Logged in"); | ||
| await reader.ReadLineAsync(); // FEAT | ||
| await writer.WriteLineAsync("211 End"); | ||
| await reader.ReadLineAsync(); // OPTS UTF8 | ||
| await writer.WriteLineAsync("200 OK"); | ||
| await reader.ReadLineAsync(); // TYPE I |
There was a problem hiding this comment.
Same issue as DisposeTests: replying to FEAT with only 211 End means the client won't enable UTF8 and therefore won't send OPTS UTF8 ON, but the fake server blocks waiting for it (// OPTS UTF8). Return a feature list including UTF8 or adjust the server script to handle either command sequence.
src/CoreFtp/FtpClient.cs
Outdated
| LogOutAsync().GetAwaiter().GetResult(); | ||
| } | ||
| } | ||
| catch { } |
There was a problem hiding this comment.
Dispose() currently swallows all exceptions from LogOutAsync() with an empty catch { }, which can hide unexpected failures and makes diagnosing disconnect issues difficult. Consider at least logging the exception, and/or applying a short timeout (e.g., using DisconnectTimeoutMilliseconds) so Dispose() can't block for up to the full TimeoutSeconds waiting for a server response.
| catch { } | |
| catch (Exception ex) | |
| { | |
| Logger?.LogError( ex, "An error occurred while logging out during FtpClient.Dispose()." ); | |
| } |
sparkeh9
force-pushed
the
fix/pre-v2-bug-fixes
branch
from
24e7873 to
7ed6a3d
Compare
No description provided.