Skip to content

Escape values in the SQL Server ODBC connection string - #13

Open
maharanay22 wants to merge 1 commit into
mindsdb:mainfrom
maharanay22:fix-mssql-odbc-connection-string-escaping
Open

maharanay22 wants to merge 1 commit into
mindsdb:mainfrom
maharanay22:fix-mssql-odbc-connection-string-escaping

Conversation

@maharanay22

Copy link
Copy Markdown

Description

When the SQL Server handler connects through ODBC (use_odbc: true or a driver parameter), _connect_odbc() builds the connection string by putting each value between KEY= and ; with no escaping:

DRIVER={...};SERVER=host,port;DATABASE=...;UID=...;PWD=...

In an ODBC connection string, ; ends an attribute. So a password (or user or database name) that contains ;, { or } breaks the string. For example, with the password Xy;9}Encrypt=no, the driver gets PWD=Xy as the password plus a stray 9}Encrypt=no attribute. The login fails with a confusing error even though the credentials are right, and on Windows the SQL Server ODBC driver also reports "Invalid connection string attribute". A value can also add or override other attributes, such as Encrypt, which shouldn't be possible from a password field

This change escapes values the way the ODBC connection string rules require. A value that contains ;, { or }, or that starts or ends with a space, is wrapped in {...} and every } inside it is doubled. This covers SERVER, DATABASE, UID, PWD, Encrypt and TrustServerCertificate. DRIVER was already wrapped in braces, and now its } characters are doubled too. Simple values are written exactly as before, so existing connection strings don't change. connection_string_args is still appended as is, since it is meant to be a raw piece of the connection string

Fixes: no linked issue, because issues are turned off for this repository. The bug is described in full above

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Verification Process

To ensure the changes are working as expected:

  • Test Location: tests/unit/handlers/test_mssql.py, new test TestMSSQLHandlerODBC::test_odbc_connection_string_escapes_special_characters
  • Verification Steps: build the connection string for the password Xy;9}Encrypt=no (with pyodbc.connect mocked), then pass it to a real ODBC driver (the built-in Windows "SQL Server" driver) and an unreachable server, so the only difference between runs is how the connection string is parsed

Before (current main):

connection string: DRIVER={ODBC Driver 18 for SQL Server};SERVER=srv.example.net,1433;DATABASE=sales;UID=admin;PWD=Xy;9}Encrypt=no;Encrypt=yes
real driver manager, password='plain': OperationalError: ('08001', '[08001] [Microsoft][ODBC SQL Server Driver][DBNETLIB]SQL Server does not exist or access denied. (17) (SQLDriverConnect); [08001] [Microsoft][ODBC SQL Server Driver][DBNETLIB]ConnectionOpen (Connect()). (53)')
real driver manager, password='Xy;9}Encrypt=no': OperationalError: ('08001', '[08001] [Microsoft][ODBC SQL Server Driver][DBNETLIB]SQL Server does not exist or access denied. (17) (SQLDriverConnect); [08001] [Microsoft][ODBC SQL Server Driver][DBNETLIB]ConnectionOpen (Connect()). (53); [08001] [Microsoft][ODBC SQL Server Driver]Invalid connection string attribute (0)')

After:

connection string: DRIVER={ODBC Driver 18 for SQL Server};SERVER=srv.example.net,1433;DATABASE=sales;UID=admin;PWD={Xy;9}}Encrypt=no};Encrypt=yes
real driver manager, password='plain': OperationalError: ('08001', '[08001] [Microsoft][ODBC SQL Server Driver][DBNETLIB]SQL Server does not exist or access denied. (17) (SQLDriverConnect); [08001] [Microsoft][ODBC SQL Server Driver][DBNETLIB]ConnectionOpen (Connect()). (53)')
real driver manager, password='Xy;9}Encrypt=no': OperationalError: ('08001', '[08001] [Microsoft][ODBC SQL Server Driver][DBNETLIB]SQL Server does not exist or access denied. (17) (SQLDriverConnect); [08001] [Microsoft][ODBC SQL Server Driver][DBNETLIB]ConnectionOpen (Connect()). (53)')

With the fix, the special password is parsed as one value and gives the same result as a plain password. The server error is expected because there is no server on that port

Unit tests. The new test fails on main and passes with the fix, and the existing tests (including the ones that check plain values like PWD=example_pass) still pass:

# main
FAILED tests/unit/handlers/test_mssql.py::TestMSSQLHandlerODBC::test_odbc_connection_string_escapes_special_characters
E           - DRIVER={ODBC Driver 18 for SQL Server};SERVER=127.0.0.1,1433;DATABASE= sales ;UID={admin;PWD=Xy;9}Encrypt=no;Encrypt=yes
E           + DRIVER={ODBC Driver 18 for SQL Server};SERVER=127.0.0.1,1433;DATABASE={ sales };UID={{admin};PWD={Xy;9}}Encrypt=no};Encrypt=yes
1 failed, 31 passed, 16 subtests passed

# this branch
32 passed, 16 subtests passed

ruff check and ruff format --check (0.11.11) pass on the changed files. No dependencies changed

Additional Media:

  • I have attached a brief loom video or screenshots showcasing the new functionality or change.

I didn't record a video or take screenshots. The before and after terminal output above shows the change instead

Checklist:

  • My code follows the style guidelines(PEP 8) of MindsDB.
  • I have appropriately commented on my code, especially in complex areas.
  • Necessary documentation updates are either made or tracked in issues.
  • Relevant unit and integration tests are updated or added.

No documentation change is needed

I used an AI assistant to help investigate this and draft the change and tests, and I reviewed and tested it myself

The ODBC connection string was built by pasting the connection values
between "KEY=" and ";" with no escaping. A password such as
"Xy;9}Encrypt=no" ended the PWD attribute at the first ';', so the
driver got a truncated password and a stray "9}Encrypt=no" attribute
(the Windows SQL Server ODBC driver reports "Invalid connection string
attribute"), and a value could add or override attributes such as
Encrypt.

Wrap values that contain ';', '{' or '}', or that start or end with a
space, in braces and double every '}', following the ODBC connection
string rules. Simple values are written as before.

This branch has not been deployed

No deployments
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.

1 participant