Platform api keys#3024
Conversation
…rkOS Mint/list/revoke now forward the caller's session bearer to the backend's /web/api-keys routes; bearer-auth validates sk_mcpjam_ keys with a single service-token call (format-gated + rate-limited by hash) instead of WorkOS + binding lookup. Same context fields downstream, same client shapes. Removes the workos-client / identity / workos-key-bindings services. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…leanup - request-local: stop collapsing cached null to undefined (invalid keys were re-validated when bearer-auth runs on parent + sub-router) - bearer-auth: cap the rate-limit bucket map at 50k entries (distinct valid-format invalid keys could grow it unbounded between sweeps), align log labels, cross-reference the key-format contract with the backend - errors.ts: shared statusToErrorCode, used by oauth + api-keys routes - types/hono.ts: docstrings updated for the platform-key flow - docs: drop ORPHANED_KEY (no longer emitted), document sk_mcpjam_ format - new platform-api-key-validation tests (routing-404 disambiguation, sha256 parity vector) porting the deleted identity.test.ts coverage Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
dosubot
Bot
added
the
size:XXL
ignaciojimenezr
temporarily deployed
to
preview-pr-3024
GitHub Actions
Inactive
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Internal previewPreview URL: https://mcp-inspector-pr-3024.up.railway.app |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR migrates ChangesRelated issues: None provided Sequence Diagram(s)See the embedded diagrams above. Poem: 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. 🔧 ESLint
mcpjam-inspector/client/src/components/settings/api-keys/RevealOnceDialog.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: ESLint configuration in --config is invalid:
mcpjam-inspector/server/middleware/__tests__/bearer-auth.test.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: ESLint configuration in --config is invalid:
mcpjam-inspector/server/middleware/bearer-auth.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: ESLint configuration in --config is invalid:
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. 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: 2
🧹 Nitpick comments (4)
mcpjam-inspector/server/routes/v1/__tests__/computer-environments.test.ts (1)
29-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated platform-key mock into a shared test helper.
The same
vi.mock("../../../services/platform-api-key-validation.js", ...)block (importOriginal + stub returningnull) is duplicated verbatim across this file,harness.test.ts,hosts.test.ts, andtunnels.test.ts. Since all four only need the same "unreachable, stub to null" behavior, a sharedmockPlatformApiKeyValidation()test helper would cut the duplication without changing behavior.🤖 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 `@mcpjam-inspector/server/routes/v1/__tests__/computer-environments.test.ts` around lines 29 - 37, The platform API-key mock setup is duplicated across multiple test files, so extract the repeated vi.mock for platform-api-key-validation.js into a shared test helper and reuse it from computer-environments.test.ts, harness.test.ts, hosts.test.ts, and tunnels.test.ts. Keep the helper responsible for the same importOriginal plus validatePlatformApiKey stub that resolves to null, and update each test file to call the helper instead of inlining the mock block.docs/reference/public-api.mdx (1)
106-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSame stale
sk_…prefix nearby.Line 107 still says
an sk_… bearer fail with 403 FORBIDDEN, while the key format was just updated tosk_mcpjam_…a few lines above (90-91). Minor, but worth a pass for consistency while this file is being edited.✏️ Suggested tweak
- with an `sk_…` bearer fail with `403 FORBIDDEN`. Create and revoke keys in + with an `sk_mcpjam_…` bearer fail with `403 FORBIDDEN`. Create and revoke keys in🤖 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 `@docs/reference/public-api.mdx` around lines 106 - 108, Update the API key-management note in public-api.mdx to use the new key prefix consistently. The stale “sk_…” reference in the sentence about `403 FORBIDDEN` should be changed to match the updated `sk_mcpjam_…` format already used earlier in the same document, keeping the wording aligned around the API key-management behavior.docs/reference/api-keys.mdx (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale
sk_…reference next to the newly updated format wording.Line 19 still reads
Bearer sk_…while the surrounding bullets (lines 9-13, 22) were just updated to the explicitsk_mcpjam_…format. Worth aligning for consistency now that this doc is being touched.✏️ Suggested tweak
-**When it's used:** Every `Authorization: Bearer sk_…` request to `/api/v1/*` — validating servers, running the doctor, listing tools/prompts/resources, reading resources, exporting snapshots, and **saving `@mcpjam/sdk` eval results** (set `MCPJAM_API_KEY` to this key; see [Saving Results](/sdk/concepts/saving-results)). +**When it's used:** Every `Authorization: Bearer sk_mcpjam_…` request to `/api/v1/*` — validating servers, running the doctor, listing tools/prompts/resources, reading resources, exporting snapshots, and **saving `@mcpjam/sdk` eval results** (set `MCPJAM_API_KEY` to this key; see [Saving Results](/sdk/concepts/saving-results)).🤖 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 `@docs/reference/api-keys.mdx` at line 19, The API key usage note still refers to the old generic `Bearer sk_…` token format, which is inconsistent with the updated `sk_mcpjam_…` wording elsewhere in this doc. Update the “When it’s used” line in `api-keys.mdx` to use the same explicit `sk_mcpjam_…` format, keeping the rest of the MCPJAM_API_KEY guidance unchanged.mcpjam-inspector/server/routes/web/api-keys.ts (1)
85-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing the shared
CONVEX_HTTP_URLguard and distinguishing timeouts from unreachability.Two small nits in
callBackendApiKeys:
- The missing-
CONVEX_HTTP_URLcheck (lines 85-92) duplicatesgetConvexHttpUrl()inoauth.tsalmost verbatim — worth extracting toerrors.tsalongside the newly-addedstatusToErrorCode, following the same pattern this PR just established.- The catch block (106-116) maps every fetch failure — including an
AbortErrorfrom the 30sAbortSignal.timeout— to502 SERVER_UNREACHABLE. A timed-out backend and an unreachable one are different failure modes; distinguishing viaerror.name === "AbortError"→ErrorCode.TIMEOUT(504) would give clients a more actionable signal, consistent withErrorCode.TIMEOUTalready existing for this purpose.♻️ Proposed refactor sketch
+function getConvexHttpUrlOrThrow(): string { + const convexUrl = process.env.CONVEX_HTTP_URL; + if (!convexUrl) { + throw new WebRouteError( + 500, + ErrorCode.INTERNAL_ERROR, + "Server missing CONVEX_HTTP_URL configuration", + ); + } + return convexUrl; +} ... - const convexUrl = process.env.CONVEX_HTTP_URL; - if (!convexUrl) { - throw new WebRouteError( - 500, - ErrorCode.INTERNAL_ERROR, - "Server missing CONVEX_HTTP_URL configuration", - ); - } + const convexUrl = getConvexHttpUrlOrThrow(); const target = `${convexUrl}/web/api-keys${pathAndQuery}`; ... } catch (error) { logger.error("API key backend request failed", { method, error: error instanceof Error ? error.message : String(error), }); + if (error instanceof Error && error.name === "AbortError") { + throw new WebRouteError( + 504, + ErrorCode.TIMEOUT, + "Timed out reaching the MCPJam backend to manage API keys", + ); + } throw new WebRouteError( 502, ErrorCode.SERVER_UNREACHABLE, "Could not reach the MCPJam backend to manage API keys", ); }🤖 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 `@mcpjam-inspector/server/routes/web/api-keys.ts` around lines 85 - 116, The `callBackendApiKeys` backend fetch error handling currently treats all failures the same and duplicates the `CONVEX_HTTP_URL` guard. Reuse the shared `getConvexHttpUrl` helper from `errors.ts` instead of inlining the environment check, and update the `fetch` catch in `callBackendApiKeys` to distinguish `AbortError` from other failures so timeouts map to `ErrorCode.TIMEOUT` with a 504 response while non-timeout failures remain `SERVER_UNREACHABLE`.
🤖 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.
Inline comments:
In `@mcpjam-inspector/server/middleware/bearer-auth.ts`:
- Around line 178-180: The error logging in bearer-auth’s validation catch block
is passing a plain object as the second argument, so the caught exception is not
being treated as the actual error by logger.error. Update the logger.error call
in the bearer-auth middleware to pass the caught error value itself as the
second argument, and move any structured metadata into the third context
argument so Sentry/Axiom receives the exception correctly.
In `@mcpjam-inspector/server/services/platform-api-key-validation.ts`:
- Around line 46-53: The platform API key validation fetch can hang
indefinitely, stalling `sk_mcpjam_...` auth requests. Update the `fetch` call in
`validatePlatformApiKey` to pass an `AbortSignal.timeout(...)` via the request
options so slow or dead backends are aborted. Keep the existing error handling
path in `bearer-auth.ts` unchanged, since it already handles the abort and
returns the current 500 response.
---
Nitpick comments:
In `@docs/reference/api-keys.mdx`:
- Line 19: The API key usage note still refers to the old generic `Bearer sk_…`
token format, which is inconsistent with the updated `sk_mcpjam_…` wording
elsewhere in this doc. Update the “When it’s used” line in `api-keys.mdx` to use
the same explicit `sk_mcpjam_…` format, keeping the rest of the MCPJAM_API_KEY
guidance unchanged.
In `@docs/reference/public-api.mdx`:
- Around line 106-108: Update the API key-management note in public-api.mdx to
use the new key prefix consistently. The stale “sk_…” reference in the sentence
about `403 FORBIDDEN` should be changed to match the updated `sk_mcpjam_…`
format already used earlier in the same document, keeping the wording aligned
around the API key-management behavior.
In `@mcpjam-inspector/server/routes/v1/__tests__/computer-environments.test.ts`:
- Around line 29-37: The platform API-key mock setup is duplicated across
multiple test files, so extract the repeated vi.mock for
platform-api-key-validation.js into a shared test helper and reuse it from
computer-environments.test.ts, harness.test.ts, hosts.test.ts, and
tunnels.test.ts. Keep the helper responsible for the same importOriginal plus
validatePlatformApiKey stub that resolves to null, and update each test file to
call the helper instead of inlining the mock block.
In `@mcpjam-inspector/server/routes/web/api-keys.ts`:
- Around line 85-116: The `callBackendApiKeys` backend fetch error handling
currently treats all failures the same and duplicates the `CONVEX_HTTP_URL`
guard. Reuse the shared `getConvexHttpUrl` helper from `errors.ts` instead of
inlining the environment check, and update the `fetch` catch in
`callBackendApiKeys` to distinguish `AbortError` from other failures so timeouts
map to `ErrorCode.TIMEOUT` with a 504 response while non-timeout failures remain
`SERVER_UNREACHABLE`.
🪄 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: c326faa6-74d1-4953-88f4-28317ea5400e
📒 Files selected for processing (21)
docs/reference/api-keys.mdxdocs/reference/public-api.mdxmcpjam-inspector/server/middleware/__tests__/bearer-auth.test.tsmcpjam-inspector/server/middleware/bearer-auth.tsmcpjam-inspector/server/middleware/request-local.tsmcpjam-inspector/server/routes/v1/__tests__/computer-environments.test.tsmcpjam-inspector/server/routes/v1/__tests__/harness.test.tsmcpjam-inspector/server/routes/v1/__tests__/hosts.test.tsmcpjam-inspector/server/routes/v1/__tests__/tunnels.test.tsmcpjam-inspector/server/routes/v1/__tests__/write-routes.test.tsmcpjam-inspector/server/routes/web/__tests__/api-keys.test.tsmcpjam-inspector/server/routes/web/api-keys.tsmcpjam-inspector/server/routes/web/errors.tsmcpjam-inspector/server/routes/web/oauth.tsmcpjam-inspector/server/services/__tests__/identity.test.tsmcpjam-inspector/server/services/__tests__/platform-api-key-validation.test.tsmcpjam-inspector/server/services/identity.tsmcpjam-inspector/server/services/platform-api-key-validation.tsmcpjam-inspector/server/services/workos-client.tsmcpjam-inspector/server/services/workos-key-bindings.tsmcpjam-inspector/server/types/hono.ts
💤 Files with no reviewable changes (4)
- mcpjam-inspector/server/services/tests/identity.test.ts
- mcpjam-inspector/server/services/workos-client.ts
- mcpjam-inspector/server/services/identity.ts
- mcpjam-inspector/server/services/workos-key-bindings.ts
| logger.error("Platform API key validation failed", { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep run --pattern 'error($_, $_, $_) { $$$ }' --lang typescript mcpjam-inspector/server/utils/logger.ts
rg -nP 'logger\.error\(' mcpjam-inspector/server -A2Repository: MCPJam/inspector
Length of output: 31834
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the logger implementation and its tests for parameter order/serialization.
sed -n '1,260p' mcpjam-inspector/server/utils/logger.ts
printf '\n--- TESTS ---\n'
sed -n '1,180p' mcpjam-inspector/server/utils/__tests__/logger.test.tsRepository: MCPJam/inspector
Length of output: 10986
Pass the caught error as the second logger argument
logger.error(message, error?, context?) treats the second slot as the exception, so { error: ... } lands as a plain object in Sentry/Axiom. Forward the caught value as error and keep structured fields in the third argument.
🤖 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 `@mcpjam-inspector/server/middleware/bearer-auth.ts` around lines 178 - 180,
The error logging in bearer-auth’s validation catch block is passing a plain
object as the second argument, so the caught exception is not being treated as
the actual error by logger.error. Update the logger.error call in the
bearer-auth middleware to pass the caught error value itself as the second
argument, and move any structured metadata into the third context argument so
Sentry/Axiom receives the exception correctly.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 21 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="mcpjam-inspector/server/routes/web/errors.ts">
<violation number="1" location="mcpjam-inspector/server/routes/web/errors.ts:156">
P3: HTTP 503 (Service Unavailable) isn't mapped and falls through to `INTERNAL_ERROR`. If an upstream provider returns 503 during a transient outage, the frontend will surface a generic internal error rather than an unreachable-server message. Consider adding `if (status === 503) return ErrorCode.SERVER_UNREACHABLE;` since the semantics align with the existing 502 path.</violation>
<violation number="2" location="mcpjam-inspector/server/routes/web/errors.ts:156">
P2: A private `statusToErrorCode` with identical logic already exists in `server/utils/server-secrets.ts`. Since that file already imports from `./errors.js`, it can import `statusToErrorCode` from the shared module instead — removing the private copy keeps one source of truth for the mapping.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| * Map an upstream HTTP status onto the matching web ErrorCode when relaying a | ||
| * backend/provider error. Shared by the OAuth proxy and API key routes. | ||
| */ | ||
| export function statusToErrorCode(status: number): ErrorCode { |
There was a problem hiding this comment.
P2: A private statusToErrorCode with identical logic already exists in server/utils/server-secrets.ts. Since that file already imports from ./errors.js, it can import statusToErrorCode from the shared module instead — removing the private copy keeps one source of truth for the mapping.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/routes/web/errors.ts, line 156:
<comment>A private `statusToErrorCode` with identical logic already exists in `server/utils/server-secrets.ts`. Since that file already imports from `./errors.js`, it can import `statusToErrorCode` from the shared module instead — removing the private copy keeps one source of truth for the mapping.</comment>
<file context>
@@ -149,6 +149,21 @@ export function assertBearerToken(c: any): string {
+ * Map an upstream HTTP status onto the matching web ErrorCode when relaying a
+ * backend/provider error. Shared by the OAuth proxy and API key routes.
+ */
+export function statusToErrorCode(status: number): ErrorCode {
+ if (status === 400) return ErrorCode.VALIDATION_ERROR;
+ if (status === 401) return ErrorCode.UNAUTHORIZED;
</file context>
| @@ -149,6 +149,21 @@ export function assertBearerToken(c: any): string { | |||
| return authHeader.slice("Bearer ".length); | |||
There was a problem hiding this comment.
P3: HTTP 503 (Service Unavailable) isn't mapped and falls through to INTERNAL_ERROR. If an upstream provider returns 503 during a transient outage, the frontend will surface a generic internal error rather than an unreachable-server message. Consider adding if (status === 503) return ErrorCode.SERVER_UNREACHABLE; since the semantics align with the existing 502 path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/routes/web/errors.ts, line 156:
<comment>HTTP 503 (Service Unavailable) isn't mapped and falls through to `INTERNAL_ERROR`. If an upstream provider returns 503 during a transient outage, the frontend will surface a generic internal error rather than an unreachable-server message. Consider adding `if (status === 503) return ErrorCode.SERVER_UNREACHABLE;` since the semantics align with the existing 502 path.</comment>
<file context>
@@ -149,6 +149,21 @@ export function assertBearerToken(c: any): string {
+ * Map an upstream HTTP status onto the matching web ErrorCode when relaying a
+ * backend/provider error. Shared by the OAuth proxy and API key routes.
+ */
+export function statusToErrorCode(status: number): ErrorCode {
+ if (status === 400) return ErrorCode.VALIDATION_ERROR;
+ if (status === 401) return ErrorCode.UNAUTHORIZED;
</file context>
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="mcpjam-inspector/server/routes/web/errors.ts">
<violation number="1" location="mcpjam-inspector/server/routes/web/errors.ts:156">
P3: HTTP 503 (Service Unavailable) isn't mapped and falls through to `INTERNAL_ERROR`. If an upstream provider returns 503 during a transient outage, the frontend will surface a generic internal error rather than an unreachable-server message. Consider adding `if (status === 503) return ErrorCode.SERVER_UNREACHABLE;` since the semantics align with the existing 502 path.</violation>
<violation number="2" location="mcpjam-inspector/server/routes/web/errors.ts:156">
P2: A private `statusToErrorCode` with identical logic already exists in `server/utils/server-secrets.ts`. Since that file already imports from `./errors.js`, it can import `statusToErrorCode` from the shared module instead — removing the private copy keeps one source of truth for the mapping.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
ignaciojimenezr
had a problem deploying
to
preview-pr-3024
GitHub Actions
Error
2 participants
Fix API key creation (drop WorkOS)
Right now, creating an API key requires the user to belong to a WorkOS "organization" — only three of us are in the org, so it's broken for real users.
This change stops using WorkOS for keys and creates/checks them in Convex instead. We used to do this before WorkOS (for the old
mcpjam_keys), so it's mostly reviving that approach — just scoped to orgs now.sk_..., CLI/SDK and the settings UI are untouchedValidated in dev: a newly created sk_mcpjam_... key successfully authenticated against Inspector /api/v1/me with 200 OK via backend platform key validation.
Summary by cubic
Migrated MCPJam authentication to platform API keys validated by the Convex backend, replacing WorkOS. New keys use the
sk_mcpjam_…format and are documented; oldersk_…keys now return 401.Refactors
bearer-auth: gate onsk_mcpjam_[0-9a-f]{48}, hash with SHA-256, validate via backend, memoize per request; rate-limit by key hash (60/min, burst 10) and spray-limit by canonical client IP; cap buckets at 50k.authMethod: "workos_api_key",workosApiKeyId,workosUserId,mcpjamUserId,mcpjamOrganizationId)./api/web/api-keys: proxy to backend/web/api-keyswith the caller’s bearer; map to existing shapes; unify errors viastatusToErrorCode; reject legacysk_…bearers with 403 before proxying.sk_mcpjam_…; dropORPHANED_KEY; clarify 401/403 copy.workos-client,workos-key-bindings,identity) and related tests; addplatform-api-key-validation(sha256 parity and routing-404 tests).RevealOnceDialogresponsive and prevent overflow on small screens.Migration
sk_mcpjam_…keys now 401 ("Invalid API key").CONVEX_HTTP_URLandINSPECTOR_SERVICE_TOKENare required;WORKOS_API_KEYis no longer needed in the inspector./api/v1/*callers; key management via API is blocked (403) — use Settings → API keys.Written for commit a64bf42. Summary will update on new commits.