mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
128 stars 41 forks source link

Loading circle animation continues if ajax-requests raise csrf error #5847

Closed M-Reimer closed 4 years ago

M-Reimer commented 6 years ago

Describe the problem and steps to reproduce it:

A few weeks ago, I was still able to change my Addon without needing to enable the Referer. Today I was unable to upload a new Screenshot. I wasn't even informed why this fails (just have the "loading circle" forever) and had to open the developer tools to review the webserver response.

What happened?

I wasn't able to add a new screenshot. No proper error displayed as the request seems to happen via "Ajax".

What did you expect to happen?

Updates to Addons shouldn't require the Referer to be sent. Even PayPal removed their Referer requirement so why is it required on AMO? You already have the Cookie to check for!

EnTeQuAk commented 6 years ago

I do agree that there should be proper error handling and not just that loading circle animation. So that's a bug.

We do use the Referer as part of our CSRF checks for several reasons, partially because cookies alone don't give the user the best security in that regard as protection against man-in-the-middle attacks.

I'll leave this open for the circle-animation but right now I don't think we will be doing anything against using the Referer header, this has been updated as part of our update to Django 1.11 (the web-framework we're using) which introduced more advanced security checks for CSRF protection. mozilla/addons-server#8738

M-Reimer commented 6 years ago

IMHO a "Referer check" never makes anything more secure and a MITM attack on the cookie should be impossible if HTTPS is used. It just harms privacy to not allow the user to disable this "feature" altogether. But its some thing that is built into a framework and maybe not easily possible to disable. Sad, but a proper error message would be a start. Just one page more where Referer has to be temporarily enabled before killing it again.

EnTeQuAk commented 6 years ago

We actually have strict referer checking precisely for HTTPS requests tbh. That means that even if a subdomain can set or modify cookies on your domain, it can't force a user to post to our application since that request won't come from your own exact domain.

"This also addresses a man-in-the-middle attack that’s possible under HTTPS when using a session independent secret, due to the fact that HTTP Set-Cookie headers are (unfortunately) accepted by clients even when they are talking to a site under HTTPS. (Referer checking is not done for HTTP requests because the presence of the Referer header isn’t reliable enough under HTTP.)" (from Django docs)

M-Reimer commented 6 years ago

So someone would have to trick someone to a HTTP subdomain in the mozilla.org context and manipulate the network to change the content of this subdomain. Seems to be not that simple.

In my opinion it is not a good idea to insist on the Referrer at all. This header should have been deleted from the HTTP standard years ago. Webservers shouldn't be allowed to know where I came from.

But that's just my opinion and shouldn't be discussed here in detail. I'll just enable the Referrer if I manage my Addons. Please just don't destroy the rest of AMO (parts without login requirement) with Referrer checks.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. If you think this bug should stay open, please comment on the issue with further details. Thank you for your contributions.

M-Reimer commented 5 years ago

Still no proper error handling but locked dialogs if I forget to enable referrer on AMO.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. If you think this bug should stay open, please comment on the issue with further details. Thank you for your contributions.

M-Reimer commented 4 years ago

Still no proper fix

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. If you think this bug should stay open, please comment on the issue with further details. Thank you for your contributions.

M-Reimer commented 4 years ago

Nothing changed

diox commented 4 years ago

We're unlikely to fix this as this is done by Django itself.

M-Reimer commented 4 years ago

You could at least show a proper error message. If I remember correctly you have more useful error messages in other areas on AMO.