fix(api-manager): show validation errors inline in modals, not behind backdrop (#1920)

Integrated into release/v3.7.9
This commit is contained in:
Andrew Munsell
2026-05-03 20:14:16 -07:00
committed by GitHub
parent 0c27b89c63
commit b73f3610fc
5 changed files with 384 additions and 38 deletions

View File

@@ -871,7 +871,12 @@ export async function getAccessToken(provider, credentials, log, proxyConfig: un
}
const entry = { promise: null, waiters: 0 };
entry.promise = _getAccessTokenWithStalenessCheck(provider, credentials, log, proxyConfig).finally(() => {
entry.promise = _getAccessTokenWithStalenessCheck(
provider,
credentials,
log,
proxyConfig
).finally(() => {
connectionRefreshMutex.delete(connectionId);
});
connectionRefreshMutex.set(connectionId, entry);

View File

@@ -107,7 +107,9 @@ export default function ApiManagerPageClient() {
const [editingKey, setEditingKey] = useState<ApiKey | null>(null);
const [showPermissionsModal, setShowPermissionsModal] = useState(false);
const [searchModel, setSearchModel] = useState("");
const [error, setError] = useState<string | null>(null);
const [pageError, setPageError] = useState<string | null>(null);
const [nameError, setNameError] = useState<string | null>(null);
const [createError, setCreateError] = useState<string | null>(null);
const [isSubmitting, setIsSubmitting] = useState(false);
const [usageStats, setUsageStats] = useState<Record<string, KeyUsageStats>>({});
const [sessionCounts, setSessionCounts] = useState<Record<string, number>>({});
@@ -226,19 +228,20 @@ export default function ApiManagerPageClient() {
}
};
const clearError = useCallback(() => setError(null), []);
const clearPageError = useCallback(() => setPageError(null), []);
const handleCreateKey = async () => {
// Validate raw input first, then sanitize
const validation = validateKeyName(newKeyName, t);
if (!validation.valid) {
setError(validation.error || t("invalidKeyName"));
setNameError(validation.error || t("invalidKeyName"));
return;
}
const sanitizedName = sanitizeInput(newKeyName);
setIsSubmitting(true);
clearError();
setNameError(null);
setCreateError(null);
try {
const res = await fetch("/api/keys", {
@@ -254,11 +257,11 @@ export default function ApiManagerPageClient() {
setNewKeyName("");
setShowAddModal(false);
} else {
setError(data.error || t("failedCreateKey"));
setCreateError(data.error || t("failedCreateKey"));
}
} catch (error) {
console.error("Error creating key:", error);
setError(t("failedCreateKeyRetry"));
setCreateError(t("failedCreateKeyRetry"));
} finally {
setIsSubmitting(false);
}
@@ -267,14 +270,14 @@ export default function ApiManagerPageClient() {
const handleDeleteKey = async (id: string) => {
// Validate ID format to prevent injection
if (!id || typeof id !== "string" || !/^[a-zA-Z0-9_-]+$/.test(id)) {
setError(t("invalidKeyId"));
setPageError(t("invalidKeyId"));
return;
}
if (!confirm(t("deleteConfirm"))) return;
setIsSubmitting(true);
clearError();
clearPageError();
try {
const res = await fetch(`/api/keys/${encodeURIComponent(id)}`, { method: "DELETE" });
@@ -282,11 +285,11 @@ export default function ApiManagerPageClient() {
setKeys((prev) => prev.filter((k) => k.id !== id));
} else {
const data = await res.json();
setError(data.error || t("failedDeleteKey"));
setPageError(data.error || t("failedDeleteKey"));
}
} catch (error) {
console.error("Error deleting key:", error);
setError(t("failedDeleteKeyRetry"));
setPageError(t("failedDeleteKeyRetry"));
} finally {
setIsSubmitting(false);
}
@@ -329,23 +332,15 @@ export default function ApiManagerPageClient() {
) => {
if (!editingKey || !editingKey.id) return;
// Validate raw input first, then sanitize
const nameValidation = validateKeyName(name, t);
if (!nameValidation.valid) {
setError(nameValidation.error || t("invalidKeyName"));
return;
}
const sanitizedName = sanitizeInput(name);
// Validate models array
if (!Array.isArray(allowedModels)) {
setError(t("invalidModelsSelection"));
return;
}
// Limit number of selected models to prevent abuse
if (allowedModels.length > MAX_SELECTED_MODELS) {
setError(t("cannotSelectMoreThanModels", { max: MAX_SELECTED_MODELS }));
return;
}
@@ -364,7 +359,7 @@ export default function ApiManagerPageClient() {
: 0;
setIsSubmitting(true);
clearError();
clearPageError();
try {
const res = await fetch(`/api/keys/${encodeURIComponent(editingKey.id)}`, {
@@ -388,11 +383,11 @@ export default function ApiManagerPageClient() {
setEditingKey(null);
} else {
const data = await res.json();
setError(data.error || t("failedUpdatePermissions"));
setPageError(data.error || t("failedUpdatePermissions"));
}
} catch (error) {
console.error("Error updating permissions:", error);
setError(t("failedUpdatePermissionsRetry"));
setPageError(t("failedUpdatePermissionsRetry"));
} finally {
setIsSubmitting(false);
}
@@ -441,12 +436,12 @@ export default function ApiManagerPageClient() {
return (
<div className="flex flex-col gap-8">
{/* Error Banner */}
{error && (
{pageError && (
<div className="flex items-center gap-3 p-4 bg-red-500/10 border border-red-500/30 rounded-lg">
<span className="material-symbols-outlined text-red-500">error</span>
<p className="text-sm text-red-700 dark:text-red-300 flex-1">{error}</p>
<p className="text-sm text-red-700 dark:text-red-300 flex-1">{pageError}</p>
<button
onClick={clearError}
onClick={clearPageError}
className="text-red-500 hover:text-red-700 transition-colors"
>
<span className="material-symbols-outlined">close</span>
@@ -520,7 +515,15 @@ export default function ApiManagerPageClient() {
<h2 className="text-lg font-semibold">{t("keyManagement")}</h2>
<p className="text-sm text-text-muted">{t("keyManagementDesc")}</p>
</div>
<Button icon="add" onClick={() => setShowAddModal(true)}>
<Button
icon="add"
onClick={() => {
setNameError(null);
setCreateError(null);
clearPageError();
setShowAddModal(true);
}}
>
{t("createKey")}
</Button>
</div>
@@ -554,7 +557,14 @@ export default function ApiManagerPageClient() {
</div>
<p className="text-text-main font-medium mb-2">{t("noKeys")}</p>
<p className="text-sm text-text-muted mb-4">{t("noKeysDesc")}</p>
<Button icon="add" onClick={() => setShowAddModal(true)}>
<Button
icon="add"
onClick={() => {
setNameError(null);
setCreateError(null);
setShowAddModal(true);
}}
>
{t("createFirstKey")}
</Button>
</div>
@@ -759,6 +769,8 @@ export default function ApiManagerPageClient() {
onClose={() => {
setShowAddModal(false);
setNewKeyName("");
setNameError(null);
setCreateError(null);
}}
>
<div className="flex flex-col gap-4">
@@ -768,25 +780,42 @@ export default function ApiManagerPageClient() {
</label>
<Input
value={newKeyName}
onChange={(e) => setNewKeyName(e.target.value)}
onChange={(e) => {
setNewKeyName(e.target.value);
setNameError(null);
}}
placeholder={t("keyNamePlaceholder")}
maxLength={MAX_KEY_NAME_LENGTH}
error={nameError}
autoFocus
/>
<p className="text-xs text-text-muted mt-1.5">{t("keyNameDesc")}</p>
</div>
{createError && (
<div className="flex items-center gap-2 px-3 py-2 rounded-lg bg-red-500/10 border border-red-500/30">
<span className="material-symbols-outlined text-red-500 text-sm">error</span>
<p className="text-sm text-red-700 dark:text-red-300 flex-1">{createError}</p>
</div>
)}
<div className="flex gap-2">
<Button
onClick={() => {
setShowAddModal(false);
setNewKeyName("");
setNameError(null);
setCreateError(null);
}}
variant="ghost"
fullWidth
>
{tc("cancel")}
</Button>
<Button onClick={handleCreateKey} fullWidth disabled={!newKeyName.trim()}>
<Button
onClick={handleCreateKey}
fullWidth
disabled={!newKeyName.trim()}
loading={isSubmitting}
>
{t("createKey")}
</Button>
</div>
@@ -905,6 +934,8 @@ const PermissionsModal = memo(function PermissionsModal({
const [scheduleTz, setScheduleTz] = useState(
apiKey?.accessSchedule?.tz ?? Intl.DateTimeFormat().resolvedOptions().timeZone
);
const [nameError, setNameError] = useState<string | null>(null);
const [saveError, setSaveError] = useState<string | null>(null);
const [selectedConnections, setSelectedConnections] = useState<string[]>(initialConnections);
const [allowAllConnections, setAllowAllConnections] = useState(initialConnections.length === 0);
const [expandedProviders, setExpandedProviders] = useState<Set<string>>(() => {
@@ -992,6 +1023,29 @@ const PermissionsModal = memo(function PermissionsModal({
);
const handleSave = useCallback(() => {
// Clear previous inline errors
setNameError(null);
setSaveError(null);
// Validate name inline before calling onSave
const validation = validateKeyName(keyName, t);
if (!validation.valid) {
setNameError(validation.error || t("invalidKeyName"));
return;
}
// Validate models selection
if (!allowAll && !Array.isArray(selectedModels)) {
setSaveError(t("invalidModelsSelection"));
return;
}
// Limit number of selected models to prevent abuse
if (!allowAll && selectedModels.length > MAX_SELECTED_MODELS) {
setSaveError(t("cannotSelectMoreThanModels", { max: MAX_SELECTED_MODELS }));
return;
}
const schedule: AccessSchedule | null = scheduleEnabled
? {
enabled: true,
@@ -1027,6 +1081,7 @@ const PermissionsModal = memo(function PermissionsModal({
scheduleUntil,
scheduleDays,
scheduleTz,
t,
]);
const selectedCount = selectedModels.length;
@@ -1048,13 +1103,25 @@ const PermissionsModal = memo(function PermissionsModal({
<div className="w-48 shrink-0">
<Input
value={keyName}
onChange={(e) => setKeyName(e.target.value)}
onChange={(e) => {
setKeyName(e.target.value);
setNameError(null);
}}
placeholder={t("keyNamePlaceholder")}
maxLength={MAX_KEY_NAME_LENGTH}
error={nameError}
/>
</div>
</div>
{/* Inline save error */}
{saveError && (
<div className="flex items-center gap-2 px-3 py-2 rounded-lg bg-red-500/10 border border-red-500/30">
<span className="material-symbols-outlined text-red-500 text-sm">error</span>
<p className="text-sm text-red-700 dark:text-red-300 flex-1">{saveError}</p>
</div>
)}
{/* Access Mode Toggle */}
<div className="flex gap-2 p-1 bg-surface rounded-lg">
<button

View File

@@ -777,10 +777,9 @@ export function runMigrations(db: Database.Database, options?: { isNewDb?: boole
// "duplicate column name" means the column already exists — end state achieved, mark applied.
if (message.includes("duplicate column name")) {
const applyMarkerOnly = db.transaction(() => {
db.prepare("INSERT OR IGNORE INTO _omniroute_migrations (version, name) VALUES (?, ?)").run(
migration.version,
migration.name
);
db.prepare(
"INSERT OR IGNORE INTO _omniroute_migrations (version, name) VALUES (?, ?)"
).run(migration.version, migration.name);
});
applyMarkerOnly();
count++;

View File

@@ -381,4 +381,263 @@ test.describe("API keys flow", () => {
await expect(permissionsDialog).not.toBeVisible({ timeout: UI_STABILITY_TIMEOUT_MS });
await expect(page.getByText("Renamed Key")).toBeVisible();
});
test("validation error appears inside the create key modal, not behind the backdrop", async ({
page,
}) => {
const state = {
keys: [] as ApiKeyRecord[],
nextId: 1,
};
// Stub all API routes
await page.route("**/v1/models", async (route) => {
await fulfillJson(route, { data: [] });
});
await page.route("**/api/connections", async (route) => {
await fulfillJson(route, {
connections: [
{ id: "conn-openai", name: "OpenAI Main", provider: "openai", isActive: true },
],
});
});
await page.route(/\/api\/usage\/call-logs(?:\?.*)?$/, async (route) => {
await fulfillJson(route, []);
});
await page.route("**/api/sessions", async (route) => {
await fulfillJson(route, { byApiKey: {} });
});
await page.route("**/api/keys", async (route) => {
const method = route.request().method();
if (method === "GET") {
await fulfillJson(route, {
keys: state.keys.map(({ fullKey, ...record }) => record),
allowKeyReveal: true,
});
return;
}
if (method === "POST") {
const payload = (route.request().postDataJSON() as { name?: string }) || {};
const id = `key-${state.nextId++}`;
const suffix = String(1000 + state.nextId);
const fullKey = `sk-live-${suffix}-demo-secret`;
const maskedKey = `sk-live-****${suffix}`;
state.keys.push({
id,
name: payload.name || "New Key",
key: maskedKey,
fullKey,
allowedModels: null,
allowedConnections: null,
createdAt: new Date().toISOString(),
});
await fulfillJson(route, { key: fullKey, id });
return;
}
await fulfillJson(route, { error: "Method not allowed" }, 405);
});
await page.route(/\/api\/keys\/[^/]+$/, async (route) => {
await fulfillJson(route, { error: "Not found" }, 404);
});
await page.route(/\/api\/keys\/[^/]+\/reveal$/, async (route) => {
await fulfillJson(route, { key: "" });
});
await gotoDashboardRoute(page, "/dashboard/api-manager", {
timeoutMs: NAVIGATION_TIMEOUT_MS,
});
await waitForPageToSettle(page);
await waitForNextDevCompileToFinish(page);
// Open the create key modal
const createFirstKeyButton = page.getByRole("button", {
name: /create (your )?first key/i,
});
await expect(createFirstKeyButton).toBeVisible({ timeout: UI_STABILITY_TIMEOUT_MS });
await createFirstKeyButton.click();
const createDialog = page.getByRole("dialog", { name: /create api key/i });
await expect(createDialog).toBeVisible({ timeout: UI_STABILITY_TIMEOUT_MS });
// Fill an invalid name (special characters) and submit
const nameInput = createDialog.locator("input").first();
await nameInput.fill("bad@name!");
const createKeyButton = createDialog.getByRole("button", { name: /create api key/i });
await createKeyButton.click({ force: true });
// The validation error must appear as an inline <p role="alert"> INSIDE the modal
const inlineError = createDialog.locator("p[role='alert']");
await expect(inlineError).toBeVisible({ timeout: UI_STABILITY_TIMEOUT_MS });
// The error must be visible — i.e. not obscured by the backdrop.
// A covered element has box visibility but fails the check below because
// pointer events and paint are blocked by the overlay div.
await expect(inlineError).toBeInViewport();
// No page-level error banner should appear behind the modal
const pageErrorBanner = page.locator('.flex.flex-col.gap-8 > [class~="bg-red-500/10"]');
await expect(pageErrorBanner).not.toBeVisible();
// Typing should clear the error
await nameInput.fill("valid-name");
await expect(inlineError).not.toBeVisible();
// A valid name should succeed without errors
await createKeyButton.click({ force: true });
await expect.poll(() => state.keys.length).toBe(1);
});
test("validation error appears inside the permissions modal when renaming, not behind the backdrop", async ({
page,
}) => {
const state: {
keys: ApiKeyRecord[];
nextId: number;
} = {
keys: [],
nextId: 1,
};
// Stub all API routes
await page.route("**/v1/models", async (route) => {
await fulfillJson(route, { data: [] });
});
await page.route("**/api/settings", async (route) => {
await fulfillJson(route, {});
});
await page.route("**/api/providers", async (route) => {
await fulfillJson(route, {
connections: [
{ id: "conn-openai", name: "OpenAI Main", provider: "openai", isActive: true },
],
});
});
await page.route(/\/api\/usage\/call-logs(?:\?.*)?$/, async (route) => {
await fulfillJson(route, []);
});
await page.route("**/api/sessions", async (route) => {
await fulfillJson(route, { byApiKey: {} });
});
await page.route(/\/api\/keys\/[^/]+$/, async (route) => {
if (route.request().method() === "PATCH") {
const keyId = route.request().url().split("/").pop() || "";
const payload = (await route.request().postDataJSON()) as { name?: string };
const record = state.keys.find((key) => key.id === keyId);
if (!record) {
await fulfillJson(route, { error: "Key not found" }, 404);
return;
}
if (payload.name) {
record.name = payload.name;
}
await fulfillJson(route, { message: "API key settings updated successfully", ...payload });
return;
}
await fulfillJson(route, { error: "Method not allowed" }, 405);
});
await page.route("**/api/keys", async (route) => {
const method = route.request().method();
if (method === "GET") {
await fulfillJson(route, {
keys: state.keys.map(({ fullKey, ...record }) => record),
allowKeyReveal: true,
});
return;
}
if (method === "POST") {
const payload = (route.request().postDataJSON() as { name?: string }) || {};
const id = `key-${state.nextId++}`;
const suffix = String(1000 + state.nextId);
const fullKey = `sk-live-${suffix}-demo-secret`;
const maskedKey = `sk-live-****${suffix}`;
state.keys.push({
id,
name: payload.name || "New Key",
key: maskedKey,
fullKey,
allowedModels: null,
allowedConnections: null,
createdAt: new Date("2026-04-05T20:00:00.000Z").toISOString(),
});
await fulfillJson(route, { key: fullKey, id });
return;
}
await fulfillJson(route, { error: "Method not allowed" }, 405);
});
await gotoDashboardRoute(page, "/dashboard/api-manager", {
timeoutMs: NAVIGATION_TIMEOUT_MS,
});
await waitForPageToSettle(page);
await waitForNextDevCompileToFinish(page);
// Create a key first
const createFirstKeyButton = page.getByRole("button", {
name: /create (your )?first key/i,
});
await expect(createFirstKeyButton).toBeVisible({ timeout: UI_STABILITY_TIMEOUT_MS });
await createFirstKeyButton.click();
const createDialog = page.getByRole("dialog", { name: /create api key/i });
await expect(createDialog).toBeVisible({ timeout: UI_STABILITY_TIMEOUT_MS });
await createDialog.locator("input").first().fill("Original Name");
await createDialog.getByRole("button", { name: /create api key/i }).click({ force: true });
// Dismiss the created dialog
const createdDialog = page.getByRole("dialog", { name: /api key created/i });
await createdDialog.getByRole("button", { name: /done/i }).click();
await waitForPageToSettle(page);
await waitForNextDevCompileToFinish(page);
await expect(page.getByText("Original Name")).toBeVisible({
timeout: UI_STABILITY_TIMEOUT_MS,
});
// Open the permissions modal
const keyRow = page
.locator("div")
.filter({ has: page.getByText("Original Name", { exact: true }) })
.first();
await keyRow.locator('button[title="Edit permissions"]').click({ force: true });
const permissionsDialog = page.getByRole("dialog", {
name: /permissions: original name/i,
});
await expect(permissionsDialog).toBeVisible({ timeout: UI_STABILITY_TIMEOUT_MS });
// --- Try to save with an invalid name (empty) ---
const nameInput = permissionsDialog.locator("input").first();
await nameInput.clear();
await nameInput.fill(" ");
const saveButton = permissionsDialog.getByRole("button", { name: /save permissions/i });
await saveButton.click({ force: true });
// Inline error should appear inside the permissions modal
const inlineError = permissionsDialog.locator('[id$="-error"]');
await expect(inlineError).toBeVisible({ timeout: UI_STABILITY_TIMEOUT_MS });
// No page-level error banner should appear behind the modal
const pageErrorBanner = page.locator('.flex.flex-col.gap-8 > [class~="bg-red-500/10"]');
await expect(pageErrorBanner).not.toBeVisible();
// Typing a valid name should clear the error
await nameInput.fill("Renamed Key");
await expect(inlineError).not.toBeVisible();
// Save should succeed now
await saveButton.click({ force: true });
await expect.poll(() => state.keys[0]?.name).toBe("Renamed Key");
await expect(permissionsDialog).not.toBeVisible({ timeout: UI_STABILITY_TIMEOUT_MS });
await expect(page.getByText("Renamed Key")).toBeVisible();
});
});

View File

@@ -1055,12 +1055,28 @@ test("getAccessToken per-connection mutex: different connections run independent
async () => {
const [groupA, groupB] = await Promise.all([
Promise.all([
getAccessToken("custom-oauth-conn-mutex", { connectionId: "conn-A", refreshToken: "rt-a" }, log),
getAccessToken("custom-oauth-conn-mutex", { connectionId: "conn-A", refreshToken: "rt-a" }, log),
getAccessToken(
"custom-oauth-conn-mutex",
{ connectionId: "conn-A", refreshToken: "rt-a" },
log
),
getAccessToken(
"custom-oauth-conn-mutex",
{ connectionId: "conn-A", refreshToken: "rt-a" },
log
),
]),
Promise.all([
getAccessToken("custom-oauth-conn-mutex", { connectionId: "conn-B", refreshToken: "rt-b" }, log),
getAccessToken("custom-oauth-conn-mutex", { connectionId: "conn-B", refreshToken: "rt-b" }, log),
getAccessToken(
"custom-oauth-conn-mutex",
{ connectionId: "conn-B", refreshToken: "rt-b" },
log
),
getAccessToken(
"custom-oauth-conn-mutex",
{ connectionId: "conn-B", refreshToken: "rt-b" },
log
),
]),
]);