Extend browser.createUserContext with acceptInsecureCerts parameter

[sadym-chromium]

Addressing #789. Allow for setting acceptInsecureCerts for per user context.

Open questions:

• How to tight service worker to a user context?
• Do we need even better integration with Classic? In order to properly specify behavior in case of capability set to "acceptInsecureCerts: true", and user context to "acceptInsecureCerts: false", the classic command on the user context's browsing context will still ignore certificate errors due to this:

If the remote end's accept insecure TLS flag is true, no certificate errors that would normally cause the user agent to abort and show a security warning are to hinder navigation to the requested address.

---

Preview | Diff

[whimboo]

Thank you @sadym-chromium for getting started on this PR! I know it's still in Draft mode but let me add one comment given that this would affect the way of implementation a lot.

[jgraham]

I think you need to integrate this better with the WebDriver classic spec (which admittedly is a mess).

For example: what happens if the WebDriver session ends? If you end a classic session then the flag is unset, and cert checking resumes. But BiDi supports multiple sessions, and this is global state, so do we need to revert it only when the last session ends? Or when the session that set the flag ends (I guess the latter). Currently it seems like it would persist indefinitely. Also, in theory, the capability can be unsupported. It seems reasonable that we handle the unsupported case here too (e.g. by returning an error).

(A really good patch here would have a check in fetch at the point at which a request is started. That would allow answering questions like "what about service workers?". But I understand that's a lot of work, so I don't consider it a blocker)

[sadym-chromium]

what happens if the WebDriver session ends?

I believe the behavior of acceptInsecureCerts and proxy should be identical. As long as we cannot configure proxy AFTER the user context is created, I'd propose to keep the acceptInsecureCerts and proxy behavior even after the WebDriver BiDi session is closed.

In order to prevent conflicts with Classic sessions, I'd propose to make the command params acceptInsecureCerts and proxy mutually exclusive with the capability.

Currently it seems like it would persist indefinitely.

This is intentional.

Also, in theory, the capability can be unsupported. It seems reasonable that we handle the unsupported case here too (e.g. by returning an error).

Good point. Will do.

[sadym-chromium]

Done:

• acceptInsecureCerts is tight to the session created the user context. When the session is disconnected, the default behavior taken from WebDriverClassic is restored.
• If the acceptInsecureCerts is not supported, error will be returned.

Open question:

• WRT the service workers, IDK how to tight one to the user context. Do you have any ideas of that, or do you think we can leave it for now?

[sadym-chromium]

FWIW: I believe the TLS check happens in fetch: create a connection, step 2, "... create a connection ...".

Let connection be a new connection whose key is key, origin is origin, credentials is credentials, and timing info is timingInfo. Record connection timing info given connection and use connection to establish an HTTP connection to host, taking proxy and origin into account, with the following caveats: [HTTP] [HTTP1] [TLS]

[jgraham]

Currently it seems like it would persist indefinitely.
This is intentional.

I think we should try rather hard to avoid persisting this beyond the end of WebDriver sessions. I don't think we want to allow a scenario where https cert checking is disabled for some browsing contexts even when automation is disconnected; that would seem to weaken some security properties that WebDriver classic has.

[css-meeting-bot]

The Browser Testing and Tools Working Group just discussed Access downloaded file content..

The full IRC log of that discussion

<AutomatedTester> topic: Access downloaded file content.
<AutomatedTester> github: https://github.com//issues/894
<AutomatedTester> sadym: we had a use case raised where someone downloaded a file and wanted to check the contents
<AutomatedTester> ... do we want to have a mechanism for progress and a way to assert the contents
<AutomatedTester> q+
<jgraham> q+
<Rob> q+
<AutomatedTester> ... puppeteer/playwright has a way of specifying the directory
<whimboo> q+
<Rob> q-
<Rob> q+
<AutomatedTester> Automatedtester: I don't think the file contents should be solved by this working group. We should tell people that it is downloaded and where it is and thats it
<AutomatedTester> sadym: what about selenium grid?
<AutomatedTester> automatedtester: we can have a grid endpoint if poeple want to download the file and not have it solved in this working group. it's mostly for intermediary nodes to solve
<AutomatedTester> ack next
<AutomatedTester> ack next
<AutomatedTester> sadym: so we can just a
<AutomatedTester> have capability for the download
<AutomatedTester> jgraham: If people turned on the response bodies then they would be able to go download it
<AutomatedTester> ... if you're wanting it to be downloaded and then access the file then there can be use cases for the client to solve it
<AutomatedTester> ... but I think getting response bodies working better would be the best first step
<AutomatedTester> ack next
<Rob> misread!
<AutomatedTester> whimboo: what would be important to have a capability for each user context for when tests are running parallel
<jgraham> 9I suggest we don't use the term "capability" for things that are actually configurable per user context)
<jgraham> s/9I/(I/
<AutomatedTester> ... once the session is ended should we delete the files?
<AutomatedTester> ack next
<AutomatedTester> Rob: is download to a local file system is the most important part. or to remote
<AutomatedTester> ... this complicates things for doing the download "twice"
<AutomatedTester> ... if the file is downloaded properly but not accessible it can create a DOS scenario by filling the disk space
<AutomatedTester> q?
<whimboo> q+
<AutomatedTester> whimboo: about download progess, how often does this get emited?
<AutomatedTester> ... we dont want to emit these too often
<AutomatedTester> ... we could do them in 10% increments
<AutomatedTester> sadym: I will go look and get back to you
<jgraham> For test use cases the download progress notification are a footgun, since they will differ between browsers and test runs
<jgraham> They make more sense for other use cases
<AutomatedTester> AutomatedTester: So it looks the consensus we want an event for progress (how often TBD), event for download complete and we want to have response bodies improved
<orkon> q+
<AutomatedTester> ... for downloaded files we still want to figure that out
<AutomatedTester> ack next
<jgraham> q- whimboo
<AutomatedTester> ack next
<Rob> Even saying 10% increments might be challenging (downloading a 78-byte file - do you emit on every 7.8 bytes?) We would need to be very concrete
<Rob> q+
<AutomatedTester> orkon: we should probably add a way to disable downloads
<AutomatedTester> ack next
<jgraham> q+
<AutomatedTester> Rob: If a person disabled the download, would it prevent the download from hitting the disk since you could probably see the response bodies
<AutomatedTester> ack next
<AutomatedTester> jgraham: technincally that is already allowed. THere is no guarantee what happens to the file
<AutomatedTester> ... in practise clients would complain but there is a lot of undefined behaviour so we need to be a lot more precise
<AutomatedTester> q?
<AutomatedTester> topic: Specify `acceptInsecureCerts` per user context
<AutomatedTester> github: https://github.com//pull/895
<AutomatedTester> sadym: we have discussed in the PR quite heavily
<jgraham> q+
<AutomatedTester> .... jgraham has questioned when should we revert the acceptInsecureCerts when the session for that user context disappears
<AutomatedTester> q?
<AutomatedTester> ack next
<AutomatedTester> jgraham: If there was a user context that existed before the session and then set it we should revert it
<Rob> re: webdriver before-after state being reset: agree
<AutomatedTester> ... there is a security reason for making sure we revert when the session is closed
<AutomatedTester> ... the lifetime of the config should be the lifetime of that session
<orkon> q+
<AutomatedTester> ... question is if session a sets X and session b extends it but it's a no op and then close the session does it affect all the sessions on the revert
<sadym> q+
<AutomatedTester> ack next
<jgraham> If session A sets the state and then session B sets basically the same state, but it's a noop, and then session A is closed, should it be extended to the lifetime of session B?
<AutomatedTester> orkon: in sadym in PR the config is set when the session is created. are you wanting to move back to setting the config separatrely from the session
<AutomatedTester> jgraham: ok, we don't want the latter, I was just calling this out
<orkon> s/session is created/user context is created/
<AutomatedTester> ack next
<jgraham> q+
<sadym> 1. are we fine with "it's implementation-specific"?
<sadym> 2. Should we allow setting the `setAcceptInsecureCerts` flag only if the cabaility is not set to reduce confusion and overrides?
<AutomatedTester> ack next
<AutomatedTester> jgraham: in terms of t
<AutomatedTester> ... the spec. In classic we say "do this thing"
<AutomatedTester> ... the ideal spec would hook into fetch and explain it does this thing when the navigate happen
<AutomatedTester> ... instead of doing "do an implementation defined thing here"
<AutomatedTester> ... for the capabilities here set here shuld be the default and then allow people to override
<AutomatedTester> ... I think it would be ok to have different behaviour if this is too difficult to implement
<AutomatedTester> q?

[OrKoN]

LGTM