WordPress / WordPress/Requests

testSNISupport no longer verifies SNI

Open
#1,077 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Component: Transports Status: help wanted Type: testing/chores/QA
Dominant language
PHP
Stars
3.6k
Forks
500
Avg merge
2d 21h
Merged PRs (30d)
5

Description

testSNISupport in tests/Transport/BaseTestCase.php is meant to check that our transports send Server Name Indication. It does not do that anymore, and has been marked as skipped until it can be made to do so again.

Why it does not test anything

The test works by requesting an HTTPS host and expecting a 200. That only proves something if the host serves a different certificate when no SNI is sent, so that a client which omits it fails. The docblock said as much:

humanmade.com (owned by Human Made and used with permission) points to CloudFront, and will fail if SNI isn't sent.

That is no longer true. Comparing the certificate served with and without SNI:

humanmade.com                      with SNI: hm-marketing-prod.altis.cloud   without: identical
wordpress.org                      with SNI: wordpress.org                   without: identical
www.php.net                        with SNI: www.php.net                     without: identical
requests-test-server.onrender.com  with SNI: *.onrender.com                  without: identical

None of them distinguishes the two cases, so a transport which silently stopped sending SNI would still pass the test. sni.velox.ch, which used to be the go-to host for this sort of check, no longer resolves at all.

This is unlikely to be fixable by picking a different public host: servers these days generally present a usable default certificate whether or not SNI is sent.

Why it is skipped rather than repointed

The host it currently uses answers 403 to every client, including a plain curl, so the test fails for reasons which have nothing to do with the library. Swapping in another host would make the suite green again, but it would also make the test look like it covers something it does not, so skipping it is the more honest state to leave it in.

Worth noting the domain has already been changed once for a similar reason: feelingrestful.com was decommissioned in 2019 (fcb184a), which is how we ended up on the current one.

Ways forward

These are not mutually exclusive:

  1. Restore real coverage with a local TLS endpoint serving two certificates on one port, so a request without SNI receives the wrong one and fails. mitmproxy is already a test dependency and can terminate TLS, so the pieces are largely there. This would also take a third party out of the test entirely.
  2. Rename the test to describe what it actually does and drop the SNI framing, so the name stops implying coverage we do not have.
Related

SNI_enabled, and its decoupling from the verifyname option in 213f7e7, currently has no test which would catch a regression.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Read tests/Transport/BaseTestCase.php and the skipped testSNISupport, then inspect the existing mitmproxy test setup. Choose between restoring coverage with a local TLS endpoint presenting different certificates or renaming the test to match its behavior. Done means the suite reliably tests SNI_enabled or no longer claims the skipped test provides SNI coverage.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
networking, testing-qa
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.