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

Prevent timing attack on CSRF, completing wonderful pr by @eutopian #174

Merged
merged 4 commits into from
Oct 12, 2023

Conversation

jhartzler
Copy link

@jhartzler jhartzler commented Sep 27, 2023

Attempting to complete the CSRF fix introduced in #123 by @eutopian
It looks like that PR has been untouched since 2021 so opening a new one that is up to date with latest master and implements the renamed test that was suggested

@jhartzler
Copy link
Author

@BobbyMcWho @nov do either of you have write access? 🙏
New contributor just trying to fix a vuln, if you are around thank you so much in advance for your time!

@jhartzler
Copy link
Author

Let me fix this build error

@@ -83,8 +83,7 @@ def token_params

def callback_phase # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/MethodLength, Metrics/PerceivedComplexity
error = request.params["error_reason"] || request.params["error"]
if !options.provider_ignores_state && (request.params["state"].to_s.empty? || request.params["state"] != session.delete("omniauth.state"))
fail!(:csrf_detected, CallbackError.new(:csrf_detected, "CSRF detected"))
elsif !options.provider_ignores_state && (request.params["state"].to_s.empty? || !secure_compare(request.params["state"], session.delete("omniauth.state")))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
elsif !options.provider_ignores_state && (request.params["state"].to_s.empty? || !secure_compare(request.params["state"], session.delete("omniauth.state")))
if !options.provider_ignores_state && (request.params["state"].to_s.empty? || !secure_compare(request.params["state"], session.delete("omniauth.state")))

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review. Looks like I didn't copy from the initial PR properly

@jhartzler
Copy link
Author

Running specs locally it looks like there might be some more work to reconcile this PR with master. I'm looking into it

@jhartzler
Copy link
Author

Alright everything should be fixed now as far as I can tell. Specs pass locally.

@jhartzler
Copy link
Author

@BobbyMcWho thank you for the earlier review and code suggestion
Not sure what your cadence/availability is but when you get a chance could I get a workflow kickoff and a re review?

@jhartzler
Copy link
Author

I will buy someone a coffee if they review this PR lol

@BobbyMcWho
Copy link
Member

No coffee needed, I don't mind merging, but the whole release process is the headache

@BobbyMcWho BobbyMcWho merged commit c830138 into omniauth:master Oct 12, 2023
25 checks passed
@jhartzler
Copy link
Author

Haha thank you for the review + merge
I'm guessing there's nothing I can do to help the release process along?

@BobbyMcWho
Copy link
Member

I just haven't released on this laptop yet so I gotta do some pre requisite installs, that's all

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

Successfully merging this pull request may close these issues.

2 participants