-
Notifications
You must be signed in to change notification settings - Fork 46
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
DisallowedHost errors should not trigger a Sentry error #1687
Comments
First of all, why is it even passed into Django? Shouldn't this host header check be done in Nginx or the load balancer or something? @sciurus ?? Incidentally, I noticed that on
But at least it doesn't send anything to Sentry. Second of all, it's a weird exception. Or is it just Sentry being weird? |
@sciurus We can move this to Bugzilla if you prefer. |
We don't have a standard practice of blocking based on the host header in nginx or the load balancer. Currently, applications where it matters can use their framework's preferred method of blocking access, e.g. ALLOWED_HOSTS for Django. In Normandy in AWS we did add configuration to block unknown hosts in https://github.com/mozilla-services/cloudops-deployment/pull/1524/ . This was essentially "belt and suspenders" to deal with the cache poisoning security issue. Since ALLOWED_HOSTS is working, we just went back to our standard nginx configuration for Normandy in GCP. Presumably you're seeing this sentry error now because we cut a lot of Normandy's traffic over to GCP yesterday. I think the idea of continuing to block within nginx is interesting. I could add it back just to Normandy, but @milescrabill is currently working on the next generation of our team's standard nginx configuration so it might be better to raise the idea with him. For now though, I think that things are working as expected, except for the errors going to Sentry. You're in a better position to investigate and this that than I am. |
I can definitely tackle the not-sending-DisallowedHost-exceptions-to-Sentry problem. Mind you, I think we might switch from python-raven to sentry-sdk so that might change things. And, as you said, this is working as expected in Django (apart from the noisy Sentry pings) which is maybe not a terrible thing. In our projects, we have it as a "policy" that Nginx should be rather "dumb" when it comes to serving static assets and allow Django (technically Whitenoise) to do that part. |
I'm struggling to solve this. The right place to add a list of Lines 283 to 286 in 2dda0b0
I did test it by adding: Two questions I don't know the answer to...
|
I don't understand how (1) is a question. For (2), the default is nothing. I found this by grepping the source of Raven for I think is a good way to solve the issue. |
Regarding (1), what I meant to ask was; How do you reproduce causing a Sentry send on a DisallowedHost exception? |
Ah, I see now. I tried to get Sentry to send a @sciurus Do you know if there is something special we need to be doing to get Raven to actually send errors to Sentry? |
Glad it's not just me! I do wonder if it's related to logging. Perhaps it's instructed somewhere to swallow certain exceptions. If so, it's sad that I can't get my local env to simulate how we do things in production. |
Stale. I'm tempted to go ahead and just do the RAVEN_CONFIG trick mentioned above. These errors are still bombarding our Sentry. |
We'll just mark these as muted in sentry rather than adding custom code. |
https://sentry.prod.mozaws.net/operations/normandy-prod/issues/5108824/
The text was updated successfully, but these errors were encountered: