This needs to have the ]'s escaped

So what I'm talking about is that you need to escape ]'s in the DB name by doubling them. This is because currently if you try to run that query against a DB that has a ] in its name then you'd end up with a query like this (assuming you have a DB named my]db)

SELECT name FROM [my]db].sys.schemas

which would break because it would see the ] as the end of the object name and thus have invalid syntax.

So the fix is to do something like this

const query = this.databaseDropdown.value ? 'SELECT name FROM [${this.databaseDropdown.value.replace(/]/g, ']]')}].sys.schemas' : 'SELECT name FROM sys.schemas';

So that you end up with a query like this :

SELECT name FROM [my]]db].sys.schemas

which will then run correctly.

(This also safeguards against the SQL injection issues that were mentioned in the previous PR)

I am more concerned about spaces in databases names. If they have a bracket in their name I think they have more issues to resolve. I also don't see how SQL injection would play since this is just populating a drop down which is then selected. Unless a user has the ability to adjust the drop down I'm not seeing how they would be able to inject anything.

Bracket-quoting the name is correct, I agree.

But we do require all queries that use values outside of our control to be correctly escaped. And database names fall under that bucket of "out of our control" since a server could have a database with a malicious name (which - while unlikely - is possible).

Unless that is being done correctly we can't take this fix.

I'm not quite sure how you would escape this, it's not like a quote character. The database names are coming directly from the database connection above which is in control, thus this would also be in control. This isn't a webpage, you can't edit the form and post to your app. Unless I'm missing something here.

I gave you the code above to escape it (you escape them by replacing every ] with ]]) :

const query = this.databaseDropdown.value ? 'SELECT name FROM [${this.databaseDropdown.value.replace(/]/g, ']]')}].sys.schemas' : 'SELECT name FROM sys.schemas';

Here's a possible situation that we are preventing with this. Some bad actor has somehow gotten access to create DBs on the server but not anything else. They create a DB with this name

master].sys.tables; CREATE LOGIN foo WITH PASSWORD='12345' --

Then another person comes along with more permissions - such as being able to create a login. They open the dialog and your code above will create this query :

SELECT name FROM [master].sys.tables; CREATE LOGIN foo WITH PASSWORD='12345' --].sys.schemas

(Note the rest of the query is commented out because of the --)

This query is ran and now this bad actor has silently caused this other user to create a login on the DB. But you can run any query. This is extremely dangerous and as said - while unlikely to actually happen - is something we can't allow to be a possibility.

You're checking for this.databaseDropdown.value to be truthy here but you've already used it in on line 273 - if it was undefined it would have failed there.

Also I just looked at it myself and the dropdown value is a string | CategoryValue - and specifically in this instance we're populating it with CategoryValue objects. Did you test this yourself? Because I'm surprised it worked with the code you had. Calling toString() on that is just going to return [object Object]

Something like this should work, but you should be testing this yourself too to make sure it works as expected.

const escapedQuotedDb = this.databaseDropdown.value ? `[${(<azdata.CategoryValue>this.databaseDropdown.value).name.replace(/]/g, ']]')}` : '';
const query = `SELECT name FROM ${escapedQuotedDb}.sys.schemas`;

The period should actually also be in the previous part (we don't want to include the period if there isn't a DB name selected)

Sorry about that. I overlooked that. Updated.

It looks like you're using single quotes ( ' ) here instead of backticks ( ` ). You need to use backticks in order for string interpolation to work correctly (otherwise you'll just end up with a string that has ${...} in it)

Also you dropped bracket-quoting the name at some point. I tested this locally and it should work :

		const escapedQuotedDb = this.databaseDropdown.value ? `[${(<azdata.CategoryValue>this.databaseDropdown.value).name.replace(/]/g, ']]')}].` : '';
		const query = `SELECT name FROM ${escapedQuotedDb}sys.schemas`;