Skip to content

fix(kernel): require verified caller identity for the split-view bridge - #672

Merged
kshivang merged 3 commits into
risa-labs-inc:devfrom
Antriksh1984:fix/splitview-caller-identity
Sep 15, 2026
Merged

kshivang merged 3 commits into
risa-labs-inc:devfrom
Antriksh1984:fix/splitview-caller-identity

Conversation

@Antriksh1984

@Antriksh1984 Antriksh1984 commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

Problem (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:

  • openUrlInActivePanel: force an arbitrary window's active panel to navigate to an attacker-chosen URL. No confirmation of any kind for an ordinary http(s) URL. A boss:// URL routes through the deep-link dispatch as DeepLinkOrigin.EXTERNAL - boss://terminal gets that tier's confirmation prompt, but per this repo's own AGENTS.md every other boss:// host (boss://plugin?id=… included) has no confirmation gate at all regardless of origin.
  • openFileInActivePanel/openFileInEditor/openFileInBrowser/openFileAtPosition: open an arbitrary file path in the user's editor or browser with no path confinement.
  • preserveCurrentState: overwrite a workspace snapshot under an attacker-chosen name.
  • setActivePanel/selectTabInPanel: manipulate which panel/tab is active.

Fix

Applies the same fail-closed pattern already used for the existing identity-enforcing kernel bridges: every unary RPC requires a verified AUTHENTICATED_PROCESS_ID (refusing PERMISSION_DENIED otherwise), and every mutation gets an attributed audit log line.

This change secures SplitView only. #666 addresses ProjectData separately; #53 tracks the remaining IPC surface. Authentication does not add per-plugin capabilities or path confinement.

Tests

New SplitViewServiceBridgeTest against a real gRPC server with ProcessIdentityInterceptor/ProcessTokenRegistry/ProcessTokenClientInterceptor:

  • every RPC refused with no credential, and never reaches the provider
  • an authenticated caller can open a URL, files and position
  • an authenticated caller can drive panel and workspace state
  • a revoked caller cannot invoke a unary RPC

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 - clean
  • CI: all ten checks passed on integrated head af19c69b50d49a3def03d962d0196dad65fd556e (run)

Contributor credit and review follow-up

Original security implementation and four real-gRPC tests by @Antriksh1984 (Antriksh). Codex review repairs clarify the documentation, verify the production late-registration path with a real BossIpcServer, assert forwarded arguments, and check that the refusal test exercises every declared RPC. The original enforcement behavior is retained.

Final integration validation

Merged current dev into this branch, retaining the equivalent shared documentation fixes already landed by #666. The final dev diff is limited to SplitViewServiceBridge and its tests. All 101 kernel-service tests passed locally, including five SplitView tests; detekt and desktop main/test ktlint checks passed. No live database deployment was performed.

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 (risa-labs-inc#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.
@kshivang

Copy link
Copy Markdown
Contributor

@claude Please review the current head 48b8c5a for correctness, caller-identity enforcement, and regression coverage.

@claude

claude Bot commented Sep 15, 2026 •

Copy link
Copy Markdown

Claude finished @kshivang's task in 5m 29s —— View job


Review of 48b8c5a — split-view bridge caller identity

  • Read the diff and the head commit
  • Verify every RPC in split_view.proto is gated
  • Verify the enforcement mechanism end to end (interceptor → registry → server wiring → client)
  • Compare against the sibling bridges that already carry this pattern
  • Assess regression coverage in the new test
  • Check the doc claims the PR makes about the rest of the IPC surface

Verdict

The enforcement itself is correct and complete for this service. All nine RPCs declared in modules/boss-ipc/src/main/proto/boss/ipc/v1/services/split_view.proto are overridden, and every one of them calls authenticatedCallerOrRefuse before touching provider, so the refusal is genuinely fail-closed rather than a post-hoc check. There are no streaming RPCs on this service, so the CURRENT_IDENTITY re-validation dance that ActiveTabsServiceBridge.watchActiveTabs / LogServiceBridge / GitServiceBridge need does not apply here — AUTHENTICATED_PROCESS_ID (a per-call snapshot) is the right key for a unary-only service.

I also confirmed the guard is not a silent total outage for legitimate callers, which is the failure mode BossIpcServer's own KDoc warns about: the kernel server is constructed with a real registry (KernelBootstrap.kt:485, BossIpcServer(kernelAddress!!, tokenRegistry)), and child processes attach the credential through ChildProcessBootstrap reading BOSS_PROCESS_TOKEN.

Findings below are ordered by what I'd want addressed; none of them is a hole in the guard itself.


1. The new KDoc contradicts itself about ProjectData, and names RunConfig wrongly (confirmed)

SplitViewServiceBridge.kt:16-17 lists ProjectData among the bridges the pattern has "since applied to", and SplitViewServiceBridge.kt:31-32 then says "ProjectData's identical fix is in flight separately". Both cannot be true, and the second one is the accurate one.

The list also names RunConfig, which has no identity check at this head. I grepped every bridge in composeApp/src/desktopMain/kotlin/ai/rever/boss/kernel/services/; only 9 of the 16 reference AUTHENTICATED_PROCESS_ID/CURRENT_IDENTITY. The ones with none are:

RunConfigServiceBridge, ProjectDataServiceBridge, NotificationServiceBridge, DirectoryPickerServiceBridge, PanelEventServiceBridge, PerformanceServiceBridge, ContextMenuServiceBridge.

The PR body makes the same RunConfig claim. Worth correcting in both places, since this KDoc is the running record of which of the #53 surface is closed. Fix this →

2. The ActiveTabsServiceBridge doc edit reads as an exhaustive remainder, and isn't

ActiveTabsServiceBridge.kt:27-28 now says "ProjectData still exposes unguarded operations on the same server". Given the six other unguarded bridges above, that sentence understates what is left. The pre-edit text ("SplitView and ProjectData") was equally non-exhaustive, so this is inherited rather than introduced — but the edit is the natural moment to make it either exhaustive or explicitly non-exhaustive ("ProjectData and several lower-risk bridges … still expose unguarded operations").

3. BossIpcServer's fail-closed list is now stale, and it is load-bearing (confirmed)

modules/boss-ipc/src/main/kotlin/ai/rever/boss/ipc/BossIpcServer.kt:36-42 says the set of services that fail closed on a missing identity is "currently SecretServiceBridge and RoleManagementServiceBridge", and draws a real conclusion from it: "Any kernel server hosting one of those bridges must be constructed with a real registry." SplitViewServiceBridge is the ninth member of that set and isn't added. A reader constructing a BossIpcServer without a registry consults exactly this list to decide whether that is safe. Either extend it or replace the enumeration with the rule ("any bridge under ai.rever.boss.kernel.services that calls authenticatedCallerOrRefuse").

4. Coverage gap: the test exercises a registration path SplitView never takes in production

The new test builds the server with build-time addService() (SplitViewServiceBridgeTest.kt:58-64). In production this bridge is added by KernelBootstrap.registerPluginServices() (KernelBootstrap.kt:982, ipcServer!!.addService(service)) to an already-started server, so it lands in BossIpcServer's fallback MutableHandlerRegistry rather than the build-time registry — a different resolution path for interceptors.

SecretServiceBridgeTest.kt:188-203 has a dedicated test for exactly this, with the reason spelled out: "if interceptors ever stopped covering fallback-resolved methods, the vault would be reachable anonymously instead of refused." Behaviourally I'd expect it to hold (it's proven for Secret), but the late path is the only path SplitView takes in production, which makes the case for mirroring that test stronger here than it was for Secret. ActiveTabsServiceBridgeTest also lacks it, so this is a pattern gap rather than something this PR regressed.

Fix this →

5. Coverage gap: the fake records method names only, so argument routing is unverified

FakeSplitViewOperations appends a bare string per call, so the two positive tests only prove which method ran. These would all still pass:

  • selectTabInPanel called with the arguments swapped — and note the proto declares panel_id = 1, tab_id = 2 while the Kotlin signature is (tabId, panelId), i.e. the opposite order, so this is precisely the call where a swap is easy and invisible;
  • preserveCurrentState(workspaceName, workspaceId) swapped;
  • line/column transposed in openFileAtPosition;
  • forceNewTab dropped (it is never set to true anywhere in the test).

Recording the arguments rather than the name is a one-line change per override and makes an authenticated caller can open a URL, files and position mean something beyond "nine methods are reachable". Fix this →

6. Nothing pins "every RPC on this service is gated"

The refusal test enumerates the nine RPCs by hand. A tenth RPC added to split_view.proto and overridden without authenticatedCallerOrRefuse reopens the hole with every test still green — which is the same class of silent drift this repo guards elsewhere with FileTypeCategoriesTest, OsOpenArgumentsTest and RetiredPluginsTest. A cheap generic guard: derive the expected set from SplitViewServiceBridge(provider).bindService().methods and assert it equals the set the refusal test actually exercised, so the new RPC fails a named test instead of shipping. Fix this →

7. Minor: the helper KDoc dropped the caveat its siblings carry

Secret, RoleManagement and ActiveTabs all open that helper's doc with "Unary RPCs only: AUTHENTICATED_PROCESS_ID is a per-call snapshot; use CURRENT_IDENTITY instead (see PluginUIServiceBridge.streamUI)". SplitViewServiceBridge.kt:105-108 drops it, and refuseIdentity drops the stage field from the log map. Harmless today (no streams here) — but that sentence is exactly the note that tells whoever adds a streaming RPC to this service what to reach for, and its absence is how a stream ends up checking identity only at bind.

8. Minor: applyWorkspace is gated but not logged

Correct as it stands, since the method is still a no-op stub and there is no mutation to attribute. But logMutation has to arrive with the implementation, and nothing in the file says so. One comment on the stub would carry that forward.

9. Observation: the audit lines omit the target

logMutation records rpc and caller but not what was requested. For this bridge the target is the interesting datum — an incident responder reading "caller x called openUrlInActivePanel" learns nothing about where the browser was sent, which is the whole substance of the vulnerability being closed. LogSanitizer.maskUriParams exists for the URL case; for file paths, omitting them is defensible (AGENTS.md notes project paths routinely carry usernames). Consistent with ActiveTabsServiceBridge.createBrowserTab, so this is a pattern-level call rather than a defect in this PR.

10. Operational risk worth confirming out-of-repo

SplitViewOperationsProxy (plugin-platform/plugin-api-ipc/.../SplitViewOperationsProxy.kt) wraps every stub call in catch (_: Exception) {}. So for a caller whose channel does not carry the credential, this change turns every split-view operation into a silent no-op — no plugin-side log, no user-visible error, the only signal being the host's WARN. The proxy has no construction site in this repo (it is consumed by the out-of-repo boss-microkernel-runtime), so it's worth confirming that runtime builds its channel through ChildProcessBootstrap (which attaches ProcessTokenClientInterceptor when BOSS_PROCESS_TOKEN is set) rather than a bare ManagedChannelBuilder. Mitigating: ActiveTabs, Git and Log already shipped the same guard, so if that wiring were wrong it would already be broken.

11. Minor, pattern-level: the refusal path logs WARN per call

An unauthenticated local process can spam any of these RPCs and get a WARN line each time. True of all nine guarded bridges, so not this PR's to fix, but the refusal log is unbounded and attacker-triggerable.


Things I checked and found clean

  • All nine proto RPCs overridden and gated; no RPC falls through to UNIMPLEMENTED unguarded.
  • Refusal precedes the provider call in every method, and the test asserts provider.calls.isEmpty() after the whole refused batch — the right shape for proving silence.
  • StatusException(PERMISSION_DENIED) matches the sibling contract; revoke → refusal is covered, and ProcessTokenRegistry.identityFor returns null for absent/blank/unknown/revoked alike, so the "unknown token" case is behaviourally identical to the tested "no token" one.
  • KDoc references ai.rever.boss.components.plugin.providers.SplitViewOperationsImpl and ai.rever.boss.utils.DeepLinkOrigin — both FQNs resolve, and the boss:// claim is accurate: SplitViewOperationsImpl.kt:39 routes through routePluginDeepLink(url, DeepLinkHandler::processDeepLink).
  • @Suppress("TooManyFunctions") justified (12 members now), and the added prose uses spaced hyphens — the one em-dash in the file is on a pre-existing comment line, which AGENTS.md puts out of scope.
  • No println/printStackTrace; BossLogger with LogCategory.AUTH, matching the siblings.

One caveat on method: I could not execute Gradle in this environment (the commands need approval here), so this is a static review — I did not independently re-run compileKotlinDesktop, desktopTest, ktlintCheck or detekt. The PR's own checklist reports all four clean, and CI is the remaining unchecked box.
· branch fix/splitview-caller-identity

@kshivang

Copy link
Copy Markdown
Contributor

Reviewed the original contribution and the Claude review. Antriksh's guard is correct: all nine unary RPCs reject missing identity before provider dispatch. The original implementation is preserved.

Review follow-up:

  • Findings 1-3: replaced contradictory/incomplete bridge enumerations with scope-specific rules in SplitView, ActiveTabs, BossIpcServer, and the PR description. This avoids claiming the entire IPC surface is secured.
  • Finding 4: added a real BossIpcServer test which starts the server first, then adds SplitView, and proves anonymous refusal plus authenticated dispatch through production late registration.
  • Finding 5: positive tests assert forwarded arguments, including forceNewTab=true, line/column, workspace id/name, and tab/panel order.
  • Finding 6: the refusal test compares actual RPC names observed by a recording server interceptor against the bound service descriptor, so an added RPC requires coverage.
  • Findings 7-8: documented unary snapshot semantics and the need for mutation auditing when applyWorkspace gains an implementation. No stage field is added: there is only one unary entry stage to distinguish here.
  • Finding 9: retained rpc/caller-only audit data. URLs and local paths may disclose sensitive data; changing the established audit payload is a separate policy decision, not required for identity enforcement.
  • Finding 10: inspected boss-microkernel-runtime/PluginProcessMain.kt: ChildProcessBootstrap.connect supplies connection.kernelClient.channel to RemotePluginContext and PluginUiClient. The runtime does use the authenticated bootstrap path.
  • Finding 11: WARN-rate limiting is a shared bridge concern, not introduced by this change. This PR retains the existing refusal logging policy.

#666 (ProjectData) and merged #634 (RunConfig) are distinct fixes, not duplicates. No other PR was modified or closed. Original code and initial four transport tests remain credited to @Antriksh1984; the additional documentation and coverage are Codex review repairs.

@kshivang
kshivang merged commit 315bfc8 into risa-labs-inc:dev Sep 15, 2026
10 checks passed
kshivang added a commit that referenced this pull request Sep 15, 2026
…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>
kshivang added a commit that referenced this pull request Sep 15, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants