search

sinatra / rack-protection

NOTE: This project has been merged upstream to sinatra/sinatra
https://github.com/sinatra/sinatra/tree/master/rack-protection
818 stars 58 forks source link

HttpOrigin should be disabled by default #37

Closed p0deje closed 11 years ago

p0deje commented 11 years ago

Apparently some people struggle when it's enabled by default // cc @homakov

homakov commented 11 years ago

PLZPLZ NO MAKE OTHER PPL WORK NEW YEAR NIGHT AGAIN

rkh commented 11 years ago

What's other people? Is this with or without Sinatra?

p0deje commented 11 years ago

@homakov said they have Rack gateway which is being used by some banks. Some of them send back with POST and thus Origin doesn't equal base_url. Apparently, they should be whitelisted, but it took time to figure out what was going wrong.

homakov commented 11 years ago

http_origin aims to fix CSRF by denying ALL cross POSTs. First of all Origin is not supported by every browser thus the fix is not truly helpful. and second it is hard to figure out why i get Forbidden, i use authenticity_token to protect from CSRF. so it's better to disable it by default.

rkh commented 11 years ago

I think the real fix would be to clear the session, though (as it already does when set up by Sinatra).

homakov commented 11 years ago

@rkh clearing session is not any better than "Forbidden" response. No session - no working app :) The last two years I am :+1: :+1: :+1: :+1: :+1: :+1: :+1: :+1: for denying Cross POSTs but http_origin as i said previously is not enough: Origin is not supported by every browser... up to you. i need others' opinions

p0deje commented 11 years ago

@homakov While it's not supported by other browsers except to Chrome, it's not harmful to unsupported browsers since they don't send Origin at all.

@rkh The real problem is that in some cases Origin should be configured properly (i.e. whitelist should be set up). So, since many people just turn on rack-protection and leave it as is (without figuring out what it really does), Origin may be a bad option to be enabled by default.

p0deje commented 11 years ago

I have added logging to HttpOrigin. @homakov Is it enough or you still would like to disable it by default?

homakov commented 11 years ago

@p0deje this logging already exists by default. i had logs on remote server so they were not helpful. I think changing Forbidden to Cross POST is forbidden is better help for developers :)

p0deje commented 11 years ago

@homakov I mean https://github.com/p0deje/rack-protection/commit/ee6720705846fbebb74c97132723e2f33969e200#L0R28

homakov commented 11 years ago

@p0deje yes i know, it already logs by default this way https://github.com/p0deje/rack-protection/blob/master/lib/rack/protection/base.rb#L45

p0deje commented 11 years ago

@homakov Ah, I didn't notice that. Wasn't it useful for you?

@rkh Can we merge this?

homakov commented 11 years ago

@p0deje hard question. i didn't even know that cross posts are denied - so i didn't read the logs.

p0deje commented 11 years ago

@rkh Do you think it should be merged or closed?

rkh commented 11 years ago

I don't know, I think this should be kept and only drop the session by default and you should remove it in your app or whitelist that endpoint. Though I am not sure if I'm right with that opinion.

homakov commented 11 years ago

until it gets more bug reports @p0deje i think it can be closed