PHP Notice breaks scan

Hi @ov3rfly

the function is called in a for/each loop using the line and not the complete array, so it should be a string:
https://github.com/pluginkollektiv/antivirus/blob/dd4e4884ac0d71ae818cb449e024e3afac25770a/inc/class-antivirus-checkinternals.php#L109-L114

Which theme is causing this issue? Is this something public, so I can try to reproduce the issue?

And on which server/IS/filesystem are you experiencing this (Windows local, Mac local, Linux local, IIS, Apache, nginx, etc.)?

All the best,
Torsten

Analyzed the array, it appears to be the result of a get_option() call.

1. Put this as 1.php in any theme like twenty ninteen/twenty/twentyone:

<?php
$x = get_option('active_plugins');

2. Add a check at beginning of _check_file_line() similar to this:

if ( is_array( $line ) ) {
	die( print_r( $line ) );
}

3. Start a manual scan.

4. Watch ajax response in browser.

Something is seriously broken in current plugin logic.

• This reply was modified 4 years, 2 months ago by Ov3rfly.

You are absolutely right. Ran into the same issue yesterday when writing unit tests.

In fact the “broken“ logic is quite old (v1.3.10 code), wondering why it was never reported until now… Lower chance because fewer files were scanned before and likely the generous implicit string conversion in older PHP versions did the rest for most cases (value can be of an type).

No idea what the actual intention behind the iteration on the option value was, but nevertheless we will fix it.

Cheers,
Stefan

Thanks for quick feedback.

wondering why it was never reported until now…

My guess would be that many users simply install & activate this plugin and that’s it. They never start a scan or even open the configuration page. This renders the plugin pretty useless but users feel safe because they installed it and it is active. A similar feeling is generated when long outdated, incompatible and/or abandoned plugins show no updates in plugins list so users think everything would be up to date, a well known major flaw in WordPress plugin repository, but that’s a different story.

My second guess would be insufficient testing before releases. Previous reports in forums and also directly to various plugin authors showed that many authors don’t bother to even once enable WP_DEBUG or otherwise allow notices and warnings on their testservers.

Both guesses seem pretty unfair. Both points are correct and there have not been any automated tests before 1.4, but …

• there are many themes out there that actually do read options
• many options then are scalar, so silently convert to string
• it’s only a notice in PHP <= 7.4 and a warning in 8, so it does not actually break the check without WP_DEBUG and just prints a line to the log (though the line check doesn’t do anything useful in this case and the output is likely corrupted)

In fact I could not find any theme that reads array-options downloading random themes from the repo.
Chances are high that for 95% or even more of the users, this will never be a problem.

Sadly the intention behind the options check, as it is performed right now, is undocumented. It’s there since the very first SVN commit (11 years ago) without any comment. Maybe some kind of options injection, but re-evaluating some patterns on the options value does not make much sense in my eyes. Waiting for team feedback, but I personally would either drop or completely rethink this check.

In fact I could not find any theme that reads array-options downloading random themes from the repo.

• Airi in class-tgm-plugin-activation.php – get_option( ‘recently_activated’ )
• probably more themes which use TGMPA
• MH Magazine lite in functions.php and others (save customizer at least once) – get_option(‘mh_magazine_lite_options’)
• MH NewsDesk lite in functions.php and others (save customizer at least once) – get_option(‘mh_newsdesk_lite_options’)
• probably more MH Themes lite versions
• MH Magazine (not in repo) – in functions.php and others
• MH NewsDesk (not in repo) – in functions.php and others
• …

The author of the relevant plugin code was obviously aware that $line param also could be not a string, that’s why $line is always cast to string. Won’t argue about reasons why this has not been noticed or maybe has been noticed but not been tracked down before. It’s clearly a bug though. Thanks for supporting the plugin, I never used it before, just found it at two sites now in one week when being asked to fix issues of the sites.