Possibility to masquerade as another user when uploading a file!

[macman31]

Hey @jalik,

While trying to understand your library to make sure that I use it best for my use case, I found this line in the code: https://github.com/jalik/jalik-ufs/blob/master/ufs-methods.js#L99 file.userId = file.userId || this.userId;

Why are you allowing the file.userId to be set by the client??? It allows anyone to upload a file that will appear to have been upladed by somone else.

The issue is easily reproducible using your demo project: just set file.userId = "<existing user ID>" right before let uploader = new UploadFS.Uploader({ (in /imports/client/templates/templates.js, that's client-side code!), and try to upload a file while not being logged in.

The upload will cause "Forbidden" errors on the server, it will never execute onFinishUpload, but the upload will succeed, it will execute transformWrite and the file will be present in the DB and visible if you log in as the user with the user ID you entered!

I suggest changing the line to file.userId = this.userId;.

Thanks for coding this library, btw!

[jalik]

Hi @macman31,

I understand your preoccupation about security, but I've coded this package with security and flexibility in mind.

So you start with almost nothing to do, just a little of code and it works, but since you want to secure your application with roles, permissions etc which is not the job of UFS, you have to do it yourself using your own code/package, this is why I allow a user to be shipped in the file, for example if someone wants to create an app where a user (like an admin) is allowed to upload a file for someone else, it is possible, so I don't do that for you because I don't know how you want to implement your security layer.

I've updated the UFS Demo, update the code via git to see the changes. In your case, what you need to do is defining store permissions like below. The file.userId must be empty or equals to userId, otherwise it's forbidden, but you can put whatever condition you want there.

export const FileStore = new UploadFS.store.Local({
    collection: Files,
    name: 'files',
    path: './uploads/files',
    filter: FileFilter,
    // Overwrite default permissions
    permissions: new UploadFS.StorePermissions({
        insert(userId, file) {
            console.log(userId, file.userId);
            // allow anyone to upload a file, but check that uploaded file is attached to the user that uploads the file
            return !file.userId || file.userId === userId;
        },
        remove(userId, file) {
            // allow anyone to remove public files, but only owners to remove their files
            return !file.userId || userId === file.userId;
        },
        update(userId, file) {
            // allow anyone to update public files, but only owners to update their files
            return !file.userId || userId === file.userId;
        },
    })
});

[macman31]

@jalik I can see the use case for specifying the userId in the upload now! Thank you for the update on the UFS demo, it makes it clear for me how UFS works!

[jalik]

I am glad that helped you. Happy coding.