faker-js / faker

Generate massive amounts of fake data in the browser and node.js
https://fakerjs.dev
Other
12.64k stars 909 forks source link

Reimplement internet.password() with more generation options #768

Open ST-DDT opened 2 years ago

ST-DDT commented 2 years ago

Clear and concise description of the problem

The passwords generated by internet.password() are not really secure.

Suggested solution

It would be nice if there would be various generation ways one can choose from.

Alternative

Remove the default pattern, that severely limits the password security

Additional context

No response

xDivisionByZerox commented 2 years ago

This looks interesting, I might provide a PR for this in the near future.

xDivisionByZerox commented 2 years ago

The suggestions you made, would they all be implemented in a separate function? Or would there only be a single parameter like faker.internet.password({ mode: 'secure' })?

ST-DDT commented 2 years ago

IMO it should be accessed via a single method with options. Whether we use internal helper methods for each generation strategy doesn't really matter to me.

xDivisionByZerox commented 2 years ago

For parameter options, I would like something like https://passwordsgenerator.net/plus is doing. This would not be as simple with a single method and a mode.

The signature could also look something like this:

type PasswordMode = 'secure' | 'memorable' | 'simple';

password(
  opt:
    | PasswordMode
    | {
        mode?: PasswordMode;
      }
    | {
        length?: number;
        includeSymbols?: boolean;
        includeNumbers?: boolean;
        includeLowercaseChars?: boolean;
        includeUppercaseChars?: boolean;
        noSimilarChars?: boolean;
        noDuplicateChars?: boolean;
        noSequentialChars?: boolean;
        bannedChars?: Array<string>;
      }
) {
  // implementation
}
xDivisionByZerox commented 2 years ago

Based on my last comment: If you provide all the groups (symbol, lower, upper, number) included and set a length less than 4, what would the expected behavior be? Throwing an error? Returning the string with one of each group?

ST-DDT commented 2 years ago

This depends on the definition/description you are going to add to the parameters. IMO include != required -> so you can have even long passwords without any of them. It's just a matter of chance. If you are going for require, then I would throw.

xDivisionByZerox commented 2 years ago

I have read the include properties as required ("at least one of") since if you don't want to have any specific chars (don't include them at all) you can provide them via the banned chars array. But in this case, it would be best to name the properties appropriately (include => require).

xDivisionByZerox commented 2 years ago

Another approach would be to provide a different signature (advanced options only):

{
  length: number;
  symbol: {
    include: boolean;
    require: boolean;
  };
  number: {
    include: boolean;
    require: boolean;
  };
  lowercase: {
    include: boolean;
    require: boolean;
  };
  uppercase: {
    include: boolean;
    require: boolean;
  };
  noSimilarChars: boolean;
  noDuplicateChars: boolean;
  noSequentialChars: boolean;
  bannedChars?: readonly LiteralUnion<PasswordChar>[];
}

I really like this, because we can extend each character group with more options. Like a minimal count for example.

ST-DDT commented 2 years ago

I'm not sure whether we are taking this too far at this point. We are not a password generator.

ST-DDT commented 2 years ago

Team decision

Lightweight password generator (non-secure in jsdocs).

vandrieiev-godaddy commented 1 year ago

https://github.com/faker-js/faker/issues/474 how about that bug in current version? i still get that error when passing regex, e.g.:

faker.internet.password(15, false, /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d).+$/)
ST-DDT commented 1 year ago

The password method is bad when a random regexp is given. You might want to use https://next.fakerjs.dev/api/helpers.html#fromregexp instead.

xDivisionByZerox commented 1 year ago

But faker.string.fromRegExp is not out yet, tho. It's not even in the latest alpha release 🤔

import-brain commented 10 months ago

This looks interesting, I might provide a PR for this in the near future.

@xDivisionByZerox Are you still willing to work on this?

xDivisionByZerox commented 10 months ago

If you want to work on this, feel free. I already have a branch that implements this if you want to take a look. I'm on my phone, so linking is hard right now.

ST-DDT commented 10 months ago

https://github.com/faker-js/faker/tree/refactor/internet/password

import-brain commented 10 months ago

If you want to work on this, feel free. I already have a branch that implements this if you want to take a look. I'm on my phone, so linking is hard right now.

Sorry, I didn't realize you already made a branch implementing this. From my understanding, we want to make it more lightweight than what you have currently?

jacobg commented 8 months ago

https://github.com/faker-js/faker/tree/refactor/internet/password

@ST-DDT This is great! Are you planning to open a pull request?

ST-DDT commented 8 months ago

Sadly, I can currently only answer that with "maybe". We are about to wrap up development for v8.x (today) and start with v9. For that we have some improvements planned that will likely also affect the implementation of password method (same as all other methods), so this might change in implementation, but not much in API.

Which feature is added by the branch, that you would like to use that is not possible with the current one? Maybe we can recommend a workaround until then?

FFR: https://fakerjs.dev/api/internet.html#password