System.Net.Mail.MailAddress.ToString() issue with embedded double quotes in DisplayName

[J0nKn1ght]

Description

If a MailAddress instance is created with a DisplayName that includes embedded double quote characters, the ToString method doesn't properly escape the embedded double quotes.

The ToString method in the repository is currently:

        /// <summary>
        /// this returns the full address with quoted display name.
        /// i.e. "some email address display name" &lt;user@host&gt;
        /// if displayname is not provided then this returns only user@host (no angle brackets)
        /// </summary>
        public override string ToString()
        {
            if (string.IsNullOrEmpty(DisplayName))
            {
                return Address;
            }
            else
            {
                return "\"" + DisplayName + "\" " + SmtpAddress;
            }
        }

link to above source

If the DisplayName property includes double quotes, then these are not escaped properly before surrounding the string with additional double quotes.
For example, if the display name is 'Henry "The Fonz" Winkler', when the ToString method is called, the resulting string would be:
\"Henry \"The Fonz\" Winkler\" <[email protected]>
but should be:
\"Henry \\\"The Fonz\\\" Winkler\" <[email protected]>

Configuration

Tested on .Net Core v2.2, but the source in this repository looks like it is still an issue in the latest version.

Regression?

Not known if this worked correctly on previous versions of .Net Framework / .Net Core

Other information

[ghost]

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

[scalablecory]

ToString() is not intended to return a serialized string, merely one that is human-readable. I don't know that I would want to escape the inner quotes here, as it would sacrifice readability.

[J0nKn1ght]

OK, thanks. So is there no built in way to produce a full mail address string which can be re-parsed? I've added an extension method to build the elements up and escape double quotes in the DisplayName property, but it seemed like there should be a method in the framework to produce a string which can be re-parsed using the MailAddress constructor (or anything else that accepts an RFC 2822 format email address).
Happy to carry on using my extension method if you don't think it would be useful.

[karelz]

Triage: We already allow proper parsing (added in #907). Adding proper serialization as mirroring functionality makes sense. Using ToString is probably best.
Looks like fairly straightforward change -- any interest to contribute a PR @J0nKn1ght?
cc @MarcoRossignoli

[J0nKn1ght]

Hi @karelz - Sorry, I've been busy with project deadlines. As you say, it's a simple change, so I could look at doing a PR. Do you think that it's acceptable to always return a mail address from ToString with escaped embedded double quotes? Can you envisage any circumstances where that could break existing code that uses ToString?

[jaymin93]

I was also checking for the issue you opened but when I add double quotes string the values I am getting back the same way so can you explain. Why you need this actually I seen your expected output but it's escaping the already escaped string not the original one so what display name currently returns that seems perfect for me

[jaymin93]

And in your extension method are you replacing only double quotes or any other special char can you show extension method ?

[J0nKn1ght]

@jaymin93 So, if you have a variable (say 'displayName') that contains:

Henry "The Fonz" Winkler

and you instantiate a new MailAddress with:

var mailAddress = new MailAddress("[email protected]", displayName);

if you call the ToString() method and view the result it contains this:

"Henry "The Fonz" Winkler" <[email protected]>

i.e. there are four double quote characters, and the ones around 'The Fonz' are not escaped (to a higher level than the surrounding ones). Therefore, anything trying to parse the mail address will match the first double quote with the one before 'The...', not the one after 'Winkler'. To correctly parse the mail address, the double quotes around 'The Fonz' must be escaped.

We're actually getting the error when passing the value from ToString to the constructor of MailAddress again, which should parse it correctly, but I suspect that anything trying to parse an RFC 2822 mail address will have the same problem. There's information on page 9 of the RFC 2822 document about quoted pairs, and an example on page 41 of a mail address containing double quote characters, showing that they have to be escaped.

The extension method that I'm using is this:

        public static string ToSafeString(this MailAddress mailAddress)
        {
            if (!string.IsNullOrEmpty(mailAddress.DisplayName))
                return $"\"{mailAddress.DisplayName.Replace("\"", "\\\"")}\" <{mailAddress.Address}>";
            else
                return mailAddress.Address;
        }

As the C# string contains double quote characters that are already escaped, the extension method replaces any embedded double quotes with two additional escape characters, so that the escape character is itself escaped.

Does that make sense?

[jaymin93]

ok now i got what your concern is all about , and i will be happy to add pr for same if you do not mind . so should i go ahead and add pr ?

@karelz i am interested adding pr for same , should i create overload of tostring( bool escape) or should i add replace logic in the existing to string ?

[wfurt]

I think we should just fix ToString() @jaymin93 . Adding any new API surface would require API review and it would defeat the intention to use ToString() for serialization as well as for debug.

[jaymin93]

OK so I will go ahead with fixing with replacement logic and send pr will it work?

[karelz]

That's the best bet - let's hope no surprises / other consideration come into your PR's way ;)

Thanks @jaymin93

[jaymin93]

I have added logic for replacement and ran the test,(it's my first pr on gihutb and I am new here) what should I do? to add pr I try to commit the code from vs, obviously I should be not Able to commit in master and it shown me the same error. Then in which branch Should I commit this code? Please guide me in case I am doing any wrong steps for committing code thanks.

[wfurt]

Take a look at https://github.com/dotnet/runtime/blob/master/docs/pr-guide.md
In essence:

• make fork in your workspace
• create brach (name does not matter but I like them descriptive)
• build and run tests current tests
• add tests
• make fix
• run tests
• repeat previous 3 stems as needed

when ready, create PR, reference this issue in description.

[jaymin93]

i have certain issues with unit test as below , any advice ?

i have all pre requisite for building libs , i ran build.cmd but vs shows various error

[wfurt]

try to build the whole project from command line with tests - at least once.
@ViktorHofer may have more insight to what exact VS version you need.
There were some recent changes and I think it may needs particular version to make tests working right.

[ViktorHofer]

The minimum required VS version is 16.6 Preview 2, see the windows-requirements.md file for more infos.

[ViktorHofer]

Can you please try to restore the solution from CLI first?

[jaymin93]

yes using your suggestion to restore from cli worked and errors for tests are gone now and it's building the both source and test , but while running the test i am getting below output in both vs stable and latest preview

any help or suggestion would be appreciated thanks.

[ViktorHofer]

Sure, happy to help. How did you build the repository first? Did you invoke "build.cmd clr+libs -rc Release"?

[jaymin93]

i used this steps

https://github.com/dotnet/runtime/blob/master/docs/workflow/building/libraries/README.md

and executed below steps for building lib
https://github.com/dotnet/runtime/blob/master/docs/workflow/building/libraries/README.md

git clean -xdf
git pull upstream master & git push origin master
:: Build Debug libraries on top of Release runtime:
build -subset clr+libs -runtimeConfiguration Release
:: The above you may only perform once in a day, or when you pull down significant new changes.

:: If you use Visual Studio, you might open System.Text.RegularExpressions.sln here.
build -vs System.Net.Mail

:: Switch to working on a given library (RegularExpressions in this case)
cd src\libraries\System.Net.Mail

:: Change to test directory
cd tests

:: Then inner loop build / test
:: (If using Visual Studio, you might run tests inside it instead)
pushd ..\src & dotnet build & popd & dotnet build /t:test

[ViktorHofer]

Oh I see what happened here. When you open VS can you also pass in the runtimeconfiguration which you built before:

build.cmd -vs System.Net.Mail -rc Release

[jaymin93]

i used command you shared

build.cmd -vs System.Net.Mail -rc Release

still shows the same issue

---------- Starting test discovery for requested test run ----------
Using automatically detected runsettings file(s). To learn more visit https://aka.ms/vs-runsettings.
Microsoft.VisualStudio.TestPlatform.ObjectModel.TestPlatformException: Testhost process exited with error: Could not resolve CoreCLR path. For more details, enable tracing by setting COREHOST_TRACE environment variable to 1
. Please check the diagnostic logs for more information.
at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager.ThrowOnTestHostExited(Boolean testHostExited)
at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager.SetupChannel(IEnumerable1 sources, String runSettings) at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyDiscoveryManager.DiscoverTests(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEventsHandler2 eventHandler) Microsoft.VisualStudio.TestPlatform.ObjectModel.TestPlatformException: Testhost process exited with error: Could not resolve CoreCLR path. For more details, enable tracing by setting COREHOST_TRACE environment variable to 1 . Please check the diagnostic logs for more information. at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager.ThrowOnTestHostExited(Boolean testHostExited) at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager.SetupChannel(IEnumerable1 sources, String runSettings)
at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyDiscoveryManager.DiscoverTests(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEventsHandler2 eventHandler)
Microsoft.VisualStudio.TestPlatform.ObjectModel.TestPlatformException: Testhost process exited with error: Could not resolve CoreCLR path. For more details, enable tracing by setting COREHOST_TRACE environment variable to 1
. Please check the diagnostic logs for more information.
at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager.ThrowOnTestHostExited(Boolean testHostExited)
at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager.SetupChannel(IEnumerable`1 sources, String runSettings)
at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyDiscoveryManager.DiscoverTests(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEventsHandler2 eventHandler)
========== Test discovery aborted: 0 Tests found in 638.1 ms ==========
No tests found to run.

[ViktorHofer]

OK sorry for the issue that you are hitting here. I will look into it later today and will get back to you by tomorrow.

[jaymin93]

@ViktorHofer its strange my two machines 1 dell 2 surface go shows issues running test while my other PC which is not accessible most of time runs test well ( very slow 3 minutes to run single but runs ) where as all of them are on same vs and source code and windows required components installed