Skip to content

fix(network): restore bounded BiDi opening-response validation - #242

Draft
seonghobae wants to merge 15 commits into
feat/webdriver-bidi-websocket-opening-writefrom
repair/webdriver-bidi-opening-response-restack
Draft

fix(network): restore bounded BiDi opening-response validation#242
seonghobae wants to merge 15 commits into
feat/webdriver-bidi-websocket-opening-writefrom
repair/webdriver-bidi-opening-response-restack

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Current boundary and dependency

Draft opening-response repair at exact head 2d0e9f69df9ade21d8e8e3d807c3ff644d83b310, still recorded against the pre-recovery #195 base generation 48eb2d23009c1c804520dd5efcd0d4d072aacef1. The live #195 branch has since advanced to repaired exact 63997bcf555e2c5c8e91ba287734ffba3837a1b7, but that foundation currently has an exercised CI RED owned by #279, so this dependent branch is not restacked yet.

The bounded RFC 6455 response parser validates HTTP/1.1 status 101, Upgrade/Connection tokens, the single client-key-correlated accept value, header limits and monotonic deadlines on the original peer-verified stream. Unknown extension fields may contain RFC 9110 obs-text; required handshake fields remain strict. No DNS, reconnect, proxy, TLS substitution, browser action or authority grant is added.

Current fixture repair

A complete Rust run while adopting the parent into #243 reproduced macOS EINVAL during opening-write cleanup after 198 bytes in the mismatched-accept fixture. The invalid-deadline and revoked-stream fixtures had already been repaired, but three sibling exchange fixtures still closed early. The request-only server dropped its stream after reading; the successful-response server's one-byte close probe could read the request instead of closure; the rejected-response server returned immediately after writing.

The test-only shared exchange helper reads the complete bounded request before replying and retains the accepted stream until the client releases it after its assertions. All three fixtures reuse it. No timing sleep, production retry, weakened cleanup error, new dependency or coverage exclusion is introduced. Source-grounded RCA is in docs/doctoring.md and CHANGELOG.md records the change.

Exact-tree verification

For exact head 2d0e9f69df9ade21d8e8e3d807c3ff644d83b310, native CI 33952463254 is now terminal success. The earlier local evidence also remains predecessor-local evidence for this same tree: Rust 1.97.1 formatting, workspace/all-target/all-feature check, complete workspace tests, strict Clippy, warnings-denied rustdoc, 139 repository contracts, and 50 complete opening-exchange fixture-suite runs covering 250 cases. The CI-equivalent historical nightly-2026-08-01 coverage measurement was 809/809 functions, 7802/7802 lines, 9959/9959 regions and 942/942 branches.

That hosted success validates this exact historical tree only. It does not validate adoption of the repaired #195 foundation, current protected workflow generation, central security/review gates, or browser acceptance.

Historical regression lineage

Status-line RED cb24998c62fda3b2f885ee4f1425569dd4a17598 failed semantically in CI 33243277333. Opaque ignored-extension RED 3c52466d1cd3978e1bb0a8f48a82c0c0e846fd43 failed in CI 33246054621. Behavioral repair 70cab65592c21d7d1008b467c7810eeae382e427 passed CI 33246188780. Prior documentation/fixture head 55fef0c3fae1724eddada53e52c4a0311f509aa3 and CI 33246431722 remain predecessor evidence only.

Authority and integration

Keep Draft until the prerequisite and current repository policy are satisfied. The dependency order is now explicit: #279's authorized workflow reconstruction must produce exact-head evidence on repaired #195 63997bcf...; only then should this branch adopt that verified foundation content-aware/non-destructively and regenerate its own exact-head checks. Later descendants must follow the same order rather than preserving an old child tree that could reintroduce the stale-tree deletion.

Do not transfer predecessor checks, synthesize approval, force-push, bypass protection, alter workflows/rulesets/secrets, tag, release or publish.

@coderabbitai

coderabbitai Bot commented Aug 29, 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.

Bring PR #242 onto PR #195 exact head b852245 without rewriting either branch.

Commit-Message-Assisted-by: Claude (via Claude Code)
Signed-off-by: Seongho Bae <me@seonghobae.me>
Keep the accepted loopback socket alive until the invalid response deadline is rejected, eliminating the macOS cleanup race without changing production behavior.

Commit-Message-Assisted-by: Claude (via Claude Code)
Signed-off-by: Seongho Bae <me@seonghobae.me>
@seonghobae

Copy link
Copy Markdown
Contributor Author

Exact head 17754d71dd57f88163f69f58cc57282265a85b6e now adopts #195 b852245 and fixes the sibling macOS loopback race in the invalid response-deadline integration path. The peer stays alive through opening-write cleanup; production behavior is unchanged. The focused regression passed 50/50, followed by 139/139 Python contracts, Rust fmt/check/test/all-target Clippy/rustdoc, and exact 100% production function/line/region/branch coverage. Fresh hosted exact-head checks remain required.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Correction: the exact current #242 head is 17754d7. The prior comment transposed the suffix; no check or review evidence is transferred from that typo.

Bring PR #242 onto PR #195 exact head 48eb2d2 without rewriting either branch.

Commit-Message-Assisted-by: Claude (via Claude Code)
Signed-off-by: Seongho Bae <me@seonghobae.me>
@seonghobae

Copy link
Copy Markdown
Contributor Author

Exact head 55fef0c now adopts #195 exact 48eb2d2. The merge conflict was limited to retaining both independent CHANGELOG bullets. Exact-head local validation passed 139/139 Python contracts, full Rust gates, and exact 100% function/line/region/branch coverage; fresh hosted checks remain required.

seonghobae added a commit that referenced this pull request Sep 5, 2026
Bring PR #93 onto PR #242 exact head 17754d7 so the shared response-deadline fixture no longer races socket cleanup on macOS.

Commit-Message-Assisted-by: Claude (via Claude Code)
Signed-off-by: Seongho Bae <me@seonghobae.me>
seonghobae added a commit that referenced this pull request Sep 5, 2026
Bring PR #93 onto PR #242 exact head 55fef0c so both macOS socket-lifetime regressions are fixed without rewriting history.

Commit-Message-Assisted-by: Claude (via Claude Code)
Signed-off-by: Seongho Bae <me@seonghobae.me>
Read each request before replying and retain the peer through client assertions. Reuse one test-only exchange helper for request-only, valid response, and rejected response paths; keep production cleanup failures unchanged.

Commit-Message-Assisted-by: Codex (via Codex)
Signed-off-by: Seongho Bae <me@seonghobae.me>

@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.

Exact-head RFC 6455 opening-negotiation finding for 2d0e9f69df9ade21d8e8e3d807c3ff644d83b310. This is the canonical opening-response owner and the same parser is inherited by current #255 07ef43ec71b6dbd8540629bf1df5a63b81541ee4.

The client opening request offers neither Sec-WebSocket-Extensions nor Sec-WebSocket-Protocol. parse_opening_response(...), however, tracks only Upgrade, Connection, and Sec-WebSocket-Accept; every other syntactically admitted header is ignored. A peer can therefore return a valid 101/Accept plus Sec-WebSocket-Extensions: permessage-deflate or Sec-WebSocket-Protocol: unexpected, and OriginWeave will construct WebDriverBiDiWebSocketEstablished. That is protocol-state confusion: the later frame boundary explicitly operates with no negotiated extensions and rejects reserved bits, while the opening boundary has already accepted a server claim that an extension/subprotocol is in use.

RFC 6455 §4.1 items 5–6 require the client to fail the WebSocket connection when the server response selects an extension or subprotocol that was not present in the client handshake. Because this client currently offers neither, any server selection is unoffered and must fail closed before an established-state value exists.

Add realistic loopback REDs at this owner: after reading the exact ordinary OriginWeave opening request, return otherwise-valid 101 Switching Protocols responses with a correct Sec-WebSocket-Accept plus (a) Sec-WebSocket-Extensions: permessage-deflate and (b) Sec-WebSocket-Protocol: unexpected. Current code should accept them; repaired code must return a typed opening-response failure and never yield WebDriverBiDiWebSocketEstablished. Preserve unknown ordinary HTTP extension headers such as X-* as opaque where allowed; the defect is specifically the two WebSocket negotiation headers.

Minimal causal repair: explicitly parse these two response headers against the set actually offered by WebDriverBiDiWebSocketHandshakePlan. With the present request shape the offered sets are empty, so non-empty server selections fail closed. Do not add permessage-deflate, generic extension authority, or a subprotocol merely to make the response acceptable. If such support is later required, make the offer/selection a versioned typed negotiation contract and carry the negotiated state into framing. Keep this repair at #242 and restack descendants non-destructively after its own exact RED→GREEN; do not widen #255's currently active operation-wide teardown-deadline source slice.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant