Skip to content

fix: wrapper download-chain integrity + honest runtime version (#392, #393) - #398

Open
kiki830621 wants to merge 5 commits into
mainfrom
idd/392-393-wrapper-integrity
Open

fix: wrapper download-chain integrity + honest runtime version (#392, #393)#398
kiki830621 wants to merge 5 commits into
mainfrom
idd/392-393-wrapper-integrity

Conversation

@kiki830621

@kiki830621 kiki830621 commented Aug 26, 2026

Copy link
Copy Markdown
Member

Refs #392
Refs #393

Cluster PR(兩案同檔 plugin/bin/che-apple-mail-mcp-wrapper.sh,Conflict Class C —— 分開改必互撞)。

本 body 於 2026-09-09 重寫(#398 verify round 3)。 前一版描述的是 round-1 的設計,其中一項機制(curl --fail已被 head 刻意否決,測試數字停在 25/25,並列了一列實測不存在的覆蓋。PR body 是決定合併時實際被讀的東西,讓它落後於 head 等於把 #318 的「closing comment 是死信」提早一個 artifact 重演。

#392 — 下載鏈完整性

  1. HTTP 狀態碼判讀(不是 curl --fail。前一版 body 說要用 --fail;head 明確拒絕:

    # Deliberately NOT -f: distinguishing a definitive 404 from rate-limits /
    # timeouts / 5xx is the whole point
    

    因為 --fail 會把「pinned tag 確定不存在(404)」與「暫時性失敗(403 rate-limit / 5xx / timeout)」壓成同一種 curl 失敗,而這兩者必須分開才能決定「fallback 一次並記 marker」還是「保留現有 binary、下次 spawn 重試」。改用 -w '%{http_code}' 讀狀態碼。錯誤頁的防線因此是另外兩道:狀態碼閘 + looks_like_html 形狀檢查(後者在 round 3 之前沒有測試,見下)。

  2. sha256 驗證,fail closed。digest 取自 release 自己的 asset list;比對在 chmod / mv 之前。唯一允許未驗證安裝的情形是該 release 根本沒發布 digest。「本機算不出 digest」(無 hash 工具、或 shasum 存在但壞掉)是拒裝,不是降級 —— round 1 在這裡 fail open,正是 bug: wrapper 下載鏈零完整性驗證 — curl 缺 --fail、有 .sha256 asset 卻不驗、pinned-tag 落空每次 spawn 重下載 (#335 verify) #392 要關掉的洞。

  3. asset URL 結構化驗證(round 3 新增)。round 2 用字串前綴比對「釘住」host,round 3 證明那不是釘住:curl 會在送出請求前正規化路徑。實測 curl -w '%{url_effective}'

    .../che-apple-mail-mcp/releases/download/../../../../attacker-org/evil-repo/releases/download/v1/CheAppleMailMCP
      → https://github.com/attacker-org/evil-repo/releases/download/v1/CheAppleMailMCP
    

    .sha256 走同一條選取規則、來自同一個攻擊者路徑,所以驗證會通過、印出 (sha256 verified)、然後 exec 別人的 binary。#fragment 同理在送出前被丟棄,於是 basename 檢查可以被伺服器根本看不到的文字滿足。現在改為結構化驗證:/download/恰好兩段、無 query、無 fragment、任何拼法的 dot-segment 皆拒。

  4. latest-fallback 迴圈 guard。pinned tag 確定 404 → fallback latest 一次並寫 .fallback-tried<pin> <epoch> <miss|verify>,24h TTL)。pin 改變、TTL 到期、或手動刪檔皆解除。

#393 — runtime state 記實際版本

version_at_spawn 原寫 DESIRED_VERSION(期望值)——下載失敗保舊 binary 時 runtime 記著新版號,staleness hook 比對 plugin.json 恆相等 → 假陰性永不 respawn。現在寫入前重讀 sidecar;sidecar 無法解析 tag 時寫 unknown回填 pin(回填 pin 正是 #393 的謊)。

degraded_pin 不變量(round 2 建立,round 3 未變)

degraded_pin 有值 若且唯若同一個 pin 寫了 marker。其餘所有降級結果(暫時性 API 失敗、下載錯誤、digest 抓取失敗、rename 失敗)刻意留空,因為對那些情況而言 hook 的 kill 就是重試觸發器 —— 它讓 Claude Code 重新 spawn wrapper。round 1 在暫時性路徑也蓋 degraded_pin,等於掐掉唯一的重試觸發器,把「下次 spawn 重試」變成「永不重試」。

hook 端另外重新驗證 marker 本身(存在、pin 相符、TTL 未過、無時鐘偏移),因為 TTL 只有 wrapper 會判讀,而 wrapper 要等 kill 才會再跑 —— 只信 runtime 欄位會讓降級狀態永久化。

Verification

plugin/tests/test-wrapper-download.sh(PATH-shim mock curl)——95 asserts / 28 scenarios(round 1 是 25、round 2 是 76)。

涵蓋含:minified API body、外部 host、dot-segment(兩種形狀)、#fragment、無 hash 工具、present-but-broken shasum、HTML 錯誤頁、marker 注入 payload、未來日期 epoch、SemVer build metadata、temp 洩漏與檔案權限、逾時預算、hook 整合(含 miss / verify 兩種 marker 與負對照)。

Round 3 稽核了 round 2「每個修復都做過 mutation 測試」這句話,發現它是假的 —— 五個修復零覆蓋(stale-marker 清除、HTML 錯誤頁拒裝、openssl fallback、SemVer build metadata、未來 epoch guard),加上 hook 的 verify 分支。全部補上 case。

九個 mutation 全被抓到 —— 但其中兩個新 case 一開始自己也在空轉:dot-segment payload 早被「恰好兩段」的 regex 擋掉、根本沒走到 dot-segment 檢查;stale-marker case 讓下載成功,所以事後清理無論那個分支在不在都會清掉 marker。兩者都已改成真的觸及它們宣稱測的程式碼。

Case 28 是結構性的:grep 出貨腳本裡的 bash-4 專屬語法。shebang 是 /bin/bash,在 macOS 上是 3.2,${x,,} 在那裡 bash -n 解析得過、展開時才炸 —— 這一輪就出貨過這個錯,套件當場 0/76 而症狀看起來像邏輯 bug。

bash -n 通過(wrapper + hook + suite)。

…in runtime state (#392) (#393)

curl --fail everywhere; sha256 verification against the release asset before install (missing asset = disclosed unverified, backward compat); a fallback-tried marker stops the every-spawn re-download loop when a pinned tag is missing upstream; version_at_spawn now re-reads the sidecar so a failed download that keeps an old binary no longer stamps runtime state with the new version (the session-start staleness hook was permanently false-negative in that state). New test-wrapper-download.sh: 25 asserts across 6 mock-curl scenarios.

Refs #392, Refs #393
Nine HIGH findings drove a redesign: PIN_MISS now requires a definitive 404 (or an asset-less 200) - transient 403/5xx/timeouts keep the installed binary and retry next spawn (the reproduced H1 permanent-wrong-pin regression is gone); the fallback marker gains a TTL (24h) and a reason field, and sha-mismatch also marks to stop the 18MB-per-spawn re-download loop; a new degraded_pin runtime field plus a hook-side suppression branch kills the kill-at-every-session-start loop the honest runtime otherwise caused (negative control keeps the kill for genuine staleness); sha256 URLs come from the release's own asset list (absence is definitive, fetch failure is a verification failure, not an unverified install), digests are 64-hex-validated with openssl fallback and a no-hash-tool disclosure; downloads use unique mktemp temps (TOCTOU); sidecar-absent runtime says unknown instead of the pin; legacy no-binary_version plugins keep DESIRED semantics (the #73 trap); runtime JSON values are sanitized; mv is checked before the sidecar is written. Test suite: 25 -> 55 asserts across 12 scenarios including hook-integration with negative control; the mock is now HTTP-code-aware and serves one asset per line (a single-line mock made the greedy sed grab the sha URL - exactly the parsing the real GitHub API shape exercises).

Refs #392, Refs #393
…he download budget

Round-2 fixes from the #392/#393 verify (PR #398). The cross-model round found
18 HIGH-severity defects; the load-bearing ones, each reproduced:

- VERIFIED REGRESSION: unifying the curl calls behind one http_get helper put
  the 18 MB binary on the metadata call's 30-second budget. At 400 KiB/s a
  fresh install can never finish -- exit 1, no MCP server. http_get now takes
  a timeout; the binary gets 300s. The mock ignored --max-time, so the suite
  was structurally blind to this: it now records and asserts both budgets.

- degraded_pin was written on TRANSIENT failures too, and the hook suppressed
  the kill whenever it was set. The kill is the only thing that makes Claude
  Code respawn the wrapper, so "will retry next spawn" became "never retries".
  degraded_pin is now set if and only if a marker was written, and that
  invariant is stated at the top of the wrapper.

- The hook trusted degraded_pin without looking at the marker. The 24h TTL is
  only ever evaluated by the wrapper, which cannot run until a kill respawns
  it -- so at TTL+1h the hook still suppressed, and the pin was never retried.
  Deleting the marker by hand did not help either, because nothing read it.
  The hook now re-derives the decision from the marker (existence, pin, TTL,
  clock skew) and falls through to the normal kill once it lapses.

- sha256 failed OPEN when the hash tool was missing, and sha256_of dispatched
  on `command -v shasum` alone, so a shasum that exists but is broken never
  reached the openssl fallback -- and then reported "no sha256 tool available"
  and installed anyway. Both now refuse: a published digest that cannot be
  checked is a refusal, not a downgrade.

- asset_url was line-based grep+greedy sed, so a minified API body returned
  the LAST url on the line -- the .sha256, or the zipball. It now extracts one
  url per line, matches the basename exactly, and pins host+path to this
  repo's own release downloads.

- The marker's epoch reached bash arithmetic unvalidated, where a crafted
  value is executed as a command, and a non-numeric one aborted the wrapper.
  Validated now, along with future-dated epochs.

- Four mktemp sites had no trap: unique names turned a bounded leak into
  ~18 MB per interrupted spawn. Fixed -- and the first attempt was itself
  broken, because `X=$(new_temp ...)` runs the function in a subshell and
  discards the registration. The temp-leak assert caught it.

- Also: the sidecar no longer falls back to the DESIRED pin when the tag
  cannot be parsed from the URL (that is the #393 lie); a fresh install that
  substitutes `latest` for an unresolvable pin now says so; an HTML error page
  cannot be installed on the no-digest path; chmod is explicit (755) and
  checked; the marker's reason field is read, so a digest mismatch is reported
  as possible tampering rather than "unavailable upstream"; a stale marker for
  a pin we no longer want is cleared.

Suite 55 -> 76 asserts across 19 scenarios. Nine mutations run, nine caught --
including one that first SURVIVED and exposed the corrupt-marker case as
assertion theater: its payload contained whitespace, so `read` split it into
three fields and it never reached the arithmetic it claimed to test.

Refs #392
Refs #393
@kiki830621

Copy link
Copy Markdown
Member Author

6-AI verify — round 2 + fixes (#392, #393)

Backend: pai-ensemble 2.20.0 · 4 lenses + devil's-advocate + Codex gpt-5.6-sol (xhigh) · dispatch model opus · 65 findings (18 HIGH / 27 MEDIUM / 10 LOW / 10 INFO).

Round 1 drove an architectural redesign. Round 2 showed the redesign had introduced a regression of its own and had the retry story backwards.

The load-bearing findings

Finding Verdict Fix
Binary download timeout 300s → 30s Real regression, verified against the pre-patch file (line 66 was --max-time 30 for metadata, line 80 --max-time 300 for the binary). Unifying both behind one http_get collapsed the 18 MB download onto 30s. At 400 KiB/s a fresh install can never finish: exit 1, no MCP server http_get takes a timeout; the binary gets 300s. The mock ignored --max-time, so the suite was structurally blind — it now records and asserts both budgets
degraded_pin written on TRANSIENT failures Real The hook suppresses the kill whenever it is set, and the kill is the only thing that respawns the wrapper — so "will retry next spawn" meant "never retries". degraded_pin is now set iff a marker was written, stated as a named invariant at the top of the wrapper
The hook never checked the marker or its TTL Real, and it made the degraded state permanent The 24h TTL is only ever evaluated by the wrapper, which cannot run until a kill respawns it — so at TTL+1h the hook still suppressed. Deleting the marker by hand did not help either, because nothing read it. The hook now re-derives the decision from the marker (existence, pin, TTL, clock skew)
sha256 fails OPEN when the hash tool is missing Real — the fail-open #392 exists to close Refuses. A published digest that cannot be checked is a refusal, not a downgrade
sha256_of dispatched on command -v shasum alone Real — a shasum that exists but is broken never reached the openssl fallback, then reported "no sha256 tool available" and installed anyway Tries each, validates the output shape, falls through
asset_url is line-based grep + greedy sed Real — a minified body returns the LAST url on the line (the .sha256, or the zipball). Nothing in the API contract promises pretty-printing, and round 1 had reshaped the fixture to fit the parser One url per line, exact basename match, host+path pinned to this repo's own release downloads
Marker epoch reaches bash arithmetic unvalidated Real — a crafted value is executed there, and a non-numeric one aborted the wrapper Validated, plus future-dated epochs
Four mktemp sites, zero traps Real — unique names turned a bounded leak into ~18 MB per interrupted spawn Trap added
Sidecar falls back to DESIRED when the tag cannot be parsed Real — that is the #393 lie Writes unknown
Marker's third field written but never read Real A digest mismatch is now reported as possible tampering, not "unavailable upstream"

Also fixed: an HTML error page can no longer be installed on the no-digest path; chmod is explicit (755) and checked; a fresh install that substitutes latest for an unresolvable pin says so; a stale marker for a pin we no longer want is cleared.

Evidence

bash plugin/tests/test-wrapper-download.sh   → 76 pass / 0 fail  (19 scenarios; was 55/12)
bash -n  (wrapper + hook + suite)            → clean

Nine mutations run, nine caught — each fix above had its defect deliberately re-introduced, one at a time, and every one turned the suite red. That exercise also caught one of my own assertions testing nothing: the corrupt-marker payload contained whitespace, so read split it across three fields and it never reached the arithmetic it claimed to exercise. Fixed with a whitespace-free ${IFS} payload, then re-run.

Two of my own bugs were caught the same way: X=$(new_temp …) ran the registration in a subshell and discarded it (the trap had nothing to clean), and the Case 17 shim omitted cp, which the mock curl itself uses — so the wrapper failed for the wrong reason while the case's negative asserts still passed.

Honest status

Not independently re-verified. MAX_ROUND=2 was reached. What stands behind these fixes is reproduction + the mutation battery above, not another adversarial round. Given this round found 18 HIGH on a diff that had already been through one full round, a third round on the current head is worth running before merge.

…es that had no tests

Round-3 fixes from the #392/#393 verify. That round completed only 2 of 6
reviewers (quota), and both of the two found something load-bearing.

THE HOST PIN WAS NOT A PIN. Round 2 validated the download URL as a STRING
(starts with https://github.com/<repo>/releases/download/) and then handed it
to curl, which normalises the path before requesting. Verified locally with
curl -w '%{url_effective}':

    .../che-apple-mail-mcp/releases/download/../../../../attacker-org/evil-repo/releases/download/v1/CheAppleMailMCP
      prefix check passes, basename is CheAppleMailMCP
      curl actually GETs github.com/attacker-org/evil-repo/.../CheAppleMailMCP

and since the .sha256 is selected by the same rule it comes from the same
attacker path -- verification PASSES and the wrapper prints "(sha256 verified)"
while exec'ing someone else's binary. A #fragment is dropped before the request
too, so the basename check can be satisfied by text the server never sees.

The URL is now validated structurally: exactly two path components after
/download/, no query, no fragment, no dot-segment in any spelling.

FIVE ROUND-2 FIXES HAD NO TESTS -- and the CHANGELOG claimed all of them were
mutation-tested. The reviewer re-ran the suite and mutated each: all five
survived at 76/0. Cases added for every one:

  - stale-marker-for-a-different-pin clearing
  - the HTML-error-page refusal on the no-digest path. This is not merely a
    round-2 claim: it is an explicit acceptance item of #392's own diagnosis
    ("--fail 擋錯誤頁") that never had a test. Case 5 exercises the HTTP-status
    gate, never the body gate.
  - the openssl fallback for a shasum that EXISTS but fails (Case 17 removes
    both tools, which is a different scenario)
  - SemVer build metadata surviving json_escape (+build was silently dropped)
  - the future-dated-epoch clock-skew guard
  - plus the hook's `verify` branch, which every case avoided by writing `miss`

The CHANGELOG sentence asserting otherwise is corrected rather than deleted --
it is the same failure the round-2 process itself named (a case that "passed
while testing nothing"), and the correction is the useful part.

Suite 76 -> 95 asserts across 28 scenarios. Nine mutations run, nine caught --
but only after TWO of the new cases were themselves caught testing nothing:
the dot-segment payload was rejected by the component-count regex before ever
reaching the dot-segment check, and the stale-marker case let the download
succeed, so the post-install cleanup removed the marker whether or not the
branch under test existed. Both rewritten.

Case 28 is new and structural: it greps the shipped scripts for bash-4-only
syntax. The shebang is /bin/bash, which is 3.2 on macOS, where ${x,,} parses
fine under `bash -n` and fails at expansion time. This round shipped exactly
that and the suite went to 0/76 with a failure that read like a logic bug.

Refs #392
Refs #393
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant