Skip to content

fix(webui): enforce 12-char dashboard password policy with backend+frontend validation#7338

Open
Soulter wants to merge 7 commits intomasterfrom
fix/dashboard-password-policy
Open

fix(webui): enforce 12-char dashboard password policy with backend+frontend validation#7338
Soulter wants to merge 7 commits intomasterfrom
fix/dashboard-password-policy

Conversation

@Soulter
Copy link
Copy Markdown
Member

@Soulter Soulter commented Apr 3, 2026

closes #7321

Summary

  • Enforce dashboard password minimum length to 12 characters with backend validation and shared utility checks.
  • Require backend password complexity (uppercase, lowercase, digit) for account update path.
  • Add automatic normalization for empty dashboard password config to default-hashed value to avoid blank-password edge cases.
  • Update frontend account auth/password hints, validation labels and login/password-change related UX copy to 12-char policy and default-password reminder text.
  • Backward compatibility remains for legacy MD5 authentication verification.

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Apr 3, 2026
@dosubot dosubot bot added the area:webui The bug / feature is about webui(dashboard) of astrbot. label Apr 3, 2026
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've found 2 issues, and left some high level feedback:

  • The frontend password rules only enforce a 12-character minimum while the backend now adds complexity requirements (uppercase/lowercase/digit), so users may get opaque server-side errors; consider aligning the client-side validation and messages with the backend policy.
  • normalize_dashboard_password_hash() silently replaces an empty password with a hash of the weak default 'astrbot', which also violates the new complexity policy; consider either enforcing the complexity policy on this path or requiring an explicit password change instead of auto-falling back to the default.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The frontend password rules only enforce a 12-character minimum while the backend now adds complexity requirements (uppercase/lowercase/digit), so users may get opaque server-side errors; consider aligning the client-side validation and messages with the backend policy.
- normalize_dashboard_password_hash() silently replaces an empty password with a hash of the weak default 'astrbot', which also violates the new complexity policy; consider either enforcing the complexity policy on this path or requiring an explicit password change instead of auto-falling back to the default.

## Individual Comments

### Comment 1
<location path="astrbot/dashboard/routes/stat.py" line_range="236" />
<code_context>
                         ProviderStat.created_at >= query_start_utc,
                     )
-                    .order_by(ProviderStat.created_at.asc())
+                    .order_by(col(ProviderStat.created_at).asc())
                 )
                 records = result.scalars().all()
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `col(ProviderStat.created_at)` instead of `ProviderStat.created_at` directly is likely incorrect and could break the query.

This was previously `.order_by(ProviderStat.created_at.asc())`, which is the standard SQLAlchemy pattern. `col()` is generally used with string column names, and passing an ORM attribute may be unnecessary or even alter the generated SQL depending on the SQLAlchemy version. Unless there’s a specific need for `col()` here, please revert to `ProviderStat.created_at.asc()`.
</issue_to_address>

### Comment 2
<location path="astrbot/core/utils/auth_password.py" line_range="68" />
<code_context>
+    return isinstance(stored, str) and stored.startswith(_PBKDF2_FORMAT)
+
+
+def verify_dashboard_password(stored_hash: str, candidate_password: str) -> bool:
+    """Verify password against legacy md5 or new PBKDF2-SHA256 format."""
+    if not isinstance(stored_hash, str) or not isinstance(candidate_password, str):
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring `verify_dashboard_password` into a small dispatcher plus per-scheme helpers so each hash type’s detection, parsing, and verification are clearly separated.

`verify_dashboard_password` is carrying multiple responsibilities (scheme detection, parsing, and verification) and mixing semantics in a single branch. You can keep all behavior but make it easier to read and test by introducing a small dispatcher + per-scheme helpers.

For example:

```python
from enum import Enum, auto

class _HashScheme(Enum):
    LEGACY_MD5 = auto()
    PBKDF2 = auto()
    UNKNOWN = auto()


def _get_hash_scheme(stored_hash: str) -> _HashScheme:
    if _is_legacy_md5_hash(stored_hash):
        return _HashScheme.LEGACY_MD5
    if _is_pbkdf2_hash(stored_hash):
        return _HashScheme.PBKDF2
    return _HashScheme.UNKNOWN
```

Then split the verification logic into named functions so the compatibility behavior is explicit:

```python
def _verify_legacy_md5_hash(stored_hash: str, candidate_password: str) -> bool:
    # Keep compatibility with existing md5-based deployments:
    # new clients send plain password, old clients may send md5 of it.
    candidate_md5 = hashlib.md5(candidate_password.encode("utf-8")).hexdigest()
    stored = stored_hash.lower()
    return (
        hmac.compare_digest(stored, candidate_md5.lower())
        or hmac.compare_digest(stored, candidate_password.lower())
    )


def _verify_pbkdf2_hash(stored_hash: str, candidate_password: str) -> bool:
    parts: list[str] = stored_hash.split("$")
    if len(parts) != 4:
        return False
    _, iterations_s, salt, digest = parts
    try:
        iterations = int(iterations_s)
        stored_key = bytes.fromhex(digest)
        salt_bytes = bytes.fromhex(salt)
    except (TypeError, ValueError):
        return False

    candidate_key = hashlib.pbkdf2_hmac(
        "sha256",
        candidate_password.encode("utf-8"),
        salt_bytes,
        iterations,
    )
    return hmac.compare_digest(stored_key, candidate_key)
```

`verify_dashboard_password` then becomes a straightforward dispatcher:

```python
def verify_dashboard_password(stored_hash: str, candidate_password: str) -> bool:
    if not isinstance(stored_hash, str) or not isinstance(candidate_password, str):
        return False

    scheme = _get_hash_scheme(stored_hash)
    if scheme is _HashScheme.LEGACY_MD5:
        return _verify_legacy_md5_hash(stored_hash, candidate_password)
    if scheme is _HashScheme.PBKDF2:
        return _verify_pbkdf2_hash(stored_hash, candidate_password)
    return False
```

This keeps all existing behavior (including the legacy MD5 dual comparison and default-password checks) but makes each piece self-contained and easier to reason about and test individually.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

ProviderStat.created_at >= query_start_utc,
)
.order_by(ProviderStat.created_at.asc())
.order_by(col(ProviderStat.created_at).asc())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

issue (bug_risk): Using col(ProviderStat.created_at) instead of ProviderStat.created_at directly is likely incorrect and could break the query.

This was previously .order_by(ProviderStat.created_at.asc()), which is the standard SQLAlchemy pattern. col() is generally used with string column names, and passing an ORM attribute may be unnecessary or even alter the generated SQL depending on the SQLAlchemy version. Unless there’s a specific need for col() here, please revert to ProviderStat.created_at.asc().

return isinstance(stored, str) and stored.startswith(_PBKDF2_FORMAT)


def verify_dashboard_password(stored_hash: str, candidate_password: str) -> bool:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

issue (complexity): Consider refactoring verify_dashboard_password into a small dispatcher plus per-scheme helpers so each hash type’s detection, parsing, and verification are clearly separated.

verify_dashboard_password is carrying multiple responsibilities (scheme detection, parsing, and verification) and mixing semantics in a single branch. You can keep all behavior but make it easier to read and test by introducing a small dispatcher + per-scheme helpers.

For example:

from enum import Enum, auto

class _HashScheme(Enum):
    LEGACY_MD5 = auto()
    PBKDF2 = auto()
    UNKNOWN = auto()


def _get_hash_scheme(stored_hash: str) -> _HashScheme:
    if _is_legacy_md5_hash(stored_hash):
        return _HashScheme.LEGACY_MD5
    if _is_pbkdf2_hash(stored_hash):
        return _HashScheme.PBKDF2
    return _HashScheme.UNKNOWN

Then split the verification logic into named functions so the compatibility behavior is explicit:

def _verify_legacy_md5_hash(stored_hash: str, candidate_password: str) -> bool:
    # Keep compatibility with existing md5-based deployments:
    # new clients send plain password, old clients may send md5 of it.
    candidate_md5 = hashlib.md5(candidate_password.encode("utf-8")).hexdigest()
    stored = stored_hash.lower()
    return (
        hmac.compare_digest(stored, candidate_md5.lower())
        or hmac.compare_digest(stored, candidate_password.lower())
    )


def _verify_pbkdf2_hash(stored_hash: str, candidate_password: str) -> bool:
    parts: list[str] = stored_hash.split("$")
    if len(parts) != 4:
        return False
    _, iterations_s, salt, digest = parts
    try:
        iterations = int(iterations_s)
        stored_key = bytes.fromhex(digest)
        salt_bytes = bytes.fromhex(salt)
    except (TypeError, ValueError):
        return False

    candidate_key = hashlib.pbkdf2_hmac(
        "sha256",
        candidate_password.encode("utf-8"),
        salt_bytes,
        iterations,
    )
    return hmac.compare_digest(stored_key, candidate_key)

verify_dashboard_password then becomes a straightforward dispatcher:

def verify_dashboard_password(stored_hash: str, candidate_password: str) -> bool:
    if not isinstance(stored_hash, str) or not isinstance(candidate_password, str):
        return False

    scheme = _get_hash_scheme(stored_hash)
    if scheme is _HashScheme.LEGACY_MD5:
        return _verify_legacy_md5_hash(stored_hash, candidate_password)
    if scheme is _HashScheme.PBKDF2:
        return _verify_pbkdf2_hash(stored_hash, candidate_password)
    return False

This keeps all existing behavior (including the legacy MD5 dual comparison and default-password checks) but makes each piece self-contained and easier to reason about and test individually.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request migrates dashboard password security from MD5 to PBKDF2-HMAC-SHA256 and implements a 12-character minimum complexity policy. Key changes include a new auth_password utility for secure hashing and verification, updated API routes with improved input validation, and frontend modifications to remove client-side hashing and display security warnings for legacy credentials. Feedback identifies a need to align frontend validation with backend complexity rules, add server-side validation for username updates, and rename misleading variables in the frontend code to reflect the removal of MD5 hashing.

if not isinstance(stored_hash, str) or not isinstance(candidate_password, str):
return False

if _is_legacy_md5_hash(stored_hash):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's better to enforce user to update the password by a specially constructed function, rather than being inserted into the validation stream.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

currently after user updates astrbot and opens the WebUI, a mandatory password change dialog will appear.

// @ts-ignore
authStore.returnUrl = new URLSearchParams(window.location.search).get('redirect');
return authStore.login(username.value, password_).then((res) => {
return authStore.login(username.value, password.value).then((res) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The password should be hashed at the user's side; And the unique salt can be safely passed in plain to the client for iteration, whether it is fixed or not. If not, use TLS to secure the plain text passoword.

@Raven95676
Copy link
Copy Markdown
Member

个人推荐通过argon2-cffi使用Argon2id

@AlanCui4080
Copy link
Copy Markdown

AlanCui4080 commented Apr 4, 2026

个人推荐通过argon2-cffi使用Argon2id

是的,PBKDF2 在今天被视为是可接受的,除非你需要通过FIPS认证,不然应该迁移到更新的KDF。但是我认为,强制执行 TLS 才是提升安全性的重中之重,否则双方应该通过某种交互式零知识证明交换 PIN。如果强制执行了 TLS,那么弱一些的 KDF 仍然可以保护密码免受明文恢复攻击,至少他们所基于的哈希算法单射特性十分好。

@AlanCui4080
Copy link
Copy Markdown

AlanCui4080 commented Apr 4, 2026

旮旯game里面不是这样的,你应该生成两个盐,一个从数据库里面取,一个随机生成,一起发给我,我加第一个盐生成一个哈希,再加第二个盐再生成一个哈希,你拿去和数据库里面预存的哈希加盐再哈希,比对结果,再告诉我密码正确不正确!你怎么上来就跟我要密码!旮旯game里根本不是这样的,我不接受!

否则双方应该通过某种交互式零知识证明交换 PIN。

可以参考 Secure Remote Password Protocol 。

@LIghtJUNction
Copy link
Copy Markdown
Member

https就好了
支持https,只是不强制

@AlanCui4080
Copy link
Copy Markdown

AlanCui4080 commented Apr 4, 2026

https就好了 支持https,只是不强制

在原本的实现中,MD5哈希极易遭受彩虹表攻击,会导致用户密码明文被恢复

如果采取当前 PR ,HTTPS 必须被强制执行,否则明文密码会被发送到服务端。或者,可以将哈希转移到客户端进行。或者,可以执行密码认证的密钥交换。

@Soulter Soulter force-pushed the fix/dashboard-password-policy branch from 9ab169d to b749f62 Compare April 4, 2026 07:50
@Raven95676
Copy link
Copy Markdown
Member

个人推荐通过argon2-cffi使用Argon2id

是的,PBKDF2 在今天被视为是可接受的,除非你需要通过FIPS认证,不然应该迁移到更新的KDF。但是我认为,强制执行 TLS 才是提升安全性的重中之重,否则双方应该通过某种交互式零知识证明交换 PIN。如果强制执行了 TLS,那么弱一些的 KDF 仍然可以保护密码免受明文恢复攻击,至少他们所基于的哈希算法单射特性十分好。

考虑到用户部署环境的差异性,TLS应作为可选配置而非强制要求。且TLS的安全性高度依赖可信CA,而自签名证书在多数场景下难以防范中间人攻击且用户体验不佳。

至于交互式零知识证明,其部署难度高于TLS,在现实中只能作为TLS的强安全替代方案,且在绝大多数通用业务场景下并不需要如此之高的安全性。

@LIghtJUNction
Copy link
Copy Markdown
Member

不是所有人都是远程部署的
还有相当一部分人是localhost 访问,安全

@LIghtJUNction
Copy link
Copy Markdown
Member

而且https需要购买域名

@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Apr 4, 2026
@AlanCui4080
Copy link
Copy Markdown

考虑到用户部署环境的差异性,TLS应作为可选配置而非强制要求。且TLS的安全性高度依赖可信CA,而自签名证书在多数场景下难以防范中间人攻击且用户体验不佳。

HTTPS中,证书只作为识别对端身份的手段,实际通讯密钥是通过临时密钥协商产生的。只要使用HTTPS,安全性就由临时密钥协商保证。而 MITM 成本比侦听明文 HTTP 流高多了。

正如我上面所说的,如果不强制执行TLS安全性,密钥就应该由客户端派生。

@LIghtJUNction
Copy link
Copy Markdown
Member

自签名也挺安全,但是麻烦啊
自签名应该算是最安全的了
零信任
但是需要自己导入自己整的根证书
安卓的话还需要root才行

@Soulter Soulter force-pushed the fix/dashboard-password-policy branch from 21bff02 to b749f62 Compare April 4, 2026 07:57
@dosubot dosubot bot removed the size:XXL This PR changes 1000+ lines, ignoring generated files. label Apr 4, 2026
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Apr 4, 2026
@AlanCui4080
Copy link
Copy Markdown

AlanCui4080 commented Apr 4, 2026

而且https需要购买域名

可以使用自签证书,Chrome在首次信任某个自签证书后会保留他的指纹,如果指纹变更会重新触发警告页面,事实上,大部分实现都是服务端自签的。

自签名也挺安全,但是麻烦啊 自签名应该算是最安全的了 零信任 但是需要自己导入自己整的根证书 安卓的话还需要root才行

自建CA和自签证书是两个概念。对于自建根CA,Android应用可以选择相信用户存储区的证书,Chrome也默认信任用户存储区,这是不需要root的。

@AlanCui4080
Copy link
Copy Markdown

image image

FYI

@AlanCui4080
Copy link
Copy Markdown

至于交互式零知识证明,其部署难度高于TLS,在现实中只能作为TLS的强安全替代方案,且在绝大多数通用业务场景下并不需要如此之高的安全性。

参见 https://github.com/cocagne/pysrp

@LIghtJUNction
Copy link
Copy Markdown
Member

确实

@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Apr 4, 2026
@LIghtJUNction
Copy link
Copy Markdown
Member

设置两环境变量就启用https了,挺方便的

@Raven95676
Copy link
Copy Markdown
Member

Raven95676 commented Apr 4, 2026

考虑到用户部署环境的差异性,TLS应作为可选配置而非强制要求。且TLS的安全性高度依赖可信CA,而自签名证书在多数场景下难以防范中间人攻击且用户体验不佳。

HTTPS中,证书只作为识别对端身份的手段,实际通讯密钥是通过临时密钥协商产生的。只要使用HTTPS,安全性就由临时密钥协商保证。而 MITM 成本比侦听明文 HTTP 流高多了。

正如我上面所说的,如果不强制执行TLS安全性,密钥就应该由客户端派生。

安全不应建立在“攻击者可能比较懒”的假设之上。我个人观点来说自签名证书会给用户带来虚假的安全感。

对于提到的临时密钥协商,一个可能的攻击方案是中间人分别对客户端以及服务端各自做一次ECDHE,这种情况下ECDHE无法提供确实的安全性。此外,假如引导用户无视TLS证书报警,就会消解用户对安全风险的警惕性,攻击者只需要诱导用户点击一次继续即可。

个人认为,AstrBot可以在未配置 TLS 时给出明确的安全风险提示,将选择权交给用户。

@Raven95676
Copy link
Copy Markdown
Member

至于交互式零知识证明,其部署难度高于TLS,在现实中只能作为TLS的强安全替代方案,且在绝大多数通用业务场景下并不需要如此之高的安全性。

参见 https://github.com/cocagne/pysrp

确实,这个实现对于用户来说几乎是无感的

@LIghtJUNction
Copy link
Copy Markdown
Member

考虑到用户部署环境的差异性,TLS应作为可选配置而非强制要求。且TLS的安全性高度依赖可信CA,而自签名证书在多数场景下难以防范中间人攻击且用户体验不佳。

HTTPS中,证书只作为识别对端身份的手段,实际通讯密钥是通过临时密钥协商产生的。只要使用HTTPS,安全性就由临时密钥协商保证。而 MITM 成本比侦听明文 HTTP 流高多了。
正如我上面所说的,如果不强制执行TLS安全性,密钥就应该由客户端派生。

安全不应建立在“攻击者可能比较懒”的假设之上。我个人观点来说自签名证书会给用户带来虚假的安全感。

对于提到的临时密钥协商,一个可能的攻击方案是中间人分别对客户端以及服务端各自做一次ECDHE,这种情况下ECDHE无法提供确实的安全性。此外,假如引导用户无视TLS证书报警,就会消解用户对安全风险的警惕性,攻击者只需要诱导用户点击一次继续即可。

个人认为,AstrBot可以在未配置 TLS 时给出明确的安全风险提示,将选择权交给用户。

用户导入根证书就不会警告,就和其他权威根证书一样的效果,就不至于警告疲劳了。

@Raven95676
Copy link
Copy Markdown
Member

考虑到用户部署环境的差异性,TLS应作为可选配置而非强制要求。且TLS的安全性高度依赖可信CA,而自签名证书在多数场景下难以防范中间人攻击且用户体验不佳。

HTTPS中,证书只作为识别对端身份的手段,实际通讯密钥是通过临时密钥协商产生的。只要使用HTTPS,安全性就由临时密钥协商保证。而 MITM 成本比侦听明文 HTTP 流高多了。
正如我上面所说的,如果不强制执行TLS安全性,密钥就应该由客户端派生。

安全不应建立在“攻击者可能比较懒”的假设之上。我个人观点来说自签名证书会给用户带来虚假的安全感。
对于提到的临时密钥协商,一个可能的攻击方案是中间人分别对客户端以及服务端各自做一次ECDHE,这种情况下ECDHE无法提供确实的安全性。此外,假如引导用户无视TLS证书报警,就会消解用户对安全风险的警惕性,攻击者只需要诱导用户点击一次继续即可。
个人认为,AstrBot可以在未配置 TLS 时给出明确的安全风险提示,将选择权交给用户。

用户导入根证书就不会警告,就和其他权威根证书一样的效果,就不至于警告疲劳了。

对于用户来说,导入根证书的麻烦程度其实高于配置自动化申请权威证书(如acme.sh)。

自签名证书存在的合理场景是纯内网环境且无法访问外网,或者企业环境下的强制要求。

@AlanCui4080
Copy link
Copy Markdown

AlanCui4080 commented Apr 4, 2026

不建议自己手动实现密钥交换。

目前建议默认监听127.0.0.1和::1,并在用户试图更改监听地址时警告实施TLS的必要性,或者,可以在此时强制实施TLS。同时,在客户端执行持久化盐KDF并至少同时发送一个nonce并要求执行嵌套KDF。

即,服务器对每个用户保持一个盐,这个盐被发送到客户端,并生成一个nonce,发送到客户端。
客户端返回
kdf(password,salt),kdf(kdf(password,salt),nonce)

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

Labels

area:webui The bug / feature is about webui(dashboard) of astrbot. size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Insecured and unsalted hash algorithm used in WebUI

5 participants