search

feross / simple-get

Simplest way to make http get requests. Supports HTTPS, redirects, gzip/deflate, streams in < 100 lines
MIT License
401 stars 50 forks source link

Max redirects bug?! #29

Closed VoidMonk closed 7 years ago

VoidMonk commented 7 years ago

Hi, this is a neat little library.

Playing around I noticed a possible bug with maxRedirects. If its value is set to say 5 then up-to five redirects should be allowed after the main request, but it falls short by 1 (allowing only 4 redirects), due to opts.maxRedirects being decremented before the actual check in the code at https://github.com/feross/simple-get/blob/master/index.js#L47. I suppose it should rather be:

if (opts.maxRedirects > 0) {
    opts.maxRedirects -= 1
    simpleGet(opts, cb)
}

It can be tested with this httpbin endpoint: https://httpbin.org/redirect/5

Also, I think the extra parseOptsUrl(opts) within the redirection handler code at https://github.com/feross/simple-get/blob/master/index.js#L44 isn't necessary, as it will be called at https://github.com/feross/simple-get/blob/master/index.js#L15 anyways on the next request.

feross commented 7 years ago

You are correct! This is fixed in 2.5.0.

feross commented 7 years ago

I removed the extra parseOptsUrl() call in 2.5.1.