Conversation
The run endpoints took filesystem paths straight from the request body and
passed them to the pipeline command. subprocess is called with a list, so
there was no shell injection, but the paths themselves were the problem:
- java_bin and beagle_jar name executables the pipeline then runs.
- input_vcf, ref_dir, ccre_bed, ncrna_bed and genos_evee_db could point
anywhere the service user can read.
- output_dir was mkdir'd and written to without any check, and because
/jobs/{job_id}/files{,/{path}} serve files relative to the output_dir
recorded in status.json, a caller could set output_dir to any directory
and then list and download it. safe_job_file() confined paths to that
root, but the root itself was attacker-supplied. dry_run=true was enough
— no pipeline execution required.
Confirmed against the pre-fix code: submitting output_dir=<dir outside the
repo> returned 202, /files listed that directory, and the download route
returned the file contents with 200.
Changes:
- Add a path sandbox. Read paths must resolve inside one of the roots the
deployment already configures through path_utils (repo root, jobs dir,
ref dir, beagle/GENOS/cCRE/ncRNA data dirs, OPENRARE_DATA_ROOT,
OPENRARE_PUBLIC_DATA_ROOT), extendable via
FULL_PIPELINE_API_ALLOWED_ROOTS. Outputs must resolve inside
FULL_PIPELINE_API_OUTPUT_ROOT (default: the jobs dir). resolve() is used
throughout, so '..' and symlinks cannot escape a root.
- Drop java_bin and beagle_jar from RunRequest and the multipart form.
Both stay server-side configuration (JAVA_BIN, FULL_PIPELINE_BEAGLE_JAR).
Requests that omitted them already got the script defaults, so behaviour
is unchanged for every non-malicious caller.
- Validate job_id against the uuid4 hex format it is always generated in,
so a crafted id cannot walk out of the jobs directory via status.json.
- Re-check output_dir and the log path on read, so a job recorded before
this change cannot still be used to read outside the roots.
- Resolve JOBS_DIR once at import: FULL_PIPELINE_API_JOBS_DIR may be
relative or symlinked, and every containment check compares resolved
paths.
Add test/test_path_sandbox.py (22 checks, no pytest dependency, registered
as `pixi run path-sandbox-test`) covering each escape above plus the
in-sandbox requests that must still succeed.
Fixes OpenRare2026#17
Signed-off-by: FenjuFu <fufenjupku@gmail.com>
This was referenced Jul 21, 2026
pokemonjs
reviewed
Sep 14, 2026
pokemonjs
left a comment
Collaborator
There was a problem hiding this comment.
代码层面未发现会阻止合并的高危问题,核心安全修复有效;建议补充文档后合并。
发现的问题:
- [P2] API 文档仍声明 beagle_jar 和 java_bin 可由请求传入,但 PR 已从 RunRequest 和 multipart 接口中移除这两个字段。当前 Pydantic 默认会忽略这两个额外字段,调用方不会收到明确错误,容易造成“配置成功但实际未生效”的误解。应同步更新 [modules/pipeline/complete_pipeline/README.md],并说明改用 FULL_PIPELINE_BEAGLE_JAR / JAVA_BIN 服务端配置。
beagle_jar and java_bin are no longer API fields, and path fields are confined to the allowed roots. Explain the new settings and that old requests passing those fields are silently ignored. Signed-off-by: FenjuFu <92919259+FenjuFu@users.noreply.github.com>
Contributor
Author
|
@pokemonjs 感谢 review。P2 已在 f15e075 处理:
|
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
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.
Fixes #17
先说复现
Issue 里判断「不存在 shell 注入,但可指定任意可执行文件 + 任意路径」是准的。实际验证下来,还有一条 issue 没点到的链路,危害更直接:
output_dir由调用方指定且未经校验,而/jobs/{job_id}/files和/jobs/{job_id}/files/{path}是相对status.json里记录的output_dir提供文件的。safe_job_file()确实做了relative_to(root)防穿越 —— 但那个 root 本身就是攻击者给的。于是:在修复前的代码上实测:三步分别返回
202/200(列出了目录内容)/200+ 文件明文。dry_run: true即可,完全不需要真的跑 pipeline。同一次验证还确认了:
--java-bin /tmp/evil.sh确实进入了最终命令行;任意深度目录会被mkdir(parents=True)创建出来。改法
路径沙箱。读路径必须落在部署本来就配置好的根目录内(仓库根、jobs 目录、ref/beagle/GENOS/cCRE/ncRNA 数据目录、
OPENRARE_DATA_ROOT、OPENRARE_PUBLIC_DATA_ROOT),额外根用FULL_PIPELINE_API_ALLOWED_ROOTS追加。写路径必须落在FULL_PIPELINE_API_OUTPUT_ROOT(默认 jobs 目录)内。全程走resolve(),所以..和符号链接都逃不出去。java_bin/beagle_jar从请求模型和 multipart 表单里删掉,改为纯服务端配置(JAVA_BIN/FULL_PIPELINE_BEAGLE_JAR)—— 按 issue 的方案 2。注意:原先不传这两个字段时走的就是脚本默认值,所以对任何非恶意调用方行为不变。job_id格式校验(uuid4 hex)。这是顺带发现的:status_path()直接拼JOBS_DIR / job_id,构造job_id可以走出 jobs 目录。读取侧复检:
output_root_for_job()和日志接口再确认一次路径在根内 —— 这样本次改动之前已经落库的恶意 job 也不会继续可用(返回 403),不需要清库。JOBS_DIR在 import 时resolve()一次。原先它直接取环境变量,相对路径或软链会让后续所有包含判断失准。测试
新增
test/test_path_sandbox.py,22 项检查全绿,注册为pixi run path-sandbox-test。不依赖 pytest(仓库目前没有 pytest 基建,见 #19),直接python test/test_path_sandbox.py即可跑。覆盖:上面每一条逃逸路径 + 「合法请求必须仍然成功」的反向用例(默认 output_dir、显式指定根内 output_dir、根内 input_vcf)。
补充一个测试里踩到的点:
tempfile.mkdtemp()在 Windows 返回 8.3 短路径,与resolve()后的路径不相等 —— 这也正是我把JOBS_DIR改成启动时 resolve 的原因,否则真实部署里若 jobs 目录含软链,包含判断会静默失效。说明
本 PR 从
dev切出,不含 #29(Fixes #15)的改动,两个可独立审阅、独立合并。#16(无认证 / IDOR)我认为是这三个安全 issue 里唯一真正的架构决策,我会在 issue 下写方案而不是直接提 PR —— 沙箱化只是把「任何人可读任意文件」收敛成「任何人可读数据目录内的文件」,在认证落地之前,暴露面依然存在。