Fix improper state handling #417
Conversation
joshcanhelp
force-pushed
the
fix-state-handling-for-shortcode
branch
from
81a4456
to
b0f3707
Compare
WP_Auth0.php
Outdated
| @@ -120,6 +120,9 @@ public function init() { | |||
|
|
|||
| $this->check_signup_status(); | |||
|
|
|||
| // Store nonce value before headers are sent | |||
| new WP_Auth0_State_Handler(); | |||
There was a problem hiding this comment.
why isn't this being assigned anywhere? If this is some kind of "static helper" maybe you'd want to have a singleton to access it instead of creating new instances.
lib/WP_Auth0_State_Handler.php
Outdated
| * | ||
| * @return string | ||
| */ | ||
| public function issue() { |
There was a problem hiding this comment.
isn't this a breaking change?
There was a problem hiding this comment.
Has not gone out yet
lib/WP_Auth0_State_Handler.php
Outdated
| @@ -72,7 +77,7 @@ public static function reset() { | |||
| * | |||
| * @return string | |||
| */ | |||
| public static function generateNonce() { | |||
| protected function generateNonce() { | |||
There was a problem hiding this comment.
isn't this a breaking change?
There was a problem hiding this comment.
Has not gone out yet
lib/WP_Auth0_State_Handler.php
Outdated
| @@ -40,6 +44,7 @@ public function issue() { | |||
| * @return bool | |||
| */ | |||
| protected function store() { | |||
| $_COOKIE[ self::COOKIE_NAME ] = $this->uniqid; | |||
There was a problem hiding this comment.
who was setting this value to $_COOKIE previously?
There was a problem hiding this comment.
setcookie() (PHP core) does the work sending the header but it doesn't set the global for use later. I need to check for that cookie later on during processing
joshcanhelp
force-pushed
the
fix-state-handling-for-shortcode
branch
4 times, most recently
from
32df76e
to
6d05e0a
Compare
State generation, specifically cookie storage, needs to happen before any output. The previous arch was ok for login page generation but failed with a "headers already set" error if used in the shortcode. This addresses the issue by storing state earlier and getting the value later.
joshcanhelp
force-pushed
the
fix-state-handling-for-shortcode
branch
from
6d05e0a
to
1b344ea
Compare
joshcanhelp
changed the title
joshcanhelp
force-pushed
the
fix-state-handling-for-shortcode
branch
2 times, most recently
from
7713d2b
to
bcb9eac
Compare
| * @return bool | ||
| */ | ||
| protected function validate_state() { | ||
| $valid = isset( $_COOKIE[ WPA0_STATE_COOKIE_NAME ] ) |
There was a problem hiding this comment.
Are we going to force people to set the state then? Or better put.. is this library setting a new state on every request?
There was a problem hiding this comment.
Setting a new state when initializing Lock, yes
lib/WP_Auth0_Nonce_Handler.php
Outdated
| * | ||
| * @var string | ||
| */ | ||
| const COOKIE_NAME = 'auth0_uniqid'; |
There was a problem hiding this comment.
maybe you want to use "auth0_nonce" here?
lib/WP_Auth0_Nonce_Handler.php
Outdated
| final class WP_Auth0_Nonce_Handler { | ||
|
|
||
| /** | ||
| * Cookie name used to store unique state value |
There was a problem hiding this comment.
"unique state value" shouldn't be "nonce"?
lib/WP_Auth0_Nonce_Handler.php
Outdated
| private $_uniqid; | ||
|
|
||
| /** | ||
| * WP_Auth0_State_Handler constructor |
lib/WP_Auth0_Nonce_Handler.php
Outdated
| // Have a state cookie, don't want to generate a new one | ||
| $this->_uniqid = $_COOKIE[ self::COOKIE_NAME ]; | ||
| } else { | ||
| // No state cookie, need to create one |
lib/WP_Auth0_Nonce_Handler.php
Outdated
| } | ||
|
|
||
| /** | ||
| * Return the unique ID used for state validation |
lib/WP_Auth0_Nonce_Handler.php
Outdated
| } | ||
|
|
||
| /** | ||
| * Check if the stored state matches a specific value |
lib/WP_Auth0_Nonce_Handler.php
Outdated
| /** | ||
| * Check if the stored state matches a specific value | ||
| * | ||
| * @param $state |
lib/WP_Auth0_Nonce_Handler.php
Outdated
| * | ||
| * @return bool | ||
| */ | ||
| public function validate( $state ) { |
| */ | ||
| protected function get_state() { | ||
| protected function get_state( $decoded = FALSE ) { |
There was a problem hiding this comment.
why would I need the "encoded" state? URL encoding a state shouldn't be responsibility of this class, but the class who constructs the requests.
There was a problem hiding this comment.
References base64 encoded, not URL. URL decoding has to happen, this is just basically "do you want the raw value or the object"
There was a problem hiding this comment.
Why would I need it to be base64 encoded? The form of the nonce or state values should be obscure. I don't care how you generate it internally as long as it's "safe and secure". (guarantying that it's too hard to repeat itself in a short period of time, etc).
And TBH, the server doesn't care either. It's a plain text string that it needs to put somewhere so the client can later obtain it and validate it with the one it sent before. So unless you have a reason why to have a method supporting both encoded/decoded scenarios, I think it's useless.
There was a problem hiding this comment.
It's base64 encoded when it's created in WP_Auth0_Lock10_Options->get_state_obj() ... more specifically, it's a JSON encoded object that's then base64 encoded. That's what I store in a cookie then validate during the login process, still base64 encoded. Once someone is logged in, then I need that object I created before to get a redirect URL.
If you look for where get_state is used in this class, you'll see what I mean
Originally was generating and setting a nonce value in a state object, then checking that nonce on return from Auth0 instead of checking the entire state parameter.
joshcanhelp
force-pushed
the
fix-state-handling-for-shortcode
branch
from
bcb9eac
to
fdcd069
Compare
State generation, specifically cookie storage, needs to happen before
any output. The previous arch was ok for login page generation but
failed with a "headers already set" error if used in the shortcode.
This addresses the issue by storing state earlier and getting the
value later.