revel / modules

Officially supported Revel modules
http://revel.github.io/modules/
MIT License
49 stars 46 forks source link

REVEL CSRF: Same origin mismatch. #83

Closed ptman closed 5 years ago

ptman commented 6 years ago

I'm not sure if it's because of running behind a proxy or not, but with some logging just after https://github.com/revel/modules/blob/03baf1b2e50c5071501f13f238fff4eeb389f8c1/csrf/app/csrf.go#L58 :

sameorigin=false referer=https://dev.carecode.net/ url=/

Clearly the test will fail with those inputs:

    return u1.Scheme == u2.Scheme && u1.Host == u2.Host

Something isn't working. I see that the logic is a bit different from cbonello/revel-csrf

ptman commented 6 years ago

Is something stripping out the Host -header? Logging c.Request.Header.Get("Host") gives me nothing. But firefox claims the header is set.

ptman commented 6 years ago

I'm still getting this:

Server Error:
Forbidden: REVEL CSRF: Same origin mismatch.

I prepared a test revel project: https://github.com/ptman/revel-csrf-test , please guide me to fix it

notzippy commented 6 years ago

One other thing with your test. The form value posted must be csrftoken (not _csrftoken) and you need to have a referrer page to start from. So you need to start at page A, have a link to page B (which has the form) and final post the result to page C and you should be good to go

ptman commented 5 years ago

A -> B -> C sounds nonsensical. Why couldn't the landing page include a form that has csrf-protection? You don't need to have referer & token to show the form (GET), only to process the form (POST).

golddranks commented 5 years ago

Hi, sorry to butt in. I just tested the code on the development branch. The "request URL fixing" ( https://github.com/revel/modules/blob/develop/csrf/app/csrf.go#L66 ) that adds http:// or https:// to the request URL breaks some things: it breaks relative URLs, changing stuff like /login/ to http:///login/. Because the URL is dispatched at this point, this doesn't have a direct effect to routing, but for example, when doing redirects, it makes the location header behave oddly with relative URLs, stuff like Location: http:///

notzippy commented 5 years ago

Hi @golddranks the issue you seem to be having is that the hostname is missing from the request, then it would compact in that manner, the missing hostname may be due to revel running behind a proxy. Is that your setup ?

golddranks commented 5 years ago

I have revel running inside a container locally and behind a load balancer in AWS. Both have the same behaviour: revel.Controller.Request.URL is relative.

I tried to run revel directly without container on localhost, but it still behaves the same.

golddranks commented 5 years ago

Looking at the code (EDIT, here: https://github.com/revel/revel/blob/master/server_adapter_go.go#L241 ), I don't find that behaviour surprising: https://stackoverflow.com/questions/23151827/how-to-get-url-in-http-request

So it seems that the current way of "fixing" the URL is broken.

notzippy commented 5 years ago

Updated PR #91 , specifically this section which should cover most cases where the hostname is unknown. @golddranks let me know if this is still an issue

ptman commented 5 years ago

@notzippy Much better, thank you.

@golddranks Does it work behind a proxy for you?

ptman commented 5 years ago

Tests seem to be failing for me. And also running behind the proxy started by revel run.

DEBUG 13:26:23    app   csrf.go:122: Using                                    request host=0.0.0.0:44917 cookie domain=  path=/login/password request url host= namespace=App\\  ip=127.0.0.1 method=GET action=App.LoginPassword
DEBUG 13:26:23    app   csrf.go:161: getFullRequestURL                          ip=127.0.0.1 path=/login/password method=GET action=App.LoginPassword namespace=App\\ requesturl=http://0.0.0.0:44917
ptman commented 5 years ago

Do I have to rewrite all tests in order to keep the csrf filter happy? https://github.com/ptman/revel-csrf-test/commit/f069d3d0d600e67f42927cc429462fff890f88a3

ptman commented 5 years ago

Maybe add helpers that take care of the csrf-specific bits during tests?

notzippy commented 5 years ago

Will create a new issue on that