search

microsoft / go-sqlcmd

The new sqlcmd, CLI for SQL Server and Azure SQL (winget install sqlcmd / sqlcmd create mssql / sqlcmd open ads)
https://learn.microsoft.com/sql/tools/sqlcmd/go-sqlcmd-utility
MIT License
334 stars 58 forks source link

Add support for customized database name with --using #290

Closed apoorvdeshmukh closed 1 year ago

apoorvdeshmukh commented 1 year ago

This commit adds support for specifying database name with --using option by specifying ,DbName at the end of the URL This PR closes #216

Example without specifying customized database name, in which case Dbname defaults to the filename without extension

\git\go-sqlcmd>sqlcmd create mssql --accept-eula --using https://aka.ms/AdventureWorksLT.bak
Downloading mcr.microsoft.com/mssql/server:latest
Starting mcr.microsoft.com/mssql/server:latest
Created context "mssql" in "C:\Users\apdeshmukh\.sqlcmd\sqlconfig", configuring user account...
Disabled "sa" account (and rotated "sa" password). Creating user "apdeshmukh"Downloading AdventureWorksLT.bak from aka.ms
Restoring database AdventureWorksLT
Processed 840 pages for database 'AdventureWorksLT', file 'AdventureWorksLT2012_Data' on file 1.
Processed 2 pages for database 'AdventureWorksLT', file 'AdventureWorksLT2012_Log' on file 1.
Converting database 'AdventureWorksLT' from version 904 to the current version 957.
Database 'AdventureWorksLT' running the upgrade step from version 904 to version 905.
Database 'AdventureWorksLT' running the upgrade step from version 955 to version 956.
Database 'AdventureWorksLT' running the upgrade step from version 956 to version 957.
RESTORE DATABASE successfully processed 842 pages in 0.038 seconds (173.005 MB/sec).
Now ready for client connections on port 1435HINT:
  1. Open in Azure Data Studio: sqlcmd open ads
  2. Run a query:               sqlcmd query "SELECT @@version"  3. Start interactive session: sqlcmd query
  4. View sqlcmd configuration: sqlcmd config view
  5. See connection strings:    sqlcmd config connection-strings
  6. Remove:                    sqlcmd delete
\git\go-sqlcmd>.\sqlcmd.exe query
1> SELECT * FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_TYPE='BASE TABLE'
2> go
TABLE_CATALOG                                                                                                                    TABLE_SCHEMA                                                                                                                     TABLE_NAME
                                                                                                           TABLE_TYPE
-------------------------------------------------------------------------------------------------------------------------------- -------------------------------------------------------------------------------------------------------------------------------- -------------------------------------------------------------------------------------------------------------------------------- ----------AdventureWorksLT                                                                                                                 dbo                                                                                                                              ErrorLog
                                                                                                           BASE TABLE
AdventureWorksLT                                                                                                                 dbo                                                                                                                              BuildVersion
                                                                                                           BASE TABLE
AdventureWorksLT                                                                                                                 SalesLT                                                                                                                          Address
                                                                                                           BASE TABLE
AdventureWorksLT                                                                                                                 SalesLT                                                                                                                          Customer
                                                                                                           BASE TABLE
AdventureWorksLT                                                                                                                 SalesLT                                                                                                                          CustomerAddress                                                                                                                  BASE TABLE
AdventureWorksLT                                                                                                                 SalesLT                                                                                                                          Product
                                                                                                           BASE TABLE
AdventureWorksLT                                                                                                                 SalesLT                                                                                                                          ProductCategory                                                                                                                  BASE TABLE
AdventureWorksLT                                                                                                                 SalesLT                                                                                                                          ProductDescription                                                                                                               BASE TABLE
AdventureWorksLT                                                                                                                 SalesLT                                                                                                                          ProductModel
                                                                                                           BASE TABLE
AdventureWorksLT                                                                                                                 SalesLT                                                                                                                          ProductModelProductDescription                                                                                                   BASE TABLE
AdventureWorksLT                                                                                                                 SalesLT                                                                                                                          SalesOrderDetail                                                                                                                 BASE TABLE
AdventureWorksLT                                                                                                                 SalesLT                                                                                                                          SalesOrderHeader                                                                                                                 BASE TABLE
(12 rows affected)

Example with customized database name, in which case Dbname is set to user provided value

\git\go-sqlcmd>sqlcmd create mssql --accept-eula --using https://aka.ms/AdventureWorksLT.bak,WorldWideImporters
Downloading mcr.microsoft.com/mssql/server:latest
Starting mcr.microsoft.com/mssql/server:latest
Created context "mssql" in "C:\Users\apdeshmukh\.sqlcmd\sqlconfig", configuring user account...
Disabled "sa" account (and rotated "sa" password). Creating user "apdeshmukh"Downloading AdventureWorksLT.bak from aka.ms
Restoring database WorldWideImporters
Processed 840 pages for database 'WorldWideImporters', file 'AdventureWorksLT2012_Data' on file 1.
Processed 2 pages for database 'WorldWideImporters', file 'AdventureWorksLT2012_Log' on file 1.
Converting database 'WorldWideImporters' from version 904 to the current version 957.
Database 'WorldWideImporters' running the upgrade step from version 904 to version 905.
Database 'WorldWideImporters' running the upgrade step from version 954 to version 955.
Database 'WorldWideImporters' running the upgrade step from version 955 to version 956.
Database 'WorldWideImporters' running the upgrade step from version 956 to version 957.
RESTORE DATABASE successfully processed 842 pages in 0.038 seconds (173.005 MB/sec).
Now ready for client connections on port 1435HINT:
  1. Open in Azure Data Studio: sqlcmd open ads
  2. Run a query:               sqlcmd query "SELECT @@version"  3. Start interactive session: sqlcmd query
  4. View sqlcmd configuration: sqlcmd config view
  5. See connection strings:    sqlcmd config connection-strings
  6. Remove:                    sqlcmd delete
\git\go-sqlcmd>.\sqlcmd.exe query
1> select db_name()
2> go
--------------------------------------------------------------------------------------------------------------------------------WorldWideImporters
(1 row affected)
1> SELECT * FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_TYPE='BASE TABLE'
2> go
TABLE_CATALOG                                                                                                                    TABLE_SCHEMA                                                                                                                     TABLE_NAME                                                                                                                       TABLE_TYPE
-------------------------------------------------------------------------------------------------------------------------------- -------------------------------------------------------------------------------------------------------------------------------- -------------------------------------------------------------------------------------------------------------------------------- ----------WorldWideImporters                                                                                                               dbo                                                                                                                              ErrorLog                                                                                                                         BASE TABLE
WorldWideImporters                                                                                                               dbo                                                                                                                              BuildVersion                                                                                                                     BASE TABLE
WorldWideImporters                                                                                                               SalesLT                                                                                                                          Address                                                                                                                          BASE TABLE
WorldWideImporters                                                                                                               SalesLT                                                                                                                          Customer                                                                                                                         BASE TABLE
WorldWideImporters                                                                                                               SalesLT                                                                                                                          CustomerAddress                                                                                                                  BASE TABLE
WorldWideImporters                                                                                                               SalesLT                                                                                                                          Product                                                                                                                          BASE TABLE
WorldWideImporters                                                                                                               SalesLT                                                                                                                          ProductCategory                                                                                                                  BASE TABLE
WorldWideImporters                                                                                                               SalesLT                                                                                                                          ProductDescription                                                                                                               BASE TABLE
WorldWideImporters                                                                                                               SalesLT                                                                                                                          ProductModel                                                                                                                     BASE TABLE
WorldWideImporters                                                                                                               SalesLT                                                                                                                          ProductModelProductDescription                                                                                                   BASE TABLE
WorldWideImporters                                                                                                               SalesLT                                                                                                                          SalesOrderDetail                                                                                                                 BASE TABLE
WorldWideImporters                                                                                                               SalesLT                                                                                                                          SalesOrderHeader                                                                                                                 BASE TABLE
(12 rows affected)
shueybubbles commented 1 year ago

please add tests. There's a pretty big matrix of input to cover here and the tests need to show what's valid and what's not. We need to handle edge cases that could lead to sql injection if we don't escape the name, etc. Can I pass a local file path like --using "c:\path with a space and a comm,a\path","db space and co,mma" ?

shueybubbles commented 1 year ago

do we want to allow the user to provide the db name as a bracketed identifier? eg 

--using https://someurl/somefile.bak,[my database]

In reply to: 1460392606

shueybubbles commented 1 year ago

if there's a space in the name or the path will the user have to surround the entire parameter with quotes?

In reply to: 1460405555

stuartpa commented 1 year ago

Yes, add an example with --using with a space in the db name (it works, we tested it)

In reply to: 1460405555

shueybubbles commented 1 year ago 
  String:        &c.usingDatabaseUrl,

Is there a way we could change this flag type to a StringSlice and restrict it to only being used once? That could let cobra do the heavy lifting of delimiting the components by the comma, and our code could potentially allow more parameters in the future.

I think the default behavior of StringSlice is to allow the same flag to be used multiple times like

-u f1,f2 -u f3

Closed

Refers to: cmd/modern/root/install/mssql-base.go:192 in 246bd0e. [](commit_id = 246bd0ee6d8d80eddd658fa719dd6c91d26f6859, deletion_comment = False)

apoorvdeshmukh commented 1 year ago

please add tests. There's a pretty big matrix of input to cover here and the tests need to show what's valid and what's not. We need to handle edge cases that could lead to sql injection if we don't escape the name, etc. Can I pass a local file path like --using "c:\path with a space and a comm,a\path","db space and co,mma" ?

We do not support filesystem paths at the moment. Only http and https. But I will add tests to check what happens if such input is intentionally passed.

apoorvdeshmukh commented 1 year ago

do we want to allow the user to provide the db name as a bracketed identifier? eg

--using https://someurl/somefile.bak,[my database]

In reply to: 1460392606

I checked with @stuartpa and for now we will only support URL,DbName where ,DbName is optional. If there are spaces or commas in the DbName then we would expect user to surround the entire argument of --using with "" E.g. --using "https://aka.ms/AdventureWorksLT.bak,My Db"

shueybubbles commented 1 year ago

per our meeting - make sure you bracket-escape the name before putting it in a SQL query "https:/server/file.bak,db [name]" ==> [db [name]]]

see https://github.com/microsoft/sqlmanagementobjects/blob/bf460b75d413fb3d366aa59fc5d48f3c01cccdd5/src/Microsoft/SqlServer/Management/Smo/SqlSmoObject.cs#L3494

In reply to: 1461812698

shueybubbles commented 1 year ago

SET @sql = 'RESTORE DATABASE [%s] FROM DISK = ''%s/%s'' WITH '

if databaseName has a ] in it this query becomes a vector for sql injection. Similarly if any of the other parameters have a ' in their value this query will go awry.

Either use a properly parameterized query (go-mssqldb supports parameterized queries) or escape everything appropriately. It's even more complicated here because it's using dynamic SQL. Any values you are adding that have a ' in them may need to be double escaped. Look at the SMO code for guidance/examples.

Refers to: cmd/modern/root/install/mssql-base.go:530 in 7a37b69. [](commit_id = 7a37b697417c615cee774b5bc6d61b9e0a2ea236, deletion_comment = False)

stuartpa commented 1 year ago

We want to incrementally improve the quality of the experience here. There is a quality issue with the experience right now, in that folks don't want to be stuck with the name of the .bak file as the database name. It would improve the experience if we allow a database name to be specified. In the limit we should support a database name that covers the entire input space for T/SQL identifiers, but we don't have to do that in a single PR. It is fine in this PR to limit the input space for the database name so SQL injection is not possible, and enter an issue to widen the input space. What we don't want to see is we end up reducing quality by not thinking through the complete solution for all T/SQL identifier input space in one PR.

shueybubbles commented 1 year ago

We should be able to write a functional test for this in the ADO pipeline. ADO hosted agents have docker on them, so the pipeline should be able to use the sqlcmd it builds to run some permutations on sqlcmd create --using and verify the E2E scenario. The unit test is insufficient to prove we are doing the right thing with the user input all the way through the download and rename process.

In reply to: 1462251401

apoorvdeshmukh commented 1 year ago

We should be able to write a functional test for this in the ADO pipeline. ADO hosted agents have docker on them, so the pipeline should be able to use the sqlcmd it builds to run some permutations on sqlcmd create --using and verify the E2E scenario. The unit test is insufficient to prove we are doing the right thing with the user input all the way through the download and rename process.

In reply to: 1462251401

Created #306 to add functional tests for E2E testing scenarios