Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix model leak issue #5883

Merged
merged 4 commits into from
Dec 28, 2024
Merged

fix model leak issue #5883

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1a678cb
fix model leak issue
code-october Nov 29, 2024
e1ac053
add unit test
code-october Nov 30, 2024
54f6feb
update unit test
code-october Nov 30, 2024
cc5e16b
update unit test
code-october Nov 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/api/alibaba.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
import { prettyObject } from "@/app/utils/format";
import { NextRequest, NextResponse } from "next/server";
import { auth } from "@/app/api/auth";
import { isModelAvailableInServer } from "@/app/utils/model";
import { isModelNotavailableInServer } from "@/app/utils/model";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Remove unused isModelAvailableInServer function from model.ts

The old function is not being used anywhere in the codebase but still exists in app/utils/model.ts. This dead code should be removed to prevent confusion and potential future misuse.

  • app/utils/model.ts: Remove the unused isModelAvailableInServer function
🔗 Analysis chain

LGTM! Verify consistent usage across the codebase.

The import change from isModelAvailableInServer to isModelNotavailableInServer aligns with the PR objective to fix model leak issues.

Both old and new functions coexist in model.ts - needs cleanup

The verification revealed that while the new isModelNotavailableInServer function is consistently used across API handlers, the old isModelAvailableInServer function still exists in app/utils/model.ts. Let's verify if it's actually being used anywhere:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all files have been updated to use isModelNotavailableInServer
# and no occurrences of the old isModelAvailableInServer remain

echo "Checking for any remaining usage of old function name..."
rg "isModelAvailableInServer"

echo "Verifying consistent usage of new function..."
rg "isModelNotavailableInServer"

Length of output: 1674

Script:

#!/bin/bash
# Check if the old function is used anywhere in the codebase
rg -A 2 "isModelAvailableInServer\(" --type ts

Length of output: 192


const serverConfig = getServerSideConfig();

Expand Down Expand Up @@ -89,7 +89,7 @@ async function request(req: NextRequest) {

// not undefined and is false
if (
isModelAvailableInServer(
isModelNotavailableInServer(
serverConfig.customModels,
jsonBody?.model as string,
ServiceProvider.Alibaba as string,
Expand Down
4 changes: 2 additions & 2 deletions app/api/anthropic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
import { prettyObject } from "@/app/utils/format";
import { NextRequest, NextResponse } from "next/server";
import { auth } from "./auth";
import { isModelAvailableInServer } from "@/app/utils/model";
import { isModelNotavailableInServer } from "@/app/utils/model";
import { cloudflareAIGatewayUrl } from "@/app/utils/cloudflare";

const ALLOWD_PATH = new Set([Anthropic.ChatPath, Anthropic.ChatPath1]);
Expand Down Expand Up @@ -122,7 +122,7 @@ async function request(req: NextRequest) {

// not undefined and is false
if (
isModelAvailableInServer(
isModelNotavailableInServer(
serverConfig.customModels,
jsonBody?.model as string,
ServiceProvider.Anthropic as string,
Expand Down
4 changes: 2 additions & 2 deletions app/api/baidu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
import { prettyObject } from "@/app/utils/format";
import { NextRequest, NextResponse } from "next/server";
import { auth } from "@/app/api/auth";
import { isModelAvailableInServer } from "@/app/utils/model";
import { isModelNotavailableInServer } from "@/app/utils/model";
import { getAccessToken } from "@/app/utils/baidu";

const serverConfig = getServerSideConfig();
Expand Down Expand Up @@ -104,7 +104,7 @@ async function request(req: NextRequest) {

// not undefined and is false
if (
isModelAvailableInServer(
isModelNotavailableInServer(
serverConfig.customModels,
jsonBody?.model as string,
ServiceProvider.Baidu as string,
Expand Down
4 changes: 2 additions & 2 deletions app/api/bytedance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
import { prettyObject } from "@/app/utils/format";
import { NextRequest, NextResponse } from "next/server";
import { auth } from "@/app/api/auth";
import { isModelAvailableInServer } from "@/app/utils/model";
import { isModelNotavailableInServer } from "@/app/utils/model";

const serverConfig = getServerSideConfig();

Expand Down Expand Up @@ -88,7 +88,7 @@ async function request(req: NextRequest) {

// not undefined and is false
if (
isModelAvailableInServer(
isModelNotavailableInServer(
serverConfig.customModels,
jsonBody?.model as string,
ServiceProvider.ByteDance as string,
Expand Down
15 changes: 7 additions & 8 deletions app/api/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { NextRequest, NextResponse } from "next/server";
import { getServerSideConfig } from "../config/server";
import { OPENAI_BASE_URL, ServiceProvider } from "../constant";
import { cloudflareAIGatewayUrl } from "../utils/cloudflare";
import { getModelProvider, isModelAvailableInServer } from "../utils/model";
import { getModelProvider, isModelNotavailableInServer } from "../utils/model";

const serverConfig = getServerSideConfig();

Expand Down Expand Up @@ -118,15 +118,14 @@ export async function requestOpenai(req: NextRequest) {

// not undefined and is false
if (
isModelAvailableInServer(
isModelNotavailableInServer(
serverConfig.customModels,
jsonBody?.model as string,
ServiceProvider.OpenAI as string,
) ||
isModelAvailableInServer(
serverConfig.customModels,
jsonBody?.model as string,
ServiceProvider.Azure as string,
[
ServiceProvider.OpenAI,
ServiceProvider.Azure,
jsonBody?.model as string, // support provider-unspecified model
],
Comment on lines +121 to +128
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling and code clarity.

Several improvements can be made to this section:

  1. The comment "try to refuse gpt4 request" is outdated as this now handles all model restrictions
  2. The error message could be more informative
  3. The model parameter is cast to string unnecessarily at line 127

Apply these changes:

- // #1815 try to refuse gpt4 request
+ // Check if the requested model is restricted based on server configuration
  if (
    isModelNotavailableInServer(
      serverConfig.customModels,
      jsonBody?.model as string,
      [
        ServiceProvider.OpenAI,
        ServiceProvider.Azure,
-       jsonBody?.model as string,  // support provider-unspecified model
+       jsonBody?.model,  // support provider-unspecified model
      ],
    )
  ) {
    return NextResponse.json(
      {
        error: true,
-       message: `you are not allowed to use ${jsonBody?.model} model`,
+       message: `Access to model '${jsonBody?.model}' is restricted by server configuration`,
      },
      {
        status: 403,
      },
    );
  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
isModelNotavailableInServer(
serverConfig.customModels,
jsonBody?.model as string,
ServiceProvider.OpenAI as string,
) ||
isModelAvailableInServer(
serverConfig.customModels,
jsonBody?.model as string,
ServiceProvider.Azure as string,
[
ServiceProvider.OpenAI,
ServiceProvider.Azure,
jsonBody?.model as string, // support provider-unspecified model
],
isModelNotavailableInServer(
serverConfig.customModels,
jsonBody?.model as string,
[
ServiceProvider.OpenAI,
ServiceProvider.Azure,
jsonBody?.model, // support provider-unspecified model
],

)
) {
return NextResponse.json(
Expand Down
4 changes: 2 additions & 2 deletions app/api/glm.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
import { prettyObject } from "@/app/utils/format";
import { NextRequest, NextResponse } from "next/server";
import { auth } from "@/app/api/auth";
import { isModelAvailableInServer } from "@/app/utils/model";
import { isModelNotavailableInServer } from "@/app/utils/model";

const serverConfig = getServerSideConfig();

Expand Down Expand Up @@ -89,7 +89,7 @@ async function request(req: NextRequest) {

// not undefined and is false
if (
isModelAvailableInServer(
isModelNotavailableInServer(
serverConfig.customModels,
jsonBody?.model as string,
ServiceProvider.ChatGLM as string,
Expand Down
4 changes: 2 additions & 2 deletions app/api/iflytek.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
import { prettyObject } from "@/app/utils/format";
import { NextRequest, NextResponse } from "next/server";
import { auth } from "@/app/api/auth";
import { isModelAvailableInServer } from "@/app/utils/model";
import { isModelNotavailableInServer } from "@/app/utils/model";
// iflytek

const serverConfig = getServerSideConfig();
Expand Down Expand Up @@ -89,7 +89,7 @@ async function request(req: NextRequest) {

// not undefined and is false
if (
isModelAvailableInServer(
isModelNotavailableInServer(
serverConfig.customModels,
jsonBody?.model as string,
ServiceProvider.Iflytek as string,
Expand Down
4 changes: 2 additions & 2 deletions app/api/moonshot.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
import { prettyObject } from "@/app/utils/format";
import { NextRequest, NextResponse } from "next/server";
import { auth } from "@/app/api/auth";
import { isModelAvailableInServer } from "@/app/utils/model";
import { isModelNotavailableInServer } from "@/app/utils/model";

const serverConfig = getServerSideConfig();

Expand Down Expand Up @@ -88,7 +88,7 @@ async function request(req: NextRequest) {

// not undefined and is false
if (
isModelAvailableInServer(
isModelNotavailableInServer(
serverConfig.customModels,
jsonBody?.model as string,
ServiceProvider.Moonshot as string,
Expand Down
4 changes: 2 additions & 2 deletions app/api/xai.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
import { prettyObject } from "@/app/utils/format";
import { NextRequest, NextResponse } from "next/server";
import { auth } from "@/app/api/auth";
import { isModelAvailableInServer } from "@/app/utils/model";
import { isModelNotavailableInServer } from "@/app/utils/model";

const serverConfig = getServerSideConfig();

Expand Down Expand Up @@ -88,7 +88,7 @@ async function request(req: NextRequest) {

// not undefined and is false
if (
isModelAvailableInServer(
isModelNotavailableInServer(
serverConfig.customModels,
jsonBody?.model as string,
ServiceProvider.XAI as string,
Expand Down
24 changes: 24 additions & 0 deletions app/utils/model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,3 +202,27 @@ export function isModelAvailableInServer(
const modelTable = collectModelTable(DEFAULT_MODELS, customModels);
return modelTable[fullName]?.available === false;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical: Function has incorrect logic that caused the model leak

The isModelAvailableInServer function has inverted logic that likely caused the model leak issue:

  1. The function name suggests it checks if a model is available, but it returns true when the model is NOT available
  2. The condition available === false means it only returns true when explicitly marked as unavailable, allowing access to undefined or unspecified models

This function should be deprecated in favor of the new isModelNotavailableInServer function, as its behavior is counterintuitive and could lead to security issues.

/**
* Checks if a model is not available on any of the specified providers in the server.
*
* @param {string} customModels - A string of custom models, comma-separated.
* @param {string} modelName - The name of the model to check.
* @param {string|string[]} providerNames - A string or array of provider names to check against.
*
* @returns {boolean} True if the model is not available on any of the specified providers, false otherwise.
*/
export function isModelNotavailableInServer(
customModels: string,
modelName: string,
providerNames: string | string[],
) {
const modelTable = collectModelTable(DEFAULT_MODELS, customModels);
const providerNamesArray = Array.isArray(providerNames) ? providerNames : [providerNames];
for (const providerName of providerNamesArray){
const fullName = `${modelName}@${providerName.toLowerCase()}`;
if (modelTable[fullName]?.available === true)
return false;
}
return true;
}