Skip to content

Always render the port in the HttpWaitStrategy probe URI - #12098

Open
bunnysayzz wants to merge 1 commit into
testcontainers:mainfrom
bunnysayzz:fix/http-wait-explicit-port-12096
Open

bunnysayzz wants to merge 1 commit into
testcontainers:mainfrom
bunnysayzz:fix/http-wait-explicit-port-12096

Conversation

@bunnysayzz

@bunnysayzz bunnysayzz commented Oct 1, 2026 •

Copy link
Copy Markdown

Fixes #12096.

buildLivenessUri dropped the port suffix when the resolved check port was 80 (plain HTTP) or 443 (TLS). That elision is only valid when host:<port> really is the mapped service, which cannot be assumed here: with custom WaitStrategyTarget implementations or proxied environments the resolved check port can be 443 while the service is not at https://host/, silently probing the wrong endpoint.

It also broke the port un-mapping in the log line: URI.getPort() returns -1 when the port is elided, so the exposed-port lookup threw IllegalStateException (caught, but every wait logged "Unexpected error occurred").

This always renders the explicit port. In the standard case https://host:443/path connects exactly like https://host/path, so no behavior change there, and the logged/timeout URIs now show the port actually being probed.

Could not compile locally (no JDK on this machine); relying on CI for the build.

Summary by CodeRabbit

  • Bug Fixes
    • Liveness checks now use an explicit port in their request address, including for standard HTTP and HTTPS ports. This makes the address format consistent across port configurations.

buildLivenessUri dropped the port suffix when the mapped check port was 80
(plain HTTP) or 443 (TLS). That elision is only valid when host:<port> really
is the mapped service, which cannot be assumed here: with custom
WaitStrategyTarget implementations or proxied environments the resolved
check port can be 443 while the service is not at https://host/, silently
probing the wrong endpoint.

An explicit port is also what the "un-map the port for logging" path relies
on: URI.getPort() returns -1 when the port is elided, which breaks the
exposed-port lookup and trips the "Unexpected error occurred" warning on
every wait.

Fixes testcontainers#12096.
@bunnysayzz
bunnysayzz requested a review from a team as a code owner October 1, 2026 19:09
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: ea6f89eb-9df0-43b3-a09c-1af307aa3922

📥 Commits

Reviewing files that changed from the base of the PR and between 8e54951 and 499db3f.

📒 Files selected for processing (1)
  • core/src/main/java/org/testcontainers/containers/wait/strategy/HttpWaitStrategy.java

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

HttpWaitStrategy.buildLivenessUri now includes the configured liveness port in the URI, including port 80 for HTTP and port 443 for HTTPS.

Changes

HTTP liveness URI

Layer / File(s) Summary
Include the configured port
core/src/main/java/org/testcontainers/containers/wait/strategy/HttpWaitStrategy.java
buildLivenessUri now adds the liveness port to the URI for HTTP and HTTPS, including their default ports.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~5 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: kiview

Merge Risk: ⚪ Minimal · up to 499db

Probe URIs now retain the configured port, including default ports. No concrete remaining failure is established, so the change appears ready for normal CI.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 499db

The change makes the existing probe port explicit without introducing a new destination source or weakening connection security. Standard HTTP and HTTPS default-port destinations remain equivalent, and no material security risk was identified in the changed behavior.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — For the inspected standard connection path, explicit default ports do not expand reachable hosts or numeric service ports. The change introduces no additional destination source, credential source, or privilege; custom proxy routing behavior remains unverified.

Trust Boundaries and Controls

  • observed — The private connection helper continues to use Java HTTP or HTTPS connections. Its existing allowInsecure branch installs a trust-all certificate manager; that behavior predates this change, and neither its activation nor its destination scope is changed by the patch.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description check ✅ Passed The description clearly explains the defect, its impact, the corrective change, and the related issue. It also states the local build limitation.
Title check ✅ Passed The title is concise and accurately identifies the main change: always rendering the port in the HttpWaitStrategy probe URI.
Linked Issues check ✅ Passed Issue [#12096] requires HttpWaitStrategy to preserve the resolved probe port when the port is 443 with TLS or 80 without TLS. The PR summary states that buildLivenessUri now always includes the li…
Out of Scope Changes check ✅ Passed The supplied change summary identifies one modification in HttpWaitStrategy.buildLivenessUri. The modification directly implements issue [#12096]. No unrelated file or behavior change is identified.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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.

HttpWaitStrategy drops :443 from the probe URI, silently probing the wrong endpoint

1 participant