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.

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

Add validation to ensure password length does not exceed 72 bytes #6280

Closed
4 tasks done
syobocat opened this issue Oct 5, 2023 · 5 comments
Closed
4 tasks done

Add validation to ensure password length does not exceed 72 bytes #6280

syobocat opened this issue Oct 5, 2023 · 5 comments
Assignees
Milestone

Comments

@syobocat
Copy link

syobocat commented Oct 5, 2023

Prerequisites

Platform (OS and CPU architecture)

FreeBSD, AMD64 (aka x86_64)

Installation

GitHub releases or script from README

Setup

On one machine

AdGuard Home version

0.107.38

Action

  1. Run installation script
  2. Go to xxx.xxx.xxx.xxx:3000 for initial setup
  3. Use a password longer than 24 bytes
  4. Complete the initial setup

Expected result

Hash of the password get written on the configuration file, and “Sign out” button is shown on the upper right corner of Dashboard

Actual result

users section of the configuration file remains empty, and no “Sign out” button appeared and users can access to Dashboard without authentication

Additional information and/or screenshots

Resetting password works with passwords of any length

* As this is too simple problem, I thought there must be a duplicate issue on GitHub.
However, I couldn’t find it, so I submitted this issue.
Sorry if I just overlooked.

@ainar-g
Copy link
Contributor

ainar-g commented Oct 5, 2023

I cannot reproduce this, sorry. I'm successfully using 123456789012345678901234567890, which is 30 bytes, as a password.

Can you launch the AGH install with the -v flag to see the verbose messages and perhaps any errors?

@ainar-g ainar-g added the waiting for data Waiting for users to provide more data. label Oct 5, 2023
@syobocat
Copy link
Author

syobocat commented Oct 5, 2023

Thank you for your reply.

It seems that Cloudflare’s cache or something prevented me to understand the problem correctly.
Passwords longer than 24 bytes don’t cause the problem as you said, however, passwords longer than 72 bytes do, and it’s obviously because bcrypt doesn’t accept inputs longer than 72 bytes.

So, I think I need to change this issue into a feature request saying “Add validation to ensure password length does not exceed 72 bytes”.

@syobocat syobocat changed the title Initial setup doesn’t save passwords longer than 24 bytes Add validation to ensure password length does not exceed 72 bytes Oct 5, 2023
@ainar-g
Copy link
Contributor

ainar-g commented Oct 5, 2023

Ah, I see, I can reproduce this now. Thanks for the report.

@ainar-g ainar-g self-assigned this Oct 5, 2023
@ainar-g ainar-g added bug P3: Medium UI and removed waiting for data Waiting for users to provide more data. labels Oct 5, 2023
@ainar-g ainar-g added this to the v0.107.39 milestone Oct 5, 2023
adguard pushed a commit that referenced this issue Oct 5, 2023
Updates #6280.

Squashed commit of the following:

commit 85014e2
Merge: 2d93201 5f61b55
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Thu Oct 5 15:58:48 2023 +0300

    Merge branch 'master' into 6280-password-length

commit 2d93201
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Thu Oct 5 15:43:05 2023 +0300

    client: rm dep

commit 3b11d10
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Thu Oct 5 15:22:58 2023 +0300

    client: imp i18n

commit f88dfc9
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Thu Oct 5 15:17:56 2023 +0300

    all: imp i18n, names

commit a7874f5
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Thu Oct 5 15:07:10 2023 +0300

    all: fix passwd check
@ainar-g
Copy link
Contributor

ainar-g commented Oct 5, 2023

This should be fixed on the Edge channel. Could you please check?

@syobocat
Copy link
Author

I confirmed that the setup screen rejects passwords longer than 72bytes, on v0.108.0-a.725+5fa11567.
Thank you so much!

@ainar-g ainar-g modified the milestones: v0.107.40, v0.107.39 Oct 12, 2023
annguyen0 pushed a commit to annguyen0/AdGuardHome that referenced this issue Nov 27, 2023
Updates AdguardTeam#6280.

Squashed commit of the following:

commit 85014e2
Merge: 2d93201 5f61b55
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Thu Oct 5 15:58:48 2023 +0300

    Merge branch 'master' into 6280-password-length

commit 2d93201
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Thu Oct 5 15:43:05 2023 +0300

    client: rm dep

commit 3b11d10
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Thu Oct 5 15:22:58 2023 +0300

    client: imp i18n

commit f88dfc9
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Thu Oct 5 15:17:56 2023 +0300

    all: imp i18n, names

commit a7874f5
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Thu Oct 5 15:07:10 2023 +0300

    all: fix passwd check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants