search

Yoast / wordpress-seo

Yoast SEO for WordPress
https://yoast.com/wordpress/plugins/seo/
Other
1.76k stars 887 forks source link

YOAST + Elementor + ACF Pro: Infinite Loop with wp_insert_post() - [Edge Case] #14643

Open acf-extended opened 4 years ago

acf-extended commented 4 years ago

Please give us a description of what happened.

I found a compatibility problem between Elementor, YOAST and ACF Pro. While this is an edge case, I think it should be looked at because it could cause some problems with others plugins.

Note: This is a cross-bugreport on both Elementor & YOAST github repo. Elementor bugreport is available here.

How can we reproduce this behavior?

add_action('wp', function(){

    if(isset($_POST['test']) && !empty($_POST['test'])){

        if(have_rows('repeater', 32)):
            while(have_rows('repeater', 32)): the_row();

                // Log
                error_log('wp_insert_post');

                // wp_insert_post
                wp_insert_post(array(
                    'post_title' => 'Page',
                    'post_type' => 'page',
                ));

                // Inifinite loop...

            endwhile;
        endif;

    }

});

add_shortcode('test', function(){

    if(have_rows('repeater', 32)):
        while(have_rows('repeater', 32)): the_row();

            // Do something with the repeater ...

        endwhile;
    endif;

    ob_start();

    ?>
    <form action="" method="post">

        <input type="hidden" name="test" value="1" />
        <button>Submit</button>

    </form>
    <?php

    return ob_get_clean();

});

The fundamental problem:

On each post creation, YOAST check the new post content for their "Link Checker" feature using the save_post hook (which is triggered inside wp_insert_post()).

When YOAST do that check, it retrieve the post content using apply_filters('the_content'). But Elementor add specific filters to the_content, and load all components and shortcodes. In our case, applying the shortcodes will print the form which trigger an infinite loop.

Potential bugfix

I found a workaround by disabling the YOAST "Link Checker" using the following hook, right before the wp_insert_post():

add_filter('wpseo_should_index_links', '__return_false');

However, this can also be fixed if we remove the filter applied on the_content in the file: /elementor/includes/frontend.php:273. The way it is implemented right now means that Elementor load all components & shortcodes when using wp_insert_post() in conjunction with YOAST, which seems problematic.

Technical info

Used versions

ite-klass commented 3 years ago

I think we have run into this issue with a slightly different setup. Took me a while to find the issue of this intransparent infinite loop.

As we verified this issue exists both on older versions and current versions I will list both:

The jobs plugin adds a form which allows for job applications to be sent in. These are created as posts with sub-type awsm_job_application (post_type in DB, object_sub_type on $indexable in the indexable link builder).

The form may be sent in via ajax, which is not a problem, or POST to the page, which causes the infinite loop.

I don’t see why the behavior should be different if YOAST SEO wants to update link metadata for new posts?

My understanding is that build_indexable is hooked into/for wp_insert_post. In https://github.com/Yoast/wordpress-seo/blob/trunk/src/integrations/watchers/indexable-post-watcher.php#L205 it identifies and changes content, and saves it at https://github.com/Yoast/wordpress-seo/blob/trunk/src/integrations/watchers/indexable-post-watcher.php#L207. This causes another wp_insert_post which triggers the hook yet again. This loop is triggered indefinitely.

So it seems to me that this hook is missing an adequate repetition/handled check.

Possibly it should only index posts of known and accepted post subtypes?

ite-klass commented 3 years ago

The result of this is thousands of junk/duplicate entries/posts btw. Cleaning those up is infeasible for an admin user (due to paging and bin) and has to be cleaned up by a DB admin every time it happens. So the impact of this hole is quiet high and bothersome.

The hook not only loops, but creates a new post every loop. It’s a busy loop too, which has potential for DoS attacks.

For that reason I would not agree with severity minor.

Djennez commented 3 years ago

If I read the original issue correctly, the problem here is the fact that potential looping code is executed when the_content filter is called. We call the_content to check the post for specific content that would be rendered when the post is live, which, I believe, is a normal usage of this filter. With WordPress being the pluggable and modular system that it is, I am having a hard time imagining no other plugins that would / could be doing the same thing and potentially triggering this loop as well. I feel it's the responsibility of the developer of the looping code to account for a possible use of the_content filter and make sure it does not cause loops.

Please provide additional thoughts on the matter. Since this is an edge-case that may not be caused by our usage of code, it currently does not have priority on our side to be picked up. But if any information is provided that would indicate that we would have to change something (or my assumption from before is incorrect), we can definitely look into this more.