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

Do not redirect XHR that would fail due to CORS bug in Firefox #494

Merged
merged 3 commits into from
Jun 5, 2018

Conversation

lidel
Copy link
Member

@lidel lidel commented Jun 4, 2018

This PR disables gateway redirect for cross-origin XHRs.

  • It fixes all the websites broken by CORS false-positive bug in Firefox (samples below).
    • It is a "lesser evil". There will be no gateway redirect for such requests, but at least "IPFS-enabled" websites that do cross-origin requests to different gateways will work.
  • I believe we want the same behavior on all browsers, so initial version of this PR conforms to the Firefox limitation in Chrome as well.
    • For: Different handling on different browsers would be a nightmare for a developer, who assumes it "works the same everywhere".
    • Against: We introduce artificial limitation for browsers other than Firefox.
  • The full context is in gateway redirect messes up CORS headers on firefox #436 (comment) including a link to the upstream bug.

Samples

Examples of sites broken in v2.3.0 by the bug:

Open Questions

I'd appreciate some quick feedback, before this gets merged and released:

  1. Is it OK to artificially limit Chrome just for sake of uniformity?
  2. Should we log a warning to the console on every skipped XHR, or is it too much?

@lidel lidel requested review from olizilla and alanshaw June 4, 2018 21:14
@lidel lidel added this to the v2.4.0 milestone Jun 4, 2018
HTTP HEAD is often used for preloading data at arbitrary gateways.
It should arrive at original destination, no matter what hostname is
used.
@fahrradflucht
Copy link

Since this is a severe limitation on what single page applications on ipfs will be able to do in for example an offline situation I think logging a warning every time this happens is a good idea. I think both firefox and chrome provide ways to silence those logs if the are expected.

@alanshaw
Copy link
Member

alanshaw commented Jun 5, 2018

If we use the isFirefox runtime check and issue a warning (only for Firefox) we wouldn't have to restrict for Chrome (and potentially others). It wouldn't add a great deal of complexity and there's precedent for that sort of thing already in the code base.

In combination with the log line, the altered behaviour in Firefox remains visible and the restriction can be removed easily when the issue is fixed. Meanwhile by leaving in the functionality for Chrome you ensure the code that is doing it continues to work as expected.

@lidel
Copy link
Member Author

lidel commented Jun 5, 2018

@alanshaw I initially did that, but got worried about providing different redirect rules for different vendors, which feels like a bad developer experience.

So your suggestion is to limit this workaround to Firefox only and and be very explicit that it is a temporary workaround for an upstream bug limited to Firefox?

This implements suggestions from:
#494 (comment)

Wider context:
#494
@lidel
Copy link
Member Author

lidel commented Jun 5, 2018

Alright, switched this PR to apply the workaround only when in Firefox runtime.
I feel this makes it very safe to merge and release to beta, as it restores broken functionality for Firefox users and does not change anything for Chrome.

@lidel lidel merged commit d1d715e into master Jun 5, 2018
@lidel lidel deleted the fix/firefox-cors-bug branch June 5, 2018 12:27
lidel added a commit that referenced this pull request Jun 8, 2018
Fix delivered to Beta in #494 looks stable enough to be promoted to Stable

This PR:
- synchronizes locales with [crowdin](https://crowdin.com/project/ipfs-companion)
- bumps version in preparation to stable release
@lidel
Copy link
Member Author

lidel commented Jun 8, 2018

For anyone interested in this fix, it just got shipped to stable channel as v2.3.1

lidel added a commit that referenced this pull request Sep 30, 2019
This change removes woraround introduced in PR #494
and restores redirect in onBeforeRequest.

The original bug was fixed in Firefox 69,
that is why we also bump minimal version.

More info at:
#436 (comment)
lidel added a commit that referenced this pull request Sep 30, 2019
This change removes woraround introduced in PR #494
and restores redirect in onBeforeRequest.

The original bug was fixed in Firefox 69,
that is why we also bump minimal version.

More info at:
#436 (comment)
lidel added a commit that referenced this pull request Oct 1, 2019
This change removes workaround introduced in PR #494
and restores redirect in onBeforeRequest.

The original bug was fixed in Firefox 69,
that is why we also bump minimal version.

More info at:
#436 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants