Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge OverviewThe PR implements a configurable API key reveal toggle via the Changes Reviewed
Design Observations
ValidationThe PR includes comprehensive unit tests that verify:
Files Reviewed (8 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 the ALLOW_API_KEY_REVEAL environment variable to control whether full API keys are visible and copyable in the dashboard. The implementation includes a new utility for key masking, updates to API routes to conditionally reveal keys, and UI adjustments to reflect the security setting. Feedback was provided to improve the robustness of the key masking logic for short strings to avoid overlapping characters.
| if (typeof key !== "string") return null; | ||
| return key.slice(0, 8) + "****" + key.slice(-4); |
There was a problem hiding this comment.
The current key masking logic can produce incorrect results for keys shorter than 12 characters because the prefix and suffix slices can overlap. For example, a 10-character key 1234567890 would be masked as 12345678****7890, which is not the intended behavior.
To make this function more robust, we should handle short keys as a special case. It's also good practice to handle empty strings.
| if (typeof key !== "string") return null; | |
| return key.slice(0, 8) + "****" + key.slice(-4); | |
| if (typeof key !== "string" || !key) return null; | |
| if (key.length < 12) { | |
| return "****" + key.slice(-4); | |
| } | |
| return key.slice(0, 8) + "****" + key.slice(-4); |
There was a problem hiding this comment.
Pull request overview
Adds an opt-in environment flag (ALLOW_API_KEY_REVEAL) to allow the dashboard API Manager to display/copy stored API keys (default remains masked), plus route-level tests and documentation.
Changes:
- Introduce
src/lib/apiKeyExposure.tshelpers to interpret the env flag and present masked vs full key values. - Update
/api/keysand/api/keys/[id]responses to includeallowKeyRevealand return masked/full keys based on the flag. - Update the dashboard API Manager UI to show a copy button for existing keys when reveal is enabled; document the flag and add unit tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/api-key-visibility-route.test.mjs | New unit tests for masked vs revealed key behavior on list/detail routes. |
| src/lib/apiKeyExposure.ts | Adds env-flag parsing + key masking/presentation helpers. |
| src/app/api/keys/route.ts | Returns presented (masked/full) keys and exposes allowKeyReveal in response. |
| src/app/api/keys/[id]/route.ts | Mirrors list behavior for single-key fetch and includes allowKeyReveal. |
| src/app/api/cli-tools/codex-settings/route.ts | Comment update clarifying masked-vs-revealed dashboard behavior. |
| src/app/api/cli-tools/claude-settings/route.ts | Same comment update for Claude settings route. |
| src/app/(dashboard)/dashboard/api-manager/ApiManagerPageClient.tsx | Adds UI toggle behavior: copy existing keys when reveal is enabled. |
| docs/USER_GUIDE.md | Documents ALLOW_API_KEY_REVEAL. |
| .env.example | Adds ALLOW_API_KEY_REVEAL=false. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const keys = await getApiKeys(); | ||
| // Mask key values — users should never see full keys after creation | ||
| const maskedKeys = keys.map((k) => ({ | ||
| const allowKeyReveal = isApiKeyRevealEnabled(); | ||
| const presentedKeys = keys.map((k) => ({ | ||
| ...k, | ||
| key: typeof k.key === "string" ? k.key.slice(0, 8) + "****" + k.key.slice(-4) : null, | ||
| key: presentStoredApiKey(k.key), | ||
| })); | ||
| return NextResponse.json({ keys: maskedKeys }); | ||
| return NextResponse.json({ keys: presentedKeys, allowKeyReveal }); |
There was a problem hiding this comment.
When ALLOW_API_KEY_REVEAL is enabled this endpoint will return full stored API keys even in cases where the management API is effectively public (e.g. requireLogin=false makes /api/* skip auth in src/proxy.ts) and also to any request authenticated via Bearer API key. That creates a high-impact key exfiltration path (a single leaked/limited key can be used to retrieve all keys, or keys can be revealed without auth). Consider restricting key revealing to verified dashboard JWT sessions only (and/or only when auth is required) and otherwise always returning masked keys even if the env flag is set.
| const allowKeyReveal = isApiKeyRevealEnabled(); | ||
| const presentedKeys = keys.map((k) => ({ | ||
| ...k, | ||
| key: typeof k.key === "string" ? k.key.slice(0, 8) + "****" + k.key.slice(-4) : null, | ||
| key: presentStoredApiKey(k.key), | ||
| })); |
There was a problem hiding this comment.
allowKeyReveal is computed once but presentStoredApiKey() recomputes the env flag on every item. This is redundant work and can theoretically create inconsistencies if the env were mutated between calls. Consider passing allowKeyReveal into the presentation logic (e.g., choose between maskStoredApiKey and the raw key based on the already-computed boolean).
| const allowKeyReveal = isApiKeyRevealEnabled(); | ||
| return NextResponse.json({ | ||
| ...key, | ||
| key: keyValue ? keyValue.slice(0, 8) + "****" + keyValue.slice(-4) : null, | ||
| key: presentStoredApiKey(key.key), | ||
| allowKeyReveal, | ||
| }); |
There was a problem hiding this comment.
Same issue as the list endpoint: with ALLOW_API_KEY_REVEAL enabled this route will return the full stored key to any caller that reaches it, including scenarios where management auth is skipped (requireLogin=false) or where the caller is authenticated via Bearer API key. To reduce blast radius, consider only revealing when the request is authenticated via a verified dashboard JWT session (and/or only when auth is required), otherwise always mask.
| function maskKey(key) { | ||
| return key.slice(0, 8) + "****" + key.slice(-4); | ||
| } |
There was a problem hiding this comment.
The test helper maskKey() duplicates the production masking algorithm, which makes the test less effective (a bug in masking logic could be mirrored here and still pass). Prefer asserting the expected shape/properties (e.g., starts with the first 8 chars, ends with the last 4, contains "****", and differs from the original) without re-implementing the same function.
|
Closing this pull request in favor of a refreshed submission with a narrower key reveal scope. |
…7558) CodeQL js/stack-trace-exposure flags ANY error-derived value returned in the mock route bridge's 500 path, not just error.stack — swapping .stack for error.message (in #7354, alert #736) left sibling alert #737 open on the same line. Replace the body with a static string; the test only asserts status===200, so the 500 body is never inspected. Clears the last open CodeQL alert repo-wide, unblocking the Quality Ratchet on every PR.
…7559) CodeQL js/stack-trace-exposure flags ANY error-derived value returned in the mock route bridge's 500 path, not just error.stack — swapping .stack for error.message (in #7354, alert #736) left sibling alert #737 open on the same line. Replace the body with a static string; the test only asserts status===200, so the 500 body is never inspected. Clears the last open CodeQL alert repo-wide, unblocking the Quality Ratchet on every PR. Companion to the release/v3.8.49 PR (merge-gates §8 — gate/CI-touching fix lands on main in the same session).
…zapw#737) (diegosouzapw#7558) CodeQL js/stack-trace-exposure flags ANY error-derived value returned in the mock route bridge's 500 path, not just error.stack — swapping .stack for error.message (in diegosouzapw#7354, alert diegosouzapw#736) left sibling alert diegosouzapw#737 open on the same line. Replace the body with a static string; the test only asserts status===200, so the 500 body is never inspected. Clears the last open CodeQL alert repo-wide, unblocking the Quality Ratchet on every PR.
…zapw#737) (diegosouzapw#7559) CodeQL js/stack-trace-exposure flags ANY error-derived value returned in the mock route bridge's 500 path, not just error.stack — swapping .stack for error.message (in diegosouzapw#7354, alert diegosouzapw#736) left sibling alert diegosouzapw#737 open on the same line. Replace the body with a static string; the test only asserts status===200, so the 500 body is never inspected. Clears the last open CodeQL alert repo-wide, unblocking the Quality Ratchet on every PR. Companion to the release/v3.8.49 PR (merge-gates §8 — gate/CI-touching fix lands on main in the same session).
…zapw#737) (diegosouzapw#7559) CodeQL js/stack-trace-exposure flags ANY error-derived value returned in the mock route bridge's 500 path, not just error.stack — swapping .stack for error.message (in diegosouzapw#7354, alert diegosouzapw#736) left sibling alert diegosouzapw#737 open on the same line. Replace the body with a static string; the test only asserts status===200, so the 500 body is never inspected. Clears the last open CodeQL alert repo-wide, unblocking the Quality Ratchet on every PR. Companion to the release/v3.8.49 PR (merge-gates §8 — gate/CI-touching fix lands on main in the same session).
…zapw#737) (diegosouzapw#7558) CodeQL js/stack-trace-exposure flags ANY error-derived value returned in the mock route bridge's 500 path, not just error.stack — swapping .stack for error.message (in diegosouzapw#7354, alert diegosouzapw#736) left sibling alert diegosouzapw#737 open on the same line. Replace the body with a static string; the test only asserts status===200, so the 500 body is never inspected. Clears the last open CodeQL alert repo-wide, unblocking the Quality Ratchet on every PR.
Summary
ALLOW_API_KEY_REVEALenvironment flag that keeps API keys masked by default and only returns full key values when explicitly enabled/api/keysand/api/keys/[id]plus the Api Manager UI so trusted deployments can copy existing keys directly from the dashboardWhy
The database stores local API keys in retrievable form, but the dashboard API always masked them after creation. That made later copy/reuse impossible even in trusted self-hosted environments.
Impact
ALLOW_API_KEY_REVEAL=truewhen they want Api Manager to expose a one-click copy action for existing keysValidation
node --import tsx/esm --test tests/unit/api-key-visibility-route.test.mjsnpm run typecheck:coregit diff --checknpm run test:unitsuccessfully