8000 Add escaped slug into logs when slug does not match any pool by nymkappa · Pull Request #1657 · mempool/mempool · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add escaped slug into logs when slug does not match any pool #1657

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

Merged
merged 2 commits into from
May 20, 2022

Conversation

nymkappa
Copy link
Member
@nymkappa nymkappa commented May 17, 2022

Fixes #1655

The reason we did not include more details in this log was because we're unsure if it's safe to include user input in there. I used mysql2 escape function to sanitize the string, I don't know if that's safe enough.

@softsimon

@nymkappa nymkappa requested a review from wiz May 17, 2022 09:31
@cla-bot cla-bot bot added the cla-signed label May 17, 2022
@nymkappa nymkappa force-pushed the nymkappa/bugfix/update-log branch from d4861d9 to d76119f Compare May 17, 2022 09:32
@wiz
Copy link
Member
wiz commented May 18, 2022

I don't understand why we would want to log this at all. What's wrong with silently returning 404?

@nymkappa nymkappa force-pushed the nymkappa/bugfix/update-log branch from a825495 to 37b4c84 Compare May 18, 2022 12:59
@nymkappa nymkappa force-pushed the nymkappa/bugfix/update-log branch from 37b4c84 to f402bfb Compare May 18, 2022 13:01
@nymkappa
Copy link
Member Author

I don't understand why we would want to log this at all. What's wrong with silently returning 404?

As discussed, I've removed the logging of this error. It will still throw a 404 with the invalid mining pool slug used:

nymkappa@nymkappas-MacBook-Pro ~ % curl -s localhost:4200/api/v1/mining/pool/nymkappa
This mining pool does not exist 'nymkappa'%

Copy link
Member
@wiz wiz left a comment

Choose a reason for hiding this comment

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

Tested ACK @ v2.4.0-dev [6cd3c312]

@wiz wiz merged commit b4beb29 into master May 20, 2022
@wiz wiz deleted the nymkappa/bugfix/update-log branch May 20, 2022 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DEBUG: This slug does not match any known pool
2 participants
0