fix(apps): harden backend health probes

[SebastianYuSun]

Problem / Motivation

#5929 already merged the backend liveness, generation-fencing, locking, retry, and tri-state reconciliation work that this older PR originally carried. One security residual remains absent from main: the app-authored backend.healthCheck is concatenated into a URL without validation, and the probe uses the ordinary opener.

Why it matters

A value such as @example.com/ can move the assembled URL authority away from loopback. The default opener also inherits proxy environment variables and follows redirects. A health check intended only for 127.0.0.1 therefore lacks a firm loopback-only boundary.

What changed

• Validate backend.healthCheck before building a URL, requiring a valid port and an absolute restricted path that cannot move the authority or be silently normalized.
• Route adoption, startup polling, and the standing watch through the same validated _health_probe contract.
• Use loopback_urlopen, which bypasses proxy environment variables and refuses redirects.
• Fail closed for invalid paths and warn once per distinct value.
• Document the absolute-path and loopback-only contract in the manifest reference and App Kit system specification.

Scope

This PR does not replay the supervisor or reconciliation state machine from the old branch. The generation, lock, retry, ownership, and tri-state behavior merged in #5929 remains unchanged.

Tests

• test/test_app_backend.py + test/test_apps_backend_coverage.py: 272 passed, 9 skipped
• focused health security, adoption/startup/watch, malformed-response, and real proxy/redirect coverage: 50 passed
• isort, flake8, scoped mypy, docs-lint, and git diff --check: passed

Related

The liveness portion of the original change is covered by #5929. This PR is limited to the remaining path-validation and no-proxy/no-redirect security boundary.

Contribution License Agreement

I agree to the terms of the Contribution License Agreement for this repository.

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed c94f71e2a348285135c81ba346d99cca77b8e644 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] c94f71e

[github-actions]

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of c94f71e2a348285135c81ba346d99cca77b8e644 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All checks done. The change reuses the existing loopback_urlopen (12+ call sites; its docstring names bare loopback urlopen as the anti-pattern), covers all three healthCheck consumers in one shared probe, and validates at the URL-construction site — the point-of-use boundary, which past load-time validation gaps (manager.py:1769's register_external hole) show is the robust level. Final review:

First-Principles-Verdict: PASS

A real authority-smuggling hole in app-authored health paths is closed at the one place URLs are built, reusing the existing hardened opener.

What this change ships

Intent: stop an app manifest's healthCheck text from steering the loopback health probe off 127.0.0.1. This is a FIX.

1. A path like @example.com/ is refused; the backend reads unhealthy — justified (external-content boundary)
2. Probes ignore proxy env vars and refuse redirects — justified; reuses existing loopback_http.loopback_urlopen (12+ call sites)
3. Adoption, startup, and watch share one validated probe — justified; all 3 manifest.backend.healthCheck consumers covered, no unfixed sibling
4. A rejected path warns once per distinct value — justified (standing watch would repeat it every sweep)
5. Backends answering health via a 3xx now count unhealthy — declared, derived from the redirect boundary
6. Impossible port numbers refused before URL build — undeclared, trivial (an int can't smuggle authority; zero option is an identical False)
7. Manifest reference and app-kit spec updated — mandated (spec-in-same-commit rule)

Depth check: the cause is app text concatenated into a URL, and the fix sits exactly there — _health_probe_url guards the only construction site, which is stronger than the alternative home in AppManifest.validate() (manifest.py:1207, enforced at manager.py:331 / discovery.py:144), since load-time validation has had documented bypass gaps (manager.py:1769). Grepped backend.routes (module path, not URL text) and _PROXY_SEGMENT_RE (handlers_instances.py:1047 — different job, per-segment on incoming requests): no duplicate mechanism, no sibling left unfixed. Description matches the diff, including the #5929 scope claim — the base already contains the liveness/generation work.

[FIRST-PRINCIPLES-REVIEWED] c94f71e

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of c94f71e2a348285135c81ba346d99cca77b8e644 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims in the description have backing code, the validation reuses the existing hardened loopback_http opener rather than inventing a second one, probe-time enforcement is the right layer (manifest-load validation alone would be editable-on-disk bypassable, and fail-closed covers every caller), and existing well-formed manifests (/health) are unaffected — no compat break, no scope creep.

Design-Verdict: PASS

Real authority-smuggling/proxy harm, closed at the one probe seam by reusing the existing hardened loopback opener — proportionate and fail-closed.

[DESIGN-REVIEWED] c94f71e

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed c94f71e2a348285135c81ba346d99cca77b8e644 via the fork AI-review pipeline; updated in place on each push.

Review details

Both candidates are self-flagged low-confidence, and both describe deliberate fail-closed tightenings in a security-hardening PR, documented in the same diff's spec/manifest changes.

Candidate 1 (redirect refusal): loopback_urlopen rejecting 3xx is the explicit purpose of the change — the spec hunk states "rejects redirects, so a probe cannot be redirected or proxied away from 127.0.0.1." There is no concrete input in practice: no evidence any shipped app relies on a redirecting health endpoint. The "wrong outcome" is an intended security boundary (EXTERNAL/authority-shift boundary), not a defect. Fails (a) and (c). Drop.

Candidate 2 (empty healthCheck): The field defaults to /health; only an explicit "healthCheck": "" is affected. The manifest reference is updated in this same diff to require "Absolute health-check path beginning with /; unsafe or ambiguous paths are refused." An empty string is not such a path, so its refusal is now documented, intended behavior — a fail-closed choice, not an observable wrong outcome. No evidence any real manifest sets it empty. Fails (a) and (c). Drop.

Step 2: The validation logic is sound — the leading / terminates the URL authority, the char class excludes @ \ [ ] # whitespace and control chars (verified against the tests' authority-smuggling cases), timeouts are preserved per call site (adoption keeps timeout=3, poll/watch keep the _HEALTH_CHECK_TIMEOUT default), and all three probe paths route through the same helper. No grounded 80+ finding to add.

No findings.

[OPUS-REVIEWED] c94f71e

[bolichen97]

Full-diff overlap audit (current head 6b102378cc8be16942b8616e3c7a474e8747d0fc): merged #5929 (ea4116f6b6323d2ee3e6a8e4d05e91b907f25172) now covers this PR's post-start liveness state machine: consecutive HTTP-failure/process-exit demotion, generation fencing, adopted-owner handoff, reversible recovery, and serialized/retried MCP reconciliation.

There is still a real residual in this diff: _health_probe_url validation and the no-proxy/no-redirect loopback_urlopen probe. #5929 still builds the URL and calls the ordinary opener, so that security boundary is not covered.

Please rebase onto current main and reduce this PR to only that safe-probe implementation and its focused tests, removing the already-merged supervisor/reconciliation code. #6079 is fully covered by #5929; #6092 should separately retain only its unrelated ChatPage lastIndexOf hunk.

[iamwhatever]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[bolichen97]

Approving per triage sweep: readiness: passed, required check PR Readiness green, mergeable, no valid change requests or unresolved threads.