From df34fe8e54b8f589c3b23b5eb55deb44c4d46747 Mon Sep 17 00:00:00 2001 From: Twooey Date: Thu, 27 Aug 2026 15:40:25 -0400 Subject: [PATCH] Fix five high-severity bugs from the full-session code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ProductSync: bsc_work.wc_product_id could go stale if a product was ever deleted out-of-band (wp-admin, a cleanup script). wc_get_product() then correctly detects "no product" and creates a new one, but the repoint was gated behind `if (!$existing_id)` — which was already true, so the stale ID never got corrected. Every future sync repeated this, one duplicate product per run. Fixed by making the repoint unconditional (cheap, idempotent UPDATE either way). Reproduced the exact scenario against dev (deleted a product out-of-band, ran sync-products twice) and confirmed: one repoint, zero duplicates, product count and per-work product count both correct across repeated runs. - Commands.php (sync-hardcover-tags): wp_set_object_terms() with an empty array clears the taxonomy rather than leaving it alone — verified directly. Hardcover legitimately returns no moods/content-warnings for plenty of books, so a --force re-sync could silently wipe existing tags, including anything hand-tagged. Fixed by skipping the call per-category when that category's array is empty. Verified via a direct eval test: pre-existing genre tag survives a sync where genre comes back empty, mood still gets set normally. - docker-compose.yml: two stray root-owned secrets/db_password and secrets/db_root_password directories were already sitting on disk — Docker auto-creating a bind-mount source as a directory from an earlier manual `docker compose` call that ran without ENVIRONMENT exported (reproducing the exact bug already fixed once this session). Removed the stray dirs and changed every `${ENVIRONMENT}` in a volume mount to `${ENVIRONMENT:?ENVIRONMENT must be set}` so Compose now hard-fails instead of silently defaulting to empty. Verified: unset ENVIRONMENT now fails config validation with a clear error; set, it still works. - HARDCOVER_API_TOKEN moved to the same _FILE secrets pattern already used for DB_PASSWORD/DB_ROOT_PASSWORD (docker-compose.yml, deploy.sh, HardcoverAdapter.php) — it was a live, consumed secret still going through Compose's ${VAR} interpolation, exposed to the same mangling bug already fixed for the DB passwords, plus visible via `docker inspect`. deploy.sh now writes secrets//hardcover_api_token (optionally empty). Verified via deploy.sh dev + wp eval: empty file -> is_configured() false, a real token value -> true. - backup.sh: mariadb-dump had no --single-transaction, so a dump against a live site would either table-lock for its duration or produce a non-atomic/inconsistent dump. Added; verified a real dump still runs clean and produces a valid, restorable-looking .sql.gz. --- deploy/backup.sh | 8 ++++++- deploy/deploy.sh | 9 ++++++- docker-compose.yml | 21 +++++++++------- .../bookstore-core/includes/Cli/Commands.php | 21 ++++++++++++---- .../includes/Integration/HardcoverAdapter.php | 24 ++++++++++++++++--- .../includes/Product/ProductSync.php | 13 +++++++--- 6 files changed, 75 insertions(+), 21 deletions(-) diff --git a/deploy/backup.sh b/deploy/backup.sh index 906c3d6..0c77a51 100755 --- a/deploy/backup.sh +++ b/deploy/backup.sh @@ -29,7 +29,13 @@ grep -Ev '^(DB_PASSWORD|DB_ROOT_PASSWORD)=' "$ENV_FILE" > "$COMPOSE_ENV_FILE" COMPOSE="docker compose -p bookstore-${ENVIRONMENT} -f docker-compose.yml -f docker-compose.${ENVIRONMENT}.yml --env-file ${COMPOSE_ENV_FILE}" echo "==> dumping database" -$COMPOSE exec -T db sh -c "exec mariadb-dump -u\"\$MARIADB_USER\" -p\"\$(cat /run/secrets/db_password)\" \"\$MARIADB_DATABASE\"" \ +# --single-transaction: without it, a dump against a live site either +# table-locks for its duration (blocking writes) or, if MARIADB_USER lacks +# LOCK TABLES privilege, produces a non-atomic dump — rows written after the +# dump starts but before it reaches their table can be captured +# inconsistently with rows it already passed. InnoDB (this project's engine +# throughout) supports a consistent snapshot via a single transaction instead. +$COMPOSE exec -T db sh -c "exec mariadb-dump --single-transaction -u\"\$MARIADB_USER\" -p\"\$(cat /run/secrets/db_password)\" \"\$MARIADB_DATABASE\"" \ | gzip > "$BACKUP_DIR/db-${TIMESTAMP}.sql.gz" echo "==> archiving uploads" diff --git a/deploy/deploy.sh b/deploy/deploy.sh index df627e0..9cfa350 100755 --- a/deploy/deploy.sh +++ b/deploy/deploy.sh @@ -60,12 +60,19 @@ DB_ROOT_PASSWORD_VALUE="$(env_get "$ENV_FILE" DB_ROOT_PASSWORD)" : "${DB_ROOT_PASSWORD_VALUE:?set DB_ROOT_PASSWORD in ${ENV_FILE}}" printf '%s' "$DB_PASSWORD_VALUE" > "$SECRETS_DIR/db_password" printf '%s' "$DB_ROOT_PASSWORD_VALUE" > "$SECRETS_DIR/db_root_password" +# Optional, unlike the two above — an environment without a Hardcover token +# just leaves HardcoverAdapter::is_configured() false. Written unconditionally +# (even empty) so the bind mount in docker-compose.yml always has a real file +# to point at, never a directory Docker auto-creates for a missing path (the +# exact bug ENVIRONMENT-unset guards elsewhere in this file exist to prevent). +HARDCOVER_API_TOKEN_VALUE="$(env_get "$ENV_FILE" HARDCOVER_API_TOKEN)" +printf '%s' "$HARDCOVER_API_TOKEN_VALUE" > "$SECRETS_DIR/hardcover_api_token" # 644, not 600: the db/wordpress/cron containers read this as their own # (non-host-matching) container UID, e.g. www-data — chmod 600 made it # unreadable to them. The containing directory (700, above) is what # actually keeps other host users out; these just need to be world-readable # within that already-restricted directory. -chmod 644 "$SECRETS_DIR/db_password" "$SECRETS_DIR/db_root_password" +chmod 644 "$SECRETS_DIR/db_password" "$SECRETS_DIR/db_root_password" "$SECRETS_DIR/hardcover_api_token" echo "==> building and starting ${ENVIRONMENT}" $COMPOSE up -d --build diff --git a/docker-compose.yml b/docker-compose.yml index a50bf7a..27764de 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -22,10 +22,11 @@ # bind mount never touches the file's content, only its path, so it's # immune to this entirely. # -# The supplier/payment keys below are still passed the old ${VAR} way and -# remain exposed to the same class of bug — nothing consumes them yet -# (bookstore-core is still a stub), so when that code is written it should -# read `_FILE` variants the same way. +# The remaining supplier/payment keys below are still passed the old ${VAR} +# way and remain exposed to the same class of bug — nothing consumes them +# yet (bookstore-core is still a stub for those), so when that code is +# written it should read `_FILE` variants the same way HARDCOVER_API_TOKEN +# now does below. x-bookstore-env: &bookstore-env WORDPRESS_DB_HOST: db WORDPRESS_DB_NAME: ${DB_NAME} @@ -47,7 +48,7 @@ x-bookstore-env: &bookstore-env HELCIM_API_TOKEN: ${HELCIM_API_TOKEN:-} HELCIM_ACCOUNT_ID: ${HELCIM_ACCOUNT_ID:-} MAILERLITE_API_KEY: ${MAILERLITE_API_KEY:-} - HARDCOVER_API_TOKEN: ${HARDCOVER_API_TOKEN:-} + HARDCOVER_API_TOKEN_FILE: /run/secrets/hardcover_api_token services: db: @@ -60,8 +61,8 @@ services: MARIADB_ROOT_PASSWORD_FILE: /run/secrets/db_root_password volumes: - db_data:/var/lib/mysql - - ./secrets/${ENVIRONMENT}/db_password:/run/secrets/db_password:ro - - ./secrets/${ENVIRONMENT}/db_root_password:/run/secrets/db_root_password:ro + - ./secrets/${ENVIRONMENT:?ENVIRONMENT must be set}/db_password:/run/secrets/db_password:ro + - ./secrets/${ENVIRONMENT:?ENVIRONMENT must be set}/db_root_password:/run/secrets/db_root_password:ro healthcheck: test: ["CMD-SHELL", "mariadb-admin ping -h 127.0.0.1 -u$$MARIADB_USER -p\"$$(cat /run/secrets/db_password)\" --silent"] interval: 5s @@ -95,7 +96,8 @@ services: - wp_themes:/var/www/html/wp-content/themes - ./wp-content/plugins/bookstore-core:/var/www/html/wp-content/plugins/bookstore-core - ./wp-content/mu-plugins:/var/www/html/wp-content/mu-plugins - - ./secrets/${ENVIRONMENT}/db_password:/run/secrets/db_password:ro + - ./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 cron: build: @@ -119,7 +121,8 @@ services: - ./wp-content/plugins/bookstore-core:/var/www/html/wp-content/plugins/bookstore-core - ./wp-content/mu-plugins:/var/www/html/wp-content/mu-plugins - ./docker/cron/entrypoint.sh:/usr/local/bin/cron-entrypoint.sh:ro - - ./secrets/${ENVIRONMENT}/db_password:/run/secrets/db_password:ro + - ./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 volumes: db_data: diff --git a/wp-content/plugins/bookstore-core/includes/Cli/Commands.php b/wp-content/plugins/bookstore-core/includes/Cli/Commands.php index 662b181..b6a34d1 100644 --- a/wp-content/plugins/bookstore-core/includes/Cli/Commands.php +++ b/wp-content/plugins/bookstore-core/includes/Cli/Commands.php @@ -174,10 +174,23 @@ class Commands $book = HardcoverAdapter::find_book($isbn13, $work->title, $work->primary_author); if ($book) { $tags = HardcoverAdapter::tags_for_book($book); - wp_set_object_terms($product_id, $tags['genre'], TagTaxonomies::GENRE, false); - wp_set_object_terms($product_id, $tags['mood'], TagTaxonomies::MOOD, false); - wp_set_object_terms($product_id, $tags['content_warning'], TagTaxonomies::CONTENT_WARNING, false); - wp_set_object_terms($product_id, $tags['tag'], TagTaxonomies::TAG, false); + // wp_set_object_terms() with an empty array CLEARS the + // taxonomy rather than leaving it untouched — verified + // directly. Hardcover legitimately returns no moods/ + // content-warnings for plenty of books, and the schema + // here is still unverified, so an empty category must + // not be treated as "wipe whatever was there before" + // (including anything hand-tagged by an admin). + foreach ([ + TagTaxonomies::GENRE => $tags['genre'], + TagTaxonomies::MOOD => $tags['mood'], + TagTaxonomies::CONTENT_WARNING => $tags['content_warning'], + TagTaxonomies::TAG => $tags['tag'], + ] as $taxonomy => $terms) { + if (!empty($terms)) { + wp_set_object_terms($product_id, $terms, $taxonomy, false); + } + } $matched++; } else { $missed++; diff --git a/wp-content/plugins/bookstore-core/includes/Integration/HardcoverAdapter.php b/wp-content/plugins/bookstore-core/includes/Integration/HardcoverAdapter.php index 8365a7f..e600899 100644 --- a/wp-content/plugins/bookstore-core/includes/Integration/HardcoverAdapter.php +++ b/wp-content/plugins/bookstore-core/includes/Integration/HardcoverAdapter.php @@ -21,7 +21,25 @@ class HardcoverAdapter public static function is_configured(): bool { - return (bool) getenv('HARDCOVER_API_TOKEN'); + return self::token() !== null; + } + + /** + * Read from the file HARDCOVER_API_TOKEN_FILE points at, not a plain + * ${VAR}-style env var — same reasoning as DB_PASSWORD/DB_ROOT_PASSWORD + * (see docker-compose.yml): Compose's ${VAR} interpolation mangles any + * value containing `$` followed by a letter, and a plain env var is also + * visible via `docker inspect`/`/proc//environ` to anything with + * host/container access, unlike a 644 file inside a 700 directory. + */ + private static function token(): ?string + { + $file = getenv('HARDCOVER_API_TOKEN_FILE'); + if (!$file || !is_readable($file)) { + return null; + } + $value = trim((string) file_get_contents($file)); + return $value !== '' ? $value : null; } /** @@ -129,9 +147,9 @@ class HardcoverAdapter private static function query(string $query, array $variables = []): array { - $token = getenv('HARDCOVER_API_TOKEN'); + $token = self::token(); if (!$token) { - throw new \RuntimeException('HARDCOVER_API_TOKEN is not set'); + throw new \RuntimeException('HARDCOVER_API_TOKEN_FILE is not set or empty'); } $response = wp_remote_post(self::ENDPOINT, [ diff --git a/wp-content/plugins/bookstore-core/includes/Product/ProductSync.php b/wp-content/plugins/bookstore-core/includes/Product/ProductSync.php index 9553699..4009ef7 100644 --- a/wp-content/plugins/bookstore-core/includes/Product/ProductSync.php +++ b/wp-content/plugins/bookstore-core/includes/Product/ProductSync.php @@ -165,9 +165,16 @@ class ProductSync $product_id = $product->save(); update_post_meta($product_id, '_bsc_work_id', $work->work_id); - if (!$existing_id) { - Work::set_product_id((int) $work->work_id, $product_id); - } + // Unconditional, not "if new": $existing_id can be a stale pointer to + // a product that was deleted out-of-band (wp-admin, a cleanup script, + // anything that didn't also clear bsc_work.wc_product_id). In that + // case wc_get_product() above returned false, a *new* product was + // just created, and the guard this replaced (`if (!$existing_id)`) + // would have skipped repointing bsc_work at it — leaving the stale ID + // in place forever and creating another duplicate on every future + // run. This UPDATE is idempotent, so making it unconditional costs + // nothing on the normal "already correct" path. + Work::set_product_id((int) $work->work_id, (int) $product_id); self::sync_categories($product_id, $work->subjects ?? null);