diff --git a/AGENTS.md b/AGENTS.md index 77df643ff..328f7daa8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -31,24 +31,21 @@ A single `make dev` / Docker stack runs four cooperating services: | **Frontend** | `3000` | Next.js web interface | | **Provisioner** | `8002` | Optional — only when sandbox is configured for provisioner/K8s mode | -Nginx is the single public entry: it serves the frontend and proxies `/api/langgraph/*` -to the Gateway's LangGraph runtime, rewriting it to Gateway's native `/api/*` routes; all -other `/api/*` go straight to the Gateway REST routers. See -[backend/AGENTS.md](backend/AGENTS.md) for the runtime and router detail. -It compresses HTML and configured textual assets, while deliberately leaving SSE, -fonts, images, audio, and video uncompressed at the proxy layer. +Nginx is the single public entry: it proxies `/api/*` to the Gateway, rewriting +`/api/langgraph/*` onto the Gateway's native routes, and serves the frontend — see +[backend/AGENTS.md](backend/AGENTS.md) for the runtime and router detail. It compresses +HTML and configured textual assets, deliberately leaving SSE, fonts, images, audio, and +video uncompressed at the proxy layer. Both compose files publish that entry as `"${BIND_HOST:-127.0.0.1}:${PORT:-2026}:2026"` -— **loopback by default**, matching the README's documented deployment model. A bare -`"${PORT}:2026"` binds `0.0.0.0`, which does not. -The root `PORT` value is Docker ingress configuration only; local orchestration pins -Next.js to `3000` so loading `.env` cannot make `make dev` wait on the wrong port. -Nginx itself listens `default_server` on IPv4+IPv6 and the -Gateway binds `0.0.0.0:8001` inside the container on purpose — both are container- -internal; the published nginx port is the entire external surface, and the Gateway's -`8001` is deliberately not published. Any new published port needs an explicit bind -address; `backend/tests/test_compose_default_bind_host.py` pins this for every service -in both compose files. +— **loopback by default**, matching the README's documented deployment model; a bare +`"${PORT}:2026"` binds `0.0.0.0`, which does not. The root `PORT` value is Docker ingress +configuration only; local orchestration pins Next.js to `3000` so loading `.env` cannot +make `make dev` wait on the wrong port. Nginx listening `default_server` on IPv4+IPv6 and +the Gateway binding `0.0.0.0:8001` are container-internal on purpose: the published nginx +port is the entire external surface. Any new published port needs an explicit bind +address; `backend/tests/test_compose_default_bind_host.py` pins this for every service in +both compose files. ## Repository Map @@ -105,19 +102,16 @@ Skill quality review note: edits from their head revision, but only the manifest from the trusted base revision can suppress that run. Entries match one error finding exactly, include the reviewed file's SHA-256 and an expiry date, remain visible in CI - output, and can never waive blocker findings. An entry may also preapprove a - bounded list of future full-file SHA-256 values. Those hashes become effective - only after the manifest change lands in the trusted base. Adding or changing a - waiver and relying on it therefore requires two steps: merge the reviewed - manifest change first, then update the affected public skill in a later pull - request. After that skill change lands, promote its hash to `file_sha256` and - remove the consumed preapproval in a follow-up manifest cleanup. + output, and can never waive blocker findings. An entry may also preapprove + future full-file SHA-256 values, effective only once the manifest change lands + in the trusted base — so relying on a waiver takes two merges: the manifest + first, the skill change after, then promote the consumed hash to `file_sha256` + in a follow-up cleanup. Scheduled-task note: - The scheduled-task MVP adds a workspace page at `/workspace/scheduled-tasks` plus a background scheduler service gated by `config.yaml -> scheduler.enabled`. -- Scheduled background runs are intentionally non-interactive: they execute through the normal run lifecycle, but the lead-agent toolset excludes `ask_clarification` when `context.non_interactive=true`. The key is honored only for internally-authenticated callers (the scheduler launch path); client-supplied `context.non_interactive` is dropped. +- Scheduled background runs are intentionally non-interactive: the lead-agent toolset excludes `ask_clarification` when `context.non_interactive=true`. That key, `disable_clarification`, and `github_token` are honored only for internally-authenticated callers; client-supplied copies are dropped from both `body.context` and `body.config`. - Busy scheduled occurrences are persisted as `queued`; `launching` is a short lease-fenced claim, `running` remains the normal Gateway run lifecycle, and `scheduler.queue_timeout_seconds` bounds the durable wait. Do not reintroduce skip-on-overlap or count waiting rows against `max_concurrent_runs`. -- Scheduled launches use `scheduler.recursion_limit` (default 1000, matching the web UI's `recursion_limit: 1000`, clamped by `max_recursion_limit`). The value is read at dispatch, so a YAML edit applies to the next scheduled run without a Gateway restart. ## Commands: Root vs. Module diff --git a/CHANGELOG.md b/CHANGELOG.md index afba9fa06..2b208a336 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -558,6 +558,20 @@ This section accumulates work toward the **2.1.0** milestone ### Fixed +- **gateway:** Honor `disable_clarification` and `github_token` only for + internally-authenticated callers, the way `non_interactive` already was. + Both keys were forwarded from `body.context` regardless of the caller and + were not scrubbed from the free-form `body.config` that the run config + copies verbatim, so any session or PAT caller could set them. + `disable_clarification` is the stronger of the two: `ClarificationMiddleware` + answers every clarification — `risk_confirmation` included — with "proceed + without asking", and `SandboxMiddleware` reads it as the same + non-interactive signal as `non_interactive`. `github_token` reached + `runtime.context`, where the bash tool exports it as `GH_TOKEN`/`GITHUB_TOKEN`, + and a copy smuggled through `body.config['configurable']` was persisted in + the checkpoint store. The scheduler, IM channels, and the GitHub webhook + channel authenticate over the internal request channel and are unaffected. + ([#5338]) - **artifacts:** Keep `PUT /api/threads/{id}/artifacts/{path}` confined to `/mnt/user-data/outputs`. The outputs-only guard was a string-prefix check on the raw path, so a percent-encoded `..` (`outputs/%2e%2e/uploads/x.txt`) — @@ -2699,3 +2713,4 @@ with **180 merged pull requests** since the first 2.0 milestone tag. [#5284]: https://github.com/bytedance/deer-flow/pull/5284 [#5287]: https://github.com/bytedance/deer-flow/pull/5287 [#5321]: https://github.com/bytedance/deer-flow/pull/5321 +[#5338]: https://github.com/bytedance/deer-flow/pull/5338 diff --git a/CHANGELOG_zh.md b/CHANGELOG_zh.md index 905d2fb21..07c042099 100644 --- a/CHANGELOG_zh.md +++ b/CHANGELOG_zh.md @@ -397,6 +397,16 @@ ### 修复 +- **Gateway：** `disable_clarification` 与 `github_token` 现在与 `non_interactive` + 一样，仅对内部认证的调用方生效。此前这两个键无论调用方身份都会从 `body.context` + 透传，而且不会从被逐字复制进 run config 的自由格式 `body.config` 中清除，因此任何 + 会话或 PAT 调用方都能设置它们。其中 `disable_clarification` 影响更大： + `ClarificationMiddleware` 会把包括 `risk_confirmation` 在内的所有澄清请求替换为 + "无需确认，继续执行"，`SandboxMiddleware` 也把它与 `non_interactive` 视作同一个 + 非交互信号。`github_token` 则会进入 `runtime.context`，被 bash 工具导出为 + `GH_TOKEN`/`GITHUB_TOKEN`；若经由 `body.config['configurable']` 夹带，还会被写入 + checkpoint 存储。定时任务、IM 渠道与 GitHub webhook 渠道走内部请求通道，不受影响。 + ([#5338]) - **Artifact：** `PUT /api/threads/{id}/artifacts/{path}` 现在严格限制在 `/mnt/user-data/outputs` 之内。此前 outputs-only 校验只是对原始路径做字符串前缀 检查，百分号编码的 `..`（`outputs/%2e%2e/uploads/x.txt`，nginx 原样转发、Starlette @@ -2091,3 +2101,4 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写，核心包含子 [#5284]: https://github.com/bytedance/deer-flow/pull/5284 [#5287]: https://github.com/bytedance/deer-flow/pull/5287 [#5321]: https://github.com/bytedance/deer-flow/pull/5321 +[#5338]: https://github.com/bytedance/deer-flow/pull/5338 diff --git a/backend/AGENTS.md b/backend/AGENTS.md index 036558dcc..a0fc7d93f 100644 --- a/backend/AGENTS.md +++ b/backend/AGENTS.md @@ -15,7 +15,7 @@ DeerFlow is a LangGraph-based AI super agent system with a full-stack architectu - Gateway streams `write_file` and `str_replace` argument deltas in bounded batches for multi-mode `messages-tuple` consumers; single-mode message consumers retain the original per-chunk contract. Non-message frames flush pending batches, and `values` remains an optional complete-state snapshot rather than a prerequisite for batching. - With `stream_subgraphs`, subgraph frames keep their namespace in the SSE event name (`values|`, LangGraph Platform style) instead of impersonating root frames — a delegated subagent inherits the parent checkpoint namespace, so publishing its `values` snapshot as bare `values` replaces the whole thread view in SDK clients (#4399). Root-only consumers (file-tool chunk batcher, subagent event persistence, LLM error-fallback detection) ignore namespaced frames. The web frontend does not request subgraph streaming; subtask progress rides root-namespace `task_*` custom events. - Background subagent identity is deliberately split: the provider `tool_call_id` remains the correlation key for `ToolMessage`, `task_*` SSE events, persisted lifecycle events, frontend cards, and the public `ExtensionData.scope_id` contract (stored as `SubagentResult.external_task_id`), while `SubagentExecutor.execute_async()` generates a full server-side `execution_id` for `SubagentResult.task_id`, the process-wide registry, polling, cancellation, timeout handling, and cleanup. Provider IDs are not globally unique across parent runs, so they must never become registry ownership keys; scheduler closures retain their own `SubagentResult` rather than resolving ownership again through the mutable registry. Terminal subagent token usage travels in the current run's `ToolMessage.additional_kwargs` and is attributed from message state, never through a process-global provider-ID cache. -- Scheduled-task executions must reuse that same Gateway run lifecycle. The scheduler may decide *when* work runs, but it must dispatch through the existing run path rather than introducing a parallel execution stack. Scheduled launches pass `scheduler.recursion_limit` (default 1000, matching the web UI's `recursion_limit: 1000`, clamped by `max_recursion_limit`) via `launch_scheduled_thread_run`; the value is read from `get_app_config()` at dispatch. +- Scheduled-task executions must reuse that same Gateway run lifecycle. The scheduler may decide *when* work runs, but it must dispatch through the existing run path rather than introducing a parallel execution stack. Scheduled launches pass `scheduler.recursion_limit` (default 1000, matching the web UI's `recursion_limit: 1000`, clamped by `max_recursion_limit`) via `launch_scheduled_thread_run`; the value is read from `get_app_config()` at dispatch, so a YAML edit applies to the next scheduled run without a Gateway restart. - The background scheduler is single-instance by default. `scheduler.multi_instance=true` opts into lease-aware recovery across Gateway instances and requires shared Postgres, `run_ownership.heartbeat_enabled=true`, and `run_events.backend=db`; otherwise startup rejects the configuration. Live scheduled runs are preserved when a peer starts; expired launch claims return to the durable queue, expired run leases are atomically taken over, stale launch writes are fenced by lease ownership, and the Postgres advisory-locked budget makes `max_concurrent_runs` a shared global cap for `launching`/`running` rows. - Long-running MCP work uses a separate durable task runtime (`McpTaskService` + `mcp_tasks`, lease-based recovery) rather than keeping remote task IDs or status polling inside the Agent loop; only submit remains Agent-visible, the database is the source of truth, and `ThreadState` receives only a bounded current-thread projection. Full contract (leases, cancellation fencing, delivery idempotency, management-tool exposure): [packages/harness/deerflow/mcp/AGENTS.md](packages/harness/deerflow/mcp/AGENTS.md). - MCP task notification retries, dead-lettering, and the cancel endpoint's worker-stopped 503 are part of that same contract — see [packages/harness/deerflow/mcp/AGENTS.md](packages/harness/deerflow/mcp/AGENTS.md). @@ -225,6 +225,15 @@ the tool graph or subagent executor during state/schema imports. SQLite, and PostgreSQL: missing differs from null, bool differs from int, and float filters accept integer or real JSON numbers through `json_value_matches`. +### Gateway Run-Context Trust Boundary + +A server-produced run-context key must be gated on both client-writable feeds: +`body.context` (whitelist-merged) and free-form `body.config` (copied verbatim). +`merge_run_context_overrides` forwards it only when `internal=True`; +`strip_internal_context_keys` scrubs it from the assembled `context` *and* +`configurable`. Trust and destination are separate axes, so a new key needs both +decisions — and `disable_clarification` is no milder than `non_interactive`. + ## Development Workflow ### Test-Driven Development (TDD) — MANDATORY diff --git a/backend/app/gateway/services.py b/backend/app/gateway/services.py index f93574575..508de8469 100644 --- a/backend/app/gateway/services.py +++ b/backend/app/gateway/services.py @@ -457,8 +457,28 @@ _SERVER_OWNED_RUNTIME_CONTEXT_KEYS: frozenset[str] = ( # ``disable_clarification`` — set for non-interactive channels (GitHub # webhooks) so ClarificationMiddleware proceeds # instead of dead-ending the run. +# +# Both are produced server-side by the channel run policies +# (``ChannelManager._apply_channel_policy`` and ``app.gateway.github.run_policy``), +# which reach the Gateway over the internally-authenticated request channel, so +# they are internal-only as well — see :data:`_INTERNAL_ONLY_CONTEXT_KEYS`. _CONTEXT_RUNTIME_ONLY_KEYS: frozenset[str] = frozenset({"github_token", "disable_clarification"}) +# Every run-context key an external client may never supply, in either section. +# The two sets differ only in *where* a legitimate internal caller's value lands +# (both sections vs. ``context`` alone); their trust requirement is identical. +# +# ``disable_clarification`` is not a milder cousin of ``non_interactive``: +# ``ClarificationMiddleware`` answers every clarification — ``risk_confirmation`` +# included — with "proceed without asking" instead of interrupting, and +# ``SandboxMiddleware`` reads the two keys as the same non-interactive signal. +# Accepting it from a client therefore reproduces the effect the +# ``non_interactive`` gate exists to prevent. ``github_token`` is a live +# credential that ``bash`` exports as ``GH_TOKEN``/``GITHUB_TOKEN``, and a copy +# smuggled through ``body.config['configurable']`` would be written to the +# checkpoint store. +_INTERNAL_ONLY_CONTEXT_KEYS: frozenset[str] = _CONTEXT_INTERNAL_CALLER_KEYS | _CONTEXT_RUNTIME_ONLY_KEYS + def strip_internal_context_keys(config: dict[str, Any]) -> None: """Drop internal-only keys a non-internal caller smuggled into the run config. @@ -471,7 +491,7 @@ def strip_internal_context_keys(config: dict[str, Any]) -> None: for section in ("context", "configurable"): value = config.get(section) if isinstance(value, dict): - for key in _CONTEXT_INTERNAL_CALLER_KEYS: + for key in _INTERNAL_ONLY_CONTEXT_KEYS: value.pop(key, None) @@ -492,10 +512,11 @@ def merge_run_context_overrides(config: dict[str, Any], context: Mapping[str, An by :func:`strip_internal_context_keys`. A second set of keys (``_CONTEXT_RUNTIME_ONLY_KEYS`` — e.g. ``github_token``, - ``disable_clarification``) is forwarded into ``config['context']`` only, never - ``configurable``. These are secrets / runtime flags read by tools and middlewares - from ``runtime.context``; keeping them out of ``configurable`` avoids persisting a - short-lived token in the checkpoint store. + ``disable_clarification``) is likewise forwarded only when ``internal`` is True, + and then into ``config['context']`` only, never ``configurable``. These are + secrets / runtime flags read by tools and middlewares from ``runtime.context``; + keeping them out of ``configurable`` avoids persisting a short-lived token in the + checkpoint store. """ if not context: return @@ -509,10 +530,12 @@ def merge_run_context_overrides(config: dict[str, Any], context: Mapping[str, An if isinstance(runtime_context, dict): runtime_context.setdefault(key, context[key]) # Context-only keys (secrets / runtime flags) land in ``config['context']`` - # only — never ``configurable`` (which is persisted in checkpoints). - for key in _CONTEXT_RUNTIME_ONLY_KEYS: - if key in context and isinstance(runtime_context, dict): - runtime_context.setdefault(key, context[key]) + # only — never ``configurable`` (which is persisted in checkpoints) — and only + # for internal callers, the sole legitimate producers. + if internal: + for key in _CONTEXT_RUNTIME_ONLY_KEYS: + if key in context and isinstance(runtime_context, dict): + runtime_context.setdefault(key, context[key]) if "user_id" in context and isinstance(runtime_context, dict): runtime_context.setdefault("user_id", context["user_id"]) diff --git a/backend/tests/test_gateway_services.py b/backend/tests/test_gateway_services.py index c2fc577a1..f017504ce 100644 --- a/backend/tests/test_gateway_services.py +++ b/backend/tests/test_gateway_services.py @@ -1673,6 +1673,7 @@ def test_merge_run_context_overrides_forwards_context_only_keys(): "disable_clarification": True, "agent_name": "coding-llm-gateway", }, + internal=True, ) # Forwarded into runtime context — what tools/middlewares read. @@ -1685,15 +1686,46 @@ def test_merge_run_context_overrides_forwards_context_only_keys(): assert "disable_clarification" not in config.get("configurable", {}) +def test_context_only_keys_are_internal_only(): + """``github_token`` / ``disable_clarification`` are produced by the channel run + policies, which reach the Gateway over the internally-authenticated channel. A + non-internal caller must not be able to supply either through ``body.context``. + + ``disable_clarification`` is not a milder cousin of ``non_interactive``: + ``ClarificationMiddleware`` answers every clarification — ``risk_confirmation`` + included — with "proceed without asking", and ``SandboxMiddleware`` reads the two + keys as the same non-interactive signal. Forwarding it ungated reopened exactly + the gate ``_CONTEXT_INTERNAL_CALLER_KEYS`` exists to close. + """ + from app.gateway.services import build_run_config, merge_run_context_overrides + + config = build_run_config("thread-1", None, None) + merge_run_context_overrides( + config, + { + "github_token": "attacker-supplied", + "disable_clarification": True, + "agent_name": "coding-llm-gateway", + }, + ) + + assert "github_token" not in config["context"] + assert "disable_clarification" not in config["context"] + assert "github_token" not in config.get("configurable", {}) + assert "disable_clarification" not in config.get("configurable", {}) + # Whitelisted agent-config keys still come through for ordinary callers. + assert config["context"]["agent_name"] == "coding-llm-gateway" + + def test_merge_run_context_overrides_context_only_keys_do_not_override_existing(): - """A token already in ``config['context']`` must not be clobbered by a - client-supplied one (defense in depth — the manager is the only legitimate - source, but ``setdefault`` keeps the contract explicit).""" + """A token already in ``config['context']`` must not be clobbered by one supplied + in ``body.context`` (defense in depth — ``setdefault`` keeps the contract explicit + even now that only internal callers reach this branch).""" from app.gateway.services import build_run_config, merge_run_context_overrides config = build_run_config("thread-1", None, None) config["context"] = {"github_token": "pre-existing"} - merge_run_context_overrides(config, {"github_token": "attacker-supplied"}) + merge_run_context_overrides(config, {"github_token": "later-supplied"}, internal=True) assert config["context"]["github_token"] == "pre-existing" @@ -2916,6 +2948,71 @@ def test_strip_internal_context_keys_scrubs_config_smuggled_non_interactive(): assert "non_interactive" not in via_configurable["configurable"] +def test_strip_internal_context_keys_scrubs_config_smuggled_context_only_keys(): + """The context-only internal keys need the same ``body.config`` scrub as + ``non_interactive``: ``build_run_config`` copies both sections verbatim, so gating + ``merge_run_context_overrides`` alone still leaves ``body.config['context']`` open. + + The ``configurable`` half matters on its own — that dict is persisted in + checkpoints, so a smuggled ``github_token`` would write a live credential into the + checkpoint store even though no tool reads it from there. + """ + from app.gateway.services import build_run_config, strip_internal_context_keys + + via_context = build_run_config( + "thread-1", + {"context": {"github_token": "attacker-supplied", "disable_clarification": True, "model_name": "gpt"}}, + None, + ) + strip_internal_context_keys(via_context) + assert "github_token" not in via_context["context"] + assert "disable_clarification" not in via_context["context"] + assert via_context["context"]["model_name"] == "gpt" + + via_configurable = build_run_config( + "thread-1", + {"configurable": {"github_token": "attacker-supplied", "disable_clarification": True}}, + None, + ) + strip_internal_context_keys(via_configurable) + assert "github_token" not in via_configurable["configurable"] + assert "disable_clarification" not in via_configurable["configurable"] + + +def test_start_run_sequence_drops_context_only_keys_for_session_caller(): + """Replay the real ``start_run`` assembly order for a session-authenticated caller + that pushes the keys through *both* smuggling surfaces at once.""" + request = _make_request_with_auth_source("session") + config = _assemble_authz_run_config( + {"context": {"github_token": "via-config", "disable_clarification": True}}, + request, + body_context={"github_token": "via-body-context", "disable_clarification": True}, + ) + + assert "github_token" not in config["context"] + assert "disable_clarification" not in config["context"] + assert "github_token" not in config.get("configurable", {}) + assert "disable_clarification" not in config.get("configurable", {}) + + +def test_start_run_sequence_keeps_context_only_keys_for_internal_caller(): + """The channel path (internal auth) must keep carrying the minted token and the + non-interactive flag, and neither may land in checkpoint-persisted ``configurable``.""" + from app.gateway.internal_auth import INTERNAL_SYSTEM_ROLE + + request = _make_request_with_auth_source(AUTH_SOURCE_INTERNAL, system_role=INTERNAL_SYSTEM_ROLE) + config = _assemble_authz_run_config( + {}, + request, + body_context={"github_token": "ghs_installation_token", "disable_clarification": True}, + ) + + assert config["context"]["github_token"] == "ghs_installation_token" + assert config["context"]["disable_clarification"] is True + assert "github_token" not in config.get("configurable", {}) + assert "disable_clarification" not in config.get("configurable", {}) + + # --- Authorization identity anti-forgery tests --- diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 18d26d490..6bdc6b700 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -154,7 +154,7 @@ These span both layers and require reading multiple files to understand: tasks reuse the *same* Gateway run lifecycle (scheduler decides *when*, not *how*). - **Scheduled tasks** — workspace page `/workspace/scheduled-tasks` + a background scheduler gated by `config.yaml → scheduler.enabled`; non-interactive runs drop `ask_clarification` - and client-supplied `non_interactive`. + and client-supplied `non_interactive` (see the run-context trust boundary in §6). - **Long-running MCP** — a durable `McpTaskService` (leased rows, DB as source of truth) keeps remote task IDs/polling out of the agent loop. - **Version sources** — a release version must match in `backend/pyproject.toml`, @@ -172,6 +172,12 @@ These span both layers and require reading multiple files to understand: sandbox is dev-only direct execution. - **MCP isolation**: each MCP server runs in its own process with runtime env-var resolution; servers toggle independently. +- **Run-context trust boundary**: run context reaches the agent from two client-writable + surfaces — `body.context` and the free-form `body.config` — so every server-produced key + is gated on both. `non_interactive`, `disable_clarification`, and `github_token` are + honored only for internally-authenticated callers (the scheduler and IM/webhook channel + policies) and scrubbed from a non-internal caller's config; identity and sandbox + lifecycle fields are cleared unconditionally and restamped from auth state. - **Loopback-by-default ingress**: nginx is the only published surface; the Gateway's `8001` is container-internal and never published. A bare `"${PORT}:2026"` bind (0.0.0.0) is rejected by convention and CI. See the Security Notice in [`README.md`](../README.md) before