Skip to content

Fix serialization issue for config#1386

Merged
abhishekkumams merged 3 commits intomainfrom
dev/abhishekkuma/fix_serialization_issue_for_config
Mar 30, 2023
Merged

Fix serialization issue for config#1386
abhishekkumams merged 3 commits intomainfrom
dev/abhishekkuma/fix_serialization_issue_for_config

Conversation

@abhishekkumams
Copy link
Copy Markdown
Contributor

@abhishekkumams abhishekkumams commented Mar 30, 2023

Why make this change?

What is this change?

  • Updated the Encoder used in the config serialization.
  • Now using UnsafeRelaxedJsonEscaping, it does not escape HTML-sensitive characters such as <, >, &. Since we do not want any kind of change during string serialization, it's the best we can use.
  • It also allows UnicodeRanges.All to go through unescaped. Hence, it doesn't regress our existing changes.

How was this tested?

  • Unit Tests
  • I created a new user in Azure SQL with a password containing special characters such as [!@#$%^&*() ], and i was able to start the engine succesfully.

NOTE:

Since the config file is json and it is storing connection-string as string,there are still some characters that can't be used like single quotes ' or double quotes " , or a backslash \ as password.

@abhishekkumams abhishekkumams self-assigned this Mar 30, 2023
ayush3797
ayush3797 reviewed Mar 30, 2023
View reviewed changes
Comment thread src/Cli.Tests/InitTests.cs Outdated
@ayush3797 ayush3797 added the bug Something isn't working label Mar 30, 2023
@abhishekkumams abhishekkumams mentioned this pull request Mar 30, 2023
Closed
1 task
@ayush3797 ayush3797 added the cli label Mar 30, 2023
@abhishekkumams abhishekkumams added this to the Mar2023 milestone Mar 30, 2023
@severussundar severussundar enabled auto-merge (squash) March 30, 2023 10:02
@abhishekkumams abhishekkumams disabled auto-merge March 30, 2023 10:04
@abhishekkumams abhishekkumams enabled auto-merge (squash) March 30, 2023 10:06
@abhishekkumams abhishekkumams merged commit 3b7f5b2 into main Mar 30, 2023
@abhishekkumams abhishekkumams deleted the dev/abhishekkuma/fix_serialization_issue_for_config branch March 30, 2023 10:25
@Aniruddh25 Aniruddh25 mentioned this pull request Sep 7, 2023
Merged
1 task
Aniruddh25 added a commit that referenced this pull request Sep 7, 2023
@Aniruddh25 @abhishekkumams
Avoid escaping HTML sensitive characters when writing configuration t…
1b82fb8 
…o file (#1691)

## Why make this change?

- Closes #1687 

## What is this change?

- Set the Encoder for JsonSerializationOptions to be
[`JavaScriptEncoder.UnsafeRelaxedJsonEscaping`](https://learn.microsoft.com/en-us/dotnet/api/system.text.encodings.web.javascriptencoder.unsaferelaxedjsonescaping?view=net-7.0)
instead of the default encoder.
- Although [this
article](https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/character-encoding#serialize-all-characters)
mentions use this encoder with caution, since we are sure the file
contents will be deserialized as UTF-8 JSON, we should be safe to use
this encoder.
- We were using this encoder previously from the fix in #1386, but with
#1402, that original fix was essentially not being used since #1402
introduced usage of the new `GetJsonSerializerOptions()` function in
`RuntimeConfigLoader`.

## How was this tested?

- [X] Unit Tests. `TestSpecialCharactersInConnectionString` is moved to
the `ConfigGeneratorTests` since keeping it as part of `InitTests` with
the Verify framework wasn't really testing the scenario since we ignore
connection strings in the snapshots.
- This regression from #1402 was not caught because this test scenario
was effectively being skipped.
- Note, this test is modified to explicitly compare the expected and
actual json string from file since any usage of
JsonSerializer/JObject.Parse would require using the same encoder to
deserialize making the test itself run similar code which it is testing.
Parsing into JSON objects would treat `\u0027` and `'` as equal whereas
string comparison wont. Hence, comparison using string is necessary to
catch regressions here.

## Sample Request(s)

BEFORE:
`dab init --host-mode development --database-type mssql
--connection-string "@env('my-connection-string')"`

creates the configuration file as follows:

```
{
  "$schema": "https://github.com/Azure/data-api-builder/releases/download/v0.8.49/dab.draft.schema.json",
  "data-source": {
    "database-type": "mssql",
    "connection-string": "@env(\u0027my-connection-string\u0027)",
    "options": {
      "set-session-context": false
    }
  }
```
Note, the value of connection string.

AFTER: Same command creates the following file:
```
{
  "$schema": "https://github.com/Azure/data-api-builder/releases/download/v0.8.49/dab.draft.schema.json",
  "data-source": {
    "database-type": "mssql",
    "connection-string": "@env('my-connection-string')",
    "options": {
      "set-session-context": false
    }
  }
```

---------

Co-authored-by: Abhishek Kumar <[email protected]>
Aniruddh25 added a commit that referenced this pull request Sep 7, 2023
@Aniruddh25 @abhishekkumams
Avoid escaping HTML sensitive characters when writing configuration t…
4ae413d 
…o file (#1691)

## Why make this change?

- Closes #1687

## What is this change?

- Set the Encoder for JsonSerializationOptions to be
[`JavaScriptEncoder.UnsafeRelaxedJsonEscaping`](https://learn.microsoft.com/en-us/dotnet/api/system.text.encodings.web.javascriptencoder.unsaferelaxedjsonescaping?view=net-7.0)
instead of the default encoder.
- Although [this
article](https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/character-encoding#serialize-all-characters)
mentions use this encoder with caution, since we are sure the file
contents will be deserialized as UTF-8 JSON, we should be safe to use
this encoder.
- We were using this encoder previously from the fix in #1386, but with
#1402, that original fix was essentially not being used since #1402
introduced usage of the new `GetJsonSerializerOptions()` function in
`RuntimeConfigLoader`.

## How was this tested?

- [X] Unit Tests. `TestSpecialCharactersInConnectionString` is moved to
the `ConfigGeneratorTests` since keeping it as part of `InitTests` with
the Verify framework wasn't really testing the scenario since we ignore
connection strings in the snapshots.
- This regression from #1402 was not caught because this test scenario
was effectively being skipped.
- Note, this test is modified to explicitly compare the expected and
actual json string from file since any usage of
JsonSerializer/JObject.Parse would require using the same encoder to
deserialize making the test itself run similar code which it is testing.
Parsing into JSON objects would treat `\u0027` and `'` as equal whereas
string comparison wont. Hence, comparison using string is necessary to
catch regressions here.

## Sample Request(s)

BEFORE:
`dab init --host-mode development --database-type mssql
--connection-string "@env('my-connection-string')"`

creates the configuration file as follows:

```
{
  "$schema": "https://github.com/Azure/data-api-builder/releases/download/v0.8.49/dab.draft.schema.json",
  "data-source": {
    "database-type": "mssql",
    "connection-string": "@env(\u0027my-connection-string\u0027)",
    "options": {
      "set-session-context": false
    }
  }
```
Note, the value of connection string.

AFTER: Same command creates the following file:
```
{
  "$schema": "https://github.com/Azure/data-api-builder/releases/download/v0.8.49/dab.draft.schema.json",
  "data-source": {
    "database-type": "mssql",
    "connection-string": "@env('my-connection-string')",
    "options": {
      "set-session-context": false
    }
  }
```

---------

Co-authored-by: Abhishek Kumar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@severussundar severussundar severussundar approved these changes

@mbhaskar mbhaskar Awaiting requested review from mbhaskar

@Aniruddh25 Aniruddh25 Awaiting requested review from Aniruddh25 Aniruddh25 is a code owner

@JelteF JelteF Awaiting requested review from JelteF

@seantleonard seantleonard Awaiting requested review from seantleonard

@Mathos1432 Mathos1432 Awaiting requested review from Mathos1432

@gledis69 gledis69 Awaiting requested review from gledis69

@aaronburtle aaronburtle Awaiting requested review from aaronburtle aaronburtle is a code owner

@milismsft milismsft Awaiting requested review from milismsft

@junsu0ms junsu0ms Awaiting requested review from junsu0ms

@tarazou9 tarazou9 Awaiting requested review from tarazou9

@aaronpowell aaronpowell Awaiting requested review from aaronpowell

@ravishetye ravishetye Awaiting requested review from ravishetye

+1 more reviewer

@ayush3797 ayush3797 ayush3797 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@abhishekkumams abhishekkumams

Labels

bug Something isn't working cli

Projects

Data API builder
Status: Done

Milestone

Mar2023

Development

Successfully merging this pull request may close these issues.

[Bug]: When using dab init with a password with special characters, it escapes them and the DB cannot login

3 participants

@abhishekkumams @severussundar @ayush3797