fix: enforce API key model restrictions across all /v1/* endpoints - #131
Conversation
…endpoints isModelAllowedForKey() existed in src/lib/db/apiKeys.ts but was never called anywhere. API keys with allowedModels restrictions could access any model through any endpoint. Changes: - Add shared enforceApiKeyPolicy() middleware (model restriction + budget) - Wire it into chat handler (replacing inline budget-only check) - Wire it into all /v1/* endpoints: embeddings, images/generations, audio/speech, audio/transcriptions, moderations, rerank - Wire it into provider-specific endpoints: /v1/providers/[provider]/embeddings, /v1/providers/[provider]/images/generations The middleware checks: 1. Model restriction — if key has allowedModels, verify the model is permitted 2. Budget limit — if key has budget configured, verify it hasn't been exceeded Fixes diegosouzapw#130
Summary of ChangesHello @ersintarhan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical security and resource management gap by implementing a robust API key policy enforcement mechanism. It centralizes model restriction and budget limit checks into a single, reusable middleware and integrates it across all relevant Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR closes a policy enforcement gap by introducing a shared API-key policy helper and wiring it into the various /v1/* endpoints so that API keys with allowedModels and/or budget limits are consistently enforced.
Changes:
- Added
enforceApiKeyPolicy()shared utility to enforce model allowlists and budget limits for API-keyed requests. - Replaced/augmented endpoint-specific logic across chat + multiple
/v1/*routes to call the shared policy helper. - Centralized the model restriction + budget rejection behavior to reduce drift across endpoints.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/shared/utils/apiKeyPolicy.ts | New shared policy helper that fetches API key metadata and enforces model allowlists + budget limits. |
| src/sse/handlers/chat.ts | Replaces inline budget-only checks with enforceApiKeyPolicy() during the chat pipeline. |
| src/app/api/v1/embeddings/route.ts | Enforces API key policy before forwarding embedding requests. |
| src/app/api/v1/images/generations/route.ts | Enforces API key policy before forwarding image generation requests. |
| src/app/api/v1/audio/speech/route.ts | Enforces API key policy before forwarding TTS requests. |
| src/app/api/v1/audio/transcriptions/route.ts | Enforces API key policy for multipart transcription requests. |
| src/app/api/v1/moderations/route.ts | Enforces API key policy for moderation requests (including default model behavior). |
| src/app/api/v1/rerank/route.ts | Enforces API key policy for rerank requests. |
| src/app/api/v1/providers/[provider]/embeddings/route.ts | Enforces API key policy after provider-prefix normalization for provider-specific embeddings. |
| src/app/api/v1/providers/[provider]/images/generations/route.ts | Enforces API key policy after provider-prefix normalization for provider-specific image generation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Enforce API key policies (model restrictions + budget limits) | ||
| const policy = await enforceApiKeyPolicy(request, body.model); | ||
| if (policy.rejection) return policy.rejection; | ||
|
|
There was a problem hiding this comment.
Policy enforcement runs before parsing/normalizing the image model. Since parseImageModel supports bare model IDs, a restricted key may be incorrectly denied when the client omits the provider prefix (while allowedModels typically uses provider/model). Consider normalizing (parseImageModel -> provider/model) before calling enforceApiKeyPolicy.
| const model = body.model || "omni-moderation-latest"; | ||
|
|
||
| // Enforce API key policies (model restrictions + budget limits) | ||
| const policy = await enforceApiKeyPolicy(request, model); | ||
| if (policy.rejection) return policy.rejection; |
There was a problem hiding this comment.
Policy enforcement runs before parsing/normalizing the moderation model. parseModerationModel supports both "openai/model" and bare model IDs; /v1/models returns provider/model IDs. Consider building a canonical provider/model string (e.g., from parseModerationModel) before calling enforceApiKeyPolicy so allowedModels checks behave consistently.
| // Enforce API key policies (model restrictions + budget limits) | ||
| const policy = await enforceApiKeyPolicy(request, body.model); | ||
| if (policy.rejection) return policy.rejection; |
There was a problem hiding this comment.
Policy enforcement runs before parsing/normalizing the speech model. parseSpeechModel supports bare model IDs; if allowedModels is stored as provider/model (as in /v1/models), restricted keys may be incorrectly denied when clients omit the provider prefix. Consider normalizing (parseSpeechModel -> provider/model) before calling enforceApiKeyPolicy.
| } catch { | ||
| // If metadata fetch fails, don't block — degrade gracefully | ||
| return { apiKey, apiKeyInfo: null, rejection: null }; |
There was a problem hiding this comment.
If getApiKeyMetadata() throws (DB unavailable/transient error), this helper currently fails open and skips both model and budget enforcement. That can reintroduce the original bypass during outages. Consider at least logging the error and/or failing closed (e.g., 503) when an API key is presented, or making fail-open vs fail-closed configurable.
| } catch { | |
| // If metadata fetch fails, don't block — degrade gracefully | |
| return { apiKey, apiKeyInfo: null, rejection: null }; | |
| } catch (err) { | |
| // If metadata fetch fails, fail closed to avoid bypassing policy checks | |
| console.error("Failed to fetch API key metadata", err); | |
| return { | |
| apiKey, | |
| apiKeyInfo: null, | |
| rejection: errorResponse( | |
| HTTP_STATUS.SERVICE_UNAVAILABLE, | |
| "Unable to validate API key metadata. Please try again later." | |
| ), | |
| }; |
| export async function enforceApiKeyPolicy( | ||
| request: Request, | ||
| modelStr: string | null | ||
| ): Promise<ApiKeyPolicyResult> { | ||
| const apiKey = extractApiKey(request); |
There was a problem hiding this comment.
This new shared policy layer introduces security-critical behavior (model allowlist patterns + budget rejection) but currently has no automated tests. Adding unit tests covering exact match, prefix (provider/*), wildcard patterns, and budget-exceeded responses would help prevent regressions as more /v1 endpoints adopt this helper.
|
|
||
| // ── Check 1: Model restriction ── | ||
| if (modelStr && apiKeyInfo.allowedModels && apiKeyInfo.allowedModels.length > 0) { | ||
| const allowed = await isModelAllowedForKey(apiKey, modelStr); |
There was a problem hiding this comment.
enforceApiKeyPolicy already fetched apiKeyInfo (including allowedModels), but isModelAllowedForKey() will fetch metadata again internally. Even with caching, this is duplicated work for restricted keys. Consider refactoring the model-allowed check to reuse the already-fetched allowedModels/metadata.
| const allowed = await isModelAllowedForKey(apiKey, modelStr); | |
| const allowed = | |
| Array.isArray(apiKeyInfo.allowedModels) && | |
| apiKeyInfo.allowedModels.includes(modelStr); |
| // Pipeline: API key policy enforcement (model restrictions + budget limits) | ||
| telemetry.startPhase("policy"); | ||
| if (apiKeyInfo?.id) { | ||
| try { | ||
| const budgetOk = checkBudget(apiKeyInfo.id); | ||
| if (!budgetOk.allowed) { | ||
| log.warn("BUDGET", `API key ${apiKeyInfo.id} exceeded budget: ${budgetOk.reason}`); | ||
| return errorResponse(429, budgetOk.reason || "Budget limit exceeded"); | ||
| } | ||
| } catch { | ||
| // Budget check is best-effort — don't block on errors | ||
| } | ||
| const policy = await enforceApiKeyPolicy(request, modelStr); | ||
| if (policy.rejection) { | ||
| log.warn("POLICY", `API key policy rejected: ${modelStr} (key=${policy.apiKeyInfo?.id || "unknown"})`); | ||
| return policy.rejection; | ||
| } | ||
| const apiKeyInfo = policy.apiKeyInfo; |
There was a problem hiding this comment.
Model strings can be provided without a provider prefix (e.g. "whisper-1", "text-embedding-3-large"), but allowedModels are typically stored as provider/model (as returned by /v1/models). Since policy enforcement runs before model normalization, restricted keys may be incorrectly rejected (or require clients to change model format). Consider normalizing to a canonical provider/model string (via getModelInfo) before calling enforceApiKeyPolicy, while still allowing combo names to pass through.
| // Enforce API key policies (model restrictions + budget limits) | ||
| const policy = await enforceApiKeyPolicy(request, body.model); | ||
| if (policy.rejection) return policy.rejection; | ||
|
|
There was a problem hiding this comment.
Policy enforcement runs before parsing/normalizing the embedding model. Because parseEmbeddingModel supports bare model IDs (no provider prefix), a restricted key with allowedModels like "openai/text-embedding-3-large" will be denied if the client sends "text-embedding-3-large". Consider parsing first and passing a normalized provider/model string into enforceApiKeyPolicy.
| // Enforce API key policies (model restrictions + budget limits) | ||
| const policy = await enforceApiKeyPolicy(request, body.model); | ||
| if (policy.rejection) return policy.rejection; |
There was a problem hiding this comment.
Policy enforcement runs before parsing/normalizing the rerank model. Since parseRerankModel supports bare model IDs, allowedModels entries stored as provider/model may not match when clients omit the provider prefix. Consider parsing first and passing a normalized provider/model string into enforceApiKeyPolicy.
| // Enforce API key policies (model restrictions + budget limits) | ||
| const policy = await enforceApiKeyPolicy(request, model as string); | ||
| if (policy.rejection) return policy.rejection; |
There was a problem hiding this comment.
Policy enforcement runs before parsing/normalizing the transcription model. parseTranscriptionModel supports bare model IDs; if allowedModels is stored as provider/model, restricted keys may be incorrectly denied when clients omit the provider prefix. Consider normalizing (parseTranscriptionModel -> provider/model) before calling enforceApiKeyPolicy.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a centralized API key policy enforcement mechanism, which is a significant improvement for ensuring consistent security and usage limits across endpoints. The new enforceApiKeyPolicy middleware is well-designed to handle model restrictions and budget checks, and it has been correctly integrated into the various /v1/* routes. My feedback focuses on enhancing the new middleware's robustness by adding logging for suppressed errors and improving type safety, which will aid in future maintenance and debugging.
| /** API key string (null if no key provided) */ | ||
| apiKey: string | null; | ||
| /** Metadata from DB (null if no key or key not found) */ | ||
| apiKeyInfo: any | null; |
There was a problem hiding this comment.
The apiKeyInfo property is typed as any. To leverage TypeScript's type safety and improve code clarity, consider defining a specific interface for the API key metadata (e.g., ApiKeyMetadata) and using it here. This would help prevent potential runtime errors and make the code easier to understand and maintain.
| try { | ||
| apiKeyInfo = await getApiKeyMetadata(apiKey); | ||
| } catch { | ||
| // If metadata fetch fails, don't block — degrade gracefully | ||
| return { apiKey, apiKeyInfo: null, rejection: null }; | ||
| } |
There was a problem hiding this comment.
The try...catch block for getApiKeyMetadata currently suppresses errors. While degrading gracefully is the correct behavior for the user, logging the error is crucial for maintainability. It would help you debug issues with the database or the metadata retrieval logic. You'll need to import the logger: import * as log from "@/sse/utils/logger";
| try { | |
| apiKeyInfo = await getApiKeyMetadata(apiKey); | |
| } catch { | |
| // If metadata fetch fails, don't block — degrade gracefully | |
| return { apiKey, apiKeyInfo: null, rejection: null }; | |
| } | |
| try { | |
| apiKeyInfo = await getApiKeyMetadata(apiKey); | |
| } catch (error) { | |
| // If metadata fetch fails, don't block — degrade gracefully but log it | |
| log.warn("API_POLICY", "Failed to fetch API key metadata. Policies will not be applied.", { error }); | |
| return { apiKey, apiKeyInfo: null, rejection: null }; | |
| } |
| } catch { | ||
| // Budget check is best-effort — don't block on errors | ||
| } |
There was a problem hiding this comment.
Similar to the metadata fetch, this catch block suppresses errors from the budget check. To help with debugging and monitoring, it's best to log these errors, even if the request is allowed to proceed. This requires importing the logger if you haven't already: import * as log from "@/sse/utils/logger";
} catch (error) {
// Budget check is best-effort — don't block on errors, but log them.
log.warn("API_POLICY", "Budget check failed. Request will be allowed.", { error });
}
|
this pull very nice |
…atch in usage.ts - Added ApiKeyMetadata interface to replace 'any' types in apiKeyPolicy.ts - Added error logging in catch blocks for getApiKeyMetadata() and checkBudget() - Fixed claude-sonnet-4-6-thinking → claude-sonnet-4-6 mismatch in usage.ts importantModels Follow-up fixes for merged PRs #131 and #128
Approved: Critical security fix for API key model restrictions. Minor improvements (error logging, type safety) will be applied in a follow-up commit.
…atch in usage.ts - Added ApiKeyMetadata interface to replace 'any' types in apiKeyPolicy.ts - Added error logging in catch blocks for getApiKeyMetadata() and checkBudget() - Fixed claude-sonnet-4-6-thinking → claude-sonnet-4-6 mismatch in usage.ts importantModels Follow-up fixes for merged PRs #131 and #128
- fast-uri ^3.1.3 (root + electron overrides) — GHSA host confusion via IDN (#131, #126, high) - hono ^4.12.27 (bump existing 4.12.25 override) — JSX context isolation / cx() XSS / v1 adapter req drop (#128/#129/#130, medium) - @hono/node-server ^2.0.5 — serve-static path traversal (#127, medium); major bump, MCP transport verified - body-parser ^2.3.0 — DoS on invalid limit (#125, low), via express 5 All four packages now clear in `npm audit`; lockfile-lint OK; vuln-ratchet advisory count reduced. Electron lockfile updated for the second fast-uri site.
- fast-uri ^3.1.3 (root + electron) — host confusion via IDN (#131/#126, high) - hono ^4.12.27 — JSX ctx isolation / cx() XSS / v1 adapter req drop (#128/#129/#130, medium) - @hono/node-server ^2.0.5 — serve-static path traversal (#127, medium); MCP uses only getRequestListener, not serve-static - body-parser ^2.3.0 — DoS on invalid limit (#125, low) Resolved: fast-uri 3.1.4, hono 4.12.31, @hono/node-server 2.0.11, body-parser 2.3.0. All clear in npm audit; lockfile-lint OK.
…osouzapw#8066) - fast-uri ^3.1.3 (root + electron overrides) — GHSA host confusion via IDN (diegosouzapw#131, diegosouzapw#126, high) - hono ^4.12.27 (bump existing 4.12.25 override) — JSX context isolation / cx() XSS / v1 adapter req drop (diegosouzapw#128/diegosouzapw#129/diegosouzapw#130, medium) - @hono/node-server ^2.0.5 — serve-static path traversal (diegosouzapw#127, medium); major bump, MCP transport verified - body-parser ^2.3.0 — DoS on invalid limit (diegosouzapw#125, low), via express 5 All four packages now clear in `npm audit`; lockfile-lint OK; vuln-ratchet advisory count reduced. Electron lockfile updated for the second fast-uri site.
…osouzapw#8067) - fast-uri ^3.1.3 (root + electron) — host confusion via IDN (diegosouzapw#131/diegosouzapw#126, high) - hono ^4.12.27 — JSX ctx isolation / cx() XSS / v1 adapter req drop (diegosouzapw#128/diegosouzapw#129/diegosouzapw#130, medium) - @hono/node-server ^2.0.5 — serve-static path traversal (diegosouzapw#127, medium); MCP uses only getRequestListener, not serve-static - body-parser ^2.3.0 — DoS on invalid limit (diegosouzapw#125, low) Resolved: fast-uri 3.1.4, hono 4.12.31, @hono/node-server 2.0.11, body-parser 2.3.0. All clear in npm audit; lockfile-lint OK.
…l-restriction Approved: Critical security fix for API key model restrictions. Minor improvements (error logging, type safety) will be applied in a follow-up commit.
…atch in usage.ts - Added ApiKeyMetadata interface to replace 'any' types in apiKeyPolicy.ts - Added error logging in catch blocks for getApiKeyMetadata() and checkBudget() - Fixed claude-sonnet-4-6-thinking → claude-sonnet-4-6 mismatch in usage.ts importantModels Follow-up fixes for merged PRs diegosouzapw#131 and diegosouzapw#128
…osouzapw#8067) - fast-uri ^3.1.3 (root + electron) — host confusion via IDN (diegosouzapw#131/diegosouzapw#126, high) - hono ^4.12.27 — JSX ctx isolation / cx() XSS / v1 adapter req drop (diegosouzapw#128/diegosouzapw#129/diegosouzapw#130, medium) - @hono/node-server ^2.0.5 — serve-static path traversal (diegosouzapw#127, medium); MCP uses only getRequestListener, not serve-static - body-parser ^2.3.0 — DoS on invalid limit (diegosouzapw#125, low) Resolved: fast-uri 3.1.4, hono 4.12.31, @hono/node-server 2.0.11, body-parser 2.3.0. All clear in npm audit; lockfile-lint OK.
…osouzapw#8066) - fast-uri ^3.1.3 (root + electron overrides) — GHSA host confusion via IDN (diegosouzapw#131, diegosouzapw#126, high) - hono ^4.12.27 (bump existing 4.12.25 override) — JSX context isolation / cx() XSS / v1 adapter req drop (diegosouzapw#128/diegosouzapw#129/diegosouzapw#130, medium) - @hono/node-server ^2.0.5 — serve-static path traversal (diegosouzapw#127, medium); major bump, MCP transport verified - body-parser ^2.3.0 — DoS on invalid limit (diegosouzapw#125, low), via express 5 All four packages now clear in `npm audit`; lockfile-lint OK; vuln-ratchet advisory count reduced. Electron lockfile updated for the second fast-uri site.
Summary
Fixes #130 —
isModelAllowedForKey()existed but was never called. API keys withallowedModelsrestrictions could access any model through any endpoint.Changes
New:
src/shared/utils/apiKeyPolicy.tsShared middleware that enforces two checks:
allowedModelsconfigured, verify the requested model is in the allowed list (supports exact match, prefix match likeopenai/*, and wildcard patterns)Usage in any endpoint:
Modified endpoints
src/sse/handlers/chat.tsenforceApiKeyPolicy()src/app/api/v1/embeddings/route.tsenforceApiKeyPolicy()src/app/api/v1/images/generations/route.tsenforceApiKeyPolicy()src/app/api/v1/audio/speech/route.tsenforceApiKeyPolicy()src/app/api/v1/audio/transcriptions/route.tsenforceApiKeyPolicy()src/app/api/v1/moderations/route.tsenforceApiKeyPolicy()src/app/api/v1/rerank/route.tsenforceApiKeyPolicy()src/app/api/v1/providers/[provider]/embeddings/route.tsenforceApiKeyPolicy()src/app/api/v1/providers/[provider]/images/generations/route.tsenforceApiKeyPolicy()Not modified (already covered)
/v1/responsesand/v1/providers/[provider]/chat/completions— these callhandleChat()which now includes the policy check/v1/chat/completions— same as aboveBehavior
allowedModels→ all models allowed (backward compatible)allowedModelsset → returns 403 if model not in listTesting
allowedModels: ["gpt-4o-mini"]claude-sonnet-4-20250514via any endpoint → 403 Forbiddengpt-4o-mini→ passes through normallyallowedModels→ all models work (no regression)