Skip to content

Restore SNI coverage with a local TLS fixture - #1085

Closed
jmtdev0 wants to merge 2 commits into
WordPress:developfrom
jmtdev0:fix/1077-local-sni-test
Closed

Restore SNI coverage with a local TLS fixture#1085
jmtdev0 wants to merge 2 commits into
WordPress:developfrom
jmtdev0:fix/1077-local-sni-test

Conversation

@jmtdev0

@jmtdev0 jmtdev0 commented Sep 6, 2026

Copy link
Copy Markdown

Pull Request Type

  • I have checked there is no other PR open for the same change.

This is a:

  • Bug fix
  • New feature
  • Documentation improvement
  • Code quality improvement

Context

The existing testSNISupport test relied on a public third-party host whose certificate is now the same with and without SNI. As a result, the test no longer verifies the behavior it was intended to cover and had been skipped.

This change restores meaningful SNI coverage for issue #1077 without depending on a public endpoint or its current TLS configuration.

Detailed Description

  • Replace the skipped public-host check with a local HTTPS endpoint.
  • Generate an ephemeral test CA, a trusted localhost certificate, and an untrusted fallback certificate during the test.
  • Start a small Python standard-library TLS server which selects the trusted certificate only when the client sends SNI for localhost.
  • Exercise both transport implementations and the existing verifyname option variants through the current data provider.
  • Close the child process and remove temporary key and certificate files after each test.

The helper uses Python 3 from the test environment and has no third-party Python dependency.

Quality assurance

  • This change does NOT contain a breaking change (fix or feature that would cause existing functionality to change).
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added unit tests to accompany this PR.
  • The (new/existing) tests cover this PR 100%.
  • I have (manually) tested this code to the best of my abilities.
  • My code follows the style guidelines of this project.

Validation performed:

  • PHP 8.3.6 / PHPUnit 10.5.64: 6 tests passed.
  • PHP 5.6.40 / PHPUnit 5.7.27: 6 tests passed in each of three runs, 18/18 total.
  • PHPCS passed for the changed PHP files.
  • PHP parallel-lint passed for the changed PHP files.
  • Python bytecode compilation passed for the TLS helper.
  • git diff --check passed.

Fixes #1077

AI Disclaimer

This pull request was primarily developed with assistance from OpenAI Codex, an AI coding agent. For this PR, Codex analyzed issue #1077, inspected the relevant Requests transport tests, implemented the local TLS/SNI fixture, and ran the reported validation commands under the supervision of jmtdev0.

Human involvement in this PR was very low.

If you do not agree with the use of AI assistance or with the level of human involvement in this PR, please feel free to disregard it, close it, or request changes. I will fully respect that decision.

@jmtdev0
jmtdev0 marked this pull request as ready for review September 6, 2026 18:34
@schlessera

Copy link
Copy Markdown
Member

This PR introduces too much infra directly into this repo that is overkill for the purposes of this test. The preferred approach is to extend https://github.com/RequestsPHP/test-server as needed to allow for testing this specific scenario. I'll update the related issue and contributing guide to clarify the test setup.

@schlessera schlessera closed this Sep 9, 2026
@schlessera

Copy link
Copy Markdown
Member

Thanks for the PR, @jmtdev0 , but this is not the type of approach we want for this particular issue.

@jmtdev0

jmtdev0 commented Sep 9, 2026

Copy link
Copy Markdown
Author

Understandable. Thanks for reviewing the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

testSNISupport no longer verifies SNI

2 participants