Add Active FTP mode support (Issue #43)#55
Conversation
- Add FtpDataConnectionType enum (AutoPassive, Active) - Add PORT and EPRT to FtpCommand enum - Add DataConnectionType and ActiveExternalIp config properties - Refactor ConnectDataStreamAsync into Passive/Active branches - Active mode: binds TcpListener, sends PORT, returns lazy-accept ActiveDataStream - ActiveDataStream defers AcceptTcpClient until first Read to avoid deadlock - Add WrapDataStreamAsync and LocalIpAddress to FtpControlStream - Add 2 integration tests using in-process fake FTP server - All 23 tests pass, no breaking changes
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
Activity
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 support for Active FTP mode, enhancing server compatibility through a well-designed lazy-accept stream. However, it introduces security risks by accepting incoming data connections without source IP validation and binding the listener to all network interfaces, which could lead to data interception or manipulation. Furthermore, the current active mode is limited to IPv4 via the PORT command; support for EPRT is recommended for IPv6 compatibility. A potential deadlock risk in ActiveDataStream due to sync-over-async calls also needs addressing to ensure robustness.
| return; | ||
|
|
||
| logger?.LogDebug( "[ActiveDataStream] Accepting inbound data connection from server" ); | ||
| acceptedClient = await listener.AcceptTcpClientAsync(); |
There was a problem hiding this comment.
In Active FTP mode, the client opens a local port and waits for the server to connect. The current implementation of ActiveDataStream.EnsureAcceptedAsync accepts the first TCP connection that arrives on the listening port without verifying the source IP address. An attacker who can connect to the client's listening port (e.g., by scanning or if the client is on a shared network) can intercept the data stream before the legitimate FTP server connects. This allows the attacker to either steal sensitive data being uploaded or inject malicious data into a download (e.g., a fake directory listing or a malicious file).
| public override int Read( byte[] buffer, int offset, int count ) | ||
| { | ||
| EnsureAccepted(); | ||
| return innerStream.Read( buffer, offset, count ); | ||
| } |
There was a problem hiding this comment.
This method uses EnsureAccepted(), which in turn calls EnsureAcceptedAsync().GetAwaiter().GetResult(). This sync-over-async pattern is dangerous and can lead to deadlocks in certain environments (like UI or ASP.NET applications). To make the stream safer to use, it's better to prevent synchronous operations if a fully synchronous implementation isn't feasible. I recommend throwing NotSupportedException to enforce async-only usage.
public override int Read( byte[] buffer, int offset, int count )
{
throw new NotSupportedException("Synchronous operations are not supported on this stream. Use ReadAsync instead.");
}| public override void Write( byte[] buffer, int offset, int count ) | ||
| { | ||
| EnsureAccepted(); | ||
| innerStream.Write( buffer, offset, count ); | ||
| } |
There was a problem hiding this comment.
Similar to the Read method, this synchronous Write method relies on a sync-over-async call via EnsureAccepted(), which can cause deadlocks. To ensure the safety and reliability of the library, this synchronous method should be disallowed. Throwing NotSupportedException will guide users to the correct asynchronous API.
public override void Write( byte[] buffer, int offset, int count )
{
throw new NotSupportedException("Synchronous operations are not supported on this stream. Use WriteAsync instead.");
}
src/CoreFtp/FtpClient.cs
Outdated
| if ( string.IsNullOrEmpty( localIp ) ) | ||
| throw new FtpException( "Could not determine local IP address for Active mode. Set ActiveExternalIp in configuration." ); | ||
|
|
||
| var listener = new System.Net.Sockets.TcpListener( System.Net.IPAddress.Any, 0 ); |
There was a problem hiding this comment.
The TcpListener is currently bound to IPAddress.Any, exposing the listening port on all network interfaces and increasing the attack surface. It is recommended to bind it specifically to the local IP address used for the control connection to mitigate this security risk. Furthermore, while addressing this, consider enhancing the active mode implementation within this method to support EPRT for IPv6 compatibility, as the current PORT command is limited to IPv4.
![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: 3cc0c6376d
ℹ️ 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".
| return; | ||
|
|
||
| logger?.LogDebug( "[ActiveDataStream] Accepting inbound data connection from server" ); | ||
| acceptedClient = await listener.AcceptTcpClientAsync(); |
There was a problem hiding this comment.
EnsureAcceptedAsync waits on AcceptTcpClientAsync() with no cancellation or timeout, so when Active mode is misconfigured (e.g., wrong ActiveExternalIp, NAT/firewall blocks callback, or server never opens data connection) any LIST/RETR/STOR read/write can hang indefinitely and never surface the FTP error response from the control channel. This effectively ignores TimeoutSeconds for the most failure-prone part of Active mode and can stall callers permanently.
Useful? React with 👍 / 👎.
| acceptedClient = await listener.AcceptTcpClientAsync(); | ||
| innerStream = await controlStream.WrapDataStreamAsync( acceptedClient ); |
There was a problem hiding this comment.
The first inbound connection is accepted and used as the transfer stream without checking that it comes from the same server as the control connection. In environments where another host can reach the client listener (especially plain FTP where PORT is visible on-path), a race connection can be accepted and treated as trusted data, allowing transfer corruption or data exfiltration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds Active FTP (PORT) support to CoreFtp so data connections can be established when passive mode (EPSV/PASV) is unavailable, addressing Issue #43.
Changes:
- Introduces
FtpDataConnectionTypeand configuration options to select Active vs. existing passive behavior. - Implements Active-mode data connections via a listener + lazy accept stream, plus control-stream wrapping for accepted sockets (including TLS).
- Adds integration tests using an in-process fake FTP server to validate Active-mode directory listing behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/CoreFtp.Tests.Integration/FtpClientTests/ActiveModeTests.cs | Adds integration coverage for Active mode LIST-based operations. |
| src/CoreFtp/Infrastructure/Stream/FtpControlStream.cs | Adds local IP discovery and a helper to wrap accepted Active-mode data sockets (incl. TLS). |
| src/CoreFtp/Infrastructure/Stream/ActiveDataStream.cs | Adds lazy-accept wrapper stream to avoid PORT/LIST sequencing deadlocks. |
| src/CoreFtp/FtpClientConfiguration.cs | Adds configuration switches for data connection type and external IP advertisement. |
| src/CoreFtp/FtpClient.cs | Adds Active-mode connection path and PORT command formatting helper. |
| src/CoreFtp/Enum/FtpDataConnectionType.cs | Adds enum for selecting passive vs active data connections. |
| src/CoreFtp/Enum/FtpCommand.cs | Adds PORT/EPRT commands to command enum. |
Comments suppressed due to low confidence (2)
src/CoreFtp/Infrastructure/Stream/FtpControlStream.cs:371
- WrapDataStreamAsync() is declared public on a public type but appears to be an internal implementation detail for Active mode. Making it internal would avoid expanding the public API surface with a method that likely isn’t intended for external callers.
public async Task<Stream> WrapDataStreamAsync( System.Net.Sockets.TcpClient acceptedClient )
{
Logger?.LogDebug( "[FtpSocketStream] Wrapping accepted data connection" );
var socketStream = new FtpControlStream( Configuration, dnsResolver )
{ Logger = Logger, IsDataConnection = true };
tests/CoreFtp.Tests.Integration/FtpClientTests/ActiveModeTests.cs:22
- Private readonly fields in the other FtpClient integration tests use an underscore prefix (_logger/_listener/_port). For consistency within this folder, consider renaming these fields to match the established convention.
private readonly ILogger logger;
private readonly TcpListener controlListener;
private readonly int controlPort;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public async Task<Stream> WrapDataStreamAsync( System.Net.Sockets.TcpClient acceptedClient ) | ||
| { | ||
| Logger?.LogDebug( "[FtpSocketStream] Wrapping accepted data connection" ); | ||
| var socketStream = new FtpControlStream( Configuration, dnsResolver ) | ||
| { Logger = Logger, IsDataConnection = true }; | ||
|
|
||
| socketStream.Socket = acceptedClient.Client; | ||
| socketStream.BaseStream = new NetworkStream( acceptedClient.Client ); | ||
|
|
||
| if ( IsEncrypted ) | ||
| { | ||
| await socketStream.ActivateEncryptionAsync(); | ||
| } | ||
|
|
||
| return socketStream; |
There was a problem hiding this comment.
WrapDataStreamAsync() builds a data-connection stream from an accepted socket but doesn’t apply the same socket configuration as OpenDataStreamAsync()/ConnectSocketAsync (e.g., ReceiveTimeout / linger / any other options), which can change timeout behavior for Active mode transfers and potentially hang indefinitely. Consider mirroring the data-socket setup performed for passive connections before returning the wrapped stream.
src/CoreFtp/FtpClient.cs
Outdated
| var localIp = Configuration.ActiveExternalIp ?? ControlStream.LocalIpAddress; | ||
| if ( string.IsNullOrEmpty( localIp ) ) | ||
| throw new FtpException( "Could not determine local IP address for Active mode. Set ActiveExternalIp in configuration." ); | ||
|
|
||
| var listener = new System.Net.Sockets.TcpListener( System.Net.IPAddress.Any, 0 ); | ||
| listener.Start(); | ||
|
|
||
| int localPort = ( (System.Net.IPEndPoint) listener.LocalEndpoint ).Port; | ||
|
|
||
| // Send PORT command: PORT h1,h2,h3,h4,p1,p2 | ||
| string portCommand = FormatPortCommand( localIp, localPort ); | ||
| var portResult = await ControlStream.SendCommandAsync( new FtpCommandEnvelope |
There was a problem hiding this comment.
ConnectActiveDataStreamAsync always uses PORT and FormatPortCommand assumes an IPv4 dotted-quad address. If ActiveExternalIp/LocalIpAddress is IPv6 (contains ':'), the generated command will be invalid and the listener is also IPv4-only (IPAddress.Any). Consider either implementing EPRT for IPv6 (and/or preferring EPRT with fallback), or explicitly validating and throwing a clear exception when a non-IPv4 address is supplied.
src/CoreFtp/FtpClient.cs
Outdated
| private static string FormatPortCommand( string ip, int port ) | ||
| { | ||
| string ipPart = ip.Replace( '.', ',' ); | ||
| int highByte = port / 256; | ||
| int lowByte = port % 256; | ||
| return $"{ipPart},{highByte},{lowByte}"; | ||
| } |
There was a problem hiding this comment.
FormatPortCommand accepts a free-form string and does a simple '.'→',' replace; this will silently produce malformed PORT arguments if ActiveExternalIp is a hostname, contains whitespace, or includes an IPv6 literal. Consider parsing/validating the input with IPAddress.TryParse (and rejecting non-IPv4 for PORT), so configuration mistakes fail fast with a clear message.
| private void EnsureAccepted() | ||
| { | ||
| if ( accepted ) | ||
| return; | ||
|
|
||
| EnsureAcceptedAsync().GetAwaiter().GetResult(); | ||
| } |
There was a problem hiding this comment.
EnsureAccepted() blocks on EnsureAcceptedAsync().GetAwaiter().GetResult(), which is sync-over-async and can deadlock under a SynchronizationContext. For the synchronous Read/Write paths, prefer TcpListener.AcceptTcpClient() (fully sync) or start the accept in advance and wait on it, rather than blocking on an async method.
| private async Task EnsureAcceptedAsync() | ||
| { | ||
| if ( accepted ) | ||
| return; | ||
|
|
||
| logger?.LogDebug( "[ActiveDataStream] Accepting inbound data connection from server" ); | ||
| acceptedClient = await listener.AcceptTcpClientAsync(); | ||
| innerStream = await controlStream.WrapDataStreamAsync( acceptedClient ); | ||
| accepted = true; | ||
| logger?.LogDebug( "[ActiveDataStream] Data connection accepted" ); | ||
| } |
There was a problem hiding this comment.
EnsureAcceptedAsync() uses AcceptTcpClientAsync() without any cancellation support, so ReadAsync/WriteAsync cancellation tokens won’t be able to abort a hung accept if the server never connects. Consider wiring cancellation through (e.g., AcceptTcpClientAsync(token) on newer TFMs, or Task.WhenAny with token + listener.Stop()), and ensure listener shutdown correctly unblocks the accept.
| logger?.LogDebug( "[ActiveDataStream] Accepting inbound data connection from server" ); | ||
| acceptedClient = await listener.AcceptTcpClientAsync(); | ||
| innerStream = await controlStream.WrapDataStreamAsync( acceptedClient ); | ||
| accepted = true; |
There was a problem hiding this comment.
Active mode currently accepts the first inbound connection to the listener without verifying it’s from the expected FTP server. This allows a different host to connect first and inject/consume data. Consider validating acceptedClient.RemoteEndPoint against the control connection’s remote address (or resolved server address) and rejecting unexpected peers before wrapping the stream.
- Validate active data peer IP matches control connection IP - Fix sync-over-async in ActiveDataStream by properly overriding Memory/Span APIs and disallowing sync I/O - Add cancellation/timeout support to ActiveDataStream accept logic - Bind active mode listener explicitly to resolved IPv4 address instead of IPAddress.Any - WrapDataStreamAsync applies LingerState and ReceiveTimeout correctly - Refactored RetrieveDirectoryListing to be async instead of blocking byte-by-byte reads - Ensure WrapDataStreamAsync API is internal - Update test field naming conventions
Summary
Adds Active FTP mode (PORT) support so the library can work with servers that don't support passive mode.
Closes #43
How It Works
Changes
New Files
FtpDataConnectionType.cs— enum withAutoPassive(default, existing behavior) andActiveActiveDataStream.cs— Lazy-accept wrapper stream that defersAcceptTcpClientAsyncuntil the first read, solving the PORT/LIST sequencing deadlockActiveModeTests.cs— 2 integration tests using in-process fake FTP serverModified Files
FtpCommand.cs— AddedPORTandEPRTcommandsFtpClientConfiguration.cs— AddedDataConnectionTypeandActiveExternalIppropertiesFtpClient.cs— RefactoredConnectDataStreamAsyncintoConnectPassiveDataStreamAsyncandConnectActiveDataStreamAsync; addedFormatPortCommandhelperFtpControlStream.cs— AddedLocalIpAddressproperty andWrapDataStreamAsyncmethodKey Design Decision: Lazy Accept
The FTP protocol requires the data command (LIST/STOR) to be sent after PORT, but the server only connects to the client's data port after receiving the data command. Since
ConnectDataStreamAsyncis called before the data command is sent, we useActiveDataStreamto defer the TCP accept until the firstRead()call. This avoids a deadlock without changing the existing method signatures.Backward Compatibility
AutoPassive= EPSV → PASV fallback)Test Results