Repository navigation
fix(kernel): require verified caller identity for the project-data bridge - #666
Conversation
…idge BossConsole#53: ProjectDataServiceBridge had no identity check at all. Any process able to open a connection to the kernel IPC server - not only the plugins the host itself loaded - could watch every recently opened project's path (which routinely contains a username) with no credential, and could force an arbitrary window to switch its open project via selectProject. That is not only a UI surprise: a project switch publishes a ProjectChangeEvent on the application-wide event bus every installed plugin can subscribe to, so an unauthenticated caller could trigger that broadcast on demand. Applies the same fail-closed pattern already used for ActiveTabs, Download, Git, Log, PluginUI, RoleManagement, RunConfig and Supabase: every unary RPC requires AUTHENTICATED_PROCESS_ID, the watch stream captures CURRENT_IDENTITY before collecting and re-checks it on every emission so a revoked token stops a live stream, and every mutation gets an attributed audit log line. SplitView remains the one unguarded service on this server. Rewrote ProjectDataServiceBridgeTest against a real gRPC server (the existing tests called the bridge directly, which the new gating would have refused outright) - refusal-with-no-credential, authenticated mutation, revoked unary and revoked stream cases, plus the two existing routing guarantees now exercised over an authenticated channel: selectProject's UI-dispatcher confinement and watchRecentProjects reading the process-wide ProjectState singleton rather than a per-window mirror. Also folded the stale, repeatedly-drifting enumeration of which bridges fail closed out of BossIpcServer's own KDoc - it named only two of what are now nine - in favor of pointing at each bridge's own KDoc instead.
|
Claude finished @kshivang's task in 5m 2s —— View job Review of
|
| Bridge | Identity check |
|---|---|
| ContextMenu, DirectoryPicker, Notification, PanelEvent, Performance, RunConfig, SplitView | none |
| ActiveTabs, Download, Git, Log, PluginUI, RoleManagement, Secret, Supabase, ProjectData (this PR) | gated |
The ActiveTabs line was already incomplete before this PR, but it was not false about the bridges it named; the edit makes it affirmatively false. And the sharpest case is the one the PR body lists as already fixed: RunConfigServiceBridge has no gating whatsoever, and RunConfigServiceBridge.kt:46 hands provider.execute a caller-supplied command, workingDirectory and environmentVariables with no credential. The PR body's list of eight should read Secret where it reads RunConfig.
This is precisely the drift the PR removes from BossIpcServer's KDoc, re-created two files over — and with the count now inverted (one named as remaining, seven actually remaining), a reader reasonably concludes BossConsole#53 is one bridge from done. Suggest either dropping the sentence in favour of "BossConsole#53 tracks the remaining unguarded services on this server", mirroring the new BossIpcServer wording, or naming all seven. Fix this →
2. The "stops a live stream mid-flight" guarantee is materially weaker here than for ActiveTabs
ActiveTabsServiceBridge's KDoc carries a caveat this one omits: "Revocation is checked before each emission; idle streams are not proactively disconnected."
That caveat deserves to be louder on this bridge, not dropped. ActiveTabs' source flow churns constantly (every title and URL change), so "next emission" is milliseconds away. ProjectState.recentProjects changes only when a project is opened or removed — which can be hours. So a revoked watcher here keeps its stream open, and keeps the snapshot it already received, for as long as nobody opens a project. The PR body's "a revoked token stops a live stream mid-flight, not just new calls" is true only at the next project change. Worth one sentence in the KDoc so the next reader does not over-read the guarantee. Fix this →
3. Ninth verbatim copy of the identity helpers
authenticatedCallerOrRefuse + logMutation + refuseIdentity + NO_IDENTITY are now byte-identical (modulo the log noun) across nine files under kernel/services/. Not a blocker, and following the established pattern is the right call for a bounded security patch — but finding #1 is the concrete cost: the shape is duplicated often enough that the documentation of which bridges have it has now gone stale twice. A KernelBridgeIdentity helper in that package (an interface with defaults, or a file-level function pair) would collapse nine copies and give the stream-revocation shape one definition to point the KDoc at.
4. The plugin-side read path turns a permanent refusal into a 30-second retry loop
The PR flags that ProjectDataProviderProxy's three write methods swallow PERMISSION_DENIED. The read path has the complementary problem, and it is noisier: ProjectDataProviderProxy.kt:34-51 retries watchRecentProjects forever with backoff capped at 30 s on any non-cancellation exception. PERMISSION_DENIED is permanent, not transient — so a token-less or revoked child now reconnects and is refused every 30 s for the lifetime of the process, and each attempt writes a host-side WARN from refuseIdentity. Reachable in practice only for a live child whose token was revoked (e.g. a reap misdetection), since ChildProcessBootstrap otherwise always presents one — so I would not block on it, but it belongs in the "known, accepted gap" section alongside the write path, since the correct handling (treat PERMISSION_DENIED as terminal rather than backing off) is cheap and local to the proxy.
5. Test nits
None of these affect whether the tests prove the claim; all are about them staying trustworthy.
startServeris bypassed byselectProject switches from the grpc caller to the UI dispatcher(ProjectDataServiceBridgeTest.kt:188-204), which re-implements 18 lines of setup so it can pass a dispatcher. An optionaluiDispatcher: CoroutineDispatcher = Dispatchers.Mainparameter onstartServerremoves the duplicate. The duplicate is already asymmetric — it assignsanonymousChannelbut neveranonymous, so the copies have quietly diverged on the first edit.tearDown(:92) dereferenceslateinitfields unconditionally. IfServerBuilder.start()or a channel build throws, the test failure is reported asUninitializedPropertyAccessExceptionand the real cause is lost. Guarding with::server.isInitializedkeeps failures legible.assertEquals(expected, emissions.getOrNull(0))(:269) compares against a snapshot of mutable process-wide state read at:251, before the subscription at:261.ProjectState's one-shot asyncloadRecentProjectsreplaces the whole list and can land in that window — the test's ownawaitInRecentProjectsKDoc is written around exactly that hazard.assertTrue(seed.path in emissions[0])plus the existingassertNotEquals(listOf(decoy.path), …)proves the same routing claim without depending on exact list equality across a window where the singleton can change under it.RecordingProjectDataProvider.callsis a plainArrayListwritten from gRPC executor threads and read from the test thread at:132. The RPCs are sequential so it is safe in practice, but in the refusal test the failure mode of an unsynchronized read is a false pass — the one place it is worth aCopyOnWriteArrayList.
I did not run the build or test suite in this review (the PR body reports :composeApp:desktopTest --tests "ai.rever.boss.kernel.services.*", ktlint and detekt all clean); the findings above are from reading the diff and the surrounding call sites. ProjectDataServiceBridge is at 7 functions, well under detekt's TooManyFunctions ceiling, so it correctly needs no @Suppress unlike ActiveTabs.
· branch fix/project-data-caller-identity
|
@Antriksh1984 the project-data guards close a useful missing authentication boundary, and the real-gRPC tests preserve the prior project routing guarantees. Reviewed the original head and Claude's current-head review; the original six focused tests passed locally and the original CI was green. Pushed review repairs to the original branch (repair ea55480, integrated head f5a3577):
Merged current dev 4a8a43b into this feature branch without changing shared dev/main. New validation and CI are running. Production identity enforcement is unchanged from the reviewed contributor implementation. Contributor provenance is preserved in the description. Related #672 shares only the ActiveTabs security documentation; its SplitView implementation is distinct, and #669 concerns the core control plane. No other PR was changed. |
|
Merged into dev as d55cf0b after all 10 CI checks passed on ed941b7 and local main/test ktlint plus all six focused gRPC tests passed. The shared merge lock rechecked the reviewed head, current dev, clean mergeability and checks; no checks were bypassed. Claude findings are addressed as recorded above. Antriksh remains the squash author, with the original security implementation and review repairs distinguished in the PR description. |
…ge (#672) * fix(kernel): require verified caller identity for the split-view bridge BossConsole#53: SplitViewServiceBridge had no identity check at all. Any process able to open a connection to the kernel IPC server - not only the plugins the host itself loaded - could force an arbitrary window's active panel to navigate to an attacker-chosen URL via openUrlInActivePanel (with no confirmation for an ordinary http(s) URL, and no confirmation gate at all for most boss:// hosts), open an arbitrary file path with no confinement via openFileInActivePanel/ openFileInEditor/openFileInBrowser/openFileAtPosition, manipulate panel/tab selection, or overwrite a workspace snapshot under an attacker-chosen name via preserveCurrentState. Applies the same fail-closed pattern already used for ActiveTabs, Download, Git, Log, PluginUI, RoleManagement, RunConfig and Supabase: every unary RPC requires a verified AUTHENTICATED_PROCESS_ID, and every mutation gets an attributed audit log line. This is the last bridge in this PR's own scope; ProjectData's identical fix is in flight separately (#666). New SplitViewServiceBridgeTest against a real gRPC server: every RPC refused with no credential and never reaching the provider, an authenticated caller can drive every operation, and a revoked caller cannot invoke a unary RPC. * test(kernel): verify split-view identity coverage and clarify scope --------- Co-authored-by: Shivang <shivang.iitk@gmail.com>
…idge (#666) * fix(kernel): require verified caller identity for the project-data bridge BossConsole#53: ProjectDataServiceBridge had no identity check at all. Any process able to open a connection to the kernel IPC server - not only the plugins the host itself loaded - could watch every recently opened project's path (which routinely contains a username) with no credential, and could force an arbitrary window to switch its open project via selectProject. That is not only a UI surprise: a project switch publishes a ProjectChangeEvent on the application-wide event bus every installed plugin can subscribe to, so an unauthenticated caller could trigger that broadcast on demand. Applies the same fail-closed pattern already used for ActiveTabs, Download, Git, Log, PluginUI, RoleManagement, RunConfig and Supabase: every unary RPC requires AUTHENTICATED_PROCESS_ID, the watch stream captures CURRENT_IDENTITY before collecting and re-checks it on every emission so a revoked token stops a live stream, and every mutation gets an attributed audit log line. SplitView remains the one unguarded service on this server. Rewrote ProjectDataServiceBridgeTest against a real gRPC server (the existing tests called the bridge directly, which the new gating would have refused outright) - refusal-with-no-credential, authenticated mutation, revoked unary and revoked stream cases, plus the two existing routing guarantees now exercised over an authenticated channel: selectProject's UI-dispatcher confinement and watchRecentProjects reading the process-wide ProjectState singleton rather than a per-window mirror. Also folded the stale, repeatedly-drifting enumeration of which bridges fail closed out of BossIpcServer's own KDoc - it named only two of what are now nine - in favor of pointing at each bridge's own KDoc instead. * fix(review): clarify project identity guarantees and harden test lifecycle * style(test): keep filtered flow chain continuation together --------- Co-authored-by: Shivang <shivang.iitk@gmail.com>
…ge (#672) * fix(kernel): require verified caller identity for the split-view bridge BossConsole#53: SplitViewServiceBridge had no identity check at all. Any process able to open a connection to the kernel IPC server - not only the plugins the host itself loaded - could force an arbitrary window's active panel to navigate to an attacker-chosen URL via openUrlInActivePanel (with no confirmation for an ordinary http(s) URL, and no confirmation gate at all for most boss:// hosts), open an arbitrary file path with no confinement via openFileInActivePanel/ openFileInEditor/openFileInBrowser/openFileAtPosition, manipulate panel/tab selection, or overwrite a workspace snapshot under an attacker-chosen name via preserveCurrentState. Applies the same fail-closed pattern already used for ActiveTabs, Download, Git, Log, PluginUI, RoleManagement, RunConfig and Supabase: every unary RPC requires a verified AUTHENTICATED_PROCESS_ID, and every mutation gets an attributed audit log line. This is the last bridge in this PR's own scope; ProjectData's identical fix is in flight separately (#666). New SplitViewServiceBridgeTest against a real gRPC server: every RPC refused with no credential and never reaching the provider, an authenticated caller can drive every operation, and a revoked caller cannot invoke a unary RPC. * test(kernel): verify split-view identity coverage and clarify scope --------- Co-authored-by: Shivang <shivang.iitk@gmail.com>
…idge (#666) * fix(kernel): require verified caller identity for the project-data bridge BossConsole#53: ProjectDataServiceBridge had no identity check at all. Any process able to open a connection to the kernel IPC server - not only the plugins the host itself loaded - could watch every recently opened project's path (which routinely contains a username) with no credential, and could force an arbitrary window to switch its open project via selectProject. That is not only a UI surprise: a project switch publishes a ProjectChangeEvent on the application-wide event bus every installed plugin can subscribe to, so an unauthenticated caller could trigger that broadcast on demand. Applies the same fail-closed pattern already used for ActiveTabs, Download, Git, Log, PluginUI, RoleManagement, RunConfig and Supabase: every unary RPC requires AUTHENTICATED_PROCESS_ID, the watch stream captures CURRENT_IDENTITY before collecting and re-checks it on every emission so a revoked token stops a live stream, and every mutation gets an attributed audit log line. SplitView remains the one unguarded service on this server. Rewrote ProjectDataServiceBridgeTest against a real gRPC server (the existing tests called the bridge directly, which the new gating would have refused outright) - refusal-with-no-credential, authenticated mutation, revoked unary and revoked stream cases, plus the two existing routing guarantees now exercised over an authenticated channel: selectProject's UI-dispatcher confinement and watchRecentProjects reading the process-wide ProjectState singleton rather than a per-window mirror. Also folded the stale, repeatedly-drifting enumeration of which bridges fail closed out of BossIpcServer's own KDoc - it named only two of what are now nine - in favor of pointing at each bridge's own KDoc instead. * fix(review): clarify project identity guarantees and harden test lifecycle * style(test): keep filtered flow chain continuation together --------- Co-authored-by: Shivang <shivang.iitk@gmail.com>
…ge (#672) * fix(kernel): require verified caller identity for the split-view bridge BossConsole#53: SplitViewServiceBridge had no identity check at all. Any process able to open a connection to the kernel IPC server - not only the plugins the host itself loaded - could force an arbitrary window's active panel to navigate to an attacker-chosen URL via openUrlInActivePanel (with no confirmation for an ordinary http(s) URL, and no confirmation gate at all for most boss:// hosts), open an arbitrary file path with no confinement via openFileInActivePanel/ openFileInEditor/openFileInBrowser/openFileAtPosition, manipulate panel/tab selection, or overwrite a workspace snapshot under an attacker-chosen name via preserveCurrentState. Applies the same fail-closed pattern already used for ActiveTabs, Download, Git, Log, PluginUI, RoleManagement, RunConfig and Supabase: every unary RPC requires a verified AUTHENTICATED_PROCESS_ID, and every mutation gets an attributed audit log line. This is the last bridge in this PR's own scope; ProjectData's identical fix is in flight separately (#666). New SplitViewServiceBridgeTest against a real gRPC server: every RPC refused with no credential and never reaching the provider, an authenticated caller can drive every operation, and a revoked caller cannot invoke a unary RPC. * test(kernel): verify split-view identity coverage and clarify scope --------- Co-authored-by: Shivang <shivang.iitk@gmail.com>
Problem
ProjectDataServiceBridgehad no identity check at all - BossConsole#53's original scope named ~17 kernel service bridges with this gap, and this repo has been closing them one at a time (ActiveTabs, Download, Git, Log, PluginUI, RoleManagement, Secret, Supabase). This was the next one still open, and it's more than a read -selectProjectmutates state cross-window.Concretely, before this fix, any process able to open a connection to the kernel IPC server - not only the plugins the host itself loaded - could:
watchRecentProjects: see every recently opened project's path with no credential. Project paths routinely contain a username, per this repo's own AGENTS.md.selectProject: force an arbitrary window to switch its open project. That's not only a UI surprise - a project switch publishes aProjectChangeEventon the application-wide event bus every installed plugin can subscribe to, so an unauthenticated caller could trigger that broadcast on demand and hand every plugin a filesystem path it never asked to see.updateRecentProjects/removeRecentProject: mutate the operator's global recent-projects list with no attribution.Fix
Applied the same fail-closed pattern already established for the already gated bridges (
ProcessIdentityInterceptor+AUTHENTICATED_PROCESS_ID/CURRENT_IDENTITY, introduced for Secret Service in PR #505):AUTHENTICATED_PROCESS_ID, refusingPERMISSION_DENIEDotherwise.watchRecentProjectscapturesCURRENT_IDENTITYbefore the flow starts collecting and re-checks it on every emission, so a revoked token prevents the next snapshot from being delivered. Idle streams are not proactively disconnected, and already delivered snapshots cannot be recalled.BossConsole#53 tracks the remaining unguarded services on this server; this PR closes only the project-data bridge.
Tests
Rewrote
ProjectDataServiceBridgeTestagainst a real gRPC server withProcessIdentityInterceptor/ProcessTokenRegistry/ProcessTokenClientInterceptor(the existing tests called the bridge directly, which the new gating refuses outright with no credential in scope):selectProject's UI-dispatcher confinement,watchRecentProjectsreading the process-wideProjectStatesingleton rather than a per-window mirror), now exercised over an authenticated channelAlso folded the stale, repeatedly-drifting enumeration of which bridges fail closed out of
BossIpcServer's own KDoc (it named only two of what are now nine) in favor of pointing at each bridge's own KDoc.Known, accepted gap
ProjectDataProviderProxy(the plugin-side consumer inplugin-platform/plugin-api-ipc, consumed by the out-of-repo microkernel runtime - no in-repo construction site) silently swallows every RPC exception on its three write methods, same as it did before this fix for any failure. APERMISSION_DENIEDfrom a stale/revoked token would be a silent no-op there. UnlikeRunConfigurationDataProvider,ProjectDataProvider's plugin-facing interface has no error-reporting field to surface it through without a plugin-API surface change - flagging rather than fixing in this bounded patch.The proxy read path also retries non-cancellation errors, including
PERMISSION_DENIED, with backoff capped at 30 seconds. This existing retry policy can produce repeated refusals for a revoked live child. Treating permanent authentication failures as terminal and sharing bridge identity helpers remain separate follow-ups.Test plan
./gradlew :composeApp:compileKotlinDesktop :composeApp:compileTestKotlinDesktop- clean./gradlew :composeApp:desktopTest --tests "ai.rever.boss.kernel.services.*"- all pass./gradlew :composeApp:ktlintDesktopMainSourceSetFormat :composeApp:ktlintDesktopTestSourceSetFormat :composeApp:detekt :boss-ipc:ktlintFormat :boss-ipc:detekt- cleanContributor and review provenance
@Antriksh1984 authored the verified-identity guards, attributed mutation logging, per-emission stream revocation and real-gRPC regression tests. Review confirmed host-side placement and preserved the prior project-state routing fixes; review-agent repairs clarified the security KDoc and idle-stream semantics, unified test-server setup, guarded partial fixture cleanup, made call recording thread-safe and removed a startup-load race from the routing test. Current dev was merged into the feature branch for integration validation. The production identity guards remain the contributor's implementation.