Skip to content

Extend browser.createUserContext with acceptInsecureCerts parameter #895

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

Merged
merged 18 commits into from
May 7, 2025

Conversation

sadym-chromium
Copy link
Contributor

@sadym-chromium sadym-chromium commented Mar 31, 2025

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

Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

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

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.

@sadym-chromium sadym-chromium force-pushed the sadym/accept-insecure-certificates branch from 52ebe3c to 9fbbcbb Compare April 3, 2025 09:19
@sadym-chromium sadym-chromium requested a review from jgraham April 7, 2025 13:06
@sadym-chromium sadym-chromium changed the title Specify acceptInsecureCerts per user context Extend browser.createUserContext with acceptInsecureCerts parameter Apr 7, 2025
@jgraham
Copy link
Member

jgraham commented Apr 7, 2025

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
Copy link
Member

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?

@sadym-chromium sadym-chromium marked this pull request as ready for review April 11, 2025 12:30
sadym-chromium and others added 2 commits April 11, 2025 14:41
Co-authored-by: Alex Rudenko <[email protected]>
Co-authored-by: Alex Rudenko <[email protected]>
@sadym-chromium sadym-chromium force-pushed the sadym/accept-insecure-certificates branch from e698fb6 to 687a825 Compare April 11, 2025 12:43
Co-authored-by: Alex Rudenko <[email protected]>
Co-authored-by: Alex Rudenko <[email protected]>
sadym-chromium and others added 3 commits April 11, 2025 14:52
Co-authored-by: Alex Rudenko <[email protected]>
Co-authored-by: Alex Rudenko <[email protected]>
Co-authored-by: Alex Rudenko <[email protected]>
Copy link
Contributor

@OrKoN OrKoN left a comment

Choose a reason for hiding this comment

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

LGTM

@sadym-chromium sadym-chromium force-pushed the sadym/accept-insecure-certificates branch from e79a6e4 to 9032410 Compare April 11, 2025 13:35
@sadym-chromium
Copy link
Contributor Author

@jgraham WDYT?

Copy link
Member

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

This basically looks fine, made some editorial suggestions.

@sadym-chromium sadym-chromium requested review from jgraham and whimboo May 7, 2025 14:17
@sadym-chromium sadym-chromium merged commit 48a8fdd into main May 7, 2025
5 checks passed
@sadym-chromium sadym-chromium deleted the sadym/accept-insecure-certificates branch May 7, 2025 15:26
github-actions bot added a commit that referenced this pull request May 7, 2025
…er (#895)

SHA: 48a8fdd
Reason: push, by sadym-chromium

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

5 participants