Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

[bug] Seed Password's strength [Closes #908] #909

Merged
merged 1 commit into from
Sep 13, 2015

Conversation

codydaig
Copy link
Member

No description provided.

@lirantal
Copy link
Member

Yeah, looks good, and we missed that with the secure password.
Good thing I've started a local branch to refactor the seed users functionality so it is actually testable! it's coming up :)

lirantal added a commit that referenced this pull request Sep 13, 2015
[bug] Seed Password's strength [Closes #908]
@lirantal lirantal merged commit 03da3ee into meanjs:master Sep 13, 2015
@ilanbiala
Copy link
Member

@lirantal this should actually close #907 and #908.

@lirantal
Copy link
Member

Thanks, got it.

@codydaig codydaig deleted the patch-1 branch September 13, 2015 22:07
@blueflagbj
Copy link

Bug persists, check to remove seed.js encryption like this:
................
//Add Local User
User.find({username: 'user'}).remove(function () {
// var password = crypto.randomBytes(64).toString('hex').slice(1, 20);
// seedUser.password = password;
var user = new User(seedUser);
// Then save the user
user.save(function (err) {
.................

@mleanos
Copy link
Member

mleanos commented Sep 16, 2015

I can confirm this. The password being used for the seeded Users is the randomly generated hex.

I think we have a couple options to fix..

  1. Continue to randomly generate the password, but force the generated hex to include the required upperCase, lowercase, number, and special character by appending to it.

var password = crypto.randomBytes(64).toString('hex').slice(1, 20) + 'aX5$';

My concern here would be the possibility of 3 or more consecutive characters (which isn't allowed with the password requirements) being generated by crypto. With a little bit of elbow grease we can inspect the generated password and make sure it adheres to the password requirements. It might not be pretty though :)

  1. Use the password defined in the seedAdmin & seedUser objects. Here we have the concern that originally prompted the use of crypto; we wouldn't want to reuse the same hardcoded password for reasons of security.

@mleanos
Copy link
Member

mleanos commented Sep 16, 2015

@lirantal I know you mentioned you were working on making this database seeding more testable. Have you made any progress on that? What are you thoughts on a possible fix for this?

@mleanos
Copy link
Member

mleanos commented Sep 18, 2015

@blueflagbj Until a fix is merged in, a temporary solution would be to use the password's defined in the User objects on the seed configuration, rather than the randomly generated value.

@lirantal
Copy link
Member

@blueflagbj posting some code (#909 (comment)) is not equal to reporting a bug.
Can I get an explanation of what's the actual problem?

@mleanos
Copy link
Member

mleanos commented Sep 18, 2015

@lirantal The issue is in db seed configuration. The password being used for the seeded users, is a generated hex string that doesn't pass the owasp test when the model attempts to save. Thus, the users aren't being created.

For reference: https://github.com/meanjs/mean/blob/master/config/lib/seed.js#L72-L73

I submitted #921

@lirantal
Copy link
Member

Yeah I can read the code too :)
It was a general remark - we usually have little time so I'm only asking to be more elaborate and descriptive and not let us guess.

I'll reply with my comments on #921

@mleanos
Copy link
Member

mleanos commented Sep 18, 2015

Ok :) I agree with that. Thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants