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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix model leak issue #5883

Show file tree
Hide file tree
Changes from all commits
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;
}
59 changes: 59 additions & 0 deletions test/model-available.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import { isModelNotavailableInServer } from "../app/utils/model";

describe("isModelNotavailableInServer", () => {
test("test model will return false, which means the model is available", () => {
const customModels = "";
const modelName = "gpt-4";
const providerNames = "OpenAI";
const result = isModelNotavailableInServer(customModels, modelName, providerNames);
expect(result).toBe(false);
});

test("test model will return true when model is not available in custom models", () => {
const customModels = "-all,gpt-4o-mini";
const modelName = "gpt-4";
const providerNames = "OpenAI";
const result = isModelNotavailableInServer(customModels, modelName, providerNames);
expect(result).toBe(true);
});

test("should respect DISABLE_GPT4 setting", () => {
process.env.DISABLE_GPT4 = "1";
const result = isModelNotavailableInServer("", "gpt-4", "OpenAI");
expect(result).toBe(true);
});

test("should handle empty provider names", () => {
const result = isModelNotavailableInServer("-all,gpt-4", "gpt-4", "");
expect(result).toBe(true);
});

test("should be case insensitive for model names", () => {
const result = isModelNotavailableInServer("-all,GPT-4", "gpt-4", "OpenAI");
expect(result).toBe(true);
});

test("support passing multiple providers, model unavailable on one of the providers will return true", () => {
const customModels = "-all,gpt-4@Google";
const modelName = "gpt-4";
const providerNames = ["OpenAI", "Azure"];
const result = isModelNotavailableInServer(customModels, modelName, providerNames);
expect(result).toBe(true);
});

test("support passing multiple providers, model available on one of the providers will return false", () => {
const customModels = "-all,gpt-4@Google";
const modelName = "gpt-4";
const providerNames = ["OpenAI", "Google"];
const result = isModelNotavailableInServer(customModels, modelName, providerNames);
expect(result).toBe(false);
});

test("test custom model without setting provider", () => {
const customModels = "-all,mistral-large";
const modelName = "mistral-large";
const providerNames = modelName;
const result = isModelNotavailableInServer(customModels, modelName, providerNames);
expect(result).toBe(false);
});
Comment on lines +52 to +58
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Review provider name assignment in custom model test

Setting providerNames to modelName seems incorrect and might hide potential issues. Consider using a specific provider name or an array of providers to match the real-world usage.

-        const providerNames = modelName;
+        const providerNames = "Default";  // or ["Default"] to match other test patterns
📝 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
test("test custom model without setting provider", () => {
const customModels = "-all,mistral-large";
const modelName = "mistral-large";
const providerNames = modelName;
const result = isModelNotavailableInServer(customModels, modelName, providerNames);
expect(result).toBe(false);
});
test("test custom model without setting provider", () => {
const customModels = "-all,mistral-large";
const modelName = "mistral-large";
const providerNames = "Default"; // or ["Default"] to match other test patterns
const result = isModelNotavailableInServer(customModels, modelName, providerNames);
expect(result).toBe(false);
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

provider 设计问题，default 是无效参数

Copy link
Contributor

Choose a reason for hiding this comment

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

@code-october，明白了，谢谢您的解释。确实，Default 不是有效参数，保留原来的 providerNames = modelName 设置是合理的。感谢您的澄清！

✏️ Learnings added 
Learnt from: code-october
PR: ChatGPTNextWeb/ChatGPT-Next-Web#5883
File: test/model-available.test.ts:46-52
Timestamp: 2024-11-30T07:30:13.651Z
Learning: 在测试文件中，对于没有指定 provider 的模型，`providerNames` 应该设置为 `modelName`，`"Default"` 不是有效的 provider 参数，应避免使用。

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

})