reduxframework / redux-framework-4

Redux v4
https://redux.io/redux-4/
Other
97 stars 32 forks source link

Conflict with LifterLMS after 4.1.26 update in Redux #215

Closed pablodiloreto closed 3 years ago

pablodiloreto commented 3 years ago

Hello Team! I use vLog wordpress theme that use "Redux Framework plugin". I use, also, LifterLMS plugin (lifterlms.com, in github https://github.com/gocodebox/lifterlms/).

After last updates in Redux, i started received an error "200 parsererror" in screen. If I disable or rollback Redux Framework plugin to 4.1.24 as workaround, the other plugin started working fine. I rollbacked LifterLMS to older version (to check that the problem is not in this plugin) but, with new version of Redux Framework, this error still appear. I opened a case in LifterLMS (please see https://github.com/gocodebox/lifterlms/issues/1610) and they recommend me to open a case here too.

With version 4.1.26 in my wordpress, when I see in the AJAX XHR tab in my browser, I have a 200 response but no response from server appear (but the "200 parsererror" appear in screen).

With version 4.1.24 in my wordpress, when I see in the AJAX XHR tab in my browser a normal server response appear.

Is any possibility to help this issue? my conclusion is that there is a conflict between these plugins, after the redux update.

Thank you.

thomasplevy commented 3 years ago

As the lead developer at LifterLMS I'd like to weigh in that we're using the WP heartbeat API on this particular screen. We haven't determined exactly what's going on in Redux to create the empty AJAX response but I wanted to note what APIs we're using (if it helps)

Additionally, if there is something that we're doing wrong we're happy to resolve this from our side but at this moment it feels like this is outside the realm of our control. I haven't completely unpacked the issue though.

Thanks!

kprovance commented 3 years ago

@thomasplevy wroite:

As the lead developer at LifterLMS I'd like to weigh in that we're using the WP heartbeat API on this particular screen. We haven't determined exactly what's going on in Redux to create the empty AJAX response but I wanted to note what APIs we're using (if it helps)

Additionally, if there is something that we're doing wrong we're happy to resolve this from our side but at this moment it feels like this is outside the realm of our control. I haven't completely unpacked the issue though.

Thanks!

I've recently taken over debugging duties, which I did for some six years for Redux before leaving and coming back. To begin to dig into this, I need to reproduce as best as possible the OP's environment. I see I can get your plugin via Github. What about the theme they are using? Is that a public theme or a premium theme? Any demo data or just start from scratch? Access to the theme in questionit would speed things up. :)

Thanks!

thomasplevy commented 3 years ago

@pablodiloreto :arrow_up:

@kprovance see the system report on thei initial issue we opened up about this: https://github.com/gocodebox/lifterlms/issues/1610

kprovance commented 3 years ago

I read it. He says he's using the vLog theme, which is a premium theme and not something I have access to. This will make it difficult for me to reproduce. The Law of Least Effort applies. ;-)

kprovance commented 3 years ago

@pablodiloreto , @thomasplevy

Okay, it took me a few hours, but I finally got to the bottom of this. First, I have no idea why this became an issue with 4.1.26. It should have shown up a LOT sooner, since the code causing the issue has been in Redux 4 since the beginning. I know, because I wrote it back in 2018. ;-)

public static function is_heartbeat() {

    // Disregard WP AJAX 'heartbeat'call.  Why waste resources?
    if ( isset( $_POST ) && isset( $_POST['_nonce'] ) && wp_verify_nonce( sanitize_key( wp_unslash( $_POST['_nonce'] ) ), 'heartbeat-nonce' ) ) {
        if ( isset( $_POST['action'] ) && 'heartbeat' === sanitize_text_field( wp_unslash( $_POST['action'] ) ) ) {
            // Hook, for purists.
            if ( has_action( 'redux/ajax/heartbeat' ) ) {
                do_action( 'redux/ajax/heartbeat' );
            }

            return true;
        }

        return false;
    }

    // Buh bye!
    return false;
}

The objective there was to filter out the heartbeat API running the entirety of the Redux core every 30 seconds or so, which was causing performance issues. It's also why we added an action hook, just in case.

The issue comes from the LMS plugin attaching a different nonce to the heartbeat action. As noted in the code above, we are doing our security check with the heartbeat based on the nonce string used in the WordPress core. I'm not sure what string your plugin uses, but it returned a completely different nonce, and the code above it dismissing it. And herein lies the conundrum. How does this get fixed? (Again, before anyone asks why did this show up all of a sudden, I have no idea. It's irrelevant anyways. I returned out of that function before the nonce check and everything worked.)

I may have to get with some of the WP Core team and ask what the standard here, and please understand, I take security and WordPress Coding Standard VERY seriously. That said, I'm not willing to take out the nonce check as it works based on how WordPress designed it, using their heartbeat nonce. Conversely, I noticed with the LMS plugin, when clicking the save button in the builder, it attaches its own nonce to the heartbeat action, and why Redux is barking the way it is.

I'm not sure what the fix is here.

thomasplevy commented 3 years ago

Hey @kprovance

LifterLMS isn't overwriting or using a different or custom nonce. I'm struggling to understand how a check to stop redux from running would cause any issues with the response of the heartbeat itself.

How did you determine that the nonce was being overwritten by a different nonce that LifterLMS is using?

I've run some tests and the nonce passed with the heartbeat POST request is the nonce obtained from the JS localization in the WP core. We're just attaching data to the heartbeat as documented at https://developer.wordpress.org/plugins/javascript/heartbeat-api/

Screenshot_2021-06-21_17-00-30

Could it be that you ran a test on an expired nonce? The heartbeat API does automatically refresh it's own nonces (passing them back with the response and updating the variables used to pass more nonces again in the future)

kprovance commented 3 years ago

This fix for this is available from this repo. It'll be available on the wp.org repo for the July update. Thanks!

thomasplevy commented 3 years ago

@kprovance thanks!

pablodiloreto commented 3 years ago

Thanks @kprovance & @thomasplevy :-)

webbudesign commented 3 years ago

@pablodiloreto, @thomasplevy I discover a problem with the wp cron job system after your code change request. Please read my issue report and let @kprovance about your decision on this case. My request: https://github.com/reduxframework/redux-framework/issues/3821

The cronjob area was an existing feature since the redux framework was built.