Skip to content
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

cert parameter does not accept a single pathlib.Path object as argument #5936

Open
0xallie opened this issue Sep 16, 2021 · 7 comments · May be fixed by #6454
Open

cert parameter does not accept a single pathlib.Path object as argument #5936

0xallie opened this issue Sep 16, 2021 · 7 comments · May be fixed by #6454

Comments

@0xallie
Copy link

0xallie commented Sep 16, 2021

requests seems to have special handling for when the cert parameter is a single str, but does not extend the same special handling to pathlib.Path, meaning currently you either have to do cert=str(path) or cert=(path, path).

Expected Result

Path objects should be handled just like str.

Actual Result

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/nyuszika7h/test/.venv/lib/python3.9/site-packages/requests/api.py", line 75, in get
    return request('get', url, params=params, **kwargs)
  File "/home/nyuszika7h/test/.venv/lib/python3.9/site-packages/requests/api.py", line 61, in request
    return session.request(method=method, url=url, **kwargs)
  File "/home/nyuszika7h/test/.venv/lib/python3.9/site-packages/requests/sessions.py", line 542, in request
    resp = self.send(prep, **send_kwargs)
  File "/home/nyuszika7h/test/.venv/lib/python3.9/site-packages/requests/sessions.py", line 655, in send
    r = adapter.send(request, **kwargs)
  File "/home/nyuszika7h/test/.venv/lib/python3.9/site-packages/requests/adapters.py", line 416, in send
    self.cert_verify(conn, request.url, verify, cert)
  File "/home/nyuszika7h/test/.venv/lib/python3.9/site-packages/requests/adapters.py", line 243, in cert_verify
    conn.cert_file = cert[0]
TypeError: 'PosixPath' object is not subscriptable

Reproduction Steps

from pathlib import Path

import requests

requests.get('https://example.com', cert=Path('cert.pem'))

System Information

$ python -m requests.help
{
  "chardet": {
    "version": null
  },
  "charset_normalizer": {
    "version": "2.0.5"
  },
  "cryptography": {
    "version": ""
  },
  "idna": {
    "version": "3.2"
  },
  "implementation": {
    "name": "CPython",
    "version": "3.9.2"
  },
  "platform": {
    "release": "5.10.0-8-amd64",
    "system": "Linux"
  },
  "pyOpenSSL": {
    "openssl_version": "",
    "version": null
  },
  "requests": {
    "version": "2.26.0"
  },
  "system_ssl": {
    "version": "101010bf"
  },
  "urllib3": {
    "version": "1.26.6"
  },
  "using_charset_normalizer": true,
  "using_pyopenssl": false
}
@0xallie 0xallie changed the title cert parameter does not accept pathlib.Path objects cert parameter does not accept a single pathlib.Path object as argument Sep 16, 2021
@nateprewitt
Copy link
Member

Hi @nyuszika7h, this seems like a reasonable request but is going to be somewhat difficult to implement prior to dropping 2.7 support. We don't intend to add backports as a dependency, so we'll leave this a todo for when the project finally moves to only Python 3 support. Thanks!

@sigmavirus24
Copy link
Contributor

I don't think this is entirely reasonable as pathlib is garabage. Expecting it to just work in places clearly documented as accepting either str or Tuple[str, str] isn't reasonable.

@0xallie
Copy link
Author

0xallie commented Sep 17, 2021

How exactly is it "garbage"? It has existed since Python 3.4 and is becoming a standard, take it up with the Python maintainers, not me. If you're developing a standalone program you have every right to not use pathlib if you don't like it, but developers of a major library used by thousands of other developers should consider supporting it regardless of their personal views. It doesn't really add any undue maintenance burden either.

tuple[Path, Path] already works fine with requests today, and if type hints are a concern, it can be done with an union type which is the officially recommended way if you need backwards compatibility with str. (In an ideal world, maybe we'd only accept Path objects, but that's not quite feasible yet – backwards compatibility is a concern and I would at least like p'' literals in that case – so a simple type alias can be created instead to keep function signatures simple.)

@steveberdy
Copy link
Contributor

It makes sense that it would be a good idea to allow some sort of path-like object, even without specifying examples of path-like object types with type hinting. If you look at https://docs.python.org/3/glossary.html#term-path-like-object and https://docs.python.org/3/library/os.html#os.PathLike, it shouldn't be unreasonable to check if an object is path-like.

The docs do mention here that os.PathLike is new in version 3.6. So, to preserve backwards compatibility we can check for hasattr(cert, '__fspath__') rather than checking isinstance(cert, os.PathLike). I recommend doing a similar check for if the object is subscriptable hasattr(cert, '__getitem__'), to prevent subscriptable-related TypeErrors that could otherwise be handled.

We can replace...

if cert:
if not isinstance(cert, basestring):
conn.cert_file = cert[0]
conn.key_file = cert[1]
else:
conn.cert_file = cert
conn.key_file = None

With...

if cert:
    if not isinstance(cert, basestring):
        # a subscriptable object
        if hasattr(cert, '__getitem__'):
            conn.cert_file = cert[0]
            conn.key_file = cert[1]
        elif hasattr(cert, '__fspath__'):
            # a path-like object implements __fspath__
            # see https://docs.python.org/3/library/os.html#os.PathLike
            conn.cert_file = cert.__fspath__()
        else:
            conn.cert_file = str(cert)
    else:
        conn.cert_file = cert
        conn.key_file = None

Though I've never used pathlib yet, if it is being mentioned in Python's stdlib docs and is becoming more standard, it may be good to start looking into backwards-compatible ways to support it, like in the example above. We can handle more types this way, and if errors occur, they should be because the string provided was an invalid path to a certificate, rather than because the type provided wasn't acceptable.

If os.path is allowing path-like objects, Requests should, too

@0xallie
Copy link
Author

0xallie commented Oct 1, 2021

While requests still supports Python 2, it's probably better to use getattr() rather than hasattr() as the latter catches all exceptions, not just AttributeError. But hopefully in the next major release that won't be a problem anymore as Python 2 is EOL.

@brianpatman
Copy link

Hi all,

Just combing through issues in some of my favorite python libs looking for ways to contribute.

Seems y'all have dropped support for Python 2 in the requests library, so this might be a great thing to implement at this time, unless there's something I'm not understanding fully. I'm looking for issues good for a GitHub newbie to tackle. And, while I would love to help with this, it seems @steveberdy has a solution in the notes above, so I wouldn't want to steal thunder by patching in code they've written.

Sorry for any misunderstandings and hope this helps.
Thanks.

@steveberdy
Copy link
Contributor

This issue is still ongoing with v2.28.2. I think I'll make a PR since this doesn't exactly break the rules for the indefinite feature-freeze.

@steveberdy steveberdy linked a pull request May 11, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants