Create a confirmation dialog to update root certificates when downloading add-ons#18402
Merged
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
hwf1324
reviewed
Jul 3, 2025
Contributor
|
This is useless; it will only prompt me to confirm the trust certificate over and over again. As I mentioned in #15905 (comment), Requests uses certificates from the independent certifi package. This requires adjusting the |
Member
Author
|
Ah I see we need to change the certificate source for all requests made through |
Member
Author
|
Actually, instead I think we should change our code to update the certifi certs, rather than windows root certificates: e.g. https://gist.github.com/jetersen/5efca1278e153aacc8413da71e77494c |
Contributor
|
Although certifi does not provide a way to modify the trust certificate store, we may have no choice but to do so. It is important to note that certs.pem may also be updated each time NVDA is updated. |
Member
Author
|
@hwf1324 - I've done some testing and I think overriding requests to use system certs is the best option we have https://pypi.org/project/pip-system-certs/ Can you try the next build? |
Contributor
|
No changes. Maybe I should run it from the source and observe the specific situation. |
See test results for failed build of commit e99f2feb6f |
Contributor
|
After running it from source, I successfully downloaded the add-on. It may have been caused by an incorrect launcher version downloaded from Actions previously. |
Member
Author
|
@hwf1324 it's possible that the patch is not being bundled correctly in the installer, I'll look into making sure it's exported correctly |
Member
Author
|
@hwf1324 - can you test the launcher from AppVeyor in the latest comment? I tested it, and it appears to be loading the SSL certificates from the right place |
Contributor
|
Yes, I can download add-ons directly from GitHub. no confirmation dialog pops up. So do we still need this dialog box? I think the credentials added through it are unclear. |
See test results for failed build of commit 3384603aa5 |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that NVDA uses system root certificates instead of certifi and prompts users to trust unverified root certificates when downloading add-ons.
- Switches
requeststo use system certificates by injectingpip_system_certs. - Adds a confirmation dialog to allow trusting a site’s root certificate on download failures.
- Patches remote client socket wrapping to avoid unwanted
pip_system_certsbehavior.
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| user_docs/en/changes.md | Added changelog entry for the add-on store certificate fix. |
| source/utils/networking.py | Refactored root‐cert update functions and added _getCertificate. |
| source/core.py | Injects pip_system_certs in main() to use system certificates. |
| source/addonStore/network.py | Implements _handleCertVerificationError and prompts user dialog. |
| source/addonStore/dataManager.py | Uses _fetchUrlAndUpdateRootCertificates for add-on store fetch. |
| source/_remoteClient/transport.py | Adds mock patch around wrap_socket to avoid truststore bug. |
| source/_remoteClient/server.py | Applies same mock.patch workaround for server socket. |
| pyproject.toml | Added pip_system_certs dependency and ignored certifi. |
Comments suppressed due to low confidence (3)
source/utils/networking.py:44
- [nitpick] Function name
_getCertificateuses camelCase, which is inconsistent with the project's snake_case convention. Rename it to_get_certificate.
def _getCertificate(url: str) -> bytes:
source/addonStore/network.py:316
- There are no unit tests covering the new certificate-verification fallback and user confirmation dialog flow. Consider adding tests for
_handleCertVerificationErrorand its dialog branch to ensure stability.
except requests.exceptions.SSLError as e:
source/addonStore/dataManager.py:128
- No import for
_fetchUrlAndUpdateRootCertificatesis present in this file, which will cause a NameError at runtime. Addfrom utils.networking import _fetchUrlAndUpdateRootCertificatesat the top.
response = _fetchUrlAndUpdateRootCertificates(url)
SaschaCowley
approved these changes
Jul 15, 2025
5 tasks
SaschaCowley
pushed a commit
that referenced
this pull request
Jul 18, 2025
Fixes #18460 Fixup of #18354 Summary of the issue: There is a fragile workaround introduced in #18402 (comment). This is also a soft API breakage for add-ons, forcing teleNVDA to use the workaround This is likely a bug fixed in truststore sethmlarson/truststore#149 We are using pip_system_certs to patch in an old version of truststore. this [is no longer being actively maintained](https://gitlab.com/alelec/pip-system-certs/-/issues/27#note_2372267321) and apparently truststore has the same patching feature now. Description of user facing changes: None Description of developer facing changes: Fix workaround which was required for add-ons such as tele NVDA Testing strategy: Ensure #18460 is not reintroduced. Make sure NVDA can still use update check and the add-on store in corporate network environments.
seanbudd
added a commit
that referenced
this pull request
Jul 22, 2025
### Reverts PR Reverts #18354 Reverts #18490 Reverts #18402 ### Issues fixed <!-- Issues that will be closed by reverting, i.e. the issues introduced via the PR getting reverted --> Fixes #18519 ### Issues reopened <!-- Issues that will be re-opened by reverting, i.e. the issues that were fixed via the PR getting reverted --> Reopens #15905 ### Reason for revert The current implementation breaks NVDA remote and breaks add-on APIs ### Can this PR be reimplemented? If so, what is required for the next attempt - use truststore.SSLContext directly rather than source code wide injections. It is [unclear](https://truststore.readthedocs.io/en/latest/#using-truststore-with-requests) how to do that with requests. [this SO answer suggestions an option](https://stackoverflow.com/a/78265028). - perform the changes in an API breaking release, encourage authors to use `inject_into_ssl, extract_from_ssl` where required
8 tasks
Contributor
|
@seanbudd, @SaschaCowley, please note that due to this PR, I have tested various alpha snapshots. The Please pay attention to this point when working again on this part of code, if applicable (e.g. in #18528) |
Member
|
@CyrilleB79 this PR has been reverted. I am also not sure what you mean by "The VERBOSE instead of DEBUGWARNING". Could you give an example? |
Contributor
|
@SaschaCowley sure, here is an example: In NVDA 2025.2beta3, in which this PR had not yet been reverted, in the log I get: whereas it should be: I know that this PR has been reverted. But I comment here to document the issue present in this PR so that when reimplementing it (e.g. #18528), you do not reintroduce this issue. |
SaschaCowley
added a commit
that referenced
this pull request
Oct 10, 2025
…8528) Fixes #15905 Reimplements #18354, #18402, #18490 ### Summary of the issue: When trying to download an add-on from the add-on store in certain environments such as corporates, the download fails due to the machine not trusting the root certificate of the add-on source. This can be caused by two issues: * the requests library using certifi rather than the system root certificates * the system root certificates not trusting the add-on download source When performing an update check, we update windows root certificates if our certificate is invalid or out of date. ### Description of user facing changes: NVDA should more reliably trust request endpoints using windows root certificates. e.g. accessing the add-on store in a corporate environment. ### Description of developer facing changes: requests and similar libraries now has truststore injected into it ### Description of development approach: This pull request improves how NVDA handles SSL/TLS certificate verification, particularly for HTTPS requests and add-on downloads. It ensures that the application uses the Windows root certificate store instead of the Python default (certifi), provides a fallback mechanism to update root certificates when encountering untrusted certificates, and enhances user interaction when certificate errors occur during add-on downloads. Additionally, shared networking logic has been refactored and centralized for maintainability. **Certificate Verification and Networking Improvements:** * Added the `truststore` dependency and injected it at startup so that all `requests` HTTP calls use the Windows root certificate store instead of certifi, improving compatibility in corporate and managed environments. (`pyproject.toml`, `source/core.py`) [[1]](diffhunk://#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R32-R33) [[2]](diffhunk://#diff-dbe862167add4fbe8e502757f792615f714c7603621c03d0c60c08a6a613ad68R34) [[3]](diffhunk://#diff-dbe862167add4fbe8e502757f792615f714c7603621c03d0c60c08a6a613ad68R680-R683) * Introduced a new `utils/networking.py` module that centralizes logic for fetching URLs, handling certificate verification errors, and updating Windows root certificates. This includes helper functions like `_fetchUrlAndUpdateRootCertificates`, `_getCertificate`, and `_updateWindowsRootCertificates`. (`source/utils/networking.py`) **Add-on Store and Update Check Enhancements:** * Updated add-on store and update check logic to use the new `_fetchUrlAndUpdateRootCertificates` function, ensuring robust handling of certificate verification failures and automatic root certificate updates when necessary. (`source/addonStore/dataManager.py`, `source/updateCheck.py`) [[1]](diffhunk://#diff-9fd9bbb610bac3157554c0876fba6ea3475383ea9de36c2101ba1127cf5189e5R30) [[2]](diffhunk://#diff-9fd9bbb610bac3157554c0876fba6ea3475383ea9de36c2101ba1127cf5189e5L127-R128) [[3]](diffhunk://#diff-9fd9bbb610bac3157554c0876fba6ea3475383ea9de36c2101ba1127cf5189e5L138-R143) [[4]](diffhunk://#diff-69aa1f3d46c308cf71c96483fd196eb49d6348727ab1953f834b1974ff031a43R68-R76) [[5]](diffhunk://#diff-69aa1f3d46c308cf71c96483fd196eb49d6348727ab1953f834b1974ff031a43L241-R245) * Improved the add-on download process to prompt the user when encountering an untrusted certificate, displaying the SHA256 fingerprint and allowing the user to trust and install the root certificate if appropriate. (`source/addonStore/network.py`) [[1]](diffhunk://#diff-097e46da59cc87eb0100bd63db7cfe32c7095ec66cf4be02d5dc84f28682ac7eL239-R297) [[2]](diffhunk://#diff-097e46da59cc87eb0100bd63db7cfe32c7095ec66cf4be02d5dc84f28682ac7eR314-R315) **Codebase Cleanup and Refactoring:** * Removed duplicate and now-unnecessary certificate update logic from `updateCheck.py` and centralized all certificate handling in `utils/networking.py`. (`source/updateCheck.py`) * Updated variable naming and improved code clarity in the add-on download logic. (`source/addonStore/network.py`) [[1]](diffhunk://#diff-097e46da59cc87eb0100bd63db7cfe32c7095ec66cf4be02d5dc84f28682ac7eL239-R297) [[2]](diffhunk://#diff-097e46da59cc87eb0100bd63db7cfe32c7095ec66cf4be02d5dc84f28682ac7eR314-R315) [[3]](diffhunk://#diff-097e46da59cc87eb0100bd63db7cfe32c7095ec66cf4be02d5dc84f28682ac7eL268-R324) [[4]](diffhunk://#diff-097e46da59cc87eb0100bd63db7cfe32c7095ec66cf4be02d5dc84f28682ac7eL278-R334) [[5]](diffhunk://#diff-097e46da59cc87eb0100bd63db7cfe32c7095ec66cf4be02d5dc84f28682ac7eL290-R346) ### Testing strategy: - [x] Get user testing from #15905 - [x] Smoke test using add-on store - [x] Smoke test update check ### Known issues with pull request: None --------- Co-authored-by: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link to issue number:
Related #15905 #18354
Summary of the issue:
When trying to download an add-on from the add-on store in certain environments such as corporates, the download fails due to the machine not trusting the root certificate of the add-on source.
This can be caused by two issues:
requestslibrary usingcertifirather than the system root certificatesAdditionally #18354 changed the NVDA update check to use requests rather than urllib, which would have introduced the issue with certifi to the update check instead of just the add-on store.
Description of user facing changes:
Description of developer facing changes:
None
Description of development approach:
pip_system_certswhich overridescertifito use system certs forrequestsTesting strategy:
Known issues with pull request:
None
Code Review Checklist:
@coderabbitai summary