M1I10: TlsProxy decorator#65
Merged
Merged
Conversation
s2x
commented
May 2, 2026
Contributor
Author
s2x
left a comment
There was a problem hiding this comment.
Review Summary — Round 1
| # | Severity | File:Line | Finding |
|---|---|---|---|
| 1 | MEDIUM | src/Server/Socket/TlsProxy.php:44 |
stream_context_set_option return values unchecked — silent failures |
| 2 | LOW | src/Server/Socket/TlsProxy.php:51 |
stream_socket_enable_crypto error details lost via @ |
| 3 | LOW | src/Server/Socket/TlsProxy.php:28 |
Missing @throws SocketCreationException on accept() |
Follow-up Observations
- Successful TLS handshake test:
testAcceptThrowsOnTlsHandshakeFailurecovers the failure path, but there's no test for a successful TLS handshake. Consider generating self-signed certs in-memory withopenssl_pkey_new()+openssl_csr_sign()for an integration test. File as a separate issue. - Cert format docs: The constructor docblock could explicitly mention that cert/key must be PEM-encoded files (matching
stream_context_set_optionexpectations forlocal_cert/local_pk).
What's Fine ✔️
- All acceptance criteria from issue #10 and
docs/superpowers/milestone-1/08-tls-proxy.mdare satisfied - Decorator pattern implemented correctly — delegates
createSocket()andaccept()to inner proxy STREAM_CRYPTO_METHOD_TLS_SERVERused for TLS handshake- 30s timeout set via
stream_set_timeoutbefore crypto isSupported()correctly checksfunction_existsfor both required functionsSocketCreationExceptionthrown on export/handshake failure- Tests validate delegate passthrough,
isSupported(), and TLS failure path - Reflection access on
Connection::$resourceis safe (the property isreadonly \Socket, non-nullable) - Error suppression
@onsocket_export_streamis safe — immediately followed by=== falsecheck
Notes
- Missing
@throws(finding #3): Theaccept()method throwsSocketCreationExceptionon two code paths but has no@throwstag. Other methods in the codebase (e.g.Connection::getPeerName) use@throwsannotations. Add@throws SocketCreationExceptionto the method PHPDoc.
s2x
commented
May 2, 2026
Contributor
Author
s2x
left a comment
There was a problem hiding this comment.
Review Summary — Round 2
| # | Severity | File:Line | Finding |
|---|---|---|---|
| — | — | No new findings. All Round 1 issues resolved. |
Follow-up Observations
- Successful TLS test:
testAcceptThrowsOnTlsHandshakeFailurecovers failure path, but no test for a successful handshake. Consider usingopenssl_pkey_new()+openssl_csr_sign()for in-memory self-signed cert generation in a follow-up issue. - Stream-based Connection: After
socket_export_stream(), the returnedConnectionwraps a\Socketthat should not be used withsocket_read()/socket_write()per PHP docs. A stream-awareTlsConnectionor equivalent may be needed separately.
What's Fine ✅
- All 6 acceptance criteria from issue #10 satisfied
- All 5 acceptance criteria from milestone doc
docs/superpowers/milestone-1/08-tls-proxy.mdsatisfied - Round 1 MEDIUM (
stream_context_set_optionunchecked) — FIXED with explicit return checks - Round 1 LOW (
stream_socket_enable_cryptoerror details lost) — FIXED witherror_get_last() - Round 1 LOW (missing
@throws) — FIXED with@throws SocketCreationExceptiondocblock - Clean decorator pattern, correct delegation
- Resource cleanup on all failure paths
- No security issues
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
src/Server/Socket/TlsProxy.phptests/Server/Socket/TlsProxyTest.phpTlsProxyis a decorator wrapping anySocketProxyInterfaceto add SSL/TLS encryption:createSocket()to inner proxyaccept()delegates to inner proxy, thensocket_export_stream()+stream_socket_enable_crypto()with 30s timeoutisSupported()checks function availabilitySocketCreationExceptionon TLS handshake failureAcceptance criteria
SocketProxyInterfaceaccept()callsstream_socket_enable_crypto()withSTREAM_CRYPTO_METHOD_TLS_SERVERisSupported()checks function availabilityisSupported()Closes #10