replay attack via callback url request ???

[jedie]

The oscar_docdata.views.OrderReturnView doesn't do any checks before calling facade.update_order(order)

Obviously, nothing bad is happening. However, it is not nice that the docdata server is asked for an update each time the callback url requested.

Also return_view_called will be called. I think even if the status hasn't changed. e.g.: If a notification mail will be send on return_view_called, then it will send more than one mails ;)

I think it should be checked the relationship between callback url status and the order status. e.g.: Reject every request if order status is not "new"

Maybe raise SuspiciousOperation() ?

[maerteijn]

It is correct to ask the Docdata server each time for a status update. The mechanism is that only they know the truth about the status of the order. There a a lot of different statuses to take into account as well and checking a local status won't work as it is a record of a certain moment and you can't know what happened in the meantime.

Sending email twice is a different problem and can be solved by accounting this in your project (by using the CommunicationEventType ).

[jedie]

It's not about sending mails. I do that quite differently ;)

You agree with me that an update only makes sense in some situations, isn't it?

I am unsure in what kind of constellation.

Maybe only if order status is in:

    STATUS_NEW = 'new'                      # Initial state
    STATUS_IN_PROGRESS = 'in_progress'      # In the redirect phase
    STATUS_PENDING = 'pending'              # Waiting for user to complete payment (e.g. credit cards)

At least an update with this status makes no sense:

    STATUS_PAID = 'paid'                    # End of story, paid!
    STATUS_PAID_REFUNDED = 'paid_refunded'  # Paid, and performed a partial refund
    STATUS_CANCELLED = 'cancelled'          # End of story, cancelled
    STATUS_CHARGED_BACK = 'charged_back'    # End of story, consumer asked for charge back
    STATUS_REFUNDED = 'refunded'            # End of story, refunded, merchant refunded
    STATUS_EXPIRED = 'expired'              # No results of customer, order was closed.

I don't know which is better: A black or a white list. I'm just sure it's not a good idea not to check the status. Is an invite to DOS attacks.

[maerteijn]

It's not an invite to DOS attacks: you are putting the responsibility for this in the wrong place: your frontend webserver and network infrastructure should detect DOS attacks and make sure that this does not happen (next to the fact that docdata should take care of this as well themselves).

You agree with me that an update only makes sense in some situations, isn't it?

Yes but knowing the current order state is something you have to ask Docdata first to know for sure, that is the whole point. The business logic for these status changes should be implemented in their web service as much as possible and not in our integration package. (next to the fact we don't know their logic; paid orders can be refunded later for example).

So if you are really worried about querying Docdata too often you could check if the updated field has been updated within a (configurable) specific timerange, that would solve this issue with a much simpler solution.

If you add a pull request for this with an included test I will be happy to merge it.

[jedie]

The point is a very general one: The current implementation allows code execution with input data that doesn't make sense.

IMHO it does not make any sense to request an update for a closed order. This Request should never occur in a normal situation, isn't it? This can only happen by some kind of hacking attack.

The implementation should be very restrictive, especially in sensitive areas such as payment processing.

So if you are really worried about querying Docdata too often you could check if the updated field has been updated within a (configurable) specific timerange, that would solve this issue with a much simpler solution.

DocdataOrder.updated seems to be not usable for this, because a new timestamp is set on every save() call. This happens not only on DocData requested updates. It needs something like: DocdataOrder.last_docdata_request_timestamp

But it's not so much about the direct DocData query. This can also be started internally. Queries in quick sequence might be ok?

Much more it is about the callback urls, which an external hacker can just produce fast one after the other to influence the system.

Maybe it makes the most sense to use oscar.apps.order.models.PaymentEvent and create a entry on every callback url request.

[jedie]

Example implementation to restrict the order status: https://gist.github.com/jedie/455e3e7eba64a521d9fd8fe55fec1377

[maerteijn]

You are not really reading what I'm writing.

"Hacking" is not really possible: You pass an order_key and there is a check to see if that order exists. If so, then the latest known status is requested and Docdata answers. I don't see any possibility for a hacker to influence this process but a DOS attack. This should be fixed with a throttling solution or network configuration, NOT in this software.

I don't see any real advantage in status checking either because the only statuses which really cannot be changed (finished states) are DocdataOrder.STATUS_EXPIRED and DocdataOrder.STATUS_CANCELLED. All the other statuses can be changed, also OUTSIDE this application (in the back office or within another application). So adding status checks for these two statuses would not do a call to Docdata but all others will and won't solve a DOS attack then.

To conclude this : if it makes you happy then I will accept a pull request when you filter on these two statuses.

[jedie]

the only statuses which really cannot be changed (finished states) are DocdataOrder.STATUS_EXPIRED and DocdataOrder.STATUS_CANCELLED

really? What's about 'paid', 'charged_back' and 'refunded' ? The Doc-String says that there are "End of story", too.

[maerteijn]

And the docstring is wrong so I adjusted this.

paid orders can be refunded later (otherwise you can't refund right?) charged_back is a request for refund and so are a lot of more cases which can occur.

If you look at the source code you will also see that these statuses are an interpretation so we can at least perform certain actions. These statuses actually DON'T EXIST in the API as the comment in the README already states:

The individual payment objects have a status value, but the payment cluster does not. This means that there is no global status value to read.

Look, we're not getting to something constructive this way so I suggest the following:

• Add a setting DOCDATA_ORDER_FINISHED_STATES with a default value:['expired', 'cancelled']
• Add a filter as you suggested earlier for this in the Interface class
• Don't raise a exception but use logger.error.

[jedie]

Hm. Okay... If we can only ban 'expired' and 'cancelled', then it doesn't make any sense. Thanks for the patience.

[maerteijn]

With the setting you can change it more statuses if you like?