search

jcoglan / restore

Simple remoteStorage server written in Node.js
293 stars 27 forks source link

invalid checks to username, and isValidUsername updated to test for unde... #27

Closed silverbucket closed 9 years ago

silverbucket commented 9 years ago

...fined. test added

jcoglan commented 9 years ago

I'm not sure of the purpose of the change to lib/stores/core.js here. Rejecting path traversal should be done (and it is done) in the web protocol layer, not the storage backend, unless you also plan to expose the backend via other untrusted means.

You're already blocked from obtaining authorisation and accessing files on the storage for the user .. in lib/restore.js, which will block the paths /oauth/.. and /storage/../files.

What attack vector have I missed that requires protection applied on the backend?

silverbucket commented 9 years ago

Hi @jcoglan, good to see you back.

The path traversal check ('..') was already there, so I was just adding the undefined check to fix the bug and was leaving the other check intact as I figured you had it there for a reason. Do you think we should just have the (!username) check and that's it?

jcoglan commented 9 years ago

Of course, sorry, I misread the diff.

I initially assumed that Regexp.test() would return false for all falsey values, but /[a-z][a-z]+/.test(undefined) is true :(

Thanks for the patch.