Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature to restrict API keys to discovered and public models via a 'disableNonPublicModels' toggle. The changes span the UI, API routes, database schemas, migrations, and policy enforcement logic. The review feedback highlights critical issues in the model access check within 'src/lib/db/apiKeys.ts', specifically regarding the blocking of virtual auto-routing combos ('auto/*'), potential false negatives for model IDs without slashes, the use of 'console.error' for standard logging, and inconsistent indentation.
| // Check disableNonPublicModels flag | ||
| if (disableNonPublicModels) { | ||
| const { resolveModelAlias } = await import("@omniroute/open-sse/services/modelDeprecation.ts"); | ||
| const { getSyncedAvailableModelsByConnection, getCustomModels, getModelIsHidden } = await import("@/lib/db/models"); | ||
|
|
||
| const resolvedModelId = resolveModelAlias(modelId); | ||
| const effectiveModelId = resolvedModelId || modelId; | ||
|
|
||
| console.error(`[Model Access] Checking non-public model access for key ${metadata.id} and model ${modelId} (resolved to ${effectiveModelId})`); | ||
|
|
||
| const providerId = effectiveModelId.split("/")[0]; | ||
| const shortModelId = effectiveModelId.split("/").slice(1).join("/"); | ||
| const syncedModelsByConnection = await getSyncedAvailableModelsByConnection(providerId); | ||
| const customModels = await getCustomModels(providerId); | ||
|
|
||
| // Combine synced and custom models | ||
| const allDiscoveredModels = Object.values(syncedModelsByConnection).flat().concat(customModels); | ||
| const discovered = allDiscoveredModels.some((m) => m.id === shortModelId); | ||
| if (!discovered) return false; | ||
|
|
||
| const isPublic = !getModelIsHidden(providerId, shortModelId); | ||
| console.error(`[Model Access] Model ${effectiveModelId} is public: ${isPublic}`); | ||
| if (!isPublic) return false; | ||
| } |
There was a problem hiding this comment.
Issues Identified:
- Virtual Combos (
auto/*): As noted in your reviewer notes, virtual auto-routing combos (likeauto/coding) are currently blocked because they are not real models. We should bypass this check for any model ID starting withauto/. - Robustness (No-Slash Model IDs): If
effectiveModelIddoes not contain a slash (e.g., custom models or local models requested directly),split('/')will result inshortModelId = ""and cause a false negative (denying access). We should handle the no-slash case gracefully. - Log Pollution:
console.erroris used for standard tracing/info logs. This pollutes error monitoring channels and should be changed toconsole.log. - Indentation & Style: The added block has inconsistent indentation (4 spaces instead of the file's standard 2 spaces), and the comment on line 1297 has 0 indentation.
// Check disableNonPublicModels flag
if (disableNonPublicModels && !modelId.startsWith("auto/")) {
const { resolveModelAlias } = await import("@omniroute/open-sse/services/modelDeprecation.ts");
const { getSyncedAvailableModelsByConnection, getCustomModels, getModelIsHidden } = await import("@/lib/db/models");
const resolvedModelId = resolveModelAlias(modelId);
const effectiveModelId = resolvedModelId || modelId;
console.log("[Model Access] Checking non-public model access for key " + metadata.id + " and model " + modelId + " (resolved to " + effectiveModelId + ")");
const slashIndex = effectiveModelId.indexOf("/");
const providerId = slashIndex !== -1 ? effectiveModelId.slice(0, slashIndex) : "";
const shortModelId = slashIndex !== -1 ? effectiveModelId.slice(slashIndex + 1) : effectiveModelId;
const syncedModelsByConnection = await getSyncedAvailableModelsByConnection(providerId);
const customModels = await getCustomModels(providerId);
// Combine synced and custom models
const allDiscoveredModels = Object.values(syncedModelsByConnection).flat().concat(customModels);
const discovered = allDiscoveredModels.some((m) => m.id === shortModelId);
if (!discovered) return false;
const isPublic = !getModelIsHidden(providerId, shortModelId);
console.log("[Model Access] Model " + effectiveModelId + " is public: " + isPublic);
if (!isPublic) return false;
}There was a problem hiding this comment.
I've fixed 3 and 4.
Combo aren't supposed to call this code from what I understand.
Accepting help for 2.
|
@diegosouzapw When you have time, I'm accepting any advice before continuing work here |
|
Obrigado pela contribuição! Infelizmente esta PR está com conflitos estruturais severos na interface (ApiManagerPageClient.tsx) devido a alterações recentes na branch principal. Para que possamos integrá-la, por favor faça um rebase atualizando sua branch com a |
|
I already rebased against main. I can wait for 3.8.8 changes to be emerged in main. |
…els (#3041) Integrates two community contributions into release/v3.8.8 with security hardening and conflict resolution. - **Plugins framework** (#2913 — thanks @oyi77): hooks + registry unification, plugin SDK (`definePlugin`), worker-thread sandbox, per-plugin hook rate limiting, SHA-256 integrity verification, semver-gated upgrade, and execution analytics. Plugin routes are loopback-only (`isLocalOnlyPath`); `child_process` exec is opt-in via `OMNIROUTE_PLUGINS_ALLOW_EXEC` (default off). - **API key option: disable non-published models** (#3017 — thanks @androw): a per-key flag restricting the key to discovered public models (combos / `auto/*` / `qtSd/*` routing still allowed). Hardening applied during integration: migration renumber (089/090/091), `/api/plugins` LOCAL_ONLY route-guard classification (closes the plugin-RCE vector), atomic install/upgrade with path containment, `O_EXCL` tmp-file creation (TOCTOU), rate-limit-map eviction, `validatePluginConfig` on configure, `buildErrorBody` on all plugin error paths. 246/246 tests; typecheck / cycles / docs-sync clean. Co-authored-by: oyi77 <14921983+oyi77@users.noreply.github.com> Co-authored-by: Nicolas Lorin <androw95220@gmail.com>
|
Hi @androw — thank you for this! 🙏 Your "disable non-published models" per-API-key option has been integrated into Because it shipped through the combined integration branch rather than a direct merge of this PR, I'm closing this one — but you're credited in the CHANGELOG ( |
…els (cheaper-inference#3041) Integrates two community contributions into release/v3.8.8 with security hardening and conflict resolution. - **Plugins framework** (cheaper-inference#2913 — thanks @oyi77): hooks + registry unification, plugin SDK (`definePlugin`), worker-thread sandbox, per-plugin hook rate limiting, SHA-256 integrity verification, semver-gated upgrade, and execution analytics. Plugin routes are loopback-only (`isLocalOnlyPath`); `child_process` exec is opt-in via `OMNIROUTE_PLUGINS_ALLOW_EXEC` (default off). - **API key option: disable non-published models** (cheaper-inference#3017 — thanks @androw): a per-key flag restricting the key to discovered public models (combos / `auto/*` / `qtSd/*` routing still allowed). Hardening applied during integration: migration renumber (089/090/091), `/api/plugins` LOCAL_ONLY route-guard classification (closes the plugin-RCE vector), atomic install/upgrade with path containment, `O_EXCL` tmp-file creation (TOCTOU), rate-limit-map eviction, `validatePluginConfig` on configure, `buildErrorBody` on all plugin error paths. 246/246 tests; typecheck / cycles / docs-sync clean. Co-authored-by: oyi77 <14921983+oyi77@users.noreply.github.com> Co-authored-by: Nicolas Lorin <androw95220@gmail.com>
…els (cheaper-inference#3041) Integrates two community contributions into release/v3.8.8 with security hardening and conflict resolution. - **Plugins framework** (cheaper-inference#2913 — thanks @oyi77): hooks + registry unification, plugin SDK (`definePlugin`), worker-thread sandbox, per-plugin hook rate limiting, SHA-256 integrity verification, semver-gated upgrade, and execution analytics. Plugin routes are loopback-only (`isLocalOnlyPath`); `child_process` exec is opt-in via `OMNIROUTE_PLUGINS_ALLOW_EXEC` (default off). - **API key option: disable non-published models** (cheaper-inference#3017 — thanks @androw): a per-key flag restricting the key to discovered public models (combos / `auto/*` / `qtSd/*` routing still allowed). Hardening applied during integration: migration renumber (089/090/091), `/api/plugins` LOCAL_ONLY route-guard classification (closes the plugin-RCE vector), atomic install/upgrade with path containment, `O_EXCL` tmp-file creation (TOCTOU), rate-limit-map eviction, `validatePluginConfig` on configure, `buildErrorBody` on all plugin error paths. 246/246 tests; typecheck / cycles / docs-sync clean. Co-authored-by: oyi77 <14921983+oyi77@users.noreply.github.com> Co-authored-by: Nicolas Lorin <androw95220@gmail.com>
…els (cheaper-inference#3041) Integrates two community contributions into release/v3.8.8 with security hardening and conflict resolution. - **Plugins framework** (cheaper-inference#2913 — thanks @oyi77): hooks + registry unification, plugin SDK (`definePlugin`), worker-thread sandbox, per-plugin hook rate limiting, SHA-256 integrity verification, semver-gated upgrade, and execution analytics. Plugin routes are loopback-only (`isLocalOnlyPath`); `child_process` exec is opt-in via `OMNIROUTE_PLUGINS_ALLOW_EXEC` (default off). - **API key option: disable non-published models** (cheaper-inference#3017 — thanks @androw): a per-key flag restricting the key to discovered public models (combos / `auto/*` / `qtSd/*` routing still allowed). Hardening applied during integration: migration renumber (089/090/091), `/api/plugins` LOCAL_ONLY route-guard classification (closes the plugin-RCE vector), atomic install/upgrade with path containment, `O_EXCL` tmp-file creation (TOCTOU), rate-limit-map eviction, `validatePluginConfig` on configure, `buildErrorBody` on all plugin error paths. 246/246 tests; typecheck / cycles / docs-sync clean. Co-authored-by: oyi77 <14921983+oyi77@users.noreply.github.com> Co-authored-by: Nicolas Lorin <androw95220@gmail.com>
Summary
Related Issues
Validation
npm run lintnpm run test:unitnpm run test:coverage>= 60%for statements, lines, functions, and branchesTests Added Or Updated
Coverage Notes
src/,open-sse/,electron/, orbin/, explain which tests cover the change.Coverage before:
Coverage after:
Reviewer Notes