Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- The card payload builder is best-effort: a failed risk-history fetch degrades the card (or drops it) without affecting the tool result. New contract tests in `src/__tests__/mcp-apps.test.ts` pin the `_meta` advertisement, the `ui://` resource wire shape, the neutral-default/brand-injection behavior, and the card normalization.

### Fixed
- A `KNOWBE4_BASE_URL` containing a path prefix is no longer silently discarded. `apiRequest()` built its request URL with `new URL(path, creds.baseUrl)`, and the two-argument `URL` constructor performs RFC 3986 relative resolution rather than string concatenation. Every call site passes a path-absolute reference (`/api/v1/account`, `/api/v1/phishing/security_tests`, …), which by that rule replaces the base URL's own path entirely — so a base of `https://proxy.corp.example/knowbe4/` produced `https://proxy.corp.example/api/v1/account` and every request went to the wrong place. The bug was invisible by default because the built-in region defaults (`https://us.api.knowbe4.com` and friends) are bare origins with no path to lose; it only bit self-hosters who pointed `KNOWBE4_BASE_URL` at a reverse proxy or API gateway mounted under a path, exactly the use case the README documents as "Custom base URL (overrides region)". The URL is now assembled by explicit slash normalization and concatenation, so the base's path survives regardless of leading/trailing slashes on either side, redundant slashes collapse to one, and query params still append after the joined path. Regression tests in `src/__tests__/client.test.ts` pin the exact URL handed to `fetch` for bare-origin, prefixed-with-trailing-slash, prefixed-without-trailing-slash, doubled-slash, and query-param cases.
- HTTP transport now builds a fresh `Server` + `StreamableHTTPServerTransport` per `/mcp` request (stateless mode, `sessionIdGenerator: undefined`) instead of sharing one stateful transport for the whole process. The shared stateful transport (created with `sessionIdGenerator: () => randomUUID()`) only accepted one `initialize`, so behind the multi-user gateway only the first client since container start received tools — every subsequent client got `-32600 "Server already initialized"` and saw zero tools until a restart. Each request is now independent, so multiple clients work simultaneously. Handler registration was extracted into a `createFreshServer()` factory; stdio mode keeps its single shared server. Per-request server/transport are disposed on response close, and non-`POST` `/mcp` requests now return `405`.
- Hardened the per-request HTTP handler: the whole body is wrapped so any failure returns `500 {"jsonrpc":"2.0","error":{"code":-32603,"message":"Internal error"},"id":null}` and is never rethrown, preventing a single bad request from escaping as an `unhandledRejection` that could crash the container.
- `/health` is now a shallow, unauthenticated liveness probe returning `200 {"status":"ok"}` — it no longer calls `getCredentials()`. In gateway mode (`AUTH_MODE=gateway`) credentials only arrive per-request via the `X-KnowBe4-API-Key` header, so the previous credential-gated `/health` always returned `503`, causing the Azure liveness probe to fail and SIGTERM-kill the container (crash loop). Added `/healthz` as an alias.
Expand Down
61 changes: 60 additions & 1 deletion src/__tests__/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
*/

import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
import { getCredentials } from "../utils/client.js";
import { apiRequest, getCredentials } from "../utils/client.js";

describe("getCredentials", () => {
const originalEnv = process.env;
Expand Down Expand Up @@ -86,3 +86,62 @@ describe("getCredentials", () => {
expect(creds!.baseUrl).toBe("https://eu.api.knowbe4.com");
});
});

describe("apiRequest URL construction", () => {
const originalEnv = process.env;
let fetchMock: ReturnType<typeof vi.fn>;

beforeEach(() => {
process.env = { ...originalEnv };
process.env.KNOWBE4_API_KEY = "test-key";
fetchMock = vi.fn().mockResolvedValue({
ok: true,
status: 200,
statusText: "OK",
text: async () => "{}",
});
vi.stubGlobal("fetch", fetchMock);
});

afterEach(() => {
process.env = originalEnv;
vi.unstubAllGlobals();
});

/** The URL the stubbed fetch was handed on its most recent call. */
const requestedUrl = () => fetchMock.mock.calls[0][0] as string;

it("should append the path to a bare-origin base URL", async () => {
process.env.KNOWBE4_BASE_URL = "https://us.api.knowbe4.com";
await apiRequest("/api/v1/account");
expect(requestedUrl()).toBe("https://us.api.knowbe4.com/api/v1/account");
});

it("should preserve a base URL path prefix with a trailing slash", async () => {
process.env.KNOWBE4_BASE_URL = "https://proxy.corp.example/knowbe4/";
await apiRequest("/api/v1/account");
expect(requestedUrl()).toBe("https://proxy.corp.example/knowbe4/api/v1/account");
});

it("should preserve a base URL path prefix without a trailing slash", async () => {
process.env.KNOWBE4_BASE_URL = "https://proxy.corp.example/knowbe4";
await apiRequest("/api/v1/phishing/security_tests");
expect(requestedUrl()).toBe(
"https://proxy.corp.example/knowbe4/api/v1/phishing/security_tests"
);
});

it("should collapse redundant slashes on either side of the join", async () => {
process.env.KNOWBE4_BASE_URL = "https://proxy.corp.example/knowbe4//";
await apiRequest("//api/v1/users");
expect(requestedUrl()).toBe("https://proxy.corp.example/knowbe4/api/v1/users");
});

it("should append query params after the joined path", async () => {
process.env.KNOWBE4_BASE_URL = "https://proxy.corp.example/knowbe4/";
await apiRequest("/api/v1/users", { params: { page: 2, per_page: 50 } });
expect(requestedUrl()).toBe(
"https://proxy.corp.example/knowbe4/api/v1/users?page=2&per_page=50"
);
});
});
8 changes: 7 additions & 1 deletion src/utils/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,13 @@ export async function apiRequest<T>(
);
}

const url = new URL(path, creds.baseUrl);
// Join by string, not `new URL(path, base)`: the two-arg form does RFC 3986
// relative resolution, so a path-absolute reference like "/api/v1/account"
// replaces the base's own path entirely. That silently drops the prefix of a
// KNOWBE4_BASE_URL pointing at a proxy/gateway (e.g. https://proxy/knowbe4/).
const normalizedBase = creds.baseUrl.replace(/\/+$/, "");
const normalizedPath = path.replace(/^\/+/, "");
const url = new URL(`${normalizedBase}/${normalizedPath}`);

if (options.params) {
for (const [key, value] of Object.entries(options.params)) {
Expand Down
Loading