Skip to content

WIP: Add Testing Failcase for DSN Parsing #1159

New issue

Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “No 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? No Sign in to your account

Conversation

ranchodeluxe
Copy link

@ranchodeluxe ranchodeluxe commented Jun 7, 2024

Problem

Crunchydata Postgresql Operator creates passwords by default using the US-ASCII spec. Some of them with special characters in it including @, [, ], / that seem to cause problems for asyncpg host parsing even after running urllib.parse.quote on the password.

The error below shows it return the password assuming it's the port from an example app:

  File "/usr/local/lib/python3.8/site-packages/asyncpg/connect_utils.py", line 633, in _parse_connect_arguments
    addrs, params = _parse_connect_dsn_and_args(
  File "/usr/local/lib/python3.8/site-packages/asyncpg/connect_utils.py", line 288, in _parse_connect_dsn_and_args
    host, port = _parse_hostlist(dsn_hostspec, port, unquote=True)
  File "/usr/local/lib/python3.8/site-packages/asyncpg/connect_utils.py", line 229, in _parse_hostlist
    hostlist_ports.append(int(hostspec_port))
ValueError: invalid literal for int() with base 10: 'a2Vw:yk=)CdSis[fek]tW='

Test output (from this PR) running locally:

――――――――――――――――――――――――――――――――――――――――――――――――― TestConnectParams.test_connect_params ―――――――――――――――――――――――――――――――――――――――――――――――――
Traceback (most recent call last):
  File "/usr/local/Cellar/python@3.11/3.11.8/Frameworks/Python.framework/Versions/3.11/lib/python3.11/unittest/case.py", line 57, in testPartExecutor
    yield
  File "/usr/local/Cellar/python@3.11/3.11.8/Frameworks/Python.framework/Versions/3.11/lib/python3.11/unittest/case.py", line 623, in run
    self._callTestMethod(testMethod)
  File "/usr/local/Cellar/python@3.11/3.11.8/Frameworks/Python.framework/Versions/3.11/lib/python3.11/unittest/case.py", line 579, in _callTestMethod
    if method() is not None:
       ^^^^^^^^
  File "/Users/ranchodeluxe/apps/asyncpg/tests/test_connect.py", line 1186, in test_connect_params
    self.run_testcase(testcase)
  File "/Users/ranchodeluxe/apps/asyncpg/tests/test_connect.py", line 1106, in run_testcase
    addrs, params = connect_utils._parse_connect_dsn_and_args(
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ranchodeluxe/apps/asyncpg/asyncpg/connect_utils.py", line 296, in _parse_connect_dsn_and_args
    host, port = _parse_hostlist(dsn_hostspec, port, unquote=True)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ranchodeluxe/apps/asyncpg/asyncpg/connect_utils.py", line 231, in _parse_hostlist
    hostlist_ports.append(int(hostspec_port))
                          ^^^^^^^^^^^^^^^^^^
ValueError: invalid literal for int() with base 10: 'a2Vw:yk=)CdSis[fek]tW='

@ranchodeluxe
Copy link
Author

ranchodeluxe commented Jun 7, 2024

to get this test passing locally using the testing infrastructure I had to:

  1. remove these problematic character/ then urllib.parse.quote worked:

revised passing test

        {
            'name': 'dsn_bad_characters_maybe',
            'dsn': 'postgres://eoapi:a2Vw%3Ayk%3D%29CdSis%5Bfek%5DtW%3D@eoapi-primary.default.svc:5432/eoapi',
            'result': ([('eoapi-primary.default.svc', 5432)], {
                'user': 'eoapi',
                'password': 'a2Vw:yk=)CdSis[fek]tW=',
                'database': 'eoapi',
                'ssl': True,
                'sslmode': SSLMode.prefer
                ,
                'target_session_attrs': 'any'})
        },

@ranchodeluxe ranchodeluxe changed the title WIP: special characters in password throw off parsing credentials Add Testing Failcase for DSN Parsing Jun 7, 2024
@ranchodeluxe
Copy link
Author

I see we have a _validate_port_spec.

If passwords don't allow certain characters should we validate and raise too?

@ranchodeluxe ranchodeluxe changed the title Add Testing Failcase for DSN Parsing WIP:Add Testing Failcase for DSN Parsing Jun 7, 2024
@ranchodeluxe ranchodeluxe changed the title WIP:Add Testing Failcase for DSN Parsing WIP: Add Testing Failcase for DSN Parsing Jun 7, 2024
@ranchodeluxe
Copy link
Author

ranchodeluxe commented Jun 7, 2024

thanks @elprans : urllib.parse.quote_plus() works, test is up to date

will close this

$ pytest tests/test_connect.py -k TestConnectParams

 tests/test_connect.py ✓✓✓✓✓✓✓✓✓✓                                                                                         100% ██████████

Results (0.09s):
      10 passed
      41 deselected

ranchodeluxe added a commit to ranchodeluxe/stac-fastapi-pgstac that referenced this pull request Jun 7, 2024
`asyncpg` doesn't work well for all US-ASCII characters, we need to use `quote_plus` instead

MagicStack/asyncpg#1159
vincentsarago added a commit to stac-utils/stac-fastapi-pgstac that referenced this pull request Jun 27, 2024
* use quote_plus instead of quote

`asyncpg` doesn't work well for all US-ASCII characters, we need to use `quote_plus` instead

MagicStack/asyncpg#1159

* make it fail

* fix test

* one more quote

---------

Co-authored-by: ranchodeluxe <greg.corradini@gmail.com>
Co-authored-by: vincentsarago <vincent.sarago@gmail.com>
@elprans elprans closed this Oct 18, 2024
No Sign up for free to join this conversation on GitHub. Already have an account? No Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants