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.

 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');


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: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.

…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.49%. Comparing base (47f4e52) to head (3d28d64).
⚠️ Report is 3 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.49%   -1.91%     
==========================================
  Files         285      280       -5     
  Lines       43331    66201   +22870     
==========================================
+ Hits        28337    42032   +13695     
- Misses      14994    24169    +9175

Flag	Coverage Δ
CI-SqlClient	`?`
PR-SqlClient-Project	`63.49% <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.

paulmedynski · 2026-06-18T13:17:31Z

/azp run

azure-pipelines · 2026-06-18T13:18:01Z

Azure Pipelines successfully started running 2 pipeline(s).

paulmedynski

Over all looks good. Asking for a few clarifications.

paulmedynski · 2026-06-18T13:40:30Z

+    /// <param name="database">The name of the database where the user will be created.</param>
+    /// <param name="login">The server login which the database user will be associated with.</param>
+    public DatabaseUser(SqlConnection connection, string database, ServerLogin login)
+        : base(connection, login.Name, $"FOR LOGIN {login.Name}", database, shouldCreate: true, shouldDrop: true)


I think it would help to always name the state: argument since it's type varies. It isn't obvious here that database is the state argument.

Also, seeing how state is used here, I'm not sure why it is called "State". I'm not sure what concept is the type parameter <TState> represents.

There are a few situations where we need to make specific information available to CreateObject which isn't part of the object definition. The issue is that this is called from the constructor of DatabaseObject, so any attempt to set the property in the derived class will come too late. Passing a state parameter down means that DatabaseObject can set the State property, then call CreateObject.

In the case of DatabaseUser, that state is the database name because CreateObject needs to switch to the specified database, issue the CREATE USER command, then switch back to the connection's original database.

Fixed in 83ef821.

paulmedynski · 2026-06-18T13:46:17Z

+
+    private void ExecuteCommandInDatabase(SqlCommand command)
+    {
+        string? originalDatabase = DatabaseName == command.Connection.Database ? null : command.Connection.Database;


Can you add comments explaining why we're comparing our DatabaseName to the connection's current database, and then swapping it around below?

Fixed in b65469d.

paulmedynski · 2026-06-18T15:23:52Z

+    [ConditionalFact(nameof(CanRunTests))]
+    public void BulkCopyWithoutMetadataPermission_Succeeds()
+    {
+        AssertEnvironmentCreated();


Interesting. I think all this is really doing is allowing you to access the instance members as though they are non-nullable (i.e. no null checks or ! everywhere).

We don't really need to test that xUnit behaves as documented. It will never execute a test method whose conditional isn't met, so at this point we know the constructor ran and the instance members aren't null.

I agree that this is a good approach to avoid the null clutter though.

That's correct - this is a two-part solution.

[MemberNotNull] clears away the variables' nullability annotation warnings for any method which calls it first.

Assert.NotNull's parameter is annotated with [NotNull], which eliminates the compile error which would otherwise occur (because an empty method means that control flow leaves the method without explicitly setting the variables to a non-null value.)

paulmedynski · 2026-06-19T10:44:29Z

+
+        nodeCopy.DestinationTableName = dstTable.Name;
+        nodeCopy.ColumnMappings.Add("Description", "Description");
+        nodeCopy.WriteToServer(srcDataTable);


Should we confirm that the destination received the data we expect?

paulmedynski · 2026-06-19T10:48:59Z

+            using SqlConnection connection = new(TCPConnectionString);
+
+            connection.Open();
+            using SqlCommand command = new("EXEC xp_instance_regread N'HKEY_LOCAL_MACHINE', N'Software\\Microsoft\\MSSQLServer\\MSSQLServer', N'LoginMode'", connection);


Wow that works on a Linux SQL Server?

Yes - SQL Server's PAL virtualises this

paulmedynski · 2026-06-19T10:50:56Z

+#nullable enable
+
+[AttributeUsage(AttributeTargets.Method | AttributeTargets.Property, Inherited = false, AllowMultiple = true)]
+internal sealed class MemberNotNullAttribute : Attribute


Can you document what this does, when it's used, and why we need it for .NET Framework?

paulmedynski

Whoops, forgot to choose Request changes.

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

paulmedynski enabled auto-merge (squash) June 18, 2026 13:17

paulmedynski moved this from Waiting for customer to In review in SqlClient Board Jun 18, 2026

paulmedynski reviewed Jun 19, 2026

View reviewed changes

paulmedynski requested changes Jun 19, 2026

View reviewed changes

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

edwardneal added 4 commits June 19, 2026 20:02

Code review: explicitly specify state parameter name

83ef821

Code review: add explanatory comment to ExecuteCommandInDatabase

b65469d

Code review: add explanatory comment to CodeAnalysis.netfx.cs

0041b59

Code review: verify row count in destination table after bulk copy

41d6013

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

	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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

paulmedynski commented Jun 18, 2026

Uh oh!

azure-pipelines Bot commented Jun 18, 2026

Uh oh!

paulmedynski left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jun 17, 2026 •

edited

Loading