Cross-origin URL redirection workaround

[jablko]

Browsers allow HTTP redirects for simple cross-origin requests but not for requests that require preflight.
Because they set the Range header, this includes all web seed requests (if they're cross-origin).
So currently, web seeds can be cross-origin or they can make redirects, but not both at the same time.
What do you think of the following workaround for this?

If a request fails, maybe it's because the web seed sent a redirect and the browser objected, so try a simple request.
Make a simple request to unwind any redirects and get the final URL. (I used a HEAD request for this.)
If the final URL doesn't match the original URL, retry our original request with the new URL.
I make a feeble attempt to check that this is a cross-origin request.
It catches all cross-origin cases, but matches a few same-origin cases too. I figure that's okay in the interest of simplicity, since the check is merely an optimization anyway.

[josephfrazier]

I spent little while learning about the browser issue here (it boils down to Range not being a "simple header"), and it sounds like you're pretty close to fixing it!

Other than the inline comments I made, another suggestion I have is that the window.fetch logic (both detection and usage) could be broken out into its own function, so there's less code nesting.

Note to self: you can reproduce the browser issue by running this in the console at https://example.com:

fetch('https://httpbin.org/redirect-to?url=https%3A%2F%2Fhttpbin.org%2F', {
  headers: {
    Range: 'bytes=0-'
  }
})

[josephfrazier]

Shouldn't there be a .catch() also?

[jablko]

Oops. Yes. Fixed. Thanks!

[josephfrazier]

I think res.ok should be checked as well:

            if (res.ok && res.url !== url) {

[jablko]

Good point. Fixed.

[josephfrazier]

Actually, it might be better to handle the error case with an early exit. So, lines 165-180 could be replaced by:

            if (!res.ok || res.url === url) {
              hasError = true
              return cb(err)
            }

            opts.url = res.url
            get.concat(opts, function (err, res, data) {
              if (hasError) return
              if (err) {
                hasError = true
                return cb(err)
              }
              onResponse(res, data)
            })

[jablko]

Done.

[josephfrazier]

I'm tempted to say this should be an early-exit as well (at line 162):

        if (typeof window === 'undefined' || typeof window.fetch !== 'function' || url.startsWith(window.location.origin + '/')) {
          hasError = true
          return cb(err)
        }

        // (the code at line 163)

[jablko]

Done.

[josephfrazier]

I came back to this, and I now think we actually can use simple-get here. browserify replaced http-browserify with stream-http (see the list of shims here), which does expose res.url (see jhiesey/stream-http#40).

Here's a proof of concept that you can browserify and run in the console at https://example.com:

var get = require('simple-get')

function getPreflightRedirectConcat(opts, cb) {
  return get.concat(opts, function (err, res, data) {
    if (!err || opts.url.startsWith(window.location.origin + '/')) {
      return cb(err, res, data)
    }

    return get.head(opts.url, function (errHead, resHead) {
      // If the HEAD didn't work or didn't redirect, return the original error
      if (errHead || resHead.url == opts.url) {
        return cb(err, res, data)
      }

      var opts2 = Object.assign({}, opts)
      opts2.url = resHead.url
      return get.concat(opts2, cb)
    })
  })
}

var opts = {
  url: 'https://httpbin.org/redirect-to?url=https%3A%2F%2Fhttpbin.org%2F',
  headers: {
    Range: 'bytes=0-'
  }
}
getPreflightRedirectConcat(opts, function (err, res, data) {
  if (err) {
    console.error(err)
  }
  else {
    console.log("final url: " + res.url)
    console.log(data.toString())
  }
  window.close()
})

[jablko]

Oh, excellent! I switched to using simple-get.

[jablko]

Thanks a lot for reviewing this!
I'm definitely not opposed to breaking this out into its own function, I just don't yet see how to do it in a way that improves clarity or simplicity.

[josephfrazier]

You're welcome! I think it looks pretty good now. It could still be broken into its own function (like the one I posted above), but that can happen later if needed.

We don't exactly have any tests for this part of the code, so can you confirm that it still fixes your use case?

[jablko]

Excellent! I checked manually and confirm it still resolves the issue.

[josephfrazier]

Merged, thanks!

[jablko]

Thank you!

[feross]

LGTM!

[lock]

This thread has been automatically locked because it has not had recent activity. To discuss futher, please open a new issue.