feat(core): bind browser operations to exact protocol capabilities - #138
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus 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
changed the title
Close the locateNodes result-item gap by requiring the exact node remote type and a usable sharedId within the registry identifier budget before an untrusted adapter value can be retained as a later handle. Co-authored-by: Seongho Bae <[email protected]>
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Review at exact head 64d763004dc096f3bf8a3aea4cff47e07fbe60f8
Draft stacked on #127 (65f51937ff2af64d9c157666e7c3b7df89cbb89a). Typed operation-to-capability derivation is sound. The new query and remote-node types are honest that they grant no session/context/epoch authority. That is not enough to merge, and it is not enough to treat BiDi identifiers as admitted handles.
Would be REQUEST_CHANGES if GitHub allowed a non-author event on this PR. This actor is the PR author, so the submitted event is COMMENT. The two inline findings remain must-fix before this slice is complete.
CodeRabbit CLI 0.7.3 installed; coderabbit auth login --agent failed with automatic_login_failed / timed-out browser callback. GitHub CodeRabbit skipped this Draft. Findings below are from the workspace files at this exact head.
Current-head CI (Rust contracts, production coverage, MV3 fixture) is success. That is not merge evidence for a Draft stack (mergeable_state: unstable).
Blocking defects
-
WebDriverBiDiAccessibilityQuery::newadmits control injection and whitespace-only locators —crates/originweave-core/src/browser_protocol_operation.rs:97-108Empty role/name and UTF-8 over-budget cases fail closed. Control and whitespace-only values do not.
Some(" "),Some("button\n"), andSome("Submit\u{0000}task")construct successfully. Tests inwebdriver_bidi_accessibility_query.rsonly cover""and ASCII byte budgets.Why it matters: this is the first untrusted locator-text gate for
browsingContext.locateNodes. Those strings are the values a later adapter will serialize. Accessible names may contain spaces; they must not contain controls. Roles should reject whitespace. -
WebDriverBiDiRemoteNodeReference.shared_idis consistent withvalidate_external_identifier— and that consistency is the defect —browser_protocol_operation.rs:253-255vs privatebrowser_registry.rs:404-409Both gates check only empty + 512-byte UTF-8 budget. Neither rejects control or whitespace.
" ","shared-node-42\n", and"id\0"are admitted. Tests cover missing/empty/over-budget/한multibyte, not controls.Why it matters: rustdoc and CHANGELOG present this as admission of a later node handle. A
Clonetype that stores the raw BiDisharedIdand exposesshared_id()is the ambient-authority footgun if any caller reuses it across session/context/epoch. Tighten both gates together so they stay identical.
Non-blocking notes
BrowserProtocolOperationmapping anddispatch_operation_if_context_origin_epoch_currentcorrectly derive capability internally and skip the callback on denial. No authority-boundary defect in that mapping.validate_result_countand per-itemWebDriverBiDiRemoteNodeReference::newexist as separate APIs. There is no function that applies result-count admission, then per-item admission, thenBrowserAuthorityRegistry::bind_nodeto anObservedNodeHandleon the exact current session/context/origin/epoch. Stale-epoch and cross-session reuse are therefore untested on this path because the path does not exist.- Realm-local BiDi
handleis correctly not admitted. RequiringsharedIdas local fail-closed policy is documented honestly indocs/doctoring.md:11. - Rustdoc meets
#![deny(missing_docs)]on every new public item. Thenewrustdoc phrase “as a later node handle” overclaims relative to the struct docs (“untrusted transport handle”). - Accessibility query tests lack a multibyte UTF-8 budget case (remote-node tests already have one).
- This Draft is not mergeable onto protected
mainwhile #127 and its stack remain open.
Highest-value next bounded product slice
One function: given a reviewed WebDriverBiDiAccessibilityQuery and an untrusted locateNodes item list, validate_result_count, admit each item as WebDriverBiDiRemoteNodeReference (with control/whitespace rejection), then bind_node on the exact current session/context/origin/document epoch, returning ObservedNodeHandle values. Fail closed on stale epoch and cross-session reuse. No browser I/O.
Verdict
REQUEST_CHANGES (submitted as COMMENT because GitHub rejects author request_changes). Do not APPROVE. Do not merge this Draft.
| if role.is_some_and(str::is_empty) { | ||
| return Err(WebDriverBiDiAccessibilityQueryError::EmptyRole); | ||
| } | ||
| if role.is_some_and(|value| value.len() > MAX_BROWSER_ACCESSIBILITY_QUERY_ROLE_BYTES) { | ||
| return Err(WebDriverBiDiAccessibilityQueryError::RoleTooLong); | ||
| } | ||
| if name.is_some_and(str::is_empty) { | ||
| return Err(WebDriverBiDiAccessibilityQueryError::EmptyName); | ||
| } | ||
| if name.is_some_and(|value| value.len() > MAX_BROWSER_ACCESSIBILITY_QUERY_NAME_BYTES) { | ||
| return Err(WebDriverBiDiAccessibilityQueryError::NameTooLong); | ||
| } |
There was a problem hiding this comment.
Blocking: empty and UTF-8 budget are enforced, but this is the first untrusted locator-text admission for browsingContext.locateNodes and it still accepts control injection and whitespace-only values.
Some(" "), Some("button\n"), and Some("Submit\u{0000}task") all pass. webdriver_bidi_accessibility_query.rs covers "" and over-budget ASCII only — no control, no Unicode budget, no whitespace-only role.
Reject char::is_control() on both role and name (accessible names may contain spaces; roles should not). Do not treat this as a later serialization problem: these strings are the values a future adapter will put on the wire.
| if shared_id.is_empty() || shared_id.len() > MAX_EXTERNAL_BROWSER_IDENTIFIER_BYTES { | ||
| return Err(WebDriverBiDiRemoteNodeReferenceError::InvalidSharedId); | ||
| } |
There was a problem hiding this comment.
This matches private validate_external_identifier in browser_registry.rs:404-409 (empty + MAX_EXTERNAL_BROWSER_IDENTIFIER_BYTES only). It is not consistent with a control/whitespace gate, because that helper does not have one either.
" ", "shared-node-42\n", and "id\0" are admitted as a Clone value whose rustdoc calls it a later node handle. That is the first public BiDi identifier admission surface; copying the weaker registry check lets control-bearing sharedIds become bind_node map keys later.
Fail closed on is_control() / is_whitespace() here and in validate_external_identifier so the two gates stay identical. Add tests for those cases; UTF-8 budget coverage is already good.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Do not merge this Draft. Exact head 64d763004dc096f3bf8a3aea4cff47e07fbe60f8 correctly derives typed operation capabilities and keeps BiDi sharedId from becoming session/context/epoch authority. Two admission gates still accept control-bearing and whitespace-only protocol text.
Must fix before this slice can be treated as a fail-closed locator/handle boundary:
WebDriverBiDiAccessibilityQuery::newadmits" ", newline, and NUL in role/name. Keep ordinary spaces in accessible names; reject controls in names and reject whitespace/controls in roles.WebDriverBiDiRemoteNodeReference::newandvalidate_external_identifierboth accept whitespace-only and control-bearingsharedIdvalues. Tighten both gates together and add control-injection tests.
This remains a stacked Draft on #127. Passing or in-progress checks are not merge permission. Next bounded slice after this repair: compose result-count revalidation, per-item remote-node admission, and BrowserAuthorityRegistry::bind_node into ObservedNodeHandle values on the exact current session/context/origin/document epoch, with stale-epoch and cross-session reuse failing closed. No browser I/O.
Sent by Cursor Automation: Fix Issues
| if role.is_some_and(str::is_empty) { | ||
| return Err(WebDriverBiDiAccessibilityQueryError::EmptyRole); | ||
| } | ||
| if role.is_some_and(|value| value.len() > MAX_BROWSER_ACCESSIBILITY_QUERY_ROLE_BYTES) { | ||
| return Err(WebDriverBiDiAccessibilityQueryError::RoleTooLong); | ||
| } | ||
| if name.is_some_and(str::is_empty) { | ||
| return Err(WebDriverBiDiAccessibilityQueryError::EmptyName); | ||
| } | ||
| if name.is_some_and(|value| value.len() > MAX_BROWSER_ACCESSIBILITY_QUERY_NAME_BYTES) { | ||
| return Err(WebDriverBiDiAccessibilityQueryError::NameTooLong); | ||
| } |
There was a problem hiding this comment.
This constructor is the first untrusted locator-text gate for browsingContext.locateNodes. Empty and over-budget role/name fail closed, but control and whitespace-only values still construct: Some(" "), Some("button "), and Some("Submit\u{0000}task") succeed.
Accessible names may contain ordinary spaces ("Task text" is already a valid fixture). They must not contain controls. Roles are WAI-ARIA tokens and must reject whitespace and controls. A later adapter will put these strings on the wire; do not admit them here.
Add fail-closed tests for those cases, then reject them before storing owned strings.
| if shared_id.is_empty() || shared_id.len() > MAX_EXTERNAL_BROWSER_IDENTIFIER_BYTES { | ||
| return Err(WebDriverBiDiRemoteNodeReferenceError::InvalidSharedId); | ||
| } |
There was a problem hiding this comment.
sharedId admission currently matches validate_external_identifier, and that consistency is the defect: both gates check only empty plus the 512-byte UTF-8 budget. " ", "shared-node-42 ", and "id\0" are admitted as later node-handle text.
Tighten this gate and the registry identifier check together so they stay identical. Reject control and whitespace in the shared identifier, keep the existing empty/over-budget distinction, and add realistic control-injection tests. Do not treat the admitted shared_id() string as OriginWeave session, context, origin, or document-epoch authority.
|
Superseded by #174 after fresh exact-current ancestry verification. The actual live #138 head is |
2 participants
Partial implementation of #28, stacked on exact current PR #127 head
65f51937ff2af64d9c157666e7c3b7df89cbb89a.Buyer/security gap
The strongest current context/origin/document-epoch dispatch boundary previously accepted a caller-selected raw
BrowserProtocolCapability; this branch makes the buyer-visible operation explicit and derives the exact adapter capability internally. It also adds the first bounded WebDriver BiDi accessibility-query value needed to moveQueryNodesfrom a coarse operation label toward a real semantic observation adapter without exposing CSS/XPath/script/raw-DOM authority.Current dependency / stack
Current exact #138 head is
7fc113d26f864ac14e6793409d79328bbe954a2c. Exact live prerequisite #127 is65f51937ff2af64d9c157666e7c3b7df89cbb89a.Fresh ancestry is ahead 14 / behind 0, with merge base exactly the live prerequisite. The live-base delta is limited to five paths:
CHANGELOG.md,crates/originweave-core/src/browser_protocol_operation.rs,crates/originweave-core/src/lib.rs,crates/originweave-core/tests/browser_typed_operation_protocol_dispatch.rs, andcrates/originweave-core/tests/webdriver_bidi_accessibility_query.rs. Keep Draft while #127 and its prerequisite stack remain active. No prerequisite check, review, status, coverage, mergeability, synthetic-merge, or predecessor evidence transfers.TDD / RCA sequence
Earlier test-first lineage established typed operation-to-capability binding and same-call dispatch before production support, then made that boundary exact-green.
For the buyer-visible operation vocabulary, test-only head
993529296d9e95e360571ed6cd7c11dea9b8a70freached the intended missing-production compile boundary in CI31931265479after repository contracts and canonical formatting passed. Production then addedNavigate,QueryNodes,ClickNode,TypeText,WaitForState, andObserveNetworkwith exact capability derivation.For the bounded BiDi query contract:
d1d5c736a1e50154f59d1ba718108d8610be8165exposed canonical rustfmt differences first, so that setup failure is not promoted as semantic RED;341ccdcb0980240fcdb0e8630fd5b4475abd4960passed repository contracts and formatting, then CI31941083881reached the intended workspace compile failure becauseWebDriverBiDiAccessibilityQueryand its public constants/errors did not yet exist; this is the valid production-boundary RED;750348a499f87c4a057acf0f3cd69ad6537a0148exposed only one canonical export-order rustfmt delta; and7fc113d26f864ac14e6793409d79328bbe954a2capplies that exact canonical formatting and is exact-green.No lint allow, coverage exclusion, workflow change, gate weakening, or browser-I/O shortcut was introduced.
Implemented contract
BrowserProtocolOperationprovides six bounded semantics:Navigate→Navigation;QueryNodes→SemanticObservation;ClickNode→TypedInput;TypeText→TypedInput;WaitForState→SemanticObservation; andObserveNetwork→NetworkObservation.BrowserProtocolAdapterDescriptor::dispatch_operation_if_context_origin_epoch_currentrevalidates exact session/context/origin/document epoch, validates exact runtime protocol metadata, derives the operation's required capability internally, prevents callback execution on denial, and transfers the non-cloneable validated protocol-use proof plus the same typed operation into the immediate callback.WebDriverBiDiAccessibilityQueryis a transport-parameter value for the reviewedbrowsingContext.locateNodesaccessibility locator. It:accessibilitylocator type as constants/accessors; andThese bounds are local resource-governance limits, not claims about upstream browser maxima. The query value grants no session, context, origin, node, policy, capability, network, or secret authority and performs no browser I/O.
Exact-current proof
On unchanged exact head
7fc113d26f864ac14e6793409d79328bbe954a2cagainst exact prerequisite65f51937ff2af64d9c157666e7c3b7df89cbb89a:31941250130: success;95150693728: repository contracts, canonical formatting, locked workspace/all-target check, full tests, strict Clippy, and rustdoc success;95150693729: exact owned-production function/line/region/branch measurement and enforcement success;31941250167: success;Security/SAST/central review workflows not generated for this stacked Draft are absent evidence, not passing evidence. Skipped formatting-diagnostic helpers are not represented as passing gates. No predecessor-head, prerequisite-head, queued, pending, skipped, cancelled, absent, stale, synthetic, status-only, or model-only evidence is promoted as current proof.
Truth boundary
This branch still does not serialize a BiDi command, attach the query to an authenticated live browsing context, derive runtime metadata from Chromium, parse/normalize returned remote values into
SemanticNodeObservation, authorize policy/approval/destination/network use, validate semantic-node freshness for input, execute browser I/O, expose unrestricted JavaScript, or prove an action post-condition. The next real adapter boundary must compose the bounded query with exact current context/origin/document authority and treat returned browser data as untrusted observation before any typed action can use it.Passing automation is not independent approval. This scheduled actor does not merge, self-approve, alter workflows, add secrets, weaken checks, tag, or publish.