From 498bfcd31d5644e561d9ae8dd7fffd4f104e8141 Mon Sep 17 00:00:00 2001 From: Maximiliano Sandoval Date: Wed, 6 Mar 2024 22:26:07 +0100 Subject: [PATCH 1/2] entry: Do not set protected=True on setters Setting a property would call _set_string_field which by default would set the protected status of the attribute as "True". In order to verify in tests if a property is protected we add a private method _is_property_protected which shares the code with is_custom_property_protected. Fixes: https://github.com/libkeepass/pykeepass/issues/376 --- pykeepass/entry.py | 31 +++++++++++++++++++++++++++---- tests/tests.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 4 deletions(-) diff --git a/pykeepass/entry.py b/pykeepass/entry.py index ec35e445..82452db3 100644 --- a/pykeepass/entry.py +++ b/pykeepass/entry.py @@ -92,7 +92,7 @@ def _get_string_field(self, key): if field is not None: return field.text - def _set_string_field(self, key, value, protected=True): + def _set_string_field(self, key, value, protected=None): """Create or overwrite a string field in an Entry Args: @@ -103,9 +103,22 @@ def _set_string_field(self, key, value, protected=True): fields are decrypted immediately upon opening the database. """ field = self._xpath('String/Key[text()="{}"]/..'.format(key), history=True, first=True) + + protected_str = None + if protected is None: + protected_field = self._xpath('String/Key[text()="{}"]/../Value'.format(key), first=True) + if protected_field is not None: + protected_str = protected_field.attrib.get("Protected") + else: + protected_str = str(protected) + if field is not None: self._element.remove(field) - self._element.append(E.String(E.Key(key), E.Value(value, Protected=str(protected)))) + + if protected_str is None: + self._element.append(E.String(E.Key(key), E.Value(value))) + else: + self._element.append(E.String(E.Key(key), E.Value(value, Protected=protected_str))) def _get_string_field_keys(self, exclude_reserved=False): results = [x.find('Key').text for x in self._element.findall('String')] @@ -183,7 +196,10 @@ def password(self): @password.setter def password(self, value): - return self._set_string_field('Password', value) + if self.password: + return self._set_string_field('Password', value) + else: + return self._set_string_field('Password', value, True) @property def url(self): @@ -231,7 +247,10 @@ def otp(self): @otp.setter def otp(self, value): - return self._set_string_field('otp', value) + if self.otp: + return self._set_string_field('otp', value) + else: + return self._set_string_field('otp', value, True) @property def history(self): @@ -338,6 +357,10 @@ def is_custom_property_protected(self, key): """ assert key not in reserved_keys, '{} is a reserved key'.format(key) + return self._is_property_protected(key) + + def _is_property_protected(self, key): + """Whether a property is protected.""" field = self._xpath('String/Key[text()="{}"]/../Value'.format(key), first=True) if field is not None: return field.attrib.get("Protected", "False") == "True" diff --git a/tests/tests.py b/tests/tests.py index 75842192..b5b73020 100644 --- a/tests/tests.py +++ b/tests/tests.py @@ -1019,6 +1019,38 @@ def test_issue344(self): e._element.xpath('Times/ExpiryTime')[0].text = None self.assertEqual(e.expiry_time, None) + def test_issue376(self): + # Setting the properties of an entry should not change the Protected + # property + subgroup = self.kp.root_group + e = self.kp.add_entry(subgroup, 'banana_entry', 'user', 'pass') + + self.assertEqual(e._is_property_protected('Password'), True) + self.assertEqual(e._is_property_protected('Title'), False) + self.assertEqual(e.otp, None) + self.assertEqual(e._is_property_protected('otp'), False) + + e.title = 'pineapple' + e.password = 'pizza' + e.otp = 'aa' + + self.assertEqual(e._is_property_protected('Password'), True) + self.assertEqual(e._is_property_protected('Title'), False) + self.assertEqual(e._is_property_protected('otp'), True) + + # Using protected=None should not change the current status + e._set_string_field('XYZ', '1', protected=None) + self.assertEqual(e._is_property_protected('XYZ'), False) + + e._set_string_field('XYZ', '1', protected=True) + self.assertEqual(e._is_property_protected('XYZ'), True) + + e._set_string_field('XYZ', '1', protected=None) + self.assertEqual(e._is_property_protected('XYZ'), True) + + e._set_string_field('XYZ', '1', protected=False) + self.assertEqual(e._is_property_protected('XYZ'), False) + class EntryFindTests4(KDBX4Tests, EntryFindTests3): pass From 61db593f7c03c98fd14bdbc3cac46740a8bd6921 Mon Sep 17 00:00:00 2001 From: evan Date: Tue, 9 Apr 2024 16:46:01 -0500 Subject: [PATCH 2/2] update docstring --- pykeepass/entry.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/pykeepass/entry.py b/pykeepass/entry.py index 82452db3..82ac8047 100644 --- a/pykeepass/entry.py +++ b/pykeepass/entry.py @@ -98,9 +98,11 @@ def _set_string_field(self, key, value, protected=None): Args: key (str): name of field value (str): value of field - protected (bool): mark whether the field should be protected in memory - in other tools. This property is ignored in PyKeePass and all - fields are decrypted immediately upon opening the database. + protected (bool or None): mark whether the field should be protected in memory + in other tools. If None, value is either copied from existing field or field + is created with protected property unset. + + Note: pykeepass does not support memory protection """ field = self._xpath('String/Key[text()="{}"]/..'.format(key), history=True, first=True)