Skip to content

Avoid escaping HTML sensitive characters when writing configuration to file#1691

Merged
Aniruddh25 merged 7 commits intomainfrom
fixEncodedSerialization
Sep 7, 2023
Merged

Avoid escaping HTML sensitive characters when writing configuration to file#1691
Aniruddh25 merged 7 commits intomainfrom
fixEncodedSerialization

Conversation

@Aniruddh25
Copy link
Copy Markdown
Collaborator

@Aniruddh25 Aniruddh25 commented Sep 7, 2023

Why make this change?

What is this change?

How was this tested?

  • 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 Overhaul of config system #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
    }
  }

Comment thread src/Cli.Tests/ConfigGeneratorTests.cs
Copy link
Copy Markdown
Contributor

@seantleonard seantleonard left a comment

Choose a reason for hiding this comment

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

One question otherwise, lgtm!

@Aniruddh25 Aniruddh25 enabled auto-merge (squash) September 7, 2023 05:49
@Aniruddh25 Aniruddh25 merged commit 1b82fb8 into main Sep 7, 2023
@Aniruddh25 Aniruddh25 deleted the fixEncodedSerialization branch September 7, 2023 06:05
Aniruddh25 added a commit that referenced this pull request Sep 7, 2023
…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
- Cherry Picks #1691 into release/0.8

---------

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Escape characters in Config from CLI

3 participants