From e742a540a4c9082d12c5cc964ac6423c99076e58 Mon Sep 17 00:00:00 2001 From: houseme Date: Sat, 11 Jul 2026 22:51:24 +0800 Subject: [PATCH] test(cache): guard the body-cache eligibility gate against deny-list regressions (#1146) (#4742) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `full_object_plaintext_len` decides whether a body-cache hook hit may serve bytes in place of the erasure read. It is a fail-closed allow-list: it excludes every read whose `ReadPlan::build` applies some other transform (ranged/part, raw/data-movement, restore, encrypted, remote) with an early `return None`, then returns a `Some(..)` length only for the whole-plaintext cases. A newly added `ReadPlan` branch that nobody teaches this gate about falls through to `None` and safely bypasses the cache. Flip it to a deny-list and the same new branch silently serves bytes in the wrong representation — the backlog#1108 / #1109 / #1146 class of bug. The existing unit and e2e tests only cover the branches that exist today. This adds `scripts/check_body_cache_whitelist.sh`, a structural guard wired into pre-commit / pre-pr / dev-check and CI, that asserts every exclusion predicate and a `return None` still precede the first `Some(..)`. Reordering a predicate, dropping one, moving the positive return ahead of the gate, or renaming/removing the function all fail; wording, formatting, and adding a new exclusion in the same gate do not. Mutation-tested against all four regression shapes. This machine-enforces the structural invariant that backlog#1146 was kept open to guard by hand. Co-authored-by: heihutu --- .config/make/lint-fmt.mak | 5 ++ .config/make/pre-commit.mak | 6 +- .github/workflows/ci.yml | 3 + crates/ecstore/src/set_disk/ops/object.rs | 5 ++ scripts/check_body_cache_whitelist.sh | 96 +++++++++++++++++++++++ 5 files changed, 112 insertions(+), 3 deletions(-) create mode 100755 scripts/check_body_cache_whitelist.sh diff --git a/.config/make/lint-fmt.mak b/.config/make/lint-fmt.mak index 73d315976..56d721f91 100644 --- a/.config/make/lint-fmt.mak +++ b/.config/make/lint-fmt.mak @@ -55,6 +55,11 @@ extension-schema-check: ## Check extension-schema stays a lightweight contract c @echo "🧩 Checking extension schema boundaries..." ./scripts/check_extension_schema_boundaries.sh +.PHONY: body-cache-whitelist-check +body-cache-whitelist-check: ## Check the body-cache eligibility gate stays a fail-closed allow-list + @echo "🧱 Checking body-cache whitelist guard..." + ./scripts/check_body_cache_whitelist.sh + .PHONY: compilation-check compilation-check: core-deps ## Run compilation check @echo "🔨 Running compilation check..." diff --git a/.config/make/pre-commit.mak b/.config/make/pre-commit.mak index 842f49ef4..e812ad71b 100644 --- a/.config/make/pre-commit.mak +++ b/.config/make/pre-commit.mak @@ -14,13 +14,13 @@ doc-paths-check: ## Check that instruction/architecture docs reference existing ./scripts/check_doc_paths.sh .PHONY: pre-commit -pre-commit: fmt-check unsafe-code-check architecture-migration-check logging-guardrails-check tokio-io-uring-check extension-schema-check doc-paths-check quick-check ## Run fast pre-commit checks without clippy/full tests +pre-commit: fmt-check unsafe-code-check architecture-migration-check logging-guardrails-check tokio-io-uring-check extension-schema-check body-cache-whitelist-check doc-paths-check quick-check ## Run fast pre-commit checks without clippy/full tests @echo "✅ All pre-commit checks passed!" .PHONY: pre-pr -pre-pr: fmt-check unsafe-code-check architecture-migration-check logging-guardrails-check tokio-io-uring-check extension-schema-check doc-paths-check clippy-check test ## Run full pre-PR checks with clippy and tests +pre-pr: fmt-check unsafe-code-check architecture-migration-check logging-guardrails-check tokio-io-uring-check extension-schema-check body-cache-whitelist-check doc-paths-check clippy-check test ## Run full pre-PR checks with clippy and tests @echo "✅ All pre-PR checks passed!" .PHONY: dev-check -dev-check: fmt-check unsafe-code-check architecture-migration-check logging-guardrails-check tokio-io-uring-check extension-schema-check doc-paths-check quick-check ## Run fast local development checks +dev-check: fmt-check unsafe-code-check architecture-migration-check logging-guardrails-check tokio-io-uring-check extension-schema-check body-cache-whitelist-check doc-paths-check quick-check ## Run fast local development checks @echo "✅ Fast development checks passed!" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bad5aaada..4f5a823e5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -131,6 +131,9 @@ jobs: - name: Check extension schema boundaries run: ./scripts/check_extension_schema_boundaries.sh + - name: Check body-cache whitelist guard + run: ./scripts/check_body_cache_whitelist.sh + test-and-lint: name: Test and Lint if: github.event_name != 'pull_request' || github.event.action != 'closed' diff --git a/crates/ecstore/src/set_disk/ops/object.rs b/crates/ecstore/src/set_disk/ops/object.rs index 88e7346d1..0fca28cb2 100644 --- a/crates/ecstore/src/set_disk/ops/object.rs +++ b/crates/ecstore/src/set_disk/ops/object.rs @@ -48,6 +48,11 @@ use crate::object_api::GetObjectBodySource; /// Compressed objects ARE eligible: `ReadTransform::Compressed` returns the full /// plaintext and rewrites `object_info.size` to the decompressed length, which /// the caller must replicate with the returned length (backlog#1109). +/// +/// The fail-closed shape here is enforced by `scripts/check_body_cache_whitelist.sh` +/// (backlog#1146): the guard requires every exclusion predicate and a `return +/// None` to precede the first `Some(..)`, so a refactor to a deny-list fails CI. +/// If you rename or move this function, update that script. fn full_object_plaintext_len(range: &Option, opts: &ObjectOptions, object_info: &ObjectInfo) -> Option { if range.is_some() || opts.part_number.is_some() diff --git a/scripts/check_body_cache_whitelist.sh b/scripts/check_body_cache_whitelist.sh new file mode 100755 index 000000000..38f034aa5 --- /dev/null +++ b/scripts/check_body_cache_whitelist.sh @@ -0,0 +1,96 @@ +#!/usr/bin/env bash + +set -euo pipefail + +# Guard: the app-layer body cache eligibility gate must stay a fail-closed +# allow-list. +# +# `full_object_plaintext_len` (crates/ecstore/src/set_disk/ops/object.rs) +# decides whether a body-cache hook hit may serve bytes IN PLACE of the erasure +# read. A hit bypasses `ReadPlan`/`ReadTransform` entirely, so it is sound only +# where the normal read path would return that exact plaintext under that exact +# `object_info.size`. The function encodes this as a positive allow-list: it +# excludes every read whose `ReadPlan::build` applies some other transform +# (ranged/part reads, raw/data-movement reads, restore reads, encrypted and +# remote objects) with an early `return None`, THEN — and only then — returns a +# `Some(..)` length for the whole-plaintext cases. +# +# That ordering is the whole invariant. A newly added `ReadPlan` branch that +# nobody teaches this gate about falls through to `None` and safely bypasses the +# cache. Flip the structure to a deny-list — default `Some(..)`, subtract known +# bad cases — and the same new branch silently serves bytes in the WRONG +# representation. That is exactly the backlog#1108 / #1109 / #1146 class of bug +# (an erasure read returning a decrypted/decompressed body under a stored-byte +# `size`, or vice versa). Unit and e2e tests only cover the branches that exist +# today; this guard is what keeps a future refactor from regressing the shape. +# +# The check is deliberately structural, not a byte-for-byte snapshot: it asserts +# that each exclusion predicate and a `return None` appear BEFORE the first +# `Some(` in the function body. Reordering a predicate, dropping one, or moving +# the positive return ahead of the gate all fail; harmless edits (wording, +# adding a new exclusion in the same gate, formatting) do not. + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +ROOT_DIR="${CHECK_BODY_CACHE_WHITELIST_ROOT:-$(cd "${SCRIPT_DIR}/.." && pwd)}" + +TARGET="crates/ecstore/src/set_disk/ops/object.rs" +FN="full_object_plaintext_len" + +# Exclusion predicates that MUST be checked (returning None) before any positive +# Some(..) return. Order-independent; presence-and-position is what matters. +REQUIRED_PREDICATES=( + "range.is_some()" + "opts.part_number.is_some()" + "opts.raw_data_movement_read" + "opts.data_movement" + "restore_request_active(opts)" + "object_info.is_encrypted()" + "object_info.is_remote()" +) + +fail() { + printf 'Body-cache whitelist guard failed: %s\n' "$1" >&2 + exit 1 +} + +FILE="${ROOT_DIR}/${TARGET}" +[[ -f "$FILE" ]] || fail "${TARGET} is missing (did the file move? update this guard)" + +# Extract the function body: from the `fn (` signature line to the first +# closing brace in column 0 (the function is a free fn at module scope). awk +# prints the body with 1-based relative line numbers so we can reason about +# ordering without absolute offsets. +BODY="$(awk -v fn="fn ${FN}(" ' + index($0, fn) == 1 { inside = 1; n = 0 } + inside { + n++ + print n "\t" $0 + if (started && $0 ~ /^}/) { exit } + if ($0 ~ /\{[[:space:]]*$/) started = 1 + } +' "$FILE")" + +[[ -n "$BODY" ]] || fail "fn ${FN} not found in ${TARGET} (renamed or removed? update this guard)" + +# Line number (relative to the body) of the first positive `Some(` return. The +# allow-list's positive returns must all sit AFTER the exclusion gate, so this +# is the boundary every exclusion must precede. +some_line="$(awk -F'\t' '$2 ~ /Some\(/ { print $1; exit }' <<<"$BODY")" +[[ -n "$some_line" ]] || fail "fn ${FN} has no Some(..) return — the allow-list positive branch vanished; review by hand" + +# A `return None` must gate the excluded reads before the first positive return. +none_line="$(awk -F'\t' '$2 ~ /return None/ { print $1; exit }' <<<"$BODY")" +[[ -n "$none_line" ]] || fail "fn ${FN} has no \`return None\` before its positive branch — fail-closed gate is gone" +[[ "$none_line" -lt "$some_line" ]] || fail "\`return None\` gate no longer precedes the first Some(..) — structure looks like a deny-list" + +# Every required exclusion predicate must appear, and appear before the first +# positive Some(..). A missing or relocated predicate means some read that must +# bypass the cache can now reach a cache hit. +for pred in "${REQUIRED_PREDICATES[@]}"; do + pred_line="$(awk -F'\t' -v p="$pred" 'index($2, p) { print $1; exit }' <<<"$BODY")" + [[ -n "$pred_line" ]] || fail "exclusion predicate \`${pred}\` missing from fn ${FN} — a refused read can now hit the cache" + [[ "$pred_line" -lt "$some_line" ]] || + fail "exclusion predicate \`${pred}\` no longer precedes the first Some(..) — it is not gating the allow-list" +done + +echo "Body-cache whitelist guard passed."