diff --git a/ci/check-docs.php b/ci/check-docs.php index f3ba2c5..2f9a918 100644 --- a/ci/check-docs.php +++ b/ci/check-docs.php @@ -31,52 +31,6 @@ * Usage: php ci/check-docs.php */ -/** - * MIN_SHIPPED_PHP_FILES from ci/plugin.conf. Refuses rather than defaulting: - * an unset floor parses to 0 and every comparison against it passes, which is - * the same bug as a scan over zero files one level up. - */ -function shipped_php_floor() { - $conf = dirname( __DIR__ ) . '/ci/plugin.conf'; - - if ( is_file( $conf ) && preg_match( '/^MIN_SHIPPED_PHP_FILES=([0-9]+)/m', (string) file_get_contents( $conf ), $m ) && (int) $m[1] > 0 ) { - return (int) $m[1]; - } - - fwrite( STDERR, "::error::No usable MIN_SHIPPED_PHP_FILES in ci/plugin.conf — refusing to run with no floor.\n" ); - exit( 1 ); -} - -/** - * This plugin's own hook prefix, read from ci/plugin.conf. - * - * Refuses rather than guessing, and is deliberately NOT derived from - * TEXT_DOMAIN: here the domain is wc-email-discount and the hooks are wc_edm_, - * and in the tree this file came from the domain is nolimit-security and the - * hooks are nlsec_. A checker that guessed would be wrong in both. - */ -function hook_prefix() { - static $prefix = null; - - if ( null !== $prefix ) { - return $prefix; - } - - $conf = dirname( __DIR__ ) . '/ci/plugin.conf'; - $prefix = ''; - - if ( is_file( $conf ) && preg_match( '/^HOOK_PREFIX=([A-Za-z0-9_]+)/m', (string) file_get_contents( $conf ), $m ) ) { - $prefix = strtolower( $m[1] ); - } - - if ( '' === $prefix ) { - fwrite( STDERR, "::error::No HOOK_PREFIX in ci/plugin.conf — refusing to guess which hooks are ours.\n" ); - exit( 1 ); - } - - return $prefix; -} - /** * Is this hook part of a contract, or is it internal wiring? * @@ -114,34 +68,27 @@ function hook_prefix() { * anybody about), `nlsec_public_rest_routes` (a genuine extension point a form * plugin needs when a shop locks REST down) and `nlsec_pending_wp_config`. * - * @param string $call do_action | apply_filters | add_filter | add_action + * @param string $call One of do_action, apply_filters, add_filter, add_action. * @param string $hook The hook name. * @return bool */ function is_contract( $call, $hook ) { - // 🔴 From ci/plugin.conf, never a literal. The version of this file - // published as woo-security-manager 52dd99d asked plugin.conf for the - // prefix on the DOCUMENTED side (and refuses without it, correctly) and - // still tested the SOURCE side against a hardcoded 'nlsec_'. Ported here - // verbatim that is not a green-over-nothing — it is worse. Measured on this - // tree with docs/CONTRACT.md documenting no hook at all: - // - // 3 hook(s) in the shipped source and not in docs/CONTRACT.md: - // wpmgr_dashboard_cards, wpmgr_menu_glyph, wpmgr_menu_title - // - // Red, specific, and blind to all six wc_edm_* hooks — because 'wpmgr_' is - // the same string in every tree and 'nlsec_' is the one that is not. Fix - // those three and the check goes green over six undocumented contract - // hooks, with a red run behind you as evidence that it works. A partially - // blind check that fails is more convincing than one that passes. - // - // Note which step disarmed the guard: the floor at 20 shipped PHP files - // refused to run in this 14-file tree, which is the only thing that stopped - // it, and the porting instructions were to add HOOK_PREFIX and fix the - // floor. Doing as told is what removed the protection. Two wrongs - // cancelling into a pass, in the documented order. - $ours = 0 === strpos( $hook, hook_prefix() ); - $family = 0 === strpos( $hook, 'wpmgr_' ); + // 🔴 From the conf, never a literal. Hardcoding 'nlsec_' here is exactly + // the copied-kit trap this family has paid for twice: the check keeps + // working in the tree it was written in and silently classifies every hook + // in the next one as "not ours", so CONTRACT.md may be empty and the run + // is green. I wrote it as a literal first and caught it re-reading. + $prefix = hook_prefix(); + + $ours = 0 === strpos( $hook, $prefix ); + $family = false; + + foreach ( sibling_prefixes() as $other ) { + if ( 0 === strpos( $hook, $other ) ) { + $family = true; + break; + } + } if ( ! $ours && ! $family ) { return false; @@ -156,14 +103,147 @@ function is_contract( $call, $hook ) { return $family; } +/** + * The shipped-file floor, from MIN_SHIPPED_PHP_FILES in ci/plugin.conf. + * + * Refuses rather than defaulting: an unset floor parses to 0 and every + * comparison against it passes, which is a scan over zero files one level up. + * + * @return int + */ +function shipped_php_floor() { + global $conf_root; + + $conf = $conf_root . '/ci/plugin.conf'; + + if ( is_file( $conf ) && preg_match( '/^MIN_SHIPPED_PHP_FILES=([0-9]+)/m', (string) file_get_contents( $conf ), $m ) && (int) $m[1] > 0 ) { + return (int) $m[1]; + } + + fwrite( STDERR, "::error::No usable MIN_SHIPPED_PHP_FILES in ci/plugin.conf — refusing to run with no floor.\n" ); + exit( 1 ); +} + +/** + * This plugin's own hook prefix, from ci/plugin.conf. + * + * Refuses rather than guessing. TEXT_DOMAIN is not it — here that is + * nolimit-security while the hooks are nlsec_ — and a checker that guessed + * would guess wrong in this tree and silently right in another, which is the + * worse of the two. + * + * @return string + */ +function hook_prefix() { + static $prefix = null; + + if ( null !== $prefix ) { + return $prefix; + } + + global $conf_root; + + $conf = $conf_root . '/ci/plugin.conf'; + + if ( is_file( $conf ) && preg_match( '/^HOOK_PREFIX=([A-Za-z0-9_]+)/m', (string) file_get_contents( $conf ), $m ) ) { + $prefix = strtolower( $m[1] ); + + return $prefix; + } + + fwrite( STDERR, "::error::No HOOK_PREFIX in ci/plugin.conf — refusing to guess which hooks are this plugin's.\n" ); + exit( 1 ); +} + +/** + * Prefixes owned by the family, ours excluded. + * + * 🔴 Without this the check has a hole on the CONSUMER side, found by Nolimit + * Forms by deleting two rows and watching it stay green. The first version + * tested `ours || wpmgr_`, which is right in the tree that wrote it — `nlsec_` + * IS its prefix — and wrong everywhere else: in Forms' tree `nlsec_` is + * neither theirs nor `wpmgr_`, so the whole Forms/Security boundary fell into + * "core, not ours to document" and a green was printed over its absence. + * + * That is the class `wpmgr_` was widened to catch in the first place: a name a + * SIBLING owns that we answer. `wpmgr_` was one instance of it, not the rule. + * + * ⚠️ This list goes stale the day a ninth plugin appears — the same fault as + * the hardcoded prefix, one level out, and it is worth saying rather than + * leaving to be found. It belongs in FAMILY.md once that exists, read from one + * place by all eight. Until then it is one line per repo, visible, in the file + * that already holds everything plugin-specific. + * + * Named "sibling" rather than "family" because PHPCS's compatibility sniff + * reads a function starting `fam` as the removed PHP 5.1 `fam_*` extension and + * fails the build on it — a false positive, but one that costs the next person + * the same ten minutes. + * + * @return array + */ +function sibling_prefixes() { + static $list = null; + + if ( null !== $list ) { + return $list; + } + + global $conf_root; + + $conf = $conf_root . '/ci/plugin.conf'; + $raw = is_file( $conf ) ? (string) file_get_contents( $conf ) : ''; + + /* + * 🔴 Refuse when the line is absent, rather than defaulting to wpmgr_. + * + * The default used to BE array( 'wpmgr_' ) — which is exactly the narrow + * behaviour this key exists to remove. So a tree that copied the kit and + * forgot the line got the consumer-side hole straight back, silently, with + * a plausible number and a green, while HOOK_PREFIX three lines away + * refused. Nolimit Forms named it: **a default that is the pre-fix + * behaviour is a fix that switches itself off.** + * + * `FAMILY_PREFIXES=""` is how a plugin says it answers no sibling hooks. + * Same argument as HOOK_PREFIX_MIN=0: "none" and "nobody filled this in" + * are otherwise identical silences, and only one of the two is a decision. + * + * Quotes are stripped because ci/plugin.conf is SOURCED by every ci/*.sh, + * so a bare space-separated value makes the shell try to run the second + * word. The site test found that, reporting + * `ci/plugin.conf: line 89: nlfrm_: command not found` — a message naming + * neither this file nor the setting. + */ + if ( ! preg_match( '/^FAMILY_PREFIXES=(.*)$/m', $raw, $m ) ) { + fwrite( STDERR, "::error::No FAMILY_PREFIXES in ci/plugin.conf. Without it a hook a SIBLING owns falls into \"core, not ours to document\" and this check goes green over the absence of the whole boundary. List the other plugins' prefixes, or set FAMILY_PREFIXES=\"\" to say this plugin answers none.\n" ); + exit( 1 ); + } + + $list = preg_split( '/[\s,]+/', trim( $m[1], " \t\"'" ), -1, PREG_SPLIT_NO_EMPTY ); + + // Ours is handled separately; leaving it here too is harmless but makes the + // two arms of is_contract() overlap, which reads as a bug to the next person. + $list = array_values( array_diff( $list, array( hook_prefix() ) ) ); + + return $list; +} + $root = isset( $argv[1] ) ? rtrim( $argv[1], '/' ) : '.'; $docs = $root . '/docs'; +global $conf_root; +$conf_root = $root; + $fails = 0; +/** + * Record a problem and print it where CI will pick it up. + * + * @param string $message What is wrong, naming the file or hook. + * @return void + */ function fail( $message ) { global $fails; - $fails++; + ++$fails; printf( "::error::%s\n", $message ); } @@ -193,7 +273,7 @@ foreach ( $required as $file => $floor ) { continue; } - $lines = count( file( $path, FILE_IGNORE_NEW_LINES ) ); + $lines = count( file( $path, FILE_IGNORE_NEW_LINES ) ); $lines_total += $lines; if ( $lines < $floor ) { @@ -214,7 +294,7 @@ printf( " %d documents, %d lines.\n", count( $required ), $lines_total ); * Named value, not "a version is mentioned": a check that merely finds a * version-shaped string passes on last year's. */ -$mains = glob( $root . '/*.php' ); +$mains = glob( $root . '/*.php' ); $version = ''; foreach ( (array) $mains as $main ) { @@ -253,22 +333,21 @@ if ( 0 !== $rc ) { exit( 1 ); } -$php = array_values( array_filter( $shipped, static function ( $f ) { - return '.php' === substr( $f, -4 ); -} ) ); +$php = array_values( + array_filter( + $shipped, + static function ( $f ) { + return '.php' === substr( $f, -4 ); + } + ) +); -// The floor comes from ci/plugin.conf, not from a literal. It arrived here as -// 20 — measured in the tree this file was written in — and refused to run in -// this 14-file one. A second hardcoded number per plugin is a second thing to -// re-measure on every port, and the conf already carries this one. -// -// The floor is not the fault: a copied kit that reads zero files prints a -// plausible number and passes. Refuse instead of reporting a green over -// nothing — and refuse rather than warn, because a warning next to a plausible -// number is read as a plausible number. $min_php = shipped_php_floor(); if ( count( $php ) < $min_php ) { + // The floor is not the fault: a copied kit that reads zero files prints a + // plausible number and passes. Refuse instead of reporting a green over + // nothing. fail( sprintf( 'Only %d shipped PHP files found, under the floor of %d. That is too few to be this tree — check the prefix and the paths before lowering this.', count( $php ), $min_php ) ); exit( 1 ); } @@ -341,9 +420,31 @@ foreach ( $php as $file ) { // floor gets lowered until the check reads nothing. $contract = (string) file_get_contents( $docs . '/CONTRACT.md' ); +/* + * 🔴 Table ROWS, not every backticked token in the file. + * + * Found by Nolimit Assistant from the other side and confirmed here by plant: + * delete a hook's table row, leave the name in one sentence of prose, and the + * first version printed "all 16 in docs/CONTRACT.md" and exited 0. A mention + * is not a row — the row carries the signature, what a caller gets when we are + * absent, and what a consumer must not do, and none of that is what a reader + * needs replaced by its own name in a sentence. + * + * It matters in the other direction too, which is how Assistant hit it: their + * first version read every backticked token and reported nine names as + * vanished hooks — two functions and seven WP_Error codes, every one of them + * correctly written in prose. Scoping to rows fixes both at once, and it makes + * the prose free: the tables are the contract, everything else is commentary, + * and commentary should not be able to satisfy or break a check. + * + * Verified against this document before narrowing: all 16 contract hooks are + * in rows, and the 30 backticked names that are not are field names, core + * hooks and the internal cron hooks listed under a heading that says they are + * explicitly not a contract. + */ $documented = array(); -if ( preg_match_all( '/`([a-z0-9_]+)`/', $contract, $m ) ) { +if ( preg_match_all( '/^\|[^|]*`([a-z0-9_]+)`/m', $contract, $m ) ) { foreach ( $m[1] as $name ) { $documented[ $name ] = true; } @@ -368,14 +469,63 @@ sort( $undocumented ); // rather than from a pattern over the hook names — a copied kit that guesses // the prefix from its own contents is how a check ends up reading zero files // in the next tree and printing a plausible number. -$prefix = ''; +$prefix = hook_prefix(); -if ( is_file( $root . '/ci/plugin.conf' ) && preg_match( '/^HOOK_PREFIX=([A-Za-z0-9_]+)/m', (string) file_get_contents( $root . '/ci/plugin.conf' ), $m ) ) { - $prefix = strtolower( $m[1] ); +/* + * 🔴 A wrong prefix is GREEN, and that is the whole reason this block exists. + * + * Measured on this tree by setting HOOK_PREFIX=zzzz_ : the run printed + * "6 hooks in 46 shipped PHP files, all 6 in docs/CONTRACT.md" and exited 0. + * Sixteen became six — only the wpmgr_ names survived — and nothing looked + * wrong, because six is a plausible number and every one of the six really was + * documented. Two wrongs cancelling into a pass, in a checker I had just told + * seven sibling sessions to copy into trees with seven different prefixes. + * + * Same shape as the copied kit that read zero always-loaded files: the port is + * not done when plugin.conf is, and the error you can see is never the one + * that matters. So: refuse. A plugin may genuinely define no hooks of its own, + * but that is a thing to state rather than to arrive at by a typo. + */ +$own = 0; + +foreach ( array_keys( $in_code ) as $hook ) { + if ( 0 === strpos( $hook, $prefix ) ) { + ++$own; + } } -if ( '' === $prefix ) { - fail( 'No HOOK_PREFIX in ci/plugin.conf — refusing to guess which documented names are ours.' ); +$min_own = 1; + +if ( is_file( $root . '/ci/plugin.conf' ) && preg_match( '/^HOOK_PREFIX_MIN=([0-9]+)/m', (string) file_get_contents( $root . '/ci/plugin.conf' ), $m ) ) { + $min_own = (int) $m[1]; +} + +if ( $own < $min_own ) { + // Nolimit Cache's improvement on the first version of this message, and it + // is the better one: say what IS there. "No hook carries nlsec_" leaves the + // reader to work out whether the scan found nothing at all or found the + // wrong things, and those need different fixes. + $present = array(); + + foreach ( array_keys( $in_code ) as $hook ) { + if ( preg_match( '/^([a-z]{2,8}_)/', $hook, $pm ) ) { + $present[ $pm[1] ] = isset( $present[ $pm[1] ] ) ? $present[ $pm[1] ] + 1 : 1; + } + } + + $summary = array(); + + foreach ( $present as $seen => $count ) { + $summary[] = sprintf( '%s* (%d)', $seen, $count ); + } + + $message = sprintf( + 'HOOK_PREFIX is %s* and no hook in the shipped source starts with it — the prefixes actually present are: %s. Either HOOK_PREFIX is wrong for this plugin, or this plugin defines none of its own; if it really defines none, say so in docs/CONTRACT.md and set HOOK_PREFIX_MIN=0 in ci/plugin.conf.', + $prefix, + $summary ? implode( ', ', $summary ) : 'none' + ); + + fail( $message ); exit( 1 ); } @@ -418,19 +568,23 @@ foreach ( array_keys( $documented ) as $name ) { sort( $stale ); if ( $undocumented ) { - fail( sprintf( - '%d hook(s) in the shipped source and not in docs/CONTRACT.md: %s', - count( $undocumented ), - implode( ', ', $undocumented ) - ) ); + fail( + sprintf( + '%d hook(s) in the shipped source and not in docs/CONTRACT.md: %s', + count( $undocumented ), + implode( ', ', $undocumented ) + ) + ); } if ( $stale ) { - fail( sprintf( - '%d name(s) in docs/CONTRACT.md that no longer exist in the source: %s', - count( $stale ), - implode( ', ', $stale ) - ) ); + fail( + sprintf( + '%d name(s) in docs/CONTRACT.md that no longer exist in the source: %s', + count( $stale ), + implode( ', ', $stale ) + ) + ); } if ( ! $undocumented && ! $stale ) { diff --git a/ci/dist-files.sh b/ci/dist-files.sh index 22be557..da020d6 100755 --- a/ci/dist-files.sh +++ b/ci/dist-files.sh @@ -25,7 +25,18 @@ done < .distignore # with nothing said. (-z also covers paths with spaces.) Found in a sibling # plugin that has exactly such a filename; this repo has none today, which is # precisely why it would have sat here until someone added one. -git ls-files -z | while IFS= read -r -d '' file; do +# In a git-less copy (ci_php's php container, which is handed exactly the +# tracked files and has no git) the tree IS the tracked set, so walk it. +# selftest.sh asserts the two forms print the same list. +tracked() { + if [ -e .git ]; then + git ls-files -z + else + find . -type f -print0 | sed -z 's|^\./||' | LC_ALL=C sort -z + fi +} + +tracked | while IFS= read -r -d '' file; do keep=1 for p in "${patterns[@]}"; do # A bare name excludes the file or directory wherever it appears, which is diff --git a/ci/plugin.conf b/ci/plugin.conf index a7e02b3..64f6b54 100644 --- a/ci/plugin.conf +++ b/ci/plugin.conf @@ -110,3 +110,6 @@ OLD_PLUGIN_NAME="WooCommerce Email Discount Manager" # run reports green having tested two thirds of what it claims. MIN_SITE_ASSERTIONS=60 HOOK_PREFIX=wc_edm_ +# Hook prefixes owned by the sibling plugins. Quoted because this file is +# sourced. Read from each sibling's HOOK_PREFIX, not guessed. +FAMILY_PREFIXES="wpmgr_ nlfrm_ nlseo_ nlvw_ kws_ nlass_ nlcac_ nlsec_" diff --git a/ci/selftest.sh b/ci/selftest.sh index 232b751..51175da 100755 --- a/ci/selftest.sh +++ b/ci/selftest.sh @@ -400,6 +400,19 @@ else fi cp "$work/helpers.orig" includes/class-helpers.php +# 1b. the same fault for a hook a SIBLING owns and we answer. Before +# FAMILY_PREFIXES this fell into "core, not ours to document" and stayed green. +printf '\n// Added by ci/selftest.sh.\nadd_filter( %s, %s );\n' \ + "'nlsec_selftest_sibling'" "'__return_true'" >> includes/class-helpers.php +if [ "$(grep -c 'nlsec_selftest_sibling' includes/class-helpers.php)" -lt 1 ]; then + bad "could not stage the sibling-hook fixture" +else + printf '%s' "$(docs_verdict)" | grep -q 'not in docs/CONTRACT.md: .*nlsec_selftest_sibling' \ + && ok "flagged a sibling-owned hook we answer and CONTRACT.md does not mention" \ + || bad "NOT FLAGGED: an undocumented sibling hook — the whole boundary to another plugin is invisible" +fi +cp "$work/helpers.orig" includes/class-helpers.php + # 2. 🔴 the control that matters: the same name, in a COMMENT only. The checker # must stay GREEN, and this asserts the specific success line rather than an # exit code — a refusal, a missing file or a fatal all exit non-zero too, and @@ -451,11 +464,29 @@ else fi git checkout -q -- docs/README.md 2>/dev/null || cp "$work/README.md.orig" docs/README.md +# 5b. the file list must be the same without git. ci_php runs check-docs.php in +# a php container that is handed the tracked files and has no git; dist-files.sh +# used to exit 127 there, so the docs check failed on the Gitea runner alone. +nogit=$(mktemp -d) +git ls-files -z | tar --null -T - -cf - | tar -xf - -C "$nogit" +if [ -e "$nogit/.git" ]; then + bad "could not stage the git-less copy" +else + with_git=$(./ci/dist-files.sh) + without_git=$(cd "$nogit" && ./ci/dist-files.sh 2>&1 || echo "exit $?") + if [ -n "$with_git" ] && [ "$with_git" = "$without_git" ]; then + ok "dist-files.sh lists the same $(printf '%s\n' "$with_git" | wc -l | tr -d ' ') files with and without git" + else + bad "dist-files.sh differs without git — the docs check goes red on the Gitea runner only" + fi +fi +rm -rf "$nogit" + # 6. both conf values must refuse rather than default. An unset floor parses to # 0 and every comparison against it passes, and a guessed prefix classifies # every hook in the tree as somebody else's. cp ci/plugin.conf "$work/conf.orig" -for key in HOOK_PREFIX MIN_SHIPPED_PHP_FILES; do +for key in HOOK_PREFIX MIN_SHIPPED_PHP_FILES FAMILY_PREFIXES; do grep -v "^$key=" "$work/conf.orig" > ci/plugin.conf if [ "$(grep -c "^$key=" ci/plugin.conf)" -ne 0 ]; then bad "could not stage the missing-$key fixture"