d0zingcat/claude-code
1
0
Fork 0
mirror of https://github.com/instructkr/claude-code.git synced 2026-05-23 06:06:43 +00:00
Code Issues Packages Projects Releases Wiki Activity

docs(roadmap): add oauth authorize extra param collision gap

Browse Source
This commit is contained in:
Yeachan-Heo
2026-05-22 20:01:13 +00:00
parent 46f3bff7ef
commit 20c9d9d6c3
1 changed files with 4 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
ROADMAP.md
View File

@@ -6740,6 +6740,8 @@ Original filing (2026-04-18): the session emitted `SessionStart hook (completed)
593. **Plugin uninstall is a three-store non-atomic transaction: it deletes the plugin directory before persisting registry/settings cleanup, so an IO failure can leave registry and `enabledPlugins` pointing at a removed plugin** — dogfooded 2026-05-22 from the `#clawcode-building-in-public` 19:00 UTC nudge on `/home/bellman/Workspace/claw-code-pr2967` with branch/origin `docs/roadmap-workdir-provenance@244bdb7`. Active tmux session at probe time: `gajae-pr-358-review-session-vanish-replacement-review`; no active claw-code implementation session. Focused validation: `cd rust && cargo test -p plugins installs_enables_updates_and_uninstalls_external_plugins -- --nocapture` passed 1/1, confirming the happy-path lifecycle test does not cover interrupted persistence. Code inspection: `plugins/src/lib.rs::PluginManager::uninstall` removes the plugin record from the in-memory registry, then `remove_dir_all(record.install_path)`, then `store_registry(&registry)`, then `write_enabled_state(plugin_id, None)`. If directory removal succeeds but `store_registry` fails, `installed.json` still records the old plugin while the on-disk install directory is gone. If `store_registry` succeeds but `write_enabled_state` fails, the registry and disk are removed but `.claw/settings.json` can still contain `enabledPlugins[plugin_id]=true`. The next `plugin_registry_report` may silently prune the stale registry entry, but the enabled state can remain as orphaned configuration noise and the original uninstall command has already destroyed the install before it can report a clean transactional result. Existing `installs_enables_updates_and_uninstalls_external_plugins` asserts only the success path; it does not simulate registry write failure, settings write failure, or crash after `remove_dir_all`. **Required fix shape:** (a) make plugin uninstall a transaction with a tombstone/backup phase: first persist intended disabled/removed state or acquire a lock, then move the install directory to a same-filesystem trash/backup path, then update registry/settings, then remove backup after all metadata commits succeed; (b) if metadata cleanup fails, restore or report a recoverable tombstoned state with `rollback_available:true`; (c) make stale enabled settings for missing plugins produce a structured warning and auto-clean only through a safe metadata path; (d) add tests injecting failures after directory move/removal, after `store_registry`, and after `write_enabled_state`; (e) reuse the atomic JSON/write-lock helper from #508 so registry/settings writes cannot be partially written. **Why this matters:** uninstall is the destructive plugin lifecycle verb. Deleting files before committing metadata means a transient settings/registry IO failure turns a local cleanup into stale-branch-style plugin confusion: inventory can say installed/enabled while the executable hook/tool files are already gone. Source: gaebal-gajae dogfood response to Clawhip message `1507458055812546630` on 2026-05-22.
594. **MCP stdio frame reader trusts arbitrary `Content-Length` and allocates that many bytes before reading, so a buggy or hostile MCP server can OOM the runtime with one header** — dogfooded 2026-05-22 from the `#clawcode-building-in-public` 19:30 UTC nudge on `/home/bellman/Workspace/claw-code-pr2967` with branch/origin `docs/roadmap-workdir-provenance@d996b65`. Active tmux session at probe time: `gajae-pr-358-review-session-vanish-replacement-review`; no active claw-code implementation session. Focused validation: `cd rust && cargo test -p runtime mcp_stdio -- --nocapture` passed 22/22, confirming current MCP stdio tests cover normal/mismatched/lowercase frames but not oversized frames. Code inspection: `runtime/src/mcp_stdio.rs::McpStdioProcess::read_frame` parses `Content-Length` into `usize`, then immediately does `let mut payload = vec![0_u8; content_length]; self.stdout.read_exact(&mut payload).await?;`. There is no maximum frame size, no per-server byte budget, no early rejection on huge lengths, and no streaming cap. A server can send `Content-Length: 10000000000
594. **MCP stdio frame reader trusts arbitrary `Content-Length` and allocates that many bytes before reading, so a buggy or hostile MCP server can OOM the runtime with one header** — dogfooded 2026-05-22 from the `#clawcode-building-in-public` 19:30 UTC nudge on `/home/bellman/Workspace/claw-code-pr2967` with branch/origin `docs/roadmap-workdir-provenance@d996b65`. Active tmux session at probe time: `gajae-pr-358-review-session-vanish-replacement-review`; no active claw-code implementation session. Focused validation: `cd rust && cargo test -p runtime mcp_stdio -- --nocapture` passed 22/22, confirming current MCP stdio tests cover normal/mismatched/lowercase frames but not oversized frames. Code inspection: `runtime/src/mcp_stdio.rs::McpStdioProcess::read_frame` parses `Content-Length` into `usize`, then immediately does `let mut payload = vec![0_u8; content_length]; self.stdout.read_exact(&mut payload).await?;`. There is no maximum frame size, no per-server byte budget, no early rejection on huge lengths, and no streaming cap. A server can send `Content-Length: 10000000000
` (or any value near available memory) and force allocation before any payload bytes arrive; the surrounding `run_process_request` timeout does not protect the allocation itself. This is distinct from HTTP body caps (#503) and SSE parser buffering (#506): it is the MCP JSON-RPC stdio framing layer. Existing tests assert lowercase `Content-Length`, missing/mismatched IDs, timeout, and retry/reset behavior, but none assert a maximum accepted frame length. **Required fix shape:** (a) add a conservative `MAX_MCP_STDIO_FRAME_BYTES` default and optional per-server override; (b) after parsing `Content-Length`, reject values above the cap with `io::ErrorKind::InvalidData` carrying `content_length` and `max_frame_bytes`; (c) read the body through a bounded buffer/helper so allocation is capped and timeout/error surfaces stay typed as MCP invalid response; (d) add regression scripts that emit huge `Content-Length` with no body and oversized body, proving no large allocation and a structured invalid-response error; (e) include frame-size metadata in MCP degraded/error reports so operators can distinguish protocol abuse from transport EOF. **Why this matters:** MCP servers are extension processes. The client must treat their stdio as untrusted protocol input; one oversized length header should not be able to OOM a prompt startup, tool discovery, or resource read before degraded-mode reporting can fire. Source: gaebal-gajae dogfood response to Clawhip message `1507465601499660349` on 2026-05-22.
595. **OAuth authorize URL builder allows `extra_params` to override core PKCE/OAuth parameters after they were already set, so plugin/config extras can replace `state`, `code_challenge`, `redirect_uri`, or `response_type`** — dogfooded 2026-05-22 from the `#clawcode-building-in-public` 20:00 UTC nudge on `/home/bellman/Workspace/claw-code-pr2967` with branch/origin `docs/roadmap-workdir-provenance@46f3bff`. Active tmux sessions at probe time: none; no active claw-code implementation session. Focused validation: `cd rust && cargo test -p runtime oauth -- --nocapture` passed 9/9, confirming current OAuth tests cover happy-path URL/form/callback parsing but not reserved extra-param collisions. Code inspection: `runtime/src/oauth.rs::OAuthAuthorizationRequest::build_url` creates a `params` vector containing core parameters (`response_type=code`, `client_id`, `redirect_uri`, `scope`, `state`, `code_challenge`, `code_challenge_method`), then blindly `extend`s `self.extra_params` into the same query. `with_extra_param` accepts any key and stores it in a `BTreeMap`, with no reserved-name validation. A caller that sets `with_extra_param("state", "attacker")`, `code_challenge`, `redirect_uri`, `response_type`, `client_id`, or `scope` produces a URL with duplicate query parameters where the extra value appears after the core value. Because many OAuth parsers use last-value-wins semantics, this can desynchronize the locally expected state/PKCE verifier from the authorization-server-visible values, or change redirect/scope semantics. Jobdori separately filed duplicate callback parameters (#603); this is the outbound sibling: duplicates are generated by the client itself before the browser redirect, not just accepted on callback. **Required fix shape:** (a) define a reserved parameter set for OAuth authorization requests (`response_type`, `client_id`, `redirect_uri`, `scope`, `state`, `code_challenge`, `code_challenge_method`) and reject attempts to add them via `with_extra_param`; (b) make `with_extra_param` return `Result<Self, OAuthError>` or validate in `build_url` with a typed error rather than silently emitting duplicates; (c) add tests for reserved collisions (`state`, `code_challenge`, `redirect_uri`) and a safe extension like `login_hint`; (d) if an override is intentionally supported, make it explicit and update the stored expected state/verifier/redirect to match so callback/token exchange cannot drift; (e) document provider-specific extra params as additive-only. **Why this matters:** `state` and PKCE are the OAuth anti-CSRF/proof-of-possession controls. Letting arbitrary extras duplicate or override them in the authorization URL creates prompt/auth lifecycle ambiguity and can turn a provider-specific hint hook into a security-sensitive parameter injection footgun. Source: gaebal-gajae dogfood response to Clawhip message `1507473155273265172` on 2026-05-22.