Fix | Reenable SqlBulkCopy in least-privilege environments by edwardneal · Pull Request #4306 · dotnet/SqlClient

edwardneal · 2026-05-24T14:16:32Z

Description

SqlBulkCopy has traditionally only normally required SELECT and INSERT permissions over the table being copied to (src). However, recent additions to the class also required SELECT permissions over the [sys].[all_columns] metadata view. These permissions are present by default but are sometimes removed as part of SQL Server hardening.

This change to the permissions contract was unintentional, the failure of SqlBulkCopy in least-privilege environments which only grant SELECT & INSERT permissions was a bug. This PR resolves the bug by guarding all access to the metadata view behind a simple HAS_PERMS_BY_NAME check.

As a result of this change, least-privilege environments will be unable to use the newer features introduced in #3677 and #3590. I think this is unavoidable - there's no way to implement the features safely without access to the metadata view.

HAS_PERMS_BY_NAME is documented here as being supported in SQL Server, Azure SQL, managed instances and Fabric. It doesn't document support for two platforms: Azure Synapse Analytics, and APS/PDW. However, other documentation confirms that Azure Synapse Serverless pools support this function, and testing against a dedicated pool confirms that this does too. Furthermore, the APS/PDW 2016 release notes explicitly document the introduction of this function.

I've chosen to use this approach because wrapping metadata access in a TRY/CATCH block won't work - when XACT_ABORT is enabled, even a caught exception will result in the wrapping transaction being doomed, and Synapse doesn't allow this option to be temporarily turned off.

Issues

Fixes #4293.

Testing

Existing SqlBulkCopy tests continue to pass. I've added two new tests which build a low-privileged environment, then run a bulk copy inside it. They verify that the copy succeeds, but also that the newer alias-based functionality is unavailable.

As part of this, I've added ServerLogin and DatabaseUser primitives. The latter primitive highlighted the need for some form of state (the database in which we create the user) to be made available at the point of creation, so I've added this.

This widened test infrastructure and more complex testing accounts for about 3/4 of the changes in this PR.

Could someone start a CI run please?

* Add an UnescapedName property to DatabaseObject and use it in XEventsTracingTest. * Create ServerLogin primitive. * Create DatabaseUser primitive.

This makes DatabaseUser simpler to work with. It can store a reference to the database the user should be created in, and the class can handle switching to/from that database.

Fix bug by checking HAS_PERMS_BY_NAME over the sys.all_columns view. This is safe on all supported platforms (including Synapse Analytics dedicated and shared pools - documentation is inconsistent.) Regression tests create a low-privileged login and user in on-premise environments, then use those credentials to perform a SqlBulkCopy.

paulmedynski · 2026-06-11T14:58:24Z

/azp run

azure-pipelines · 2026-06-11T14:59:16Z

Azure Pipelines successfully started running 2 pipeline(s).

mdaigle

Still reviewing test changes.

mdaigle · 2026-06-12T22:13:47Z

 DECLARE @Column_Name_Query_SORT NVARCHAR(MAX);
 DECLARE @Column_Name_Query NVARCHAR(MAX);
 DECLARE @Column_Names NVARCHAR(MAX) = NULL;
+DECLARE @Has_Permissions INT = HAS_PERMS_BY_NAME('{catalogNameStringLiteral}.[sys].[all_columns]', 'OBJECT', 'SELECT');


✏️

Suggested change

DECLARE @Has_Permissions INT = HAS_PERMS_BY_NAME('{catalogNameStringLiteral}.[sys].[all_columns]', 'OBJECT', 'SELECT');

DECLARE @Has_Sys_All_Columns_Permissions INT = HAS_PERMS_BY_NAME('{catalogNameStringLiteral}.[sys].[all_columns]', 'OBJECT', 'SELECT');

No objection here, but I'll change this once the rest of the PR review is complete.

edwardneal

Early responses and some additional commentary on the tests.

edwardneal · 2026-06-12T23:37:45Z

 DECLARE @Column_Name_Query_SORT NVARCHAR(MAX);
 DECLARE @Column_Name_Query NVARCHAR(MAX);
 DECLARE @Column_Names NVARCHAR(MAX) = NULL;
+DECLARE @Has_Permissions INT = HAS_PERMS_BY_NAME('{catalogNameStringLiteral}.[sys].[all_columns]', 'OBJECT', 'SELECT');


No objection here, but I'll change this once the rest of the PR review is complete.

edwardneal · 2026-06-15T05:47:44Z

+/// The type of the internal state accessible to derived types at the point of object creation
+/// via the <see cref="State"/> property.
+/// </typeparam>
+public abstract class DatabaseObject<TState> : IDisposable


This is designed to solve a specific problem: generically passing state to a database object, if that state is required in order to create it. A database user is one example, but another example may be passing column encryption keys for creating Always Encrypted tables, or certificates for creating column encryption keys.

We can't just a property to do this - the base class' constructor will run first and try to create the object before the property is set.

edwardneal · 2026-06-15T05:52:39Z

+        SqlConnectionStringBuilder tcpConnectionBuilder = new(DataTestUtility.TCPConnectionString)
+        {
+            IntegratedSecurity = false,
+            UserID = _unprivilegedLogin.UnescapedName,


This requirement for the unescaped name uses identical logic to XEventsTracingTest (and a few other places which are yet to have the RAII object retrofitted) so I added it to the DatabaseObject<TState> base class.

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR improves SqlBulkCopy behavior when the caller lacks permissions to read sys.all_columns, adding permission-gated alias/hidden-column support and new manual tests/fixtures for unprivileged-login scenarios.

Changes:

Gate sys.all_columns access in SqlBulkCopy.CreateInitialQuery() via HAS_PERMS_BY_NAME, falling back to * when metadata access is denied.
Add manual tests that create an unprivileged SQL login/user and validate bulk copy behavior with/without column aliases.
Add test infrastructure: generic DatabaseObject<TState>, transient ServerLogin/DatabaseUser, and server-role/auth-mode helpers.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/XEventsTracingTest.cs	Switches to `UnescapedName` helper for SP name normalization in tracing verification.
src/Microsoft.Data.SqlClient/tests/ManualTests/Extensions/CodeAnalysis.netfx.cs	Adds a NETFRAMEWORK-only `MemberNotNullAttribute` shim for nullability annotations.
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs	Adds cached role/auth-mode checks and helpers used to gate new manual tests.
src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/UnprivilegedLogin.cs	Introduces manual tests that run bulk copy under an unprivileged SQL login.
src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/CopyAllFromReader.cs	Updates expected tracing counter values.
src/Microsoft.Data.SqlClient/tests/Common/Fixtures/DatabaseObjects/ServerLogin.cs	Adds transient server-login fixture (create/drop) for tests.
src/Microsoft.Data.SqlClient/tests/Common/Fixtures/DatabaseObjects/DatabaseUser.cs	Adds transient database-user fixture (create/drop) for tests.
src/Microsoft.Data.SqlClient/tests/Common/Fixtures/DatabaseObjects/DatabaseObject.cs	Refactors fixture base class to `DatabaseObject<TState>` and adds `UnescapedName`.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs	Adds metadata-permission gating and alias behavior changes in initial query generation.

edwardneal · 2026-06-17T20:20:30Z

+                CatalogName = SqlServerEscapeHelper.EscapeIdentifier(CatalogName);
            }

            string objectName = ADP.BuildMultiPartName(parts);
            string escapedObjectName = SqlServerEscapeHelper.EscapeStringAsLiteral(objectName);
+            string catalogNameStringLiteral = CatalogName is null ? null : SqlServerEscapeHelper.EscapeStringAsLiteral(CatalogName);


This is incorrect for a few reasons:

EscapeStringAsLiteral doesn't wrap the text in quotes, it just escapes them. Wrapping it in quotes in HAS_PERMS_BY_NAME is thus valid.

It's used as an identifier in {catalogNameStringLiteral}.[sys]... because that query is dynamically built as a string.

.[sys].[all_columns] is a completely valid three-part name. Not all of the parts need to be specified, and so there's no need for null handling - the conversion to an empty string will be absolutely fine.

The same apply for the other four identical comments.

 DECLARE @Column_Name_Query_SORT NVARCHAR(MAX);
 DECLARE @Column_Name_Query NVARCHAR(MAX);
 DECLARE @Column_Names NVARCHAR(MAX) = NULL;
+DECLARE @Has_Permissions INT = HAS_PERMS_BY_NAME('{catalogNameStringLiteral}.[sys].[all_columns]', 'OBJECT', 'SELECT');


+IF @Has_Permissions = 1
 BEGIN
-    SET @Column_Name_Query_FILTER = N'WHERE [object_id] = @Object_ID';
+    IF EXISTS (SELECT TOP 1 * FROM {CatalogName}.[sys].[all_columns] WHERE [object_id] = OBJECT_ID('{catalogNameStringLiteral}.[sys].[all_columns]') AND [name] = 'graph_type')


+
+        EXEC sp_executesql N'
+        INSERT INTO #Column_Aliases ([Canonical_Column_Name], [Canonical_Column_Id], [Aliased_Column_Name])
+            SELECT [name], [column_id], ''$to_id'' FROM {catalogNameStringLiteral}.[sys].[all_columns] WHERE [object_id] = @Object_ID AND COALESCE([graph_type], 0) = 8


+    BEGIN
+        SET @Column_Name_Query_FILTER = N'WHERE [object_id] = @Object_ID';
+    END
+    SET @Column_Name_Query = @Column_Name_Query_SELECT + ' FROM {catalogNameStringLiteral}.[sys].[all_columns] ' + @Column_Name_Query_FILTER + ' ' + @Column_Name_Query_SORT + ';'


edwardneal · 2026-06-17T20:24:23Z

+            using SqlCommand command = new("SELECT IS_SRVROLEMEMBER(@role)", connection);
+
+            connection.Open();
+            command.Parameters.AddWithValue("@role", roleName);


That's fine. @role is used as the first parameter to IS_SRVROLEMEMBER, which is defined as sysname - an nvarchar(128).

edwardneal · 2026-06-17T20:24:57Z

    public string Name { get; }

-    protected DatabaseObject(SqlConnection connection, string name, string definition, bool shouldCreate, bool shouldDrop)
+    public string UnescapedName => Name.Substring(1, Name.Length - 2).Replace("]]", "]");


Name is always bracket-escaped.

edwardneal · 2026-06-17T20:25:12Z

+[AttributeUsage(AttributeTargets.Method | AttributeTargets.Property, Inherited = false, AllowMultiple = true)]
+internal sealed class MemberNotNullAttribute : Attribute
+{
+    public MemberNotNullAttribute(string member) => Members = [member];


LangVersion is appropriately set.

edwardneal · 2026-06-17T20:26:26Z

+        Random rnd = new();
+
+        for(int i = 0; i < 5; i++)
+        {
+            passwordDigits[i] = (char)(UpperCaseStart + rnd.Next(26));
+        }


That's fair. I'll change this at the same time as I rename the @Has_Permissions variable.

+        for (int i = 10; i < PasswordLength; i++)
+        {
+            passwordDigits[i] = (char)(DigitsStart + rnd.Next(10));
+        }


…ssue-4293

mdaigle · 2026-06-17T18:36:12Z

/azp run

azure-pipelines · 2026-06-17T18:36:44Z

Azure Pipelines successfully started running 2 pipeline(s).

codecov · 2026-06-17T20:31:01Z

Codecov Report

❌ Patch coverage is 96.55172% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 63.47%. Comparing base (47f4e52) to head (9babd0e).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...Client/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs	96.55%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4306      +/-   ##
==========================================
- Coverage   65.39%   63.47%   -1.93%     
==========================================
  Files         285      280       -5     
  Lines       43331    66201   +22870     
==========================================
+ Hits        28337    42021   +13684     
- Misses      14994    24180    +9186

Flag	Coverage Δ
CI-SqlClient	`?`
PR-SqlClient-Project	`63.47% <96.55%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

edwardneal requested a review from a team as a code owner May 24, 2026 14:16

github-project-automation Bot added this to SqlClient Board May 24, 2026

github-project-automation Bot moved this to To triage in SqlClient Board May 24, 2026

cheenamalhotra moved this from To triage to In review in SqlClient Board May 26, 2026

mdaigle modified the milestones: 7.0.2, 7.1.0-preview2 Jun 9, 2026

mdaigle added the Hotfix 7.0.2 PRs targeting main that should be backported to release/7.0 for the 7.0.2 release. label Jun 9, 2026

mdaigle assigned mdaigle and apoorvdeshmukh Jun 9, 2026

paulmedynski modified the milestone: 7.1.0-preview2 Jun 11, 2026

edwardneal added 4 commits June 11, 2026 11:10

RAII primitive scaffolding

ec18bd2

* Add an UnescapedName property to DatabaseObject and use it in XEventsTracingTest. * Create ServerLogin primitive. * Create DatabaseUser primitive.

Add relevant test conditions

823de0f

Modify DatabaseObject to support state

2d672e4

This makes DatabaseUser simpler to work with. It can store a reference to the database the user should be created in, and the class can handle switching to/from that database.

paulmedynski enabled auto-merge (squash) June 11, 2026 14:15

paulmedynski force-pushed the fix/issue-4293 branch from 9e71953 to c12875c Compare June 11, 2026 14:57

mdaigle reviewed Jun 12, 2026

View reviewed changes

edwardneal commented Jun 15, 2026

View reviewed changes

apoorvdeshmukh requested review from Copilot, mdaigle and paulmedynski June 17, 2026 17:13

Copilot started reviewing on behalf of apoorvdeshmukh June 17, 2026 17:13 View session

Copilot stopped reviewing on behalf of apoorvdeshmukh due to an error June 17, 2026 17:33
Copilot had to stop work due to a timeout.

Copilot AI reviewed Jun 17, 2026

View reviewed changes

mdaigle requested changes Jun 17, 2026

View reviewed changes

Comment thread src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/CopyAllFromReader.cs

Comment thread src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/UnprivilegedLogin.cs

github-project-automation Bot moved this from In review to Waiting for customer in SqlClient Board Jun 17, 2026

mdaigle added 2 commits June 17, 2026 11:30

Add to test set.

934cee7

Merge branch 'main' of https://github.com/dotnet/SqlClient into fix/i…

9babd0e

…ssue-4293

mdaigle previously approved these changes Jun 17, 2026

View reviewed changes

edwardneal added 3 commits June 17, 2026 23:14

Code review: use RandomNumberGenerator when generating passwords

3c800f7

Code review: correct reference to database user in permission grant

35b1a1f

Code review: rename variable

3d28d64

auto-merge was automatically disabled June 17, 2026 22:25
Head branch was pushed to by a user without write access

edwardneal dismissed mdaigle’s stale review via 3d28d64 June 17, 2026 22:25

edwardneal requested a review from mdaigle June 17, 2026 22:26

	DECLARE @Has_Permissions INT = HAS_PERMS_BY_NAME('{catalogNameStringLiteral}.[sys].[all_columns]', 'OBJECT', 'SELECT');
	DECLARE @Has_Sys_All_Columns_Permissions INT = HAS_PERMS_BY_NAME('{catalogNameStringLiteral}.[sys].[all_columns]', 'OBJECT', 'SELECT');

Conversation

edwardneal commented May 24, 2026

Description

Issues

Testing

Uh oh!

paulmedynski commented Jun 11, 2026

Uh oh!

azure-pipelines Bot commented Jun 11, 2026

Uh oh!

mdaigle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

edwardneal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mdaigle commented Jun 17, 2026

Uh oh!

azure-pipelines Bot commented Jun 17, 2026

Uh oh!

codecov Bot commented Jun 17, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants