Skip to content

[fix][evaluation] add OpenAPI extra_output field and fix two horizontal privilege escalations - #564

Merged
jamesonics merged 10 commits into
mainfrom
add_openapires_param
Jun 25, 2026
Merged

jamesonics merged 10 commits into
mainfrom
add_openapires_param

Conversation

@jamesonics

Copy link
Copy Markdown
Collaborator

What

本 PR 合并了 evaluation 模块的三项改动(一次发布):

1. feat: ListExperimentResultOApi 响应新增 extra_output 字段

  • domain_openapi/evaluator.thrift 新增 EvaluatorExtraOutputContent struct 及 extra_output 字段,重新生成
    kitex_gen
  • eval_openapi_app.go 注入 fileProvider,对 TOS URI 签名为可访问 URL(fillExtraOutputURLs)
  • 补充 convertor 与单测

2. fix: BatchGetExptAggrResult 水平越权校验

  • expt_result_aggr_impl.go:取出实验后按 expt.SpaceID == spaceID 过滤出
    validExptIDs,下游聚合/评估器引用/标签查询全部改用过滤后的 ID,防止跨 workspace 读取他人实验聚合结果

3. fix: UpdateExperiment 水平越权校验

  • experiment_app.go:在写库前补 got.SpaceID != req.GetWorkspaceID() 校验(对齐 DeleteExperiment
    既有范式),防止用本 workspace 的 id 覆盖/搬移他人实验的 space_id

Why

#2、#3 是同类水平越权(horizontal privilege escalation)修复:攻击者持有 A workspace
的合法凭证,却能读取/修改属于 B workspace 的实验资源。修复对齐已有 DeleteExperiment 校验范式,复用
errno.CommonBadRequestCode,不新增错误码。

关联 Meego: https://meego.larkoffice.com/fornax/story/detail/7344006097 (水平校验修复)

单元测试(gomock + testify,本地 GOTOOLCHAIN=go1.24.6 go test 全绿):

  • TestExperimentApplication_UpdateExperiment 新增 "workspace mismatch with experiment space" 用例:mock 返回
    SpaceID 不匹配的实验,断言报错且不调用 Update(先校验后写库);红→绿变异验证通过
  • TestExptAggrResultServiceImpl_BatchGetExptAggrResultByExperimentIDs 新增 cross-space filter 用例:混入跨
    space 实验,断言下游查询只对合法 exptID 发起;移除过滤逻辑后用例如期 FAIL
  • evaluation application + domain/service 全模块回归通过

PPE 泳道端到端验证(ppe_test 泳道,跨 workspace 越权场景):

  • 越权读(A 查 B 的实验聚合结果)→ 静默过滤返回空结果,无穿透报错(对比线上 baseline 601200702)
  • 正向读(A 查自己实验)→ 正常返回完整聚合结果,未误伤
  • 越权写(A 改 B 的实验)→ 返回 601200201 "expt ... not found in space ...",DB 未写入
  • LogID 日志确认请求落在 ppe 泳道(_env=ppe)

Scope note

本 PR 仅含 evaluation/openapi 改动(16 文件,其中 3 个为 kitex_gen 生成代码)。仅修水平越权校验,未改 DAO 层
WHERE(纵深防御另议);notification_conf / pool.go 经核查当前代码已一致,不在本次范围。

VinCinx and others added 8 commits June 22, 2026 19:32
…ponse

- Add EvaluatorExtraOutputContent struct to domain_openapi IDL
- Regenerate kitex_gen from updated IDL
- Add OpenAPIEvaluatorExtraOutputContentDO2DTO convertor
- Inject fileProvider into EvalOpenAPIApplication for URI-to-URL signing
- Add fillExtraOutputURLs to sign TOS URIs before returning response
- Regenerate wire_gen.go
- Add unit tests for extra_output conversion
…riment convertor)

The ListExperimentResultOApi uses openAPIEvaluatorOutputDataDO2DTO (lowercase,
in convertor/experiment/openapi.go) instead of the public
OpenAPIEvaluatorOutputDataDO2DTO (in convertor/evaluator/openapi.go).
The private method was missing the ExtraOutput and Stdout field mapping.
… tracing

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…r BatchGetExptAggrResultByExperimentIDs

Add a dedicated table-driven case asserting that experiments whose SpaceID
differs from the input spaceID are filtered out (validExptIDs), so all
downstream queries (BatchGetExptAggrResultByExperimentIDs / GetEvaluatorRefByExptIDs /
batchGetTagInfoByExperimentIDs) only receive the valid exptID and the
cross-space exptID is dropped. Verified via mutation: removing the filter
makes this case fail.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rizontal privilege escalation

UpdateExperiment used req.WorkspaceID to overwrite the DB space_id without
verifying ownership, allowing an attacker to move another workspace's
experiment into their own. Validate got.SpaceID == req.GetWorkspaceID()
right after Get and before any write, mirroring DeleteExperiment.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add 'workspace mismatch with experiment space' case asserting the request
is rejected before manager.Update is invoked, mirroring the
DeleteExperiment test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@jamesonics
jamesonics requested review from VinCinx and xueyizheng June 25, 2026 12:59
@jamesonics jamesonics changed the title feat(evaluation): add OpenAPI extra_output field + fix two horizontal privilege escalations [fix][evaluation] add OpenAPI extra_output field and fix two horizontal privilege escalations Jun 25, 2026
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@jamesonics jamesonics changed the title [fix][evaluation] add OpenAPI extra_output field and fix two horizontal privilege escalations [fix][evaluation] add OpenAPI extra_output field and fix two horizontal privilege escalations Jun 25, 2026
VinCinx
VinCinx previously approved these changes Jun 25, 2026
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@jamesonics
jamesonics requested a review from VinCinx June 25, 2026 14:23
@jamesonics
jamesonics merged commit 695daf3 into main Jun 25, 2026
12 of 13 checks passed
@jamesonics
jamesonics deleted the add_openapires_param branch June 25, 2026 14:24
@codecov

codecov Bot commented Jun 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.65217% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...modules/evaluation/application/eval_openapi_app.go 93.75% 2 Missing and 1 partial ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #564      +/-   ##
==========================================
+ Coverage   77.60%   78.12%   +0.51%     
==========================================
  Files         670      670              
  Lines       75995    76214     +219     
==========================================
+ Hits        58979    59543     +564     
+ Misses      13565    13238     -327     
+ Partials     3451     3433      -18     
Flag Coverage Δ
unittests 78.12% <95.65%> (+0.51%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...luation/application/convertor/evaluator/openapi.go 96.53% <100.00%> (+4.63%) ⬆️
...uation/application/convertor/experiment/openapi.go 89.82% <100.00%> (+6.25%) ⬆️
...d/modules/evaluation/application/experiment_app.go 85.25% <100.00%> (+0.90%) ⬆️
...evaluation/domain/service/expt_result_aggr_impl.go 79.75% <100.00%> (+0.08%) ⬆️
...modules/evaluation/application/eval_openapi_app.go 92.33% <93.75%> (+0.05%) ⬆️

... and 26 files with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 016641a...0f3db7a. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Colin4k1024 pushed a commit to Colin4k1024/coze-loop that referenced this pull request Aug 28, 2026
…al privilege escalations (coze-dev#564)

* feat(openapi): add extra_output field to ListExperimentResultOApi response

- Add EvaluatorExtraOutputContent struct to domain_openapi IDL
- Regenerate kitex_gen from updated IDL
- Add OpenAPIEvaluatorExtraOutputContentDO2DTO convertor
- Inject fileProvider into EvalOpenAPIApplication for URI-to-URL signing
- Add fillExtraOutputURLs to sign TOS URIs before returning response
- Regenerate wire_gen.go
- Add unit tests for extra_output conversion

* fix: add extra_output field in openAPIEvaluatorOutputDataDO2DTO (experiment convertor)

The ListExperimentResultOApi uses openAPIEvaluatorOutputDataDO2DTO (lowercase,
in convertor/experiment/openapi.go) instead of the public
OpenAPIEvaluatorOutputDataDO2DTO (in convertor/evaluator/openapi.go).
The private method was missing the ExtraOutput and Stdout field mapping.

* debug: add key logs for OpenAPI ListExperimentResultOApi extra_output tracing

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* test(convertor): add unit tests for evaluator and experiment convertors to reach 90% coverage

* [fix][evaluation] add spaceID validation for BatchGetExptAggrResult to prevent horizontal privilege escalation

* test(evaluation): add cross-space privilege escalation filter case for BatchGetExptAggrResultByExperimentIDs

Add a dedicated table-driven case asserting that experiments whose SpaceID
differs from the input spaceID are filtered out (validExptIDs), so all
downstream queries (BatchGetExptAggrResultByExperimentIDs / GetEvaluatorRefByExptIDs /
batchGetTagInfoByExperimentIDs) only receive the valid exptID and the
cross-space exptID is dropped. Verified via mutation: removing the filter
makes this case fail.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(evaluation): add space_id check in UpdateExperiment to prevent horizontal privilege escalation

UpdateExperiment used req.WorkspaceID to overwrite the DB space_id without
verifying ownership, allowing an attacker to move another workspace's
experiment into their own. Validate got.SpaceID == req.GetWorkspaceID()
right after Get and before any write, mirroring DeleteExperiment.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(evaluation): cover UpdateExperiment workspace mismatch authz

Add 'workspace mismatch with experiment space' case asserting the request
is rejected before manager.Update is invoked, mirroring the
DeleteExperiment test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(evaluation): cover fillExtraOutputURLs and raise patch coverage

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* style(evaluation): gofumpt format test files to pass lint

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: wangtao.everett <wangtao.everett@bytedance.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
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