Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

passwords: require a digit in passwords generated from default rules #562

Merged
merged 1 commit into from
May 22, 2024
Merged

passwords: require a digit in passwords generated from default rules #562

merged 1 commit into from
May 22, 2024

Conversation

sjbarag
Copy link
Contributor

@sjbarag sjbarag commented May 14, 2024

Reviewer: @shakyShane
Asana: https://app.asana.com/0/1198964220583541/1205924276944158/f

Description

We previously required only a character in [-!?$&#%] when generating passwords for sites with neither a passwordrules attribute on their password input nor an override in rules.json. In many cases we'd include a number by random chance, but the relative weight of digits in the default pool meant they would sometimes not appear at all. Since sites with password content rules commonly require a digit to be present, this led to generated and saved passwords that immediately fail password validation. Require a digit for passwords generated from the default ruleset.

Steps to test

  1. npm run test
    • There's now a test that generates 10,000 passwords and ensures they all have at least one digit
    • One snapshot was updated to include the new 'digit' requirement.

@sjbarag
Copy link
Contributor Author

sjbarag commented May 14, 2024

@shakyShane It's probably worth a very brief discussion on whether this is safe, since it does change the defaults. My assumption is that a site that disallows digits in a password would also disallow one of the symbols we already require (-!?$&#%), so the risk seems quite minimal.

@sjbarag sjbarag requested a review from shakyShane May 14, 2024 18:40
shakyShane
shakyShane requested changes May 15, 2024
View reviewed changes
Copy link
Collaborator

@shakyShane shakyShane left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sjbarag - I think this is a safe change to make, I don't believe omitting numbers was deliberate back in the day.

Could you remove the formatting changes from the PR? I think some might be caused by jest inline snapshotting, but I can't be sure - perhaps prettier too?

If the changes are the result of the snapshot testing please let me know and I'll approve - but as you can imagine I'd rather not have formatting changes alongside password changes if possible - let me know!

GioSensation
GioSensation reviewed May 15, 2024
View reviewed changes
Copy link
Member

@GioSensation GioSensation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of notes. I'll leave the proper code review to Shane.

src/PasswordGenerator.test.js Outdated Show resolved Hide resolved
packages/password/tests/generate.test.js

function testUniqueTimes (domain, passwordRules, num = 10) {
const pws = []
for (let i = 0; i < num; i++) {
// these 3 domains have rulesets so weak that collisions are likely
// these 3 domains have rulesets so weak that collisions are likely
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Time to revisit adding prettier? I had a PR a loooong while ago, can't recall why I didn't go through with it.

@sjbarag
Copy link
Contributor Author

sjbarag commented May 21, 2024

Thanks @sjbarag - I think this is a safe change to make, I don't believe omitting numbers was deliberate back in the day.

Could you remove the formatting changes from the PR? I think some might be caused by jest inline snapshotting, but I can't be sure - perhaps prettier too?

If the changes are the result of the snapshot testing please let me know and I'll approve - but as you can imagine I'd rather not have formatting changes alongside password changes if possible - let me know!

This is unfortunately the result of npm run test:unit -- -u and a followup npm run lint:fix. I can't find a way to avoid it. Skipping npm run lint:fix provides a different diff, but it's still a big diff. Ditto manually running node_modules/.bin/prettier --write packages/password/tests/generate.test.js :/

@sjbarag
passwords: require a digit in passwords generated from default rules
2c0aa18 
We previously required only a character in `[-!?$&#%]` when generating
passwords for sites with neither a `passwordrules` attribute on their
password input nor an override in `rules.json`. In many cases we'd
include a number by random chance, but the relative weight of digits
in the default pool meant they would sometimes not appear at all.
Since sites with password content rules commonly require a digit to be
present, this led to generated and saved passwords that immediately fail
password validation. Require a digit for passwords generated from the
default ruleset.
@sjbarag sjbarag force-pushed the sbarag/require-number-in-generated-passwords branch from e8ae60d to 2c0aa18 Compare May 21, 2024 18:24
@shakyShane
Copy link
Collaborator

no worries @sjbarag - I was only interested in solving if it was a mistake - it's possible that a dependency bump on things like jest will have caused this (since we don't re-run snapshot updates when the deps are updated).

shakyShane
shakyShane approved these changes May 22, 2024
View reviewed changes
Copy link
Collaborator

@shakyShane shakyShane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sjbarag @GioSensation - approving with the following rationale:

1 - I don't think that omitting numbers was ever a deliberate choice
2 - I can't imagine a scenario where our existing special character set is accepted on a site, but then the addition of a number would cause it not to be
3 - if this change is released, and it causes more issues than it solves, we should consider making this configurable remotely

@GioSensation
Copy link
Member

if this change is released, and it causes more issues than it solves

FWIW, I don't think it's likely. Let's proceed. 🚀

@sjbarag sjbarag merged commit c20fac5 into main May 22, 2024
1 check passed
@sjbarag sjbarag deleted the sbarag/require-number-in-generated-passwords branch May 22, 2024 16:23
This was referenced Jun 20, 2024
Closed
Merged
Closed
Closed
Closed
CDRussell pushed a commit to duckduckgo/Android that referenced this pull request Jun 21, 2024
@daxmobile @sjbarag
Update autofill to 12.0.0 (#4673)
8b4808f 
Task/Issue URL:
https://app.asana.com/0/1207623126907992/1207623126907992
Autofill Release:
https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/12.0.0


## Description
Updates Autofill to version
[12.0.0](https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/12.0.0).

### Autofill 12.0.0 release notes
* Major version bump to 12.0.0 caused by #396, which impacts only
Android.
* This release also translates the autofill experience to the following
language codes:
    * bg
    * cs
    * da
    * de
    * el
    * en
    * es
    * et
    * fi
    * fr
    * hr
    * hu
    * it
    * lt
    * lv
    * nb
    * nl
    * pl
    * pt
    * ro
    * ru
    * sk
    * sl
    * sv
    * tr


## What's Changed
* Ema/more minor fixes by @GioSensation in
duckduckgo/duckduckgo-autofill#556
* l10n: extract remaining strings to prepare for localization by
@sjbarag in duckduckgo/duckduckgo-autofill#558
* l10n: pull translated strings into separate JSON files by @sjbarag in
duckduckgo/duckduckgo-autofill#559
* Update password-related json files (2024-05-13) by @daxmobile in
duckduckgo/duckduckgo-autofill#561
* Update password-related json files (2024-05-19) by @daxmobile in
duckduckgo/duckduckgo-autofill#569
* vcs: ignore .helix directory by @sjbarag in
duckduckgo/duckduckgo-autofill#565
* l10n: remove placeholder from 'Manage ${ITEM}…' string by @sjbarag in
duckduckgo/duckduckgo-autofill#560
* real-world: remove expected failure from clientssp.fnfis.com test by
@sjbarag in duckduckgo/duckduckgo-autofill#563
* real-world: remove expected failure for agile.appian.com by @sjbarag
in duckduckgo/duckduckgo-autofill#567
* autofill: left-align HTML autofill buttons by @sjbarag in
duckduckgo/duckduckgo-autofill#571
* l10n: add translated messages by @sjbarag in
duckduckgo/duckduckgo-autofill#572
* passwords: require a digit in passwords generated from default rules
by @sjbarag in
duckduckgo/duckduckgo-autofill#562
* classifiers: fix another batch of test page expected username failures
by @sjbarag in
duckduckgo/duckduckgo-autofill#573
* Update password-related json files (2024-05-30) by @daxmobile in
duckduckgo/duckduckgo-autofill#577
* Update password-related json files (2024-05-31) by @daxmobile in
duckduckgo/duckduckgo-autofill#578
* l10n: update translations based on ship review feedback by @sjbarag in
duckduckgo/duckduckgo-autofill#581
* android: Revert "Android: enable iframe support (#536)" by @sjbarag in
duckduckgo/duckduckgo-autofill#582

## New Contributors
* @sjbarag made their first contribution in
duckduckgo/duckduckgo-autofill#558

**Full Changelog**:
duckduckgo/duckduckgo-autofill@11.0.2...12.0.0

## Steps to test
This release has been tested during autofill development. For smoke test
steps see [this
task](https://app.asana.com/0/1198964220583541/1200583647142330/f).

Co-authored-by: sjbarag <665775+sjbarag@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GioSensation GioSensation GioSensation left review comments

@shakyShane shakyShane shakyShane approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sjbarag @shakyShane @GioSensation