Skip to content

Log URL with correct protocol in Net::HTTP requests#186

Open
amleaver wants to merge 1 commit intotrusche:masterfrom
amleaver:net_https_scheme_logging
Open

Log URL with correct protocol in Net::HTTP requests#186
amleaver wants to merge 1 commit intotrusche:masterfrom
amleaver:net_https_scheme_logging

Conversation

@amleaver
Copy link

@amleaver amleaver commented Feb 5, 2026

Previously URLs were always logged with "http://" even if the connection was made over SSL. E.g.

Connecting: example.com:443
Sending: GET http://example.com:443/foo/bar/baz

use_ssl? is now checked when deciding what protocol to prefix the URL with.

Test are passing and tweak has been manually confirmed with an SSL connection:

bundle exec rspec spec --format=progress
2026-02-05 13:42:16 +0000 Thin web server (v2.0.1 codename Thinception)
2026-02-05 13:42:16 +0000 Maximum connections set to 1024
2026-02-05 13:42:16 +0000 Listening on 127.0.0.1:9292, CTRL+C to stop
..................................................................................................................................................................................................................................................
..................................................................................................................................................................................................................................................
..................................................................................................................................................................................................................................................
..................................................................................................................................................................................................................................................
..................................................................................................................................................................................................................................................
..................................................................................................................................................................................................................................................
.................................................................................................................................................................................................

Finished in 1.7 seconds (files took 0.55314 seconds to load)
1645 examples, 0 failures

Previously URLs were always logged with "http://"
even if the connection was made over SSL.
Copilot AI review requested due to automatic review settings February 5, 2026 13:43
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a bug where URLs were always logged with "http://" prefix even when connections were made over SSL. The fix checks the use_ssl? method to determine whether to use "https" or "http" as the protocol prefix.

Changes:

  • Added a private protocol helper method that returns "https" or "http" based on the SSL connection state
  • Updated URL construction to use the dynamic protocol instead of hardcoded "http://"

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +52 to +54
def protocol
use_ssl? ? 'https' : 'http'
end
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the implementation is correct, there is currently no test coverage for HTTPS connections in the test suite. The shared examples in spec/support/shared_examples.rb:3 hardcode "http://" in the expected URL, and none of the test adapters create SSL connections. Consider adding test coverage for HTTPS scenarios to ensure this change works as expected and to prevent regressions.

Copilot uses AI. Check for mistakes.
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.

1 participant