Fix/plugin readme local assets#8360
Open
clown145 wants to merge 4 commits into
Open
Conversation
dosubot
Bot
added
size:L
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- For the
/api/plugin/assetendpoint, consider avoiding passing the JWT via atokenquery parameter (used inReadmeDialog.vue) and instead reuse the existing auth mechanism (e.g., cookie orAuthorizationheader), since tokens in URLs are more likely to be logged or leaked via referrers and browser history.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- For the `/api/plugin/asset` endpoint, consider avoiding passing the JWT via a `token` query parameter (used in `ReadmeDialog.vue`) and instead reuse the existing auth mechanism (e.g., cookie or `Authorization` header), since tokens in URLs are more likely to be logged or leaked via referrers and browser history.
## Individual Comments
### Comment 1
<location path="dashboard/src/components/shared/ReadmeDialog.vue" line_range="259-260" />
<code_context>
+ name: props.pluginName,
+ path: decodedPath,
+ });
+ const token = localStorage.getItem("token");
+ if (token) params.set("token", token);
+ return `/api/plugin/asset?${params.toString()}`;
+}
</code_context>
<issue_to_address>
**🚨 issue (security):** Avoid putting the auth token into image URLs if possible
Putting the dashboard JWT in the `<img>` `src` makes it more likely to leak via browser caches, server logs, referrers, and copy‑pasted URLs. If possible, have `/api/plugin/asset` rely on existing cookie/session auth (headers instead of query params), or use a short‑lived asset-specific token rather than the main dashboard token in the URL.
</issue_to_address>
### Comment 2
<location path="astrbot/dashboard/server.py" line_range="267-268" />
<code_context>
return None
is_plugin_page_path = PluginPageAuth.is_protected_path(request.path)
token = self._extract_dashboard_jwt()
+ if not token and request.path == "/api/plugin/asset":
+ token = request.args.get("token", "").strip()
if not token and is_plugin_page_path:
token = PluginPageAuth.extract_asset_token()
</code_context>
<issue_to_address>
**🚨 issue (security):** Using JWT from query string for `/api/plugin/asset` raises security and logging concerns
Passing the dashboard JWT as a `token` query param exposes it to logs, browser history, and referrers, increasing the risk of leakage or accidental sharing. Since this endpoint should be protected like the rest of the dashboard, prefer the existing auth mechanism (cookies/headers) or a dedicated short-lived/scope-limited token instead of reusing the main JWT in the URL.
</issue_to_address>
### Comment 3
<location path="dashboard/src/components/shared/PluginReadmeImageSourceSetting.vue" line_range="8" />
<code_context>
+ class="readme-image-source-switch"
+ color="primary"
+ density="compact"
+ hide-details="true"
+ :label="tm('network.proxySelector.readmeImages.useGitHub')">
+ </v-switch>
</code_context>
<issue_to_address>
**nitpick:** Use a boolean binding for `hide-details` instead of a string literal
`hide-details` is a boolean prop (or can be the special value `"auto"`). Using `hide-details="true"` passes a string, not a boolean. Prefer `:hide-details="true"` or just `hide-details` to keep the typing and behavior correct and explicit.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+259
to
+260
| const token = localStorage.getItem("token"); | ||
| if (token) params.set("token", token); |
Contributor
There was a problem hiding this comment.
🚨 issue (security): Avoid putting the auth token into image URLs if possible
Putting the dashboard JWT in the <img> src makes it more likely to leak via browser caches, server logs, referrers, and copy‑pasted URLs. If possible, have /api/plugin/asset rely on existing cookie/session auth (headers instead of query params), or use a short‑lived asset-specific token rather than the main dashboard token in the URL.
Comment on lines
+267
to
+268
| if not token and request.path == "/api/plugin/asset": | ||
| token = request.args.get("token", "").strip() |
Contributor
There was a problem hiding this comment.
🚨 issue (security): Using JWT from query string for /api/plugin/asset raises security and logging concerns
Passing the dashboard JWT as a token query param exposes it to logs, browser history, and referrers, increasing the risk of leakage or accidental sharing. Since this endpoint should be protected like the rest of the dashboard, prefer the existing auth mechanism (cookies/headers) or a dedicated short-lived/scope-limited token instead of reusing the main JWT in the URL.
| class="readme-image-source-switch" | ||
| color="primary" | ||
| density="compact" | ||
| hide-details="true" |
Contributor
There was a problem hiding this comment.
nitpick: Use a boolean binding for hide-details instead of a string literal
hide-details is a boolean prop (or can be the special value "auto"). Using hide-details="true" passes a string, not a boolean. Prefer :hide-details="true" or just hide-details to keep the typing and behavior correct and explicit.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to load plugin README images either locally or from GitHub, including a toggle setting in the dashboard and backend routing to safely serve local assets. The review feedback highlights opportunities to improve security and code quality: specifically, adding stricter validation to GitHub repository URL parsing to prevent potential directory traversal, setting a Content-Security-Policy header on served assets to mitigate XSS risks from SVGs, and refactoring the new Vue component to use the modern <script setup> syntax and Composition API.
Comment on lines
+217
to
+240
| def _parse_github_repo_url(self, repo_url: str | None) -> tuple[str, str] | None: | ||
| if not repo_url: | ||
| return None | ||
|
|
||
| parsed = urlsplit(repo_url.strip()) | ||
| if parsed.scheme not in ("http", "https"): | ||
| return None | ||
| if parsed.netloc.lower() != "github.com": | ||
| return None | ||
|
|
||
| parts = [part for part in parsed.path.strip("/").split("/") if part] | ||
| if len(parts) < 2: | ||
| return None | ||
|
|
||
| owner = parts[0] | ||
| repo = parts[1].removesuffix(".git") | ||
| if not owner or not repo: | ||
| return None | ||
| if not GITHUB_REPO_PART_PATTERN.fullmatch(owner): | ||
| return None | ||
| if not GITHUB_REPO_PART_PATTERN.fullmatch(repo): | ||
| return None | ||
|
|
||
| return owner, repo |
Contributor
There was a problem hiding this comment.
为了提高代码的健壮性与安全性，建议在此处进行以下改进：
- 增加对
repo_url的类型检查，确保其为str类型，避免在传入非字符串类型时调用.strip()导致AttributeError。 - 显式地将
.和..排除在合法的owner或repo之外，防止潜在的目录穿越或不合规的 URL 解析问题。
def _parse_github_repo_url(self, repo_url: str | None) -> tuple[str, str] | None:
if not isinstance(repo_url, str):
return None
parsed = urlsplit(repo_url.strip())
if parsed.scheme not in ("http", "https"):
return None
if parsed.netloc.lower() != "github.com":
return None
parts = [part for part in parsed.path.strip("/").split("/") if part]
if len(parts) < 2:
return None
owner = parts[0]
repo = parts[1].removesuffix(".git")
if not owner or not repo:
return None
if owner in (".", "..") or repo in (".", ".."):
return None
if not GITHUB_REPO_PART_PATTERN.fullmatch(owner):
return None
if not GITHUB_REPO_PART_PATTERN.fullmatch(repo):
return None
return owner, repo
Comment on lines
+391
to
+412
| async def get_plugin_asset(self): | ||
| plugin_name = request.args.get("name") | ||
| asset_path = request.args.get("path") | ||
|
|
||
| if not plugin_name or not asset_path: | ||
| return await self._plugin_page_error_response(404, "Plugin asset not found") | ||
|
|
||
| plugin = self._get_plugin_metadata_by_name(plugin_name) | ||
| if not plugin: | ||
| return await self._plugin_page_error_response(404, "Plugin not found") | ||
|
|
||
| try: | ||
| file_path = await self._resolve_plugin_asset_file(plugin, asset_path) | ||
| except (FileNotFoundError, ValueError, OSError): | ||
| logger.warning(f"插件资源访问失败: {plugin_name}/{asset_path}") | ||
| return await self._plugin_page_error_response(404, "Plugin asset not found") | ||
|
|
||
| mimetype, _ = mimetypes.guess_type(file_path.name) | ||
| if not mimetype or not mimetype.startswith(PLUGIN_ASSET_MIME_PREFIX): | ||
| return await self._plugin_page_error_response(404, "Plugin asset not found") | ||
|
|
||
| return await self._serve_plugin_page_static_asset(file_path) |
Contributor
There was a problem hiding this comment.
为了防止潜在的跨站脚本攻击（XSS），特别是当服务中允许加载并直接在浏览器中打开 SVG 格式的图片时，建议为该接口的响应头部添加严格的 Content-Security-Policy。
通过设置 Content-Security-Policy: default-src 'none'，可以确保即使恶意的 SVG 图片中包含 <script> 标签，在浏览器中被直接打开时也无法执行任何脚本。
async def get_plugin_asset(self):
plugin_name = request.args.get("name")
asset_path = request.args.get("path")
if not plugin_name or not asset_path:
return await self._plugin_page_error_response(404, "Plugin asset not found")
plugin = self._get_plugin_metadata_by_name(plugin_name)
if not plugin:
return await self._plugin_page_error_response(404, "Plugin not found")
try:
file_path = await self._resolve_plugin_asset_file(plugin, asset_path)
except (FileNotFoundError, ValueError, OSError):
logger.warning(f"插件资源访问失败: {plugin_name}/{asset_path}")
return await self._plugin_page_error_response(404, "Plugin asset not found")
mimetype, _ = mimetypes.guess_type(file_path.name)
if not mimetype or not mimetype.startswith(PLUGIN_ASSET_MIME_PREFIX):
return await self._plugin_page_error_response(404, "Plugin asset not found")
response = await self._serve_plugin_page_static_asset(file_path)
response.headers["Content-Security-Policy"] = "default-src 'none'"
return response
Comment on lines
+17
to
+61
| <script> | ||
| import { useModuleI18n } from '@/i18n/composables'; | ||
| import { | ||
| PLUGIN_README_IMAGE_SOURCE, | ||
| getPluginReadmeImageSource, | ||
| setPluginReadmeImageSource | ||
| } from '@/utils/githubProxy'; | ||
|
|
||
| export default { | ||
| setup() { | ||
| const { tm } = useModuleI18n('features/settings'); | ||
| return { tm }; | ||
| }, | ||
| data() { | ||
| return { | ||
| readmeImageSource: PLUGIN_README_IMAGE_SOURCE.LOCAL, | ||
| initializing: true, | ||
| } | ||
| }, | ||
| computed: { | ||
| readmeImageUseGitHub: { | ||
| get() { | ||
| return this.readmeImageSource === PLUGIN_README_IMAGE_SOURCE.GITHUB; | ||
| }, | ||
| set(value) { | ||
| this.readmeImageSource = value | ||
| ? PLUGIN_README_IMAGE_SOURCE.GITHUB | ||
| : PLUGIN_README_IMAGE_SOURCE.LOCAL; | ||
| } | ||
| } | ||
| }, | ||
| mounted() { | ||
| this.readmeImageSource = getPluginReadmeImageSource(); | ||
| this.initializing = false; | ||
| }, | ||
| watch: { | ||
| readmeImageSource: function (newVal) { | ||
| if (this.initializing) { | ||
| return; | ||
| } | ||
| setPluginReadmeImageSource(newVal); | ||
| } | ||
| } | ||
| } | ||
| </script> |
Contributor
There was a problem hiding this comment.
为了与项目中的其他组件（如 ReadmeDialog.vue 和 Settings.vue）保持一致，建议将此组件重构为 Vue 3 推荐的 <script setup> 语法和 Composition API。
这不仅能使代码更加简洁、易读，还能利用 watch 的默认行为（在初始化时不触发），从而优雅地省去 initializing 状态和 mounted 生命周期的手动控制逻辑。
<script setup>
import { ref, watch, computed } from 'vue';
import { useModuleI18n } from '@/i18n/composables';
import {
PLUGIN_README_IMAGE_SOURCE,
getPluginReadmeImageSource,
setPluginReadmeImageSource
} from '@/utils/githubProxy';
const { tm } = useModuleI18n('features/settings');
const readmeImageSource = ref(getPluginReadmeImageSource());
const readmeImageUseGitHub = computed({
get() {
return readmeImageSource.value === PLUGIN_README_IMAGE_SOURCE.GITHUB;
},
set(value) {
readmeImageSource.value = value
? PLUGIN_README_IMAGE_SOURCE.GITHUB
: PLUGIN_README_IMAGE_SOURCE.LOCAL;
}
});
watch(readmeImageSource, (newVal) => {
setPluginReadmeImageSource(newVal);
});
</script>
Soulter
force-pushed
the
master
branch
3 times, most recently
from
a4c4a7d to
9bd38ca
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
插件 README 中使用相对路径引用的图片，之前在 WebUI 中不一定能正常加载。对于把截图或说明文档资源放在插件仓库、插件目录内的插件，这会导致文档展示不完整。
此改动增加了一种更稳妥、也更照顾低上传带宽服务器的 README 图片加载方式：
Modifications / 改动点
github_raw_base，前端可基于 GitHub 默认分支构造 raw 图片链接，避免硬编码main或其他分支名。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
Improve plugin README image handling by serving local assets via a new API and allowing configurable GitHub-based loading with proxy support.
New Features:
Enhancements:
Tests: