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

Don't log sensitive data with logged method #244

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jakurban
Copy link

sensitive parameter was introduced recently, but it was not reflected in log function decorator. This PR changes behaviour to hide all attributes except the safe ones.

@codecov
Copy link

codecov bot commented Jul 31, 2023

Codecov Report

Merging #244 (7c7e083) into master (b891f49) will decrease coverage by 0.06%.
The diff coverage is 66.66%.

❗ Current head 7c7e083 differs from pull request most recent head 1df4fc4. Consider uploading reports for the commit 1df4fc4 to get more accurate results

@@            Coverage Diff             @@
##           master     #244      +/-   ##
==========================================
- Coverage   86.45%   86.40%   -0.06%     
==========================================
  Files          18       18              
  Lines        2570     2574       +4     
==========================================
+ Hits         2222     2224       +2     
- Misses        348      350       +2     
Flag Coverage Δ
unittests 86.40% <66.66%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/widgetastic/log.py 93.50% <66.66%> (-2.39%) ⬇️

@jakurban
Copy link
Author

jakurban commented Aug 3, 2023

@digitronik @mshriver could you please look at this ?

Copy link
Member

@digitronik digitronik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can't control selenium side logs

DEBUG:urllib3.connectionpool:http://localhost:4444 "POST /session/9bab0357d071d4eaae6a146e8fa94eba/execute/sync HTTP/1.1" 200 0
DEBUG:selenium.webdriver.remote.remote_connection:Remote response: status=200 | data={"value":"foo"} | headers=HTTPHeaderDict({'content-length': '15', 'content-type': 'application/json; charset=utf-8', 'cache-control': 'no-cache'})
DEBUG:selenium.webdriver.remote.remote_connection:Finished Request
INFO:widgetastic_null:[TextInput]: fill(*****, sensitive=True) -> False (elapsed 89 ms)

@@ -30,8 +30,15 @@ def call_sig(args: Iterator[Any], kwargs: MutableMapping[str, Any]) -> str:
Returns:
A string that contains parameters in parentheses like the call to it.
"""
arglist = [repr(x) for x in args]
arglist.extend(f"{k}={v!r}" for k, v in kwargs.items())
safe_args = ["sensitive", "locator"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why locator?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i suppose debugging is completely ruined unless at least locator is not hidden

@mshriver
Copy link
Collaborator

@JaurbanRH marking draft until the comments have been addressed. If you're not able to continue work on this let us know and we'll take over ownership of the commit.

Thanks!

@mshriver mshriver marked this pull request as draft August 27, 2024 14:05
@jakurban
Copy link
Author

@mshriver Yes, I am unable to continue to work on this issue.

@RonnyPfannschmidt
Copy link
Collaborator

im wondering if we could make use of pydantic Secret objects to explicitly mark secrets, and then have a logging filter for the selenium namespace to explicitly mark the sensitive data

unfortunately introducing secrets now will be very disruptive for users at first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants