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

Avoid decoding data in request logging #485

Closed
fabioasdias opened this issue Sep 1, 2023 · 2 comments · Fixed by #571
Closed

Avoid decoding data in request logging #485

fabioasdias opened this issue Sep 1, 2023 · 2 comments · Fixed by #571
Assignees
Labels
bug Something isn't working

Comments

@fabioasdias
Copy link

Hi all,

First, thanks for the library, really handy so I don't have to deal with sigv4 myself.

However, I'm running into an issue in this line:

body = body.decode("utf-8", "ignore")

where the request body is unnecessarily processed and ends up causing memory issues on a rather heavy bulk request. There's also the fact one might not want the body to be logged, there are security implications there in some cases. That's the reason behind the Pylint rule to use lazy evaluation on logging, one shouldn't pay the computational price for a log message that's not even persisted/desired.

@github-actions github-actions bot added the untriaged Need triage label Sep 1, 2023
@wbeckler
Copy link
Contributor

wbeckler commented Sep 1, 2023

Maybe something like
if logger.isEnabledFor(debug) [decode the body]

@dblock
Copy link
Member

dblock commented Sep 5, 2023

100% we should not process the body unless explicitly needed for logging, please contribute a fix @fabioasdias?

@saimedhi saimedhi removed the untriaged Need triage label Sep 5, 2023
@dblock dblock added the bug Something isn't working label Nov 10, 2023
@dblock dblock changed the title Unused log data causing memory errors Avoid decoding data in request logging Nov 10, 2023
@dblock dblock self-assigned this Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants