Add HTTPS detection E2E tests (#64)

[s2x]

Summary

• Adds E2E tests for HTTPS detection through the full SymfonyController stack
• Tests verify HTTPS is detected from both port 443 and X-Forwarded-Proto header
• Validates isSecure(), getScheme() return correct values

Changes

• tests/SymfonyControllerTest.php: Added testHttpsE2E() and testXForwardedProtoHttpsE2E()
• CHANGELOG.md: Updated with E2E tests entry for RequestConverter — Missing HTTPS flag breaks isSecure(), getScheme(), and URL generation #64

Issue

Closes #64

[s2x]

Code Review

Strengths

• Full-stack E2E approach: Tests exercise the complete path (Workerman Request → SymfonyController → RequestConverter → SymfonyRequest), not just unit-level mocks
• Both detection paths covered: Port 443 and X-Forwarded-Proto header each have dedicated tests
• Multi-assertion validation: Each test verifies HTTPS server var, isSecure(), and getScheme() — catching regressions at multiple levels
• Clean mock connections: Anonymous classes properly implement required methods (getLocalPort, getRemoteIp, etc.) with readonly typed properties
• Follows existing conventions: Matches the established TestRequestTrackingKernel pattern used by other E2E tests in the file
• CHANGELOG updated: Proper entry with issue link

---

Issues

Important (Should Fix)

1. No negative test for plain HTTP

• Files: tests/SymfonyControllerTest.php
• Problem: There is no test verifying that a regular HTTP request (port 80, no X-Forwarded-Proto) correctly returns isSecure() === false and getScheme() === 'http'.
• Why it matters: Without a negative test, a regression that unconditionally sets HTTPS = 'on' (e.g. from an overeager refactor) would go undetected — all existing tests would still pass because they only assert the positive path.
• How to fix: Add testHttpE2E() with a connection on port 80 and no forwarded header:

  $this->assertNull($symfonyRequest->server->get('HTTPS'));
  $this->assertFalse($symfonyRequest->isSecure());
  $this->assertSame('http', $symfonyRequest->getScheme());

2. No test for case-insensitive X-Forwarded-Proto

• File: tests/SymfonyControllerTest.php:770
• Problem: The implementation lowercases the header value (strtolower at RequestConverter.php:35), but there is no test confirming this works with values like "HTTPS" or "Https".
• Why it matters: Some reverse proxies (e.g. older HAProxy configs, custom middleware) send uppercase values. Without a test, someone could remove the strtolower call and the test suite would not catch it.
• How to fix: Either add a dedicated test case with X-Forwarded-Proto: HTTPS, or add an additional assertion in the existing test with a mixed-case value.

---

Minor (Nice to Have)

3. Duplicated mock connection classes

• Files: tests/SymfonyControllerTest.php:726-748, tests/SymfonyControllerTest.php:776-798, and tests/RequestConverterTest.php:344-362
• Problem: The anonymous TcpConnection subclass is copy-pasted across two tests in this file and a third time in RequestConverterTest.php.
• Why it matters: If TcpConnection gains a new abstract method or changes its constructor signature, all three copies need updating. This is a maintenance burden and a source of future test breakage.
• How to fix: Extract to a private helper method createMockConnection(int \$port): TcpConnection in each test class, or better — share it via a trait or a common base test class. RequestConverterTest.php already has a createMockConnection() helper that could be reused.

4. No E2E verification of SERVER_PORT value

• Files: RequestConverter.php:44, tests/SymfonyControllerTest.php
• Problem: The converter sets SERVER_PORT based on the HTTPS detection logic (\$localPort ?? (\$isHttps ? 443 : 80)). Unit tests in RequestConverterTest cover this, but the E2E tests do not verify it propagates through the full stack.
• Why it matters: getPort() is used by getSchemeAndHttpHost() and URL generation. If the port mapping breaks somewhere between RequestConverter and the final Request object, the E2E tests would not catch it.
• How to fix: Add assertions to the HTTPS E2E tests:

  \$this->assertSame(443, \$symfonyRequest->getPort());
  \$this->assertSame(80, \$symfonyRequest->getPort()); // in the HTTP negative test

---

Assessment

Ready to merge: No — with fixes

Reasoning: The implementation is solid and the positive-path E2E tests are well-written. The missing negative test (plain HTTP should stay HTTP) is a genuine test gap that could mask a regression where all requests are incorrectly marked as HTTPS. The case-insensitivity test is lower priority but still valuable for defense-in-depth.

[s2x]

Code Review

Strengths

• Full-stack E2E approach: Tests exercise the complete path (Workerman Request → SymfonyController → RequestConverter → SymfonyRequest), not just unit-level mocks
• Both detection paths covered: Port 443 and X-Forwarded-Proto header each have dedicated tests
• Multi-assertion validation: Each test verifies HTTPS server var, isSecure(), and getScheme() — catching regressions at multiple levels
• Clean mock connections: Anonymous classes properly implement required methods (getLocalPort, getRemoteIp, etc.) with readonly typed properties
• Follows existing conventions: Matches the established TestRequestTrackingKernel pattern used by other E2E tests in the file
• CHANGELOG updated: Proper entry with issue link

---

Issues

Important (Should Fix)

1. No negative test for plain HTTP

• Files: tests/SymfonyControllerTest.php
• Problem: There is no test verifying that a regular HTTP request (port 80, no X-Forwarded-Proto) correctly returns isSecure() === false and getScheme() === 'http'.
• Why it matters: Without a negative test, a regression that unconditionally sets HTTPS = 'on' (e.g. from an overeager refactor) would go undetected — all existing tests would still pass because they only assert the positive path.
• How to fix: Add testHttpE2E() with a connection on port 80 and no forwarded header:

  $this->assertNull($symfonyRequest->server->get('HTTPS'));
  $this->assertFalse($symfonyRequest->isSecure());
  $this->assertSame('http', $symfonyRequest->getScheme());

2. No test for case-insensitive X-Forwarded-Proto

• File: tests/SymfonyControllerTest.php:770
• Problem: The implementation lowercases the header value (strtolower at RequestConverter.php:35), but there is no test confirming this works with values like "HTTPS" or "Https".
• Why it matters: Some reverse proxies (e.g. older HAProxy configs, custom middleware) send uppercase values. Without a test, someone could remove the strtolower call and the test suite would not catch it.
• How to fix: Either add a dedicated test case with X-Forwarded-Proto: HTTPS, or add an additional assertion in the existing test with a mixed-case value.

---

Minor (Nice to Have)

3. Duplicated mock connection classes

• Files: tests/SymfonyControllerTest.php:726-748, tests/SymfonyControllerTest.php:776-798, and tests/RequestConverterTest.php:344-362
• Problem: The anonymous TcpConnection subclass is copy-pasted across two tests in this file and a third time in RequestConverterTest.php.
• Why it matters: If TcpConnection gains a new abstract method or changes its constructor signature, all three copies need updating. This is a maintenance burden and a source of future test breakage.
• How to fix: Extract to a private helper method createMockConnection(int \$port): TcpConnection in each test class, or better — share it via a trait or a common base test class. RequestConverterTest.php already has a createMockConnection() helper that could be reused.

4. No E2E verification of SERVER_PORT value

• Files: RequestConverter.php:44, tests/SymfonyControllerTest.php
• Problem: The converter sets SERVER_PORT based on the HTTPS detection logic (\$localPort ?? (\$isHttps ? 443 : 80)). Unit tests in RequestConverterTest cover this, but the E2E tests do not verify it propagates through the full stack.
• Why it matters: getPort() is used by getSchemeAndHttpHost() and URL generation. If the port mapping breaks somewhere between RequestConverter and the final Request object, the E2E tests would not catch it.
• How to fix: Add assertions to the HTTPS E2E tests:

  \$this->assertSame(443, \$symfonyRequest->getPort());
  \$this->assertSame(80, \$symfonyRequest->getPort()); // in the HTTP negative test

---

Assessment

Ready to merge: No — with fixes

Reasoning: The implementation is solid and the positive-path E2E tests are well-written. The missing negative test (plain HTTP should stay HTTP) is a genuine test gap that could mask a regression where all requests are incorrectly marked as HTTPS. The case-insensitivity test is lower priority but still valuable for defense-in-depth.