Use http.Transport instead of http.Client.

[derekkraan]

http.Client follows redirects by default and has support for cookies.
http.Transport only performs one round-trip. Using http.Client is
vulnerable to bad behaviour if there's a redirect loop somewhere in your
app. Since we don't need (or want) redirects, and don't use cookies,
http.Transport seems like the most logical choice.

(Before this patch, we ended up DOSing our own site whenever someone discovered a redirect loop)

[buger]

Thanks. I'm not sure that disabling cookies and redirects is best solution, because Gor aims to replicate exact same requests. If you have redirect loop in your production environment i guess its a bug and should be fixed, right?

But it make sense try to limit number for redirects, not sure if standard Go lib support it.

[derekkraan]

The default behaviour (which gor does not override) of http.Client is to not send cookies with requests and ignore them in responses. The default behaviour is also to follow redirects up to 10 times per request. So the only effective change of this pull request is to not follow redirects, which I believe is correct behaviour for what gor is attempting to do.

Yep, redirect loops are a bug, but not necessarily a bug that should bring our entire stack to its knees. :P

[buger]

Are you sure that it not send cookies? Because request object should contains info about cookies.

I suggest just to add CustomRedirect function like this:

func customCheckRedirect(req *Request, via []*Request) error {
    if len(via) >= 2 {
        return errors.New("stopped after 2 redirects")
    }
    return nil
}

...

client := &http.Client{
    CheckRedirect: customCheckRedirect,
}

[derekkraan]

I'm not an expert (I have only ever written a few lines of go), but if I read the documentation I find this:

type Client struct {
    // Transport specifies the mechanism by which individual
    // HTTP requests are made.
    // If nil, DefaultTransport is used.
    Transport RoundTripper

    // CheckRedirect specifies the policy for handling redirects.
    // If CheckRedirect is not nil, the client calls it before
    // following an HTTP redirect. The arguments req and via are
    // the upcoming request and the requests made already, oldest
    // first. If CheckRedirect returns an error, the Client's Get
    // method returns both the previous Response and
    // CheckRedirect's error (wrapped in a url.Error) instead of
    // issuing the Request req.
    //
    // If CheckRedirect is nil, the Client uses its default policy,
    // which is to stop after 10 consecutive requests.
    CheckRedirect func(req *Request, via []*Request) error

    // Jar specifies the cookie jar.
    // If Jar is nil, cookies are not sent in requests and ignored
    // in responses.
    Jar CookieJar
}

And right now we don't supply a Jar when we create Client. That's the basis on which I've concluded that there are no cookies being sent.

[buger]

@derekkraan I limit redirects by 2, also added integration tests to ensure that cookies is transferred https://github.com/buger/gor/blob/master/integration_test.go

[derekkraan]

I believe the correct behaviour is to follow 0 redirects. If the user's browser receives a redirect, it'll follow that redirect and gor will pick that up, still perfectly duplicating the load on the production stack on the staging stack. But by following redirects up to two times:

• every redirect will be followed once on the production stack, and twice on the staging stack
• redirect loops will still explode and cause a DOS of your (or in any case, our) app

[buger]

@derekkraan i finally got you idea, you right. I disabled redirects.