Repository navigation
[codex] Add scoped API key copy flow for Api Manager - #740
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| import { isApiKeyRevealEnabled } from "@/lib/apiKeyExposure"; | ||
|
|
||
| // GET /api/keys/[id]/reveal - Reveal full API key for explicit copy actions | ||
| export async function GET(_request, { params }) { |
There was a problem hiding this comment.
CRITICAL: Missing authentication check — The route exposes full API keys without verifying the request is from an authenticated user.
Other management routes (e.g., /api/db-backups/export/route.ts) use this pattern:
if (await isAuthRequired()) {
if (!(await isAuthenticated(request))) {
return NextResponse.json({ error: "Unauthorized" }, { status: 401 });
}
}Without this check, anyone who can access the dashboard (or if REQUIRE_API_KEY=false, anyone on the network) can retrieve full API keys by calling /api/keys/[id]/reveal.
| if (!keyId) return; | ||
|
|
||
| try { | ||
| const res = await fetch(`/api/keys/${encodeURIComponent(keyId)}/reveal`); |
There was a problem hiding this comment.
WARNING: No user feedback on failure — When the reveal request fails (e.g., network error, server error), the user is not informed. The error is only logged to console.
Consider adding error state to inform users why the copy operation failed.
Code Review SummaryStatus: 1 Critical Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (6 files)
|
![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 feature to reveal and copy full API keys, managed by the ALLOW_API_KEY_REVEAL environment variable. It includes a new API endpoint, UI updates for the reveal action, and utility functions for key masking. Review feedback suggests enhancing user-facing error handling in the dashboard and simplifying the handling of route parameters by removing unnecessary await calls in the API route and unit tests.
| const handleCopyExistingKey = async (keyId: string) => { | ||
| if (!keyId) return; | ||
|
|
||
| try { | ||
| const res = await fetch(`/api/keys/${encodeURIComponent(keyId)}/reveal`); | ||
| if (!res.ok) { | ||
| console.log("Error revealing key:", await res.text()); | ||
| return; | ||
| } | ||
|
|
||
| const data = await res.json(); | ||
| if (typeof data?.key === "string") { | ||
| await copy(data.key, `existing_key_${keyId}`); | ||
| } | ||
| } catch (error) { | ||
| console.log("Error copying existing key:", error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The current implementation of handleCopyExistingKey logs errors to the console, which is not visible to the user. For a better user experience, it's recommended to use the existing setError state to display a user-facing error message in the UI, similar to how other actions in this component are handled. This provides clear feedback to the user if the copy action fails. You may need to add new translation keys like failedToRevealKey and failedToCopyKey.
const handleCopyExistingKey = async (keyId: string) => {
if (!keyId) return;
clearError();
try {
const res = await fetch(`/api/keys/${encodeURIComponent(keyId)}/reveal`);
if (!res.ok) {
const data = await res.json().catch(() => ({ error: t("failedToRevealKey") }));
setError(data.error || t("failedToRevealKey"));
return;
}
const data = await res.json();
if (typeof data?.key === "string") {
await copy(data.key, `existing_key_${keyId}`);
}
} catch (error) {
console.error("Error copying existing key:", error);
setError(t("failedToCopyKey"));
}
};
| return NextResponse.json({ error: "API key reveal is disabled" }, { status: 403 }); | ||
| } | ||
|
|
||
| const { id } = await params; |
There was a problem hiding this comment.
In Next.js App Router route handlers, the params object is not a promise. The await here is unnecessary and likely a leftover from a testing setup. It should be removed for code clarity and correctness in a production environment.
| const { id } = await params; | |
| const { id } = params; |
| const response = await revealRoute.GET(request, { | ||
| params: Promise.resolve({ id: created.id }), | ||
| }); |
There was a problem hiding this comment.
The params object passed to the route handler should be a plain object, not a promise. The route handler in Next.js receives { params } directly. Wrapping it in Promise.resolve in the test forces an unnecessary await in the production code. This should be corrected here and in the other test case.
const response = await revealRoute.GET(request, {
params: { id: created.id },
});| const response = await revealRoute.GET(request, { | ||
| params: Promise.resolve({ id: created.id }), | ||
| }); |
There was a problem hiding this comment.
Similar to the other test case, the params object should be a plain object, not a promise, to accurately reflect the Next.js route handler signature.
const response = await revealRoute.GET(request, {
params: { id: created.id },
});There was a problem hiding this comment.
Pull request overview
Adds an opt-in “reveal + copy existing API key” flow for the dashboard Api Manager while keeping the general /api/keys listing masked by default.
Changes:
- Introduces
ALLOW_API_KEY_REVEALflag plus shared helpers for reveal enablement and key masking. - Extends
GET /api/keysto keep keys masked while returningallowKeyRevealto drive UI behavior. - Adds
GET /api/keys/[id]/revealand updates Api Manager UI + docs/tests to support on-demand copy.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/api-key-reveal-route.test.mjs | Adds unit coverage for masked list behavior and the new reveal endpoint gating. |
| src/lib/apiKeyExposure.ts | Centralizes reveal-flag parsing and key masking logic. |
| src/app/api/keys/route.ts | Uses shared masking helper and returns allowKeyReveal flag. |
| src/app/api/keys/[id]/reveal/route.ts | New endpoint to return full key by id when reveal is enabled. |
| src/app/(dashboard)/dashboard/api-manager/ApiManagerPageClient.tsx | Adds UI state/behavior to show copy action and call reveal endpoint when enabled. |
| docs/USER_GUIDE.md | Documents ALLOW_API_KEY_REVEAL. |
| .env.example | Adds ALLOW_API_KEY_REVEAL example entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| export function maskStoredApiKey(key: unknown): string | null { | ||
| if (typeof key !== "string") return null; | ||
| return key.slice(0, 8) + "****" + key.slice(-4); |
There was a problem hiding this comment.
maskStoredApiKey can fully disclose old/short API keys because the prefix (8) and suffix (4) slices overlap when key.length <= 12 (e.g., legacy format sk-{random8}), making the “masked” string reconstructable. Consider guarding on length and masking more aggressively for short keys (or choosing prefix/suffix lengths that never overlap).
| return key.slice(0, 8) + "****" + key.slice(-4); | |
| const str = key as string; | |
| const len = str.length; | |
| // For empty or very short keys, mask aggressively to avoid leaking the full value. | |
| if (len === 0) { | |
| return null; | |
| } | |
| if (len <= 4) { | |
| return "****"; | |
| } | |
| // For short keys, ensure prefix and suffix do not cover the entire string. | |
| if (len <= 12) { | |
| // Reveal only first and last character, mask everything in between. | |
| return str[0] + "****" + str.slice(-1); | |
| } | |
| // For longer keys, keep existing behavior: show first 8 and last 4 characters. | |
| return str.slice(0, 8) + "****" + str.slice(-4); |
| // GET /api/keys/[id]/reveal - Reveal full API key for explicit copy actions | ||
| export async function GET(_request, { params }) { | ||
| try { | ||
| if (!isApiKeyRevealEnabled()) { | ||
| return NextResponse.json({ error: "API key reveal is disabled" }, { status: 403 }); | ||
| } | ||
|
|
||
| const { id } = await params; | ||
| const key = await getApiKeyById(id); |
There was a problem hiding this comment.
The reveal handler doesn’t enforce management-route authentication. In production this may be covered by proxy middleware, but unit tests and any direct invocation will bypass that guard, and this endpoint returns a full secret. Consider calling requireManagementAuth(request) (or isAuthRequired + isAuthenticated) early and returning 401 when auth is required but missing.
| return NextResponse.json({ error: "Key not found" }, { status: 404 }); | ||
| } | ||
|
|
||
| return NextResponse.json({ key: key.key }); |
There was a problem hiding this comment.
This endpoint returns a sensitive secret but doesn’t set any cache-prevention headers. Add Cache-Control: no-store (and optionally Pragma: no-cache) on the 200 response to reduce the risk of intermediary/browser caching of the revealed API key.
| return NextResponse.json({ key: key.key }); | |
| return NextResponse.json( | |
| { key: key.key }, | |
| { | |
| headers: { | |
| "Cache-Control": "no-store", | |
| "Pragma": "no-cache", | |
| }, | |
| }, | |
| ); |
|
|
||
| // GET /api/keys/[id]/reveal - Reveal full API key for explicit copy actions | ||
| export async function GET(_request, { params }) { | ||
| try { | ||
| if (!isApiKeyRevealEnabled()) { | ||
| return NextResponse.json({ error: "API key reveal is disabled" }, { status: 403 }); | ||
| } | ||
|
|
||
| const { id } = await params; | ||
| const key = await getApiKeyById(id); | ||
|
|
||
| if (!key || typeof key.key !== "string") { | ||
| return NextResponse.json({ error: "Key not found" }, { status: 404 }); | ||
| } | ||
|
|
||
| return NextResponse.json({ key: key.key }); | ||
| } catch (error) { | ||
| console.log("Error revealing key:", error); | ||
| return NextResponse.json({ error: "Failed to reveal key" }, { status: 500 }); |
There was a problem hiding this comment.
This new management API route uses ad-hoc NextResponse.json({ error: "..." }) payloads. The repo has canonical helpers in src/shared/utils/apiResponse.ts intended for management routes (structured { error: { code, message, ... } }). Using those helpers here would keep client error handling consistent.
| // GET /api/keys/[id]/reveal - Reveal full API key for explicit copy actions | |
| export async function GET(_request, { params }) { | |
| try { | |
| if (!isApiKeyRevealEnabled()) { | |
| return NextResponse.json({ error: "API key reveal is disabled" }, { status: 403 }); | |
| } | |
| const { id } = await params; | |
| const key = await getApiKeyById(id); | |
| if (!key || typeof key.key !== "string") { | |
| return NextResponse.json({ error: "Key not found" }, { status: 404 }); | |
| } | |
| return NextResponse.json({ key: key.key }); | |
| } catch (error) { | |
| console.log("Error revealing key:", error); | |
| return NextResponse.json({ error: "Failed to reveal key" }, { status: 500 }); | |
| import { errorResponse, successResponse } from "@/shared/utils/apiResponse"; | |
| // GET /api/keys/[id]/reveal - Reveal full API key for explicit copy actions | |
| export async function GET(_request, { params }) { | |
| try { | |
| if (!isApiKeyRevealEnabled()) { | |
| return errorResponse( | |
| 403, | |
| "API_KEY_REVEAL_DISABLED", | |
| "API key reveal is disabled", | |
| ); | |
| } | |
| const { id } = await params; | |
| const key = await getApiKeyById(id); | |
| if (!key || typeof key.key !== "string") { | |
| return errorResponse(404, "API_KEY_NOT_FOUND", "Key not found"); | |
| } | |
| return successResponse({ key: key.key }); | |
| } catch (error) { | |
| console.log("Error revealing key:", error); | |
| return errorResponse(500, "API_KEY_REVEAL_FAILED", "Failed to reveal key"); |
|
Thanks @rdself for this great contribution! 🎉 The feature is now merged and will be part of the next release. We appreciate your effort! |
…uzapw#740/diegosouzapw#741) (diegosouzapw#7628) Validated in local merge-train @ 8f27177d1 (full parity suite green: typecheck+file-size+complexity+cognitive+changelog+unit shards 1&2+vitest)
…uzapw#740/diegosouzapw#741) (diegosouzapw#7628) Validated in local merge-train @ 8f27177d1 (full parity suite green: typecheck+file-size+complexity+cognitive+changelog+unit shards 1&2+vitest)
Summary
ALLOW_API_KEY_REVEALenvironment flag that is disabled by default/api/keysmasked for all dashboard consumers while exposing reveal capability only through an explicit/api/keys/[id]/revealrouteWhy
Administrators sometimes need to copy an existing local API key again after creation, but the dashboard currently only shows the full value once. This change enables an intentional, opt-in copy flow without turning the shared key list endpoint into a general full-key response.
Impact
ALLOW_API_KEY_REVEAL=trueenables one-click copy from Api Manager/api/keyscontinue to receive masked key valuesValidation
node --import tsx/esm --test tests/unit/api-key-reveal-route.test.mjsnpm run typecheck:coregit diff --checknpm run test:unit