Repository navigation
feat(mcp): add a session-wide action guard - #1365
Mihir-Rabari wants to merge 41 commits into
Conversation
|
Thanks for the session guard contribution. I reviewed the full submitted diff against current I am converting this to draft because the submitted head is not safe to merge as-is:
Please rebase onto current |
bdcf620 to
bc9aa3b
Compare
5341aa6 to
15813f2
Compare
|
Rebased the isolated session-guard commit onto current dev and narrowed the PR description to its actual McpToolRegistryCore.invoke coverage. Terminal-tab host tools have a separate server path and are outside this guard; the UI/audit wording now states the registry boundary. Fresh exact-head CI is running. |
|
@claude review this PR - security-sensitive changes, part of the backlog sweep. Focus on auth, signing, path handling, secrets, and injection surfaces. |
|
Claude finished @fluck-boss's task in 4m 10s —— View job Security review - MCP Session Action Guard
SummaryThe core design is sound. I verified the two properties this feature lives or dies on:
Also good: Three findings, one of which I'd call blocking. 1. The guard's mutating classifier is strictly narrower than the policy engine's ·
|
| Tool | Risk | In mutating catalog? |
|---|---|---|
docker_run |
CRITICAL | no (_run is not a suffix) |
k8s_use_context |
CRITICAL | no |
secret_create |
HIGH | no (_create is not a suffix) |
secret_update |
HIGH | no (_update is not a suffix) |
McpToolDefinition.readOnly defaults to true, so an ordinary, honest plugin that simply never overrides the default contributes docker_run as declaredReadOnly = true → isMutating returns false → tryAcquire hands out a no-op permit and the call executes while the operator believes mutations are paused. It is also counted as read-only, so it never appears in blockedMutatingActions.
Failure scenario, concretely: an agent earned an Always-Allow (or session trust) on docker_run earlier in the session - which is exactly the state that produces an agent worth pausing. The operator hits Pause Actions, the bottom bar turns alert-red, and docker_run keeps executing silently. Note the asymmetry this creates in the secrets surface the README highlights: secret_get is in the catalog and is blocked, while secret_create / secret_update are not.
The fix I'd suggest is one shared predicate rather than a second copy of the risk check - extract the risk >= HIGH || isMutating(...) expression from policyFor into McpMutatingToolCatalog (or a small McpMutationClassification helper) and call it from policyFor, blockIfPaused and tryAcquire, so the guard bucket can never again be a subset of the ASK bucket. Whichever way it goes, the AGENTS.md sentence should describe what the code does.
2. A durable ALLOW can be persisted during a pause, and the ledger hides it · McpToolRegistryImpl.kt:798-808
blockIfPaused only prevents new prompts. A prompt already on screen when the operator hits Pause still runs to completion inside authorizeInvocation → approvedAuthorization, which writes the persisted, tool-wide rule to mcp-tool-policy.json (or grants session trust) before tryAcquire refuses the call.
Two consequences:
- The emergency brake can leave the session more permissive on resume than it was when it was pulled - the operator's "Always Allow" survives the restart the guard deliberately does not.
- The audit record for that invocation reports
SESSION_GUARD_BLOCKED/ withheld only.dispositionis overwritten at line 801, so the ledger entry for the one call in which a durable policy change happened says nothing about it.
Neither is exploitable on its own, and it's defensible that an operator who clicks Always Allow meant it. But the guard's own AGENTS.md paragraph says "The guard does not persist or rewrite policies/kill-switches", which a reader will take to mean nothing persists while paused. Worth either rechecking the guard inside approvedAuthorization's persistence path (refusing the durable write the way a stale-revocation refusal already does), or recording the fact in the ledger and saying so in the docs.
3. README and the in-app string still carry the coverage overclaim that was narrowed in the PR body · README.md:159, McpActivityLogDialog.kt (sessionGuardDetail)
The PR description was narrowed to the McpToolRegistryCore.invoke boundary after @kshivang's review, but the two surfaces a user actually reads were not:
README.md: "block every new mutating tool call across all connected agents"sessionGuardDetail(): "Pause new mutating calls from every agent without changing saved policies."
Terminal-tab host tools go through a separate server path, so both overstate the guarantee at exactly the moment the operator is deciding whether the situation is contained. Suggest "every mutating tool call routed through BOSS" (or "through the BOSS MCP tool registry") in both.
Nits
McpSessionGuard.kt:97-(current.inFlightMutatingActions - 1).coerceAtLeast(0)cannot trigger today (the permit is idempotent and only handed out on increment), so the clamp only hides a future unbalanced release. An assertion or a warning log would be more useful than a silent floor.McpSessionGuard.kt:73-77-_state.valueis read three times inside onesynchronizedblock to build the increment. Correct, but a localval current = _state.valuematchesrecordBlocked/releaseMutatingright below it.BossBottomBar.kt:472- while paused, the status text replaces the last-operation line, so the✕error signal disappears for the duration. Given the operator paused because something went wrong, keeping the last op visible alongside the paused marker may read better.- Resume is a single click with no confirmation, and it is the risky direction (re-arming an agent mid-incident). Not required by anything in AGENTS.md - the confirmation rule there covers durable policy writes - but worth a thought.
Things I checked and found clean
- Auth/RBAC: the guard sits after the
_toolspermitted filter, adds no bypass, and does not interact withmcpToolPermitted.setMutatingActionsPausedis deliberately not on the plugin-facingMcpToolRegistryinterface (unlikesetToolEnabled), so a plugin cannot resume the brake through the API. - Injection:
SESSION_GUARD_BLOCK_MESSAGEis a constant - no$toolNamereflected back into the result or the UI. - Secrets: no new sanitizer surface; the new log line carries only an integer count; the ledger call is unchanged and still routes args through
McpArgumentSanitizer. - Signing / path handling: untouched by this diff.
- Enum growth:
unsuccessfulCategory's exhaustivewhenwas updated, preserving the compile-error-on-growth property feat(mcp): add a clickable activity log for recent tool calls #662 established.
I did not run Gradle here (no build in this environment); CI is the authority on ktlintCheck / detekt / desktopTest.
· branch feat/mcp-session-guard
|
Verified the classifier-mismatch finding against the real code - CONFIRMED and fixed (7be52fd). |
Added McpApprovalScope enum and related functions for approval flags and labels. Implemented McpApprovalDialog composable to handle user interactions for tool approval requests.
…ApprovalCountdown.kt) Implement McpApprovalDialog for AI agent tool approval with various scopes and user interactions.
…to-dev Sync main into dev
risa-labs-inc#1594) * fix(cli): keep an & in `boss <anything>` from running a second command boss.bat's smart detection tested the argument with `echo %arg% | findstr`. cmd substitutes %arg% into the line before it parses the line, so an & in the argument ended the echo and ran the rest as its own command, once per detection line. A file named R&D.txt routed nowhere, and `boss "x&echo side-effect>marker.txt"` wrote marker.txt. The "could not determine type" echoes read %arg% unquoted too. The URL and TLD checks are now quoted string tests (a prefix substring and a case-insensitive substring replacement), with no subshell, and the error echoes quote the argument. Routing is otherwise unchanged. test-boss-bat-urlencode.ps1 gains a source-shape check that :detect_and_route reads %arg% only inside quotes, which the Ubuntu leg enforces and which fails on the old script, plus live cmd.exe probes: the payload creates no marker, R&D.txt routes to boss://file with its full path, URL and domain detection give the same answers, and a mutation check shows the old findstr line does run the payload. Follows up risa-labs-inc#1347, whose empty-path half risa-labs-inc#1059 already fixed. * boss.bat: drop literal ampersands from REM comments Reword the two REM lines so they don't contain a bare & (flagged in review as a possible command split). Comment-only change. --------- Co-authored-by: fluck-boss <62e43a68001@mail.instinct.com>
…lways Allow (risa-labs-inc#1613) * fix(mcp): ask before a destructive shell command even under a saved Always Allow A saved ALLOW on run_command (or any shell tool) went straight to AUTO_ALLOWED, and the risk evaluator only ran on the ASK path to label the dialog. So one "Always Allow" for a harmless command auto-ran every later call, including rm -rf, git push --force and mkfs, which the evaluator itself rates CRITICAL. - McpToolRegistryCore.invoke now passes the saved policy through askBeforeDestructiveShell: an ALLOW on a shell tool drops to ASK when the call's own args rate CRITICAL. Every shell call already rates HIGH, so CRITICAL is the line that keeps "Always Allow" meaning "don't ask for routine calls". The args evaluated are the ones executed, and shell tools are identified by the evaluator's own normalization (the new DefaultMcpRiskEvaluator.isShellTool). A provider-wide "Trust This Plugin" ALLOW is covered too. Non-shell tools, DENY and ASK are unchanged. - The destructive-command matcher decides this now, so it reads each command of a chain for the flag shapes real calls take: rm with any recursive flag in any order or path (rm -fr, rm -r -f, /bin/rm -R, sudo rm --recursive), del/erase/rd/rmdir /s, Remove-Item -Recurse, and git push with a force flag anywhere after push. Runs of whitespace are collapsed. Every existing pattern still matches. - WorkspaceMcpToolProviderTest's newline check now uses a benign second line: its old `rm -rf /` payload is stopped earlier, at the approval gate, under that test's provider-wide ALLOW. Fixes risa-labs-inc#1577 * test(mcp): pin newline-chained and operand-first recursive rm as CRITICAL Add additional cleanup commands to the risk evaluation test. * fix(mcp): split shell chains before collapsing whitespace, read rm flags after operands * fix(mcp): escalate on >= CRITICAL so a higher tier stays fail-closed --------- Co-authored-by: fluck-boss <62e43a68001@mail.instinct.com>
…ot win (risa-labs-inc#1027) (risa-labs-inc#1622) * fix(browser): take ResolvedHostsStore save snapshot under saveLock `recordLoaded` captured `hosts.toList().sorted()` on the calling thread and only acquired `saveLock` inside the launched save. Two recordLoaded calls landing close together both captured the same snapshot, raced for the lock, and whichever won LAST overwrote the newer in-memory state with the older snapshot - silently dropping the host added by the first save. Moving the snapshot inside `saveLock.withLock` keeps the in-memory set (already a ConcurrentHashMap.newKeySet) as the source of truth while making the on-disk write reflect everything the caller could possibly have added by the time the lock was taken. Fixes risa-labs-inc#1307 Co-authored-by: Claude <noreply@anthropic.com> * fix(ci): ktlintFormat ResolvedHostsStore and its test * test(browser): make ResolvedHostsStore tests fail on the pre-fix race; correct comments The regression tests each ended with a fresh full-snapshot saveNowBlocking(), which masked the out-of-order write they were meant to catch, so all passed on the old call-site-snapshot code. Rework them to observe what the launched production saves actually leave on disk via a new test-only awaitPendingSaves() seam (no forced full write): a two-writer 'older never wins' test and a concurrent burst, both of which fail on the pre-fix snapshot and pass with the snapshot taken under saveLock. Also correct three production comments that overclaimed: the ConcurrentHashMap toList() is a weakly-consistent iteration, not a stable snapshot; there is no debounce; and saveNowBlocking is not the production write path (it runs inline). --------- Co-authored-by: choksi2212 <manaschoksiwork@gmail.com> Co-authored-by: Claude <noreply@anthropic.com>
risa-labs-inc#1527) * fix(terminal): queue concurrent send writers instead of fail-fast spin TerminalSession.send's tryLock-fail path now queues writers on a kotlinx.coroutines Mutex for a bounded 5s wait (injectable via inputQueueTimeoutMillis); callers that outwait the bound are shed with RESOURCE_EXHAUSTED carrying a google.rpc RetryInfo retry-delay via StatusProto instead of an instant spin-inducing failure. The queue wait suspends rather than parking a Dispatchers.IO thread, so a stalled pipe cannot pin the pool under load, and coroutine cancellation abandons a queued write instead of letting it through after the caller gave up; cancellation surfaces as CANCELLED. * fix(terminal): keep the NUL check on environment variable names Added additional null character check for environment variable values. --------- Co-authored-by: Raghav Bansal <273537979+Raghav-Bansal-15@users.noreply.github.com> Co-authored-by: fluck-boss <62e43a68001@mail.instinct.com>
Co-authored-by: kshivang <kshivang@users.noreply.github.com>
…abs-inc#563) * fix(wizard): skip a plugin only when it is actually installed, not merely recorded The first-run wizard's skip guard called DynamicPluginManager.isInstalled, which is pluginStates.containsKey. An entry is not a usable plugin. installPlugin registers a DISABLED entry for a jar it rejected as binary-incompatible and then deletes that jar, so the wizard announced "already installed, skipping" for a plugin that was not there, added its id to installedIds, and finished reporting success for something it never installed. The user picked it during setup and did not get it. This is the trap AGENTS.md already records for the dependency prompt, at a call site that was not converted with it. The same file was already using the correct predicate after the loop, for the dependency report. It now uses PluginDependencyResolution.installedAndOnDisk, the codebase's single definition: LOADED, or the jar exists and the plugin is not recorded as binary incompatible. Both clauses carry their weight here. Requiring only an entry is the bug above. Requiring only the jar would refuse to skip a running plugin whose file has moved, because neither PluginJarReconciler nor the updater repoints the manager's jarPath, and reinstalling it fails with "already loaded". Recomputed per call rather than hoisted, since the loop installs plugins and a snapshot taken before it would go stale exactly when a later entry depends on an earlier one. Six tests, written around the cases where the two predicates disagree, since the cases where they agree would pass either way. The last one pins the disagreement as a set difference, so a future change to either definition shows up as a changed disagreement rather than silently converging. 4405 composeApp tests pass. ktlint and detekt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(wizard): reject unusable install results before reporting success * fix(wizard): refuse to overwrite an installed plugin, rather than roll back Takes the bounded alternative offered in review, and collapses four commits of my own CI repairs into the change they were repairing. **Refuse an occupied destination.** Nothing already at the installed path is replaced. The previous attempt moved over it and rolled back on failure, and that grew a tail of its own failure paths: a backup that will not rename, a promotion that throws between backup and install, a restore that fails silently, a fixed-name backup that can destroy the only original left by an earlier failure. Each could lose the artifact the rollback existed to protect, and each needed an injected file-operation seam to test. Refusing has no tail: nothing is overwritten, so nothing needs undoing. It also makes cleanup unambiguous, because everything at the destination afterwards belongs to this call. **The signature travels with the jar.** `RemotePluginRepository` writes the sidecar beside the DOWNLOAD path, so promotion is when it has to move. Moving only the jar left every signed store install loading unsigned while its signature sat at the staging name. `SignedArtifact` moves and deletes the jar with whichever sidecar exists, carrying both suffixes the loader knows. **Both downloads stage.** The Store path used a fixed `-downloading.jar`, which a directory scan reads as a plugin and which two installs of one id collide on. Both paths now use a unique name whose marker breaks the `.jar` suffix, with one constant defining it so writer and reader cannot drift. `isPluginResident` answers what `isInstalled` cannot: that reports whether an entry exists, while `disablePlugin` leaves a DISABLED plugin resident in the loader, which is the case that made this reachable at all. 11 tests over real files and real moves: an occupied destination refused with its bytes intact and the loader never reached, a resident plugin refused before anything moves, the signature arriving with its jar, a failure removing both and leaving no orphan sidecar, a throwing loader not escaping the cleanup, and the staging name being unique, unscannable and reversible. Rebased onto current dev, which has now been rewritten four times today. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(wizard): make promotion all-or-nothing, and refuse the whole footprint Review found that refusing an occupied JAR does not make promotion indivisible, and it was right. `SignedArtifact.moveTo` moved the jar and then its sidecar, and `stageAndInstall` called it BEFORE the try/cleanup block. If the second move failed - the final `.sig` path being a nonempty directory is enough, and a permission change between the check and the move will do it too - the jar was left at its scannable installed name with its signature stranded at the staging name, nothing cleaned it up, and the occupied-destination refusal then blocked every retry. Three changes: **Promotion is inside the failure boundary.** `moveTo` records each path as it lands and deletes exactly those on failure, so a partial promotion leaves the destination as it was found. Deleting rather than moving back is deliberate: every path was established free beforehand, so deleting cannot destroy an artifact this call did not create, whereas a move back has a failure of its own that would leave the same mess. What is lost is a download, which the retry re-fetches. The loader is never reached. **The refusal covers the whole footprint**, not just the jar. A stale `.sig` beside a jar removed by hand is still somebody else's file, and promoting an unsigned download next to it would leave an old signature asserting bytes it never vetted - which PluginSignatureSidecar's own contract calls out as the thing never to do, and which is worse than no signature because the loader reads it as present-and-invalid. Refusing also means every path this call writes was established free, which is what makes the cleanup precise rather than indiscriminate. The message names the obstructing file so the user knows what to remove. **The destination-ownership claim is corrected, not restated.** The check and the move are two operations, so it holds against what is already on disk, not against a second installer racing this one. `ATOMIC_MOVE` cannot close that gap: on POSIX it is rename(2), which replaces the target silently - verified with a probe on this platform rather than assumed. Promotion therefore uses a plain non-replacing move, so a racing installer collides with FileAlreadyExistsException and this call reverts, instead of the two overwriting each other silently. Serialising installs is the only thing that would make the race impossible, and that does not belong in this function; the KDoc says so rather than implying a guarantee that is not there. Three regression tests over real files and real moves: a blocked destination sidecar (loader not called, no jar at the installed name, the obstruction and its contents untouched, and the retry succeeds once it is removed); an unsigned download meeting an old sidecar; and a promotion onto an occupied path failing rather than replacing it. Reported-by: kshivang Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(test): call the two-argument assertContains, not the Iterable overload `assertContains(charSequence, other, message)` binds the message to `ignoreCase: Boolean`, so overload resolution falls through to the Iterable and array candidates and the call does not compile. The message moves to a comment; the assertion is unchanged. This was already fixed in my working tree when 3d7f888 was pushed - the amend that should have carried it was part of a compound command that was denied, and I pushed the pre-fix commit without re-checking. The commit, not the working tree, is what CI builds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Shivang <shivang.iitk@gmail.com>
…lag-shaped branch cannot reach git checkout (risa-labs-inc#950) * fix(git): re-validate the remote-prefix-stripped checkout name so a flag-shaped remote branch cannot force-discard uncommitted work Fixes risa-labs-inc#919. checkout() ran isSafeRefName on the name as it arrives, but for a non-local name containing '/' it derives localName = substringAfter('/') and passes THAT to 'git checkout <localName> --' without re-validating. A remote-tracking branch literally named '-f' (origin/-f) passes the guard, strips to '-f', and the argv becomes [checkout, -f, --]: git parses -f as --force with an empty pathspec set and force-checks-out HEAD over the worktree, silently discarding every uncommitted modification. The post-strip name now passes the same isSafeRefName gate before it can reach the command line. 2 regression tests with a real lab repo: a planted origin/-f ref is refused with the PRECIOUS uncommitted content intact (and a safe slashed name still checks out carrying the change over), and a local branch whose whole name is a flag is refused on arrival. * fix(git): pass the window's project path on the top-bar checkout and pin the empty-strip and HEAD-stability contracts Re-audit follow-up on risa-labs-inc#919: the top-bar onCheckout was the one write verb of its cluster that omitted projectPathOverride, so with two windows on different repos the branch picker's checkout ran against whichever window last aligned the global current-project path - the wrong repository. It now passes windowProjectPath exactly like its neighboring pull/push/merge/rebase call sites. Two regression tests join the suite: 'origin/' strips to empty and is refused with the worktree intact, and a refusal leaves HEAD and the uncommitted content untouched. 4/4 tests, detekt and ktlint green.
fluck-boss
left a comment
fluck-boss
left a comment
There was a problem hiding this comment.
Thanks for this, @Mihir-Rabari - the direction is good and we want to land it. CI is red on the current head (7be52fd), so I am marking it changes requested and moving it to draft until that is fixed:
build-test (ubuntu-latest)fails to compile:McpPolicy.kt:178- unresolved referenceMcpToolArgs(the K/V inference errors on the same line follow from it). Looks like the branch has drifted fromdev; a rebase onto currentdevplus the right import/type should fix it.
Push the fix, keep working until the ubuntu and windows legs are green, then mark it ready for review and it goes straight back into the merge queue. Appreciate the work here!
fluck-boss
added
the
changes requested
…length-cap boundary (risa-labs-inc#1611) * fix(git): close the NEL and U+2028 gap in isSafeCloneUrl (risa-labs-inc#1602) The guard's comment claims the same control-character limits as isSafeRefName, but code < 0x20 || DEL misses the characters isWhitespace() covers there: NEL (U+0085), U+2028 and U+2029. Every whitespace character except the plain space is now refused with the C0 controls and DEL, so the comment is true again and the last line-break characters cannot forge a log line via the URL. Spaces stay allowed for local paths, and the local-path accept case for './ext::sh -c x' moved next to the other path pins. Also the risa-labs-inc#1602 follow-ups: an accept-side boundary test for MAX_CLONE_URL_LENGTH (4096 chars accepted, 4097 refused), and the control-character and length assertions split into separate tests so neither can mask the other. * fix(git): refuse NEL explicitly in isSafeCloneUrl isWhitespace() no longer reports U+0085: Unicode reclassified NEL from LINE SEPARATOR to CONTROL, so modern JDKs return false for both isSpaceChar and isWhitespace there. The previous round caught U+2028/U+2029 through the isWhitespace() clause but relied on it for NEL too, and the pinned NEL refusal test failed the hosted ubuntu leg. NEL is now refused by codepoint alongside the C0 controls and DEL, and the comments state the reclassification so the clause cannot be 'simplified' back into the gap.
…cap each one (risa-labs-inc#1148 follow-up) (risa-labs-inc#1625) * perf(loader): read only the class bytes the validator actually validates BinaryCompatibilityValidator checks a plugin's own `ai.rever.boss.plugin.*` classes against the host and deliberately ignores the third-party runtime a plugin bundles. It read every class's bytes anyway, up front and all at once, then skipped most of them. Measured on shipped plugins: boss-plugin-terminal-tab 2.5.74 9,469 classes 40.4MB read 0.50MB used boss-plugin-fluck-browser 1.2.29 2,452 classes 10.3MB read 2.69MB used Both figures are uncompressed class content held simultaneously, since the sequence was materialised with toList(). Entry names are genuinely needed for every class - the same-JAR reference check uses the full set - but names are not what costs, and a JarEntry is metadata. So the pass now collects names and entries, and reads bytes inside the loop, only for classes that pass the prefix filter. That also makes the code honour a rule it already states. The comment says a bundled third-party class "must not disable the plugin", but reading one that could not be read threw into the JAR-level catch, which returns isCompatible=false - and DynamicPluginLoaderImpl refuses the load on exactly that. A plugin could be rejected over bytes the validator had already decided were none of its business. Those bytes are no longer read, so they can no longer fail it. The read moved inside the constant-pool try rather than getting a catch of its own: a class this cannot read is exactly as informative as one it cannot parse, and the existing clause already treats that as "skip, not a compatibility failure". Two tests. One builds a JAR whose bundled entry is corrupt at the deflate level - the name still enumerates, reading throws - and asserts the plugin is not refused; against the previous validator it fails with "Failed to read JAR: invalid distance too far back", which is the defect exactly. The other pins the boundary: a JAR that cannot be opened at all is still refused, so reading less has not turned an unusable archive into a pass. plugin-loader suite 146/146, detekt and ktlintCheck clean across all modules. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(loader): cap each plugin class file before its class loader reads it Reading one class at a time bounds what the validator holds in total, but not what a single class can cost. Zeros deflate about 1000:1, so a 573 KiB JAR can carry a 576 MiB class file, and the plugin's own class loader reads the whole entry inside Class.forName to define it. With a 512 MiB heap that ran out of memory in forName, before the validator's own read was reached. Each of the plugin's own classes is now read first with readNBytes(MAX_CLASS_BYTES + 1) and refused past 8 MiB, and only then loaded. A cap checked after loading would be too late. Bundled third-party classes are still never read, so the cap never applies to them. The cap, its value, and reading before Class.forName are from choksi2212's BossConsole#1293, which found the single-class case. Together with the one-class-at-a-time read already on this branch, all three shapes of bad JAR are refused without exhausting the heap: many classes just under the cap, many plugin classes just under the cap, and one class past it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(loader): name the stage that failed, and pin both halves of the unreadable-entry rule From the review of f7a1bfe. The JAR-level catch used to wrap only the read pass. Reading bytes inside the loop moved `Class.forName` under it too, so a SecurityException from one class load (a sealed package, a signed-JAR digest mismatch) came back as "Failed to read JAR" and discarded every per-class error found before it. The catch now knows which own class it was on: a throw while opening is still "Failed to read JAR", a throw during the loop is reported against that class, and the earlier errors are kept. Still refused in both cases. The review suggested rewording the message as the cheap fix. That would have quietly weakened a test: the scope test asserts "Failed to read JAR" is ABSENT for an unreadable bundled class, and after a reword that assertion passes whether or not the bug returns. Splitting by stage keeps it meaningful. Tests for the behaviour nothing pinned: - a throw mid-validation names its class, keeps earlier errors, and does not blame the JAR - a corrupt copy of a class the host already provides is skipped (a deliberate loosening) - a corrupt class only the plugin provides is still refused, against that class - a JAR that never opens still says "Failed to read JAR" Mutation-checked: restoring the old catch fails only the first; skipping the loader when the bounded read fails lets a corrupt plugin class through and fails only the third. Nits: `OWN_CLASS_PREFIX` now used at the two `verifyReference` sites that still spelled it out; `validated` renamed `classCount`, since it counts every entry; and a comment on why the one unbounded read left, on the unreadable branch, cannot exceed the cap. plugin-loader 177/177; detekt and ktlintCheck clean across all modules. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
… + log forgery) (risa-labs-inc#1609) * fix(git): stop CloneProjectDialog logging the raw clone URL (risa-labs-inc#1602) The dialog's "Starting clone operation" line logged repositoryUrl raw, before GitService.cloneRepository runs, so no validation had seen it: a https://user:<PAT>@host clone URL reached the log file and the Console capture verbatim, and an interior control character could forge a second record, because BossLogger appends the data map to the line as-is. The URL now goes through cloneUrlForLog - LogSanitizer.maskUriParams, the same mask the service layer's own clone log uses, after stripping the C0 controls, DEL, NEL (U+0085) and U+2028/U+2029. Spaces survive, because a local clone path legitimately contains them. Pinned by CloneProjectDialogUrlLogTest: the masking behavior, and a source assertion that the log call carries cloneUrlForLog(repositoryUrl) and never the raw value. * fix(git): strip log-forging control chars from the clone target too Filter out log forging control characters from target directory in clone operation logging. * test: pin the clone log call with whitespace-tolerant patterns, including the filtered target --------- Co-authored-by: fluck-boss <62e43a68001@mail.instinct.com>
…sa-labs-inc#1488) * fix(mcp): persist the operation ledger off the invoking coroutine record() links the chained hash under writeLock (queue order == chain order) and only enqueues; a daemon writer owns the file and batches rotate/mkdirs/chmod/appendText per 256-record pass, with a drop counter on overflow. invoke's finally no longer hops to Dispatchers.IO, and tests drain via awaitIdle(). (Rebuilt onto dev; the earlier head carried unrelated main-merge commits.) * fix(mcp): rotate mid-batch at record boundaries Review/CI feedback: the first async version checked rotation once per batch, so a batch crossing the size cap appended past the limit. writeBatch now tracks the pending byte length and rotates at the same record boundary the synchronous writer would have, chunking appends across the rotation. * fix(mcp): keep the ledger chain contiguous and drain it on shutdown Review on risa-labs-inc#1488: hashing moved to the writer thread at append time, so a record that never reaches disk never enters the chain - a dropped or failed write is lost coverage, not a LINK_BROKEN tamper verdict. The shutdown sequence now drains the queue before the logger stops, queue drops warn (rate-limited) and count into droppedWrites, the activity dialog renders queued vs not-persisted, rotation re-stats after a failed rename, and record() keeps the Dispatchers.IO hop. Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * style: satisfy detekt MaxLineLength and ktlint when-brace rules * fix(mcp): publish ledger draft before queue offer to close markPersisted race record() offered to the write queue before inserting the draft into recentOperations, so a fast writer could persist the record and run markPersisted before the draft existed in the ring buffer - the by-id swap found nothing and the unhashed draft landed after, leaving the row hash==null forever. CI hit this on loaded runners; locally the caller usually wins the race. Insert the draft first so the swap always has a target. --------- Co-authored-by: Raghav Bansal <273537979+Raghav-Bansal-15@users.noreply.github.com> Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…nterfaces (risa-labs-inc#1037) (risa-labs-inc#1038) The stub Ktor server bound host = "0.0.0.0", exposing it on every network interface (the LAN) on port 8080. It is a minimal stub, so the exposure is low-value, but binding a local development stub to the LAN by default is the wrong posture. Bind 127.0.0.1 so it is reachable only from the local machine.
…labs-inc#1039) (risa-labs-inc#1040) revokeIfCurrent compared a credential token with ==, a short-circuiting content compare on a security-critical value. This is not an authentication boundary - authentication is the credentials map lookup of a 256-bit random token, where there is no practical timing attack - so nothing here was exploitable. But a security-critical module should not compare a credential with ==, so use java.security.MessageDigest.isEqual instead. Behaviour is otherwise identical: a matching current token still revokes, a non-matching or absent token still does not.
…, not URI.path (risa-labs-inc#1340) * fix(windows): resolve the code source through the filesystem provider, not URI.path `codeSource.location.toURI().path` returns only the URI's path component, and a file on a Windows network share does not keep its server there - the server is the URI's authority. `File` then resolved what was left against the current drive, so a share install computed `C:\software\BOSS\app\BOSS.jar` for `file://nas01/software/BOSS/app/BOSS.jar`. Nothing threw and nothing logged. Four callers acted on that path: the `boss://` protocol command and `shell\open\command` for every registered extension (both written straight into the registry), the StartMenuInternet browser candidate, and the JAR updater, whose `exists()` check failed so the update silently never ran. Three more used `File(uri)`, which rejects an authority outright and failed safe to null, costing the restart and the launcher their code-source fallback. One helper, `CodeSourceLocation`, replaces all seven. `Paths.get(uri)` hands the URI to the platform's filesystem provider, which restores the authority as a UNC root, and returns exactly the previous result for a drive-letter install - so a local install sees no behaviour change. Routing through it also lets WindowsDefaultBrowserHandler reach its development-executable lookup when the code source cannot be resolved, instead of giving up on the one case that shape describes. * fix(windows): answer the risa-labs-inc#1340 review - localhost, fail closed, log, hidden shares - CodeSourceLocation drops a `localhost` authority before Paths.get, as OsOpenArguments.pathFromFileUrl does: the Unix provider rejects every authority, so file://localhost/... resolved to null where URI.path answered it. A real server authority is untouched. - Each null from fileOf is logged at debug with its reason (no URL, the scheme, or the provider's exception class), never the path. - WindowsDefaultBrowserHandler fails closed on an unresolvable code source, as WindowsProtocolHandler does, instead of registering a BOSS.exe found under user.dir. The dev lookup stays for a code source that resolved to a classes directory. The logged path is masked. - UpdateInstaller: drop the dead try/catch around fileFor and its stale KDoc, reword getCurrentJarPath's message, and document that a hidden share (`apps$`) installs without a relaunch. The validator refuses `$` anywhere, which jpackage.app-path already hit before this change. - Tests: a localhost URL, a network-share install resolved end to end from its code-source URL through windowsLauncherPathFor, and the hidden-share outcome pinned.
…-labs-inc#1638) It is SECURITY DEFINER, takes a caller-supplied user id and checks nothing, so any signed-in user could ask how many active API keys any other user holds. The table already refuses that question: plugin_api_keys has RLS and its SELECT policy gives a reader their own rows only. The function answered it anyway. Tracked in risa-labs-inc#1590. It was the last of the five plugin-store SECURITY DEFINER RPCs a client could still execute; the other four were closed by 20260909130000, 20260910000000 and 20260911010000. Its only caller is the plugin-store Edge Function's key-limit check, which runs on the service-role client, so revoking client access loses nothing. A revoke rather than an own-keys check, because a check keeps an RPC reachable by every client for a question no client asks. Same shape as 20260923122000 (risa-labs-inc#994), and the same durability note: CREATE OR REPLACE keeps the ACL, DROP and CREATE would hand it back to authenticated through 20251023000014's default privileges. Tests: nine assertions, including the call itself made as authenticated for another user's id and refused with 42501 rather than answered with a 0 that would read as "no keys". Not run locally for want of a Postgres here; Database (pgTAP) is the check. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: fluck-boss <fluck-boss@users.noreply.github.com>
… store record (risa-labs-inc#1643) importWorkspace (Open from File) saved a Space under whatever id its JSON carried, with neither of the guards the MCP create path has (risa-labs-inc#926). An id of `Space_Themes` overwrote the theme store and wiped every Space's theme; `Last_Session_Set` broke session restore; `last-session` replaced the crash-recovery record - and that is exactly the id an exported Last Session carries, so opening one on another machine replaced THAT machine's record. - withImportableId(), beside withStableId(), re-mints an id that is a slot or would save over a reserved record; importWorkspace uses it. Re-minted rather than refused: the person asked for a layout and never chose the id. Not folded into withStableId, which the load scan uses and where the real record carries `last-session` legitimately. - isSpaceSlot folds case (risa-labs-inc#1590). Ids become file names, and APFS and NTFS fold case. - reservedWorkspaceStoreFileName moves from the MCP provider to WorkspaceIdentity.kt, unchanged but for its KDoc: it must move with the load scan's skips, and it is no longer the MCP path's alone. risa-labs-inc#1590's own example is already refused on the MCP path, by risa-labs-inc#926's case-insensitive file-name check; the unguarded path was the import. Tests drive the rule either side of a real WorkspaceFileManager, as IdlessWorkspaceImportTest does, plus a source check that importWorkspace applies it. Mutation-checked: each of the three changes, reverted, fails a named test. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: fluck-boss <fluck-boss@users.noreply.github.com>
…ce boundary (risa-labs-inc#1619) notification_post stored the agent-supplied `source` argument verbatim, so an agent posting with source: "System" produced an inbox entry indistinguishable from a genuine host notice; the string was the only origin signal and it came from the payload. Provenance is now stamped by the kernel-side boundary instead of trusted from the argument: - BossNotification carries an authoritative `origin` field (NotificationOrigin.HOST / AGENT). The serialized default is AGENT, so entries that predate the field or load with it missing fail closed: they cannot present as host origin. - NotificationCenter.post takes `origin` as a required parameter with no default, so every call site must answer who is posting at this boundary; the answer is never derived from caller-supplied text. - For AGENT posts the boundary demotes the caller-supplied label to a display string - "agent" bare, "agent: <label>" when one was offered, bounded to 80 characters - so agent text cannot masquerade as system origin regardless of the string sent. HOST posts keep the host's own label. - NotificationMcpToolProvider classifies every post that crosses the MCP boundary as AGENT, and both the post result and notifications_list expose `origin`, so a host notice and an agent notice stay distinguishable. The demotion lives in NotificationCenter rather than in the MCP provider so the contract cannot be bypassed by a caller that reaches post() without going through the provider: the only route to host presentation is host code explicitly claiming NotificationOrigin.HOST, which is a review-visible act. Fixes risa-labs-inc#1587
…talled predicate (risa-labs-inc#1640) - writeStagedDownload is tested on both halves of its contract: the whole stream lands, and a stream that dies mid-download removes its partial file and rethrows. The dying stream serves 16 KiB first, so bytes are on disk when it fails; a stream that failed on its first read would pass against a version that never deleted anything. - Both install paths are pinned to stageAndInstall by a source test (comments stripped, since the KDoc names atomicMoveFrom on purpose). Reverting a call site to an inline atomicMoveFrom kept every behavioural test green; now it fails one. - The resident refusal no longer deletes the download when the download IS the destination. stageAndInstall models that case explicitly, and in it the unconditional delete removed the installed jar - in the one refusal that exists because something is still running from that file. - DynamicPluginManager.isInstalled is renamed hasEntry rather than deleted. It still had five production callers (PluginRemoval x3, DependentRestartPrompt, and the manager's own uninstall-cancelled log), so deleting it would have broken plugin removal. The name was the hazard: an entry survives a rejected, deleted jar, which is the reading risa-labs-inc#563 proved wrong. Its KDoc now says so and points at installedAndOnDisk. - The KDoc displacement was already in order on the merged code; nothing to change. Mutation-checked: each guard's removal fails exactly one named test. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: fluck-boss <fluck-boss@users.noreply.github.com>
…icate The guard bucketed mutations with McpMutatingToolCatalog.isMutating alone, but the policy engine's ASK bucket is risk >= HIGH OR the catalog - and docker_run, k8s_use_context, secret_create and secret_update are HIGH/CRITICAL by name while matching no catalog entry or suffix. A provider declaring one of them readOnly would see it execute while the operator believed mutations were paused, and it never counted as blocked. The predicate now lives once in McpMutatingToolCatalog.isMutationClassified and is called from policyFor, blockIfPaused and tryAcquire, so the two buckets cannot drift. AGENTS.md updated to describe the shared predicate.
…review) McpPolicy.isMutationClassified evaluates the no-args risk shape through DefaultMcpRiskEvaluator.evaluateRisk(toolName, McpToolArgs(emptyMap())), but the file never imported ai.rever.boss.plugin.api.McpToolArgs - the ubuntu and compatibility legs failed on the unresolved reference, exactly as the review diagnosed. The same call already compiles in McpPolicyEngine.kt and McpPolicyManagerDialog.kt with this import.
7be52fd to
8c744e2
Compare
… baseline The session guard's admission code (blockIfPaused, tryAcquire, permit release in finally) lives inside McpToolRegistryCore, the placement the security review endorsed as the single execution seam. Those ~30 lines push the class past detekt's 600-line LargeClass threshold. The class is already frozen for TooManyFunctions in this baseline, so the crossed threshold is frozen the same way rather than restructuring the audited invoke path.
|
Rebased the branch onto current
Hosted checks green on 23f628e. Marking ready for review. |
|
@claude please review this exact head (23f628e) for merge into dev. Check that the change is correct and complete for its stated goal, that tests cover the risk being fixed (a test that passes without the fix is a finding), that the earlier review findings on this PR are addressed, and flag any security issues (secrets, injection, auth or approval bypass, path handling). Keep the review focused on this diff. |
|
Claude finished @fluck-boss's task in 5m 25s —— View job Review of
|
| Test | Fails without the fix? |
|---|---|
pause brakes high-risk names the mutating catalog does not list |
✅ all three assertions flip under isMutating-only |
pause after an approval is queued wins at the final execution seam |
✅ handlerCalled becomes true without the tryAcquire recheck |
pause does not cancel a mutating handler that already owns a permit |
✅ pins both the in-flight count and the return to zero |
paused registry audits a mutating refusal and still executes a read-only tool |
✅ drives the real core and the real ledger, not a stub |
pause reports admitted work as finishing and rejects later admissions |
✅ the double close() pins permit idempotence |
session action guard control pauses and resumes live |
✅ real Compose click through the state round trip |
The one thing not covered by a failing-before test is the durable-write behaviour in finding #1 - which is consistent with it not having been addressed.
· branch feat/mcp-session-guard
fluck-boss
left a comment
fluck-boss
left a comment
There was a problem hiding this comment.
Thanks @Mihir-Rabari - the feature is correct, the classifier regression is genuinely fixed, and the new tests fail without it (Claude's review of 23f628e). One item from the earlier review is still unanswered and needs closing before merge:
- An approval already on screen when Pause is hit still persists a durable ALLOW or session trust (
approvedAuthorizationabout :954,confirmApprovalabout :897). The call in hand is refused, but after Resume the session is more permissive than when the brake was pulled, and the ledger records onlySESSION_GUARD_BLOCKED. Please refuse the persist/trust write while paused, the same wayapprovedAuthorizationalready refuses a stale write whenrevocationVersionmoves, and add a test for it. - The README (
:159) and the in-app string (McpActivityLogDialog.kt:246) still overclaim coverage. Please bring them in line with the code. - Smaller: Pause/Resume aren't recorded in the ledger, while YOLO's switch is.
Changes requested + draft until then. Push, get the ubuntu and windows legs green, mark it ready for review, and it goes straight back into the merge queue.
924c565 to
7e62348
Compare
072a2bd to
2ae2565
Compare
This branch has not been deployed
13 participants
Description
Adds a one-click Session Action Guard to BOSS's governed MCP surface. When an agent starts behaving unexpectedly, the operator can open the MCP activity log and pause new mutating calls routed through the BOSS MCP tool registry across connected agents while read-only inspection remains available. The bottom bar shows the paused state, already-running mutations are counted as finishing, blocked attempts are audited, and resuming does not rewrite saved policies or per-tool kill-switches.
The critical race is covered: if a mutating call is waiting in an approval dialog when the operator pauses actions, the final admission check rejects it before plugin code starts.
🔄 Type of Change
📦 Version Impact
✅ Testing Checklist
🧪 Local Testing
🖥️ Platform Testing
🏗️ Build Verification
:composeApp:ktlintCheckand:composeApp:detektpass🔍 Code Quality
📸 Screenshots/Media
Before
Stopping an unexpected agent required finding and disabling individual tools or changing durable policies while calls could still be queued for approval.
After
The MCP activity log exposes Pause Actions / Resume Actions with live paused, finishing, and blocked counts. The bottom-bar MCP status turns into a persistent alert while the guard is active.
📝 Additional Context
🎯 Motivation and Context
BOSS already governs individual registry tools, but operators lacked a fast session-wide response when an agent began a surprising sequence of actions. A practical operator console needs a brake that can be hit immediately without taking away the read-only tools needed to understand the situation.
🧠 Implementation Details
McpSessionGuardwith one synchronized admission boundary and observableStateFlowstate.SESSION_GUARD_BLOCKEDdisposition and classifies them as withheld in the activity log.None. The control and disposition are additive, and the guard starts unpaused.
👀 Review Focus Areas
Test coverage
./gradlew :composeApp:desktopTest --tests "ai.rever.boss.mcp.*" --tests "ai.rever.boss.components.dialogs.McpActivityLogDialogTest" --tests "ai.rever.boss.components.dialogs.McpActivityLogLayoutTest" :composeApp:ktlintCheck :composeApp:detektCoverage boundary
The guard applies to calls routed through McpToolRegistryCore.invoke. Terminal-tab host tools use a separate server path and are outside this control. The UI and audit counts describe registry-routed MCP calls only.