feat: add ChatUI project workspaces#9066
Conversation
dosubot
Bot
added
size:XXL
![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 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/tools/computer_tools/fs.py" line_range="80" />
<code_context>
-def _restricted_env_path_labels(umo: str, *, include_plugin_skills: bool) -> list[str]:
+def _restricted_env_path_labels(
+ umo: str,
+ *,
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing a single effective workspace root helper, passing a concrete root instead of threading optional state, and splitting path normalization from authorization to simplify the filesystem tooling logic.
You can keep the new “current workspace” behavior but reduce complexity by centralizing workspace resolution and separating concerns a bit.
### 1. Centralize workspace resolution
Instead of repeating `(current_workspace_root or _workspace_root(umo))` in multiple helpers, introduce one small helper:
```python
def _effective_workspace_root(umo: str, current_workspace_root: Path | None) -> Path:
return (current_workspace_root or _workspace_root(umo)).resolve(strict=False)
```
Then in helpers that currently accept both `umo` and `current_workspace_root`, call this once:
```python
def _read_allowed_roots(umo: str, current_workspace_root: Path | None) -> tuple[Path, ...]:
workspace_root = _effective_workspace_root(umo, current_workspace_root)
return (
Path(get_astrbot_skills_path()).resolve(strict=False),
*_plugin_skill_roots(),
workspace_root,
Path(get_astrbot_system_tmp_path()).resolve(strict=False),
Path(get_astrbot_temp_path()).resolve(strict=False),
)
```
Apply the same pattern to `_write_allowed_roots`, `_restricted_env_path_labels`, `_resolve_tool_path`, `_resolve_user_path`, `_is_path_within_allowed_roots`, etc., instead of open-coding the `(current_workspace_root or _workspace_root(umo))` expression.
### 2. Pass a concrete `effective_root` instead of threading `current_workspace_root`
At the tool entrypoints, compute the effective root once and pass that down, rather than passing `umo` + `current_workspace_root` everywhere:
```python
async def call(...):
umo = context.context.event.unified_msg_origin
current_workspace_root = workspace_root_for_context(context) # existing helper
effective_root = _effective_workspace_root(umo, current_workspace_root)
normalized_path = (
_normalize_rw_path(
path,
restricted=restricted,
local_env=local_env,
workspace_root=effective_root,
write=True,
)
if local_env else path.strip()
)
```
Then change helpers to accept `workspace_root: Path` instead of both `umo` and `current_workspace_root`:
```python
def _resolve_tool_path(path: str, *, local_env: bool, workspace_root: Path) -> str:
normalized_path = path.strip()
if not normalized_path:
return normalized_path
candidate = Path(normalized_path).expanduser()
if candidate.is_absolute():
return str(candidate.resolve(strict=False))
if local_env:
return str((workspace_root / candidate).resolve(strict=False))
return normalized_path
```
This keeps the feature intact but removes optional-threading and makes call sites easier to follow.
### 3. Separate normalization and authorization
Right now `_normalize_rw_path` and `_normalize_search_paths` both resolve paths and enforce allowed-roots. Splitting them into two small helpers reduces parameter coupling:
```python
def _normalize_path_for_env(
path: str,
*,
local_env: bool,
workspace_root: Path,
) -> str:
return _resolve_tool_path(path, local_env=local_env, workspace_root=workspace_root)
def _ensure_rw_authorized(
normalized_path: str,
*,
restricted: bool,
workspace_root: Path,
write: bool,
) -> None:
if not restricted:
return
allowed_roots = (
_write_allowed_roots_for_root(workspace_root)
if write
else _read_allowed_roots_for_root(workspace_root)
)
if not _is_path_within_allowed_roots(normalized_path, allowed_roots=allowed_roots):
# build labels from workspace_root rather than recomputing
...
_reject_multi_link_file(normalized_path)
```
And then `_normalize_rw_path` becomes a slim orchestrator:
```python
def _normalize_rw_path(
path: str,
*,
restricted: bool,
local_env: bool,
workspace_root: Path,
write: bool = False,
) -> str:
normalized_path = _normalize_path_for_env(
path, local_env=local_env, workspace_root=workspace_root
)
if not normalized_path:
raise ValueError("`path` must be a non-empty string.")
_ensure_rw_authorized(
normalized_path,
restricted=restricted,
workspace_root=workspace_root,
write=write,
)
return normalized_path
```
This keeps all existing semantics (including current workspace resolution) while reducing branching and repeated resolution logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
|
|
||
| def _restricted_env_path_labels(umo: str, *, include_plugin_skills: bool) -> list[str]: | ||
| def _restricted_env_path_labels( |
There was a problem hiding this comment.
issue (complexity): Consider introducing a single effective workspace root helper, passing a concrete root instead of threading optional state, and splitting path normalization from authorization to simplify the filesystem tooling logic.
You can keep the new “current workspace” behavior but reduce complexity by centralizing workspace resolution and separating concerns a bit.
1. Centralize workspace resolution
Instead of repeating (current_workspace_root or _workspace_root(umo)) in multiple helpers, introduce one small helper:
def _effective_workspace_root(umo: str, current_workspace_root: Path | None) -> Path:
return (current_workspace_root or _workspace_root(umo)).resolve(strict=False)Then in helpers that currently accept both umo and current_workspace_root, call this once:
def _read_allowed_roots(umo: str, current_workspace_root: Path | None) -> tuple[Path, ...]:
workspace_root = _effective_workspace_root(umo, current_workspace_root)
return (
Path(get_astrbot_skills_path()).resolve(strict=False),
*_plugin_skill_roots(),
workspace_root,
Path(get_astrbot_system_tmp_path()).resolve(strict=False),
Path(get_astrbot_temp_path()).resolve(strict=False),
)Apply the same pattern to _write_allowed_roots, _restricted_env_path_labels, _resolve_tool_path, _resolve_user_path, _is_path_within_allowed_roots, etc., instead of open-coding the (current_workspace_root or _workspace_root(umo)) expression.
2. Pass a concrete effective_root instead of threading current_workspace_root
At the tool entrypoints, compute the effective root once and pass that down, rather than passing umo + current_workspace_root everywhere:
async def call(...):
umo = context.context.event.unified_msg_origin
current_workspace_root = workspace_root_for_context(context) # existing helper
effective_root = _effective_workspace_root(umo, current_workspace_root)
normalized_path = (
_normalize_rw_path(
path,
restricted=restricted,
local_env=local_env,
workspace_root=effective_root,
write=True,
)
if local_env else path.strip()
)Then change helpers to accept workspace_root: Path instead of both umo and current_workspace_root:
def _resolve_tool_path(path: str, *, local_env: bool, workspace_root: Path) -> str:
normalized_path = path.strip()
if not normalized_path:
return normalized_path
candidate = Path(normalized_path).expanduser()
if candidate.is_absolute():
return str(candidate.resolve(strict=False))
if local_env:
return str((workspace_root / candidate).resolve(strict=False))
return normalized_pathThis keeps the feature intact but removes optional-threading and makes call sites easier to follow.
3. Separate normalization and authorization
Right now _normalize_rw_path and _normalize_search_paths both resolve paths and enforce allowed-roots. Splitting them into two small helpers reduces parameter coupling:
def _normalize_path_for_env(
path: str,
*,
local_env: bool,
workspace_root: Path,
) -> str:
return _resolve_tool_path(path, local_env=local_env, workspace_root=workspace_root)
def _ensure_rw_authorized(
normalized_path: str,
*,
restricted: bool,
workspace_root: Path,
write: bool,
) -> None:
if not restricted:
return
allowed_roots = (
_write_allowed_roots_for_root(workspace_root)
if write
else _read_allowed_roots_for_root(workspace_root)
)
if not _is_path_within_allowed_roots(normalized_path, allowed_roots=allowed_roots):
# build labels from workspace_root rather than recomputing
...
_reject_multi_link_file(normalized_path)And then _normalize_rw_path becomes a slim orchestrator:
def _normalize_rw_path(
path: str,
*,
restricted: bool,
local_env: bool,
workspace_root: Path,
write: bool = False,
) -> str:
normalized_path = _normalize_path_for_env(
path, local_env=local_env, workspace_root=workspace_root
)
if not normalized_path:
raise ValueError("`path` must be a non-empty string.")
_ensure_rw_authorized(
normalized_path,
restricted=restricted,
workspace_root=workspace_root,
write=write,
)
return normalized_pathThis keeps all existing semantics (including current workspace resolution) while reducing branching and repeated resolution logic.
![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 introduces support for project-level and custom workspaces in AstrBot, updating the database schema, backend path resolution logic, and the frontend dashboard to allow users to configure and view shared or custom workspace paths. The review feedback highlights a critical security vulnerability in workspace_path_to_root where path traversal or arbitrary absolute paths could bypass the sandbox and allow unauthorized file access. Additionally, a potential TypeError was identified in the frontend ProjectDialog.vue when calling .trim() on a potentially null or undefined workspace_path.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def workspace_path_to_root(path: str) -> Path: | ||
| """Resolve a custom workspace path. | ||
|
|
||
| Args: | ||
| path: Stored workspace path. Relative values are rooted under AstrBot | ||
| workspaces. | ||
|
|
||
| Returns: | ||
| Absolute resolved path. | ||
| """ | ||
| candidate = Path(path).expanduser() | ||
| if not candidate.is_absolute(): | ||
| candidate = Path(get_astrbot_workspaces_path()) / candidate | ||
| return candidate.resolve(strict=False) |
There was a problem hiding this comment.
Security Vulnerability: Path Traversal and Arbitrary File Access
The current implementation of workspace_path_to_root is vulnerable to path traversal and arbitrary file access:
- Relative Path Traversal: If a user provides a relative path containing directory traversal sequences (e.g.,
../../etc),Path(get_astrbot_workspaces_path()) / candidatewill resolve to a path outside the workspaces directory. - Arbitrary Absolute Paths: Any user can specify an absolute path (e.g.,
/etcor/home/user/.ssh), allowing them to configure any directory on the host system as their project workspace. Since non-admin users are allowed to read/write within their resolved workspace, this completely bypasses the restricted environment sandbox and allows unauthorized access to sensitive system files.
Recommendation:
- For relative paths, ensure the resolved path is strictly within the workspaces directory using
is_relative_to. - For absolute paths, restrict them to admin users only, or disallow them entirely for project workspaces.
def workspace_path_to_root(path: str) -> Path:
"""Resolve a custom workspace path.
Args:
path: Stored workspace path. Relative values are rooted under AstrBot
workspaces.
Returns:
Absolute resolved path.
"""
candidate = Path(path).expanduser()
workspaces_path = Path(get_astrbot_workspaces_path()).resolve()
if not candidate.is_absolute():
candidate = workspaces_path / candidate
resolved = candidate.resolve(strict=False)
if not resolved.is_relative_to(workspaces_path):
raise ValueError("Path traversal detected: path escapes workspaces directory")
return resolved| const canSave = computed(() => { | ||
| if (!form.value.title.trim()) return false; | ||
| if (form.value.workspace_type !== 'custom') return true; | ||
| return form.value.workspace_path.trim().length > 0; | ||
| }); |
There was a problem hiding this comment.
Defensive Programming: Prevent potential TypeError on null/undefined workspace_path
If form.value.workspace_path is somehow set to null or undefined (e.g., if cleared or through binding), calling .trim() directly on it will throw a TypeError. Using a fallback empty string (form.value.workspace_path || '') ensures robustness.
const canSave = computed(() => {
if (!form.value.title.trim()) return false;
if (form.value.workspace_type !== 'custom') return true;
return (form.value.workspace_path || '').trim().length > 0;
});
Soulter
force-pushed
the
codex/chatui-project-workspaces
branch
from
46ac71a to
c8996b4
Compare
1 participant
Summary
Tests
Summary by Sourcery
Introduce workspace-aware ChatUI projects with improved header, sidebar, and mobile chat layout while routing local tools through the resolved workspace.
New Features:
Enhancements:
Tests: