Fix | Reenable SqlBulkCopy in least-privilege environments#4306
Merged
Conversation
mdaigle
added
the
Hotfix 7.0.3
* 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
enabled auto-merge (squash)
June 11, 2026 14:15
paulmedynski
force-pushed
the
fix/issue-4293
branch
from
June 11, 2026 14:57
9e71953 to
c12875c
Compare
Contributor
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
mdaigle
reviewed
Jun 12, 2026
mdaigle
left a comment
mdaigle
left a comment
Contributor
There was a problem hiding this comment.
Still reviewing test changes.
edwardneal
commented
Jun 15, 2026
edwardneal
left a comment
edwardneal
left a comment
Contributor
Author
There was a problem hiding this comment.
Early responses and some additional commentary on the tests.
Contributor
There was a problem hiding this comment.
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_columnsaccess inSqlBulkCopy.CreateInitialQuery()viaHAS_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>, transientServerLogin/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. |
mdaigle
requested changes
Jun 17, 2026
github-project-automation
Bot
moved this from In review
to Waiting for customer
in SqlClient Board
github-project-automation
Bot
moved this from In review
to Waiting for customer
in SqlClient Board
auto-merge was automatically disabled
June 19, 2026 19:12
Head branch was pushed to by a user without write access
Contributor
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Contributor
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
paulmedynski
approved these changes
Jun 22, 2026
apoorvdeshmukh
approved these changes
Jun 22, 2026
paulmedynski
added a commit
to SimonCropp/SqlClient
that referenced
this pull request
Jun 24, 2026
- Guard graph-table (AS NODE/AS EDGE) UnprivilegedLogin test on SQL 2017+, since graph tables are unsupported on SQL Server 2016. - Correct CopyAllFromReader pre-2017 IduCount/Transactions expectations from 0 to 1; the least-privilege bulk copy change (dotnet#4306) added one IDU/transaction but only updated the >=2017 branch, leaving the pre-2017 branch stale.
cheenamalhotra
pushed a commit
that referenced
this pull request
Jul 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_NAMEcheck.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_NAMEis 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/CATCHblock won't work - whenXACT_ABORTis 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
ServerLoginandDatabaseUserprimitives. 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?