[logging] inspector: consume backend InternalLogContext in authorize wrapper

[Vu-John]

Summary

Adds a security-safe wrapper around authorizeServer and authorizeBatch that strips the optional internalLogContext field (returned by the backend authorize endpoint) from the Convex response and merges it into c.var.requestLogContext. This ensures backend-private identity context flows into Axiom events without ever reaching the browser. The signature of both functions now takes c: Context as the first argument so the log context can be side-effected without callers having to handle it.

Changes

• server/routes/web/auth.ts — added InternalLogContext type, ClientSafeAuthorizeResponse export, mapInternalToRequestContext helper; updated authorizeServer, authorizeBatch, createAuthorizedManager to take c: Context as first arg and strip/merge internalLogContext; withEphemeralConnection passes c to createAuthorizedManager
• server/routes/web/servers.ts — updated 2 authorizeServer call sites
• server/routes/web/conformance.ts — updated resolveHostedHttpConfig and resolveHostedServerConfig helpers to take c: any, updated 3 call sites
• server/routes/web/chat-v2.ts — updated createAuthorizedManager call site
• server/routes/web/evals.ts — updated createAuthorizedManager call site
• server/middleware/request-log-context.ts — fixed TS assertion on spread
• server/routes/web/__tests__/auth-manager.test.ts — updated existing tests to pass mock context as first arg

Out of scope

• No typed event emission (that is PR3)
• No backend changes
• No removal of legacy logger.warn/error callsites

Test plan

Automated

• All 755 existing server tests pass
• auth-manager.test.ts 3 existing tests updated and passing with new signature

Manual

• Call /api/web/servers/validate; confirm no internalLogContext key appears in the JSON response body
• Once the backend authorize endpoint returns internalLogContext, confirm c.var.requestLogContext.workspaceRole is populated after a hosted request

Risk and rollback

• Risk: Breaking change to authorizeServer/authorizeBatch signatures; all call sites updated in this PR
• Rollback: revert the PR; no schema or data migration required

Cross-repo dependencies

• Depends on: Inspector PR1 (logging-inspector-typed-foundation)
• Unblocks: Inspector PR3 (logging-inspector-route-conversions-wave-1)

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[chelojimenez]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[github-actions]

Internal preview

Preview URL: https://mcp-inspector-pr-1931.up.railway.app
Deployed commit: 5e4078d
PR head commit: c67139a
Backend target: staging fallback.
Health: ❌ Convex unreachable — see upsert-preview job logs (staging may need convex deploy)
Access is employee-only in non-production environments.

[Vu-John]

• [logging] inspector: add deprecation guards, LOGGING.md, and ESLint enforcement #1934
• [logging] inspector: convert second-wave hosted routes to typed events #1933
• [logging] inspector: convert chat ingest, tunnels, widgets to typed events #1932
• [logging] inspector: consume backend InternalLogContext in authorize wrapper #1931 👈 (View in Graphite)
• [logging] inspector: add typed event foundation #1930
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[coderabbitai]

Walkthrough

Authorization functions now accept a Hono Context as their first parameter and may receive backend-only internalLogContext in authorize responses. authorizeServer and authorizeBatch map any returned internalLogContext into a request RequestLogContext by calling setRequestLogContext on the request, then strip internalLogContext from client-visible results. Batch authorization selects an attribution source from the first successful entry with internalLogContext and nulls server-specific fields when multiple servers succeed. Signatures for authorizeServer, authorizeBatch, and createAuthorizedManager were updated and call sites were adjusted accordingly.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (4)

mcpjam-inspector/server/routes/web/__tests__/auth-manager.test.ts (1)

24-27: Test coverage gap: the new stripping/merging behavior is never exercised.

mockContext.var.requestLogContext is undefined, so setRequestLogContext short-circuits at its if (!current) return guard, and none of the fixtures include an internalLogContext field. The tests confirm the new signature compiles but say nothing about the actual contract of this PR — that backend internalLogContext is (a) merged into the request log context and (b) absent from the returned result.

Consider seeding requestLogContext and adding a fixture with internalLogContext so a regression to the wrapper would actually fail a test:

🧪 Proposed additional coverage

it("strips internalLogContext from results and merges it into requestLogContext", async () => {
  const setSpy = vi.fn();
  const ctx = {
    var: { requestLogContext: { authType: "signedIn" } },
    set: setSpy,
  } as unknown as Context;

  global.fetch = vi.fn(async () =>
    new Response(
      JSON.stringify({
        results: {
          "server-1": {
            ok: true,
            role: "member",
            accessLevel: "workspace_member",
            permissions: { chatOnly: false },
            serverConfig: {
              transportType: "http",
              url: "https://server-1.example.com/mcp",
              headers: {},
              useOAuth: false,
            },
            internalLogContext: {
              authType: "signedIn",
              workspaceRole: "owner",
              workspaceId: "workspace-1",
            },
          },
        },
      }),
      { status: 200, headers: { "Content-Type": "application/json" } },
    ),
  ) as typeof fetch;

  await createAuthorizedManager(ctx, "bearer-token", "workspace-1", ["server-1"], 10_000);

  expect(setSpy).toHaveBeenCalledWith(
    "requestLogContext",
    expect.objectContaining({ workspaceRole: "owner", workspaceId: "workspace-1" }),
  );
});

Also applies to: 90-144

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mcpjam-inspector/server/routes/web/__tests__/auth-manager.test.ts` around
lines 24 - 27, The tests never exercise the new strip/merge behavior because
mockContext.var.requestLogContext is undefined; seed requestLogContext and add a
fixture containing internalLogContext so the merge and removal are exercised.
Specifically, create a ctx with var.requestLogContext (e.g., { authType:
"signedIn" }) and a set spy, stub global.fetch to return a JSON payload under
results that includes an internalLogContext for a server, call
createAuthorizedManager(...) with that ctx and server id, then assert ctx.set
was called with "requestLogContext" containing the merged fields (workspaceRole,
workspaceId) and that the returned/observed server result no longer contains
internalLogContext. Use the existing mocks (mockContext / setSpy) and the
createAuthorizedManager call to locate where to add this test.

mcpjam-inspector/server/routes/web/auth.ts (2)

304-330: Two small type cleanups while you're here.

• Line 328: Omit<ConvexAuthorizeResponse, "internalLogContext">["serverConfig"] is a no-op — serverConfig is not the omitted key, so this is identical to ConvexAuthorizeResponse["serverConfig"]. The added complexity hints at a guard that isn't actually doing anything.
• Lines 313–330: ConvexBatchAuthorizeSuccess still publicly carries internalLogContext?, even though authorizeBatch is now contractually obliged to strip it before returning. Consumers of ConvexBatchAuthorizeResponse may reasonably (and incorrectly) believe the field can surface on a result. Mirroring the ClientSafeAuthorizeResponse pattern would close the loop.

♻️ Proposed cleanup

-export type ConvexBatchAuthorizeSuccess = {
+type ConvexBatchAuthorizeSuccessWithInternal = {
   ok: true;
   role: "owner" | "admin" | "member";
   accessLevel: "workspace_member" | "shared_chat";
   oauthAccessToken?: string | null;
   permissions: {
     chatOnly: boolean;
   };
-  serverConfig: Omit<ConvexAuthorizeResponse, "internalLogContext">["serverConfig"];
+  serverConfig: ConvexAuthorizeResponse["serverConfig"];
   internalLogContext?: InternalLogContext;
 };
+
+export type ConvexBatchAuthorizeSuccess = Omit<
+  ConvexBatchAuthorizeSuccessWithInternal,
+  "internalLogContext"
+>;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mcpjam-inspector/server/routes/web/auth.ts` around lines 304 - 330, Update
the ConvexBatchAuthorizeSuccess type to mirror the ClientSafeAuthorizeResponse
pattern by removing the optional internalLogContext property (it should not be
exposed to clients) and simplify the serverConfig type: replace
Omit<ConvexAuthorizeResponse, "internalLogContext">["serverConfig"] with
ConvexAuthorizeResponse["serverConfig"] (since omitting internalLogContext does
not affect the serverConfig property); ensure you only modify the
ConvexBatchAuthorizeSuccess declaration and leave AuthorizedServerConfigHolder
and ClientSafeAuthorizeResponse as-is.

---

245-285: Import canonical log-context types to prevent type inconsistencies.

The local InternalLogContext re-declares string-literal unions for authType, workspaceRole, accessLevel, serverTransport, and surface. The canonical definitions in utils/log-events.ts diverge: AuthType includes "system" and "unknown" in addition to "signedIn" and "guest", but the local type omits them. This narrows the accepted values, creating a type safety gap—code assigning "system" to authType would pass local type-checking but fail when the result flows into Partial<RequestLogContext>. Importing and using the canonical types makes this constraint explicit and maintainable.

♻️ Proposed type unification

-import type { RequestLogContext } from "../../utils/log-events.js";
+import type {
+  RequestLogContext,
+  AuthType,
+  WorkspaceRole,
+  AccessLevel,
+  ServerTransport,
+  Surface,
+} from "../../utils/log-events.js";
…
 type InternalLogContext = {
-  authType: "signedIn" | "guest";
+  authType: AuthType;
   userId?: string | null;
   …
-  workspaceRole?: "owner" | "admin" | "member" | "guest" | "editor" | "chat" | null;
-  accessLevel?: "workspace_member" | "shared_chat" | null;
+  workspaceRole?: WorkspaceRole | null;
+  accessLevel?: AccessLevel | null;
   serverId?: string | null;
-  serverTransport?: "stdio" | "http" | null;
+  serverTransport?: ServerTransport | null;
   chatboxId?: string | null;
-  surface?: "preview" | "share_link" | null;
+  surface?: Surface | null;
 };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mcpjam-inspector/server/routes/web/auth.ts` around lines 245 - 285, The
InternalLogContext type redeclares string-literal unions that diverge from the
canonical types in utils/log-events.ts; update mapInternalToRequestContext and
replace the local InternalLogContext declaration by importing and using the
canonical types (e.g., AuthType, WorkspaceRole, AccessLevel, ServerTransport,
Surface, etc.) from utils/log-events.ts so the fields (authType, workspaceRole,
accessLevel, serverTransport, surface) use those imported types and keep
optional/null semantics compatible with Partial<RequestLogContext>.

mcpjam-inspector/server/routes/web/conformance.ts (1)

43-44: Prefer Context from hono over any for the new helper parameters.

authorizeServer is now strictly typed as (c: Context, …), so widening these helpers to c: any discards the type guarantees the rest of auth.ts carefully restored. A one-line import change keeps the surface honest.

♻️ Proposed typing tightening

+import type { Context } from "hono";
 …
 async function resolveHostedHttpConfig(
-  c: any,
+  c: Context,
   bearerToken: string,
   body: Record<string, unknown>,
 ): Promise<{
 …
 async function resolveHostedServerConfig(
-  c: any,
+  c: Context,
   bearerToken: string,
   body: Record<string, unknown>,
 ): Promise<MCPAppsConformanceConfig> {

Also applies to: 127-128

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mcpjam-inspector/server/routes/web/conformance.ts` around lines 43 - 44, The
helper functions (e.g., resolveHostedHttpConfig) currently accept c: any which
discards the stricter auth types; change their parameter types to Context from
hono by adding an import like `import { Context } from "hono"` and update the
function signatures (e.g., async function resolveHostedHttpConfig(c: Context,
...) ) and any other helper(s) in this file that use c: any (including the one
around lines 127-128) so they match authorizeServer's `(c: Context, ...)`
typing.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@mcpjam-inspector/server/routes/web/auth.ts`:
- Around line 505-519: The loop in authorizeBatch merges each result's
InternalLogContext into the request-level log via setRequestLogContext(c,
mapInternalToRequestContext(internalLogContext)), causing per-server fields to
be overwritten when multiple servers are authorized; update the loop that
iterates Object.entries(raw.results) to detect multi-result batches and, when
results.length > 1, strip per-server fields (serverId, serverTransport,
chatboxId) from internalLogContext before calling mapInternalToRequestContext
and setRequestLogContext so only workspace-level context is merged (leave
strippedResults population and client-safe result extraction unchanged).

---

Nitpick comments:
In `@mcpjam-inspector/server/routes/web/__tests__/auth-manager.test.ts`:
- Around line 24-27: The tests never exercise the new strip/merge behavior
because mockContext.var.requestLogContext is undefined; seed requestLogContext
and add a fixture containing internalLogContext so the merge and removal are
exercised. Specifically, create a ctx with var.requestLogContext (e.g., {
authType: "signedIn" }) and a set spy, stub global.fetch to return a JSON
payload under results that includes an internalLogContext for a server, call
createAuthorizedManager(...) with that ctx and server id, then assert ctx.set
was called with "requestLogContext" containing the merged fields (workspaceRole,
workspaceId) and that the returned/observed server result no longer contains
internalLogContext. Use the existing mocks (mockContext / setSpy) and the
createAuthorizedManager call to locate where to add this test.

In `@mcpjam-inspector/server/routes/web/auth.ts`:
- Around line 304-330: Update the ConvexBatchAuthorizeSuccess type to mirror the
ClientSafeAuthorizeResponse pattern by removing the optional internalLogContext
property (it should not be exposed to clients) and simplify the serverConfig
type: replace Omit<ConvexAuthorizeResponse,
"internalLogContext">["serverConfig"] with
ConvexAuthorizeResponse["serverConfig"] (since omitting internalLogContext does
not affect the serverConfig property); ensure you only modify the
ConvexBatchAuthorizeSuccess declaration and leave AuthorizedServerConfigHolder
and ClientSafeAuthorizeResponse as-is.
- Around line 245-285: The InternalLogContext type redeclares string-literal
unions that diverge from the canonical types in utils/log-events.ts; update
mapInternalToRequestContext and replace the local InternalLogContext declaration
by importing and using the canonical types (e.g., AuthType, WorkspaceRole,
AccessLevel, ServerTransport, Surface, etc.) from utils/log-events.ts so the
fields (authType, workspaceRole, accessLevel, serverTransport, surface) use
those imported types and keep optional/null semantics compatible with
Partial<RequestLogContext>.

In `@mcpjam-inspector/server/routes/web/conformance.ts`:
- Around line 43-44: The helper functions (e.g., resolveHostedHttpConfig)
currently accept c: any which discards the stricter auth types; change their
parameter types to Context from hono by adding an import like `import { Context
} from "hono"` and update the function signatures (e.g., async function
resolveHostedHttpConfig(c: Context, ...) ) and any other helper(s) in this file
that use c: any (including the one around lines 127-128) so they match
authorizeServer's `(c: Context, ...)` typing.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e368c1d4-0612-4dba-b912-479aa0d38e20

📥 Commits

Reviewing files that changed from the base of the PR and between 69c62ef and a324b93.

📒 Files selected for processing (6)

• mcpjam-inspector/server/routes/web/__tests__/auth-manager.test.ts
• mcpjam-inspector/server/routes/web/auth.ts
• mcpjam-inspector/server/routes/web/chat-v2.ts
• mcpjam-inspector/server/routes/web/conformance.ts
• mcpjam-inspector/server/routes/web/evals.ts
• mcpjam-inspector/server/routes/web/servers.ts

[coderabbitai]

🧹 Nitpick comments (4)

mcpjam-inspector/server/routes/web/__tests__/authorize-batch.test.ts (2)

186-222: Security invariant test — exactly the right guard.

Asserting internalLogContext === undefined on every returned successful entry is the defining check for this PR's "must never reach the browser" promise. Consider an additional assertion that Object.values(result.results).length > 0 purely to defend against a fetch-mock regression silently producing an empty results object (a passing test for the wrong reason).

♻️ Optional belt-and-braces

+    expect(Object.values(result.results).length).toBeGreaterThan(0);
     for (const entry of Object.values(result.results)) {
       expect((entry as Record<string, unknown>).internalLogContext).toBeUndefined();
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mcpjam-inspector/server/routes/web/__tests__/authorize-batch.test.ts` around
lines 186 - 222, Add a guard assertion to the test that the batch actually
contains results before checking for stripped fields: after calling
authorizeBatch(c, "bearer", "ws-1", [...]) assert that
Object.values(result.results).length > 0 (or
expect(Object.keys(result.results).length).toBeGreaterThan(0)) before the loop
that verifies internalLogContext is undefined, referencing the existing result
variable and result.results to prevent a false positive if fetch was mocked to
return an empty results object.

---

31-40: Mock context plays its part — though a brittle understudy.

The Proxy get trap blindly indexes t[p as string], which fine for the string keys this file uses but will misbehave if any code path inadvertently touches a Symbol (e.g., Symbol.toPrimitive). Since authorizeBatch only reads c.var.requestLogContext via setRequestLogContext, the current shape is sufficient — just worth keeping in mind if future tests expand the surface area exercised through c.

♻️ Optional hardening

-  const c = {
-    var: new Proxy(vars, { get: (t, p) => t[p as string] }),
-    set: (key: string, value: unknown) => {
-      vars[key] = value;
-    },
-  } as unknown as Context;
+  const c = {
+    var: new Proxy(vars, {
+      get: (t, p) => (typeof p === "string" ? t[p] : undefined),
+    }),
+    get: (key: string) => vars[key],
+    set: (key: string, value: unknown) => {
+      vars[key] = value;
+    },
+  } as unknown as Context;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mcpjam-inspector/server/routes/web/__tests__/authorize-batch.test.ts` around
lines 31 - 40, The Proxy get trap in makeContext naively does t[p as string],
which will break if a Symbol property (e.g., Symbol.toPrimitive) is accessed;
update the get trap implementation in makeContext to only treat string/property
keys by checking typeof p === 'string' (or using String(p) safely) and return
undefined (or Reflect.get) for Symbol keys so c.var and setRequestLogContext
continue to work and future symbol accesses don't crash the test.

mcpjam-inspector/server/routes/web/auth.ts (2)

328-328: The Omit here is a no-op disguised as intent.

Omit<ConvexAuthorizeResponse, "internalLogContext">["serverConfig"] is structurally identical to ConvexAuthorizeResponse["serverConfig"] — the Omit strips a sibling key from the outer object, but you immediately index into serverConfig, which never contained internalLogContext to begin with. Simplify for clarity.

♻️ Suggested simplification

-  serverConfig: Omit<ConvexAuthorizeResponse, "internalLogContext">["serverConfig"];
+  serverConfig: ConvexAuthorizeResponse["serverConfig"];

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mcpjam-inspector/server/routes/web/auth.ts` at line 328, The type expression
using Omit is misleading and a no-op: replace the type
"Omit<ConvexAuthorizeResponse, \"internalLogContext\">[\"serverConfig\"]" with
the simpler "ConvexAuthorizeResponse[\"serverConfig\"]" wherever the
serverConfig type is declared (the symbol named serverConfig in this file) to
improve clarity and remove the redundant Omit usage.

---

788-789: Mixed c: any and c: Context — let's not leave the type coverage half-built.

This PR rightly tightens authorizeServer, authorizeBatch, and createAuthorizedManager to take c: Context, but withEphemeralConnection (line 789), handleRoute (line 708), requireGuestId (line 167), and createGuestEphemeralManager (line 180) still accept c: any. They're now passing c directly into strongly-typed callees, so swapping these signatures to Context (or Context<{ Variables: { requestLogContext?: RequestLogContext; guestId?: string } }>) would catch shape drift earlier without behavioral risk. Optional, but feels like the right moment to harmonize.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mcpjam-inspector/server/routes/web/auth.ts` around lines 788 - 789, The
functions withEphemeralConnection, handleRoute, requireGuestId, and
createGuestEphemeralManager currently accept c: any; update their signatures to
use the proper Context type to match the other authorized helpers — e.g., change
c: any to Context or the more specific Context<{ Variables: {
requestLogContext?: RequestLogContext; guestId?: string } }> so callers pass a
consistently-typed context into
authorizeServer/authorizeBatch/createAuthorizedManager; also add any needed
imports for Context and RequestLogContext and adjust generics on
withEphemeralConnection<S, T> if required to preserve existing behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@mcpjam-inspector/server/routes/web/__tests__/authorize-batch.test.ts`:
- Around line 186-222: Add a guard assertion to the test that the batch actually
contains results before checking for stripped fields: after calling
authorizeBatch(c, "bearer", "ws-1", [...]) assert that
Object.values(result.results).length > 0 (or
expect(Object.keys(result.results).length).toBeGreaterThan(0)) before the loop
that verifies internalLogContext is undefined, referencing the existing result
variable and result.results to prevent a false positive if fetch was mocked to
return an empty results object.
- Around line 31-40: The Proxy get trap in makeContext naively does t[p as
string], which will break if a Symbol property (e.g., Symbol.toPrimitive) is
accessed; update the get trap implementation in makeContext to only treat
string/property keys by checking typeof p === 'string' (or using String(p)
safely) and return undefined (or Reflect.get) for Symbol keys so c.var and
setRequestLogContext continue to work and future symbol accesses don't crash the
test.

In `@mcpjam-inspector/server/routes/web/auth.ts`:
- Line 328: The type expression using Omit is misleading and a no-op: replace
the type "Omit<ConvexAuthorizeResponse,
\"internalLogContext\">[\"serverConfig\"]" with the simpler
"ConvexAuthorizeResponse[\"serverConfig\"]" wherever the serverConfig type is
declared (the symbol named serverConfig in this file) to improve clarity and
remove the redundant Omit usage.
- Around line 788-789: The functions withEphemeralConnection, handleRoute,
requireGuestId, and createGuestEphemeralManager currently accept c: any; update
their signatures to use the proper Context type to match the other authorized
helpers — e.g., change c: any to Context or the more specific Context<{
Variables: { requestLogContext?: RequestLogContext; guestId?: string } }> so
callers pass a consistently-typed context into
authorizeServer/authorizeBatch/createAuthorizedManager; also add any needed
imports for Context and RequestLogContext and adjust generics on
withEphemeralConnection<S, T> if required to preserve existing behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3fb42c75-f694-4872-a715-dd983b1a8794

📥 Commits

Reviewing files that changed from the base of the PR and between a324b93 and c67139a.

📒 Files selected for processing (2)

• mcpjam-inspector/server/routes/web/__tests__/authorize-batch.test.ts
• mcpjam-inspector/server/routes/web/auth.ts