Skip to content

fix: deny HTTP/1 Upgrade in immutable Pingora peers - #33

Draft
seonghobae wants to merge 17 commits into
fix/payload-safe-pingora-logging-v1from
fix/deny-upgrades-at-peer-v1
Draft

fix: deny HTTP/1 Upgrade in immutable Pingora peers#33
seonghobae wants to merge 17 commits into
fix/payload-safe-pingora-logging-v1from
fix/deny-upgrades-at-peer-v1

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Fixes #32.

Finding

The existing HTTP/1 protocol-transition admission guard returns 501 before application admission or origin contact, but the immutable Pingora peer below that guard inherited HttpUpstreamRequestPolicy::standard(). At pinned supplier revision 09696b51bc59315353d96686355861604d0bb48c, that standard policy retains WebSocket upgrade capability. A later callback/composition refactor could therefore widen the protocol surface without changing the transport-neutral CWL contract.

Fresh supplier evidence on 2026-09-09 still shows protected cloudflare/pingora/main at 09696b51bc59315353d96686355861604d0bb48c, issue cloudflare/pingora#946 open, and proposed fix cloudflare/pingora#947 open/unmerged. Those are supplier-owner evidence only; this PR does not consume a mutable PR head.

RED → causal fix

Historical RED b0719a893c0136083efc12918c6e906a28f39319 requires public build_peer() to produce HttpUpstreamRequestPolicy::deny_upgrades(). Historical source fix 0109031b97ce1816a7936987a4d63330a2d19cba demonstrated the minimal semantic change.

Current source keeps that exact repair on final #31: pingora_delivery::build_peer_from_validated uses deny_upgrades() and documents the invariant. tests/peer_protocol_policy.rs independently locks the immutable peer option, with constraint-oriented rustdoc so the test/documentation coverage gate is not weakened. Standard hop-by-hop and Connection-nomination sanitization remain; only the supplier-default HTTP/1 upgrade capability is denied.

No WebSocket implementation, product auth/business routing, retry/idempotency policy, Wardnet/EgressWeave verdict, or Keyverse identity authority is added.

Ordinary/non-force succession

Historical #33 head d6eaa9f9249a77551e24014d72adc3940e6a4404 was based on an obsolete #31 snapshot. Final #31 is 8230f8d1311adccc133b7f6ed86df441533e486c, with terminal CI 34251472731, Supply Chain 34251472751, and an exact-head owner technical COMMENT reporting no actionable finding.

Ordinary two-parent commit b98646582698ea99f94f62910c217d6ee4bbcdb9 preserves historical #33 as first parent and final #31 as second parent. Its resolution tree is final #31 plus only the valid peer-policy source/test/ADR delta; no force-push, destructive rebase, stale-parent source, or predecessor receipt was replayed. Subsequent documentation-only commits project that same decision into ARCHITECTURE, SECURITY, TEST_STRATEGY, TRD, CHANGELOG and docs/product-technical-gap-baseline.md without changing runtime semantics.

Final exact head is df2a9bef4e99678259d1666c0262ae1cae9c4ff1. Fresh compare against final #31 has merge base exactly 8230f8d... and zero behind debt. Effective scope is the peer-policy source/test/Proposed ADR plus code-current documentation.

Exact-head closure

The unchanged final head has terminal evidence:

  • CI 34254199047: terminal success on exact df2a9bef...; formatting, compile/tests, strict Clippy, warnings-denied public rustdoc, 100% owned-production coverage, lock verification, generic/routed Rust-origin load, and dual-profile OCI runtime all closed.
  • Supply Chain 34254199064: terminal success on exact df2a9bef... after dependency audit, candidate builds, SBOM/image scans and exact-source evidence binding.
  • Exact-head artifacts include coverage, Cargo.lock, generic k6 and routed pg-erd k6 receipts bound to df2a9bef....
  • Exact-head owner technical COMMENT review 5144705431 found no actionable source, test, architecture or documentation finding. It is technical evidence only, not an independent human APPROVED governance decision.

No final #31 receipt was transferred; #33 independently re-executed on its own unchanged final head. ADR 0009 remains Proposed until protected governance/integration rather than creating a status-only self-staling descendant. HTTP/1 Upgrade is intentionally unsupported; WebSocket/Extended CONNECT/H2/H3/QUIC parity remains separate versioned work.

#37 may now ordinarily/non-force succeed from this exact tree, preserving its distinct pg-erd upstream TLS delta and independently reacquiring hosted/review evidence.

Keep Draft. No self-approval, force-push/destructive rebase, protected merge, immutable release, canary, cutover, rollback or legacy-removal credit is claimed.

@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Owner technical review on exact head df2a9bef4e99678259d1666c0262ae1cae9c4ff1.

Reviewed the full final-#31 → current-#33 range after ordinary/non-force succession. Merge base is exactly final #31 8230f8d1311adccc133b7f6ed86df441533e486c with zero behind debt. The runtime delta is bounded to pingora_delivery: immutable peers switch from HttpUpstreamRequestPolicy::standard() to deny_upgrades() while keeping existing TLS/ALPN/timeouts/trust handling. Pinned supplier source confirms default() uses H1UpgradePolicy::WebSocketOnly with standard stripping fields and deny_upgrades() changes only h1_upgrade to Deny via ..Self::default().

tests/peer_protocol_policy.rs exercises public build_peer() and locks exact peer policy; helper/test rustdoc describes the defense-in-depth constraint rather than restating code. Existing real-listener #29 acceptance remains the wire-visible 501/no-origin-contact evidence, so this peer-level contract does not manufacture WebSocket parity. ADR 0009 remains Proposed and the current ARCHITECTURE/SECURITY/TEST_STRATEGY/TRD/CHANGELOG/gap-baseline projection is consistent with the source boundary. Product auth/business routing, Wardnet/EgressWeave policy and Keyverse identity are not introduced.

No actionable source, test, architecture or documentation finding remains in this exact-range technical review. This COMMENT is technical evidence only, not an independent human APPROVED governance decision. Exact df2a9bef... CI/Supply Chain are still pending and predecessor #31 receipts do not transfer.

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

Labels

bug Something isn't working priority: high type: bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant