Skip to content

Escape double quotes in connection string #230

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zijchen merged 7 commits into from
Jun 27, 2024
Merged

Escape double quotes in connection string #230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d51efb4
Escape double quotes in connection string
zijchen Jun 24, 2024
7093dd4
Build main.js
zijchen Jun 25, 2024
4c75a0f
Double up double quotes
zijchen Jun 25, 2024
84ae62b
Fix tests
zijchen Jun 25, 2024
8edd06c
Add some tests
zijchen Jun 25, 2024
34ef367
Merge branch 'master' of https://github.com/Azure/sql-action into zij…
zijchen Jun 26, 2024
77bd254
Add comment
zijchen Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 32 additions & 4 deletions __tests__/AzureSqlAction.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@ describe('AzureSqlAction tests', () => {
expect(execSpy).toHaveBeenCalledTimes(1);

if (actionName == 'DriftReport') {
expect(execSpy).toHaveBeenCalledWith(`"SqlPackage.exe" /Action:${actionName} /TargetConnectionString:"${inputs.connectionConfig.ConnectionString}" ${sqlpackageArgs}`);
expect(execSpy).toHaveBeenCalledWith(`"SqlPackage.exe" /Action:${actionName} /TargetConnectionString:"${inputs.connectionConfig.EscapedConnectionString}" ${sqlpackageArgs}`);
} else {
expect(execSpy).toHaveBeenCalledWith(`"SqlPackage.exe" /Action:${actionName} /TargetConnectionString:"${inputs.connectionConfig.ConnectionString}" /SourceFile:"${inputs.filePath}" ${sqlpackageArgs}`);
expect(execSpy).toHaveBeenCalledWith(`"SqlPackage.exe" /Action:${actionName} /TargetConnectionString:"${inputs.connectionConfig.EscapedConnectionString}" /SourceFile:"${inputs.filePath}" ${sqlpackageArgs}`);
}
});
});
Expand Down Expand Up @@ -142,9 +142,9 @@ describe('AzureSqlAction tests', () => {
expect(execSpy).toHaveBeenCalledTimes(2);
expect(execSpy).toHaveBeenNthCalledWith(1, `dotnet build "./TestProject.sqlproj" -p:NetCoreBuild=true --verbose --test "test value"`);
if (actionName === 'DriftReport') {
expect(execSpy).toHaveBeenNthCalledWith(2, `"SqlPackage.exe" /Action:${actionName} /TargetConnectionString:"${inputs.connectionConfig.ConnectionString}" ${sqlpackageArgs}`);
expect(execSpy).toHaveBeenNthCalledWith(2, `"SqlPackage.exe" /Action:${actionName} /TargetConnectionString:"${inputs.connectionConfig.EscapedConnectionString}" ${sqlpackageArgs}`);
} else {
expect(execSpy).toHaveBeenNthCalledWith(2, `"SqlPackage.exe" /Action:${actionName} /TargetConnectionString:"${inputs.connectionConfig.ConnectionString}" /SourceFile:"${expectedDacpac}" ${sqlpackageArgs}`);
expect(execSpy).toHaveBeenNthCalledWith(2, `"SqlPackage.exe" /Action:${actionName} /TargetConnectionString:"${inputs.connectionConfig.EscapedConnectionString}" /SourceFile:"${expectedDacpac}" ${sqlpackageArgs}`);
}
});
});
Expand Down Expand Up @@ -182,6 +182,34 @@ describe('AzureSqlAction tests', () => {
});
});

describe('validate connection string escaping in sqlpackage commands', () => {
const inputs = [
['Basic connection string', 'Server=testServer;Database=testDB;Authentication=Active Directory Password;User Id=testUser;Password=placeholder', 'Server=testServer;Database=testDB;Authentication=Active Directory Password;User Id=testUser;Password=placeholder;'],
['Authentication at the end', 'Server=testServer;Database=testDB;User Id=testUser;Password=placeholder;Authentication=Active Directory Password', 'Server=testServer;Database=testDB;User Id=testUser;Password=placeholder;Authentication=Active Directory Password;'],
['Authentication with double quotes', 'Server=testServer;Database=testDB;Authentication="Active Directory Password";User Id=testUser;Password=placeholder', 'Server=testServer;Database=testDB;Authentication=\"\"Active Directory Password\"\";User Id=testUser;Password=placeholder;'],
['Authentication with double quotes at the end', 'Server=testServer;Database=testDB;User Id=testUser;Password=placeholder;Authentication="Active Directory Password"', 'Server=testServer;Database=testDB;User Id=testUser;Password=placeholder;Authentication=\"\"Active Directory Password\"\";'],
['Authentication with single quotes', 'Server=testServer;Database=testDB;Authentication=\'Active Directory Password\';User Id=testUser;Password=placeholder', 'Server=testServer;Database=testDB;Authentication=\'Active Directory Password\';User Id=testUser;Password=placeholder;'],
['Authentication with single quotes at the end', 'Server=testServer;Database=testDB;User Id=testUser;Password=placeholder;Authentication=\'Active Directory Password\'', 'Server=testServer;Database=testDB;User Id=testUser;Password=placeholder;Authentication=\'Active Directory Password\';'],
['Password enclosed with double quotes', `Server=test1.database.windows.net;User Id=user;Password="placeholder'=placeholder''c;123";Initial catalog=testdb`, `Server=test1.database.windows.net;User Id=user;Password=""placeholder'=placeholder''c;123"";Initial catalog=testdb;`],
['Password enclosed with single quotes', `Server=test1.database.windows.net;User Id=user;Password='placeholder;1""2"placeholder=33';Initial catalog=testdb`, `Server=test1.database.windows.net;User Id=user;Password='placeholder;1""2"placeholder=33';Initial catalog=testdb;`],
['Password with double quotes enclosed with double quotes', `Server=test1.database.windows.net;User Id=user;Password="placeholder;1""2""placeholder(012j^72''placeholder;')'=33";Initial catalog=testdb`, `Server=test1.database.windows.net;User Id=user;Password=""placeholder;1""2""placeholder(012j^72''placeholder;')'=33"";Initial catalog=testdb;`],
['Password with single quotes enclosed with single quotes', `Server=test1.database.windows.net;User Id=user;Password='placeholder""c;1''2''"''placeholder("0""12j^72''placeholder;'')''=33';Initial catalog=testdb`, `Server=test1.database.windows.net;User Id=user;Password='placeholder""c;1''2''"''placeholder("0""12j^72''placeholder;'')''=33';Initial catalog=testdb;`],
];

it.each(inputs)('%s', async (testName, inputConnectionString, escapedConnectionString) => {
let inputs = getInputs(ActionType.DacpacAction, inputConnectionString) as IDacpacActionInputs;
let action = new AzureSqlAction(inputs);

let getSqlPackagePathSpy = jest.spyOn(AzureSqlActionHelper, 'getSqlPackagePath').mockResolvedValue('SqlPackage.exe');
let execSpy = jest.spyOn(exec, 'exec').mockResolvedValue(0);

await action.execute();

expect(getSqlPackagePathSpy).toHaveBeenCalled();
expect(execSpy).toHaveBeenCalledWith(`"SqlPackage.exe" /Action:Publish /TargetConnectionString:"${escapedConnectionString}" /SourceFile:"./TestPackage.dacpac" /TargetTimeout:20`);
});
});

/**
* Gets test inputs used by the SQL action based on actionType.
* @param actionType The action type used for testing
Expand Down
5 changes: 2 additions & 3 deletions __tests__/SqlConnectionConfig.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ describe('SqlConnectionConfig tests', () => {
it.each(validConnectionStrings)('Input `%s` %s', (connectionStringInput, testDescription, passwordOutput) => {
const connectionString = new SqlConnectionConfig(connectionStringInput);

expect(connectionString.ConnectionString).toMatch(connectionStringInput);
expect(connectionString.Password).toMatch(passwordOutput);
expect(connectionString.UserId).toMatch(`user`);
expect(connectionString.Database).toMatch('testdb');
Expand Down Expand Up @@ -83,7 +82,7 @@ describe('SqlConnectionConfig tests', () => {

expect(config.Server).toMatch('test1.database.windows.net');
expect(config.Database).toMatch('testdb');
expect(config.ConnectionString).toMatch(connectionStringInput);
expect(config.EscapedConnectionString.replace(/""/g, '"')).toMatch(connectionStringInput);
expect(config.FormattedAuthentication ?? '').toMatch(expectedAuthType);
switch (expectedAuthType) {
case '':
Expand Down Expand Up @@ -118,7 +117,7 @@ describe('SqlConnectionConfig tests', () => {
expect(config.Server).toMatch(expectedServerName);
expect(config.Port?.toString() || '').toMatch(expectedPortNumber);
expect(config.Database).toMatch('testdb');
expect(config.ConnectionString).toMatch(connectionStringInput);
expect(config.EscapedConnectionString).toMatch(connectionStringInput);
});
});
})
2 changes: 1 addition & 1 deletion lib/main.js
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions src/AzureSqlAction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,10 +128,10 @@ export default class AzureSqlAction {
case SqlPackageAction.Publish:
case SqlPackageAction.Script:
case SqlPackageAction.DeployReport:
args += `/Action:${SqlPackageAction[inputs.sqlpackageAction]} /TargetConnectionString:"${inputs.connectionConfig.ConnectionString}" /SourceFile:"${inputs.filePath}"`;
args += `/Action:${SqlPackageAction[inputs.sqlpackageAction]} /TargetConnectionString:"${inputs.connectionConfig.EscapedConnectionString}" /SourceFile:"${inputs.filePath}"`;
break;
case SqlPackageAction.DriftReport:
args += `/Action:${SqlPackageAction[inputs.sqlpackageAction]} /TargetConnectionString:"${inputs.connectionConfig.ConnectionString}"`;
args += `/Action:${SqlPackageAction[inputs.sqlpackageAction]} /TargetConnectionString:"${inputs.connectionConfig.EscapedConnectionString}"`;
break;

default:
Expand Down
1 change: 1 addition & 0 deletions src/Constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
export default class Constants {
static readonly ipv4MatchPattern = /\b\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}\b/;
static readonly connectionStringTester = /^[;\s]*([\w\s]+=(?:('[^']*(''[^']*)*')|("[^"]*(""[^"]*)*")|((?!['"])[^;]*)))(;[;\s]*([\w\s]+=(?:('[^']*(''[^']*)*')|("[^"]*(""[^"]*)*")|((?!['"])[^;]*))))*[;\s]*$/;
static readonly connectionStringParserRegex = /(?<key>[\w\s]+)=(?<val>('[^']*(''[^']*)*')|("[^"]*(""[^"]*)*")|((?!['"])[^;]*))/g;

static readonly dacpacExtension = ".dacpac";
static readonly sqlFileExtension = ".sql";
Expand Down
34 changes: 28 additions & 6 deletions src/SqlConnectionConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,18 @@ import Constants from './Constants';

export default class SqlConnectionConfig {
private _parsedConnectionString: Record<string, string | number | boolean>;
private _connectionString: string;
private _rawConnectionString: string;

constructor(connectionString: string) {
this._validateConnectionString(connectionString);

this._connectionString = connectionString;
this._rawConnectionString = connectionString;
this._parsedConnectionString = parseSqlConnectionString(connectionString, true, true);

this._maskSecrets();
this._validateconfig();
}

public get ConnectionString(): string {
return this._connectionString;
}

public get Server(): string {
const server = this._parsedConnectionString['data source'] as string;
if (server?.includes(',')) {
Expand Down Expand Up @@ -56,6 +52,32 @@ export default class SqlConnectionConfig {
return auth?.replace(/\s/g, '').toLowerCase();
}

/**
* Returns the connection string escaped by double quotes.
*/
public get EscapedConnectionString() : string {
Copy link
Contributor

@chlafreniere chlafreniere Jun 26, 2024

Choose a reason for hiding this comment

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

Can we do effectively the same thing with _parsedConnectionString and iterate through keys/values so that we don't need to parse a second time?

Copy link
Contributor Author

@zijchen zijchen Jun 26, 2024

Choose a reason for hiding this comment

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

I did think about that but the issue with the key/value pair is the keys may have changed (e.g. Server becomes 'data source') and we'd still need to escape each value again. Since the full connection string is only used by sqlpackage, I wanted it to be as close to the one user supplied without changing it too much.

Copy link
Contributor

@chlafreniere chlafreniere Jun 26, 2024

Choose a reason for hiding this comment

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

Ahh, I see. Ok, I can live with that for sure. Can you put a comment here describing this decision?

Copy link
Contributor Author

@zijchen zijchen Jun 26, 2024

Choose a reason for hiding this comment

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

Added a comment

let result = '';

// Isolate all the key value pairs from the raw connection string
// Using the raw connection string instead of the parsed one to keep it as close to the original as possible
const matches = Array.from(this._rawConnectionString.matchAll(Constants.connectionStringParserRegex));
for (const match of matches) {
if (match.groups) {
const key = match.groups.key.trim();
let val = match.groups.val.trim();

// If the value is enclosed in double quotes, escape the double quotes
if (val.startsWith('"') && val.endsWith('"')) {
val = '""' + val.slice(1, -1) + '""';
}

result += `${key}=${val};`;
}
}

return result;
}

/**
* The basic format of a connection string includes a series of keyword/value pairs separated by semicolons.
* The equal sign (=) connects each keyword and its value. (Ex: Key1=Val1;Key2=Val2)
Expand Down