Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protect request phase against CSRF. #1

Closed
wants to merge 3 commits into from

Conversation

DouweM
Copy link

@DouweM DouweM commented May 27, 2015

See omniauth/omniauth#809 for background.

There's a lot of duplication between this and Rails's request_forgery_protection.rb, but I don't see a way around that right now as Rails doesn't expose the RequestForgeryProtection functionality in any way that doesn't depend on the controller context.

Implementations of RequestForgeryProtection are included for Rails 3.2, 4.0, 4.1 and 4.2. I've only had time to test with Rails 4.1 yet.

cc @sferik @tenderlove @rafaelfranca @josevalim @carlosantoniodasilva

@sferik
Copy link
Contributor

sferik commented May 27, 2015

This looks good. My only remaining concern is the lack of tests. How can we be confident that this actually fixes what it’s supposed to?

@DouweM
Copy link
Author

DouweM commented May 27, 2015

How can we be confident that this actually fixes what it’s supposed to?

We can't, I agree we should have tests. request_forgery_protection_test.rb may come in useful.

I'll see if I can fit it into my schedule today or tomorrow, but unfortunately I have very little time this week.

@sferik
Copy link
Contributor

sferik commented May 28, 2015

@DouweM Ping. Any luck writing some tests for this? I was hoping to release it today. I fly to Kiev tomorrow for RubyC, so if it doesn’t happen today, it may have to wait until June 1st or 2nd.

@DouweM
Copy link
Author

DouweM commented May 28, 2015

@sferik Unfortunately not. I've got an exam on Monday, and I don't think my professor will find the fact that I'd rather be working on getting a vulnerability patch out of the door a good enough reason to postpone it. I won't have time to look into tests until June 2nd.

@sferik
Copy link
Contributor

sferik commented May 29, 2015

No problem. Good luck on your exam. 🍀

@reedloden
Copy link

Any update on this? As far as I know, this is still vulnerable.

@DouweM
Copy link
Author

DouweM commented Sep 23, 2016

@reedloden No update, I had it on my todo list for a little while last year, but other stuff kept pushing it down, so I ended up removing it and forgetting about it entirely...

@reedloden
Copy link

@DouweM, what do you need in order to move this forward?

@sdogruyol
Copy link

The original issue (omniauth/omniauth#809) is now assigned as CVE-2015-9284, thanks to @reedloden.

@DouweM do you have any plans of moving this forward? If not I'd be happy to hear about other alternatives.

@DouweM
Copy link
Author

DouweM commented Apr 16, 2019

@reedloden @sdogruyol I'm afraid I won't be able to move this forward; apologies for dropping the ball on this then and now.

The code in here is now pretty outdated, and we'd probably be better off with a solution that uses Rails's public interface instead of copy-pasting a bunch of Rails internals that may change from version to version, so I've closed this PR in the hope someone will create a new one we can actually move forward with.

@LucasArruda
Copy link

@sdogruyol @DouweM I'm happy to help to try fix this. I just don't know how to start but maybe we can work together or offer a clue?

@alexventuraio
Copy link

Is there any progress to test this gem to fix the vulnerability? 🤔

@bonsohi
Copy link

bonsohi commented Oct 29, 2019

Same here. Awaiting the fix - sooner than later! 😄

@alexventuraio
Copy link

Does anyone have any idea of the progress for this fix?
Or anybody else that had found a way to make it work in a Rails 5/6 app?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants