kw/nolimit-discounts
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 2 Wiki Activity

Assert structure rather than text, and never leave a half-built zip

Browse Source 
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/<slug>.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) <noreply@anthropic.com>
This commit is contained in:
kristianwind
2026-09-20 10:20:17 +02:00
co-authored by Claude Opus 5
parent 53c34067bc
commit c54b5c2cef
3 changed files with 54 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files
ci/build-zip.sh
+9
View File
@@ -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/<slug>.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; }
tests/smoke.php
+22 -8
View File
@@ -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() ) ) );
woocommerce-email-discount-manager.php
+23 -11
View File
@@ -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 ) {