Skip to content

建立 worker 信任边界:stderr 消毒、env.deny 名单、权限默认 allow_once (PR-2) - #30

Merged
Fei2411 merged 5 commits into
fix/mcp-lifecycle-acp-diagnosticsfrom
devin/fix-pr2-trust-boundary
Oct 6, 2026
Merged

Fei2411 merged 5 commits into
fix/mcp-lifecycle-acp-diagnosticsfrom
devin/fix-pr2-trust-boundary

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Summary

worker 输出与环境之间缺消毒/分类层的统一修复。

  • H-04:新模块 diagnostics.py 托管 redact_diagnostic(从 acp.py 提升并增强,上限 4096)。边界消毒点:task.error/task.warnings 赋值、codex finalize_codex_turn、codex/antigravity 转录 error 事件。result_text 与结构化 payload 不消毒(属产品输出)。此前 codex stderr → state.json/transcript/logs/*.log 全链明文留存 sk-/tp- 形态密钥。
  • G1:env.deny glob 名单 + DEFAULT_ENV_DENY_PATTERNS(AWS_*/AZURE_*/KUBE*/DOCKER_*/SSH_*/GITHUB_TOKEN/GITLAB_*/*_SECRET*/*_PASSWORD*/*_PRIVATE_KEY*/DATABASE_URL/OPENAI_*/ANTHROPIC_* 等;刻意无 *_API_KEY 泛匹配——LLM 厂商键本就在 DEFAULT_INHERIT_KEYS)。优先级:env.set/agent env > env.deny(用户) > env.inherit > 默认表。过滤放 build_worker_env 而非 resolve_env——后者被 install_host_env 复用,会从协调器自身环境删密钥。
  • D2:permission_policy 三档(allow_once 新默认 / allow_always 原行为 / deny 全拒),_pick_permission_option 按策略排名;未知值回落 allow_once。

语义变化:默认权限从 allow_always 降为 allow_once(可配置回);非 inherit 的凭据键不再下发 worker。

测试:+16 用例,全套 640 passed / 1 skipped(含假密钥端到端落盘验证)。

Link to Devin session: https://app.devin.ai/sessions/b270264aa3634bc5934cad9bcec9f9ae
Open in Devin Desktop: https://app.devin.ai/desktop/session/b270264aa3634bc5934cad9bcec9f9ae?variant=devin
Requested by: @FeiZhuLulu


Devin Review

- 新增 diagnostics.py 托管 redact_diagnostic;Registry 持久化/出参边界统一消毒 task.error/warnings 与转录 error 事件(H-04)
- env.deny glob 名单 + DEFAULT_ENV_DENY_PATTERNS 覆盖主流凭据模式;env.set/agent env 显式豁免,env.inherit 仅豁免默认表(G1)
- 过滤在 build_worker_env 而非 resolve_env——install_host_env 复用后者,误伤协调器自身密钥
- 新增 permission_policy 三档,默认 allow_once 最小权限(D2)
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

devin-ai-integration[bot]

This comment was marked as resolved.

- allow_once 策略下不再回退选中 allow_always:一次性授权不得变成 worker 内持久授权;仅剩 allow_always 时返回拒绝
- DSH launch 注入的 apiKeyEnv 凭据现在过 denied_by_policy(同一判定源),被 deny 的键不会从 Windows user/machine env 回填进 worker
- _coerce_env 中 deny/inherit 遇裸字符串按单个模式处理,不再被拆成单字符 glob(inherit 存量同款 bug 一并修)
- 抽出 denied_by_policy 判定供 worker_env 与 dsh_home 共用
…olicy 只应用用户 env.deny,显式 set/agent env 仍优先,inherit 不再豁免 deny
devin-ai-integration[bot]

This comment was marked as resolved.

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

审了当前 diff(含移除内置 env 拒绝名单的最新提交)。H-04 消毒边界、permission_policy=allow_once 不再回退 allow_always、以及 DSH 回填走 denied_by_policy,这几处是成立的。默认拒绝名单拿掉是提交里写明的取舍,不作为缺陷重报。

仍有两处 env.deny 控制面的高置信漏洞,见行内评论。

Open in Web View Automation 

Sent by Cursor Automation: GitHub PR 自动审核

machine_env=machine_env,
)
cfg = config or EnvConfig()
explicit = set(cfg.set) | set(overrides or {})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

严重程度:中

问题: explicit 把 env.set / agent env 里的键名全部当成「已赋值」从而豁免 env.deny,但 _assign 对空/纯空白值是直接 continue 的,并不会写入、也不会清掉已从进程环境拷贝进来的值。

触发条件: 【env】deny = [“AWS_*”],同时 set.AWS_SECRET_ACCESS_KEY = ""(或 agent env 里同名键为空字符串),而协调进程环境里已有 AWS_SECRET_ACCESS_KEY=real-secret。

影响: deny 不会剔掉该键,worker 仍拿到宿主凭据。操作者用空字符串「清掉」加 deny 做防御时,两层都不生效。

建议: explicit 只应包含真正写入 env 的键(str(value).strip() 非空),与 _assign 的跳过条件一致。

user = user_env if user_env is not None else read_windows_user_env()
for incoming in (machine, user):
for key, value in pick_env_keys(incoming, extra_keys).items():
if key not in out and env_cfg is not None and denied_by_policy(key, env_cfg):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

严重程度:中

问题: DSH 回填用 pick_env_keys(按小写字查 Windows env,写出的键名是 settings.yaml 里 apiKeyEnv 的原样大小写),接着 denied_by_policy 用 fnmatch.fnmatchcase 做匹配。两边对大小写的偏好不一致,刚补的 DSH 回填检查仍能绕过。

触发条件: 【env】deny = [“GITHUB_*”] 已从 worker env 剔掉 GITHUB_TOKEN;DSH settings.yaml 的 apiKeyEnv 为 github_token(或任何与 glob 不同的大小写);Windows user/machine env 里仍有该令牌。

影响: pick_env_keys 返回 github_token,denied_by_policy("github_token") 对 GITHUB_* 为 False,令牌被 setdefault 回 worker。Windows 下进程环境大小写不敏感,效果等同 deny 失效。

建议: 这里用与 pick_env_keys 相同的小写字比对(或对 key 和 glob 都 .casefold() 再 fnmatch);源键名与 apiKeyEnv 两种写法都要能命中 deny。

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 3 new potential issues.

3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)

Devin Review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Startup sweep deletes live task results

When a sibling writes a result before its task row reaches disk, _sweep_orphan_results treats the file as orphaned. The state lock ends before deletion, so the sibling can persist its row and still lose the result.

(Refers to this code)

Learn more

A task result is written before the final task-state save in _run_task and _run_task. The startup sweep reads the task index under a lock, then releases that lock before examining and deleting files. A sibling can write a result while its task row is pending, or persist its row after the sweep's read, and the sweep deletes a result that belongs to a live task.

Example: Instance B finishes task task_b, writes results/task_b.txt, and queues its state save. Instance A reads state without task_b; B's save completes; A unlinks task_b.txt. B now reports a completed task but cannot return its full result.

Recommended fix: Do not infer that an unindexed result belongs to a dead task while sibling instances can produce files. Coordinate result publication and sweeping with a shared lock covering the full check-and-delete operation and the result-to-row transition, or defer cleanup until ownership and a safe age can be established.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

部分成立。上一轮修复后,清扫已在 file_lock 内重读磁盘并把磁盘 task 行并入 known——「先落行后扫」的交错已闭合。残留的窗口是评审指出的这个:兄弟实例先写结果文件、task 行尚在 save 队列里,本实例此时扫描会误判孤儿。这个窗口在 _run_task 内只有毫秒级,触发还需要恰好有第三个实例在此刻启动,概率很低但确实存在。

最小修复方向:给孤儿判定加一个文件 mtime 宽限期(例如结果文件比本次启动早才删),比建立结果发布协议简单得多,且不改变单实例语义。会随上面那条一起在独立小 PR 修复。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Restarted tasks block dispatch retries

After restart marks an unfinished task bridge_restarted, start restores its request binding. A retry with the same request_id reuses that failed task instead of dispatching work.

(Refers to this code)

Learn more

On startup, start converts interrupted queued and running tasks to failed bridge_restarted. The restored binding makes dispatch_task return that failed record for an identical retry without starting another turn. The prior in-memory binding expired with the instance, so restarting did not trap the retry this way.

Example: A coordinator sends request ID R, the Bridge crashes while its task is running, and the coordinator resends R after restart. The new Bridge returns reused=true for a task already marked bridge_restarted, rather than launching the requested task.

Recommended fix: Do not restore request bindings for tasks converted to bridge_restarted; if deduplication across restarts is desired for completed tasks, define and document that separate contract explicitly.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

确认是真实缺陷,是 H-01 幂等持久化引入的语义扩展副作用。

原内存时代绑定随实例消亡,所以崩溃后重发同 request_id 会正常重新派发;现在绑定持久化后,start() 把 bridge_restarted 终态任务的绑定也恢复了,重试被路由到一条已失败的任务记录上。

最小修复方向:start() 恢复绑定时只保留 status == "completed" 的任务绑定(完成态才需要跨重启去重;failed/cancelled/bridge_restarted 的重试应该放行),同时把这些终态绑定从持久表丢弃。修复代码在已合并的 registry.py 里,会以一个独立小 PR 跟进,不追加到本 PR。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Remote tasks report fabricated silence

For a sibling's running task, _task_snapshot computes silence from this instance's activity clock. No local activity defaults to zero, so silent_for_sec reports 0 instead of unknown.

(Refers to this code)

Learn more

A task read from a sibling's state row is not tracked in this Bridge's memory. worker_silence_sec reads only local process activity; when it has none, _task_snapshot turns None into 0. That value falsely claims the remote running worker emitted output just now.

Example: Instance A has a running task silent for ten minutes. Instance B calls check_task and gets from_state=true and silent_for_sec=0, even though B cannot observe A's last worker output.

Recommended fix: For snapshots identified as disk fallbacks, set silent_for_sec to None regardless of task status; only compute it for tasks owned by this instance.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

确认。from_state 的快照不该伪造活跃值——本实例观察不到兄弟实例 worker 的最后输出时刻,报 0 等价于声称「刚刚还有输出」,比不报告更有误导性。

最小修复方向:磁盘回落路径 silent_for_sec 直接置 None(仅本地 self.tasks 拥有的任务才计算)。属显示层修正,随前两条一起在独立小 PR 修复。

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

审了当前 diff(含 merge 与文档提交,HEAD 9c6ca37)。H-04 消毒边界、permission_policy=allow_once 不再回退 allow_always、以及默认拒绝名单移除的取舍,仍成立、不重报。

上一轮两处 env.deny 控制面漏洞仍未修,见行内评论。

Open in Web View Automation 

Sent by Cursor Automation: GitHub PR 自动审核

machine_env=machine_env,
)
cfg = config or EnvConfig()
explicit = set(cfg.set) | set(overrides or {})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

严重程度:中

问题: explicit 把 env.set / agent env 里的键名全部当成「已赋值」从而豁免 env.deny,但 _assign 对空/纯空白值是直接 continue 的,并不会写入、也不会清掉已从进程环境拷贝进来的值。HEAD 9c6ca37 仍是 explicit = set(cfg.set) | set(overrides or {}),没有跟 _assign 的跳过条件对齐。

触发条件: [env] deny = ["AWS_*"],同时 set.AWS_SECRET_ACCESS_KEY = ""(或 agent env 里同名键为空字符串/纯空白),而协调进程环境里已有 AWS_SECRET_ACCESS_KEY=real-secret。

影响: deny 不会剔掉该键,worker 仍拿到宿主凭据。操作者用空字符串「清掉」加 deny 做防御时,两层都不生效。

建议: explicit 只应包含真正写入 env 的键(str(value).strip() 非空),与 _assign 的跳过条件一致。

user = user_env if user_env is not None else read_windows_user_env()
for incoming in (machine, user):
for key, value in pick_env_keys(incoming, extra_keys).items():
if key not in out and env_cfg is not None and denied_by_policy(key, env_cfg):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

严重程度:中

问题: DSH 回填用 pick_env_keys(按小写字查 Windows env,写出的键名是 settings.yaml 里 apiKeyEnv 的原样大小写),接着 denied_by_policy 用 fnmatch.fnmatchcase 做匹配。两边对大小写的偏好不一致;_apply_env_deny 可能已按正规大小写剔掉该凭据,这里仍能用另一套大小写回填。HEAD 9c6ca37 仍是 denied_by_policy(key, env_cfg),未对 key/glob 做 casefold。

触发条件: [env] deny = ["GITHUB_*"] 已从 worker env 剔掉 GITHUB_TOKEN;DSH settings.yaml 的 apiKeyEnv 为 github_token(或任何与 glob 不同的大小写);Windows user/machine env 里仍有该令牌。

影响: pick_env_keys 返回 github_token,denied_by_policy("github_token") 对 GITHUB_* 为 False,令牌被 setdefault 回 worker。Windows 下进程环境大小写不敏感,效果等同 deny 失效。

建议: 这里用与 pick_env_keys 相同的小写字比对(或对 key 和 glob 都 .casefold() 再 fnmatch);源键名与 apiKeyEnv 两种写法都要能命中 deny。

@Fei2411
Fei2411 merged commit ddb49ff into fix/mcp-lifecycle-acp-diagnostics Oct 6, 2026
6 checks passed
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.

1 participant