Release 1.0.0: wp.org submission readiness - #5
Open
rayhanwealcoder wants to merge 30 commits into
Open
Conversation
No real support/community links exist yet — motionkit.io/support, motionkit.io/community, and motionkit.io/docs are either 404 or unready. Rather than ship dead links, remove the Help tab entirely: sidebar nav entry, switch-case dispatch, and render_help_tab() (the Support banner + Community/Documentation cards). ?tab=help now falls through to the default Connect tab instead of erroring. Only Connect and Tools remain as navigable tabs. CLAUDE.md synced (file-tree comment + Auth section), and corrected a stale claim that License was a separate tab — it's actually rendered inline inside Connect via the private render_license_tab().
Dead code: its only call site was already commented out, and the method itself had a real bug — DOING_CRON would return true unconditionally, which (if ever re-enabled) would skip Plugin::init() on cron requests entirely. That would break LicenseStatus's daily refresh hook registration, which init_auth() explicitly registers outside the is_admin() branch specifically so cron keeps working (see the comment there). Removed the method, its commented call, and the matching phpstan-baseline.neon suppression entry. PHPStan/phpcs clean.
ConnectPage::print_menu_icon_style() echoed a raw <style> tag on admin_head, unconditionally on every wp-admin page. Replaced with enqueue_menu_icon_style(), which wp_enqueue_style()s a new src/css/admin-menu-icon.css (builds to assets/build/, same pattern as admin.css/admin-tools.css/admin-bar.css) on admin_enqueue_scripts. Kept it as its own enqueue (not folded into enqueue_admin_styles(), which is gated to the MotionKit settings page only) since the menu icon needs sizing on every admin page, not just ours. Verified live: style enqueues correctly with the right registered src outside the MotionKit settings page context; built CSS matches the original inline rule exactly. Also fixed a stale docblock still listing "Connect, Tools, License, Help" as tabs — License renders inline in Connect and Help was removed in the previous commit.
…unslash - Plugin name contained "WordPress" (wp.org naming policy disallows this anywhere in a plugin's display name): "Motionkit - Visual Animation with GSAP for WordPress" -> "Motionkit - Visual Animation with GSAP" in both readme.txt and motionkit.php. - Tested up to must be major.minor only, no patch version: 7.0.2 -> 7.0 in both files (the version was right, the granularity wasn't). - ConnectPage.php: $_POST['_wpnonce'] now sanitized+unslashed before wp_verify_nonce(), matching the pattern used elsewhere in the class. - Removed scripts/copy-to-editor copy.js, a stray unreferenced duplicate (older hardcoded-path version of copy-to-editor.js). - phpstan-bootstrap.php: synced stale MOTIONKIT_VERSION 1.5.1 -> 1.0.0. Investigated but did not change: PCP's hidden-files/markdown/.claude/ .github findings are dist-scope false positives (all already excluded via .distignore, PCP scans the raw working dir not the filtered zip); NonceVerification.Recommended warnings are all read-only $_GET display flags with no state-changing side effect; DirectDatabaseQuery warnings are correctly wp->prepare()'d one-shot admin/uninstall queries. Documented in WPORG-SUBMISSION.md's new "Fifth pass" section.
WordPress.DB.PreparedSQL.InterpolatedNotPrepared /
PreparedSQLPlaceholders.UnfinishedPrepare flagged the dynamic
IN ({$placeholders}) clause — the sniff can't trace that $placeholders
is itself built from %s tokens sized to $hot_options (a hardcoded
literal, never external input), which is the standard $wpdb->prepare()
pattern for a variable-length IN (...) list. Added a targeted
phpcs:ignore on the flagged line rather than another blanket
phpcs.xml.dist exclusion, since PCP runs its own fixed ruleset and
doesn't honor that file. Verified clean against the unmodified
WordPress-Extra standard directly (not just this project's lenient
ruleset), and PHPStan stays clean too.
Documented in WPORG-SUBMISSION.md's "Fifth pass" section.
…ings Resolves ConnectPage.php's WordPress.Security.NonceVerification.Recommended findings from Plugin Check with two different fixes depending on what the data actually is: - ?tab= (enqueue_admin_styles, render_page): now genuinely nonce-gated. wp_nonce_url() stamps the sidebar tab links with a 'motionkit_tab_nav' nonce; both methods verify it and fall back to the default 'connect' tab on a missing/invalid nonce rather than hard-failing (an expired bookmark shouldn't lock out navigation, and nothing here changes state either way — this is defense-in-depth, not a functional requirement). Verified live: valid nonce opens the requested tab, missing/garbage nonce falls back correctly in both cases. - render_notices()'s ?error=/?connected=/?disconnected=/?license_refresh=/ ?tools_deleted=/?verify= flags and get_error_message()'s ?error_message=: these are appended by server-side redirects *after* an action that already verified its own nonce (OAuth callback, handle_tools_actions()). They only pick which notice banner to show — a crafted URL at worst displays a fake success banner with no effect on actual state — so method-level phpcs:disable/enable blocks suppress these with the reasoning documented inline, rather than nonce-stamping every redirect target for no real security benefit. Verified against the unmodified WordPress-Extra standard directly (not just this project's lenient phpcs.xml.dist, since Plugin Check doesn't honor that file): 24 warnings on 12 lines -> 0. phpcs.xml.dist and PHPStan both stay clean too.
…ld mkit_pg_ prefix is_valid_page_type_config() — the security check that confines editor- session writes to MotionKit's own key shape — still matched the old 'mkit_pg_' prefix instead of 'motionkit_pg_'. Since the new prefix string doesn't start with the old one, this rejected every real post-rename payload, breaking save_current_page_animation, save_current_page_settings, and delete_current_page_settings entirely. Global settings/animations don't go through this validator, so they kept working — that's why this wasn't caught by the earlier live DB round-trip test during the original prefix rename (commit 5a94c25), and why the repo-wide grep sweep at the time missed it (it searched for the two full patterns with animation_/settings_ suffixes, not the bare shared-prefix literal this method actually checks). Found via a full Playwright E2E regression pass across the whole connect/editor/settings flow. Fixed: 'mkit_pg_' -> 'motionkit_pg_'. Verified live via 3 direct REST calls: new-prefix payload succeeds and actually persists to wp_options; old-prefix payload is still correctly rejected (400, proving the security boundary itself is intact, not just permissive now); an unrelated option name (siteurl) is also still rejected (proving the check isn't just accepting everything).
…/ScrollSmoother Full includes/ sweep against the unmodified WordPress-Extra standard (not just this repo's lenient phpcs.xml.dist, since Plugin Check doesn't honor that file) found ~40 more NonceVerification.Recommended warnings beyond ConnectPage.php, each resolved per its actual context: - OAuthHandler::handle_callback() — reached via motionkit.io's redirect, not a same-site form/link, so a WP nonce isn't the right mechanism here (motionkit.io has no way to know WP's nonce secret). The OAuth 'state' parameter (crypto-random, single-use transient, hash_equals() check before anything is written) is the correct CSRF protection for this flow. Suppressed with that reasoning documented inline. - Frontend::is_editor_preview() — read-only mode check gated by JWT validation, a stronger authentication than a nonce would provide. - Frontend::is_full_preview() / ScrollSmoother::run_scroll_smoother() — pure read-only boolean flags, no state change. - Frontend::enqueue_editor_preview_scripts() — only ever reached after is_editor_preview() already validated the token earlier in the same request; motionkit_token here is a re-read, not a fresh unauthenticated one. Each gets a targeted method-level phpcs:disable/enable pair with site-specific reasoning, rather than one blanket justification. Verified: full includes/ sweep against unmodified WordPress-Extra now returns zero NonceVerification findings. phpcs.xml.dist and PHPStan both clean. Site smoke-tested live (loads clean, no fatals). WPORG-SUBMISSION.md's Fifth-pass section updated to reflect the actual resolution instead of "confirmed false-positive, left as-is."
Per explicit direction to prefer an actual nonce over a suppress comment wherever the redirect is one this plugin controls: every wp_safe_redirect() that appends a notice-triggering flag (?error=/?connected=/?disconnected=/?license_refresh=/?tools_deleted=/ ?verify=) is now wp_nonce_url()-stamped with a shared 'motionkit_notice' nonce action: - OAuthHandler::handle_callback() — 3 redirect sites - OAuthHandler::handle_verify() — 2 redirect sites - OAuthHandler::handle_disconnect() — 1 redirect site - ConnectPage::handle_tools_actions() — 2 redirect sites - ConnectPage's bulk-delete JS (tools_all_deleted, set client-side after the AJAX call already succeeded) — nonce localized via wp_create_nonce() and appended by the JS itself render_notices() verifies this nonce once at the top and returns early (shows no notice) on missing/invalid, rather than hard-failing — none of these flags are destructive either way, so silent fallback is enough. get_error_message() is only ever called from within the already-verified render_notices(), so it documents that inheritance rather than re-verifying independently. Left as documented phpcs:disable suppressions where a WP nonce genuinely isn't the right mechanism: OAuthHandler::handle_callback()'s inbound code/state reads (OAuth's own state transient + hash_equals() is the correct CSRF check — motionkit.io's redirect has no way to produce a WP nonce), Frontend::is_editor_preview() (JWT-gated, stronger than a nonce, and the URL is editor-generated not WP-admin- generated), Frontend::is_full_preview()/ ScrollSmoother::run_scroll_smoother() (read-only booleans, no state change), and Frontend::enqueue_editor_preview_scripts() (re-reads an already-validated token). Verified live via 4 cases: valid nonce shows the notice; missing, garbage, and wrong-action nonces all correctly show nothing. Confirmed wp_nonce_url()'s output round-trips cleanly through wp_safe_redirect(). Full includes/ sweep against unmodified WordPress-Extra: zero NonceVerification findings. phpcs.xml.dist and PHPStan clean. Site smoke-tested live (loads clean, no fatals). WPORG-SUBMISSION.md updated to document the real-nonce vs. documented-suppression split accurately.
A phpcs:disable comment only reaches someone reading that exact file with phpcs context in mind — a human wp.org reviewer going through the plugin doesn't necessarily open every source file, and the comment alone might not read as sufficiently convincing on its own. Added a readme.txt FAQ entry, "Why don't all requests use a WordPress nonce?", in plain language: every state-changing admin action uses a real nonce, and the two exceptions (OAuth callback, editor-preview iframe) use OAuth's state parameter and a signed JWT instead because those requests originate from motionkit.io/editor.motionkit.io, not from a link this WordPress site generated — a WP nonce is derived from this site's own secret keys, which the other party has no way to know, so it literally cannot produce one. Cross-referenced both directions: the readme FAQ names the affected code paths, and the two remaining phpcs:disable comments (OAuthHandler::handle_callback, Frontend::is_editor_preview) now point back at the FAQ entry by name, so a reviewer following either the code or the docs lands on the same explanation. Verified: php -l, phpcs.xml.dist, and PHPStan all clean; site loads without errors.
ConnectPage.php.bak and LicenseStatus.php.bak were tracked, so the build script copied them straight into the distributed zip — 80 KB of stale source shipping to every site that installs the connector. Backup copies of auth and licensing code are the last thing that should be sitting in a public package, and WP.org review flags leftovers like this. Both were verified strictly older than the live files before deletion, so nothing is lost: the ConnectPage backup predates the 'inactive' license branch, and the LicenseStatus backup still carries the old KNOWN_STATUSES list without 'inactive' and the empty_state() return that the never-checked fix replaced. Nothing in the plugin referenced either file. Add *.bak and *.orig to .gitignore so a stray editor backup cannot be committed again, and to .distignore as a second line of defence for anything created locally during a build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Prepares the plugin for its first wp.org submission at version 1.0.0. Covers Plugin Check (PCP) findings, admin-page cleanup, and dead-code removal.
1.5.1→1.0.0acrossreadme.txt/motionkit.php/MOTIONKIT_VERSION, matching the "no public release yet" reality (first wp.org listing should start at 1.0.0).readme.txt+motionkit.php.Tested up tomust bemajor.minoronly —7.0.2→7.0.ConnectPage.php:$_POST['_wpnonce']now sanitized+unslashed beforewp_verify_nonce().scripts/copy-to-editor copy.js).PreparedSQLfalse-positive onPlugin.php's dynamicIN (...)clause with a targetedphpcs:ignore(verified clean against unmodifiedWordPress-Extra, not just this repo's lenient ruleset).wp_nonce_url()+wp_verify_nonce(), graceful fallback to the default tab on missing/invalid nonce — verified live for all three cases: valid, missing, garbage). Suppressed the remainingNonceVerificationwarnings onrender_notices()/get_error_message()with documented reasoning (those flags come from already-nonce-verified server redirects, not user-forgeable state changes).motionkit.io/support,/community,/docs) are dead or unready; no real destination exists yet.<style>echoes (admin bar icon CSS, top-level menu icon CSS) to properly enqueued, versioned, cacheable stylesheets (wp_enqueue_style()+ newsrc/css/admin-bar.css/admin-menu-icon.css, built via webpack same asadmin.css).Plugin::should_skip_init()— unused (its only call site was already commented out) and itsDOING_CRONbranch had a real bug that would have brokenLicenseStatus's daily cron refresh if ever re-enabled.CLAUDE.md,WPORG-SUBMISSION.md) synced throughout to match.Test plan
php -lclean on every touched filephpcsclean against both this repo'sphpcs.xml.distand the unmodifiedWordPress-Extrastandardphpstanclean (no baseline regressions)development.local: admin bar CSS enqueues correctly outside the MotionKit settings page; menu-icon CSS enqueues on non-MotionKit admin pages; sidebar tab nonce round-trip (valid/missing/garbage) all behave correctlywp dist-archive(or manual zip) dry run to confirm shipped contents match the.distignore-filtered simulation already verified this session🤖 Generated with Claude Code