Skip to content

fix(mssql): escape special characters in passwords#10437

Merged
gforsyth merged 2 commits intoibis-project:mainfrom
grieve54706:bugfix/special-character
Nov 11, 2024
Merged

fix(mssql): escape special characters in passwords#10437
gforsyth merged 2 commits intoibis-project:mainfrom
grieve54706:bugfix/special-character

Conversation

@grieve54706
Copy link
Copy Markdown
Contributor

Description of changes

I found the error

'IM002', '[IM002] [unixODBC][Driver Manager]Data source name not found and no default driver specified (0) (SQLDriverConnect)'

Because the password of mssql includes special characters like {R;3G1/8Al2AniRye that start with { or include ;.
It should be covered by { and } and replace } with}}.

Reference:
https://github.com/mkleehammer/pyodbc/wiki/Connecting-to-databases
https://stackoverflow.com/questions/78531086/pyodbc-connection-string-correctly-escaping-password-with-special-characters/78532507#78532507

@github-actions github-actions bot added the mssql The Microsoft SQL Server backend label Nov 5, 2024
@grieve54706
Copy link
Copy Markdown
Contributor Author

I have no idea how to make it be tested in the test suite. If you have any ideas, please let me know. Thanks.

@gforsyth
Copy link
Copy Markdown
Member

gforsyth commented Nov 5, 2024

Hey @grieve54706 -- thanks for putting this in. What would happen if a user passed in a properly escaped password directly? We should make sure that this works in that case, too.

As for testing, I would recommend a test just of the escaping function, ensuring that unescaped strings are properly escaped, and properly escaped strings are left untouched

@grieve54706
Copy link
Copy Markdown
Contributor Author

Hi @gforsyth, thanks for your point. I tested the normal password and the escaped password by testcontainers.

from testcontainers.mssql import SqlServerContainer
import pyodbc

def test_password_with_special_characters():
    passwords = [
        "1bis_Testing!",
        "{1bis_Testing!",
        "1bis_Testing!}",
        "{1bis_Testing!}",
        "1bis}Testing!",
        "{R;3G1/8Al2AniRye",
        "{R;3G1/8Al2AniRye}",
    ]
    for pwd in passwords:
        with SqlServerContainer(
            mssql_image,
            dialect="mssql+pyodbc",
            password=pwd,
        ) as mssql:
            pyodbc.connect(
                user=mssql.username,
                server=f"{mssql.get_container_host_ip()},{mssql.get_exposed_port(mssql.port)}",
                password=_escape_special_characters(pwd),
                database=mssql.dbname,
                driver="FreeTDS",
            )

def _escape_special_characters(value: str) -> str:
    return "{" + value.replace("}", "}}") + "}"

They are all good.
And I also add a test case for _escape_special_characters.

@github-actions github-actions bot added the tests Issues or PRs related to tests label Nov 6, 2024
@gforsyth
Copy link
Copy Markdown
Member

Nice, thanks for the update, @grieve54706 !

This looks good to me -- one last thing I'm unsure of here -- do left curly-braces also need to be escaped? e.g. should there also be a replace('{', '{{') bit?

@grieve54706
Copy link
Copy Markdown
Contributor Author

Thanks @gforsyth for your reply.
We don't need to escape {. We can find this message

occurrences of `{` inside a quoted value can be as-is, since the end of a quoted value is indicated by an unescaped `}`.

in wiki of pyodbc

Copy link
Copy Markdown
Member

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for working on this @grieve54706 , and for providing useful reference info for the pyodbc escaping conventions.

@gforsyth gforsyth changed the title fix(mssql): handle special characters fix(mssql): escape special characters in passwords Nov 11, 2024
@gforsyth gforsyth merged commit caf3632 into ibis-project:main Nov 11, 2024
@cpcloud cpcloud added this to the 10.0 milestone Nov 11, 2024
@grieve54706 grieve54706 deleted the bugfix/special-character branch November 12, 2024 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mssql The Microsoft SQL Server backend tests Issues or PRs related to tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants