fix(ibac): Guard nil Session and bypass MCP housekeeping#443
Conversation
IBAC.OnRequest dereferences pctx.Session at plugin.go:252 without
checking for nil. The forward-proxy listener leaves Session nil
whenever no inbound A2A request has seeded the active session bucket
yet (server.go:205-209), which is the normal state at agent startup.
Result: every outbound-first request — most commonly an MCP
`initialize` to the agent's tool server — panics, and the agent
never gets its MCP transport open.
Two fixes:
1. Nil-Session guard. Before LastIntent(), explicitly check Session
is non-nil and fail closed with a distinct reason ("no_session")
so operator dashboards can tell "no inbound has happened yet"
apart from "session exists but contains no user intent"
("no_intent"). Both still 403 — neither state is safe to allow.
2. MCP housekeeping bypass. When mcp-parser populated the request
and the method is connection setup (initialize, ping), capability
discovery (tools/list, prompts/list, resources/list,
resources/templates/list), housekeeping (completion/complete,
logging/setLevel), or any notifications/*, emit Skip and continue.
These methods carry no user-actionable intent and judging them
would be a category error. Side-effect methods (tools/call,
prompts/get, resources/read) still reach the judge.
Tests: cover the nil-Session path, all 10 bypassed MCP methods, and
guard tools/call against accidentally being treated as housekeeping.
Closes rossoctl#442
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Kelly Abuelsaad <kna@us.ibm.com>
📝 WalkthroughWalkthroughIBAC plugin now guards against nil ChangesIBAC nil-session and MCP housekeeping bypass
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
authbridge/authlib/plugins/ibac/plugin_test.go (2)
355-374: ⚡ Quick winCover the other documented side-effect MCP methods too.
tools/callis locked down, butplugin.goalso treatsprompts/getandresources/readas judged paths. Adding them here would guard against accidentally broadeningisMCPHousekeepinglater.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@authbridge/authlib/plugins/ibac/plugin_test.go` around lines 355 - 374, Extend the existing test that ensures MCP side-effects are judged by adding cases for the other MCP methods handled as non-housekeeping in plugin.go: set pctx.Extensions.MCP.Method to "prompts/get" and "resources/read" (with appropriate Params if needed) and invoke invokeOnRequest for each, asserting fakeJudge.calls increments (or assert judge was called once per invocation) just like the existing "tools/call" case; you can either parameterize TestOnRequest_MCPToolsCallIsNotHousekeeping to loop over the three methods or add two small sibling tests, using the same helpers newConfiguredIBAC, makePCtx, invokeOnRequest, and fakeJudge to locate the relevant code paths.
319-352: ⚡ Quick winAssert the recorded
mcp_housekeepingreason here.This only proves the request continued and skipped the judge. A regression in
pctx.Skip("mcp_housekeeping")would still pass, even though that reason is part of the new operator-facing contract.Suggested test hardening
if fj.calls != 0 { t.Errorf("judge should not be called for housekeeping method %q", method) } + inv := lastInvocation(t, pctx) + if inv.Reason != "mcp_housekeeping" { + t.Errorf("Invocation reason = %q, want 'mcp_housekeeping' for method %q", inv.Reason, method) + } }) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@authbridge/authlib/plugins/ibac/plugin_test.go` around lines 319 - 352, In TestOnRequest_MCPHousekeepingBypass, after calling invokeOnRequest(p, pctx) add an assertion that the request was marked with the skip reason "mcp_housekeeping" (i.e. verify pctx recorded the skip); use whatever API exists on the pipeline context (e.g. pctx.IsSkipped("mcp_housekeeping") or assert that pctx.SkipReasons / pctx.Skipped contains "mcp_housekeeping") so the test ensures the specific skip reason was set in addition to Continue and no judge call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@authbridge/authlib/plugins/ibac/plugin_test.go`:
- Around line 355-374: Extend the existing test that ensures MCP side-effects
are judged by adding cases for the other MCP methods handled as non-housekeeping
in plugin.go: set pctx.Extensions.MCP.Method to "prompts/get" and
"resources/read" (with appropriate Params if needed) and invoke invokeOnRequest
for each, asserting fakeJudge.calls increments (or assert judge was called once
per invocation) just like the existing "tools/call" case; you can either
parameterize TestOnRequest_MCPToolsCallIsNotHousekeeping to loop over the three
methods or add two small sibling tests, using the same helpers
newConfiguredIBAC, makePCtx, invokeOnRequest, and fakeJudge to locate the
relevant code paths.
- Around line 319-352: In TestOnRequest_MCPHousekeepingBypass, after calling
invokeOnRequest(p, pctx) add an assertion that the request was marked with the
skip reason "mcp_housekeeping" (i.e. verify pctx recorded the skip); use
whatever API exists on the pipeline context (e.g.
pctx.IsSkipped("mcp_housekeeping") or assert that pctx.SkipReasons /
pctx.Skipped contains "mcp_housekeeping") so the test ensures the specific skip
reason was set in addition to Continue and no judge call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 011a3586-8dee-45d6-8909-e09449a29fc6
📒 Files selected for processing (2)
authbridge/authlib/plugins/ibac/plugin.goauthbridge/authlib/plugins/ibac/plugin_test.go
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Targeted fix for both bugs in #442 — the nil-Session panic on outbound-first MCP traffic and the no_intent denial of MCP protocol housekeeping. Approach (bootstrap bypass via housekeeping allowlist) is the right call given lazy MCP init likely isn't feasible in the agent runtime; ordering housekeeping bypass before the session check is correct and locked in by tests. The no_session vs no_intent split is good operator hygiene. CI green, single signed commit.
Three non-blocking suggestions inline. Two echo CodeRabbit's nits — they're both worth taking. Nothing must-fix.
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
| "resources/list", | ||
| "resources/templates/list", | ||
| "completion/complete", | ||
| "logging/setLevel": |
There was a problem hiding this comment.
suggestion: logging/setLevel is the only bypassed method that mutates server state (it changes the MCP server's log verbosity). It's low-impact and reasonable to bypass — but the function doc currently frames the allowlist as connection setup / capability discovery / housekeeping, which doesn't quite fit a state-changing call.
Worth a sentence in isMCPHousekeeping's doc explicitly justifying its inclusion (e.g. "transport-level config, not user-actionable intent") so a future reader doesn't pattern-match the list as "read-only methods" and broaden it to other state-changing methods on the same logic.
| _ = invokeOnRequest(p, pctx) | ||
|
|
||
| if fj.calls != 1 { | ||
| t.Errorf("judge calls = %d, want 1 (tools/call must be judged)", fj.calls) |
There was a problem hiding this comment.
nit (echoing CodeRabbit): tools/call is the canonical side-effect, but plugin.go doc also calls out prompts/get and resources/read as judged paths. Parameterizing this test (or adding two siblings) over all three would catch a future maintainer accidentally adding any of them to isMCPHousekeeping. Cheap defense-in-depth — the bug it prevents is hard to spot in review.
| t.Errorf("got %v, want Continue for housekeeping method %q", action.Type, method) | ||
| } | ||
| if fj.calls != 0 { | ||
| t.Errorf("judge should not be called for housekeeping method %q", method) |
There was a problem hiding this comment.
nit (echoing CodeRabbit): The test proves the judge wasn't called, but not that the recorded reason is mcp_housekeeping specifically. A typo'd pctx.Skip("mcp_housekeeping") (e.g. mcp_house_keeping) would still pass this test, even though the reason string is part of the new operator-facing contract per the PR description. Adding lastInvocation(t, pctx).Reason == "mcp_housekeeping" would lock that contract in.
github-project-automation
Bot
moved this from New /:ToDo
to Done
in Kagenti Issue Prioritization
3 participants
Summary
Fixes #442. Two related bugs in
authlib/plugins/ibacthat combine tobreak any agent whose first action is an outbound MCP call (e.g. an
initializeagainst its tool server at startup):Nil-pointer panic.
IBAC.OnRequestdereferencedpctx.Sessionat
plugin.go:252without a nil check. The forward-proxy listenerleaves
Sessionnil whenever no inbound A2A request has seeded theactive session bucket yet (
forwardproxy/server.go:205-209) — thenormal state at agent startup.
No concept of MCP protocol housekeeping. Even with the nil
guard, MCP
initialize/tools/list/notifications/*would bedenied as
no_intent, which is a category error: these methods areconnection setup and capability discovery, not user-actionable side
effects.
Changes
Bug 1 — nil-Session guard
Before
LastIntent(), checkSession != nilexplicitly and failclosed with a distinct
no_sessionreason. Kept separate fromno_intentso operator dashboards can tell "no inbound has happenedyet" apart from "session exists but contains no user intent". Both
still 403 — neither state is safe to allow.
Bug 2 — MCP housekeeping bypass (option a from #442)
New step #5 in
OnRequest, mirroring the existinginference_bypassshape: when
mcp-parserpopulatedpctx.Extensions.MCPand themethod is in the housekeeping set, emit
pctx.Skip("mcp_housekeeping")and continue. Bypassed methods:
initialize,pingtools/list,prompts/list,resources/list,resources/templates/listcompletion/complete,logging/setLevelnotifications/*(one-way protocol signals)Side-effect methods (
tools/call,prompts/get,resources/read)are unchanged — they still reach the judge.
Test plan
go test ./plugins/ibac/...passes (17 tests, 10 sub-tests)go test ./..., 29 packages)gofmtandgo vetcleanTestOnRequest_NilSession_FailsClosed— covers bug 1TestOnRequest_MCPHousekeepingBypass— table-driven over all 10 bypassed methods, withSession=nilto prove housekeeping bypasses before the session checkTestOnRequest_MCPToolsCallIsNotHousekeeping— guard against accidentally lettingtools/callslip throughReproducer (no longer panics)
Pipeline:
Agent does an MCP
initializeto its tool server at startup. Withthis PR, IBAC bypasses cleanly (
mcp_housekeeping); the agent's MCPtransport opens and subsequent
tools/calls still get judged once aninbound A2A request seeds the session.
Closes #442
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
Summary by CodeRabbit
Release Notes
Performance
Bug Fixes