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

Ensure mock call verifies authenticity tokens with same logic as real call #1033

Merged
merged 1 commit into from Feb 2, 2021

Conversation

jsdalton
Copy link
Contributor

@jsdalton jsdalton commented Feb 2, 2021

Readjusts when the authenticity token verification happens in #mock_call to align with the execution of the real call.

Purpose

The current implementation of mock_call was verifying the token for all requests, regardless of whether the current path is on the omniauth request path. The change was introduced recently in 1b784ff. See #1032 for details.

This creates two problems:

  1. When test mode is on, the authenticity verification logic is run inappropriately against requests where this may not even be wanted.
  2. The behavior varies from actual production behavior, potentially allowing bugs to be introduced by unwary developers.

Approach

  • Move the request validation logic into #mock_request_call so that it tracks more closely with the real validation logic that is triggered from #request_call.
  • Add a spec test to capture the regression

Please feel free to introduce this change in a different manner if needed. My main interest is unblocking a project I have where the newly introduced logic change is breaking many tests. We are disabling the request_validation_phase in all tests, but would like to re-enable it for integration tests to ensure our tests specify the behavior correctly.

Copy link
Member

@BobbyMcWho BobbyMcWho left a comment

Choose a reason for hiding this comment

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

Appreciate the issue report and PR @jsdalton, I think this also may relate to #1030

@BobbyMcWho BobbyMcWho merged commit 6c822c0 into omniauth:master Feb 2, 2021
@BobbyMcWho
Copy link
Member

This has been tagged and released to rubygems as v2.0.2

@jsdalton
Copy link
Contributor Author

jsdalton commented Feb 2, 2021

Thanks for the quick merge and update @BobbyMcWho. 🙇

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

2 participants