Search: do not record invalid queries #9349
Conversation
We are generating lot of errors on Sentry because `query='\x00'`. This commit skips logging those queries. Reference: https://sentry.io/organizations/read-the-docs/issues/3066070420/?project=148442
Block requests that contain "\x00" characters on their GET attributes. Raise `SuspiciousOperation` on these cases. Related to #9349
Block requests that contain "\x00" characters on their GET attributes. Raise `SuspiciousOperation` on these cases. Related to #9349 Sentry issues: https://sentry.io/organizations/read-the-docs/issues/2941721921/?project=148442
| log.bind( | ||
| project_slug=project_slug, | ||
| version_slug=version_slug, | ||
| ) |
There was a problem hiding this comment.
The log object is for the entire module --There are also log functions in other tasks, so I wonder: couldn't this produce log messages with project_slug and version_slug in unrelated calls?
There was a problem hiding this comment.
This is how structulog works (https://github.com/jrobichaud/django-structlog). Every log instance is tied to each request. Take a look at https://www.structlog.org/en/stable/getting-started.html#building-ctx
There was a problem hiding this comment.
Shouldn't it be log = log.bind ?
There was a problem hiding this comment.
I think that is required when running a normal Python application. However, we also have django-structlog and we are installing a middleware from there that I understand that "reset" the logger for each different thread/request: https://django-structlog.readthedocs.io/en/latest/getting_started.html
The same happens with the Celery integration we are using from the same module.
I don't remember from the top of my mind exactly how this works, but I think what I'm saying it's pretty close 😄 . I haven't seen log entries with invalid/inconsistent key/values pairs, so I'd assume it's working fine so far; but maybe we just haven't hit the issue yet.
There was a problem hiding this comment.
It makes sense that it's something that this middleware would provide 👍 Thanks for a great explanation 👍
|
Shouldn't the middleware already be blocking these? The search query is passed in a get parameter. |
|
Yes. I think this is not required anymore now that we have that middleware. I'm closing it. We can re-open if required. |
We are generating lot of errors on Sentry because
query='\x00'. This commitskips logging those queries.
Reference: https://sentry.io/organizations/read-the-docs/issues/3066070420/?project=148442