Skip to content

Add hosted Sysdig Secure MCP - #101

Merged
jamesgagan merged 9 commits into
bcgov:mainfrom
SungYu444:feature/sysdig-mcp
Nov 3, 2026
Merged

jamesgagan merged 9 commits into
bcgov:mainfrom
SungYu444:feature/sysdig-mcp

Conversation

@SungYu444

@SungYu444 SungYu444 commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds Sysdig Secure to RAVEN's MCP registry as a hosted remote MCP server. The MCP client (VS Code / Copilot) connects directly to Sysdig's /mcp/secure endpoint and handles the OAuth sign-in itself. RAVEN runs no Sysdig code and stores no Sysdig credentials.

"sysdig-secure-mcp": {
  "type": "http",
  "url": "https://secure.sysdig.com/mcp/secure"
}

This PR previously contained a local read-only stdio proxy (packages/sysdig-mcp). That proxy has been removed; see "Why not a local proxy" below.

Changes

File Change
.mcp.json Adds the remote sysdig-secure-mcp entry (type: "http").
scripts/release/release-lib.mjs validateCatalog accepts a .mcp.json entry without command only if it is on a reviewed allowlist (APPROVED_REMOTE_MCP_SERVERS), contains only type: "http" and url, and the URL is HTTPS on an approved Sysdig host and path with no credentials, port, query or fragment. Anything else fails the release. Remote entries are excluded from the bundled release catalog.
scripts/release/release.test.mjs Tests for the Sysdig entry plus negative tests: unapproved ID, __proto__, headers, non-http type, invalid URL, http:, foreign host, look-alike host (sysdig.com.attacker.example), embedded credentials, query string, wrong path.
scripts/gen-inventory.lib.mjs, docs/TOOL_INVENTORY.md Documents Sysdig as a hosted OAuth MCP with a dynamic tool surface (excluded from fixed totals). Regenerated with npm run gen-inventory.
README.md, docs/SYSTEM_DESIGN_AND_ARCHITECTURE.md, .github/copilot-instructions.md Sysdig usage, data-egress and data-residency statements: US-East-hosted third-party service; STRA completed and data sovereignty acknowledged; responses do not pass through PiScrubber; tool permissions governed by the user's Sysdig role.

Compared with main, this PR touches only the 8 Sysdig-related files above. No other MCP server, package, lockfile entry or setup script is changed.

Why not a local proxy

The proxy version was tested and did not work in practice:

  • The first tool call blocked on browser OAuth for up to 5 minutes, but MCP clients time out requests after 60 s (MCP request timed out after 60000 ms).
  • The proxy-driven sign-in page rendered blank after Sysdig redirected to its SPA login.
  • It added an always-on loopback callback listener, in-memory-only tokens (re-sign-in on every restart) and a new OAuth client registration on every start.

Client-managed OAuth worked reliably, keeps tokens in the client's secure storage, and needs no RAVEN code.

Security and compliance

  • Data residency: Sysdig Secure MCP is a US-East-hosted third-party service (Sysdig does not publish a Canadian region). Its STRA has been completed and data sovereignty acknowledged.
  • Credentials: none in RAVEN. OAuth is performed and stored by the MCP client.
  • Access control: governed by the signed-in user's Sysdig role. The tool surface is Sysdig's and may include write-capable tools; this is documented.
  • PI scrubbing: Sysdig responses do not pass through RAVEN; this is documented.
  • Release guard: only reviewed, credential-free HTTPS Sysdig endpoints are accepted (see tests).
  • Copilot cloud agent and code review do not support remote OAuth MCP servers.

Testing

  • npm run build passes.
  • npm run gen-inventory:check: TOOL_INVENTORY.md is up to date.
  • npx vitest run --root . scripts/release: 10 passed.
  • Manual: sysdig-secure-mcp connects in VS Code, signs in through the MCP servers panel, and lists and calls Sysdig tools.

Acceptance checklist

  • sysdig-secure-mcp appears in the MCP client and signs in via browser OAuth.
  • Sysdig tools can be listed and called after sign-in.
  • npm run build, npm run gen-inventory:check and the release tests pass.
  • Release validation rejects unapproved or credential-bearing remote entries.
  • No changes to other MCP servers.

Copilot AI and others added 3 commits October 22, 2026 15:12
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

@jamesgagan jamesgagan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Sysdig Secure MCP proxy

Thanks for this. The proxy's overall design is good:

  • It exposes tools only.
  • It re-checks the policy on every call.
  • Results go through PiScrubber.
  • The OAuth callback binds to loopback and checks a 256-bit state before reading anything else.

That makes it stricter than jarvis-mcp. Some things need fixing before merge, mainly the OAuth lifecycle, the read-only guarantee, and the compliance docs.

Summary: 9 required changes, 5 minor items, 2 pre-existing notes. Reviewed at 8609bb3e0c3e79c8f59794714fbd9aad2724858f, checked against the MCP SDK 1.32.0 source this PR locks.

Required changes

Bugs

  • 1. Recover from token expiry (oauth.ts, index.ts)
    • Implement invalidateCredentials.
    • Add a .catch to the waitForCode promise stored in redirectToAuthorization.
    • On UnauthorizedError, drop the cached connection and reconnect. If that is out of scope, fail fast with "restart to sign in" and don't open a tab.
  • 2. Don't block tools/list on browser sign-in (server.ts:75-81)
    • Return the static allowlist until connected.
    • Declare tools: { listChanged: true } and send notifications/tools/list_changed once connected.
    • Start OAuth only on the first tools/call.
  • 3. Harden the callback listener (oauth.ts:78-84)
    • Wrap new URL(request.url…) in try/catch and return 400 on failure.
    • Close the callback server once a code arrives.
    • Close it in shutdown() even when connection is still undefined.

Security

  • 4. Make the allowlist authoritative (read-only.ts:20-24): admit a tool only if it is on DOCUMENTED_READ_ONLY_TOOLS and not annotated readOnlyHint: false. Update the tests and the README wording to match.
  • 5. Validate the authorization URL before opening it (oauth.ts:187-216)
    • Require authorizationUrl.protocol === "https:" and a host under *.sysdig.com.
    • Call rundll32.exe by its absolute %SystemRoot%\System32 path.
  • 6. Scrub errors and structuredContent (server.ts:83-94)
    • Catch upstream callTool errors and rethrow them with scrubText(message).
    • Scrub structuredContent as one serialized string, not leaf by leaf.

Docs, compliance and release

  • 7. Correct the docs this PR makes false
    • TOOL_INVENTORY.md:275 still describes the earlier remote-URL design.
    • SYSTEM_DESIGN_AND_ARCHITECTURE.md:85, :167 and :783 need to mention the loopback OAuth listener.
    • SYSTEM_DESIGN_AND_ARCHITECTURE.md:681 and .github/copilot-instructions.md:142 must say Sysdig is a US-hosted third-party service.
  • 8. Data residency
    • Remove the built-in US East default so SYSDIG_SECURE_MCP_URL is required.
    • Keep the server opt-in (out of the default .mcp.json) until a STRA/PIA covers it.
    • Add a STRA/PIA note to the docs and to this PR's description.
  • 9. Revert the validateCatalog change (scripts/release/release-lib.mjs:199-221) to const configuredIds = Object.keys(mcp.mcpServers ?? {});.
1. Token expiry is never recovered, and one path crashes the process

}
async redirectToAuthorization(authorizationUrl: URL): Promise<void> {
if (!this.expectedState) throw new Error("Sysdig OAuth state was not initialized.");
this.authorizationCode = this.callback.waitForCode(this.expectedState);
process.stderr.write("[sysdig-mcp] Opening browser for Sysdig OAuth sign-in.\n");
try {
await openBrowser(authorizationUrl.toString());
} catch {
process.stderr.write(
`[sysdig-mcp] Could not launch a browser automatically. Open this URL to authorize:\n${authorizationUrl.toString()}\n`,
);
}
}
waitForAuthorizationCode(): Promise<string> {
if (!this.authorizationCode) throw new Error("Sysdig OAuth authorization did not start.");
return this.authorizationCode;
}
}

const getClient = async (): Promise<SysdigUpstreamClient> => {
connectionPromise ??= connectSysdigClient(sysdigUrl)
.then((connected) => {
connection = connected;
return connected;
})
.catch((error: unknown) => {
connectionPromise = undefined;
throw error;
});
return (await connectionPromise).client;
};

this.authorizationCode = this.callback.waitForCode(this.expectedState); // nothing awaits this after the first connect

When the access token expires and there is no usable refresh token:

  • The SDK's 401 handler (client/streamableHttp.js → client/auth.js authInternal) calls redirectToAuthorization again and throws UnauthorizedError.
  • Only the first-connect catch (oauth.ts:236) ever calls finishAuth. If the user signs in now, they see "sign-in complete", but no token is stored. The next call opens another tab.
  • If the user ignores the tab, the 5-minute timer rejects a promise that has no handler. On Node 24 that terminates the process.
  • Without invalidateCredentials, an invalid_grant error is retried with the same dead refresh token on every call.

I understand tokens are deliberately in memory and a restart re-authenticates. The problem is that the user is never told to restart.

2. tools/list blocks on browser sign-in at every host start

server.setRequestHandler(ListToolsRequestSchema, async (request) => {
const result = await (await getClient()).listTools(request.params);
return {
...result,
tools: filterReadOnlyTools(result.tools),
};
});

  • The tools/list handler awaits getClient(), which opens the browser and waits up to 5 minutes (oauth.ts:16).
  • Hosts list tools right after initialize.
  • The server is in .mcp.json and the release catalog, so every RAVEN user gets a sign-in tab whenever their IDE starts, including users who never use Sysdig.
  • The server declares no listChanged. A host whose first tools/list timed out (the SDK client default is 60 s) has no signal to list again.
  • Client registration is in memory only, so every start also registers a new OAuth client with Sysdig.
  • The README says sign-in happens "on first use", but it happens at host startup.
4. The read-only filter trusts Sysdig's own annotation

export function isReadOnlyTool(tool: Pick<Tool, "name" | "annotations">): boolean {
if (tool.annotations?.readOnlyHint === false) return false;
if (tool.annotations?.readOnlyHint === true) return true;
return DOCUMENTED_READ_ONLY_TOOLS.has(tool.name);
}

if (tool.annotations?.readOnlyHint === true) return true;   // any tool name passes
return DOCUMENTED_READ_ONLY_TOOLS.has(tool.name);           // only consulted when unannotated
  • The MCP spec says clients should not make tool-use decisions based on annotations from servers they don't control.
  • With the current logic, any new upstream tool that labels itself read-only reaches the LLM without Raven review.
  • The proxy relays readOnlyHint: true to clients, which may auto-approve those calls.
  • The README and TOOL_INVENTORY.md say "only read-only tools are exposed". The code only guarantees "tools Sysdig labels read-only".
5. The authorization URL is opened with no scheme or host check

async redirectToAuthorization(authorizationUrl: URL): Promise<void> {
if (!this.expectedState) throw new Error("Sysdig OAuth state was not initialized.");
this.authorizationCode = this.callback.waitForCode(this.expectedState);
process.stderr.write("[sysdig-mcp] Opening browser for Sysdig OAuth sign-in.\n");
try {
await openBrowser(authorizationUrl.toString());
} catch {
process.stderr.write(
`[sysdig-mcp] Could not launch a browser automatically. Open this URL to authorize:\n${authorizationUrl.toString()}\n`,
);
}
}
waitForAuthorizationCode(): Promise<string> {
if (!this.authorizationCode) throw new Error("Sysdig OAuth authorization did not start.");
return this.authorizationCode;
}
}
async function openBrowser(url: string): Promise<void> {
const wsl = Boolean(process.env["WSL_DISTRO_NAME"]);
const command =
process.platform === "win32" || wsl
? "rundll32.exe"
: process.platform === "darwin"
? "open"
: "xdg-open";
const args = command === "rundll32.exe" ? ["url.dll,FileProtocolHandler", url] : [url];
await execFileAsync(command, args, { windowsHide: true });
}

  • authorization_endpoint comes from discovered OAuth metadata, and the authorization server's host is not pinned. config.ts constrains only the MCP URL.
  • The SDK's SafeUrlSchema rejects only javascript:, data: and vbscript:, so a file:, smb: or custom-protocol URL is passed to rundll32 url.dll,FileProtocolHandler, open or xdg-open.
  • This is the same class as CVE-2025-6514 in mcp-remote.
  • It needs hostile or compromised metadata to exploit. The fix is a one-line protocol and host check.
  • execFile without a shell is correct and rules out argument injection.
6. PI scrubbing misses errors and structuredContent

server.setRequestHandler(CallToolRequestSchema, async (request) => {
const upstream = await getClient();
const tool = await findReadOnlyTool(upstream, request.params.name);
if (!tool) {
throw new McpError(
ErrorCode.InvalidParams,
`Sysdig tool '${request.params.name}' is unavailable or is not approved as read-only.`,
);
}
const result = normalizeToolResult(await upstream.callTool(request.params));
return scrubStringValues(result, scrubber) as CallToolResult;
});

  • Errors: an error thrown by upstream.callTool reaches the client unscrubbed. The SDK sends error.message and error.data as-is, and StreamableHTTPError includes the upstream response body. #93 fixed the same error-body gap (194f8eb).
  • structuredContent: scrubStringValues scrubs each leaf string on its own, so {"password":"hunter2"} calls scrubText("hunter2"). The key-based credential rule in pi-scrubber.ts never fires.
3. A malformed request crashes the proxy, and the listener stays open

private handleRequest(request: IncomingMessage, response: import("node:http").ServerResponse): void {
const requestUrl = new URL(request.url ?? "/", "http://127.0.0.1");
if (request.method !== "GET" || requestUrl.pathname !== "/callback" || !this.pending) {
response.writeHead(404);
response.end();
return;
}

  • A request target such as // makes new URL() throw inside the request listener. The exception is uncaught, so the process exits.
  • The listener stays bound until shutdown, so any local process can trigger this at any time, and so can a browser sending requests to the loopback port.
  • Separately, a SIGTERM during a pending sign-in leaves the listener open, because connection is still undefined.
7. Docs made false by this PR
  • docs/TOOL_INVENTORY.md:275 says Sysdig is "configured as a remote MCP server directly in .mcp.json". It is a local stdio command entry. https://github.com/bcgov/raven/blob/8609bb3e0c3e79c8f59794714fbd9aad2724858f/docs/TOOL_INVENTORY.md#L274-L276
  • SYSTEM_DESIGN_AND_ARCHITECTURE.md:85 (rewritten in this PR) says "no network listener". :167 says "no exposed ports" and :783 says "No network ports exposed". The OAuth callback listens on 127.0.0.1 for the whole process lifetime.
    4. **Raven-owned servers use local stdio.** These MCP servers run as child processes with no network listener. The Sysdig wrapper is a Raven-owned local stdio MCP server that connects upstream to Sysdig's hosted Streamable HTTP MCP using its own interactive OAuth session.
  • SYSTEM_DESIGN_AND_ARCHITECTURE.md:681 says "All enterprise data stays on BC Gov infrastructure". This row is used in security walkthroughs.
    | **FOIPPA s.30 (Protection of PI)** | PI scrubber anonymizes personal information before it reaches the AI. `RAVEN_SCRUB_PI=true` controls this globally. |
    | **FOIPPA s.30.1 (Storage in Canada)** | All enterprise data stays on BC Gov infrastructure. MCP servers run locally. GitHub Copilot Business does not retain data. |
    | **BC Gov ISP (Information Security Policy)** | Credentials stored with restricted permissions (`chmod 600`). Authentication via SiteMinder SSO or Basic Auth. TLS for all network communication. |
  • .github/copilot-instructions.md:142 says the servers "add no third-party data sinks".
    Everything above is for repo navigation and PR review. **The sections below are for Copilot Chat in IDEs when this repo's MCP servers are connected** (clients pick these up via `.mcp.json`). RAVEN's MCP servers connect only to your configured BC Gov upstreams (Jira, Confluence, Bitbucket, app servers, and — when enabled — ADO, Sonar, Artifactory, RFC Buddy, and the `jarvis-mcp` remote proxy); they add no third-party data sinks beyond those. Artifactory direct downloads may additionally read from explicitly allowlisted HTTPS storage hosts without sending Artifactory credentials. See [`../docs/TOOL_INVENTORY.md`](../docs/TOOL_INVENTORY.md).

The same kind of compliance statement was raised in the #96 review.

8. US East default with no residency or STRA/PIA note

const DEFAULT_SYSDIG_MCP_URL = "https://secure.sysdig.com/mcp/secure";
export function resolveSysdigMcpUrl(
env: Record<string, string | undefined> = process.env,
): URL {
const raw = env["SYSDIG_SECURE_MCP_URL"] ?? DEFAULT_SYSDIG_MCP_URL;
let url: URL;

  • The built-in default is secure.sysdig.com, which is Sysdig US East.
  • The only residency text in the PR is "review … for FOIPPA and data-residency requirements".
  • BC Gov standards require Canadian regions only, and a STRA/PIA for any product handling personal data.
  • #90 removed Jarvis's built-in default endpoint so the destination is always explicit.

Can you confirm whether our Sysdig tenant has a Canadian region? I couldn't find one in Sysdig's region list.

9. validateCatalog dead branch weakens the release guard

}
const configuredServers = Object.entries(mcp.mcpServers ?? {});
for (const [id, configured] of configuredServers) {
if (configured.command !== undefined) continue;
let url;
try {
url = new URL(configured.url);
} catch {
throw new Error(`Remote MCP entry '${id}' must have a valid HTTPS URL.`);
}
if (
configured.type !== "http" ||
url.protocol !== "https:" ||
url.username ||
url.password
) {
throw new Error(`Remote MCP entry '${id}' must use OAuth-capable HTTPS Streamable HTTP.`);
}
}
const configuredIds = configuredServers
.filter(([, configured]) => configured.command !== undefined)
.map(([id]) => id);
const serverIds = new Set(catalog.servers.map((server) => server.id));
if (
configuredIds.some((id) => !serverIds.has(id)) ||
serverIds.size !== configuredIds.length
) {
throw new Error(
"Release catalog and .mcp.json contain different server IDs.",
);
}
return catalog;

  • Any .mcp.json entry without a command now skips the catalog-parity check. Any HTTPS host passes, and headers are not inspected.
  • The branch was added for the earlier remote-URL design (first commit) and is unreachable with the final .mcp.json. No test covers it.
  • Reverting to the original one-liner restores the guard.

Minor (non-blocking)

  • Lockfile. package-lock.json drops 14 unrelated libc fields. This is an npm 10 rewrite artifact. Start from main's lockfile and re-add only the sysdig-mcp hunks.
  • Docs and tests.
    • Add JSDoc to the exported functions and classes.
    • Add a SYSDIG_SECURE_MCP_URL row to the env-var table in TOOL_INVENTORY.md.
    • Add tests for oauth.ts (state mismatch, a single pending callback, timeout).
  • OAuth client metadata.
    • clientMetadata sets no scope, so every advertised scope is requested while the consent screen says "Read-only".
    • Client registration is in memory only, so each start creates a new registration.
  • PR hygiene.
    • Use the title format TICKET-### - Description.
    • Add a description with an acceptance checklist.
    • Drop the stray blank-line change in scripts/setup-credentials.ps1.
  • Package count. SYSTEM_DESIGN_AND_ARCHITECTURE.md:178 says "20 packages"; there are 22.

Pre-existing (not blocking, for awareness)

  • Prompt injection, as a class. Sysdig event text (process command lines, image names) can be attacker-influenced. It now reaches the same LLM session as the write-capable Jira, Confluence, Bitbucket and GitHub servers. Server logs already do this today; this PR adds another feed.
  • jarvis-mcp has the same gaps as items 4 and 6.
    • It forwards results and errors with no PI scrubbing (packages/jarvis-mcp/src/index.ts:86-93).
    • Its catalog entry says "access": "read-only", but it forwards a write tool (release/server-catalog.json:98-104).
  • setup-credentials.ps1 -Verify (:43-48) prints the first and last two characters of each secret, and the asterisk count reveals its length.

@SungYu444 SungYu444 reopened this Oct 27, 2026
@SungYu444 SungYu444 changed the title Feature/sysdig mcp ABC-123 - Add hosted Sysdig Secure MCP Oct 29, 2026
@SungYu444

Copy link
Copy Markdown
Contributor Author

@jamesgagan thanks for the thorough review. I've changed direction: the local sysdig-mcp proxy is removed, and Sysdig is now a hosted remote MCP entry (type: "http") whose OAuth is handled by the MCP client.

Why: in testing, the proxy didn't work reliably. The first tool call blocked on browser OAuth (up to 5 min) while the client timed out at 60 s, and the proxy-driven sign-in page rendered blank. Client-managed OAuth works, and it removes the listener, token-handling and registration issues you raised.

Below is each item and its status (latest: d4215c9).

Required changes

# Item Status Notes
1 Recover from token expiry ✅ Resolved by removal RAVEN no longer runs OAuth. Token refresh, expiry and re-sign-in are handled by the MCP client.
2 Don't block tools/list on sign-in ✅ Resolved by removal No proxy. The client shows a Sign in action instead of opening a tab at IDE startup, and users who don't use Sysdig aren't prompted.
3 Harden the callback listener ✅ Resolved by removal There is no loopback callback listener in RAVEN any more.
4 Authoritative read-only allowlist ⚠️ Not adopted There is no RAVEN process in the path to enforce an allowlist. Access is governed by the signed-in user's Sysdig role, which is the authoritative permission model for Sysdig data. The docs no longer claim "only read-only tools are exposed": README, TOOL_INVENTORY.md and the architecture doc now say the tool surface, including any write-capable tools, is defined by Sysdig and limited by the user's role. Clients still prompt for tool approval.
5 Validate the authorization URL before opening it ✅ Resolved by removal RAVEN no longer opens browser URLs. The client performs OAuth discovery and authorization.
6 Scrub errors and structuredContent ⚠️ Not adopted Without a proxy, responses go straight from Sysdig to the client and cannot pass through PiScrubber. Sysdig returns security findings (vulnerabilities, posture, runtime events) rather than RAVEN-sourced personal records. The service is covered by a completed STRA, with data sovereignty acknowledged. This is now stated explicitly in the README, TOOL_INVENTORY.md and the architecture doc.
7 Correct the docs ✅ Fixed TOOL_INVENTORY.md's "remote MCP server directly in .mcp.json" statement is now accurate. The "no listener / no exposed ports" statements are true again (no listener), and the local-only row now separates Raven-owned servers from hosted Sysdig. The data-residency row and .github/copilot-instructions.md:142 now say Sysdig is a US-East-hosted third-party service with a completed STRA.
8 Data residency ✅ Partly fixed; default kept, with reasons Sysdig does not publish a Canadian region (to your question: our tenant is US East). The STRA is completed and data sovereignty acknowledged; this is now in the docs and the PR description. Because the review is done, I've kept the entry in the default .mcp.json. It is still opt-in in practice: nothing connects until a user explicitly signs in. The URL is a single explicit, documented line that can be pointed at another region's /mcp/secure.
9 Revert the validateCatalog change ⚠️ Kept, hardened instead A remote entry needs some handling, or catalog parity fails. Instead of the old permissive branch, a remote entry is now accepted only if its ID is in a reviewed allowlist (APPROVED_REMOTE_MCP_SERVERS), it contains only type and url (no headers), and the URL is HTTPS on an approved Sysdig host and path with no credentials, port, query or fragment. The branch is now reachable and covered by negative tests (unapproved ID, __proto__, headers, http:, foreign host, look-alike host, embedded credentials, query, wrong path).

Minor

Item Status
Lockfile libc drops ✅ Fixed. package-lock.json is identical to main.
JSDoc, env-var row, oauth.ts tests ✅ N/A. The package and SYSDIG_SECURE_MCP_URL are gone. The new exported release helpers have JSDoc and tests.
OAuth scope and per-start registration ✅ N/A. The client owns registration and scopes.
PR hygiene ✅ Description and acceptance checklist added. Stray setup-credentials.ps1 blank-line change reverted. Title updated.
Package count ✅ N/A. No package is added, so main's count stands.

Pre-existing

  • Prompt injection: acknowledged. Sysdig event text is another untrusted feed in the same session as write-capable servers, like server logs today. Clients' per-tool approval remains the control.
  • jarvis-mcp gaps and setup-credentials.ps1 -Verify: agreed they're worth fixing, but out of scope for this Sysdig-only PR. Happy to open follow-up tickets.

Validation: npm run build, npm run gen-inventory:check and the release tests (10) pass. Compared with main, this PR touches only the 8 Sysdig-related files.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The release gate accepts array-valued URLs because new URL() coerces its input, allowing malformed remote configuration that MCP clients cannot use.

1 open issue
What changed in this PR

Adds Sysdig Secure to RAVEN’s MCP registry as a hosted service with client-managed OAuth, without a local proxy or RAVEN-managed credentials.

Changes:

  • Registers the remote Sysdig endpoint and excludes it from bundled releases.
  • Adds remote-entry validation and tests for approved, credential-free HTTPS endpoints.
  • Updates documentation and generated inventory for Sysdig’s dynamic tools, permissions, and data handling.
File Description
scripts/​release/​release.test.mjs Tests Sysdig configuration, bundle exclusion, and rejected remote entries.
scripts/​release/​release-lib.mjs Validates approved remote entries separately from bundled servers.
scripts/​gen-inventory.lib.mjs Adds hosted Sysdig documentation and excludes dynamic tools from totals.
README.md Explains OAuth setup, permissions, and data-residency considerations.
docs/​TOOL_INVENTORY.md Regenerates inventory with the hosted Sysdig integration.
docs/​SYSTEM_DESIGN_AND_ARCHITECTURE.md Documents Sysdig’s remote architecture and security boundaries.
.mcp.json Registers the hosted Sysdig Secure HTTP endpoint.
.github/​copilot-instructions.md Adds Sysdig hosting and assessment context.

Review effort: Balanced

Comment thread scripts/release/release-lib.mjs Resolved
@SungYu444 SungYu444 changed the title ABC-123 - Add hosted Sysdig Secure MCP Add hosted Sysdig Secure MCP Nov 2, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

A critical validation bypass lets mixed-transport catalog entries retain a command while adding a foreign URL and credential-bearing headers, evading remote allowlist and credential checks.

1 open issue
1 resolved since last review

Review effort: Balanced

Comment thread scripts/release/release-lib.mjs Outdated
const configuredIds = Object.keys(mcp.mcpServers ?? {});
const configuredIds = Object.entries(mcp.mcpServers ?? {})
.filter(([id, configured]) => {
if (configured?.command !== undefined) return true;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6e35b38: validateCatalog now treats an entry as local only when it has exactly command and args; anything else, including entries that keep a command but add type, url or headers, goes through validateRemoteMcpEntry and fails unless it is the approved Sysdig endpoint. validateCatalog also accepts an optional .mcp.json object, so the new regression test runs tampered jira entries (command + http URL + headers, command + headers, and type: "stdio") through it, and all are rejected.

@jamesgagan
jamesgagan self-requested a review November 3, 2026 14:29

@jamesgagan jamesgagan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Hosted Sysdig Secure MCP (round 2)

Thanks for the rework. Moving OAuth into the MCP client removes every round-1 listener, token and browser-launch issue, and RAVEN no longer holds any Sysdig code or credentials. The release gate also holds up well. It fails closed on every bypass I tried:

  • mixed-transport entries
  • null, array and string entries
  • __proto__ and constructor IDs
  • look-alike hosts
  • userinfo, query, fragment and port

It also runs on every PR through npm test, not only at release.

Summary: 2 required doc changes, 5 minor items and 2 pre-existing notes. Round-1 item 8 is accepted as is. Reviewed at 6e35b386c2c104c535b1ffba6b9b57e3dfc682c6.

Required changes

Docs, compliance and release

  • 1. Scope the PI-scrubbing claims to Raven-owned servers. These still say all output is scrubbed:

    • README.md:3, :849
    • SYSTEM_DESIGN_AND_ARCHITECTURE.md:43, :169, :376, :400, :620, :677

    Name the exception in each one: hosted Sysdig, and jarvis-mcp too.

  • 2. Scope the local-stdio and fixed-tool claims, and list Sysdig as an endpoint.

    • SYSTEM_DESIGN_AND_ARCHITECTURE.md:85, :221 and :652: say "Raven-owned servers".
    • :255: add Sysdig to the §4.2 endpoint list.
1. PI-scrubbing claims made false by this PR

The PR says it plainly at README.md:39 and SYSTEM_DESIGN:632: Sysdig responses do not pass through PiScrubber. The blanket claims around them were left as they were:

  • raven/README.md

    Lines 2 to 4 in 6e35b38

    Local-first AI analysis suite for BC Gov NR Sector. Connects Jira, Confluence, and Bitbucket behind SiteMinder SSO to any LLM — running locally, with personal information scrubbed by default before anything reaches the model.
    ("personal information scrubbed by default before anything reaches the model")
  • raven/README.md

    Lines 848 to 850 in 6e35b38

    - **FOIPPA compliant.** Personal information from Jira/Confluence is processed locally only.
    - **PI scrubbing.** Set `RAVEN_SCRUB_PI=true` in `~/.raven/.env` to automatically anonymize personal information (names, usernames) before it reaches the LLM. One setting controls all servers.
    - **Your IDIR credentials.** RAVEN authenticates as you — it sees exactly what you can see, nothing more.
    ("One setting controls all servers.")
  • - **No data is retained by the AI.** Under a GitHub Copilot Business license with data exclusion enabled, prompts and responses are not stored, not used for training, and not logged by GitHub or any third party.
    - **Personal information is scrubbed before it reaches the AI.** A FOIPPA-compliant PI scrubber replaces names, emails, phone numbers, IDIR usernames, SINs, and tokens with anonymized placeholders.
    - **Credentials never leave the local machine.** Authentication tokens are held in process memory and attached to outbound HTTP requests. They are never included in MCP responses and never sent to the AI.
    ("Personal information is scrubbed before it reaches the AI.")
  • | **Per-tool schema validation** | Zod schemas enforce parameter types, ranges, and enums at the boundary |
    | **PI scrubbing layer** | FOIPPA-compliant anonymization before data reaches the AI |
    | **Read-only by default** | 121 of 181 locally-registered tools are read-only; 60 are writes. Jenkins and Artifactory writes include protected local transfers and guarded upstream mutations. See [TOOL_INVENTORY.md](TOOL_INVENTORY.md) for the authoritative split |
    (two rows below the row this PR edited)
  • RAVEN includes a personal information scrubber (`PiScrubber`) that is applied to all data before it is returned to the AI. This ensures compliance with the BC Freedom of Information and Protection of Privacy Act (FOIPPA).
    ("applied to all data before it is returned to the AI")
  • Every MCP server instantiates a `PiScrubber` and applies it consistently:
    ("Every MCP server instantiates a PiScrubber")
  • RAVEN does not expose arbitrary HTTP or filesystem access. Tool output passes through the PI scrubber before the AI sees it. The Artifactory MCP is the deliberate exception for file transfer: it may read or write only beneath configured, mode-restricted transfer directories and may send uploads only to the configured internal HTTPS Artifactory endpoint. Direct downloads may follow bounded HTTPS redirects only to the configured Artifactory context or exact storage hostnames in `RAVEN_ARTIFACTORY_DOWNLOAD_REDIRECT_HOSTS`; external storage requests receive no Artifactory credentials.
    ("Tool output passes through the PI scrubber before the AI sees it." This is 12 lines above the new Sysdig bullet.)
  • |-------------|-------------------|
    | **FOIPPA s.30 (Protection of PI)** | PI scrubber anonymizes personal information before it reaches the AI. `RAVEN_SCRUB_PI=true` controls this globally. |
    | **Data residency review** | Local MCP execution does not establish Canadian storage for connected services. Cloud services (including Azure DevOps Services and SonarQube Cloud) are opt-in and require an approved privacy/data-residency assessment before enterprise data is sent. SonarQube Cloud offers EU and US regions only. Sysdig Secure MCP is a US-East-hosted third-party service; its STRA has been completed and data sovereignty acknowledged. |
    (the FOIPPA s.30 row: "RAVEN_SCRUB_PI=true controls this globally". The PR edited the row directly below it.)

The §13 compliance table and the executive summary are the parts used in security walkthroughs. The same class was raised in the #96 review.

jarvis-mcp already had this gap before this PR, since it has no PiScrubber, so :400, :677 and README :849 were already loose. Sysdig turns them from loose into false: it is a US-hosted third party. Name both exceptions in one pass.

Suggested wording for SYSTEM_DESIGN:43: "Personal information returned by Raven-owned servers is scrubbed before it reaches the AI. Hosted Sysdig and the Jarvis proxy return responses unscrubbed (see §11.3)."

2. Local-stdio, fixed-tool and endpoint-list claims
  • 4. **Transport is local stdio.** The MCP server runs as a child process on the developer's machine. Communication happens over stdin/stdout — there is no network listener, no open port, and no remote access.
    ("Transport is local stdio … no remote access")
  • 1. **Tool registration is static.** When a server starts, it registers a fixed set of tools. These cannot be modified at runtime by the AI or by any external input.
    ("Tool registration is static … cannot be modified at runtime by … any external input")
  • | Change permissions | **No** | No IAM or permission tools exist. |
    | Access other systems | **No** | Tools connect only to fixed, configured endpoints: Jira, Confluence, Bitbucket, Jira Assets, Jenkins, application servers, and — when configured — Azure DevOps, SonarQube, Artifactory, RFC Buddy, and the Jarvis proxy (§11.3). |
    (§4.2 "Access other systems: No". The endpoint list omits Sysdig, even though the PR updated the matching list in §11.3.)
  • 3. **The AI cannot create new tools.** The tool manifest is fixed at server startup. No amount of prompt engineering can add capabilities that don't exist.
    ("The tool manifest is fixed at server startup")

Sysdig's tool list is defined by Sysdig at runtime and, per Sysdig's docs, changes between releases. The §11.6 "Can the AI be tricked?" answer relies on a fixed manifest.

Round-1 follow-up

  • Item 8 (data residency and default-on): accepted as is. No change needed to the STRA wording or to the default-on entry.
  • Optional: Sysdig stays default-on, its output is unscrubbed, monitored workloads can influence that output, and it shares the session with Raven's write tools. Two cheap mitigations:
    • Add a short Sysdig section to .github/copilot-instructions.md: treat Sysdig output as untrusted, confirm before any Sysdig tool that changes state, and don't paste raw Sysdig output into Jira, Confluence or PR text.
    • Revisit the "Prompt injection via tool output: Low/Low" row at SYSTEM_DESIGN:667.

Minor (non-blocking)

  • ${VAR} URLs pass the gate. validateRemoteMcpEntry checks the parsed URL, not the raw string.
    • https://${X}.sysdig.com/mcp/secure parses to host ${x}.sysdig.com and passes every check.
    • Claude Code expands ${VAR} in .mcp.json url fields, so the host actually contacted would come from the environment.
    • Fix: add /^[a-z0-9.-]+$/.test(hostname) and url.href === configured.url.
    • let url;
      try {
      url = new URL(configured.url);
      } catch {
      throw new Error(`Remote MCP entry '${id}' must have a valid HTTPS URL.`);
      }
      const hostname = url.hostname.toLowerCase();
      if (
      url.protocol !== "https:" ||
      url.username ||
      url.password ||
      url.port ||
      url.search ||
      url.hash ||
      (hostname !== approved.hostSuffix &&
      !hostname.endsWith(`.${approved.hostSuffix}`)) ||
      url.pathname !== approved.pathname
      ) {
  • The host allowlist is wider than the assessment.
    • hostSuffix: "sysdig.com" accepts the apex and any subdomain, and the test asserts that eu1.app.sysdig.com is accepted.
    • The README tells users to switch regions, but the stated STRA covers US East.
    • Fix: pin secure.sysdig.com (or an explicit host list), and say in the docs that another region needs its own assessment.
    • assert.doesNotThrow(() =>
      validateRemoteMcpEntry("sysdig-secure-mcp", { ...ok, url: "https://eu1.app.sysdig.com/mcp/secure" }),
      );
  • No validateCatalog test tampers with the approved entry itself.
    • The mixed-transport test only tampers with jira, which fails the allowlist first. If the wiring skipped validation for approved IDs, every test would stay green.
    • Fix: add validateCatalog(undefined, …) cases for sysdig-secure-mcp with headers, and with a foreign url.
    • test("mixed-transport entries cannot bypass remote validation", () => {
      const mcp = readJson(new URL("../../.mcp.json", import.meta.url));
      const jira = mcp.mcpServers.jira;
      const cases = [
      { ...jira, type: "http", url: "https://attacker.example/mcp", headers: { Authorization: "Bearer x" } },
      { ...jira, headers: { Authorization: "Bearer x" } },
      { ...jira, type: "stdio" },
      ];
      for (const entry of cases) {
      const tampered = { mcpServers: { ...mcp.mcpServers, jira: entry } };
      assert.throws(() => validateCatalog(undefined, tampered), /not an approved remote server/);
      }
      });
  • Misleading error for local entries.
    • A local entry with env, cwd or the valid "type": "stdio" now fails with "Remote MCP entry 'jira' is not an approved remote server."
    • Fix: give catalog IDs their own message, for example "Local MCP entry 'jira' may contain only command and args".
    • const configuredIds = Object.entries(mcp.mcpServers ?? {})
      .filter(([id, configured]) => {
      const keys = Object.keys(configured ?? {}).sort().join(",");
      if (keys === "args,command") return true;
      validateRemoteMcpEntry(id, configured);
      return false;
      })
      .map(([id]) => id);
  • Inventory count is hard-coded.
    • data.length + 2 and the Sysdig rows are hard-coded, and gen-inventory --check never reads .mcp.json. TOOL_INVENTORY.md:10 ("CI-enforced") and :27 ("map 1:1 to the keys in .mcp.json") are therefore not enforced for remote entries.
    • Fix: derive the remote rows from APPROVED_REMOTE_MCP_SERVERS, or add a test that asserts the count matches .mcp.json.
    • "| | Count |", "|---|---|",
      `| MCP servers | **${data.length + 2}** |`,
      `| Tools registered locally (in-process) | **${localTotal}** |`,

Pre-existing (not blocking, for awareness)

  • Launcher text is built from unvalidated entrypoint segments.
    • writeLaunchers interpolates the 3rd and later path segments of entrypoint into double-quoted sh and cmd launchers without escaping, and validateCatalog never constrains those segments.
    • An entrypoint like packages/jira-mcp/dist/$(id).js, mirrored in .mcp.json, would run during validateLauncher in the release build.
    • It needs a merged catalog change. A charset check on entrypoint closes it.
    • for (const entry of releasedEntries(catalog)) {
      const packageRelative = runtimeEntrypoint(entry);
      if (platform === "win32") {
      const entrypoint = packageRelative.replaceAll("/", "\\");
      const content = `@echo off\r\nset "PATH=%~dp0..\\runtime;%PATH%"\r\nset "RAVEN_BUNDLED_RUNTIME=1"\r\nif not "%RAVEN_RELEASE_LAUNCHER_CHECK%"=="1" goto run\r\n"%~dp0..\\runtime\\node.exe" --check "%~dp0..\\runtime\\${entrypoint}"\r\nexit /b %ERRORLEVEL%\r\n:run\r\n"%~dp0..\\runtime\\node.exe" "%~dp0..\\runtime\\${entrypoint}" %*\r\n`;
      writeFileSync(join(binDir, `${entry.launcher}.cmd`), content, "utf8");
      } else {
      const content = `#!/bin/sh\nSCRIPT_DIR=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd)\nPATH="$SCRIPT_DIR/../runtime:$PATH"\nRAVEN_BUNDLED_RUNTIME=1\nexport PATH RAVEN_BUNDLED_RUNTIME\nif [ "\${RAVEN_RELEASE_LAUNCHER_CHECK:-}" = "1" ]; then\n exec "$SCRIPT_DIR/../runtime/node" --check "$SCRIPT_DIR/../runtime/${packageRelative}"\nfi\nexec "$SCRIPT_DIR/../runtime/node" "$SCRIPT_DIR/../runtime/${packageRelative}" "$@"\n`;
      const path = join(binDir, entry.launcher);
      writeFileSync(path, content, "utf8");
      chmodSync(path, 0o755);
      }
      }
      }
      /** Map a catalog source entrypoint into the bundled runtime tree. */
      export function runtimeEntrypoint(entry) {
      const [, , ...entrypointParts] = entry.entrypoint.split("/");
      if (entrypointParts.length === 0) {
      throw new Error(`Invalid catalog entrypoint for '${entry.id}'.`);
      }
      return ["node_modules", ...entry.package.split("/"), ...entrypointParts].join(
      "/",
      );
      }
  • Scrubbing default is documented wrong. SYSTEM_DESIGN:419 says scrubbing is bypassed when RAVEN_SCRUB_PI is unset. The code enables it unless the value is false or 0 (pi-scrubber.ts:12-14 and :267-270).

@SungYu444

Copy link
Copy Markdown
Contributor Author

Addressed in 358d84d: Updated the documentation to distinguish Raven’s PI-scrubbed, locally registered tools from the Jarvis proxy and hosted Sysdig. The release gate now reports a clear local-entry error; regression tests exercise tampering with the approved Sysdig entry; and the inventory generator verifies .mcp.json keys and derives its server count from them.

I’m leaving the two URL-policy suggestions unchanged in this PR. The default remains https://secure.sysdig.com/mcp/secure, but the broader Sysdig-host allowlist and ${VAR} acceptance are not being presented as resolved. As BC gov environment only is accessible when URL is set to US EAST, but it leaves the free to use ability for user with other server region.

@jamesgagan
jamesgagan self-requested a review November 3, 2026 19:30

@jamesgagan jamesgagan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR-review: #101 (no ticket)

Head SHA: 358d84d
Scope: Only what changed since round 2 (6e35b38..358d84d, one commit, 8 files), plus a check that each round-2 finding is resolved.
Tier: Deep, set by scripts/release/release-lib.mjs · Round: 3 · Decoy: rejected 5/5

TL;DR

8 findings: 0 Important, 3 Nit, 5 Pre-existing. Nothing to fix before merge. Both round-2 required changes are done. The release gate accepts exactly the same entries as before, and CI passes.

Required changes

None. It's ready to ship.

Still open from earlier rounds

  • Host allowlist breadth and ${VAR} URLs: the author declined both on purpose, so users outside BC Gov can use other regions. The docs now say the STRA covers the US East deployment only.
  • Optional mitigations (a Sysdig section in copilot-instructions.md, and the SYSTEM_DESIGN:667 row): not adopted. They were optional.
  • Launcher injection (round-2 item I, pre-existing): unchanged.

Resolved this round:

  • Round-2 required items 1 and 2.
  • The local-entry error message.
  • Tests that tamper with the Sysdig entry.
  • The inventory server count, now derived from .mcp.json.
  • The pre-existing RAVEN_SCRUB_PI default in §7.5.

Things that look scary but I verified are fine

  • Moving the "is it local?" test to catalog membership: the security lens traced 14 entry shapes, including proto, null, two-element args and an approved ID in local shape. All still fail closed. Only the error messages changed.
  • gen-inventory.lib.mjs now importing release-lib.mjs: release-lib.mjs does no I/O when imported, so the "pure" header still holds.
  • The server count of 18: 16 local servers + Jarvis + Sysdig. That matches the committed TOOL_INVENTORY, and gen-inventory --check runs in CI.
  • main has moved 6 commits: all are dependency bumps, and none touch PR files.

Lower-priority observations

Nits (3), all verified 5/5:

  • C. Connection direction reversed. SYSTEM_DESIGN:85 (
    4. **Raven-owned servers use local stdio transport.** They run as child processes on the developer's machine with no inbound network listener or open port. The Jarvis proxy forwards to a configured remote API; hosted Sysdig instead connects directly to the MCP client over HTTPS (§11.3).
    ) says "hosted Sysdig … connects directly to the MCP client". It's the other way round:
    the client connects to Sysdig.
  • A. New error is misleading for uncatalogued local entries. Such an entry now gets "Remote MCP entry … not an approved remote server", the mirror image of the round-2 error-message nit. The parity check at :215-221 is now unreachable. release-lib.mjs#L200-L223
    (
    }
    const serverIds = new Set(catalog.servers.map((server) => server.id));
    const configuredIds = Object.entries(mcp.mcpServers ?? {})
    .filter(([id, configured]) => {
    const keys = Object.keys(configured ?? {}).sort().join(",");
    if (serverIds.has(id)) {
    if (keys !== "args,command") {
    throw new Error(`Local MCP entry '${id}' may contain only command and args.`);
    }
    return true;
    }
    validateRemoteMcpEntry(id, configured);
    return false;
    })
    .map(([id]) => id);
    if (
    configuredIds.some((id) => !serverIds.has(id)) ||
    serverIds.size !== configuredIds.length
    ) {
    throw new Error(
    "Release catalog and .mcp.json contain different server IDs.",
    );
    }
    return catalog;
    )
  • G. New inventory key check is hard to diagnose. Its error names no key and is thrown outside the try/catch. The new-package checklist doesn't mention SERVERS. gen-inventory.lib.mjs#L33-L39
    (
    }
    const actual = Object.keys(servers).sort();
    if (actual.length !== expected.length || actual.some((id, i) => id !== expected[i])) {
    throw new Error("Inventory MCP server keys do not match .mcp.json.");
    }
    for (const id of remoteIds) validateRemoteMcpEntry(id, servers[id]);
    return actual.length;
    )

Pre-existing (5), all verified 5/5:

  • B. Appendix C.7 (:923, :929): says PI is "already removed" with no exception. Same class as round-2 item 1. My round-2 miss. It's the one I'd ask the author to fix, and it takes a one-line qualifier. SYSTEM_DESIGN#L922-L930
    (
    1. **Layer 1 — PI Scrubbing (RAVEN):** Personal information is anonymized before it leaves the local MCP server
    2. **Layer 2 — Content Filtering (GitHub):** Prompts pass through safety filters before reaching the model
    3. **Layer 3 — Zero Data Retention (Model Provider):** Data exists only in RAM during processing and is immediately discarded
    4. **Layer 4 — No Training (Contractual):** Customer data is contractually excluded from model training by all providers
    5. **Layer 5 — IDE Access Policy (GitHub):** Prompts and suggestions from IDE-based Chat are not retained at all
    The result: even in the brief, transient moment when data is processed by the LLM, personal information has already been removed. After the response is delivered, nothing persists anywhere in the chain.
    )
  • F. TOOL_INVENTORY:173: RAVEN_SCRUB_PI is still listed as used by "all" servers. The matching SYSTEM_DESIGN row was fixed.
  • I. README:847: "All data stays local / no Atlassian data to external LLM providers" is false with Copilot as the client.
  • H. SharePoint inline images: they're unscrubbed, but SYSTEM_DESIGN never mentions SharePoint, and :83 says responses contain "no binary data".
  • D. release-lib.mjs:195: \ to / normalisation lets ".\packages\…" pass. On POSIX that runs a different file, which only exists if the PR also commits it. Low severity.

Needs a human look

None. Every finding got 5/5.

Findings I considered and dropped

  • JSDoc on validateInventoryMcpServers and render: the repo convention is one-line JSDoc, the same call as in round 2.
  • Stale test name; the Sysdig tamper regex also matches "not approved": the test that runs against the real .mcp.json catches that case.
  • copilot-instructions:142 STRA sentence not narrowed: item 8 is accepted, and the sentence isn't false.
  • "Sysdig subdomains" also matching the apex domain; :636 omitting Sysdig; where OAuth tokens live: all fall inside the allowlist breadth the author already declined.
  • Commit subject not imperative: the PR will be squash-merged. Stale tool counts were already noted in round 2.

Net recommendation

Approve. The 3 Nits and the C.7 qualifier take about 10 minutes and can go in a fixup or a follow-up PR, whichever the author prefers.

@jamesgagan
jamesgagan merged commit 04b1870 into bcgov:main Nov 3, 2026
1 check passed
@SungYu444
SungYu444 deleted the feature/sysdig-mcp branch November 3, 2026 20:19
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.

4 participants