Skip to content

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

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.

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
merged 2 commits into from
Nov 11, 2024

Conversation

grieve54706
Copy link
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
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
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
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
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
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
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
77 checks passed
@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