Conversation
dosubot
bot
added
the
size:L
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/dashboard/server.py" line_range="303-312" />
<code_context>
+ @staticmethod
</code_context>
<issue_to_address>
**🚨 question (security):** Changing SSL misconfiguration from hard failure to warning+HTTP fallback significantly alters security posture.
Previously, missing/invalid `cert_file`, `key_file`, or `ca_certs` raised `ValueError` and stopped startup; now `_resolve_dashboard_ssl_config` only logs a warning and returns `(False, {})`, allowing the server to run over HTTP instead. This can silently downgrade a deployment that expects TLS to be mandatory. Consider adding an explicit mode (e.g., `require_ssl` flag or env var) that preserves the old fail-fast behavior for environments where TLS must not be bypassed.
</issue_to_address>
### Comment 2
<location path="tests/test_dashboard.py" line_range="112-121" />
<code_context>
+async def test_dashboard_ssl_missing_cert_and_key_falls_back_to_http(
</code_context>
<issue_to_address>
**issue (testing):** This test can be flaky/misleading if SSL-related environment variables are set in the test environment.
`_resolve_dashboard_ssl_config` prefers environment variables (`DASHBOARD_SSL_CERT` / `ASTRBOT_DASHBOARD_SSL_CERT`, `DASHBOARD_SSL_KEY` / `ASTRBOT_DASHBOARD_SSL_KEY`) over the config values used in this test. Since the test doesn’t clear these env vars, a CI or local shell setting could cause SSL to be enabled and `certfile`/`keyfile` to be non-`None`, breaking the assertions. Please explicitly control these env vars in the test (e.g. with `monkeypatch.delenv(..., raising=False)` or `monkeypatch.setenv(...)`) so the test is isolated from the external environment.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @staticmethod | ||
| def _resolve_dashboard_ssl_config( | ||
| ssl_config: dict, | ||
| ) -> tuple[bool, dict[str, str]]: | ||
| cert_file = ( | ||
| os.environ.get("DASHBOARD_SSL_CERT") | ||
| or os.environ.get("ASTRBOT_DASHBOARD_SSL_CERT") | ||
| or ssl_config.get("cert_file", "") | ||
| ) | ||
| key_file = ( |
There was a problem hiding this comment.
🚨 question (security): Changing SSL misconfiguration from hard failure to warning+HTTP fallback significantly alters security posture.
Previously, missing/invalid cert_file, key_file, or ca_certs raised ValueError and stopped startup; now _resolve_dashboard_ssl_config only logs a warning and returns (False, {}), allowing the server to run over HTTP instead. This can silently downgrade a deployment that expects TLS to be mandatory. Consider adding an explicit mode (e.g., require_ssl flag or env var) that preserves the old fail-fast behavior for environments where TLS must not be bypassed.
| async def test_dashboard_ssl_missing_cert_and_key_falls_back_to_http( | ||
| core_lifecycle_td: AstrBotCoreLifecycle, | ||
| monkeypatch, | ||
| ): | ||
| shutdown_event = asyncio.Event() | ||
| server = AstrBotDashboard(core_lifecycle_td, core_lifecycle_td.db, shutdown_event) | ||
| original_dashboard_config = copy.deepcopy( | ||
| core_lifecycle_td.astrbot_config.get("dashboard", {}), | ||
| ) | ||
| warning_messages = [] |
There was a problem hiding this comment.
issue (testing): This test can be flaky/misleading if SSL-related environment variables are set in the test environment.
_resolve_dashboard_ssl_config prefers environment variables (DASHBOARD_SSL_CERT / ASTRBOT_DASHBOARD_SSL_CERT, DASHBOARD_SSL_KEY / ASTRBOT_DASHBOARD_SSL_KEY) over the config values used in this test. Since the test doesn’t clear these env vars, a CI or local shell setting could cause SSL to be enabled and certfile/keyfile to be non-None, breaking the assertions. Please explicitly control these env vars in the test (e.g. with monkeypatch.delenv(..., raising=False) or monkeypatch.setenv(...)) so the test is isolated from the external environment.
dosubot
bot
added
the
area:webui
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the dashboard's SSL configuration logic into a dedicated helper method, _resolve_dashboard_ssl_config. This change updates the server to log a warning and fall back to HTTP when SSL files are missing or invalid, rather than raising a ValueError. A new test case has been added to verify this fallback behavior. Feedback was provided to refactor the new helper method to reduce code duplication in the path validation logic.
| def _resolve_dashboard_ssl_config( | ||
| ssl_config: dict, | ||
| ) -> tuple[bool, dict[str, str]]: | ||
| cert_file = ( | ||
| os.environ.get("DASHBOARD_SSL_CERT") | ||
| or os.environ.get("ASTRBOT_DASHBOARD_SSL_CERT") | ||
| or ssl_config.get("cert_file", "") | ||
| ) | ||
| key_file = ( | ||
| os.environ.get("DASHBOARD_SSL_KEY") | ||
| or os.environ.get("ASTRBOT_DASHBOARD_SSL_KEY") | ||
| or ssl_config.get("key_file", "") | ||
| ) | ||
| ca_certs = ( | ||
| os.environ.get("DASHBOARD_SSL_CA_CERTS") | ||
| or os.environ.get("ASTRBOT_DASHBOARD_SSL_CA_CERTS") | ||
| or ssl_config.get("ca_certs", "") | ||
| ) | ||
|
|
||
| if not cert_file or not key_file: | ||
| logger.warning( | ||
| "dashboard.ssl.enable 已启用，但未同时配置 cert_file 和 key_file，SSL 配置将不会生效。", | ||
| ) | ||
| return False, {} | ||
|
|
||
| cert_path = Path(cert_file).expanduser() | ||
| key_path = Path(key_file).expanduser() | ||
| if not cert_path.is_file(): | ||
| logger.warning( | ||
| f"dashboard.ssl.enable 已启用，但 SSL 证书文件不存在: {cert_path}，SSL 配置将不会生效。", | ||
| ) | ||
| return False, {} | ||
| if not key_path.is_file(): | ||
| logger.warning( | ||
| f"dashboard.ssl.enable 已启用，但 SSL 私钥文件不存在: {key_path}，SSL 配置将不会生效。", | ||
| ) | ||
| return False, {} | ||
|
|
||
| resolved_ssl_config = { | ||
| "certfile": str(cert_path.resolve()), | ||
| "keyfile": str(key_path.resolve()), | ||
| } | ||
|
|
||
| if ca_certs: | ||
| ca_path = Path(ca_certs).expanduser() | ||
| if not ca_path.is_file(): | ||
| logger.warning( | ||
| f"dashboard.ssl.enable 已启用，但 SSL CA 证书文件不存在: {ca_path}，SSL 配置将不会生效。", | ||
| ) | ||
| return False, {} | ||
| resolved_ssl_config["ca_certs"] = str(ca_path.resolve()) | ||
|
|
||
| return True, resolved_ssl_config |
There was a problem hiding this comment.
The method _resolve_dashboard_ssl_config contains some repetitive logic for validating file paths. This can be refactored to improve readability and maintainability by extracting the validation logic into a nested helper function.
def _resolve_dashboard_ssl_config(
ssl_config: dict,
) -> tuple[bool, dict[str, str]]:
def _validate_path(path_str: str, description: str) -> Path | None:
if not path_str:
return None
path = Path(path_str).expanduser()
if not path.is_file():
logger.warning(
f"dashboard.ssl.enable 已启用，但 SSL {description} 不存在: {path}，SSL 配置将不会生效。"
)
return None
return path
cert_file = (
os.environ.get("DASHBOARD_SSL_CERT")
or os.environ.get("ASTRBOT_DASHBOARD_SSL_CERT")
or ssl_config.get("cert_file", "")
)
key_file = (
os.environ.get("DASHBOARD_SSL_KEY")
or os.environ.get("ASTRBOT_DASHBOARD_SSL_KEY")
or ssl_config.get("key_file", "")
)
if not cert_file or not key_file:
logger.warning(
"dashboard.ssl.enable 已启用，但未同时配置 cert_file 和 key_file，SSL 配置将不会生效。"
)
return False, {}
cert_path = _validate_path(cert_file, "证书文件")
key_path = _validate_path(key_file, "私钥文件")
if not cert_path or not key_path:
return False, {}
resolved_ssl_config = {
"certfile": str(cert_path.resolve()),
"keyfile": str(key_path.resolve()),
}
ca_certs = (
os.environ.get("DASHBOARD_SSL_CA_CERTS")
or os.environ.get("ASTRBOT_DASHBOARD_SSL_CA_CERTS")
or ssl_config.get("ca_certs", "")
)
if ca_certs:
ca_path = _validate_path(ca_certs, "CA 证书文件")
if not ca_path:
return False, {}
resolved_ssl_config["ca_certs"] = str(ca_path.resolve())
return True, resolved_ssl_config1 participant
fixes: #7058
Modifications / 改动点
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能，已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试，并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库，或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Add centralized SSL configuration resolution for the dashboard and ensure the server gracefully falls back to HTTP when SSL is misconfigured.
New Features:
Bug Fixes:
Tests: