From c54b5c2cefef98a8e55c2ee4c82f727cdaf45d3a Mon Sep 17 00:00:00 2001 From: kristianwind Date: Sun, 20 Sep 2026 10:20:17 +0200 Subject: [PATCH] Assert structure rather than text, and never leave a half-built zip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things, all the same shape: a check that recognises a spelling has one blind spot per spelling, and one that reads the thing itself has none of that kind. Security hit it when an assertion matching the literal '∞' went red over a file drawing exactly that character through a CSS escape. Swept this repo's assertions for the same and found two. "the plugin itself loads the shared menu file" searched THIS FILE'S SOURCE TEXT for "'includes/shared-menu.php',". A comment mentioning that path would have satisfied it — the same hollow shape as grepping a file for the words of the guard it is supposed to have, which this kit already had once today. The admin list is now a method, admin_includes(), and the test reads the array the plugin actually loads from. Proven red by removing the entry: the failure prints the list it did find. The package-URL check asserted the absence of one particular IP address. Gitea's ROOT_URL has been a raw address before and may be again, so naming today's is blind to tomorrow's. It now asserts the host and the scheme. Proven red with the realistic fault rather than a contrived one — the raw address in the Update URI header: FAIL so it is never the raw address … expected 'gitea.nolimit.dk', got '57.129.89.117' FAIL and it is fetched over https … expected 'https', got 'http' And ci/build-zip.sh left the archive behind when it failed. Every exit in the checks section happens after the archive is written, so a failed build kept a half-built artefact that looks exactly like a good one — and both the release step and anyone publishing by hand take build/.zip by name. Found by stubbing ci/dist-files.sh empty: exit 1, correct message, zip still on disk. A trap removes it now, proven both ways. Checked while here, from Security's report that the kit's shipped-file floor was unguarded: this copy is guarded, and red for the right reason. Stubbing the file list empty gives exit 1, one FAIL naming the empty list, and zero occurrences of the "every one guarded" line — which is the assertion that matters, since being red for an unrelated reason would look identical from the exit code. Co-Authored-By: Claude Opus 5 (1M context) --- ci/build-zip.sh | 9 +++++++ tests/smoke.php | 30 +++++++++++++++++------ woocommerce-email-discount-manager.php | 34 +++++++++++++++++--------- 3 files changed, 54 insertions(+), 19 deletions(-) diff --git a/ci/build-zip.sh b/ci/build-zip.sh index f2a9862..0f04b88 100755 --- a/ci/build-zip.sh +++ b/ci/build-zip.sh @@ -53,6 +53,15 @@ zip_entries() { ci_step "what the zip contains" zip_entries | sed 's/^/ /' +# 🔴 A failed build must not leave a zip behind. Every exit below this point +# happens AFTER the archive is written, so without this the tree keeps a +# half-built artefact that looks exactly like a good one — and the release step +# takes `build/.zip` by name, as does anyone publishing by hand. The +# check that caught it: stubbing ci/dist-files.sh empty makes this script exit +# 1 with "The main plugin file is not in the zip", and the zip was still there +# afterwards. +trap 'code=$?; [ "$code" = 0 ] || { rm -f "build/$PLUGIN_SLUG.zip"; echo "Removed the incomplete build/$PLUGIN_SLUG.zip." >&2; }; exit $code' EXIT + ci_step "checks" zip_entries | grep -qx "$PLUGIN_SLUG/$PLUGIN_MAIN" \ || { echo "The main plugin file is not in the zip." >&2; exit 1; } diff --git a/tests/smoke.php b/tests/smoke.php index d5f04d2..0dc1e8f 100644 --- a/tests/smoke.php +++ b/tests/smoke.php @@ -278,13 +278,16 @@ wc_edm_ok( // The other half: the by-hand require above would pass even if the plugin // never loaded the file in a real admin request. This is the assertion that // covers that, and it is the half a reduced repro always omits. +// +// Read from the list the plugin actually loads from, NOT searched for in this +// file's source text. The previous version did the latter, and a comment +// mentioning that path would have satisfied it — the same hollow shape as +// grepping a file for the words of the guard it is supposed to have, which is +// a bug this kit has already had once today. wc_edm_ok( 'and the plugin itself loads the shared menu file in admin', - false !== strpos( - file_get_contents( WC_EDM_PLUGIN_FILE ), - "'includes/shared-menu.php'," - ), - 'it is not in the is_admin() branch of includes(), so nothing would define wpmgr_ensure_menu()' + in_array( 'includes/shared-menu.php', WC_Email_Discount_Manager::admin_includes(), true ), + 'admin includes: ' . implode( ', ', WC_Email_Discount_Manager::admin_includes() ) ); // The card on the shared dashboard. Absent from it is a different failure from @@ -390,10 +393,21 @@ wc_edm_is( $update_uri . '/releases/download/v1.4.0/woocommerce-email-discount-manager.zip', $parsed['package'] ); -wc_edm_ok( +// Asserted as the host, not as the absence of one particular IP address. +// Gitea's ROOT_URL has been a raw address before, and the package URL it +// reports carries whatever that is — so a check that names today's IP is blind +// to tomorrow's. What matters is that the URL points at the host we publish +// from, however the forge chose to describe itself. +$wc_edm_host = wp_parse_url( $parsed['package'], PHP_URL_HOST ); +wc_edm_is( 'so it is never the raw address the forge happened to report', - false === strpos( $parsed['package'], '57.129.89.117' ) && 0 === strpos( $parsed['package'], 'https://' ), - 'got ' . $parsed['package'] + 'gitea.nolimit.dk', + $wc_edm_host +); +wc_edm_is( + 'and it is fetched over https', + 'https', + wp_parse_url( $parsed['package'], PHP_URL_SCHEME ) ); wc_edm_is( 'and the tested-up-to line is read out of the release notes', '7.1', $parsed['tested'] ); wc_edm_ok( 'a release with no zip attached is not offered at all', null === $parse->invoke( new WC_EDM_Updater(), array( 'tag_name' => 'v9.9.9', 'assets' => array() ) ) ); diff --git a/woocommerce-email-discount-manager.php b/woocommerce-email-discount-manager.php index 52af16e..6ae7146 100644 --- a/woocommerce-email-discount-manager.php +++ b/woocommerce-email-discount-manager.php @@ -220,6 +220,28 @@ final class WC_Email_Discount_Manager { /** * Load class files. */ + /** + * The files loaded only in an admin request. + * + * A method rather than a literal inside includes() so a test can read the + * actual list. The assertion it replaces searched this file's SOURCE TEXT + * for the shared menu's path, which a comment mentioning that path would + * have satisfied just as well — the same hollow shape as grepping a file + * for the words of the guard it is supposed to have. + * + * @return string[] + */ + public static function admin_includes() { + return array( + 'includes/shared-menu.php', + 'includes/class-admin.php', + 'includes/class-admin-groups.php', + 'includes/class-admin-emails.php', + 'includes/class-admin-shipping.php', + 'includes/class-admin-audit.php', + ); + } + private function includes() { $files = array( 'includes/class-db.php', @@ -231,17 +253,7 @@ final class WC_Email_Discount_Manager { ); if ( is_admin() ) { - $files = array_merge( - $files, - array( - 'includes/shared-menu.php', - 'includes/class-admin.php', - 'includes/class-admin-groups.php', - 'includes/class-admin-emails.php', - 'includes/class-admin-shipping.php', - 'includes/class-admin-audit.php', - ) - ); + $files = array_merge( $files, self::admin_includes() ); } foreach ( $files as $file ) {