fix(webui): enforce 12-char dashboard password policy with backend+frontend validation#7338
fix(webui): enforce 12-char dashboard password policy with backend+frontend validation#7338
Conversation
…ontend validation
There was a problem hiding this comment.
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>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()) |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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.UNKNOWNThen 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 FalseThis 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.
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
It's better to enforce user to update the password by a specially constructed function, rather than being inserted into the validation stream.
There was a problem hiding this comment.
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) => { |
There was a problem hiding this comment.
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.
|
个人推荐通过argon2-cffi使用Argon2id |
是的,PBKDF2 在今天被视为是可接受的,除非你需要通过FIPS认证,不然应该迁移到更新的KDF。但是我认为,强制执行 TLS 才是提升安全性的重中之重,否则双方应该通过某种交互式零知识证明交换 PIN。如果强制执行了 TLS,那么弱一些的 KDF 仍然可以保护密码免受明文恢复攻击,至少他们所基于的哈希算法单射特性十分好。 |
|
可以参考 Secure Remote Password Protocol 。 |
|
https就好了 |
在原本的实现中,MD5哈希极易遭受彩虹表攻击,会导致用户密码明文被恢复 如果采取当前 PR ,HTTPS 必须被强制执行,否则明文密码会被发送到服务端。或者,可以将哈希转移到客户端进行。或者,可以执行密码认证的密钥交换。 |
9ab169d to
b749f62
Compare
考虑到用户部署环境的差异性,TLS应作为可选配置而非强制要求。且TLS的安全性高度依赖可信CA,而自签名证书在多数场景下难以防范中间人攻击且用户体验不佳。 至于交互式零知识证明,其部署难度高于TLS,在现实中只能作为TLS的强安全替代方案,且在绝大多数通用业务场景下并不需要如此之高的安全性。 |
|
不是所有人都是远程部署的 |
|
而且https需要购买域名 |
HTTPS中,证书只作为识别对端身份的手段,实际通讯密钥是通过临时密钥协商产生的。只要使用HTTPS,安全性就由临时密钥协商保证。而 MITM 成本比侦听明文 HTTP 流高多了。 正如我上面所说的,如果不强制执行TLS安全性,密钥就应该由客户端派生。 |
|
自签名也挺安全,但是麻烦啊 |
21bff02 to
b749f62
Compare
可以使用自签证书,Chrome在首次信任某个自签证书后会保留他的指纹,如果指纹变更会重新触发警告页面,事实上,大部分实现都是服务端自签的。
自建CA和自签证书是两个概念。对于自建根CA,Android应用可以选择相信用户存储区的证书,Chrome也默认信任用户存储区,这是不需要root的。 |
|
|
确实 |
|
设置两环境变量就启用https了,挺方便的 |
安全不应建立在“攻击者可能比较懒”的假设之上。我个人观点来说自签名证书会给用户带来虚假的安全感。 对于提到的临时密钥协商,一个可能的攻击方案是中间人分别对客户端以及服务端各自做一次ECDHE,这种情况下ECDHE无法提供确实的安全性。此外,假如引导用户无视TLS证书报警,就会消解用户对安全风险的警惕性,攻击者只需要诱导用户点击一次继续即可。 个人认为,AstrBot可以在未配置 TLS 时给出明确的安全风险提示,将选择权交给用户。 |
确实,这个实现对于用户来说几乎是无感的 |
用户导入根证书就不会警告,就和其他权威根证书一样的效果,就不至于警告疲劳了。 |
对于用户来说,导入根证书的麻烦程度其实高于配置自动化申请权威证书(如acme.sh)。 自签名证书存在的合理场景是纯内网环境且无法访问外网,或者企业环境下的强制要求。 |
|
不建议自己手动实现密钥交换。 目前建议默认监听127.0.0.1和::1,并在用户试图更改监听地址时警告实施TLS的必要性,或者,可以在此时强制实施TLS。同时,在客户端执行持久化盐KDF并至少同时发送一个nonce并要求执行嵌套KDF。 即,服务器对每个用户保持一个盐,这个盐被发送到客户端,并生成一个nonce,发送到客户端。 |


closes #7321
Summary