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

Add check for GET and POST globals for state validation #707

Merged
merged 2 commits into from
Aug 1, 2019

Conversation

joshcanhelp
Copy link
Contributor

@joshcanhelp joshcanhelp commented Aug 1, 2019

Changes

Add checking for state in other PHP globals in case $_REQUEST is not set.

Testing

  • This change adds unit test coverage
  • This change has been tested on WP 5.2.2

Checklist

  • All existing and new tests complete without errors
  • All code quality tools/guidelines in the Contribution guide have been run/followed
  • All active GitHub CI checks have passed

@joshcanhelp joshcanhelp added this to the 3.11.1 milestone Aug 1, 2019
@joshcanhelp joshcanhelp requested a review from a team August 1, 2019 16:36
@joshcanhelp joshcanhelp changed the title Add check for GET and POST globals Add check for GET and POST globals for state validation Aug 1, 2019
@@ -639,6 +639,12 @@ protected function query_vars( $key ) {
if ( isset( $_REQUEST[ $key ] ) ) {
return $_REQUEST[ $key ];
}
if ( isset( $_GET[ $key ] ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this function only used in the WP_Auth0_LoginManager above?

I'm wondering if now accepting values from GET or POST as well as the previous objects opens up any kind of exploits. i.e. Perhaps we were validating some parameter we got posted back from auth but now the user could pre-inject a value on the URL so when we get redirected back we take the value from there rather than from the POST...

I think this function is a bit dangerous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this function only used in the WP_Auth0_LoginManager above?

Yes, protected.

I totally understand your concerns here and thought about implications. It's used to:

  • Get an auth0 parameter, which just triggers callback processing
  • Checks for a state param, which is validated one time against a stored value
  • Gets a code parameter which is exchanged immediately for tokens

The existing checks can only be internally set in WordPress, the ones I'm adding would just roll up into those if they are present (if everything goes according to plan) so no additional risk added here.

I'll address this in the upcoming, I agree that we should be looking for these values only where we expect them.

@joshcanhelp joshcanhelp merged commit 3bfea90 into wordpress-org-plugin Aug 1, 2019
@joshcanhelp joshcanhelp deleted the fix-state-handling branch August 1, 2019 17:00
joshcanhelp added a commit that referenced this pull request Aug 1, 2019
joshcanhelp added a commit that referenced this pull request Aug 2, 2019
@joshcanhelp joshcanhelp removed this from the 3.11.1 milestone Aug 2, 2019
@joshcanhelp joshcanhelp added this to the 4.0.0 milestone Feb 7, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants