Skip to content

fix(workspace): PUT /file 与 POST /upload 补上 _builtin_skills 写保护 (#1100) - #1101

Merged
jubaoliang merged 2 commits into
TencentCloud:developfrom
sxh313:fix-builtin-skills-write-guard
Sep 24, 2026
Merged

jubaoliang merged 2 commits into
TencentCloud:developfrom
sxh313:fix-builtin-skills-write-guard

Conversation

@sxh313

@sxh313 sxh313 commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes #1100

src/octop/api/routers/workspace.py 里保护 Octop 内置技能根 _builtin_skills 的校验函数 _assert_workspace_mutable()(:39-52)已被 4 个写接口调用(mkdir :230、delete :260、move :289-290、文档 PUT /doc :408),但 PUT /workspace/file(:171)和 POST /workspace/upload(:302)漏了。后果不是「多写一个文件」:

  • 写进 _builtin_skills/<name>/SKILL.md 的内容会被 api/routers/skills.py:185-199 _resolve_skill() 当作 kind="builtin" 列进技能列表(infra/agents/manager.py:2291 同口径),用户内容以「Octop 内置技能」的身份出现;
  • 之后删不掉:workspace 的 delete / move 对该前缀一律 403,技能删除走 skills.py:1049-1050 拒绝 builtin;builtin_skills/__init__.py:70-73 的重启同步只清理 RETIRED_BUILTIN_SKILLS 白名单,不会移除多出来的目录;
  • dashboard 本来就认为这些路径不可写(WorkspaceDrawer.tsx:118-126 isProtectedPath()、:137 canCreateIn()、:531 与拖拽 :627/:668),只有后端放行。

改动是两行调用,位置与其余写接口一致(先校验再取 workspace)。upload 里把 target = path or f"/{file.filename}" 提到校验之前,校验最终真正使用的路径,并且先于 await file.read(),被拒的请求不必把请求体读进内存。

刻意没做的:

  • 不改这两个接口的路径解析口径(仍走 _workspace_io_path(path, from_workspace=...)),避免顺手改掉 from_workspace 的契约;
  • 不碰 POST /workspace/archive(zip 整包导入 / 替换),它是否该过滤 _builtin_skills 是产品决定;
  • 不碰 skills.py 的「builtin 不可删」逻辑。

Target branch

  • Base is develop (feature / fix — default)
  • Base is main (release/* or hotfix/* only)

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation
  • Refactor / chore
  • Release / hotfix

Test plan

本机是中文 Windows(cp936),make 不在 PATH,所以按 make all 的组成逐项执行;SKIP_PRECOMMIT=1 只跳过了钩子本身,钩子覆盖的检查全跑过(见下)。

  • 红→绿(真实 in-process 服务 + 真实 agent workspace,无 mock):

    • pytest tests/integration/test_workspace_api.py -k builtin_skills 打在未改动的 develop 源码上:3 failed, 1 passed in 46.81s,三条失败都是 assert 200 == 403(PUT 的 /_builtin_skills/...、/.octop/_builtin_skills/...,以及 POST /workspace/upload);已有的 test_delete_builtin_skills_forbidden 本来就通过。
    • 加上校验后同一文件全量:27 passed in 97.72s(24 项既有 + 3 项新增),说明既有的写入 / 上传 / 下载 / 文档往返用例没有被新校验打断。
  • 邻近回归:tests/integration/test_agents_shared.py tests/integration/test_subagents_api.py tests/integration/test_skills_api.py → 56 passed in 236.34s

  • 缺陷链实测(未改动的 develop @ 02a6e28d):

    PUT=200 {"path":"/_builtin_skills/evil-injected/SKILL.md","size":55}
    SKILLS_LIST_STATUS= 200
    SKILLS_ROWS= [('evil-injected', 'builtin'), ('<真正的内置技能>', 'builtin')]
    DELETE_AFTER= 403
    SKILL_DELETE_API= 404 {"error":{"code":"NOT_FOUND","details":{}}}
    
  • from_workspace=false 的绕过探测(同一套夹具,Windows):把 host 绝对路径交给 PUT /workspace/file → STATUS=404、目标文件 EXISTS=False,即后端本来就不落在 workspace 之外;加了校验后,对该前缀的 host 绝对写法返回 403。所以「不解析口径」不影响这条保护生效。

  • 静态检查:ruff check src tests → All checks passed;ruff format --check src tests → 1060 files already formatted;mypy --strict src/octop → Success: no issues found in 517 source files。

  • 全量套件:pytest -n auto -m "not live" → 20 failed, 3651 passed, 123 skipped, 9 warnings in 2280.49s。20 个失败全部落在 tests/unit/db / tests/unit/backup / tests/unit/gateway 里「按当前代码页读迁移 SQL」那一类既有问题(正是 test(db): 迁移脚本统一按 UTF-8 读,中文 Windows(cp936) 本地跑测试不再红 19 个 (#1070) #1072 / test: 读迁移 SQL 的 22 处 read_text 钉住 utf-8,让整套 tests/unit 不再依赖系统代码页 (#1062) #1073 在修的那批,中文 Windows 才有,CI 的 Linux/Windows 上是绿的),与本改动无交集;本 PR 涉及的 tests/integration/test_workspace_api.py 在全量里也全绿。

  • make all passes locally

  • Added/updated tests

Checklist

  • Updated CHANGELOG.md (if user-facing)
  • README / docs updated (if needed)

…entCloud#1100)

同一文件里 mkdir / delete / move / PUT /doc 都用 _assert_workspace_mutable
拦下 Octop 内置技能根 _builtin_skills,只有这两个写接口漏了:owner 可以往
_builtin_skills/<name>/SKILL.md 写内容,_resolve_skill 之后会把它当作
kind="builtin" 的技能列出来,而 delete/move 与 skills 的删除接口对 builtin
一律拒绝,重启同步也只清 RETIRED_BUILTIN_SKILLS 白名单,所以写进去就删不掉。

校验放在取 workspace 之前(与其余写接口一致),upload 校验的是最终真正使用的
target,并且先于读取请求体。
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants