clientManager: don't write user configs outside of lounge's users dir #238

williamboman · 2016-04-02T21:28:01Z

No description provided.

xPaw · 2016-04-02T21:37:39Z

package.json

@@ -35,6 +35,7 @@
    "lodash": "4.6.1",
    "mkdirp": "0.5.1",
    "moment": "2.12.0",
+    "path-is-inside": "1.0.1",


Let's not bring in modules for stuff that can be done in one line.

I'm thinking we should only be allowing a-zA-Z0-9_- in usernames.

Not really a one-liner though :). A custom one-liner is also far from as robust and well-tested (across numerous platforms) as a package too. Unix philosophy + Node ftw!

I'm thinking we should only be allowing a-zA-Z0-9_- in usernames.

I thought so as well, but didn't want to make such a decision in this PR.

A custom one-liner is also far from as robust and well-tested (across numerous platforms) as a package too. Unix philosophy + Node ftw!

/me looks at left-pad in panic.

Haha, that's just the shortcomings of npm though. They've retroactively done some damage control by disabling unpublish on packages that are >24 hrs old.

Do you want me to change to testing the name with /^[a-z0-9_-]+$/i instead?

I think ideally we should simply disallow what is not valid characters for the filesystem. /^[^\/\\<>:"|?*]+$/. This should cover at least Linux and Windows, and probably Mac as well. I think Windows is alone having such restricted file names.

Source: https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx

We can use path.basename to fix writing outside.

We can use path.basename to fix writing outside.

Do you mean that ../../MZuckerberg silently becomes MZuckerberg, or should it do it interactively, like;

if (path.basename(user) !== user) { // interactively ask if path.basename(user) should be used instead - else error }

I'd just error out.

Updated patch now

xPaw · 2016-05-07T09:30:44Z

src/clientManager.js

@@ -70,16 +72,20 @@ ClientManager.prototype.addUser = function(name, password) {
 		return false;
 	}
 	try {
-		var path = Helper.HOME + "/users";
+		var usersPath = Helper.HOME + "/users";


Just noticed this, do path.join(Helper.HOME, "users"); instead.

Sorry for previous comment, didn't notice it creates the folder

xPaw · 2016-05-07T11:32:13Z

Looks good to me. Should we perform same check in any other commands? Like edit or remove?

Also looking at this code try/catch is completely useless here...

williamboman · 2016-05-07T11:35:53Z

Also looking at this code try/catch is completely useless here...

Not anymore!

xPaw · 2016-05-07T11:43:51Z

👍

xPaw · 2016-05-07T12:21:28Z

src/clientManager.js

@@ -3,13 +3,31 @@ var fs = require("fs");
 var Client = require("./client");
 var mkdirp = require("mkdirp");
 var Helper = require("./helper");
+var path = require("path");
+
+var USERS_PATH = path.join(Helper.HOME, "users");


You can directly assign it to ClientManager.USERS_PATH, and in this file use this.USERS_PATH

"static" properties are hidden from the prototype chain, so this.USERS_PATH would be undefined. I could do var USERS_PATH = ClientManager.USERS_PATH = path.... though

You sure about that? It seems to work.

function ClientManager() {} ClientManager.test = 'kek'; ClientManager.w = function(){console.log(this.test);} ClientManager.w(); // "kek"

You could assign it inside the "constructor" though.

Yeah that would work since you're not working with its prototype;

function CM() {} CM.FOO = 'bar'; CM.prototype.bar = function () { console.log(this.FOO); }; a = new CM a.bar(); // undefined

Heh, I went that way as your two new functions didn't use prototype, but others in the file do.

maxpoulin64 · 2016-05-08T03:34:06Z

Looks good to me. I still think it would have been better to check for valid file names entirely, but this works too. 👍

Leaving open so other people can confirm and merge.

astorije · 2016-05-08T17:20:59Z

👍

clientManager: don't write user configs outside of lounge's users dir

xPaw reviewed Apr 2, 2016
View reviewed changes

williamboman force-pushed the fix/safer-add-user branch 3 times, most recently from 0352432 to bf123de Compare April 2, 2016 21:48

xPaw added the Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors. label Apr 20, 2016

xPaw reviewed May 7, 2016
View reviewed changes

williamboman force-pushed the fix/safer-add-user branch 2 times, most recently from cfd39c1 to 0b1365e Compare May 7, 2016 11:27

clientManager: don't write user configs outside of lounge's users dir

5e38060

williamboman force-pushed the fix/safer-add-user branch from 0b1365e to 5e38060 Compare May 7, 2016 11:35

williamboman force-pushed the fix/safer-add-user branch 6 times, most recently from 537929b to a44f4a0 Compare May 7, 2016 12:14

xPaw reviewed May 7, 2016
View reviewed changes

williamboman force-pushed the fix/safer-add-user branch from a44f4a0 to 5e38060 Compare May 7, 2016 12:49

xPaw added the second review needed label May 7, 2016

astorije merged commit 29e0717 into thelounge:master May 8, 2016

astorije removed the second review needed label May 8, 2016

astorije assigned xPaw May 8, 2016

astorije added this to the ★ Next Release milestone May 15, 2016

astorije added the Type: Security Security concern or PRs that must be reviewed with extra care regarding security. label May 21, 2016

williamboman deleted the fix/safer-add-user branch May 26, 2016 14:15

matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017

Merge pull request thelounge#238 from williamboman/fix/safer-add-user

4eb3fba

clientManager: don't write user configs outside of lounge's users dir

xPaw removed their assignment Mar 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clientManager: don't write user configs outside of lounge's users dir #238

clientManager: don't write user configs outside of lounge's users dir #238

williamboman commented Apr 2, 2016

xPaw Apr 2, 2016

williamboman Apr 2, 2016

xPaw Apr 2, 2016

williamboman Apr 2, 2016

williamboman Apr 2, 2016

maxpoulin64 Apr 2, 2016

xPaw May 7, 2016

williamboman May 7, 2016

xPaw May 7, 2016

williamboman May 7, 2016

xPaw May 7, 2016 •

edited

Loading

xPaw commented May 7, 2016 •

edited

Loading

williamboman commented May 7, 2016

xPaw commented May 7, 2016

xPaw May 7, 2016

williamboman May 7, 2016 •

edited

Loading

xPaw May 7, 2016 •

edited

Loading

williamboman May 7, 2016

xPaw May 7, 2016

maxpoulin64 commented May 8, 2016

astorije commented May 8, 2016

clientManager: don't write user configs outside of lounge's users dir #238

clientManager: don't write user configs outside of lounge's users dir #238

Conversation

williamboman commented Apr 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xPaw May 7, 2016 • edited Loading

Choose a reason for hiding this comment

xPaw commented May 7, 2016 • edited Loading

williamboman commented May 7, 2016

xPaw commented May 7, 2016

Choose a reason for hiding this comment

williamboman May 7, 2016 • edited Loading

Choose a reason for hiding this comment

xPaw May 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxpoulin64 commented May 8, 2016

astorije commented May 8, 2016

xPaw May 7, 2016 •

edited

Loading

xPaw commented May 7, 2016 •

edited

Loading

williamboman May 7, 2016 •

edited

Loading

xPaw May 7, 2016 •

edited

Loading