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

Block sending authentication cookies upon 2FA login #502

Merged
merged 5 commits into from
Jan 5, 2023

Conversation

dd32
Copy link
Member

@dd32 dd32 commented Dec 7, 2022

Based on the feedback in #406 and limited interest on changing how the login flow works in #490, this PR is a minimal patch which prevents sending the authentication cookies upon hitting the 2FA interstitial page.

As there's limited changes in this PR, this should be easier for others to review and approve.

Copy link
Collaborator

@kasparsd kasparsd left a comment

Choose a reason for hiding this comment

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

This looks good! Fewer moving parts :)

Haven’t tested it though.

@jeffpaul jeffpaul added this to the 0.8.0 milestone Dec 7, 2022
Copy link
Member

@iandunn iandunn left a comment

Choose a reason for hiding this comment

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

Tested w/ these steps:

  1. Checkout master
  2. Add this code to an mu-plugin:
    add_filter( 'wp_login', function() {
    	trigger_error( 'test', E_USER_ERROR );
    }, 9 );
  3. Log out, and then back in again with a user who has 2FA enabled. You should see the fatal error.
  4. inspect cookies in browser, verify they were sent
  5. Checkout this branch
  6. Repeat steps and verify they weren't sent

LGTM 👍🏻

@dd32 dd32 force-pushed the fix/406-alt-block-send_auth_cookies branch from 1b78334 to 9527cec Compare January 4, 2023 03:17
@iandunn
Copy link
Member

iandunn commented Jan 4, 2023

@dd32 is there anything left that's needed to merge this?

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