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

Bug for Proxy in requests? #705

Closed
liupgd opened this issue Apr 30, 2021 · 2 comments · Fixed by #833
Closed

Bug for Proxy in requests? #705

liupgd opened this issue Apr 30, 2021 · 2 comments · Fixed by #833

Comments

@liupgd
Copy link

liupgd commented Apr 30, 2021

Julia 1.6.0
HTTP.jl 0.9.7
WebSockets.jl 1.5.9

Hi, I'm using the separate package 'WebSockets.jl' which is based on HTTP.jl package. When I setted proxy for the websocket client, the client always returned 'Bad Request'. After my debug into HTTP.jl, I found maybe some code is wrong in line 95~120 of ConnectionRequest.jl.

  • Ther original code:
    try
        if proxy !== nothing && target_url.scheme == "https"
            # tunnel request
            target_url = merge(target_url, port=443)
            r = connect_tunnel(io, target_url, req)
            if r.status != 200
                close(io)
                return r
            end
            io = ConnectionPool.sslupgrade(io, target_url.host; kw...)
            req.headers = filter(x->x.first != "Proxy-Authorization", req.headers)
        end

        r =  request(Next, io, req, body; kw...)

        if (io.sequence >= reuse_limit
            || (proxy !== nothing && target_url.scheme == "https"))
            close(io)
        end

        return r
    catch e
        @debug 1 "❗️  ConnectionLayer $e. Closing: $io"
        try; close(io); catch; end
        rethrow(isioerror(e) ? IOError(e, "during request($url)") : e)
    end

The problem is that, if proxy is setted, the code only handle for https not for ws or wss.

  • New code I modified:
try
      if proxy !== nothing# && target_url.scheme == "https"
      # tunnel request
          if target_url.scheme == "https" || target_url.scheme == "wss"  # !!! wss also go through ssl, right? !!!
              target_url = merge(target_url, port=443)
          end
            r = connect_tunnel(io, target_url, req)# !!! proxy for websockets also connect with tunnel.
          if r.status != 200
              close(io)
              return r
          end
          if target_url.scheme == "https" || target_url.scheme == "wss"   # !!! So the same case here?
              io = ConnectionPool.sslupgrade(io, target_url.host; kw...)
          end
          req.headers = filter(x -> x.first != "Proxy-Authorization", req.headers)
      end

      r =  request(Next, io, req, body; kw...)

      if (io.sequence >= reuse_limit
      || (proxy !== nothing && target_url.scheme == "https"))
          close(io)
      end

      return r
  catch e
      @debug 1 "❗️  ConnectionLayer $e. Closing: $io"
      try; close(io); catch; end
      rethrow(isioerror(e) ? IOError(e, "during request($url)") : e)
  end
  • Test code
# %% Overwrite request of HTTP that will be used for WebSockets.jl
using WebSockets
import HTTP.ConnectionRequest:ConnectionPoolLayer, URI, getproxy, TCPSocket, ConnectionPool, 
                            unescapeuri, isempty, getkv, base64encode, setkv, sockettype,
                            getconnection, isioerror, IOError, defaultheader!, merge, connect_tunnel,
                            request
function request(::Type{ConnectionPoolLayer{Next}}, url::URI, req, body;
    proxy=getproxy(url.scheme, url.host),
    socket_type::Type=TCPSocket,
    reuse_limit::Int=ConnectionPool.nolimit, kw...) where Next
    if proxy !== nothing
        target_url = url
        url = URI(proxy)
        if target_url.scheme == "http"
            req.target = string(target_url)
        end

        userinfo = unescapeuri(url.userinfo)
        if !isempty(userinfo) && getkv(req.headers, "Proxy-Authorization", "") == ""
            @debug 1 "Adding Proxy-Authorization: Basic header."
            setkv(req.headers, "Proxy-Authorization", "Basic $(base64encode(userinfo))")
        end
    end

    IOType = ConnectionPool.Transaction{sockettype(url, socket_type)}
    local io
    try
        io = getconnection(IOType, url.host, url.port;
              reuse_limit=reuse_limit, kw...)
    catch e
        rethrow(isioerror(e) ? IOError(e, "during request($url)") : e)
    end

    if io.sequence >= reuse_limit
        defaultheader!(req, "Connection" => "close")
    end

    try
        if proxy !== nothing# && target_url.scheme == "https"
        # tunnel request
            if target_url.scheme == "https" || target_url.scheme == "wss"
                target_url = merge(target_url, port=443)
            end
            r = connect_tunnel(io, target_url, req)
            if r.status != 200
                close(io)
                return r
            end
            if target_url.scheme == "https" || target_url.scheme == "wss"
                io = ConnectionPool.sslupgrade(io, target_url.host; kw...)
            end
            req.headers = filter(x -> x.first != "Proxy-Authorization", req.headers)
        end

        r =  request(Next, io, req, body; kw...)

        if (io.sequence >= reuse_limit
        || (proxy !== nothing && target_url.scheme == "https"))
            close(io)
        end

        return r
    catch e
        @debug 1 "❗️  ConnectionLayer $e. Closing: $io"
        try; close(io); catch; end
        rethrow(isioerror(e) ? IOError(e, "during request($url)") : e)
    end

end

#%% Create a websocket connection to test server with proxy. Now, this works.
WebSockets.open("wss://echo.websocket.org", proxy="http://127.0.0.1:10809") do ws
    println("connected")
end

We have discussed this bug in this issue
Can anybody confirm whether my modification is right? I just do that intuitively. BTW, I've not tested 'ws' scheme, I think 'ws' should also be considerred in above.
Thanks.

@EricForgy
Copy link
Contributor

A change like this is probably a good idea. It might be easier to review / comment / suggest changes if you submit a PR though.

@liupgd
Copy link
Author

liupgd commented May 1, 2021 via email

liupgd added a commit to liupgd/HTTP.jl that referenced this issue May 2, 2021
@quinnj quinnj closed this as completed in 917fdb8 May 26, 2022
This was linked to pull requests Jun 8, 2022
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 a pull request may close this issue.

2 participants