Skip to content

Fix URI encoding in file listing and decoding to get file names #471

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.

Sign up for GitHub

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

Merged
dmitrizagidulin merged 3 commits into from
Jan 3, 2017
Merged

Fix URI encoding in file listing and decoding to get file names #471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
020b5d2
Fix URI encoding in file listing and decoding to get file names, and …
Dec 10, 2016
3e0c814
Fix bug causing .ttl files in a directory to be parsed when directory…
Dec 14, 2016
134ed40
Fix url decodeing of POST slug, add test for bad chacaretrs, add test…
Dec 14, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions lib/acl-checker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,9 @@ class ACLChecker {
debug(`${mode} access permitted to ${user}`)
return callback()
} else {
debug(`${mode} access not permitted to ${user}`)
return callback(new Error('Acl found but policy not found'))
debug(`${mode} access NOT permitted to ${user}` +
aclOptions.strictOrigin ? ` and origin ${options.origin}` : '')
return callback(new Error('ACL file found but no matching policy found'))
}
})
.catch(err => {
Expand Down
10 changes: 5 additions & 5 deletions lib/ldp-container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ var error = require('./http-error')
var fs = require('fs')
var ns = require('solid-namespace')($rdf)
var S = require('string')
var turtleExtension = '.ttl'
// var turtleExtension = '.ttl'
var mime = require('mime-types')

function addContainerStats (ldp, reqUri, filename, resourceGraph, next) {
Expand Down Expand Up @@ -58,9 +58,9 @@ function addFile (ldp, resourceGraph, containerUri, reqUri, uri, container, file
resourceGraph.sym(memberUri))

// Set up a metaFile path
var metaFile = container + file +
(stats.isDirectory() ? '/' : '') +
(S(file).endsWith(turtleExtension) ? '' : ldp.suffixMeta)
// Earlier code used a .ttl file as its own meta file, which
// caused massive data files to parsed as part of deirectory listings just looking for type triples
var metaFile = container + file + ldp.suffixMeta

getMetadataGraph(ldp, metaFile, memberUri, function (err, metadataGraph) {
if (err) {
Expand Down Expand Up @@ -147,7 +147,7 @@ function readdir (filename, callback) {
return callback(error(err, 'Can\'t read container'))
}

debug.handlers('Files in directory: ' + files)
debug.handlers('Files in directory: ' + files.toString().slice(0, 100))
return callback(null, files)
})
}
Expand Down
10 changes: 7 additions & 3 deletions lib/ldp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ class LDP {
async.each(
files,
function (file, cb) {
let fileUri = reqUri + file
let fileUri = reqUri + encodeURIComponent(file)
ldpContainer.addFile(ldp, resourceGraph, reqUri, fileUri, uri,
filename, file, cb)
},
Expand Down Expand Up @@ -185,7 +185,11 @@ class LDP {
debug.handlers('POST -- On parent: ' + containerPath)
// prepare slug
if (slug) {
slug = encodeURIComponent(slug)
slug = decodeURIComponent(slug)
if (slug.match(/\/|\||:/)) {
callback(error(400, 'The name of new file POSTed may not contain : | or /'))
return
}
}
// TODO: possibly package this in ldp.post
ldp.getAvailablePath(hostname, containerPath, slug, function (resourcePath) {
Expand Down Expand Up @@ -293,7 +297,7 @@ class LDP {
ldp.stat(filename, function (err, stats) {
// File does not exist
if (err) {
return callback(error(err, "Can't find resource requested"))
return callback(error(err, 'Can\'t find file requested: ' + filename))
}

// Just return, since resource exists
Expand Down
3 changes: 2 additions & 1 deletion lib/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ function debrack (s) {
}

function uriToFilename (uri, base) {
var filename = path.join(base, uri)
var decoded = uri.split('/').map(decodeURIComponent).join('/')

@dmitrizagidulin dmitrizagidulin Dec 10, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@dmitrizagidulin dmitrizagidulin Dec 10, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is especially relevant given the recent PR #469, which url-encodes container names.

@timbl timbl Dec 10, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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"

@timbl timbl Dec 10, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reversible mapping. The file world is self-consistent. The URL world is self-consistent.

@dmitrizagidulin dmitrizagidulin Dec 10, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@timbl timbl Dec 14, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@dmitrizagidulin dmitrizagidulin Dec 14, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)?

@timbl timbl Dec 14, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

var filename = path.join(base, decoded)
// Make sure filename ends with '/' if filename exists and is a directory.
// TODO this sync operation can be avoided and can be left
// to do, to other components, see `ldp.get`
Expand Down
24 changes: 24 additions & 0 deletions test/http.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,29 @@ describe('HTTP APIs', function () {
.expect(200, done)
})

it('should create a container with a name hex decoded from the slug', (done) => {
let containerName = 'Film%4011'
let expectedDirName = '/post-tests/Film@11/'
server.post('/post-tests/')
.set('slug', containerName)
.set('content-type', 'text/turtle')
.set('link', '<http://www.w3.org/ns/ldp#BasicContainer>; rel="type"')
.expect(201)
.end((err, res) => {
if (err) return done(err)
try {
assert.equal(res.headers.location, expectedDirName,
'Uri container names should be encoded')
let createdDir = fs.statSync(path.join(__dirname, 'resources', expectedDirName))
assert(createdDir.isDirectory(), 'Container should have been created')
} catch (err) {
return done(err)
}
done()
})
})

/* No, URLs are NOT ex-encoded to make filenames -- the other way around.

@dmitrizagidulin dmitrizagidulin Dec 14, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This test should be changed to test for a 400 error response, per our conversation.

it('should create a container with a url name', (done) => {
let containerName = 'https://example.com/page'
let expectedDirName = '/post-tests/https%3A%2F%2Fexample.com%2Fpage/'
Expand Down Expand Up @@ -574,6 +597,7 @@ describe('HTTP APIs', function () {
.expect('content-type', /text\/turtle/)
.expect(200, done)
})
*/

after(function () {
// Clean up after POST API tests
Expand Down
3 changes: 2 additions & 1 deletion test/ldp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ describe('LDP', function () {
})
})
describe('listContainer', function () {
/*

@dmitrizagidulin dmitrizagidulin Dec 14, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you mind deleting the test, instead of commenting out?

it('should inherit type if file is .ttl', function (done) {
write('@prefix dcterms: <http://purl.org/dc/terms/>.' +
'@prefix o: <http://example.org/ontology>.' +
Expand Down Expand Up @@ -145,7 +146,7 @@ describe('LDP', function () {
done()
})
})

*/
it('should not inherit type of BasicContainer/Container if type is File', function (done) {
write('@prefix dcterms: <http://purl.org/dc/terms/>.' +
'@prefix o: <http://example.org/ontology>.' +
Expand Down
12 changes: 10 additions & 2 deletions test/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,16 @@ describe('Utility functions', function () {
it('should return empty as relative path for undefined path', function () {
assert.equal(utils.pathBasename(undefined), '')
})
it('should not decode uris', function () {
assert.equal(utils.uriToFilename('uri%20', 'base/'), 'base/uri%20')
})
describe('uriToFilename', function () {
it('should decode hex-encoded space', function () {
assert.equal(utils.uriToFilename('uri%20', 'base/'), 'base/uri ')
})
it('should decode hex-encoded at sign', function () {
assert.equal(utils.uriToFilename('film%4011', 'base/'), 'base/film@11')
})
it('should decode hex-encoded single quote', function () {
assert.equal(utils.uriToFilename('quote%27', 'base/'), 'base/quote\'')
})
})
})