Mask password in URL.__repr__ for safer debugging output #1629#1630
Mask password in URL.__repr__ for safer debugging output #1629#1630mbaas038 wants to merge 3 commits intoaio-libs:masterfrom
Conversation
psf-chronographer
bot
added
the
bot:chronographer:provided
Merging this PR will degrade performance by 15.36%
Performance Changes
Comparing 
|
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (97.63%) is below the target coverage (100.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #1630 +/- ##
=======================================
Coverage 99.37% 99.37%
=======================================
Files 30 30
Lines 5940 5952 +12
Branches 282 283 +1
=======================================
+ Hits 5903 5915 +12
Misses 22 22
Partials 15 15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mbaas038
force-pushed
the
feature/add-password-masking-in-repr-1629
branch
from
2f914e6 to
80e0f97
Compare
Fix incorrect passing of mask_password parameter Co-authored-by: Jan-Hein Bührman <buhrman@xs4all.nl>
for more information, see https://pre-commit.ci
|
|
||
| def __repr__(self) -> str: | ||
| return f"{self.__class__.__name__}('{str(self)}')" | ||
| return f"{self.__class__.__name__}('{self._make_string(mask_password=True)}')" |
There was a problem hiding this comment.
An additional consideration: since repr() returns now a string that wouldn't yield anymore an object with the same value when passed to eval() in case .password is not None, it could perhaps be better to change the repr (perhaps in that case only, .password is not None) into a string enclosed in angle brackets that contains the name of the type of the object together with additional information often including the name and address of the object. [Italic parts quoted from http://docs.python.org/3/library/functions.html#repr ].
Suggestion:
| return f"{self.__class__.__name__}('{self._make_string(mask_password=True)}')" | |
| return ( | |
| f"{self.__class__.__name__}(repr({self._make_string(mask_password=False)}))" | |
| if self.password is None | |
| else f"<{self.__class__.__name__} repr({self._make_string(mask_password=True)}) (masked)>" | |
| ) |
This also requires changing the tests.
A more general remark: the explicit insertion of single quotes is tricky anyway for certain edge cases, better rely on repr() on the generated string value.
…ry (aio-libs#1643), split_url delim perf fix aio-libs#1630: URL.__repr__ exposed passwords in plaintext, risking credential leakage in logs and tracebacks. Added password masking with '********'. str(), == and all other operations remain unaffected. fix aio-libs#1643: uuid.UUID was converted to int in query params because UUID implements __int__(), matching the SupportsInt protocol. Fixed by checking whether type(v).__str__ is not object.__str__ before using int() conversion. This is a general solution that works for any type with a meaningful __str__. perf: split_url() delimiter search refactored from a for-loop over delim_chars to 3 conditional find() calls using the already-computed has_hash and has_question_mark flags. Reduces per-call overhead for URL-heavy workloads.
2 participants
What do these changes do?
yarl.URL.__repr__exposed passwords in URLs, risking credential leakage in logs, tracebacks, or debugging output.__repr__while leaving other URL parts unchanged.__repr__that replaces the password with a placeholder (********) if present.Are there changes in behavior for the user?
repr()output — may affect tests relying on exact string output.Related issue number
URL.__repr__for safer debugging output #1629Checklist
This PR was kindly supported by Sopra Steria Pythoneers