Repository navigation
Fix URI encoding in file listing and decoding to get file names - #471
Conversation
|
|
||
| function uriToFilename (uri, base) { | ||
| var filename = path.join(base, uri) | ||
| var decoded = uri.split('/').map(decodeURIComponent).join('/') |
There was a problem hiding this comment.
I think we're going to run into problems with this approach. What if the file (or container) was uploaded as already uri-encoded? Say, for example, it was uploaded as hello%20world.ttl? And exists on disk as hello%20world.ttl. If you then issue a GET on it and this method uriToFilename() is invoked, it will try and look for the filename hello world.ttl and not find it.
There was a problem hiding this comment.
This is especially relevant given the recent PR #469, which url-encodes container names.
There was a problem hiding this comment.
When a file is uploaded, any filename is URI-encoded to form the URI.
If the file "hello world.ttl" exists then the resource "hello%20world.ttl" exists already too.
If there is a file on disk called "hello%20world.ttl" then there is corresponding LDP resource "hello%2520world.ttl"
There was a problem hiding this comment.
Reversible mapping. The file world is self-consistent. The URL world is self-consistent.
There was a problem hiding this comment.
In thinking about this, I realize we still have an open issue where we don't URI-encode file names on PUT. (Only container names). So it's still possible for people to try and create files with names that are illegal as far as the OS/filesystem is concerned.
If we fix this issue (and enforce URI-encoding of filenames on PUT and POST), then there is no need to uri-encode them on listing.
There was a problem hiding this comment.
A goal of this project is to be able to export existing files system users have. This means hex encoding things like space which are not allowed in URLs. A Windows or Mac Documents or Music folder should be be able to export into Solid in a useful way.
It is NOT a goal to be able to allow clients to put files with characters which are typically illegal in a file system, like say colon, slash, or vertical bar. I think we could return an error when a PUT or PATCH tries to do that (by hex encoding them in a URL). I don't see any value in allowing this characters.
There was a problem hiding this comment.
A goal of this project is to be able to export existing files system users have.
How would this export process work? The existing files would arrive on a Solid server via PUTs and POSTs, right? Which means their filenames would be safely encoded.
It is NOT a goal to be able to allow clients to put files with characters which are typically illegal in a file system
What would you like to see as the behavior when users/developers inevitably try to PUT or POST those kind of file names (illegal in file system)?
There was a problem hiding this comment.
Maybe add * ? \
?
Or wait until the file system objects?
Example use case: I want to copy my entire Mac home folder into a solid server to sync it with other macs
|
In Mac, not much is illegal [] {} | all turn up as Chrome when you print off a page as PDF uses the windows title as a filename with very little adaptation! Should we be conservative to prevent badness down the line in file systems apps (like git, cross system backup, etc) or should web liberal so that in soemone'd complicated machup of different systems it does not mean that some in fact unnecessary constraint in node-solid-server end up breaking things which would otherwise work? |
|
I strongly believe we should be conservative, as far as accepting file name inputs via PUT, POST and PATCH, and URI-encode them. (to prevent all sorts of downstream problems, both with filesystem and with URIs). |
| }) | ||
| }) | ||
| describe('listContainer', function () { | ||
| /* |
There was a problem hiding this comment.
Do you mind deleting the test, instead of commenting out?
| }) | ||
| }) | ||
|
|
||
| /* No, URLs are NOT ex-encoded to make filenames -- the other way around. |
There was a problem hiding this comment.
This test should be changed to test for a 400 error response, per our conversation.
dmitrizagidulin
left a comment
dmitrizagidulin
left a comment
There was a problem hiding this comment.
Had further conversations about this PR with Tim, I'm on board with this direction. Merging.
…improve debug a little
… is listed, taking ages. ferossified
c7457d8 to
134ed40
Compare
Also improve debug a little: truncate massive directory listings in the debug, add filename not found when 404 not found.
Designed to address #470