8000 Censor DB password when outputting to logs by anasyusef · Pull Request #5265 · freqtrade/freqtrade · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Censor DB password when outputting to logs #5265

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jul 12, 2021
Merged

Censor DB password when outputting to logs #5265

merged 7 commits into from
Jul 12, 2021

Conversation

anasyusef
Copy link
Contributor

Summary

Censor password from logs when providing a DB connection URI
E.g:
From: postgresql+psycopg2://scott123:scott123@host/dbname
Outputs the following to logs: postgresql+psycopg2://scott123:*****@host/dbname

Quick changelog

  • Apply censoring to logs on URI that contains a password

@anasyusef anasyusef changed the title Censor db pwd logs Censor DB password when outputting to logs Jul 12, 2021
xmatthias
xmatthias requested changes Jul 12, 2021
View reviewed changes
Copy link
Member
@xmatthias xmatthias left a comment

Choose a reason for hiding this comment

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

In principle i'd say - looks good.

However, please remove the .terraform submodule which seems to have been commited by mistake (best via rebase so it's gone from history, too).

@coveralls
Copy link
coveralls commented Jul 12, 2021 •
edited
Loading

Coverage Status

Coverage increased (+0.001%) to 98.103% when pulling 08a4da6 on anasyusef:censor_db_pwd_logs into ed77889 on freqtrade:develop.

@anasyusef
Copy link
Contributor Author

Yep. It was indeed a mistake. I've removed the submodule from the branch. It should hopefully be good now

@anasyusef anasyusef requested a review from xmatthias July 12, 2021 13:29
xmatthias
xmatthias approved these changes Jul 12, 2021
View reviewed changes
Copy link
Member
@xmatthias xmatthias left a comment •
edited
Loading

Choose a reason for hiding this comment

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

seems like the only case where the db-url is logged with password is when it's wrong - but i guess then it's ok / acceptable - as it might show you the error too.

Great job - LGTM 👍

@xmatthias xmatthias merged commit a261b18 into freqtrade:develop Jul 12, 2021
@anasyusef
Copy link
Contributor Author

Thank you! I was thinking of adding validation for that edge case. But then, I guess SQLAlchemy would handle that and raise the error as It wouldn't be able to establish a connection.

@anasyusef anasyusef deleted the censor_db_pwd_logs branch July 12, 2021 21:40
@xmatthias xmatthias added the Enhancement Enhancements to the bot. Get lower priority than bugs by default. label Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xmatthias xmatthias xmatthias approved these changes

Assignees
No one assigned
Labels
Enhancement Enhancements to the bot. Get lower priority than bugs by default.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@anasyusef @coveralls @xmatthias
0