w3c / webdriver-bidi

Bidirectional WebDriver protocol for browser automation
https://w3c.github.io/webdriver-bidi/
360 stars 40 forks source link

Disallow extra keys in command responses and events emitted by the remote end #111

Open foolip opened 3 years ago

foolip commented 3 years ago

This came up in https://github.com/web-platform-tests/wpt/pull/28381#discussion_r651005674

https://w3c.github.io/webdriver-bidi/#protocol-definition has the following remote end CDDL:

CommandResponse = {
  id: uint,
  result: ResultData,
  *text => any
}
Event = {
  EventData,
  *text => any
}

The *text => any means that any additional keys alongside "id", "method" and "params" is allowed. That definitely makes sense for client bindings for BiDi, to not break if the protocol evolves.

However, at this point we don't expect anything else to be included, and it would be useful for tests to verify this.

Options:

cc @jgraham @sadym-chromium

shs96c commented 3 years ago

Even for the tests, I can see the spec evolving and suddenly both of those options will render compatible implementations "invalid". Would we imagine reworking all the existing tests if we wanted to add additional keys? What if a client has a legitimate reason for wanting to add extra keys (for example, an authentication key) before we extend the spec?

I think the most compatible option is to only check the values that we expect to see and to ignore others. "Be strict in what you send, but be liberal in what you accept"

A similar issue came up when we were formulating the original WebDriver spec, and ultimately we found that it made life easier to be lax about additional keys.

foolip commented 3 years ago

Would we imagine reworking all the existing tests if we wanted to add additional keys?

Given how https://github.com/web-platform-tests/wpt/pull/28381 is structured, it would be a single place, an assert in the on_message helper than we'd tweak, probably by adding another "if this key is in the object" branch.

Be strict in what you send, but be liberal in what you accept

Right, and this issue is effectively about how we can ensure that implementations (browsers/drivers) are in fact strict in what they send. When seen from the point of view of the tests it looks like we're being strict about what we accept and are violating this design principle, but the tests are just a means of verifying the behavior of implementations.

jgraham commented 3 years ago

We could make it explicit that browsers aren't allowed to unilaterally add additional nonstandard keys when generating data, whilst still making clients ignore additional keys so we have some path to making backwards compatible changes to the spec. Argubably the spec nearly does that since it never explicitly sets any other keys. But we often use language like "let foo be a map matching the Bar production, with key set to value", and I suppose one could argue that adding extra data to foo would still meet the criteria of matching theBarproduction. So we could define what it means to be a map that "matches the production", explicitly requiring that only the listed fields are set.

foolip commented 3 years ago

That sounds like a great solution to me!