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 creating authentication cookies when inappropriate to do so. #490

Closed
wants to merge 8 commits into from

Conversation

dd32
Copy link
Member

@dd32 dd32 commented Nov 3, 2022

As noted in #406 and #385 it's possible for plugins to accidentally bypass the two-factor requirement by setting cookies directly.

See #406 (comment) for more detailed reasonings.

Fixes #406, #385

@kasparsd
Copy link
Collaborator

kasparsd commented Nov 3, 2022

This appears to be impacting the core workflows used by this plugin. Could we please add some unit tests to go with these changes?

@dd32
Copy link
Member Author

dd32 commented Nov 16, 2022

Could we please add some unit tests to go with these changes?

I wasn't able to determine how best to add unit testing here, given the plugin doesn't appear to currently test it.

Do you have any suggestions @kasparsd?

@iandunn
Copy link
Member

iandunn commented Nov 18, 2022

#497 might be a better way to test it. I don't think that needs to be a blocker for this, though. We could just test this manually and then add the e2e tests once the tooling is in place (maybe as part of #468 )

@iandunn
Copy link
Member

iandunn commented Dec 1, 2022

This seems to work in my manual tests. @kasparsd , do you have any thoughts on the 2 comments above about automated tests?

class-two-factor-core.php Outdated Show resolved Hide resolved
Co-authored-by: Ian Dunn <ian@iandunn.name>
@dd32
Copy link
Member Author

dd32 commented Jan 5, 2023

Closing in favour of #502 which has been merged.

@dd32 dd32 closed this Jan 5, 2023
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