gerobe

After fixing the two bugs already reported (missing sanitization.php and broken admin notice), I did a broader code review of 4.6.17 and found four more issues.

Bug 1 — Critical: Operator precedence error breaks service validation (shariff.php line 413)

The security check that prevents invalid service names from being executed has its logic inverted due to a PHP operator precedence issue. In PHP, ! has lower precedence than ===, so the condition skips valid services and executes invalid ones — the exact opposite of what was intended. The whitelist check is completely ineffective as written.

// actual code – logic is flipped
if ( !in_array( $service , $valid_services, true )===true ) continue;
// correct
if ( ! in_array( $service, $valid_services, true ) ) continue;

Bug 2 — Critical: Wrong array key corrupts migration data (updates.php lines 301–302)

When upgrading from a version older than 4.6.13 the migration routine that renames add_before[‘pages’] to add_before[‘page’] reads from a wrong array key. The two surrounding lines for ‘posts’ and ‘blogpage’ are correct — only the ‘pages’ line has the typo. For any user going through this migration path, the “add buttons before pages” setting is silently lost.

// actual code – reads from empty key, result is null
$shariff3uu_basic[‘add_before’][‘page’] = $shariff3uu_basic[”][‘pages’];
unset($shariff3uu_basic[”][‘pages’]);
// correct
$shariff3uu_basic[‘add_before’][‘page’] = $shariff3uu_basic[‘add_before’][‘pages’];
unset($shariff3uu_basic[‘add_before’][‘pages’]);

Bug 3 — High: Unvalidated $_GET[‘tab’] passed to WordPress core functions (admin-menu.php line 2207)

The active tab is read directly from $_GET[‘tab’] without sanitization or validation and then passed to settings_fields() and do_settings_sections(). A crafted URL sent to a logged-in admin could cause arbitrary settings groups to be rendered. Fix: strict allowlist + sanitize_key().

// actual code
$active_tab = $_GET[‘tab’];
// correct
$allowed_tabs = array( ‘shariff3uu_basic’, ‘shariff3uu_design’, ‘shariff3uu_advanced’,
‘shariff3uu_statistic’, ‘shariff3uu_help’, ‘shariff3uu_status’, ‘shariff3uu_ranking’ );
$active_tab = ( isset( $_GET[‘tab’] ) && in_array( sanitize_key( $_GET[‘tab’] ), $allowed_tabs, true ) )
? sanitize_key( $_GET[‘tab’] ) : ‘shariff3uu_basic’;

Bug 4 — Low: Integer comparisons against string literals (shariff.php, TTL logic)

After absint() produces an integer it is compared against quoted string numbers in 8 places. PHP coerces types silently so no wrong result occurs, but it is technically incorrect.

// actual code
if ( $ttl < ’61’ ) { $ttl = ’60’; }
if ( $ttl > ‘7200’ ) { $ttl = ‘7200’; }
// correct
if ( $ttl < 61 ) { $ttl = 60; }
if ( $ttl > 7200 ) { $ttl = 7200; }

Summary:

• Bug 1 — shariff.php:413 — Critical — Service whitelist check completely inverted
• Bug 2 — updates.php:301 — Critical — “Add before pages” setting lost on migration
• Bug 3 — admin-menu.php:2207 — High — Unvalidated user input passed to WP core
• Bug 4 — shariff.php (8 places) — Low — Integer vs. string comparisons in TTL logic

The changelog for 4.6.17 states “sanitize short code uses the same checker like admin page now”. This means the sanitization functions were correctly refactored out of the admin page into a new shared file includes/sanitization.php — so that both the admin settings page and the [shariff] shortcode renderer run through identical validation. However, the file was not included in the released plugin package. The includes/ directory ships only phpqrcode.php and class-shariff-widget.php.

The missing file must define five functions called throughout the plugin:FunctionUsed instrip_hackers()shariff.php (9 call sites)shariff3uu_basic_sanitize()shariff.php:886, admin/admin-menu.php:59shariff3uu_design_sanitize()shariff.php:887, admin/admin-menu.php:117shariff3uu_advanced_sanitize()shariff.php:888, admin/admin-menu.php:278shariff3uu_statistic_sanitize()shariff.php:889, admin/admin-menu.php:417

Also relevant to the BTC fix in this release: The changelog states “update BTC address checks to work with recent schemes”. The regex in bitcoin.php already validates both legacy addresses (1…/3…) and native SegWit bech32 addresses (bc1…). The same regex needs to be applied in shariff3uu_advanced_sanitize() when storing the bitcoinaddress option — otherwise the admin page accepts addresses that bitcoin.php would reject (or vice versa). Without sanitization.php this tightened check never runs at all.

Steps to reproduce:

1. Update to Shariff Wrapper 4.6.17
2. Any page load — site is immediately inaccessible

Fix: Please add includes/sanitization.php to the plugin package and release 4.6.18.

The changelog made it clear this was a packaging mistake, not a logic bug — the file was written (the refactoring is correctly referenced everywhere in the code) but simply wasn’t committed/included in the zip. The Bitcoin address regex in bitcoin.php:18 is already the updated one; the sanitization.php I reconstructed uses the exact same pattern.

Thank you @bgermann !

Good to know you will look into it.

Thank you for the quick fix!
With version 2.18 the archive and the subcribe-forms are working again.

Except: When using the date-placeholders in a newsletter (e. g. [date:d] [date:y] …) the newsletter in the archive always shows todays date and not the date the newsletter was sent.

Thank you!

Hello,
the Test Helper seems to have worked. Thank you!