The-seo-framework: AMP plugin, pre_get_document priority problem

[infokurs]

Hi,

1. ln. 1640
   add_filter( 'pre_get_document_title', 'ampforwp_add_custom_title_tag', 10 );
   https://github.com/ahmedkaludi/accelerated-mobile-pages/blob/master/templates/features.php

2. ln. 322
   \add_filter( 'pre_get_document_title', array( $this, 'title_from_cache' ), 10 );
   https://github.com/sybrew/the-seo-framework/blob/master/inc/classes/init.class.php

Problem: same default priority and TSF filter fired before AMP filter, so don't use right title.
Question: is possible to bump priority in TSF eg. to 11 or better 99 ?

[sybrew]

Hi, @infokurs,

Moving priorities can yield unexpected results with other plugins, that would work perfectly fine as of now.

Because the AMP plugin creates a preemptive solution with compatibility layers towards SEO plugins, it might be best mentioned to lower their priority. Plugins that enact as theme/template-holders should, in my opinion, abide by the theme standards.

That said, the snippet below should solve this issue (in a straight forward way) by removing the filter right after it's being implemented:

add_action( 'pre_amp_render_post', function() {
    remove_filter( 'pre_get_document_title', 'ampforwp_add_custom_title_tag', 10 );
}, 11 );

[infokurs]

Thanks for the clarification and idea with filter removal. That's work.
Yeah, weird, shitty AMP plugin code ;p

[sybrew]

I can't really blame them, they're working very hard implementing a large number of features 😄.
But sometimes you need to sit back and write down what you want to achieve before implementing 😃,

We've also had multiple discussions regarding the title implementation. At this point, everything worked as-is: https://github.com/ahmedkaludi/accelerated-mobile-pages/pull/761

In any case, could you name what's wrong in this form:

1. The title you expected.
2. The title you got instead.

[sybrew]

I could add a fix for this in the AMP extension.

[sybrew]

The AMP for WP plugin is being developed very actively. We'll have to retest this.

[sybrew]

I'm not going to implement any features for their plugin until they fix their security issues.

I've found various exploitable points in a twenty-minute scan, these exploits notoriously include file injections, backdoor file downloading (including wp-config.php), DDoS vulnerability, database upgrading, options-and post-metadata overwriting, bandwidth exploitation (full WP media-library downloads), and unfiltered WordPress post injections.

All these exploits do not require any administrative privileges.

Aside from this, they embed the Redux framework, but they're not keeping it fully up-to-date. I didn't bother scanning this thorougly, but you can also adjust a few site options in there.

If you value your website, NEVER use the "AMP for WP – Accelerated Mobile Pages" plugin found here: https://wordpress.org/plugins/accelerated-mobile-pages/

I don't often call out on others, but the developers clearly have no clue on what they're doing.
Such combobulating actions will lead to incompatibilities at any version, and it's a waste of my time.

[infokurs]

This plug from the outside is pretty and simple, from the inside it contains a low-quality code full of errors. Authors are not even able to properly use the basic functions of PHP and WordPress, they do not stick to any standards and good practices.
Frequent updates damage more than they repair, and serious problems can not be understood and corrected so far.
I have not used this plugin for a month now, I do not have the strength to fight this crap.

@sybrew, push security note to: plugins [at] wordpress.org ;)
And 4fun, You can submit https://wpvulndb.com/submits/new

[sybrew]

If I had the time, I'd do that 👍 Let's just hope a white-hat hacker finds these comments before anyone else. I'll send a short list of exploitable hooks to the plugin team, and ask them to do a full review.

[JayHoltslander]

I'm not going to implement any features for their plugin until they fix their security issues.

I've found various exploitable points in a twenty-minute scan, these exploits notoriously include file injections, backdoor file downloading (including wp-config.php), DDoS vulnerability, database upgrading, options-and post-metadata overwriting, bandwidth exploitation (full WP media-library downloads), and unfiltered WordPress post injections.

All these exploits do not require any administrative privileges.

Aside from this, they embed the Redux framework, but they're not keeping it fully up-to-date. I didn't bother scanning this thorougly, but you can also adjust a few site options in there.

If you value your website, NEVER use the "AMP for WP – Accelerated Mobile Pages" plugin found here: https://wordpress.org/plugins/accelerated-mobile-pages/

I don't often call out on others, but the developers clearly have no clue on what they're doing.
Such combobulating actions will lead to incompatibilities at any version, and it's a waste of my time.

Whoa. Had no ideas until now that there were so many issues. Spotted this today which led me here.

[sybrew]

They fixed the security issues 😄. I'm going to take a step back and have removed the milestone.
If I see the plugin evolving at a more consistent and robust pace, I'll reconsider implementing a patch.

[sybrew]

Cross-issue: https://github.com/ahmedkaludi/accelerated-mobile-pages/issues/2670
For the title issue, see my comment above: https://github.com/sybrew/the-seo-framework/issues/203#issuecomment-335670381

I'm unaware of other issues, I've just asked for more information, however.

[MARQAS]

Hi there @sybrew,
Are you sure that this filter add_filter( 'pre_get_document_title', 'ampforwp_add_custom_title_tag', 10 ); is causing the wrong title in non-amp as well?
We are running that filter inside pre_amp_render_post which will change the title only in AMP, so I guess @infokurs is talking about the titles in AMP.
If that's the case as well then no worries, I have added the compatibility, now it will take the title from this plugin only if it is active.
@sybrew can you check it once and let us know if we are missing something?

[sybrew]

Hi @MARQAS

Thanks for following up!

There weren't any issues on non-amp.

I just tested your latest commit at 88f92bb, and everything works as intended.
Thank you for implementing it! 🏆

Just a heads-up: I am planning on changing the class structure. This minor alteration in the availability check should work for at least another five years, in file includes/features/functions.php:259:

        if ( function_exists( 'the_seo_framework' ) && 5 == $redux_builder_amp['ampforwp-seo-selection'] ) {
            $tsf_desc = $ampforwp_tsf = '';
            $ampforwp_tsf   = \the_seo_framework();
            $tsf_desc       = $ampforwp_tsf->get_description();
            if ( $tsf_desc ) {
                $desc = $tsf_desc;
            }
        }

Fun fact: calling anything unassigned inside the_seo_framework() won't result in a fatal error; the object forwards it to the "doing it wrong" error handler instead. So, method_exists() won't be needed.

[MARQAS]

Thanks, @sybrew, I made the change which you have suggested.

[sybrew]

Everything seems to be working as intended. Thanks again for updating 👍