skills(wordpress-plugin-conventions): merge security/lifecycle checks from wordpress/agent-skills
Per the 2026-08-15 skills.sh and autoskills.sh scans, both flagged wordpress/agent-skills' wp-plugin-development module (Automattic-origin, now WordPress-org-hosted) as high-value source material for iWP's plugin skill: nonce+capability dual-check discipline, late escaping, prepared SQL, cron idempotency, and uninstall-vs-deactivation guardrails. Adapted (not copied) against real iWP plugin code in wp-plugins/: - nonce+capability must-both framing, cited against class-iwp-cache-db-cleanup.php's actual AJAX handler - late-escaping and wp_unslash()/explicit-key superglobal reading - %i identifier-placeholder version gate (WP 6.2+, most iWP plugins floor at 6.0 or lower) - new "Admin settings" section documenting the real Settings-API vs. AJAX-dashboard split across the suite, since the source's generic Settings-API-first prescription doesn't match roughly half of iWP's plugins - new cron idempotency section citing the existing wp_next_scheduled() guard already used consistently in iwp-cache/iwp-woosales/iwp-booking - new uninstall-vs-deactivation section flagging that only 3 of ~15 plugins ship uninstall.php despite most creating options/tables - new release-packaging checklist tied to iWP's actual IWP_Updater version-wiring convention (header/constant/updater param must agree) Provenance noted inline with source URL. Left out the source's generic architecture/Settings-API prescription and its detect_plugins.mjs script (skill's house style is prose-only, no bundled scripts). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -81,15 +81,57 @@ find/replace, etc.) before they ever reach a live site.
|
||||
`current_user_can('manage_options')` (or the narrowest capability that
|
||||
actually applies — don't default to `manage_options` for things a
|
||||
lower-privileged role should legitimately be able to do).
|
||||
- **Always both, never either alone.** A nonce proves the request came
|
||||
from your own form/page (CSRF protection) — it says nothing about who
|
||||
is allowed to make it. A capability check proves authorization — it
|
||||
says nothing about whether the request was forged. Nonce-only lets a
|
||||
forged/leaked-nonce request through if the attacker can get one
|
||||
in-scope; capability-only lets a logged-in admin's browser be tricked
|
||||
into firing the action via CSRF. This is already the live pattern —
|
||||
`class-iwp-cache-db-cleanup.php`'s AJAX handler runs
|
||||
`check_ajax_referer('iwp_cache_db_cleanup', 'nonce')` immediately
|
||||
followed by `current_user_can('manage_options')`, never one without
|
||||
the other. Match that shape, don't drop either check because "the
|
||||
button is already hidden from non-admins" — client-side hiding is not
|
||||
a server-side check.
|
||||
- Escape on output, every time, using the context-correct function:
|
||||
`esc_html()`, `esc_attr()`, `esc_url()`, `esc_js()` — never raw-echo
|
||||
anything that traces back to user input or the database without one of
|
||||
these.
|
||||
these. Escape **late** — at the point of output, not when the value is
|
||||
first read or stored — so the stored/cached copy stays raw and every
|
||||
new rendering context gets its own correct escaping function.
|
||||
- Sanitize on input: `sanitize_text_field()`, `absint()`,
|
||||
`sanitize_email()`, etc. — appropriate to the expected shape of the
|
||||
data, applied at the point the `$_POST`/`$_GET` value is first read.
|
||||
Read superglobals by explicit key only — never loop over or dump the
|
||||
whole `$_POST`/`$_GET` array — and run `wp_unslash()` before
|
||||
sanitizing (WordPress adds slashes to superglobal values; sanitizing
|
||||
before unslashing leaves stray backslashes in what gets stored).
|
||||
- `$wpdb->prepare()` for every query with a variable in it — no string-
|
||||
interpolated SQL, ever, no exceptions.
|
||||
interpolated SQL, ever, no exceptions. The `%i` identifier placeholder
|
||||
(for table/column names) only exists from WP 6.2 — most iWP plugins
|
||||
declare `Requires at least: 6.0` or no floor at all, so don't rely on
|
||||
`%i` without either raising the plugin's stated minimum or falling
|
||||
back to an allow-listed identifier switch instead.
|
||||
|
||||
## Admin settings: Settings API vs. AJAX-backed dashboards
|
||||
|
||||
iWP plugins split roughly evenly between two real patterns — check which
|
||||
one a given plugin already uses before adding a settings field, don't
|
||||
introduce a third:
|
||||
|
||||
- **Classic Settings API** (`iwp-mailer`, `iwp-mailer-relay`,
|
||||
`iwp-woosales`, `iwp-subscriptions`): `register_setting()` with a
|
||||
`sanitize_callback`, an admin-post form, capability enforced by the
|
||||
standard `option_page_capability_*` gate.
|
||||
- **AJAX-backed dashboard** (`iwp-cache`, `iwp-security`, and most of
|
||||
the newer plugins): a JS dashboard posts to `wp_ajax_*` handlers that
|
||||
manually replicate the same discipline — `check_ajax_referer()` +
|
||||
`current_user_can()` at the top of every handler, then
|
||||
`update_option()` directly. `class-iwp-cache-db-cleanup.php` is the
|
||||
canonical shape.
|
||||
|
||||
Either is fine; match whichever the plugin you're touching already uses.
|
||||
|
||||
## Database tables
|
||||
|
||||
@@ -103,6 +145,66 @@ dbDelta($sql);
|
||||
Track a `DB_VERSION` constant + a stored option so future plugin updates
|
||||
can detect and re-run `dbDelta()` for schema changes.
|
||||
|
||||
## Cron: every scheduled hook needs a guard, and the callback must survive running twice
|
||||
|
||||
Every iWP plugin that schedules a recurring event already follows the
|
||||
same guard — wrap `wp_schedule_event()` in a `wp_next_scheduled()` check,
|
||||
never schedule unconditionally on every request:
|
||||
```php
|
||||
if (!wp_next_scheduled(self::CRON_HOOK)) {
|
||||
wp_schedule_event(time(), 'daily', self::CRON_HOOK);
|
||||
}
|
||||
```
|
||||
(see `iwp-cache/includes/class-iwp-cache-preload.php`,
|
||||
`iwp-woosales/includes/class-iwp-woosales-cron.php`,
|
||||
`iwp-booking/includes/class-iwp-booking-cron.php`). Without the guard,
|
||||
every page load or re-activation re-schedules the event, and WP-Cron
|
||||
silently accumulates duplicate instances of the same hook — they all
|
||||
fire, so a "daily" job can end up running several times a day.
|
||||
|
||||
Beyond the schedule guard, the callback itself must tolerate running
|
||||
late or twice — WP-Cron is a request-triggered pseudo-cron, not a
|
||||
real-time scheduler; a low-traffic site can miss its window for hours,
|
||||
and two concurrent requests can trigger the same due hook back-to-back.
|
||||
Make the job naturally idempotent (e.g. "sync everything modified since
|
||||
`last_sync_time`", safe to re-run) or guard the actual work with a
|
||||
short-lived transient/option lock, not just the scheduling call.
|
||||
|
||||
Always pair `wp_schedule_event()` with the matching
|
||||
`wp_clear_scheduled_hook()` — from `register_deactivation_hook()` if the
|
||||
schedule shouldn't survive deactivation, or from `uninstall.php` if it
|
||||
should survive deactivation but not a full uninstall (see next section).
|
||||
|
||||
## Uninstall vs. deactivation: decide data retention explicitly
|
||||
|
||||
Only three plugins in this suite currently ship an `uninstall.php`
|
||||
(`iwp-security`, `iwp-woosales`, `informatiq-toolkit`), even though most
|
||||
plugins create options, transients, and in several cases their own
|
||||
database tables. That split isn't automatically wrong — WordPress's own
|
||||
convention is that **deactivation should be reversible and
|
||||
non-destructive** (stop cron, leave data alone so re-activating restores
|
||||
prior state) while **uninstall is where actual cleanup belongs** — but
|
||||
it should be a decision made per plugin, not an oversight.
|
||||
|
||||
When adding a new plugin, or touching an existing one's lifecycle code,
|
||||
decide and make explicit which applies:
|
||||
|
||||
- **Deactivation** (`register_deactivation_hook`): stop cron
|
||||
(`wp_clear_scheduled_hook()`) and nothing else. Data stays so toggling
|
||||
the plugin off temporarily doesn't lose settings/history.
|
||||
- **Uninstall** (`uninstall.php`, gated on
|
||||
`if (!defined('WP_UNINSTALL_PLUGIN')) exit;` — prefer this over
|
||||
`register_uninstall_hook()` for anything nontrivial, since a flat file
|
||||
is simpler to keep in sync with the plugin's actual option/table list):
|
||||
delete every option and transient the plugin created, drop every
|
||||
custom table (`DROP TABLE IF EXISTS`), clear any cron still scheduled.
|
||||
See `iwp-security/uninstall.php` and `iwp-woosales/uninstall.php` for
|
||||
the shape.
|
||||
|
||||
A plugin that creates a custom table or a nontrivial option set and ships
|
||||
neither an `uninstall.php` nor an explicit "intentionally retained,
|
||||
because X" note is an oversight worth flagging, not a policy.
|
||||
|
||||
## PHP 7.4 compatibility
|
||||
|
||||
Unless a specific brief says otherwise, target PHP 7.4+ (a large fraction
|
||||
@@ -187,3 +289,54 @@ WooCommerce context), object caching (`wp_cache_*` functions), HTTP
|
||||
requests (`wp_remote_get`/`wp_remote_post`, not raw `curl`) — check
|
||||
whether a core API already does it. Reinventing these is both wasted
|
||||
effort and a common source of subtle bugs core already solved correctly.
|
||||
|
||||
## Release packaging: version consistency before you zip anything
|
||||
|
||||
iWP plugins auto-update through `IWP_Updater`
|
||||
(`wp-plugins/iwp-updater-client`, bundled per-plugin as
|
||||
`includes/class-iwp-updater.php`), which reads the plugin's version from
|
||||
a PHP constant, not from the zip filename or a release note. A release
|
||||
is only as good as that constant being right, so before packaging:
|
||||
|
||||
1. **Three places must agree**: the `Version:` plugin header, the
|
||||
`PREFIX_VERSION` constant defined right below the `ABSPATH` guard, and
|
||||
the `'version' => PREFIX_VERSION` line passed into
|
||||
`new IWP_Updater([...])`. Every plugin in the suite wires the
|
||||
updater's version to that same constant, never a separate literal —
|
||||
e.g. `iwp-cache.php` passes `'version' => IWP_CACHE_VERSION`,
|
||||
`iwp-booking.php` passes `'version' => IWP_BOOKING_VERSION`. Bump the
|
||||
header without bumping the constant (or vice versa) and
|
||||
`IWP_Updater`'s `pre_set_site_transient_update_plugins` filter
|
||||
compares the wrong number — customers either get nagged when already
|
||||
current, or don't get notified of a real update.
|
||||
2. **`plugin_slug` must match the catalog key** on iwp.es —
|
||||
`wp-plugins/iwp-subscriptions`'s update-check endpoint looks the slug
|
||||
up directly. Confirm against the actual product catalog, don't assume
|
||||
it matches the plugin's directory name.
|
||||
3. **Exclude dev artifacts from the shipped zip**: `.git/`, `.gitignore`
|
||||
itself, `node_modules/`, test fixtures, and any internal-only
|
||||
`README.md` content not meant for a customer (some plugin READMEs in
|
||||
this suite are customer-facing, some — like
|
||||
`iwp-updater-client/README.md` — are internal integration notes).
|
||||
4. **Verify the packaged zip before shipping it**, don't trust a clean
|
||||
build as proof it activates cleanly — deploy it with
|
||||
`wordpress-plugin-staging-verification` against the persistent
|
||||
`staging1` baseline. If this release is a rebrand/fork rather than a
|
||||
version bump on an existing iWP plugin, run
|
||||
`wordpress-plugin-rebrand`'s identifier-renaming/flattening step
|
||||
first.
|
||||
|
||||
## Provenance
|
||||
|
||||
The nonce+capability pairing framing, late-escaping wording, `%i`
|
||||
placeholder caveat, cron-idempotency guardrail, and uninstall-vs-
|
||||
deactivation framing above were adapted from
|
||||
[wordpress/agent-skills](https://github.com/wordpress/agent-skills)'
|
||||
`wp-plugin-development` skill
|
||||
(https://skills.sh/wordpress/agent-skills/wp-plugin-development, surfaced
|
||||
by the 2026-08-15 skills.sh/autoskills.sh scans) — reworked against this
|
||||
suite's actual code rather than copied verbatim. Its generic
|
||||
"Settings-API-first" architecture prescription was deliberately not
|
||||
imported wholesale: roughly half of iWP's plugins use an AJAX-backed
|
||||
dashboard pattern instead (see "Admin settings" above), and prescribing
|
||||
one true pattern would contradict the real, working code.
|
||||
|
||||
Reference in New Issue
Block a user