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

Fully Redact Password #138

Merged
merged 13 commits into from Jul 21, 2023
Merged

Conversation

jacobshaw42
Copy link
Contributor

In main.py

  • line 108: there was actually no problem here in my current environment, but this seems like a safe bet, just in case
  • line 116: The url encoded connection string is currently logging the password in plain text between encodings. This change will redact passwords in a connection string message variable to be logged.

In utils.py

  • line 127: in a linux environment, if a special character is in the password, the quote_this function will put the password in double quotes and single quotes. So, the password becomes 'password' and this line fails to redact the password
  • line 128: will log the non-redacted password in the case mentioned for 127

The solution in all cases:

  • using regular expressions to redact a password from the url_encoded connection string and two other places, to ensure password is redacted from logs

jacobshaw42 and others added 2 commits July 19, 2023 23:46
…onnection string to keep password out of logs. also redacted in 2 other logging positions for when special characters in linux break current redacting
@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Merging #138 (e2cf90f) into master (bd61a32) will increase coverage by 0.10%.
The diff coverage is 100.00%.

❗ Current head e2cf90f differs from pull request most recent head 1379638. Consider uploading reports for the commit 1379638 to get more accurate results

@@            Coverage Diff             @@
##           master     #138      +/-   ##
==========================================
+ Coverage   94.02%   94.13%   +0.10%     
==========================================
  Files           4        4              
  Lines         268      273       +5     
==========================================
+ Hits          252      257       +5     
  Misses         16       16              
Impacted Files Coverage Δ
bcpandas/main.py 97.74% <100.00%> (+0.05%) ⬆️
bcpandas/utils.py 89.65% <100.00%> (+0.24%) ⬆️

@jacobshaw42
Copy link
Contributor Author

jacobshaw42 commented Jul 20, 2023

This will fix issue #118

@yehoshuadimarsky
Copy link
Owner

Thanks so much for the contribution! This looks good to merge, just one small request - are you able to also write a (small) unit test that verifies this? Something simple that creates a credentials object, and verifies that the logs redact it - I recommend using pytests's caplog fixture https://docs.pytest.org/en/7.1.x/how-to/logging.html

@jacobshaw42
Copy link
Contributor Author

Certainly, I will create the unit test for SqlCreds

This will also have an impact on the to_sql function, but I am not aware of how to create a unit test for something that would be inserting into a database.

@jacobshaw42
Copy link
Contributor Author

@yehoshuadimarsky
Please see the update with the unit test

@yehoshuadimarsky yehoshuadimarsky merged commit 8f3d086 into yehoshuadimarsky:master Jul 21, 2023
5 checks passed
@yehoshuadimarsky
Copy link
Owner

Merged, thank you for your contribution!

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.

None yet

2 participants