feat(network): validate WebDriver BiDi JSON envelopes - #247
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
seonghobae
left a comment
There was a problem hiding this comment.
Fresh protocol review found a common-envelope contract gap that belongs here, not in command-specific #250. The current WebDriver BiDi Editor's Draft (3 Sep 2026) defines ErrorResponse.error as ErrorCode, and §3.5 defines ErrorCode as the finite protocol vocabulary (including invalid argument, session not created, unable to set cookie, unknown error, etc.). Current into_error() only applies required_text(self.error, "error"), so a syntactically valid response such as {"type":"error","id":7,"error":"attacker-defined-code","message":"m"} is admitted and error_code() exposes that arbitrary string as if it were a protocol error code. That becomes material in #250, which retains it in RemoteProtocolError.
Repair this at the canonical common-envelope owner with a realistic transport RED that proves a non-ErrorCode string is rejected while all reviewed protocol codes remain admitted, then a minimal fail-closed validator/typed value here. Do not patch the same vocabulary separately in #250. Because #247 is an ancestor of the current #248→#249 stack, any causal change here requires non-force parent-first restacking and fresh exact-head verification downstream; the existing e6d516... GREEN must not be transferred to the moved head. This is a repair finding, not approval or a request to close the PR.
seonghobae
left a comment
There was a problem hiding this comment.
Current-head repair verification for the common WebDriver BiDi envelope owner. The live W3C CDDL defines ErrorResponse.error as ErrorCode, with a finite 30-value protocol vocabulary. Commit 87854fdc3ef84a7c0fc4d6a8baf61fa1532c1c8c implements that boundary once here: it preserves the existing id/error/message/optional-stacktrace validation order, rejects any text outside the reviewed ErrorCode set before envelope construction, and adds a focused all-values acceptance test while the existing realistic TCP → RFC 6455 → message assembler → public parse test covers hostile attacker-defined-code rejection. Compare against test-only predecessor 1bdfb42... is exactly one production/test source file, +62/-0, 0 behind. Exact-head CI 33866556125 has materialized but both Production coverage 101002630503 and Rust contracts 101002630680 are still queued with no steps/runner evidence, so this is not an approval and not a GREEN/merge-ready claim. Downstream #248/#249/#250 remain parent-first blocked until this exact head receives terminal verification.
seonghobae
left a comment
There was a problem hiding this comment.
Current-head verification after source + standards documentation repair. Production hardening remains 87854fdc...: the common local-end envelope accepts only the reviewed 30-value W3C ErrorCode CDDL vocabulary and rejects hostile unknown strings before typed envelope construction. Follow-up 53acd599... corrected the branch doctoring record: the latest published Working Draft reviewed is 29 June 2026; the live Editor’s Draft retrieved 4 September identifies itself as 5 August 2026; importantly, no such client window is not a member of the local-end ErrorCode CDDL and is no longer claimed/admitted merely because the spec defines that error concept elsewhere. 7560fa106... recorded the security change in CHANGELOG, and 7f31e50c... immediately restored an unrelated redirect-cycle wording that the whole-file changelog write had disturbed. Compare 87854fdc...→7f31e50c... is now only the intended doctoring (+7/-7) and one CHANGELOG security bullet (+1/-0). Exact CI 33867227988 is non-terminal: Rust contracts 101004734350 and Production coverage 101004734558 are queued with no steps/runner. This is not approval or GREEN; #248/#249/#250 remain parent-first blocked.
seonghobae
left a comment
There was a problem hiding this comment.
Current-head test-first finding on 89140129de304e31f241ae3e77b8b82fe01a6bfa: the W3C WebDriver BiDi event definition requires method to be an event name of the form [module name].[event name], while the common-envelope parser currently accepts any JSON string once params is an object. This admits empty, bare, missing-module, and missing-event method values as typed event metadata. The new realistic regression reaches the public parser through loopback TCP → RFC 6455 opening exchange → validated server frame → bounded text-message assembly and requires these four impossible event-name shapes to fail closed as InvalidMember { member: "method" }. Full event membership is intentionally not duplicated here; extension/core event registries remain later event-specific authority. The minimal common-envelope repair, after repository-native RED evidence, is only structural event-name validation with non-empty module and event components. No hosted RED is claimed yet: immediately after the push, no pull-request workflow run had materialized for this exact SHA. Descendant #248/#249/#250 remain blocked on this moved ancestor.
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head RED is now repository-native, not inferred. CI 33871105669 ran on 89140129...: Rust contracts 101017039183 passed repository contracts then failed canonical rustfmt; artifact 9939974652 contains the exact one-hunk format repair. Production coverage 101017038959 reached the coverage test execution and failed while this head is test-only and into_event() still accepts any text method with object params. W3C §3.3/§3.6 requires event names to begin with a non-empty module name followed by . and an event-name remainder. The owner-local causal repair is structural only: reject methods with no separator or an empty component, retain extension/module membership for later event-specific authority, apply the exact rustfmt artifact delta, then rerun exact-head CI. Current primary-source dates also drifted: live Editor’s Draft 3 Sep 2026; latest published Working Draft 18 Aug 2026. Keep Draft and do not propagate predecessor GREEN downstream.
seonghobae
left a comment
There was a problem hiding this comment.
Current-head verification: c6ab0c597b625c5dcb36f63ed9e82c53176d0492 is a normal fast-forward repair after an incidental full-file write changed unrelated raw-string JSON fixtures. Fresh compare from executed-RED head 89140129de304e31f241ae3e77b8b82fe01a6bfa to this head reports files=[], 2 commits ahead, 0 behind, so the current tree is exactly the executed RED tree. The product finding remains valid: common event-envelope admission must reject methods lacking a non-empty module, ., and non-empty event-name remainder, after params object validation so error precedence is preserved. No production fix or GREEN is claimed on this head; exact CI 33884813156 is currently queued.
Signed-off-by: Seongho Bae <me@seonghobae.me>
|
Causal repair is now pushed at exact head
This remains Draft until exact-head CI/review is terminal. Descendants #248–#250 remain blocked until this head is verified, then must be reconstructed parent-first without force push. |
|
Correction to the immediately preceding update: the exact current head is |
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head review for 9b704036bfbbace93fd2cea6ffadec88d3908d42: fresh W3C primary evidence confirms both the latest published WebDriver BiDi Working Draft and current Editor’s Draft are dated 3 September 2026. The new commit changes only docs/doctoring/browser-agent-protocols.md, updating the review date and APA references without altering protocol/runtime code. Canonical docs/doctoring.md still carries the older 1 June 2026 snapshot and remains an explicit follow-up repair. CI 33898746276 is queued, so this head is not promoted to GREEN and descendants remain parent-first blocked.
seonghobae
left a comment
There was a problem hiding this comment.
Current-head finding: the 2026-09-03 W3C WebDriver BiDi document is internally inconsistent at the ErrorCode boundary. §3.5 explicitly defines no such client window, and normative browser client-window algorithms return that error, but the rendered ErrorCode CDDL block omits it. The current OriginWeave allow-list follows the rendered CDDL and therefore rejects a protocol error that the same specification can require a conforming remote end to emit. Treat this as an interoperability RED, not as permission to admit arbitrary strings: add a realistic loopback RFC 6455 regression for no such client window, observe the exact failure, then minimally admit that reviewed spec-defined code while keeping unknown values fail-closed. Doctoring should record the upstream prose/CDDL discrepancy rather than claiming the CDDL is complete.
seonghobae
left a comment
There was a problem hiding this comment.
Current-head verification note: the interoperability RED remains test-only on exact 75780785d6a6a9cc3659103e0653880ae6222fd4. I removed the initially duplicated loopback integration harness and colocated the no such client window case in the existing realistic webdriver_bidi_json_envelope.rs transport suite, so the net semantic delta is one regression and production remains unchanged. Exact CI 33907245004 is non-terminal; do not claim executed RED or compatibility repair yet. If it fails at the intended assertion, admit only the reviewed spec-defined code, retain hostile unknown-code fail-closed behavior, then update doctoring/CHANGELOG and obtain exact-head GREEN before reconstructing #248/#249/#250.
Signed-off-by: Seongho Bae <me@seonghobae.me>
|
Exact-head RCA: run 33907245004 failed because the newly added current-spec |
Signed-off-by: Seongho Bae <me@seonghobae.me>
seonghobae
left a comment
There was a problem hiding this comment.
Current-head review for 6407895f4db4bee640074cb9c9d3cbe8b0e9e13a: hosted RED is now real and attributable. CI 33907245004 on test-only 75780785... failed cargo test at the realistic loopback real_transport_accepts_spec_defined_client_window_error, with production returning InvalidMember(error) where the test required protocol Error; the other integration cases passed. Production commit 01ef4296... makes the minimal compatibility repair by admitting only no such client window while retaining unknown-code rejection. Fresh W3C primary evidence shows the rendered CDDL still has 30 values and omits this code even though §3.5 and normative client-window algorithms define/return it, so later documentation claiming a 31-value CDDL was corrected in 7a459431.../9fa4f37f...; 6407895f... adds a focused docs contract for the 30+1 exception. Exact CI 33928760537 is materialized but non-terminal. Do not merge or restack descendants until this exact generation and required review/security gates are GREEN.
Boundary
Dependency-ordered child of #246. This PR owns the common local-end WebDriver BiDi JSON-envelope boundary after validated RFC 6455 text-message assembly: complete JSON syntax validation plus fail-closed classification of success/error/event envelopes. It grants no command dispatch, browser, policy, secret, or Agent authority.
Live base is
feat/webdriver-bidi-text-message-assembly@c3165309bb94384636990c8d23a36261adb64bf3. Exact current head is6407895f4db4bee640074cb9c9d3cbe8b0e9e13a, open, Ready, and mergeable.Executed interoperability RED and minimal repair
The realistic regression on exact
75780785d6a6a9cc3659103e0653880ae6222fd4exercised loopback TCP → RFC 6455 opening exchange → validated server frame → bounded text-message assembly → publicWebDriverBiDiJsonEnvelope::parse. Repository-native CI33907245004is terminal failure. Rust contracts job101134975540passed repository contracts, rustfmt, and workspace check, then failedcargo testonly atreal_transport_accepts_spec_defined_client_window_error: production returnedErr(InvalidMember { member: "error" })where the regression requiredOk(Error). The other eight tests in that integration target passed, including hostile unknown-code rejection.The minimal causal production repair is commit
01ef4296e099d7cae5dc27c5a896cd26443c720b: it adds onlyno such client windowto the bounded allow-list (30 → 31 admitted strings) and records the compatibility change. Arbitrary error strings remain fail closed.Standards evidence
Fresh primary-source review of the 3 September 2026 W3C Working Draft shows a specification-internal inconsistency:
ErrorResponse.errorpoints toErrorCode; the rendered local-end CDDL enumerates 30 values and omitsno such client window, while §3.5 separately defines that error and normative client-window algorithms return it. The product therefore admits the rendered CDDL vocabulary plus this one specifically reviewed normative error; this is not authority to infer or admit any other string.A concurrent documentation commit briefly described the CDDL itself as 31 values. That claim was rechecked against the current W3C TR and corrected without reverting the valid production compatibility repair:
7a4594317cccbd08678e3f3fbc7d62b905851109repairsdocs/doctoring/browser-agent-protocols.md,9fa4f37f1bbe6b835c157633cdebd8c3c2d3e799aligns the CHANGELOG security wording with the same 30+1 evidence model, and current6407895f...adds a focused repository documentation contract preventing that conformance claim from silently regressing.The latest published Working Draft and the reviewed live Editor’s Draft are both dated 3 September 2026; the Working Draft remains non-Recommendation work in progress. Canonical
docs/doctoring.mdstill contains an older 1 June 2026 snapshot and remains a separate byte-preserving documentation repair; do not rewrite unrelated historical evidence.Earlier event-name RED/fix
The earlier realistic event-name generation proved that empty or malformed module/event names were admitted. Production commit
4a9d7fa4cb8d6e31604d42a4a006a374f473d42anow requiressplit_once('.')with non-empty module and event remainder without copying the full command/event catalog into this common-envelope layer.Exact-head verification
The PR was moved to Ready only after the production and scoped standards repair were present. Current synchronize CI
33928760537is materialized for exact6407895f...; Rust contracts101202859907and Production coverage101202859779are queued with no runner assignment at the current observation point. These are the only native acceptance jobs for this generation; predecessor results do not transfer.Once exact-head CI and required security/review/thread gates are terminal GREEN, reconstruct #248 → #249 → #250 non-force in dependency order. Do not move descendants first or copy parent delta into them.
No Close, self-approval, bypass, force-push, destructive rebase, workflow/ruleset/secret mutation, coverage weakening, tag, release, or publication is authorized.