Skip to content

Fix root certificate issues for add-on store #18354

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 6 commits into from
Jul 2, 2025
Merged

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Jun 27, 2025

Link to issue number:

None

Summary of the issue:

If a device doesn't trust NV Access's TLS certificate, fetches to the add-on store fails.
When performing an update check, we update windows root certificates if our certificate is invalid or out of date.

Description of user facing changes:

Users without a trusted NV Access TLS certificate installed already should be able to access the add-on store

Description of developer facing changes:

Code related to updating root certificates is deprecated as it is not intended for the public

Description of development approach:

Made the code to update certificates more generic

Testing strategy:

  • Download NV Access's SSL certificate chain.
  • Modify it to remove the last 2 certificates, which are used to verify our cert.
  • Manually override requests.get in _fetchUrlAndUpdateRootCertificates to send our bad certificate chain. e.g. result = requests.get(url, timeout=_FETCH_TIMEOUT_S, verify=r"C:\Users\sean\Downloads\nvaccess-org-chain.pem")

Known issues with pull request:

none

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Announce deprecations to mailing list
  • Security precautions taken.

@coderabbitai summary

@Copilot Copilot AI review requested due to automatic review settings June 27, 2025 05:43
@seanbudd seanbudd requested a review from a team as a code owner June 27, 2025 05:43
@seanbudd seanbudd requested a review from SaschaCowley June 27, 2025 05:43
@seanbudd seanbudd added this to the 2025.2 milestone Jun 27, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request fixes root certificate issues for the add-on store by updating and centralizing certificate update logic. Key changes include:

  • Introducing a new networking module (source/utils/networking.py) that implements generic functions for updating Windows root certificates and fetching URLs.
  • Refactoring updateCheck.py to use the new certificate update function and removing deprecated legacy code.
  • Updating addonStore/dataManager.py to utilize the new networking function for improved consistency.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
source/utils/networking.py New module to update Windows root certificates and handle URL fetches
source/updateCheck.py Replaced legacy certificate update code with a call to the new function
source/addonStore/dataManager.py Updated cache hash fetching to use the centralized networking logic

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Jul 1, 2025
@seanbudd seanbudd merged commit b15c629 into beta Jul 2, 2025
9 of 10 checks passed
@seanbudd seanbudd deleted the fixCertUpdatesAddonStore branch July 2, 2025 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants