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

Add Automation subsection #42

Merged
merged 6 commits into from
Jun 5, 2020
Merged

Add Automation subsection #42

merged 6 commits into from
Jun 5, 2020

Conversation

bwalderman
Copy link
Contributor

@bwalderman bwalderman commented Jun 2, 2020

This change updates the spec with a WebDriver extension command for modifying the user agent's storage access policy. This is to address: w3c/webdriver#1437 (comment)

* Add Automation subsection.

* Address feedback.

* Fix whitespace.
@Brandr0id Brandr0id requested review from hober and annevk June 2, 2020 23:20
Copy link
Member

@Brandr0id Brandr0id left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Copy link
Collaborator

@johnwilander johnwilander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, although I'm not a Web Driver expert.

1. If |blocked| is not a [=boolean=] return a [=WebDriver error=] with [=WebDriver error code=] [=invalid argument=].
1. Let |doc| be the [=current browsing context=]'s [=active document=].
1. Let |key| be the result of [=generate a partitioned storage key|generating a partitioned storage key=] from |doc|.
1. If |key| is failure, return a [=WebDriver error=] with [=WebDriver error code=] [=unknown error=].
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I prefer "unsupported operation" over "unknown error". This is only likely to fail if the remote end (say, WebKit) doesn't support the operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I was on the fence about this myself. Switching to "unsupported operation".

Copy link
Collaborator

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should happen if the browsing context is the top-level browsing context? What if its active document is same-site with the top-level site?

It seems there should also be API surface to ensure certain outcomes before documents are created. That is, that you know what when you create a nested document it won't have partitioned data (or will).


The [=remote end steps=] are:

1. If |parameters| is not a JSON [=Object=], return a [=WebDriver error=] with [=WebDriver error code=] [=invalid argument=].
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this verified? Does this want to call https://infra.spec.whatwg.org/#parse-json-from-bytes?

https://infra.spec.whatwg.org/#convert-a-json-derived-javascript-value-to-an-infra-value might be useful on top for the remainder of these steps. (Or more likely, all of this should be abstracted and defined as part of the extension command infrastructure.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already covered by https://w3c.github.io/webdriver/#processing-model (see step 5 where parameters is parsed) so I've removed this check.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I filed w3c/webdriver#1529 to see about improving that further.

@bwalderman
Copy link
Contributor Author

What should happen if the browsing context is the top-level browsing context? What if its active document is same-site with the top-level site?

Thanks, I've added steps to handle these cases.

It seems there should also be API surface to ensure certain outcomes before documents are created. That is, that you know what when you create a nested document it won't have partitioned data (or will).

I've changed the API slightly. It now treats the current browsing context as the top-level site, and take a parameter to specify which embedded site to block or allow. This makes it possible to set policy for an embedded site without needing to create any nested documents and switch contexts first.

1. Let |origin| be the result of [=getting a property=] from |parameters| named `origin`.
1. If |origin| is not the [=serialization of an origin=] return a [=WebDriver error=] with [=WebDriver error code=] [=invalid argument=].
1. If |origin| is an [=opaque origin=] return a [=WebDriver error=] with [=WebDriver error code=] [=invalid argument=].
1. Let |embedded site| be the result of [=obtain a site|obtaining a site=] from |origin|.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding the ability to specify this per origin seems useful. Would it be useful to allow a mechanism to specify all origins (e.g. equiv to "blockallthirdpartycookies" preferences) as well?

1. Let |blocked| be the result of [=getting a property=] from |parameters| named `blocked`.
1. If |blocked| is not a [=boolean=] return a [=WebDriver error=] with [=WebDriver error code=] [=invalid argument=].
1. Let |origin| be the result of [=getting a property=] from |parameters| named `origin`.
1. If |origin| is not the [=serialization of an origin=] return a [=WebDriver error=] with [=WebDriver error code=] [=invalid argument=].
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work. I guess what you want to be doing is parse this field as a URL and then use that URL's origin. Similar to postMessage(). That would also help the following steps which assume origin is an object (and not a string).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I've adapted some of the language from the spec for postMessage. I've also added similar handling for asterisk (*) to provide a mechanism to specify all origins per @Brandr0id's request above.

Copy link
Collaborator

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The algorithm looks good to me now, thanks! My understanding of storage-access is still incomplete so I'm not sure if this covers all scenarios, but it does seem like a good start to me.

Copy link
Member

@hober hober left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@hober hober merged commit 9a15034 into privacycg:main Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants