Open Bug 1589106 Opened 5 years ago Updated 2 years ago

Add protocol schema validation for WebDriver BiDi

Categories

(Remote Protocol :: WebDriver BiDi, task, P3)

task

Tracking

(Not tracked)

People

(Reporter: ato, Unassigned)

References

Details

(Whiteboard: [webdriver:backlog])

We want to add (WebDriver BiDi-ish/CDP) protocol schema validation
to the remote agent for a number of different reasons:

  1. It is currently possible to call intended-to-be-private methods
    on both parent- and child domains because we never verify that an
    incoming metod is a valid member of the protocol.

  2. We are being intentionally lax with type checks for the JSON
    request body, which is good because it allows us to write freer JS,
    but not so good because specific types could trigger behaviour that
    was not intended due to type coercions in JS.

  3. We also don’t verify that the output of responses and events are
    correct, which is a source for implementation bugs.

  4. The forthcoming bi-di proposal in WebDriver will insist on strict
    input and output checks, and protocol schema validation will help
    achieve conformance quicker.

Priority: -- → P3

When I did the original prototype for the remote agent I started
work on a protocol schema validator written as JS module. This was
similar in nature to the one that existed in Juggler:
https://github.com/puppeteer/juggler/blob/master/testing/juggler/protocol/Protocol.js

We later decided to scrap this approach because we had more important
things to take care of. There was also a sense at the time that it
would be good if we could reuse the serde traits offered by
https://github.com/devtools-html/rust-cdp.

With the forthcoming proposal for bi-di extension to WebDriver that
is going to be CDP-like, the requirements we have for schema
validation has sightly changed: we likely need to support a subset
of CDP, but also the new bi-di protocol as it is forthcoming. I
don’t know if this means we’ll reuse rust-cdp for one part and
invent a new crate with serde traits for the latter, or if we will
just put everything into one single entity. Time will show.

A proposal was made that we should do the schema validation using
serde in Rust. The work I’m doing in bug 1543115 will unlock some
integration across XPIDL between Rust and JS. The advantages of
this implementation strategy over a JS-based validator are that

  1. serde is quite good and we have prior experience how to use
    it;

  2. and if we move to a Rust-based HTTPD in the future we will
    inevitably need serde traits.

Depends on: 1543115
Priority: P3 → P2
Whiteboard: [puppeteer-beta-mvp]
Whiteboard: [puppeteer-beta-mvp] → [puppeteer-beta-reserve]
Priority: P2 → P3
Whiteboard: [puppeteer-beta-reserve]

I don't think that we will do this anymore for CDP so lets move this bug to WebDriver BiDi.

On the WebDriver BiDi repository we use the cddl rust crate to validate the generated CDDL definitions for the local client and the remote end. The files are attached as artifacts for each GitHub actions job.

With the new 0.9.0 release of cddl the parser and validator got heavily improved and is mostly perfect now. The only issue that I've seen is with optional fields which are based on a choice. I filed https://github.com/anweiss/cddl/issues/139 to get this fixed.

Nevertheless it would be good to see this Rust crate integrated into our code base and to have it running eg. in an experimental state like pref'ed off by default.

Component: Agent → WebDriver BiDi
Summary: Add protocol schema validation → Add protocol schema validation for WebDriver BiDi
Whiteboard: [webdriver:triage]

It would be good to also check the output in debug mode maybe to ensure that the schema make sense. I'll work on this whenever I have some time.

Whiteboard: [webdriver:triage] → [webdriver:backlog]
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.