Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serial API #431

Closed
3 of 5 tasks
reillyeon opened this issue Oct 15, 2019 · 20 comments
Closed
3 of 5 tasks

Serial API #431

reillyeon opened this issue Oct 15, 2019 · 20 comments
Assignees
Labels
Missing: venue privacy-tracker Group bringing to attention of Privacy, or tracked by the Privacy Group but not needing response. Provenance: Fugu Resolution: ambivalent Topic: native platform integration Features that enable web sites to integrate better with native platforms Topic: powerful APIs APIs that reach into your life. Topic: Streams Any kind of stream-like feature Venue: WICG

Comments

@reillyeon
Copy link

reillyeon commented Oct 15, 2019

Hello TAG!

I'm requesting a TAG review of:

Further details:

  • Relevant time constraints or deadlines: The feature is targeting an Origin Trial in Chrome from versions 80 to 82 (February 2020 through June 2020). Assuming there are no implementation issues, developer or TAG feedback remaining to implement by the end of the trial a stable launch is planned for Chrome 83 (June 2020).
  • I have read and filled out the Self-Review Questionnaire on Security and Privacy. The assessment is here.
  • I have reviewed the TAG's API Design Principles
  • The group where the work on this specification is: WICG

We recommend the explainer to be in Markdown. On top of the usual information expected in the explainer, it is strongly recommended to add:

You should also know that...

I appreciate the effort the TAG puts into reviewing specifications. I am available to answer any questions and can participate in a live video chat to discuss the specification if desired.

We'd prefer the TAG provide feedback as (please select one):

  • open issues in our GitHub repo for each point of feedback
  • open a single issue in our GitHub repo for the entire review
  • leave review feedback as a comment in this issue and @-notify [github usernames]

Please preview the issue and check that the links work before submitting. In particular, if anything links to a URL which requires authentication (e.g. Google document), please make sure anyone with the link can access the document.

¹ For background, see our explanation of how to write a good explainer.

@lknik
Copy link
Member

lknik commented Nov 5, 2019

Hi,

Security/privacy self-check comments.

If an implementation chooses to make these permissions persist across browsing sessions then the availability of a particular device consitutes a piece of state

Any observations about implementations choosing to go for persistent permissions? Why leaving it to implementations?

A privacy-sensitive user is expected to not grant

I would be cautious with wording suggesting a particular expectations from users...

@kenchris kenchris self-assigned this Nov 5, 2019
@torgo torgo added this to the 2019-11-19-telecon milestone Nov 5, 2019
@cynthia cynthia self-assigned this Nov 5, 2019
@kenchris kenchris added Topic: native platform integration Features that enable web sites to integrate better with native platforms Topic: powerful APIs APIs that reach into your life. Topic: Streams Any kind of stream-like feature labels Nov 5, 2019
@reillyeon
Copy link
Author

@lknik, please file issues in our GitHub repository for each piece of feedback. As a practical matter GitHub doesn't let me configure notifications for organizations I am not a member of and so any replies on this issue go to my personal email address. I've copied your comments into WICG/serial#77.

@kenchris
Copy link

kenchris commented Nov 7, 2019

This spec really needs a loving hand. There are multiple issues filed that would be easy to fix and many things are not even described like xoff and there is even a table with an empty description column.

@cynthia has found additional issue that he will be filing soon, so please take a look at those

@torgo torgo added Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review and removed Progress: unreviewed labels Nov 19, 2019
@torgo
Copy link
Member

torgo commented Nov 19, 2019

According to @kenchris we are waiting for some clarification from @reillyeon on what specifically you would like TAG feedback on here.

@reillyeon
Copy link
Author

Thank you for the reminder. @kenchris pointed out at BlinkOn that the early draft status of the specification was causing confusion for the TAG reviewer. At this point I think feedback should focus on the text in the explainer and the security and privacy questionnaire.

The areas that I think are well-formed enough for meaningful feedback are the open(), close(), getSignals() and setSignals() methods as well as the readable and writable attributes and integration with the WHATWG Streams API. These are the parts that I think are well described in the explainer and for which I have some draft specification text in my fork.

On the other hand the device properties exposed by the getInfo() method, filter parameters for requestDevice() and the permission persistence model are all still under development.

@cynthia
Copy link
Member

cynthia commented Dec 17, 2019

Apologies for the delay - I've internally shared a review within the TAG, but never got around to actually posting it here. It's a very crude dump, so if the tone is not friendly - I apologize in advance; these are mostly personal notes. I'm trying to check which of these are fixed in the current fork draft, but leaving these here just so I don't lose it in a long chat backlog again.

Here is the dump:

  1. Contradicting spec text about baud rates - use cases section suggest no restrictions but the struct for opening ports suggest otherwise.
  2. Do not understand why port's buffer size can be specified when the buffer can only be read opaquely through a stream interface, also what even is the buffer here? (Where is it layered? UART? OS? browser?)
  3. Not sure if I understand the flow control flags, and there is no explanation on what their roles are and the mutual exclusivity of the flags. Would be good to know the intent here - in particular why a quad bool instead of string enum. (Which was done for parity bits, oddly enough)
  4. I'm not sure if SerialPortInfo should be exposing raw serialNumbers, even if device access was granted under user consent - serialNumbers are awfully unique. Per-origin unique would be acceptable, although I don't have an algorithm to implement that from the back of my head - the other part is when the origin needs to actually know the serial number. (e.g. to know what serial commands to send for different hardware revisions based on serial numbers.)
  5. "RECOMMNEDED"
  6. Question - is serial access expected to be exclusive (and possibly, only on the current active)? If not this opens up an interesting cross-origin communication channel.

@alice alice removed this from the 2019-12-16-week milestone Jan 27, 2020
@kenchris kenchris added Review type: CG early review An early review of general direction from a Community Group Progress: pending editor update TAG is waiting for a spec/explainer update labels Mar 3, 2020
@kenchris
Copy link

Hi @reillyeon have you have chance to look at the above comments?

@reillyeon
Copy link
Author

As @cynthia indicated these were preliminary notes I haven't taken a close look. As requested on the summary I would prefer individual spec issues filed for feedback.

To give the TAG a sense of my timeline, I have not been working on the Serial API for the last two months due to COVID-19 interruptions and other priority projects. I will be giving this project more focus during the month of June. My goals are to address a number of implementation issues in the Chromium prototype and continue to develop the draft specification. I will try to take the feedback above into account.

@plinss plinss added this to the 2020-09-07-f2f milestone Aug 24, 2020
@cynthia
Copy link
Member

cynthia commented Sep 23, 2020

@reillyeon Understandable. I didn't write formal feedback as I was told that the spec will be getting a large revamp, and the feedback might be touching on things that will no longer exist when that work is done. It seems like there have been some minor changes on your end; but not significant. Should we look at this later when you have reworked the spec? (Which according to WICG/serial#96 (comment) seems to be in progress.)

@plinss plinss removed this from the 2020-09-21-F2F-Cork milestone Oct 14, 2020
@reillyeon
Copy link
Author

Thank you for your patience. I have a sent WICG/serial#105 for review which includes my current draft of the full specification. Comments are welcome.

@reillyeon
Copy link
Author

To give the TAG a sense for my timeline, I am working towards shipping this API in mid-January.

@reillyeon
Copy link
Author

The PR above has been merged. The rewritten specification is available here and I am working on resolving any reported issues which have been obsoleted by the updates.

@reillyeon
Copy link
Author

Answering questions from the comment above:

  1. Contradicting spec text about baud rates - use cases section suggest no restrictions but the struct for opening ports suggest otherwise.

The text around baud rates has been cleaned up. There are no restrictions and the baud rate parameter is required in order to force developers to research an appropriate rate for the device being connected to.

  1. Do not understand why port's buffer size can be specified when the buffer can only be read opaquely through a stream interface, also what even is the buffer here? (Where is it layered? UART? OS? browser?)

The buffer size is used to inform the browser about how much it should try to read or write from the operating system at once. The browser architecture essentially requires it and the updated specification explains how it is used to set up the streams. I've been in communication with the Streams specification authors and implementers in Chromium and we have discussed better ways exposing the behavior of these kinds of internal buffers through the Streams API. There are some notes in the revised specification.

  1. Not sure if I understand the flow control flags, and there is no explanation on what their roles are and the mutual exclusivity of the flags. Would be good to know the intent here - in particular why a quad bool instead of string enum. (Which was done for parity bits, oddly enough)

Input and output control signals are defined by the standards such as RS-232. They are non-exclusive.

  1. I'm not sure if SerialPortInfo should be exposing raw serialNumbers, even if device access was granted under user consent - serialNumbers are awfully unique. Per-origin unique would be acceptable, although I don't have an algorithm to implement that from the back of my head - the other part is when the origin needs to actually know the serial number. (e.g. to know what serial commands to send for different hardware revisions based on serial numbers.)

I've removed the serialNumber field from SerialPortInfo for the time being but there is developer feedback that some identifiers are necessary to both distinguish between identical devices and identify specific device revisions as they might speak different protocols. I will be following up with improvements to this later.

  1. "RECOMMNEDED"

Fixed. :)

  1. Question - is serial access expected to be exclusive (and possibly, only on the current active)? If not this opens up an interesting cross-origin communication channel.

Serial access is exclusive. This is enforced by the operating system.

@kenchris kenchris added Progress: propose closing we think it should be closed but are waiting on some feedback or consensus and removed Progress: pending editor update TAG is waiting for a spec/explainer update Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review Review type: CG early review An early review of general direction from a Community Group labels Jan 27, 2021
@cynthia
Copy link
Member

cynthia commented Jan 27, 2021

The text around baud rates has been cleaned up. There are no restrictions and the baud rate parameter is required in order to force developers to research an appropriate rate for the device being connected to.

Read the latest revision, thanks for cleaning this up.

The buffer size is used to inform the browser about how much it should try to read or write from the operating system at once.

Understood, thanks for the clarification.

Input and output control signals are defined by the standards such as RS-232.

I see that this has been also cleaned up too. Thanks.

but there is developer feedback that some identifiers are necessary to both distinguish between identical devices and identify specific device revisions as they might speak different protocols.

This seems like a valid use-case. Let's revisit this review when that change is in place.

Aside from that, thank you for all of the clarifications and hard work to make the spec into a shippable state! We appreciate your patience and think our work here is done. Thanks for bringing this to our attention!

@cynthia
Copy link
Member

cynthia commented Jan 27, 2021

During our plenary, the Mozilla standards position concerns were brought up, and it seemed to be right to be on record that we are aware of the potential attack surface. Since WebHID has a blocklist registry, would it make sense for this to also have such a mechanism for vendors to opt their devices out of the web?

@tomayac
Copy link

tomayac commented Jan 27, 2021

[W]ould it make sense for this to also have such a mechanism for vendors to opt their devices out of the web?

Personally, I don't think we should go there. Printer manufacturers opt out their printers from being used with third-party cartridges, phone manufacturers opt out their phones from being repaired by third-parties,… The list goes on. Makers should be able to access the devices they own by whatever technical means, be it via the original software, or via a home-brew Web-based tool. Will this void the warranty? Maybe. But it should be possible.

@cynthia
Copy link
Member

cynthia commented Jan 28, 2021

Personally, I don't think we should go there.

Personally, I agree. I'd like to see how this is used (and abused) in the wild before we make decisions on how to balance the benefits and risks. Unfortunately, that's a single person's opinion.

There have been concerns raised by other stakeholders (e.g. Mozilla's standards positions), and we can't simply ignore the concerns. Otherwise, this feature has the risk of ending up being yet another feature with a single implementation.

@reillyeon
Copy link
Author

Chromium implements a block list mechanism for USB serial devices as a defense-in-depth measure. There are currently no entries.

During the development of the WebUSB specification the feedback from Mozillians (not an official Mozilla position) was similar to @tomayac's concerns as the original design required devices to opt in to connections from web content. There are open questions about how registries such as these should be managed to balance the risks against user choice. I don't have a good answer and I think we will need to wait and see how the ecosystem evolves. As mentioned in the "Security considerations" section there is a class of devices which are intentionally insecure which we would never include in the block list.

@torgo torgo added Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review and removed Progress: propose closing we think it should be closed but are waiting on some feedback or consensus labels Feb 10, 2021
@torgo
Copy link
Member

torgo commented Mar 8, 2021

Hi @reillyeon we're happy with the API design as indicated and looking forward to hearing about the incubation experience within WICG. We'd also like to understand better where this work is intended to end up after WICG. Will this be going to a working group such as Devices and Sensors, for example? Also it remains a concern that Mozilla's position remains as considered harmful. We encourage you to work with them and other implementers on mitigation strategies against the issues they have raised.

@torgo torgo closed this as completed Mar 8, 2021
@torgo torgo added Missing: venue Resolution: ambivalent privacy-tracker Group bringing to attention of Privacy, or tracked by the Privacy Group but not needing response. and removed Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review labels Mar 8, 2021
@reillyeon
Copy link
Author

I agree that this work should end up in the Devices and Sensors WG after incubation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Missing: venue privacy-tracker Group bringing to attention of Privacy, or tracked by the Privacy Group but not needing response. Provenance: Fugu Resolution: ambivalent Topic: native platform integration Features that enable web sites to integrate better with native platforms Topic: powerful APIs APIs that reach into your life. Topic: Streams Any kind of stream-like feature Venue: WICG
Projects
None yet
Development

No branches or pull requests

9 participants