Skip to content

Fix regression: Remove length limit on password#5748

Merged
droidmonkey merged 1 commit intokeepassxreboot:release/2.6.3from
bodograumann:patch-1
Dec 4, 2020
Merged

Fix regression: Remove length limit on password#5748
droidmonkey merged 1 commit intokeepassxreboot:release/2.6.3from
bodograumann:patch-1

Conversation

@bodograumann
Copy link
Copy Markdown
Contributor

A feature of keepassxc, that I used, was to generate long passwords e.g. of 255 characters.
This feature was broken lately.

Not completely sure if this fix will just work like that, but it seemed worth a shot to me.

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)

Software should make the user free, not restrict him.
Copy link
Copy Markdown
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

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

I am fine with merging this, but let me tell you: there is absolutely NOTHING gained from using a password that long.

@droidmonkey
Copy link
Copy Markdown
Member

Can we add some form of a limit? I don't think this change is enough to ensure consistent ui

@phoerious
Copy link
Copy Markdown
Member

I am fine with users being able to manually enter higher values than what they can dial in with the the slider. I don't care how long these are, but if you want to use excessively long passwords, keep in mind that your password will be hashed into a 128 or 256-bit hash value anyway, so a 10,000-bit entropy password isn't going to do anything for you. It only increases the chances of the web service fucking something up and locking you out.

@bodograumann
Copy link
Copy Markdown
Contributor Author

I don't want to dismiss the good arguments that you bring, but I intentionally tried to avoid a discussion of whether it is sensible to use such long passwords. So let's just say I enjoy having them ;-)
(And yes, I have had problems due to that; but also when not adhering to a limit of 12 characters or when using some non-alphanumeric characters)

From a UI standpoint I don't find it too confusing if the slider stops at 128 characters. If you feel it might break the UI functionality or not achieve the goal, because the slider still keeps the value below 128, I could test that and possibly come up with a better solution.

@droidmonkey
Copy link
Copy Markdown
Member

I'm good with just the text entry being unbound

@droidmonkey droidmonkey changed the base branch from master to release/2.6.3 December 4, 2020 00:57
@droidmonkey droidmonkey added this to the v2.6.3 milestone Dec 4, 2020
@droidmonkey droidmonkey merged commit 1dd9955 into keepassxreboot:release/2.6.3 Dec 4, 2020
phoerious added a commit that referenced this pull request Jan 12, 2021
Added

- Support Argon2id KDF [#5778]
- Support XMLv2 key files [#5798]

Changed

- Improve CSV Import/Export, include time fields and TOTP [#5346]
- Support empty area dragging of the application window [#5860]
- Display default Auto-Type sequence in preview pane [#5654]
- Remove strict length limit on generated passwords [#5748]
- Hide key file path by default when unlocking database [#5779]
- Document browser extension use with Edge in managed mode [#5692]
- Windows: Prevent clipboard history and cloud sync [#5853]
- macOS: Update the application icon to Big Sur styling [#5851]

Fixed

- Re-select previously selected entry on database unlock [#5559]
- Properly save special character choice in password generator [#5610]
- Fix crash in browser integration with multiple similar entries [#5653]
- Remove offset on username field in classic theme [#5788]
- Ensure entry history is copied when drag/dropping entries and groups [#5817]
- Close modal dialogs when database is locked [#5820]
- Prevent crash when KeeShare modifies an entry that is currently being edited [#5827]
- Improve preview of entry attributes [#5834]
- Always activate/focus database open dialog preventing mistype [#5878]
- Reports: fix calculation of average password length [#5862]
- Linux: Delay startup on login to correct tray icon issues [#5724]
aswild added a commit to aswild/keepassxc that referenced this pull request Jan 13, 2021
Release 2.6.3

Added

- Support Argon2id KDF [keepassxreboot#5778]
- Support XMLv2 key files [keepassxreboot#5798]

Changed

- Improve CSV Import/Export, include time fields and TOTP [keepassxreboot#5346]
- Support empty area dragging of the application window [keepassxreboot#5860]
- Display default Auto-Type sequence in preview pane [keepassxreboot#5654]
- Remove strict length limit on generated passwords [keepassxreboot#5748]
- Hide key file path by default when unlocking database [keepassxreboot#5779]
- Document browser extension use with Edge in managed mode [keepassxreboot#5692]
- Windows: Prevent clipboard history and cloud sync [keepassxreboot#5853]
- macOS: Update the application icon to Big Sur styling [keepassxreboot#5851]

Fixed

- Re-select previously selected entry on database unlock [keepassxreboot#5559]
- Properly save special character choice in password generator [keepassxreboot#5610]
- Fix crash in browser integration with multiple similar entries [keepassxreboot#5653]
- Remove offset on username field in classic theme [keepassxreboot#5788]
- Ensure entry history is copied when drag/dropping entries and groups [keepassxreboot#5817]
- Close modal dialogs when database is locked [keepassxreboot#5820]
- Prevent crash when KeeShare modifies an entry that is currently being edited [keepassxreboot#5827]
- Improve preview of entry attributes [keepassxreboot#5834]
- Always activate/focus database open dialog preventing mistype [keepassxreboot#5878]
- Reports: fix calculation of average password length [keepassxreboot#5862]
- Linux: Delay startup on login to correct tray icon issues [keepassxreboot#5724]
@Dykam
Copy link
Copy Markdown

Dykam commented Jan 14, 2021

Just to double check, this was supposed to allow you to enter any length value into the password generator in 2.6.3, right? The thing it did for me is that now, either using the slider or by manual entry, the value doesn't go past 99. The slider still goes to a visual 128, but the effective length caps at 99. Manual entry is even less intuitive as it simply prevents me from adding a 3rd digit.

If this isn't intended behavior, I'll create a bug report.

@bodograumann
Copy link
Copy Markdown
Contributor Author

Ugh, yeah sorry @Dykam. This is my bad. I should have read the docs more carefully.
Turns out the default maximum for QSpinBox is 99.
So it seems an unbounded QSpinBox is not possible.

@droidmonkey
Copy link
Copy Markdown
Member

droidmonkey commented Jan 14, 2021

This is what I get for not explicitly testing the feature... this change doesn't work at all, it LOWERS the maximum password length. Typing into the text field a custom length is not possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants