Visitar URL original
Fix | Reenable SqlBulkCopy in least-privilege environments by edwardneal · Pull Request #4306 · dotnet/SqlClient · GitHub
Skip to content

Fix | Reenable SqlBulkCopy in least-privilege environments - #4306

Merged
cheenamalhotra merged 15 commits into
dotnet:mainfrom
edwardneal:fix/issue-4293
Jun 23, 2026
Merged

Fix | Reenable SqlBulkCopy in least-privilege environments#4306
cheenamalhotra merged 15 commits into
dotnet:mainfrom
edwardneal:fix/issue-4293

Conversation

@edwardneal

@edwardneal edwardneal commented May 24, 2026

Copy link
Copy Markdown
Contributor

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?

@edwardneal
edwardneal requested a review from a team as a code owner May 24, 2026 14:16
@github-project-automation github-project-automation Bot moved this to To triage in SqlClient Board May 24, 2026
@cheenamalhotra cheenamalhotra moved this from To triage to In review in SqlClient Board May 26, 2026
@mdaigle mdaigle modified the milestones: 7.0.2, 7.1.0-preview2 Jun 9, 2026
@mdaigle mdaigle added the Hotfix 7.0.3 PRs targeting main that should be backported to release/7.0 branch for next release. label Jun 9, 2026
@paulmedynski paulmedynski modified the milestone: 7.1.0-preview2 Jun 11, 2026
@edwardneal @paulmedynski
RAII primitive scaffolding
ec18bd2 
* Add an UnescapedName property to DatabaseObject and use it in XEventsTracingTest.
* Create ServerLogin primitive.
* Create DatabaseUser primitive.
@edwardneal @paulmedynski
Add relevant test conditions
823de0f
@edwardneal @paulmedynski
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.
@edwardneal @paulmedynski
Fix bug and add regression tests
c12875c 
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
paulmedynski enabled auto-merge (squash) June 11, 2026 14:15
@paulmedynski

paulmedynski commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

azure-pipelines Bot commented Jun 11, 2026

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

mdaigle
mdaigle reviewed Jun 12, 2026
View reviewed changes

@mdaigle mdaigle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still reviewing test changes.

Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs Resolved
Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs Resolved
Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs Resolved
Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs Resolved
edwardneal
edwardneal commented Jun 15, 2026
View reviewed changes

@edwardneal edwardneal left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Early responses and some additional commentary on the tests.

Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs Resolved
Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs Resolved
Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs Resolved
Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs Resolved
Comment thread src/Microsoft.Data.SqlClient/tests/Common/Fixtures/DatabaseObjects/DatabaseObject.cs Resolved
Comment thread src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/UnprivilegedLogin.cs Resolved
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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs Resolved
Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs Resolved
Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs Resolved
Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs Resolved
Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs Resolved
Comment thread src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/DataTestUtility.cs Resolved
Comment thread src/Microsoft.Data.SqlClient/tests/Common/Fixtures/DatabaseObjects/DatabaseObject.cs Resolved
Comment thread src/Microsoft.Data.SqlClient/tests/ManualTests/Extensions/CodeAnalysis.netfx.cs Resolved
Comment thread src/Microsoft.Data.SqlClient/tests/Common/Fixtures/DatabaseObjects/ServerLogin.cs Resolved
Comment thread src/Microsoft.Data.SqlClient/tests/Common/Fixtures/DatabaseObjects/ServerLogin.cs Resolved
mdaigle
mdaigle requested changes Jun 17, 2026
View reviewed changes
Comment thread src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/CopyAllFromReader.cs Resolved
Comment thread src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/UnprivilegedLogin.cs Resolved
@github-project-automation github-project-automation Bot moved this from In review to Waiting for customer in SqlClient Board Jun 17, 2026
This was referenced Nov 9, 2026
Open
Closed
Merged
Open
Merged
Open
Open
Closed
Merged
Open
Open
Closed
Closed
Open
Open
Closed
Merged
Merged
Open
Closed
Closed
Merged
Open
Open
Open
Open
Open
Open
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paulmedynski paulmedynski paulmedynski approved these changes
Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@apoorvdeshmukh apoorvdeshmukh apoorvdeshmukh approved these changes
@mdaigle mdaigle Awaiting requested review from mdaigle

Assignees

@mdaigle mdaigle

@apoorvdeshmukh apoorvdeshmukh

Labels

Hotfix 7.0.3 PRs targeting main that should be backported to release/7.0 branch for next release.

Projects

SqlClient Board
Status: Done

Milestone

7.1.0-preview2

Development

Successfully merging this pull request may close these issues.

SqlBulkCopy v7 breaking change: now requires metadata visibility for sys.all_columns

6 participants

@edwardneal @paulmedynski @mdaigle @apoorvdeshmukh @cheenamalhotra