Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid escaping Excon query string values #1077

Closed

Conversation

tt
Copy link

@tt tt commented Nov 4, 2024

Excon released a change in version 1.1.1 where query string values are no longer being unescaped. (See excon/excon#869 for context.)

This means that the CGI.escape in the adapter may double-escape values.

For example, a request to https://example.com/search?q=bob%40example.com will require a stub for https://example.com/search?q=bob%2540example.com.

Reproduction

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'

  gem 'excon', '= 1.1.1'
  gem 'webmock'
end

WebMock.enable!

WebMock.stub_request(:get, 'https://example.com/[email protected]')
  .to_return(status: 200)

Excon.get("https://example.com/search?q=#{URI.encode_uri_component('[email protected]')}")

Executing this code will raise the following exception:

~/.gem/ruby/3.3.5/gems/webmock-3.24.0/lib/webmock/http_lib_adapters/excon_adapter.rb:70:in `handle_request': Real HTTP connections are disabled. Unregistered request: GET https://example.com/search?q=bob%2540example.com with headers {'Accept'=>'*/*', 'Accept-Encoding'=>'deflate, gzip', 'Host'=>'example.com', 'User-Agent'=>'excon/1.1.1'} (WebMock::NetConnectNotAllowedError)

You can stub this request with the following snippet:

stub_request(:get, "https://example.com/search?q=bob%2540example.com").
  with(
    headers: {
          'Accept'=>'*/*',
          'Accept-Encoding'=>'deflate, gzip',
          'Host'=>'example.com',
          'User-Agent'=>'excon/1.1.1'
    }).
  to_return(status: 200, body: "", headers: {})

registered request stubs:

stub_request(:get, "https://example.com/[email protected]")

Downgrading the Excon version constraint from = 1.1.1 to = 1.1.0 or pointing gem 'webmock' at a path containing this pull request's change will let it pass.

@tt
Copy link
Author

tt commented Nov 4, 2024

Notifying @geemus for his awareness.

@geemus
Copy link

geemus commented Nov 4, 2024

@tt thanks for the heads up. Sorry you are hitting this. I was/am trying to fix up some of excon's direct mocking and didn't realize these indirect impacts. I also think it didn't entirely fix the issues anyway. I think this PR address what I was trying to do and probably also fixes your issue on the excon side of things: excon/excon#871. If that looks good I can merge/release for you, just let me know.

I had gotten caught up trying to fix some other potential webmock issues, but lack context. I'd welcome your insights if you have them, since I don't have much experience with webmock: excon/excon#870 and #1076.

@geemus
Copy link

geemus commented Nov 7, 2024

I think/hope this is fixed in just released excon v1.2.0

@tt
Copy link
Author

tt commented Nov 8, 2024

@geemus, thank you. This is indeed no longer an issue.

WebMock is potentially still incompatible with Excon 1.1.1 but not sure that's worth solving given the workaround needs to be maintained and that people should be able to upgrade.

@tt tt closed this Nov 8, 2024
@tt tt deleted the avoid-escaping-excon-query-string-values branch November 8, 2024 07:28
@geemus
Copy link

geemus commented Nov 8, 2024

@tt glad to hear it. Yeah, I'm open to trying to backport something if it becomes an issue, but I suspect that the upgrade should be easy and safe enough that I would definitely recommend that first if possible.

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.

2 participants