adsabs / adsws

ADS web services
Other
2 stars 15 forks source link

/v1/feedback #73

Closed romanchyla closed 8 years ago

romanchyla commented 9 years ago

seems to be a part of the api, but it is not governed/protected by the same rules - in fact, it is a simple application exposed under /v1/feedback. It doesn't have oauth scopes which means ADS team can be freely spammed by anyone who finds the endpoit. Am I reading it correctly?

jonnybazookatone commented 9 years ago

Try ;-).

Well, you need to fill a recaptcha. So yes, if you can automate google's recaptcha service then you can spam the end point. It's meant to replicate the same as the user registration page, given we don't assume people who leave feedback have to have an account.

romanchyla commented 9 years ago

wait, I go online, fill the captcha and then send you million requests to /v1/feedback with the same cookie (and token, but that is not used anyway); do you think this strategy will fail to accomplish the nefarious goal?

jonnybazookatone commented 9 years ago

Can you be a bit more specific, are you pointing out a problem with a specific line of code. Or our general approach of filtering humans from bots?

romanchyla commented 9 years ago

the app is registered under /v1/feedback https://github.com/adsabs/adsws/blob/master/wsgi.py#L30 so that is one thing; nothing checks that you have right to access it

the second is https://github.com/adsabs/adsws/blob/master/adsws/feedback/views.py#L91 i can easily circumvent the check by sending 'g-recaptcha-response' = True

unless that get_post_data actually contains a hook which verifies the data (from just reading the github code I think it does not) -- so that could be a second problem

but even if it did check the data (which I think it doesn't), I can get the captcha in the browser and send you many messages with the captcha token - but probably not million, the google would most likely identify them as robots if cming too fast

romanchyla commented 9 years ago

the last point is highly hypothetical, but i think the first two are worth digging

jonnybazookatone commented 9 years ago

Is there a reason not to advertise the feedback end point if we also advertise the user sign up end point?

if not post_data.get('g-recaptcha-response', False) or \
                not verify_recaptcha(request):

Unless I've mashed my brain, if either of these are not true, then it will fail. So sure, you can pass something silly in the 'g-recaptcha-response', but it still carries out verify_recaptcha. https://github.com/adsabs/adsws/blob/master/adsws/accounts/utils.py#L54

As per the details of retaining the 'g-recaptcha-response', and re-using it repeatedly against googles recaptcha service, I don't know the details. I'll have to have a go at spamming us.

romanchyla commented 9 years ago

yes, you are right, sorry (btw: tried to reuse the captcha for requests, google detects that)

and no, they should be advertized, the point i'm trying to make is the ambiguous nature of the dispatcher, it maps /v1/feedback to one applicaiton, and /v1/ to another application

i guess what i'm really asking is why '/feedback' doesn't live under /v1 - as a microservice

On Tue, Jul 21, 2015 at 4:27 PM, Jonny Elliott notifications@github.com wrote:

Is there a reason not to advertise the feedback end point if we also advertise the user sign up end point?

if not post_data.get('g-recaptcha-response', False) or \ not verify_recaptcha(request):

Unless I've mashed my brain, if either of these are not true, then it will fail. So sure, you can pass something silly in the 'g-recaptcha-response', but it still carries out verify_recaptcha. https://github.com/adsabs/adsws/blob/master/adsws/accounts/utils.py#L54

As per the details of retaining the cookie, and re-using it repeatedly against googles recaptcha service, I don't need the details. I'll have to have a go at spamming us.

— Reply to this email directly or view it on GitHub https://github.com/adsabs/adsws/issues/73#issuecomment-123468517.

jonnybazookatone commented 9 years ago

Ah, cool, I was gonna try that, glad it works as expected.

Hmmm. Can you elaborate a little bit more? If I look at the resources the API advertises:

    "adsws.feedback": {
        "base": "/v1/feedback", 
        "endpoints": [
            "/oauth/authorize", 
            "/oauth/invalid/", 
            "/oauth/errors/", 
            "/oauth/token", 
            "/oauth/ping/", 
            "/oauth/ping/", 
            "/oauth/info/", 
            "/slack"
        ]

so feedback is under v1. Maybe you saw the old BBB PR that had /feedback not under v1/, but then AH asked me to move it to /v1/feedback/?

romanchyla commented 9 years ago
"adsws.api": {
        "base": "/v1",
        "endpoints": [
            "/feedback",
            "/recommender/resources",
            "/graphics/resources",
            "/biblib/resources",
            "/biblib/libraries",
...

On Tue, Jul 21, 2015 at 6:52 PM, Jonny Elliott notifications@github.com wrote:

Ah, cool, I was gonna try that, glad it works as expected.

Hmmm. Can you elaborate a little bit more? If I look at the resources the API advertises:

"adsws.feedback": {
    "base": "/v1/feedback",
    "endpoints": [
        "/oauth/authorize",
        "/oauth/invalid/",
        "/oauth/errors/",
        "/oauth/token",
        "/oauth/ping/",
        "/oauth/ping/",
        "/oauth/info/",
        "/slack"
    ]

so feedback is under v1. Maybe you saw the old BBB PR that had /feedback not under v1/, but then AH asked me to move it to /v1/feedback/?

— Reply to this email directly or view it on GitHub https://github.com/adsabs/adsws/issues/73#issuecomment-123502153.

jonnybazookatone commented 9 years ago

So you're wanting it to display, '/feedback/slack', in the list?

jonnybazookatone commented 9 years ago

Or are you asking why is feedback in the core api, and not as a standalone microservice? I don't fully understand the question, sorry.

jonnybazookatone commented 9 years ago

Am I free to close this, or you wanted to have more discussion about the last part of your statements? I didn't fully understand the comments you made, so I'm unable to answer without some more input.

romanchyla commented 9 years ago

yes, yes, that is waht i'm asking

jonnybazookatone commented 9 years ago

So there are few reasons to argue for it not to be a stand alone service:

These reasons seemed more compelling than justifying it sitting as an extra standalone microservice. Of course, if there are more compelling reasons to do the alternative, I don't have any strong opinions against it.

jonnybazookatone commented 9 years ago

poke @romanchyla

romanchyla commented 9 years ago

the encapsulation is one reason (apis /v1, and /v2 will both need /feedback endpoint, yet their implementaion may differ), the divide-and-conquer approach is another reason (for development)

but i have since realized that the issue lays rather with how the api core is organized, see here: https://github.com/adsabs/adsrex/blob/master/v1_0/api/core.py

i rest my case