Repository navigation
Review only: HOUSE-79 upstream v8.0.44, part 11 of 13: plugin host, home, commands - #23
ericlitman wants to merge 1 commit into
Conversation
|
This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again. |
🛑 13 blocking findings🔴 Critical · Keep stream usage buffering bounded 🔴 Critical · Do not trust a pre-existing shared shadow directory 🔴 Critical · Persist SIWC binding to the selected storage backend 🟡 Medium · Home latency RPC adds a prohibited post-connect timeout 🟡 Medium · Unix plugin loading now depends on an executable, writable 🟡 Medium · Do not carry the probe deadline past connection setup 🟡 Medium · Preserve origin-relative native library dependencies 🟡 Medium · Suffix parsing can produce inconsistent plugin model IDs 🔵 Low · Handle the deferred probe-client close error 🔵 Low · Handle the deferred source-file close error 🔵 Low · Leave deprecated management endpoint behavior unchanged 🔵 Low · Handle the deferred verification-file close error 🔵 Low · Preserve structured diagnostic fields WalkthroughThis review-only slice of the upstream
Since the last review
Repair promptRead the
Details
First review of this pull request. @unfret review · @unfret status · @unfret help |
![unfret[bot]](https://avatars.githubusercontent.com/in/4933690?s=60&v=4){kind=small}
There was a problem hiding this comment.
🛑 13 blocking findings
🔴 Critical · Keep stream usage buffering bounded
🔴 Critical · Do not trust a pre-existing shared shadow directory
🔴 Critical · Persist SIWC binding to the selected storage backend
🟡 Medium · Home latency RPC adds a prohibited post-connect timeout
🟡 Medium · Unix plugin loading now depends on an executable, writable os.TempDir
🟡 Medium · Do not carry the probe deadline past connection setup
🟡 Medium · Preserve origin-relative native library dependencies
🟡 Medium · Suffix parsing can produce inconsistent plugin model IDs
🔵 Low · Handle the deferred probe-client close error
🔵 Low · Handle the deferred source-file close error
🔵 Low · Leave deprecated management endpoint behavior unchanged
🔵 Low · Handle the deferred verification-file close error
🔵 Low · Preserve structured diagnostic fields
Check
| commandCtx, cancelCommand := context.WithTimeout(ctx, clusterLatencyTimeout) | ||
| raw, errReport := command.Do(commandCtx, "CLUSTER", "BALANCE", "LATENCY", string(payload)).Text() |
There was a problem hiding this comment.
🟡 Medium · Home latency RPC adds a prohibited post-connect timeout
This 3-second timeout cancels latency reports from an already established Home command client; the probe path repeats the issue at line 265. On a loaded or high-latency Home taking more than three seconds, otherwise valid reports and probes are discarded, leaving stale measurements and potentially incorrect rebalance decisions. These new post-connect timeouts are not among the repository's allowed exceptions. Also reported for this defect: - Remove the post-connect latency-report deadline (internal/home/cluster_latency.go:230-231): The new three-second context deadline applies to CLUSTER BALANCE LATENCY on an established command connection, not credential acquisition. A delayed response is forcibly aborted. AGENTS.md explicitly prohibits post-establishment network timeouts, and Home telemetry is not a listed exception. - Do not deadline established Home telemetry connections (internal/home/cluster_latency.go:230-231): This adds a three-second deadline to a control RPC on the established Home command pool. With ContextTimeoutEnabled, go-redis applies that deadline to network reads and writes, so a delayed response is aborted. AGENTS.md prohibits post-connection network timeouts outside its named exceptions; Home latency reporting is not an exception or credential acquisition.
| helps.ObservePluginExecutorStreamUsage(prepared.outputFormat.String(), lineBuffer, &streamUsage) | ||
| lineBuffer = nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Critical · Keep stream usage buffering bounded
Deleting if len(lineBuffer) > maxLineBufferSize removes the only visible bound on the usage accumulator. For an auth-backed plugin stream that emits neither a newline nor complete JSON, every forwarded byte remains buffered even while the downstream consumes it. A reproduction retained approximately 17.6 MiB after forwarding 16 MiB; continuing the stream can exhaust memory and terminate the proxy. Also reported for this defect: - Keep stream usage buffering bounded (internal/pluginhost/adapters_executors.go:959): Deleting if len(lineBuffer) > maxLineBufferSize { ... lineBuffer = nil } leaves no bound on an unfinished stream line. For an auth-backed plugin stream forwarding upstream chunks without a newline or complete JSON value, every byte remains buffered even while downstream drains normally. A reproducer retained roughly 9 MB after forwarding 8 MiB; continued input can exhaust the proxy heap. No replacement bound is visible in the host HTTP or stream bridges. - Partial stream frames can grow without bound (internal/pluginhost/adapters_executors.go:959): With usage reporting enabled, a plugin stream that continuously emits bytes without a newline and without forming complete JSON keeps appending to lineBuffer. This hunk deleted if len(lineBuffer) > maxLineBufferSize { ... lineBuffer = nil }, allowing an upstream stream to exhaust host memory and crash the proxy.
| stopCancel := context.AfterFunc(ctx, func() { _ = probe.Close() }) | ||
| defer func() { | ||
| stopCancel() | ||
| _ = probe.Close() |
There was a problem hiding this comment.
🔵 Low · Handle the deferred probe-client close error
The probe cleanup discards redis.Client.Close errors, so a transport cleanup failure is neither returned nor logged. AGENTS.md requires checking and logging errors from deferred cleanup; handling errors from the PING commands does not cover this operation.
| if errMkdir := os.MkdirAll(dir, 0o700); errMkdir != nil { | ||
| return "", errMkdir | ||
| } | ||
| return dir, nil |
There was a problem hiding this comment.
🔴 Critical · Do not trust a pre-existing shared shadow directory
On Unix with shared /tmp, another user can pre-create /tmp/cliproxy-pluginhost and the predictable PID directory, including symlinks into attacker-writable storage. MkdirAll(..., 0700) neither changes existing permissions nor validates ownership. The attacker can then replace a shadow library after its checksum is read but before dlopen, executing code as the proxy user. The new Unix shadow-loading path needs a genuinely private directory. Also reported for this defect: - Protect the Unix shadow directory from other users (internal/pluginhost/shadow.go:128-131): On Linux with a shared /tmp, another user can pre-create cliproxy-pluginhost with writable permissions or supply a symlink for the predictable process directory. MkdirAll accepts existing directories without validating ownership or permissions. That user can replace the shadow image after copying or hashing but before dlopen, causing arbitrary code execution as the proxy user. This is newly reachable through the Unix loader.
| loadPath, errShadow := shadowCopyPlugin(file) | ||
| if errShadow != nil { | ||
| return nil, errShadow | ||
| } | ||
| cPath := C.CString(loadPath) | ||
| defer C.free(unsafe.Pointer(cPath)) | ||
|
|
||
| handle := C.cliproxy_dlopen(cPath) | ||
| if handle == nil { | ||
| removeShadowPlugin(loadPath) | ||
| return nil, fmt.Errorf("dlopen %s: %s", file.Path, dlerrorString()) |
There was a problem hiding this comment.
🟡 Medium · Unix plugin loading now depends on an executable, writable os.TempDir
Open() no longer dlopens file.Path. It dlopens a copy that shadowCopyPlugin writes under os.TempDir()/cliproxy-pluginhost/pid- (see shadowPluginDir in shadow.go), and nothing falls back to the original path. Two common hardened setups now break plugin loading: - Temp dir mounted noexec, as on CIS-hardened /tmp or docker run --read-only --tmpfs /tmp, whose tmpfs defaults to noexec. dlopen of the copy fails with 'failed to map segment from shared object: Operation not permitted'. - Read-only root filesystem with no writable /tmp. MkdirAll or CreateTemp fails before dlopen is reached. In both cases every configured plugin fails to load (frontend auth, executors, interceptors), even though the same plugin files loaded from the plugin directory before this change.
| diskMap["use_siwc"] = true | ||
| diskMap["siwc"] = siwcData.ToMap() | ||
|
|
||
| return codex.WriteJSONFileAtomic(authFilePath, diskMap) |
There was a problem hiding this comment.
🔴 Critical · Persist SIWC binding to the selected storage backend
With PGSTORE_DSN or OBJECTSTORE configured, main supplies the backend's local mirror as AuthDir, but this save updates only that mirror. No backend Save or PersistAuthFiles call follows before reporting success and exiting. On the next startup, Bootstrap replaces the file from the unchanged remote record, losing the newly acquired SIWC credentials.
| loadPath, errShadow := shadowCopyPlugin(file) | ||
| if errShadow != nil { | ||
| return nil, errShadow | ||
| } | ||
| cPath := C.CString(loadPath) |
There was a problem hiding this comment.
🟡 Medium · Preserve origin-relative native library dependencies
Loading only the copied plugin from the temporary directory changes native dependency resolution. A plugin linked against a sibling library using @loader_path/libhelper.dylib now fails with Library not loaded; the original plugin loads successfully. This was reproduced on Darwin, and $ORIGIN dependencies have the equivalent problem on Linux. The previous Unix loader opened the original artifact location. Also reported for this defect: - Preserve relative native-library dependency resolution (internal/pluginhost/loader_unix.go:116): Loading the copied image changes the directory used by @loader_path dependencies on macOS and $ORIGIN dependencies on Linux, but only the primary library is copied. A plugin linked to an adjacent library therefore fails during dlopen despite loading successfully from its original location. This was reproduced with a dylib referencing @loader_path/libreview_dep.dylib.
Also affects:
| return false | ||
| } | ||
| defer func() { | ||
| _ = file.Close() |
There was a problem hiding this comment.
🔵 Low · Handle the deferred verification-file close error
shadowPluginMatches silently discards a Close failure after verifying an existing shadow image. This independently violates AGENTS.md's mandatory deferred-error handling rule and suppresses the cleanup diagnostic even when verification reports success.
| baseModel = strings.TrimSpace(parsed.ModelName) | ||
| modelInfo = a.lookupPluginModelInfo(baseModel) |
There was a problem hiding this comment.
🟡 Medium · Suffix parsing can produce inconsistent plugin model IDs
For a plugin-routed model not present in the registry, a recognized thinking suffix is stripped into baseModel here even though hasPluginThinkingTarget remains false. Cross-format translation then uses the suffix-free model while nativeReq.Model remains the original suffixed value, so the plugin can receive conflicting model identifiers and reject the request or execute the wrong model.
| defer func() { | ||
| if errClose := conn.Close(); errClose != nil { | ||
| log.WithError(errClose).Debug("Home auth dispatch connection close failed") | ||
| log.Debugf("Home auth dispatch connection close failed: %v", errClose) |
There was a problem hiding this comment.
🔵 Low · Preserve structured diagnostic fields
Replacing WithError(...).Debug(...) with Debugf removes the structured error field from logrus entries, contrary to AGENTS.md. The same regression occurs at client.go:1972,2245,2257. New formatted diagnostics in siwc_bind.go:151,157,169,174,179,190,197,204,215, cluster_balance.go:79,201,216, and cluster_latency.go:196 likewise place error or plan metadata only in message text. Preserve dynamic diagnostic values as structured fields.
|
Closing: the review scope changed. Eric (2026-10-27): "i don't want to fix anything that's an issue in upstream - our only goal here is to validate our changes". Upstream code is out of scope. Our fork's own changes on v8.0.44 get one review PR instead. Findings from this part are recorded in the HOUSE-79 adjudication record. None was a defect in our code. |
Review only. Do not merge. This PR is one part of the review of #13, the merge of upstream
v8.0.44into the fork. That PR's diff (966 files) is too large for one review round.c88d701f7bc0894cde517ca7f931f5c05749cd74). The base is that tree with only this part's files restored tomain(c265779f), so the diff is this part's files alone and the surrounding code is the real final code.go.sumare in no part's diff.Linear: HOUSE-79
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.