Clarify and add security note about PersistSecurityInfo

[nycdotnet]

Summary

The existing description of PersistSecurityInfo is very difficult to understand. Setting this property to true is the wrong choice in 99% of applications and therefore opens up an unnecessary security risk.

As one example, when set to true, if a SqlConnection is serialized to JSON (such as if there is a db connection error being logged, or a query being traced), the password will be included in the JSON output and included in logs or a returned error message.

I opened this PR as an example of a way to fix this documentation. I'm sure you all may want to make improvements to this suggestion, but the current docs for this are both unclear and insufficient due to the security risks of setting this wrong.

using System.Data.SqlClient;

var csb1 = new SqlConnectionStringBuilder
{
    DataSource = "myserver",
    InitialCatalog = "mydatabase",
    UserID = "myuser",
    Password = "secretpassword",
    PersistSecurityInfo = true,
};

using var conn1 = new SqlConnection(csb1.ToString());
conn1.Open();

// conn1.ConnectionString will contain the password.  This is bad.
// JsonSerializer.Serialize(conn1) will also contain the password.  This is bad.

Thanks!

[ghost]

Tagging subscribers to this area: @cheenamalhotra, @David-Engel
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Summary

The existing description of PersistSecurityInfo is very difficult to understand. Setting this property to true is the wrong choice in 99% of applications and therefore opens up an unnecessary security risk.

As one example, when set to true, if a SqlConnection is serialized to JSON (such as if there is a db connection error being logged, or a query being traced), the password will be included in the JSON output and included in logs or a returned error message.

I opened this PR as an example of a way to fix this documentation. I'm sure you all may want to make improvements to this suggestion, but the current docs for this are both unclear and insufficient due to the security risks of setting this wrong.

using System.Data.SqlClient;

var csb1 = new SqlConnectionStringBuilder
{
    DataSource = "myserver",
    InitialCatalog = "mydatabase",
    UserID = "myuser",
    Password = "secretpassword",
    PersistSecurityInfo = true,
};

using var conn1 = new SqlConnection(csb1.ToString());
conn1.Open();

// conn1.ConnectionString will contain the password.  This is bad.
// JsonSerializer.Serialize(conn1) will also contain the password.  This is bad.

Thanks!

Author:	nycdotnet
Assignees:	-
Labels:	area-System.Data.SqlClient
Milestone:	-

[nycdotnet]

Note: there are several other places where PersistSecurityInfo is described and these should likely be updated in a similar manner to mention the security risks of turning it on in a real application:

dotnet-api-docs/xml/System.Data.OracleClient/OracleConnectionStringBuilder.xml

Line 766 in 8668630

<summary>Gets or sets a Boolean value that indicates if security-sensitive information, such as the password, is not returned as part of the connection if the connection is open or has ever been in an open state.</summary>

https://github.com/dotnet/dotnet-api-docs/search?q=PersistSecurityInfo

[opbld31]

Docs Build status updates of commit bdd6d60:

✅ Validation status: passed

File	Status	Preview URL	Details
xml/System.Data.SqlClient/SqlConnectionStringBuilder.xml	✅Succeeded	View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

• Try searching the docs.microsoft.com contributor guides
• Post your question in the Docs support channel

[opbld34]

Docs Build status updates of commit 0355928:

✅ Validation status: passed

File	Status	Preview URL	Details
xml/System.Data.SqlClient/SqlConnectionStringBuilder.xml	✅Succeeded	View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

• Try searching the docs.microsoft.com contributor guides
• Post your question in the Docs support channel

[nycdotnet]

can this be merged?

[David-Engel]

@gewarren Can this PR be merged?

[gewarren]

Left a few suggestions to better match the standard wording. Extra info should generally go in Remarks.

[gewarren]

Suggested change

<summary>Gets or sets a Boolean value indicating if security-sensitive information, such as the password or access token, should be returned as part of the connection string on a connection created with this <see cref="T:System.Data.SqlClient.SqlConnectionStringBuilder" /> after that connection has ever been in an open state. This property should only be set to <see langword="true" /> if your application has a specific need to read the password out of an already-opened database connection. The default value of <see langword="false" /> is the more secure setting; using <see langword="true" /> for this property opens your application to security risks such as accidentally logging or tracing the database password.</summary>

<summary>Gets or sets a value indicating if security-sensitive information, such as the password or access token, should be returned as part of the connection string on a connection created with this <see cref="T:System.Data.SqlClient.SqlConnectionStringBuilder" /> after that connection has ever been in an open state.</summary>

[gewarren]

Suggested change

	<value>The value of the <see cref="P:System.Data.SqlClient.SqlConnectionStringBuilder.PersistSecurityInfo" /> property, or <see langword="false" /> if none has been supplied.</value>
	<value>
	<see langword="true" /> if security-sensitive information is returned as part of the connection string; otherwise, <see langword="false" />. The default is <see langword="false" />.</value>

[gewarren]

Then I would put this info in the markdown Remarks section (I'd make a suggestion, but GitHub isn't letting me):

Note

This property should only be set to true if your application has a specific need to read the password out of an already-opened database connection. The default value of false is the more secure setting. Using true for this property opens your application to security risks such as accidentally logging or tracing the database password.

[nycdotnet]

@gewarren I made the changes you suggested. Only change is I used should be instead of is in the <value> section.

[opbld31]

Docs Build status updates of commit 2522186:

✅ Validation status: passed

File	Status	Preview URL	Details
xml/System.Data.SqlClient/SqlConnectionStringBuilder.xml	✅Succeeded	View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

• Try searching the docs.microsoft.com contributor guides
• Post your question in the Docs support channel

[gewarren]

Note block wasn't rendering properly in the preview.

[opbld34]

Docs Build status updates of commit 7b39ec6:

✅ Validation status: passed

File	Status	Preview URL	Details
xml/System.Data.SqlClient/SqlConnectionStringBuilder.xml	✅Succeeded	View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

• Try searching the docs.microsoft.com contributor guides
• Post your question in the Docs support channel

[nycdotnet]

@gewarren thanks for fix and edits.