[go: nahoru, domu]

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

support for fs: URIs without redirecting to http: #70

Merged
merged 6 commits into from
Feb 14, 2016

Conversation

the8472
Copy link
Contributor
@the8472 the8472 commented Feb 11, 2016

It works with and without e10s for basic resource loading and I also tested it against the case in #3. But it most likely needs some further testing to weed out edge-cases, so i've marked it as default-off and experimental.

Basically, from firefox's view this all runs under CORS policy because we're running the HTTP requests with the principals of the fs:/ document instead of implementing our own channels which then delegate to a privileged HTTP channel
Getting the URLs a tiny bit wrong causes redirects from the gateways which then can then void the CORS permissions under circumstances.

Implementing as a custom channel wrapper around http channels might be more robust wrt. CORS but currently is way too troublesome under e10s until the FF devs improve support (or documentation) there.

@the8472
Copy link
Contributor Author
the8472 commented Feb 11, 2016

Idk why the tests fail on travis. Running jpm test passes for me.

@lidel
Copy link
Member
lidel commented Feb 11, 2016

Thank you! I'll do my best to review and merge it this weekend.

For now I did a quick check and tests pass for me too.

It is probably a long shot, but maybe nodejs at Travis is too old?
Try updating .travis.yml in a commit to this branch (4.1 seems to be the max):

node_js:
-    - "0.12"
+    - "4.1"

and let's see if it helps.

@the8472
Copy link
Contributor Author
the8472 commented Feb 11, 2016

added that.

Another thing of note, to make jpm happy locally I had to change the .jpmignore file. Does it work as it is in the git tree for you?

url: testpage,
onReady: (tab) => {

// first load somehow doesn't have protocol handlers registered. so load resource:// first, then redirect to fs:/ page
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is the same bug as in #33 (comment)
We probably need to do this workaround for now, hopefully it will be fixed in WebExtensions.

@lidel
Copy link
Member
lidel commented Feb 11, 2016
  • jpmignore works fine (jpm 1.0.5) for me -- I've built .xpi and contents included only whitelisted files

  • Travis still fails, so it is not nodejs. Perhaps Travis is slower than our machines, and asynchronous callbacks that are fired after prefs change collide with URI tests from test/test-protocols.js.. or we have some race condition and under slow runner prefs.fsUris is sometimes left as true? No idea.

    This seems to confirm that fs: is still present in address bar, while it is expected to be changed into http link.

    Perhaps you could make these tests independent of the default value, and explicitly disable fsUris inside of each one (and perhaps add another set for prefs.fsUris = true):

    + const { prefs } = require('sdk/simple-prefs')
    
    exports['test newURI(fs:ipfs/<path>)'] = function (assert) {
    +  prefs.fsUris = false
     var newURI = fsHandler.newURI('fs:ipfs/' + ipfsPath, 'utf8', null)
    assert.equal(newURI.spec, pubGwUri.spec + 'ipfs/' + ipfsPath, 'newURI spec')
    }

    Sadly I don't have any better idea (feel free to add as many commits as you need).

@lidel lidel added kind/enhancement A net-new feature or improvement to an existing feature UX labels Feb 11, 2016
@the8472
Copy link
Contributor Author
the8472 commented Feb 11, 2016

running tests on Firefox 44.0.1/Gecko 44.0.1

That seems to be latest stable. Is there no nightly?

@lidel
Copy link
Member
lidel commented Feb 11, 2016

No nightly. Currently tests are run against latest stable, beta, and every .0 release since 43.0:
https://travis-ci.org/lidel/ipfs-firefox-addon/builds/108628692

@the8472
Copy link
Contributor Author
the8472 commented Feb 12, 2016

Well, tests pass for me now. I blame whatever doesn't work on travis

@lidel
Copy link
Member
lidel commented Feb 13, 2016

I did some manual tests locally under 44, 45 and 47a1.

Indeed super weird: automated tests pass for me too, but when I tried to actually use it (enabled fsUris on the Preferences screen) it does not seem to work: i am always redirected to http://127.0.0.1:8080/ipfs/(...).

Interestingly, it seems to work fine if redirect is disabled AND gateway is offline:
I see fs: instead of http://ipfs.io/ipfs/(..)

Could you please check if fs:/ipfs/QmYHNYAaYK5hm3ZhZFx5W9H6xydKDGimjdgJMrMSdnctEm works for you when redirect is enabled and gateway is online?

@the8472
Copy link
Contributor Author
the8472 commented Feb 13, 2016

It displays the fs:/ uri for me in nightly and loads the cat. Works both with local and ipfs.io gateway.

I have had some issues with jpm/npm modules though, maybe you're experiencing the same? Clearing it the generated folders and doing an npm update solved them for me.
Or maybe you're running into the first load issue again?

@lidel
Copy link
Member
lidel commented Feb 13, 2016

Removed node_modules and installed everything again via npm install.

My versions:

node 5.6.0
npm 3.6.0
jpm 1.0.5

I run local version via node_modules/jpm/bin/jpm -b $FIREFOX_BIN run
Still have the same problem: fs: is replaced with http: when redirect is enabled.

Are you running clear profile without any auto-reloading addons?

@the8472
Copy link
Contributor Author
the8472 commented Feb 13, 2016

node v5.5.0
npm 3.7.1
jpm 1.0.5

I have a dev profile which I use together with jpm post + extension auto installer.

@lidel
Copy link
Member
lidel commented Feb 13, 2016

Just to be sure it is something on my end,
please try without dev profile and other addons, just dry run with jpm -b $FIREFOX_BIN run.

@the8472
Copy link
Contributor Author
the8472 commented Feb 13, 2016

Ok, yeah, now I'm seeing it too sigh

Will fix.

@the8472
Copy link
Contributor Author
the8472 commented Feb 14, 2016

should work now.

lidel added a commit that referenced this pull request Feb 14, 2016
support for fs: URIs without redirecting to http:

- disabled by default
- implements #48
@lidel lidel merged commit 0640e96 into ipfs:master Feb 14, 2016
@lidel
Copy link
Member
lidel commented Feb 14, 2016

Merged – thank you!

Let's follow this in related issues:

@lidel lidel mentioned this pull request Feb 14, 2016
catman.addCategoryEntry(in string aCategory, in string aEntry, in string aValue, in boolean aPersist, in boolean aReplace)
*/

function fixupRequest (e) {
Copy link
Member

Choose a reason for hiding this comment

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

@the8472 I am going over the backlog and trying to have a better understanding of this part of code. I see this is related to protocols.js#L121-L131 but can't infer full picture from code alone.

Could you elaborate what is the purpose of this fixup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely sure if it's still necessary.

There were some cases with redirects coming from the server which needed the LOAD_REPLACE flag to be removed. If it is not removed the new channel's URI overrides originalURI which leads to a http URI in the address bar.

Copy link
Member

Choose a reason for hiding this comment

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

While implementing #72 I did some cleanup in c75a05d.

I did not notice any regressions, but would appreciate if you took a second look, perhaps I missed something.

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.

None yet

2 participants