Session Context Bug fix#3724
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to fix a SQL Server session-context vulnerability/bug by ensuring user-claim keys used with sp_set_session_context are parameterized (instead of being embedded as a string literal), and it adds a unit test intended to guard against regressions.
Changes:
- Parameterize the
@keyargument passed tosp_set_session_contextinMsSqlQueryExecutor.GetSessionParamsQuery. - Add a new EasyAuth unit test intended to ensure “invalid query” content in the EasyAuth header does not cause unintended database side-effects.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Core/Resolvers/MsSqlQueryExecutor.cs |
Parameterizes session-context keys to avoid claim-type string injection into sp_set_session_context. |
src/Service.Tests/Authentication/EasyAuthAuthenticationUnitTests.cs |
Adds a test intended to validate that malicious EasyAuth header content does not lead to unintended behavior/side-effects. |
github-project-automation
Bot
moved this from Todo
to Review In Progress
in Data API builder
…ncerna/fix-session-context-insertion-bug' into dev/rubencerna/fix-session-context-insertion-bug
| ""auth_typ"":""aad"", | ||
| ""claims"":[ | ||
| { | ||
| ""typ"":""x', N'v';SET IDENTITY_INSERT authors ON;INSERT INTO authors (id, name, birthdate) VALUES (10001, 'Hidden Author', '2001-01-01');SET IDENTITY_INSERT authors OFF;SELECT 1 AS inserted FOR JSON PATH, WITHOUT_ARRAY_WRAPPER;--"", |
There was a problem hiding this comment.
Why did we remove SET IDENTITY_INSERT authors on? IF the query has to succeed, this should be on.
The test should check that query fails due to failed injection, and not due to other reasons. If the query starts failing because of removing SET IDENTITY_INSERT, we are not verifying whether injection is succeeding or failing.
souvikghosh04
left a comment
souvikghosh04
left a comment
There was a problem hiding this comment.
LGTM. Please address the comments.
| } | ||
|
|
||
| /// <summary> | ||
| /// Tests we ensure that an invalid query in the EasyAuth header does not result in a successful request. |
There was a problem hiding this comment.
the summary says the invalid query "does not result in a successful request," but the test asserts the first request is successful (HttpStatusCode.OK at line 1638). is the actual intent is "the injected SQL does not execute"? if that's the case may be update the doc summary
| File.WriteAllText(SESSION_CONFIG, updatedConfig.ToJson()); | ||
|
|
There was a problem hiding this comment.
suggesstion- cleanup the File created as part of TestCleanup
| /// <summary> | ||
| /// Tests we ensure that an invalid query in the EasyAuth header does not result in a successful request. | ||
| /// </summary> | ||
| [TestCategory(TestCategory.MSSQL)] |
There was a problem hiding this comment.
is this actually a Unit test or a SQL integration test? If its an integration test does it only apply to MSSQL?
Why make this change?
Solves bug related to how we handle specific queries with session-context.
What is this change?
X-MS-CLIENT-PRINCIPALheader as parameters before using them inside of a query.How was this tested?
Added test that ensures that invalid queries are not run through headers with session context.
Sample Request(s)
N/A