-
Notifications
You must be signed in to change notification settings - Fork 325
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
feat: scope mfs calls to an application path #431
Conversation
This looks great, I love the video. Can we special case the https / http level to remove the colon before url-encoding? We're going to end up presenting these to the user in the new UIs and having the url-encoded version is gonna look surprising. |
} | ||
|
||
// Turn http://ipfs.io/ipfs/QmUmaEnH1uMmvckMZbh3yShaasvELPW4ZLPWnB4entMTEn | ||
// into /http%3A/ipfs.io/ipfs/QmUmaEnH1uMmvckMZbh3yShaasvELPW4ZLPWnB4entMTEn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with #431 (comment), we should add .replace(':\/\/','/')
at the beginning of the scopeToPath
pipeline to change first (and only first) occurrence of ://
into /
|
||
module.exports = createPreMfsScope | ||
|
||
const MfsPre = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question; how will it behave when a new API ipfs.files.foo
is added ?
Will it not work, or will it work with no path scoping?
If the latter is the case, we should explicitly block executions of new methods in scoped ipfs.files
API to avoid surprises in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I mean, you'd need to update the ipfs-postmsg-proxy
dependency to allow the new method to be used in the first place, but no it wouldn't be scoped automatically unless you did some work here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, being dependent on ipfs-postmsg-proxy
makes this safe enough for now.
Unless its trivial to add, I propose we add additional safeguards in separate PR.
95b9402
to
53d3326
Compare
@lidel good now? |
@alanshaw will we have a special case for WebUI? WebUI should be able to access everything. How will we do that? |
@hacdias yes, if accessed through companion we can privilege the If you're just loading (our new, yet to be built) WebUI from the gateway in the browser with ipfs companion installed (i.e. https://ipfs.io/ipfs/QmPhnvn747LqwPYMJmQVorMaGbMSgA7mRRoyyZYz3DoZRQ) then MFS stuff will be scoped. You might want to load from /ipns/webui.ipfs.io to get around this, and also the problem of different versions of the app being scoped to different folders. Check out this comment (and the backstory) for a bit more context: #330 (comment) |
@alanshaw gotcha. It makes sense. I like the |
https://youtu.be/KvgxAafphPg
See #330 (comment) for discussion.
To avoid conflicts, each app gets it's own MFS directory where it can store files. When using MFS functions (see docs) this directory will be automatically added to paths you pass. Your app's MFS directory is based on the origin and path where your application is running.
e.g.
files.write
to/myfile.txt
onhttps://domain.com/
/dapps/https/domain.com/myfile.txt
files.write
to/path/to/myfile.txt
onhttps://domain.com/feature
/dapps/https/domain.com/feature/path/to/myfile.txt
files.read
from/feature/path/to/myfile.txt
onhttps://domain.com/
/dapps/https/domain.com/feature/path/to/myfile.txt
files.stat
to/
onhttps://domain.com/feature
/dapps/https/domain.com/feature
files.read
from/../myfile.txt
onhttps://domain.com/feature
/dapps/https/domain.com/feature/myfile.txt
(no traverse above your app's root)