Detect CR/LF and URL-encoded CR/LF in FtpWebRequest URI and command parameters#128983
Conversation
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
|
Tagging subscribers to this area: @karelz, @dotnet/ncl |
|
@copilot add support for checking all |
Co-authored-by: mrek-msft <188900745+mrek-msft@users.noreply.github.com>
Done in de62365 — the check now matches any of Note This response was generated by GitHub Copilot. |
|
@copilot. Add similar check (for each 4 separately) to |
Co-authored-by: mrek-msft <188900745+mrek-msft@users.noreply.github.com>
Done in 7096392 — converted Note This response was generated by GitHub Copilot. |
There was a problem hiding this comment.
Pull request overview
This PR tightens FTP input validation to reject newline characters earlier and more consistently, both in FtpWebRequest URI handling and when formatting FTP control commands, with corresponding test coverage.
Changes:
- Extend
FtpWebRequestconstruction-time validation to reject\r,\n, and%0D/%0Ain the URI’s original string. - Extend
FtpControlStream.FormatFtpCommandvalidation similarly for command parameters. - Add/expand xUnit theories to cover the new rejection cases (URI ctor, credentials, and
RenameTo).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/libraries/System.Net.Requests/src/System/Net/FtpWebRequest.cs | Broaden newline detection in the internal constructor to reject CR/LF and %0D/%0A. |
| src/libraries/System.Net.Requests/src/System/Net/FtpControlStream.cs | Broaden FormatFtpCommand validation to reject CR/LF and %0D/%0A in parameters. |
| src/libraries/System.Net.Requests/tests/FtpWebRequestTest.cs | Add theories covering literal and encoded newline variants across URI construction and command-parameter paths. |
Co-authored-by: mrek-msft <188900745+mrek-msft@users.noreply.github.com>
Co-authored-by: mrek-msft <188900745+mrek-msft@users.noreply.github.com>
FtpWebRequestalready rejects URIs containing a literal\r\nwith a clearFormatException, but standalone\ror\nand the URL-encoded forms%0D/%0Aslip past the check and fail later with a less informative exception — a common case when a stray CR/LF from upstream parsing gets URL-encoded for safety, or when a non-compliant URI contains only one of the two characters. The same gap existed inFtpControlStream.FormatFtpCommand, which only checked for literal\r\nin command parameters.Changes
src/libraries/System.Net.Requests/src/System/Net/FtpWebRequest.cs: Extend the constructor's newline check to detect any of\r,\n,%0D,%0Aindividually (case-insensitive for the encoded forms), throwing the sameFormatException(net_ftp_no_newlines) up front.src/libraries/System.Net.Requests/src/System/Net/FtpControlStream.cs: ExtendFormatFtpCommandto check for literal\rand\nin command parameters. Note: URL-encoded forms are not checked here because parameters are already unescaped when they reachFormatFtpCommand, so checking for literal"%0A"or"%0D"strings would incorrectly reject legitimate filenames containing those character sequences.src/libraries/System.Net.Requests/src/Resources/Strings.resx: Updated thenet_ftp_no_newlinesresource string from"CRLF character pair is not allowed in FtpWebRequest inputs."to"CR and LF characters are not allowed in FtpWebRequest inputs."to accurately reflect that individual CR and LF characters (and their URL-encoded forms in URIs) are each disallowed.src/libraries/System.Net.Requests/tests/FtpWebRequestTest.cs:Ctor_NewLineInUri_ThrowsFormatExceptiontheory covering literal\r\n, standalone\rand\n, and the%0D%0A/%0D/%0A(upper and lower case) variants.Ftp_NewLineInRenameTo_GetResponse_Throws_FormatException_As_InnerExceptiontheory that exercises the newFormatFtpCommandcheck via theRenameToproperty, gated with[ConditionalTheory(typeof(FtpWebRequestTest), nameof(LocalServerAvailable))]and using the sharedabsoluteUri, asserting that each literal\r,\n,\r\nvariant results in aWebExceptionwhoseInnerExceptionis aFormatException.Ftp_Ignore_NewLine_GetRequestStream_And_GetResponse_Throws_FormatException_As_InnerExceptiontest (previously a[ConditionalFact]covering only literal\r\n) into a[ConditionalTheory]withInlineDatafor each literal\r,\n,\r\nvariant applied to both the username and password of theNetworkCredential.