Support X-Debug header that sends sentry report #498
Conversation
As a precaution, the header is only respected from a trusted ip address. This provides a secure way to to targeted diagnostic debugging. This change also reintroduced the X-Sentry-Id header, as I figured out how to include that when possible. It's not possible to do so for soft timeouts, as we don't know until after the request is sent. Nor for errors that occur after headers have been sent.
bloodearnest
force-pushed
the
x-debug
branch
from
87fa52c
to
e6d1445
Compare
There was a problem hiding this comment.
It looks like X-Forwarded-For is being treated the wrong way round or my understanding of intention is wrong.
Other than that looks good.
Not relevant to the review but I assume we replace untrusted X-Forwarded-For at our entry point but it would be good to check if we aren't sure.
| ip_str = request.environ.get('CLIENT_IP') | ||
| if ip_str is None: | ||
| # fallback to werkzeug's handling | ||
| ip_str = force_unicode(request.access_route[-1]) |
There was a problem hiding this comment.
This is the IP of the last Proxy server - is that what was intended?
There was a problem hiding this comment.
It was, as a paranoid security measure. werkzeug includes a fix to trust up to N proxies, but by default, it only trusts the previous hop, which we emulate here.
So, we generally don't have a proxy that modifies X-Forwarded-For, it just has the client ip in it, so this works, as X-Forwarded-For just contains the clients ip.
If a user sends a forged Forwarded-For, its first in the list, and we still only only trust the one we append. We don't currently strip, but we could do.
If we had a proxy in the middle that did add an entry, yes, we'd need to support this. Werkzeug supports N trusted proxies config, we could do the same (defaults to 1).
Edit: original reply below, it's wrong.
It means we do not have actual client ips, but for our use that doesn't matter - the only thing we need them for is country code, which is resolved (where they do have client ips) at the FE's and added to a header.
In theory, we could/should strip X-Forwarded-For at the frontends, and thus trust the first ip in the chain, but didn't want that to be on by default. Maybe we could add a config option for saying client ip is trusted, off by default.
Note that we do log the full forwarded chain, we just don't trust it.
This generate a debug-level sentry report for the request.
As a precaution, the header is only respected from a trusted ip address. This provides a secure way to to targeted diagnostic debugging.
This change also reintroduced the X-Sentry-Id header which had been dropped as part of #496, as I figured out how to include that when possible. It's not possible to do so for soft timeouts, as we don't know until after the request is sent. Nor for errors that occur after headers have been sent. However, the sentry report is tagged with the request id, so it's still addressable.