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.

Sign up for GitHub

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

Update transport.go , to fix ntlm 407 challenge httpresponse containing extra httpbody issue #119

Closed
wants to merge 2 commits into from
Closed

Update transport.go , to fix ntlm 407 challenge httpresponse containing extra httpbody issue #119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2b50c76
Update transport.go
ziyiwang May 23, 2024
3de31fe
Update pacrunner.go to optimise ipaddress issue return via routable …
ziyiwang May 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 14 additions & 19 deletions pacrunner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,31 +182,26 @@ func convertAddr(call otto.FunctionCall) otto.Value {
}

func myIpAddress(call otto.FunctionCall) otto.Value {
// When the host has multiple IPs, Chrome seems to go to some length to find the best one
// (see https://cs.chromium.org/chromium/src/net/proxy_resolution/pac_library.cc?g=0&l=22),
// but we'll just return the first non-loopback IPv4 address that we find (or "127.0.0.1" if
// there are none) and hope this is good enough.
addrs, err := net.InterfaceAddrs()
// By dialling a UDP address, we find out what the local IP address is.
// No packets are ever sent, because UDP is "connectionless".
// The IP address chosen is in one of the "documentation" prefixes that are guaranteed not to exist on the
// internet, and is therefore "safe" to use. It will generally follow the default route set in the OS.
conn, err := net.Dial("udp", "192.0.2.1:53") // RFC5737 TEST-NET-1
Copy link
Owner

Choose a reason for hiding this comment

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

It makes sense to do a UDP dial and take the local IP address, but it's not really clear to me which network interface the OS will choose to dial 192.0.2.1. RFC5737 says that this address "SHOULD" be unroutable, but maybe it's possible for people to, for example, set this up as a local test network which would quietly break Alpaca in unexpected ways?

Before we settle on an approach, I'd like to test a few scenarios myself first (including what happens if I try to make a UDP connection to this IP). I'll get back to you on that later this week, but let's make this myIpAddress() fix in a separate PR? It seems unrelated to the other change.

Copy link
Owner

Choose a reason for hiding this comment

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

@ziyiwang I took a look at this, and yes Alpaca does give the "wrong" IP address back - in my case, it returns my WiFi adapter's IP address, rather than the VPN address, so similar to what you were seeing.

I've written up the problem here: #122.

I personally don't need to use the myIpAddress() function, so thanks for reporting this, it's not something I would've noticed myself.

if err != nil {
return otto.UndefinedValue()
}
for _, addr := range addrs {
s := addr.String()
// Remove the first rune that is not either a digit or a dot, as well as anything
// that follows it. This turns strings like "192.0.2.1:25" and "192.168.1.6/24" into
// parsable IPv4 addresses.
if i := strings.IndexFunc(s, func(r rune) bool {
return !strings.ContainsRune("0123456789.", r)
}); i != -1 {
s = s[0:i]
}
if ipv4 := net.ParseIP(s).To4(); ipv4 != nil && !ipv4.IsLoopback() {
return toValue(ipv4.String())
}
defer conn.Close()

// If for some bizarre reason we do not get a UDPAddr back, return a default value rather than panicking.
addr, ok := conn.LocalAddr().(*net.UDPAddr)
if !ok {
return toValue("127.0.0.1")
}
return toValue("127.0.0.1")

return toValue(addr.IP.String())
}


func dnsDomainLevels(call otto.FunctionCall) otto.Value {
host := call.Argument(0).String()
return toValue(strings.Count(host, "."))
Expand Down
20 changes: 19 additions & 1 deletion transport.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"net"
"net/http"
"net/url"
"io"
)

// transport creates and manages the lifetime of a net.Conn. Between the time that a remote server
Expand Down Expand Up @@ -57,7 +58,24 @@ func (t *transport) RoundTrip(req *http.Request) (*http.Response, error) {
if err := req.Write(t.conn); err != nil {
return nil, err
}
return http.ReadResponse(t.reader, req)
var resp2 *http.Response
var err2 error

resp2,err2 = http.ReadResponse(t.reader, req)
Copy link
Owner

@samuong samuong May 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure it's correct to read another response here. This function gets called by ProxyHandler.proxyRequest(), and the body of the response gets closed later in that file. Since this is done in a defer statement, it might not happen until after the next request is sent, which I think is the bug that you're running into.

If that's the case, then I think #120 would fix it - any chance you can give that branch a try and let me know if it fixes your issue?

Copy link
Author

Choose a reason for hiding this comment

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

sure let me test it out and come back to you, most likely removing defer will also fix the issue

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds good, let's continue the discussion in that PR.


if (resp2 != nil) {
if resp2.StatusCode == http.StatusProxyAuthRequired { //if 407 extra http body is not used, discard it issue #118
//if http body is not read from stream here, it will get stuck in ioreader and cause a malformed HTTP response error i.e. <html<head>
//pop the stream reader
bodyBytes, _ := io.ReadAll(resp2.Body)
_ = bodyBytes //unused variable
}
}

return resp2,err2 //return the actual error


//return http.ReadResponse(t.reader, req)
}

func (t *transport) hijack() net.Conn {
Expand Down