Skip to content

refactor: add client parameter handling to prevent sending sensitive config in request body #7875

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

Merged
wxg0103 merged 2 commits into from
Nov 12, 2026
Merged

refactor: add client parameter handling to prevent sending sensitive config in request body #7875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
85049ca
refactor: update string filtering to be case-insensitive for improved…
wxg0103 Nov 12, 2026
90a14ef
refactor: add client parameter handling to prevent sending sensitive …
wxg0103 Nov 12, 2026
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
13 changes: 7 additions & 6 deletions apps/homepage/serializers/homepage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ def get_queryset(self, auth):
).exclude(chat_user_id="")

if name:
base_queryset = base_queryset.filter(asker__username__contains=name)
base_queryset = base_queryset.filter(asker__username__icontains=name)

# ---- 权限过滤 ----
base_queryset = self._apply_permission_filter(base_queryset, auth, workspace_id, user_id)
Expand Down Expand Up @@ -329,7 +329,7 @@ def export(self, auth, with_valid=True):
]
worksheet.append(row)
response = HttpResponse(content_type="application/vnd.ms-excel")
response["Content-Disposition"] = f'attachment; filename="data.xlsx"'
response["Content-Disposition"] = 'attachment; filename="data.xlsx"'
workbook.save(response)
return response

Expand Down Expand Up @@ -400,9 +400,10 @@ def get_queryset(self, auth):
queryset = QuerySet(Application)
is_resource_filter = True
if name:
queryset = queryset.filter(name__contains=name)
queryset = queryset.filter(name__icontains=name)
if is_workspace_manage(auth, workspace_id):
queryset = queryset.filter(workspace_id=workspace_id)
is_resource_filter = False
elif is_extends_workspace_manage(auth, workspace_id):
if has_extends_workspace_manage_permission(
auth, PermissionConstants.APPLICATION_READ.value, workspace_id
Expand Down Expand Up @@ -500,7 +501,7 @@ def export(self, auth, with_valid=True):
]
worksheet.append(row)
response = HttpResponse(content_type="application/vnd.ms-excel")
response["Content-Disposition"] = f'attachment; filename="data.xlsx"'
response["Content-Disposition"] = 'attachment; filename="data.xlsx"'
workbook.save(response)
return response

Expand Down Expand Up @@ -532,7 +533,7 @@ def get_queryset(self, auth):
is_resource_filter = True
queryset = QuerySet(Application)
if name:
queryset = queryset.filter(name__contains=name)
queryset = queryset.filter(name__icontains=name)
if is_workspace_manage(auth, workspace_id):
queryset = queryset.filter(workspace_id=workspace_id)
is_resource_filter = False
Expand Down Expand Up @@ -630,7 +631,7 @@ def export(self, auth, with_valid=True):
]
worksheet.append(row)
response = HttpResponse(content_type="application/vnd.ms-excel")
response["Content-Disposition"] = f'attachment; filename="data.xlsx"'
response["Content-Disposition"] = 'attachment; filename="data.xlsx"'
workbook.save(response)
return response

@shaohuzhang1 shaohuzhang1 Nov 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Review of the Diff

1. __contains → __icontains (lines ~245, ~400, ~533)

Good change.

  • Better UX: case-insensitive search is usually what users expect.
  • Caveat: icontains on large tables can be slower and may bypass certain DB indexes (e.g., PostgreSQL LIKE index only helps with text_pattern_ops/pg_trgm). If these querysets are large or hit hot paths, consider a trigram index or full-text search.

2. Removing redundant f-string (f'attachment; filename="data.xlsx"' → plain string)

Correct. There were no interpolation placeholders, so the f prefix was dead code. No behavior change.

3. is_resource_filter = False added after workspace_manage branch (line ~409)

Potentially behavior-changing — verify intended semantics.

Compare with the sibling method around line ~533:

if is_workspace_manage(auth, workspace_id):
    queryset = queryset.filter(workspace_id=workspace_id)
    is_resource_filter = False   # <-- only here in original
elif is_extends_workspace_manage(auth, workspace_id):
    ...

The patched block now also sets is_resource_filter = False for the workspace-manager path. Two concerns:

  1. Is this a bug fix or an accidental change? If is_resource_filter controls whether further permission filtering is applied downstream, setting it to False means workspace managers now skip that extra filtering. That is likely the intent (managers shouldn't be restricted by per-resource permissions), but it should be confirmed against tests.
  2. Consistency: The sibling method at ~533 already does this. So this change makes them consistent — good, but worth double-checking there isn't another branch (e.g., is_extends_workspace_manage) missing the same treatment.
  3. Naming clarity: is_resource_filter is ambiguous. Consider something like skip_resource_permission_filter.

4. Other observations (not in diff but visible)

  • Content-Type mismatch: content_type="application/vnd.ms-excel" is the MIME type for .xls, but the file is written as data.xlsx. For modern Excel use:

    content_type = "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet"

    Also add UTF-8 filename encoding via filename*=UTF-8''... if non-ASCII names may be used later.

  • Duplication: The export method is copy-pasted three times. Extract a shared helper like _build_excel_response(rows).

  • QuerySet(Application) — assuming this is a custom wrapper; fine, just noting.

5. Minor

  • In the removed f-string, workbook.save(response) is unchanged — fine.
  • Consider adding a test asserting case-insensitivity for each get_queryset, since icontains relies on DB collation and can differ across backends (SQLite vs MySQL vs Postgres).

Summary

Change Verdict
icontains ✅ Recommended
Remove useless f prefix ✅ Correct
is_resource_filter = False ⚠️ Verify intent + add a test
Content-Type / duplication 💡 Optimize separately

Expand Down
11 changes: 10 additions & 1 deletion apps/models_provider/impl/qianfan_model_provider/credential/llm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,16 @@ def is_valid(
model.invoke([HumanMessage(content="1")])
except Exception as e:
maxkb_logger.error(f"Exception: {e}", exc_info=True)
raise e
if isinstance(e, AppApiException):
raise e
if raise_exception:
raise AppApiException(
ValidCode.valid_error.value,
gettext("Verification failed, please check whether the parameters are correct: {error}").format(
error=str(e)
),
)
return False
return True

def encryption_dict(self, model_info: Dict[str, object]):

@shaohuzhang1 shaohuzhang1 Nov 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here are the issues and suggestions for the code:

Potential Issues:

  1. Inconsistent return behavior: The function now can return False (when raise_exception=False) or implicitly return True at the end, but in earlier paths it may raise exceptions. This is fine, but ensure callers handle both cases.

  2. Exception re-raise logic order:

    • If e is AppApiException, it re-raises as-is.
    • Otherwise, if raise_exception=True, it wraps in a new AppApiException.
    • However, the original exception loses its traceback context. Consider using raise ... from e to preserve the chain: 
      raise AppApiException(...) from e

  3. Swallowing exceptions silently: When raise_exception=False, the exception is logged (with exc_info=True) but conversion to return False hides details. If callers rely only on boolean result, this is fine, but no error message reaches them.

  4. Logging already happens before branching: Since maxkb_logger.error(...) runs for all cases, wrapped exceptions will be double-logged at the caller level. Acceptable, but note it.

  5. is_valid returns True unconditionally at the end — this means any exception path with raise_exception=False returns False, while success returns True. Just confirm this matches the intended contract (could there be other validation failures that should also return False instead of raising?).

Optimization Suggestions:

  1. Preserve the exception chain:

    raise AppApiException(...) from e

  2. Consider validating the type of raise_exception early (it's assumed boolean). Minor.

  3. The error message template uses .format(error=str(e)) — fine, but ensure gettext string has matching {error} placeholder in all translations.

  4. If AppApiException is a subclass of a general exception used elsewhere, ensure the isinstance check doesn't accidentally catch unrelated subclasses you want to wrap.

  5. Consider whether return False should also happen when raise_exception=False for the AppApiException branch — currently it always re-raises AppApiException regardless of raise_exception. Confirm that's intentional (likely yes, since it's a domain-specific error).

Summary: Main fix is adding from e to preserve traceback; otherwise the logic change looks reasonable assuming intended semantics.

Expand Down
6 changes: 6 additions & 0 deletions apps/models_provider/impl/qianfan_model_provider/model/image.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,15 @@ def is_cache_model():
@staticmethod
def new_instance(model_type, model_name, model_credential: Dict[str, object], **model_kwargs):
optional_params = MaxKBBaseModel.filter_optional_params(model_kwargs)
# request_timeout / max_retries 是 OpenAI 客户端配置项，不能随 extra_body 发送到接口请求体。
client_params = {}
for key in ("request_timeout", "max_retries"):
if optional_params.get(key) is not None:
client_params[key] = optional_params.pop(key)
return QianfanVisionModel(
model=model_name,
openai_api_base=model_credential.get("api_base"),
openai_api_key=model_credential.get("api_key"),
**client_params,
extra_body=optional_params,
)
7 changes: 7 additions & 0 deletions apps/models_provider/impl/qianfan_model_provider/model/llm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,16 @@ def is_cache_model():
@staticmethod
def new_instance(model_type, model_name, model_credential: Dict[str, object], **model_kwargs):
optional_params = MaxKBBaseModel.filter_optional_params(model_kwargs)
# request_timeout / max_retries 是 OpenAI 客户端配置项，不能随 extra_body 发送到接口请求体，
# 否则千帆 v2 网关可能返回 500 internal_error，且短超时/关闭重试不会真正生效。
client_params = {}
for key in ("request_timeout", "max_retries"):
if optional_params.get(key) is not None:
client_params[key] = optional_params.pop(key)
return QianfanChatModel(
model=model_name,
openai_api_base=model_credential.get("api_base"),
openai_api_key=model_credential.get("api_key"),
**client_params,
extra_body=optional_params,
)
79 changes: 79 additions & 0 deletions apps/models_provider/tests/test_qianfan_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
# coding=utf-8
from unittest.mock import MagicMock

from django.test import SimpleTestCase
from langchain_core.messages import HumanMessage

from common.exception.app_exception import AppApiException
from models_provider.impl.qianfan_model_provider.credential.llm import QianfanLLMModelCredential
from models_provider.impl.qianfan_model_provider.model.image import QianfanVisionModel
from models_provider.impl.qianfan_model_provider.model.llm import QianfanChatModel

CRED = {"api_base": "https://qianfan.baidubce.com/v2", "api_key": "sk-test"}


class QianfanModelConstructionTests(SimpleTestCase):
def test_client_params_are_not_sent_in_request_body(self):
for model_class in (QianfanChatModel, QianfanVisionModel):
with self.subTest(model=model_class.__name__):
model = model_class.new_instance(
"LLM",
"ernie-5.1",
CRED,
temperature=0.95,
max_tokens=1,
request_timeout=5.0,
max_retries=0,
)

# 客户端配置项应真正作用于 OpenAI 客户端
self.assertEqual(5.0, model.request_timeout)
self.assertEqual(0, model.max_retries)

# 且不应混入接口请求体
self.assertNotIn("request_timeout", model.extra_body)
self.assertNotIn("max_retries", model.extra_body)

payload = model._get_request_payload([HumanMessage(content="1")])
self.assertNotIn("request_timeout", payload)
self.assertNotIn("max_retries", payload)


class QianfanLLMCredentialTests(SimpleTestCase):
def _mock_provider(self, model):
provider = MagicMock()
provider.get_model.return_value = model
return provider

def test_validation_propagates_provider_error_message(self):
model = MagicMock()
model.invoke.side_effect = RuntimeError("Error code: 500 - {'error': {'code': 'internal_error'}}")

credential = QianfanLLMModelCredential()
with self.assertRaises(AppApiException) as raised:
credential.is_valid(
"LLM",
"ernie-5.1",
CRED,
{"temperature": 0.95},
self._mock_provider(model),
raise_exception=True,
)

self.assertIn("Error code: 500 - {'error': {'code': 'internal_error'}}", raised.exception.message)

def test_validation_returns_false_when_not_raising(self):
model = MagicMock()
model.invoke.side_effect = RuntimeError("boom")

credential = QianfanLLMModelCredential()
result = credential.is_valid(
"LLM",
"ernie-5.1",
CRED,
{"temperature": 0.95},
self._mock_provider(model),
raise_exception=False,
)

self.assertFalse(result)

@shaohuzhang1 shaohuzhang1 Nov 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here's my review of the code:

Irregularities / Potential Issues

  1. Missing test for actual success path: Neither credential test covers the happy path (valid credentials returning True). Only failure cases are tested.

  2. Brittle assertion on exception message verbatim: test_validation_propagates_provider_error_message asserts the exact string "Error code: 500 - {...}" is in .message. If the production code re-wraps or reformats the error, this breaks. Consider asserting on a stable substring like "internal_error" instead.

  3. Unused import: QianfanVisionModel and QianfanChatModel are both used, but verify MagicMock usage — it's fine. However max_tokens=1 / temperature=0.95 are arbitrary; document why if meaningful, else simplify.

  4. Hard-coded fake API key prefix sk-test: Fine for mocks, but ensure no network calls actually occur. new_instance could theoretically trigger validation/network I/O — confirm it doesn't, otherwise tests are not hermetic despite SimpleTestCase.

  5. Untested behavior in test_client_params_are_not_sent_in_request_body: The test verifies params are absent from extra_body and payload, but doesn't verify they land where expected on the underlying client beyond attribute checks. Attribute model.request_timeout existing doesn't guarantee it was passed to the OpenAI client constructor.

  6. MagicMock provider.get_model returns a mock whose .invoke raises — good, but the model's other attributes (e.g., name) may be accessed by is_valid before invoke; confirm the mock covers those accesses (currently any attribute auto-resolves to a MagicMock, which may mask bugs).

  7. Duplicated setup across tests: _mock_provider + QianfanLLMModelCredential() + argument tuple is repeated. Consider a helper or setUp.

  8. subTest loop only covers construction args identical for both models — fine, but add a comment that vision/chat share the same client-config contract intentionally.

Optimization Suggestions

  • Add a positive-path test (invoke succeeds → is_valid returns True).
  • Parametrize the two credential tests with subTest over raise_exception True/False to reduce duplication.
  • Assert AppApiException.message contains the original exception text via assertIn("internal_error", ...) rather than the full raw string.
  • Verify request params are forwarded to the client (e.g., assert against the constructed OpenAI client) to prove they're "applied," not just absent from the body.
  • Consider asserting payload does not unexpectedly contain other internal keys (snapshot-style) to catch regressions broadly.

Overall the tests are reasonable and focused; main gaps are the missing success case and over-specific error-message assertions.

Loading