[WIP] Fix squid aborts on client disconnection during CONNECT - #9419
Conversation
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
|
@copilot finish implementing this pr |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The downstream Squid guard lacks regression coverage that exercises it independently of the configuration workaround.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds layered protection against Squid crashes when CONNECT clients disconnect before sending TLS ClientHello data.
Changes:
- Disables unsafe half-closed client handling.
- Patches Squid’s empty-buffer assertion path.
- Adds configuration and integration regression coverage.
| File | Description |
|---|---|
tests/integration/network-security.test.ts |
Tests CONNECT EOF handling and Squid stability. |
src/squid/config-generator.ts |
Disables half-closed clients. |
src/squid-config-streaming.test.ts |
Verifies generated configuration. |
containers/squid/Dockerfile |
Builds and installs patched Squid. |
containers/squid/clienthello-eof.patch |
Guards empty ClientHello buffers. |
.github/workflows/test-squid-image.yml |
Extends image-build timeout. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| + if (inBuf.isEmpty()) { | ||
| + debugs(83, 3, "client EOF before TLS ClientHello: " << clientConnection); | ||
| + clientConnection->close(); | ||
| + return; |
|
@copilot address review feedback |
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Added independent image-matrix coverage with SSL bump active and |
|
✅ Copilot review passed with no inline comments. @copilot Add the |
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (8 files)
✨ New Files (2 files)
Coverage comparison generated by |
|
@copilot fix the pr title and summary and fix this failing ci check https://github.com/github/gh-aw-firewall/actions/runs/37215276207/job/111474869658?pr=9419 |
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
The CI test fix is committed as |
|
🔌 Smoke Services — All services reachable! ✅
|
|
❌ Smoke Copilot BYOK AOAI (api-key) reports failed. AOAI BYOK (api-key) mode investigation needed...
|
|
❌ Smoke Gemini reports failed. Facets need polishing...
|
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤
|
|
Smoke Cloud Hypervisor completed. Cloud Hypervisor + Copilot passed. smoke test checks could not run: curl, python3, node, and the github/safeoutputs CLI bridges are all blocked by Permission denied in this sandbox
|
|
🛡️ Smoke Copilot Network Isolation confirmed the egress allowlist is enforced. ✅
|
|
✅ Smoke Claude passed
|
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟
|
|
✅ Build Test Suite completed successfully!
|
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅
|
|
Smoke Copilot: PASS
|
Smoke Claude Results
Overall result: PASS
|
|
Services smoke test:
Overall: PASS
|
Smoke Test: Copilot BYOK (Direct) Mode ✅Test Results:
Status: PASS — All smoke tests passed. Direct BYOK mode operational.
|
|
EGRESS_RESULT allow=pass deny=pass
|
Smoke Test: Cloud Hypervisor + Copilot
Most checks could not run because curl, python3, node, jq, and the github CLI bridge were intermittently blocked by the sandbox exec policy itself ("Permission denied") before any firewall logic was reached. Only the local file write/read check passed. Label not added since not all checks passed.
|
|
OTEL smoke test
|
Chroot Version Comparison
Result: Not all tests passed (Node.js version mismatch), so the
|
🏗️ Build Test Suite Results
Overall: 6/8 ecosystems passed — FAIL Failures
All 8 repositories cloned successfully. The
|
Smoke Test
PR titles unavailable because the runner shell lacked required tools (
|

!inBuf.isEmpty(), client_side.cc:2829) when a client closes after CONNECT and before its TLS ClientHello; root cause of #9088 #9404