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

Redirect Opt-out via URL Hints #505

Merged
merged 6 commits into from
Jul 2, 2018
Merged

Redirect Opt-out via URL Hints #505

merged 6 commits into from
Jul 2, 2018

Conversation

lidel
Copy link
Member

@lidel lidel commented Jun 27, 2018

TL;DR: this PR adds support for x-ipfs-no-redirect x-ipfs-companion-no-redirect (#510 ) symbol as a way to disable gateway redirect for a single request, without disabling anything globally.

That is all. The rest is just some cleanup/refactoring without any functional changes.

Background

This PR adds an URL-based opt-out from global gateway redirect that website developers can rely on to ensure request goes to the specific HTTP gateway.

Potential consumers of this feature:

  • Public gateway preloaders (IPFS Companion already does this for uploads)
  • Health checks (eg. https://ipfs.github.io/public-gateway-checker)
  • Self-hosted clusters of IPFS gateways optimized for specific use case (eg. video streaming)

Details

Thoughts?

cc @lgierth @victorbjelkholm @kyledrake

- cleanup of existing opt-out rules
- detect `x-ipfs-no-redirect` in the URL and skip redirects
@lidel lidel added the kind/enhancement A net-new feature or improvement to an existing feature label Jun 27, 2018
@lidel lidel self-assigned this Jun 27, 2018
preload requests require explicit opt-out hint
as noted in #505
@victorb
Copy link
Member

victorb commented Jun 27, 2018

Shouldn't this be done as a header on the request instead of having to change the URL?

@lidel
Copy link
Member Author

lidel commented Jun 27, 2018

@victorbjelkholm that was my first thought, but the idea got hit by API limitations quite fast:

  • Gateway redirect happens in onBeforeRequest. This hook does not provide access to headers, but enables us to do redirects..

  • We can modify headers in onBeforeSendHeaders, but can't trigger redirect from there. 🙃

But now that I think about it, what we can do is to detect header in onBeforeSendHeaders, store requestId as a skip flag somewhere and then check if skip flag exists in onBeforeRequest.
Will look into this.

Unfortunately, onBeforeSendHeaders happens after onBeforeRequest, so we are unable to know if header is present when redirect decision is made in onBeforeRequest:

webrequest-flow

@olizilla
Copy link
Member

unless I'm missing something, I feel like if a dev wants to force a specific gateway then the urls should include that gateway. Is there a issue with /ipfs/<hash> style addresses being redirected to the global gateway when they shouldn't? Couldn't they just use https://my-rad-gateway/ipfs/<hash>

@lidel
Copy link
Member Author

lidel commented Jun 27, 2018

@olizilla right now we detect IPFS paths on any website and redirect them. This means browser extension opportunistically upgrade transport to local IPFS node everywhere IPFS paths are used.

The problem this PR solves is that there is no opt-out mechanism for rare cases such as public gateway preloads and bug workarounds.

@victorbjelkholm Even if opt-out via a header was doable, it would come at a price: fetch with additional header breaks CORS preflight request, because by default header is not whitelisted for cross-origin requests to gateways.

This means opt-out via HTTP Header would require additional setup step from gateway administrators, namely to include x-ipfs-no-redirect x-ipfs-companion-no-redirect in Access-Control-Allow-Headers, and we can't assume everyone will have a public gateway under own control or be able to work around CORS validation.

@olizilla
Copy link
Member

olizilla commented Jun 27, 2018

Should we avoid re-writing IPFS urls that explicitly refer to a custom gateway?
It feels like we're dropping some useful info if we do... they are trying to tell us which gateway to use.

/ipfs/<hash> -> redirect to local ipfs gateway
https://ipfs.io/ipfs/<hash> -> redirect to local ipfs gateway
https://dweb.video/ipfs/<hash> -> leave it alone.

@lidel
Copy link
Member Author

lidel commented Jun 27, 2018

Ok, time for some context:
What our browser extension does since 2017 is a global, opportunistic protocol upgrade that is enabled by default. It detects /ipfs/<CID> on any website and redirects it to local HTTP2IPFS gateway. This is enabled by default for every webpage.

The gateway at ipfs.io is not special: the same rules apply to all public gateways.
As long your assets are under a path that starts with valid /ipfs/<CID>/* it will be loaded over IPFS.

What this PR changes is to add optional "opt-out" mechanism: extension looks at URL of requests made to any public gateway and if it sees x-ipfs-no-redirect x-ipfs-companion-no-redirect in will leave request as-is, won't redirect anything. That is the only change proposed here: an explicit opt-out mechanism for website and dapp developers to handle edge cases when they want to be sure request goes to the specific gateway.

ps. Historical discussion about opportunistic HTTP2IPFS upgrade on every website can be found in #16.
I strongly think it should remain "opt-out" (enabled by default) and not "opt-in" due to the tyranny of the default.

These parts had a lot of scar tissue, this commit cleans things up
and makes codepaths responsible for request mutation easier to grasp.
@lidel
Copy link
Member Author

lidel commented Jun 28, 2018

I think I was just not clear enough in my initial description what this PR does, sorry about that.

The change is really, really small:

TL;DR: this PR adds support for x-ipfs-no-redirect x-ipfs-companion-no-redirect (#510) symbol as a way to disable gateway redirect for a single request ad-hoc, without disabling anything globally.

https://ipfs.io/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR#x-ipfs-companion-no-redirect
function isSafeToRedirect (request, runtime) {
  // Do not redirect if URL includes opt-out hint
  if (request.url.includes('x-ipfs-companion-no-redirect')) {
   return false
 }

That is all. The rest is just some cleanup/refactoring without any functional changes.

Copy link
Member Author

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Apart from the cleanup, actual change introduced by this PR is 4 lines long.
Let's release it to Beta.

@lidel lidel merged commit 9f9f898 into master Jul 2, 2018
@lidel lidel deleted the feat/redirect-opt-out branch July 2, 2018 08:37
@olizilla
Copy link
Member

olizilla commented Jul 2, 2018

I don't feel totally comfortable about supporting this. If it's just for companion, perhaps the token to check for should be x-ipfs-companion-no-redirect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A net-new feature or improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants