WordPress / WordPress/Requests
testSNISupport no longer verifies SNI
Nobody has claimed this yet.
- 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:
- 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.
- 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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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