Record credit failures #2620
Record credit failures #2620
Conversation
The `except KeyError:` block was never executed since we were using `.get()`, which doesn't raise `KeyError` but returns `None` instead.
|
Rebased. |
|
Roger that. Waiting for Travis ... |
| @@ -87,15 +87,14 @@ def inbound(request): | |||
| """Given a Request object, reject it if it's a forgery. | |||
| """ | |||
| if request.line.uri.startswith('/assets/'): return | |||
| if request.line.uri == '/balanced-callbacks': return | |||
There was a problem hiding this comment.
This makes me nervous. No way around this?
Also, how about /callbacks/balanced instead? Seems not unlikely that we'll have additional callbacks in the future.
There was a problem hiding this comment.
This makes me nervous.
Why? There's no need for a CSRF check there.
No way around this?
The docs say that callbacks can be sent as GET requests, which would slip through the CSRF check, but I'm not sure how that's any better.
Also, how about
/callbacks/balancedinstead?
Good idea.
|
Move done. ( |
| raise Response(405) | ||
|
|
||
| src = request.headers['X-Forwarded-For'] | ||
| if not src in ('50.18.199.26', '50.18.204.103'): |
There was a problem hiding this comment.
What's the behavior at Heroku if a client sends X-Forwarded-For? Do they replace it?
There was a problem hiding this comment.
That's what I expect, the documentation doesn't say.
There was a problem hiding this comment.
We should test that. If they don't handle X-Forwarded-For properly then a client could trivially bypass this check.
There was a problem hiding this comment.
I've just tested this: the values of X-Forwarded-For form a chain, they're joined with commas. Bottom line: we're good, our check still works.
There was a problem hiding this comment.
Okay. How hard is it to spoof an IP address otherwise? It seems like an IP filter is really weak security for this endpoint, but it looks like that's the best they give us.
As an added safety precaution to ensure event validity, you may also manually fetch the changed entity resource and compare it with records in your system before processing any changes.
There was a problem hiding this comment.
In the mean time, we should implement the workaround, which is to perform an additional API request to load the relevant resource and confirm the change.
There was a problem hiding this comment.
How hard is it to spoof an IP address otherwise?
Spoofing an IP address is not too difficult, but the connections we're receiving are HTTP, which is based on TCP, which uses a three-way handshake, so if you spoof the source address you can't establish the connection, unless you're an active MITM.
There was a problem hiding this comment.
And we're not worried about MITM because Balanced isn't hitting us from a coffee shop, and anyway they're hitting us over SSL. Right?
There was a problem hiding this comment.
And we're not worried about MITM because Balanced isn't hitting us from a coffee shop, and anyway they're hitting us over SSL. Right?
Yes, except SSL doesn't really help us in this case.
Another step of #2508. Fixes #1811. Built on top of #2579.