• 
      

    Fix fallback cert handling and improve cert testing infrastructure.

    Review Request #15188 — Created July 21, 2026 and updated

    Information

    Review Board
    release-8.x

    Reviewers

    I recently landed a change for HTTPS handling, but it was an older
    version of the branch that was missing fixes for legacy cert handling
    and testing.

    The backwards-compatibility logic for migrating old PEM data to modern
    cert handling had some flaws. It attempted to build a standalone SSL
    context, which wasn't safe or compatible with the way that
    CertificateVerificationHTTPSHandler worked, causing managed
    certificates to be omitted.

    Instead of trying to shoe-horn in a custom context, that logic now
    builds extra arguments for controlling hostname checks and extra CA data
    for the managed context. The handler class takes these and ensures
    they're factored in at the right stage.

    Hostnames and ports are no longer passed to
    CertificateVerificationHTTPSHandler. These weren't available to the
    constructor in my prior change, yet were being passed in as part of the
    call due to the branch mismatch. These need to be inferred from the URL
    in order to get the correct result. This broke several unit tests.

    Testing infrastructure has several key fixes:

    1. Cert storage is now cleared between tests. This uses the path state
      for the cert manager instead of manually building paths, like before.

    2. A new CaptureSSLMixin class is added for cert-related test classes
      that set up the appropriate spies needed to perform SSL context
      captures and to do so in a way that handles redirects. It also allows
      for control over the expected response.

    3. Unit tests no longer hard-code stored cert paths, but instead use the
      cert manager's computed state.

    Unit tests passed.

    Tested with a repository with a legacy self-signed cert. Connected to
    my server and saw it worked. The certificate store had the migrated
    cert.

    Summary ID
    Fix fallback cert handling and improve cert testing infrastructure.
    I recently landed a change for HTTPS handling, but it was an older version of the branch that was missing fixes for legacy cert handling and testing. The backwards-compatibility logic for migrating old PEM data to modern cert handling had some flaws. It attempted to build a standalone SSL context, which wasn't safe or compatible with the way that `CertificateVerificationHTTPSHandler` worked, causing managed certificates to be omitted. Instead of trying to shoe-horn in a custom context, that logic now builds extra arguments for controlling hostname checks and extra CA data for the managed context. The handler class takes these and ensures they're factored in at the right stage. Hostnames and ports are no longer passed to `CertificateVerificationHTTPSHandler`. These weren't available to the constructor in my prior change, yet were being passed in as part of the call due to the branch mismatch. These need to be inferred from the URL in order to get the correct result. This broke several unit tests. Testing infrastructure has several key fixes: 1. Cert storage is now cleared between tests. This uses the path state for the cert manager instead of manually building paths, like before. 2. A new `CaptureSSLMixin` class is added for cert-related test classes that set up the appropriate spies needed to perform SSL context captures and to do so in a way that handles redirects. It also allows for control over the expected response. 3. Unit tests no longer hard-code stored cert paths, but instead use the cert manager's computed state.
    c292dad6cd4d0e7f1f7199c1f0d055568ce4e29d
    Description From Last Updated

    Please add a new instance variable annotation for this.

    david david

    Can you put this in the doc comment next to the definition? It makes more sense there.

    david david

    HTTPSHandler.__init__() sets self._context = None, clobbering the context you saved above. https_open() resets it too. If we really want to …

    david david

    This applies check_hostname=False and the legacy cadata to all hosts, even ones reached via a redirect. This isn't a new …

    david david

    Can we catch SSLError here and reraise a CertificateVerificationError?

    david david

    CaptureSSLMixin calls a bunch of kgb stuff, which will flag if its just typed as TestCase. Can we do this …

    david david

    Needs a trailing comma

    david david

    This (and the associated url attribute) never actually work. AbstractHTTPHandler.do_open() overwrites the response URL, so any URL passed to this …

    david david

    We should add a note in this docstring that users of the mixin will need to also mix in kgb.SpyAgency …

    david david

    Typo: certificats -> certificates

    david david

    This reads a little odd. Can we say "The mock HTTP responses to serve, in order"?

    david david

    This is currently not customizable. It gets set in setUp() and then immediately passed to SpyOpReturnInOrder, which creates an internal …

    david david

    This constructs a fresh CertificateManager, while stuff in test_hosting_service_http_request.py and test_hosting_service_auth_form.py do from reviewboard.certs.manager import cert_manager. The tests mutate state …

    david david

    Copy/pasto

    david david

    The TestCase instance will get discarded after teardown, so it seems like this is all just a noop?

    david david

    The docstring for this function needs to be updated.

    david david

    It's kind of weird to reuse cert_data as a success value. Can we create a separate migrated flag that gets …

    david david

    This is now unused. And once you get rid of this declaration, I think the ssl import will also be …

    david david

    These seem unused now.

    david david

    This seems unused now.

    david david

    This docstring was incorrect before, and is also incorrect now.

    david david
    Checks run (2 succeeded)
    flake8 passed.
    JSHint passed.
    david
    1. 
        
    2. reviewboard/certs/http.py (Diff revision 1)
       
       
      Show all issues

      Please add a new instance variable annotation for this.

    3. reviewboard/certs/http.py (Diff revision 1)
       
       
       
       
       
      Show all issues

      Can you put this in the doc comment next to the definition? It makes more sense there.

    4. reviewboard/certs/http.py (Diff revision 1)
       
       
      Show all issues

      HTTPSHandler.__init__() sets self._context = None, clobbering the context you saved above. https_open() resets it too.

      If we really want to be able to override this, we should store it separately and apply it (like I tried to do in /r/15185/). That said, the only use of the context parameter was removed in this change (hostingsvcs/base/http.py line 582), so we don't use it anymore. I'd say we just get rid of the parameter.

    5. reviewboard/certs/http.py (Diff revision 1)
       
       
       
       
       
      Show all issues

      This applies check_hostname=False and the legacy cadata to all hosts, even ones reached via a redirect. This isn't a new regression (the old single-context approach was essentially the same thing), but it's not too ugly to tighten up.

      In HostingServiceHTTPRequest.open(), you were previously passing in the request URL and got rid of that in this change. I say let's bring it back for a different job:

      urlopen_handlers.append(CertificateVerificationHTTPHandler(
          local_site=hosting_account.local_site,
          legacy_cert_hostname=urlparse(url).hostname,
          **ssl_kwargs,
      ))
      

      Then the constructor above can save it, and in here:

      if hostname:
          context = self._cert_manager.build_ssl_context(...)
      
          if hostname == self._legacy_cert_hostname:
              context.check_hostname = self._rb_check_hostname
      
              if extra_cert_data := self._extra_cert_data:
                  context.load_verify_locations(cadata=extra_cert_data)
      

      test_with_http_to_https_redirect_without_certs is already testing a redirect. We can use that as a prototype for a new test that does something similar with a handler that creates the handler with legacy_cert_hostname='example.com' and then check that the resulting SSL contexts include two items, and that the first one has check_hostname=False and the second check_hostname=True.

    6. reviewboard/certs/http.py (Diff revision 1)
       
       
      Show all issues

      Can we catch SSLError here and reraise a CertificateVerificationError?

    7. reviewboard/certs/tests/testcases.py (Diff revision 1)
       
       
      Show all issues

      CaptureSSLMixin calls a bunch of kgb stuff, which will flag if its just typed as TestCase. Can we do this instead?

      class TestCaseMixinBase(kgb.SpyAgency, TestCase):
          pass
      
    8. reviewboard/certs/tests/testcases.py (Diff revision 1)
       
       
      Show all issues

      Needs a trailing comma

    9. reviewboard/certs/tests/testcases.py (Diff revision 1)
       
       
       
       
       
       
       
       
       
      Show all issues

      This (and the associated url attribute) never actually work. AbstractHTTPHandler.do_open() overwrites the response URL, so any URL passed to this will always get overwritten with the actual response URL.

      That makes the assignment in test_open_with_legacy_ssl_cert_hostname_mismatch a no-op. It happens to pass because the mocked URL is the same as the response URL, but that's passing for the wrong reason.

      Probably the best thing is to just get rid of url and geturl() here.

    10. reviewboard/certs/tests/testcases.py (Diff revision 1)
       
       
      Show all issues

      We should add a note in this docstring that users of the mixin will need to also mix in kgb.SpyAgency ahead of this.

    11. reviewboard/certs/tests/testcases.py (Diff revision 1)
       
       
      Show all issues

      Typo: certificats -> certificates

    12. reviewboard/certs/tests/testcases.py (Diff revision 1)
       
       
      Show all issues

      This reads a little odd. Can we say "The mock HTTP responses to serve, in order"?

    13. reviewboard/certs/tests/testcases.py (Diff revision 1)
       
       
       
      Show all issues

      This is currently not customizable. It gets set in setUp() and then immediately passed to SpyOpReturnInOrder, which creates an internal copy of the list. All the test cases which want to manipulate this are unspying/respying.

    14. reviewboard/certs/tests/testcases.py (Diff revision 1)
       
       
       
       
      Show all issues

      This constructs a fresh CertificateManager, while stuff in test_hosting_service_http_request.py and test_hosting_service_auth_form.py do from reviewboard.certs.manager import cert_manager. The tests mutate state through the global but then read paths off the instance. It works because they're both resolving to the same storage path, but it seems fragile.

      Can we have this mixin just use the singleton?

    15. reviewboard/certs/tests/testcases.py (Diff revision 1)
       
       
       
       
       
       
       
      Show all issues

      Copy/pasto

    16. reviewboard/certs/tests/testcases.py (Diff revision 1)
       
       
       
       
      Show all issues

      The TestCase instance will get discarded after teardown, so it seems like this is all just a noop?

    17. reviewboard/hostingsvcs/base/http.py (Diff revision 1)
       
       
       
       
       
      Show all issues

      The docstring for this function needs to be updated.

    18. reviewboard/hostingsvcs/base/http.py (Diff revision 1)
       
       
       
       
      Show all issues

      It's kind of weird to reuse cert_data as a success value.

      Can we create a separate migrated flag that gets set to True here, and then below instead of if cert_data we can do if not migrated:?

    19. reviewboard/hostingsvcs/base/http.py (Diff revision 1)
       
       
      Show all issues

      This is now unused. And once you get rid of this declaration, I think the ssl import will also be unused.

    20. Show all issues

      These seem unused now.

    21. Show all issues

      This seems unused now.

    22. Show all issues

      This docstring was incorrect before, and is also incorrect now.

    23.