From e4ee584e865d878408bf35bcdcc73a8210381cdb Mon Sep 17 00:00:00 2001 From: Twooey Date: Thu, 27 Aug 2026 15:57:13 -0400 Subject: [PATCH] Fix six low-severity issues from the full-session code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - PricingEngine::round_to_99(): ceil($price) - 0.01 undershoots whenever $price's cents are already .99 or higher — an exact integer (ceil() equals floor(), landing a full cent below $price) or, more subtly, any fractional price above X.99 itself (a division result, not something pre-rounded to 2 decimals, e.g. 20.995 -> old formula gave 20.99, below the input). Rewritten as floor()+0.99, bumped by 1 if still under $price. Verified against 8 cases including both boundary classes: every result now >= its input. - CoverSync::attach_cover(): wp_generate_attachment_metadata()'s return value was never checked. Assumed it'd return empty on failure — verified directly it does NOT: fed it 2000 bytes of garbage and got back ['filesize' => 2000], no width/height, since GD/Imagick couldn't decode it. Old code would report "attached" for a degraded image with no dimensions/srcset. Now checks for width+height specifically, and cleans up the orphaned attachment on failure so a re-run retries the product. Verified both the corrupt-image rejection (no orphan left, no thumbnail set) and that a real image still attaches normally. - Makefile: the per-invocation .env.$(ENV).compose file (holds every API key and both DB passwords, stripped of DB_PASSWORD/DB_ROOT_PASSWORD only) was never cleaned up, left at default 644 in the repo root after every `make` command. Now chmod 600 on creation and removed at the end of every target, preserving the underlying command's exit code through the cleanup. Verified both the happy path (file gone after, exit 0) and the failure path (bad ENV: file still cleaned up, real exit code still propagates through make). - docker-compose.yml: added a healthcheck to the wordpress service (bash's /dev/tcp against php-fpm's port 9000 — no HTTP endpoint to hit directly, and no `nc` in this image; verified it correctly succeeds once php-fpm is listening and fails against a closed port) and switched cron's and caddy's depends_on (across all three env overlays) from bare container-started to condition: service_healthy. Previously both could start against a wordpress container that had started but wasn't actually ready yet. Verified via a full down/up cycle: db+redis healthy, then wordpress starts and becomes healthy, only then do cron and caddy start. - .env.dev/.env.staging/.env.production chmod'd 600 (were 644) — same plaintext-credential content as secrets//, which is already 700/644 at the directory/file level respectively for a different reason (container UID readability); these have no such constraint, only the host CLI reads them. Also removed a stray .env.staging.compose left over from before the Makefile fix above existed. Noted the convention in .env.example so newly created env files follow it too. --- .env.example | 4 ++- Makefile | 29 +++++++++++++------ docker-compose.dev.yml | 3 +- docker-compose.production.yml | 3 +- docker-compose.staging.yml | 3 +- docker-compose.yml | 18 +++++++++++- .../includes/Pricing/PricingEngine.php | 17 ++++++++++- .../includes/Product/CoverSync.php | 17 +++++++++++ 8 files changed, 79 insertions(+), 15 deletions(-) diff --git a/.env.example b/.env.example index b71c6f5..e34f70d 100644 --- a/.env.example +++ b/.env.example @@ -1,5 +1,7 @@ # Copy this to .env.dev / .env.staging / .env.production and fill in real -# values for that environment. The filled-in files are gitignored — never +# values for that environment, then `chmod 600` it — it holds every API key +# and both DB passwords in plaintext, same as the secrets// files +# deploy.sh derives from it. The filled-in files are gitignored — never # commit them. Same code everywhere; only these values differ (see # design doc §01, Environments & deployment). # diff --git a/Makefile b/Makefile index 13b4872..0d5389c 100644 --- a/Makefile +++ b/Makefile @@ -4,34 +4,45 @@ export ENVIRONMENT = $(ENV) # only flow through secrets/ now, and Compose warns "variable not set" on # any $-shaped value in --env-file even when nothing consumes it. Rewritten # fresh on every `make` invocation, so it can't drift from .env.$(ENV). +# chmod 600 (not the 644 elsewhere in deploy/ — those get read by containers +# under a different UID; this one is only ever read by the `docker compose` +# CLI on the host, as $(ENV) invokes it, so there's no reason it needs to be +# world-readable) — and every target below removes it on the way out (see +# each recipe's `; rc=$$?; rm -f ...; exit $$rc`) rather than leaving a +# plaintext copy of every API key and both DB passwords sitting in the repo +# root after every `make` invocation. Make has no built-in "on exit" +# hook across arbitrary targets, so this is repeated per-target rather than +# centralized; `$$rc`/`exit $$rc` preserves the underlying command's exit +# code through the cleanup so a real failure (e.g. deploy.sh erroring) still +# fails the `make` invocation. COMPOSE_ENV_FILE := .env.$(ENV).compose -$(shell grep -Ev '^(DB_PASSWORD|DB_ROOT_PASSWORD)=' .env.$(ENV) > $(COMPOSE_ENV_FILE) 2>/dev/null) +$(shell grep -Ev '^(DB_PASSWORD|DB_ROOT_PASSWORD)=' .env.$(ENV) > $(COMPOSE_ENV_FILE) 2>/dev/null; chmod 600 $(COMPOSE_ENV_FILE) 2>/dev/null) COMPOSE = docker compose -p bookstore-$(ENV) -f docker-compose.yml -f docker-compose.$(ENV).yml --env-file $(COMPOSE_ENV_FILE) .PHONY: up down ps logs shell wp deploy backup up: - $(COMPOSE) up -d --build + $(COMPOSE) up -d --build; rc=$$?; rm -f $(COMPOSE_ENV_FILE); exit $$rc down: - $(COMPOSE) down + $(COMPOSE) down; rc=$$?; rm -f $(COMPOSE_ENV_FILE); exit $$rc ps: - $(COMPOSE) ps + $(COMPOSE) ps; rc=$$?; rm -f $(COMPOSE_ENV_FILE); exit $$rc logs: - $(COMPOSE) logs -f + $(COMPOSE) logs -f; rc=$$?; rm -f $(COMPOSE_ENV_FILE); exit $$rc # add -u root yourself for one-off root debugging (installing a package, etc.) shell: - $(COMPOSE) exec -u www-data wordpress bash + $(COMPOSE) exec -u www-data wordpress bash; rc=$$?; rm -f $(COMPOSE_ENV_FILE); exit $$rc # make wp ENV=staging ARGS="plugin list" wp: - $(COMPOSE) exec -u www-data wordpress wp $(ARGS) + $(COMPOSE) exec -u www-data wordpress wp $(ARGS); rc=$$?; rm -f $(COMPOSE_ENV_FILE); exit $$rc deploy: - ./deploy/deploy.sh $(ENV) + ./deploy/deploy.sh $(ENV); rc=$$?; rm -f $(COMPOSE_ENV_FILE); exit $$rc backup: - ./deploy/backup.sh $(ENV) + ./deploy/backup.sh $(ENV); rc=$$?; rm -f $(COMPOSE_ENV_FILE); exit $$rc diff --git a/docker-compose.dev.yml b/docker-compose.dev.yml index b31dc68..07bca03 100644 --- a/docker-compose.dev.yml +++ b/docker-compose.dev.yml @@ -12,7 +12,8 @@ services: image: caddy:2-alpine restart: unless-stopped depends_on: - - wordpress + wordpress: + condition: service_healthy ports: - "8080:80" volumes: diff --git a/docker-compose.production.yml b/docker-compose.production.yml index 781bdd4..5b4178e 100644 --- a/docker-compose.production.yml +++ b/docker-compose.production.yml @@ -12,7 +12,8 @@ services: image: caddy:2-alpine restart: unless-stopped depends_on: - - wordpress + wordpress: + condition: service_healthy ports: - "80:80" - "443:443" diff --git a/docker-compose.staging.yml b/docker-compose.staging.yml index 525a8b0..c8e8d30 100644 --- a/docker-compose.staging.yml +++ b/docker-compose.staging.yml @@ -12,7 +12,8 @@ services: image: caddy:2-alpine restart: unless-stopped depends_on: - - wordpress + wordpress: + condition: service_healthy ports: - "80:80" - "443:443" diff --git a/docker-compose.yml b/docker-compose.yml index 27764de..ceda43f 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -98,6 +98,21 @@ services: - ./wp-content/mu-plugins:/var/www/html/wp-content/mu-plugins - ./secrets/${ENVIRONMENT:?ENVIRONMENT must be set}/db_password:/run/secrets/db_password:ro - ./secrets/${ENVIRONMENT:?ENVIRONMENT must be set}/hardcover_api_token:/run/secrets/hardcover_api_token:ro + healthcheck: + # No HTTP endpoint to hit directly — php-fpm speaks FastCGI on 9000, + # not HTTP, and this image has no `nc`. bash's /dev/tcp redirection + # needs no extra binary and is enough to know php-fpm is actually + # accepting connections (verified directly: exits 0 once php-fpm is up, + # 1 against a closed port). Without this, cron/caddy's depends_on only + # waited for the container to *start*, not for php-fpm inside it to be + # ready — a slow first boot (official image copying core files in, could + # be worse on modest hardware) could let Caddy serve a transient 502 + # before php-fpm was actually listening. + test: ["CMD-SHELL", "bash -c '(exec 3<>/dev/tcp/127.0.0.1/9000)' 2>/dev/null"] + interval: 5s + timeout: 3s + retries: 20 + start_period: 30s cron: build: @@ -111,7 +126,8 @@ services: # wp-cli commands from writing new files into those same directories. user: www-data depends_on: - - wordpress + wordpress: + condition: service_healthy entrypoint: ["/bin/sh", "/usr/local/bin/cron-entrypoint.sh"] environment: *bookstore-env volumes: diff --git a/wp-content/plugins/bookstore-core/includes/Pricing/PricingEngine.php b/wp-content/plugins/bookstore-core/includes/Pricing/PricingEngine.php index 1088d00..e139ea9 100644 --- a/wp-content/plugins/bookstore-core/includes/Pricing/PricingEngine.php +++ b/wp-content/plugins/bookstore-core/includes/Pricing/PricingEngine.php @@ -26,9 +26,24 @@ class PricingEngine return self::round_to_99($raw); } + /** + * Smallest X.99 that is still >= $price — never below it, since this + * exists to preserve the margin calculate_retail_price() just computed. + * ceil($price) - 0.01 (the previous formula) gets this wrong whenever + * $price's cents are already .99 or higher: an exact integer (ceil() + * equals floor() there, so the result lands a full cent BELOW $price) + * or, more subtly, any fractional price above X.99 itself (arithmetically + * possible — this is a division result, not something pre-rounded to 2 + * decimals, e.g. $price = 20.995 -> old formula gave 20.99, which is + * below $price). + */ public static function round_to_99(float $price): float { - return ceil($price) - 0.01; + $candidate = floor($price) + 0.99; + if ($candidate < $price) { + $candidate += 1; + } + return round($candidate, 2); } /** Null if the work has no currently sellable offer (design doc §03: fulfillment resolved at render time). */ diff --git a/wp-content/plugins/bookstore-core/includes/Product/CoverSync.php b/wp-content/plugins/bookstore-core/includes/Product/CoverSync.php index df4b593..4473fa4 100644 --- a/wp-content/plugins/bookstore-core/includes/Product/CoverSync.php +++ b/wp-content/plugins/bookstore-core/includes/Product/CoverSync.php @@ -98,6 +98,23 @@ class CoverSync } $metadata = wp_generate_attachment_metadata($attachment_id, $upload['file']); + // A corrupt/truncated image can pass fetch_and_verify()'s + // content-type + byte-size checks (not full JPEG validation) yet + // still fail here. Confirmed directly: for unreadable image bytes, + // wp_generate_attachment_metadata() does NOT return an empty array — + // it returns something like ['filesize' => 2000] with no width/ + // height, since GD/Imagick couldn't decode it. empty() alone misses + // this; width/height (present on any image WordPress actually + // decoded) is the real signal. Left unchecked, set_post_thumbnail() + // below would still succeed (the attachment post itself is valid), + // silently reporting "attached" for a degraded image with no + // width/height/srcset. Clean up and count it as missed instead, so a + // re-run retries this product rather than has_post_thumbnail() + // treating the broken attachment as done. + if (empty($metadata) || !isset($metadata['width'], $metadata['height'])) { + wp_delete_attachment($attachment_id, true); + return false; + } wp_update_attachment_metadata($attachment_id, $metadata); return (bool) set_post_thumbnail($product_id, $attachment_id);