jaraco / keyring

MIT License
1.26k stars 160 forks source link

Setting a blank password causes subsequent 'set' calls to fail #573

Closed AlexanderVatov closed 1 year ago

AlexanderVatov commented 2 years ago

Describe the bug If a password is set to '', any subsequent attempts to set it to another value result in the following error:

Traceback (most recent call last):
  File "/Library/Python/3.8/site-packages/keyring/backends/macOS/__init__.py", line 40, in set_password
    api.set_generic_password(self.keychain, service, username, password)
  File "/Library/Python/3.8/site-packages/keyring/backends/macOS/api.py", line 161, in set_generic_password
    Error.raise_for_status(status)
  File "/Library/Python/3.8/site-packages/keyring/backends/macOS/api.py", line 114, in raise_for_status
    raise cls(status, "Unknown Error")
keyring.backends.macOS.api.Error: (-25299, 'Unknown Error')
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "<input>", line 1, in <module>
  File "/Library/Python/3.8/site-packages/keyring/core.py", line 60, in set_password
    get_keyring().set_password(service_name, username, password)
  File "/Library/Python/3.8/site-packages/keyring/backends/macOS/__init__.py", line 44, in set_password
    raise PasswordSetError("Can't store password on keychain: " "{}".format(e))
keyring.errors.PasswordSetError: Can't store password on keychain: (-25299, 'Unknown Error')

To Reproduce

import keyring
keyring.set_password('test-keyring-123', 'test', 'a') # Succeeds
keyring.set_password('test-keyring-123', 'test', 'b') # Succeeds
keyring.set_password('test-keyring-123', 'test', '')  # Succeeds
keyring.set_password('test-keyring-123', 'test', 'a') # Fails

Expected behavior One would expect set_password not to be affected by previous calls of set_password.

Environment

Python 3.8.9 on macOS

$ pip list | grep keyring
keyring                 23.5.0
$ keyring --list-backends
keyring.backends.chainer.ChainerBackend (priority: -1)
keyring.backends.macOS.Keyring (priority: 5)
keyring.backends.fail.Keyring (priority: 0)

Additional context Getting the blank password works fine. Deleting it and then setting it to any non-blank password also works as a workaround.

IbrahimAH commented 1 year ago

@AlexanderVatov this occurs in https://github.com/jaraco/keyring/blob/977ed03677bb0602b91f005461ef3dddf01a49f6/keyring/backends/macOS/api.py#L150 as seen below

image

as you can see, the function find generic password fetches the password as ''. however, in python, empty strings resolve as False. The if statement therefore resolves as false and never deletes the password. error -25299 means that an item with this identifier already exists. Of course it throws this error, as the item was never deleted!

in your code it can be protected by ensuring set passwords are len() > 1, but looking into this issue now


looking into this further. keyring.get_password has strange behaviour for getting empty passwords too. i think by luck it ends up being '' (empty string). when it tries to call cfstr_to_str, the data it passes in is a None pointer of size 0. this resolves to b'' which is then decoded to '' at https://github.com/jaraco/keyring/blob/main/keyring/backends/macOS/api.py#L95

IbrahimAH commented 1 year ago

thanks for the super quick review and fix @jaraco, awesome stuff!

just one question, would it be worth removing the not_found_ok flag from find_generic_password now that nothing else uses it? https://github.com/jaraco/keyring/blob/main/keyring/backends/macOS/api.py#L130 and then youd also what to remove the 2 lines of code using that flag in the same function (142-143)