fix(llm): guard OpenRouter key against JSON non-object - #17459
Open
Harsh23Kashyap wants to merge 2 commits into
Open
fix(llm): guard OpenRouter key against JSON non-object#17459Harsh23Kashyap wants to merge 2 commits into
Harsh23Kashyap wants to merge 2 commits into
Conversation
) The OpenRouter provider in rag/llm has three call sites that do an unguarded `json.loads(key).get("api_key", "")` inside a `try: ... except JSONDecodeError:` block (chat and CV) or with an `isinstance(payload, dict)` fallback (embedding): - LiteLLMBase.__init__ (chat, OpenRouter branch) -- chat_model.py:1627 - OpenRouterCV.__init__ (vision) -- cv_model.py:698 - OpenRouterEmbed.__init__ (embedding) -- embedding_model.py:1338 The `except JSONDecodeError` only catches the *parse* failure (added by PR infiniflow#15776 / commit 8e4fba6). A user pasting a JSON string that is NOT an object -- e.g. `"[1,2,3]"`, `"42"", `'"hello"'`, `"true"", `"null"` -- parses fine and then crashes on the `.get(...)` call with `AttributeError: 'list' object has no attribute 'get'` (or `'int'`, `'str'`, `'NoneType'`, `'bool'`) from inside rag/llm internals -- no indication of what the user did wrong. The embedding site does not crash (it has the `isinstance(payload, dict)` check) but silently uses the raw JSON string as api_key, which then fails at the OpenAI client with a less-actionable 401 Unauthorized. Fix: add `_resolve_openrouter_credentials` to rag/llm/key_utils, then wire the three call sites through it. The helper: - Accepts a plain (non-JSON) string and returns `{"api_key": key, "provider_order": ""}` -- matching the pre-fix `except JSONDecodeError` fallback semantics. - Accepts a dict (returned verbatim) or a JSON-string-encoded dict (parsed and returned) with both fields extracted. - On a JSON top-level type that is not a dict (list, string, number, bool, null), raises a clear `ModelException(retryable=False)` naming the required object shape and pointing at `conf/models/openrouter.json`. Operators can self-diagnose without opening an issue. Behavior unchanged for the existing happy paths: - Plain key -> `api_key = key, provider_order = ""` (preserves fallback) - JSON dict -> `api_key = parsed["api_key"]`, `provider_order = parsed["provider_order"]` - Missing fields -> defaults to `""` (unchanged from pre-fix) No public API change. No data-model change. No migration. Fixes infiniflow#17458.
…low#17458) 27 p0 tests in test/unit_test/rag/llm/test_openrouter_json_key_fallback.py: - TestResolveOpenrouterCredentials (15 tests on the helper directly): plain string key passes through, empty string falls through, Python dict passes through, JSON dict with all fields extracts correctly, JSON dict missing api_key, missing provider_order, and missing both fields is tolerated (matches pre-fix behavior for partial JSON), JSON top-level non-object types (array, string, number, float, null, bool) all raise a clear ModelException naming the type, non-string non-dict input raises, malformed JSON falls through as plain key. - TestOpenRouterChatCallSite (4 tests on the LiteLLMBase OpenRouter branch): plain key via helper, JSON dict via helper, JSON array via helper raises ModelException, JSON number via helper raises ModelException. We can't easily build a full LiteLLMBase in the test (it pulls in heavy deps), so we test the helper wiring directly on the resolved values. - TestOpenRouterCVCallSite (4 tests on the vision call site): plain key constructs with the key as api_key, JSON dict constructs with parsed api_key, JSON array raises ModelException, JSON number raises ModelException. - TestOpenRouterEmbedCallSite (4 tests on the embedding call site): plain key constructs with key as api_key and provider_order defaulting to empty, JSON dict constructs with parsed provider_order, JSON array raises ModelException (new behavior; the pre-fix code silently used the raw JSON string as api_key, which then failed auth later with a less-actionable 401), JSON number raises ModelException. All 27 pass against the fix. The OpenAI/AsyncOpenAI constructors are patched at the import site (rag.llm.cv_model) via `side_effect` to avoid real OpenRouter round-trips while still capturing the constructor kwargs (in particular `api_key`) for assertion. Fixes infiniflow#17458.
dosubot
Bot
added
size:L
Contributor
📝 WalkthroughWalkthroughOpenRouter credential parsing is centralized in a shared resolver and adopted by chat, CV, and embedding providers. Regression tests cover plain keys, JSON objects, malformed JSON, invalid top-level JSON values, provider ordering, and non-retryable errors. ChangesOpenRouter credential handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rag/llm/embedding_model.py (1)
1352-1363: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
provider_orderisn't type-normalized before.split(","), unlike the other two call sites.
resolved["provider_order"]is stored onself.provider_orderas-is;_resolve_openrouter_credentialsdoesn't coerce its type (per its docstring it only defaults missing values to""). If a user supplies a JSON key like{"api_key": "...", "provider_order": ["Anthropic", "OpenAI"]},self.provider_orderbecomes a list, and_call()'sself.provider_order.split(",")at line 1362 raisesAttributeError: 'list' object has no attribute 'split'— the exact class of crash this PR sets out to eliminate.chat_model.py's andcv_model.py's_to_order_list()helpers already guard against this by acceptingstr,list, ortuple;embedding_model.py's_callhas no equivalent guard.🛡️ Proposed fix
def _call(self, batch): extra_body = {"drop_params": True} if self.provider_order: - order = [s.strip() for s in self.provider_order.split(",") if s.strip()] + if isinstance(self.provider_order, (list, tuple)): + order = [str(s).strip() for s in self.provider_order if str(s).strip()] + else: + order = [s.strip() for s in str(self.provider_order).split(",") if s.strip()] extra_body["provider"] = {"order": order, "allow_fallbacks": False}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rag/llm/embedding_model.py` around lines 1352 - 1363, Normalize self.provider_order in the embedding model’s _call method before processing it, accepting strings, lists, and tuples consistently with the existing _to_order_list helpers in the other model call sites. Ensure JSON array provider_order values are converted into a usable order list without calling .split on non-string values, while preserving the current provider configuration and fallback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rag/llm/key_utils.py`:
- Around line 73-99: Add logging immediately before both ModelException raises
in the OpenRouter key validation flow: the unsupported input-type branch and the
parsed non-dict payload branch. Use the existing logging mechanism and include
the relevant actual type and validation context, while preserving the current
exception messages and retryable=False behavior.
---
Nitpick comments:
In `@rag/llm/embedding_model.py`:
- Around line 1352-1363: Normalize self.provider_order in the embedding model’s
_call method before processing it, accepting strings, lists, and tuples
consistently with the existing _to_order_list helpers in the other model call
sites. Ensure JSON array provider_order values are converted into a usable order
list without calling .split on non-string values, while preserving the current
provider configuration and fallback behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7411e164-bb8f-486f-9895-51516111f5cd
📒 Files selected for processing (5)
rag/llm/chat_model.pyrag/llm/cv_model.pyrag/llm/embedding_model.pyrag/llm/key_utils.pytest/unit_test/rag/llm/test_openrouter_json_key_fallback.py
Comment on lines
+73
to
+99
| if isinstance(key, dict): | ||
| payload = key | ||
| elif isinstance(key, str): | ||
| try: | ||
| payload = json.loads(key) | ||
| except (json.JSONDecodeError, TypeError): | ||
| # Plain (non-JSON) string key. Pre-fix behavior: use the | ||
| # string as-is for ``api_key`` and default ``provider_order`` | ||
| # to empty. Preserve. | ||
| return {"api_key": key, "provider_order": ""} | ||
| else: | ||
| raise ModelException( | ||
| f"OpenRouter key must be a string or dict, got {type(key).__name__}. See conf/models/openrouter.json for the full schema.", | ||
| retryable=False, | ||
| ) | ||
|
|
||
| if not isinstance(payload, dict): | ||
| # JSON parsed but the top-level type is not an object. The pre-fix | ||
| # code did ``payload.get("api_key", "")`` here and crashed with | ||
| # AttributeError on lists/strings/numbers/booleans/null. Surface a | ||
| # clear error naming the actual type and the required schema. | ||
| raise ModelException( | ||
| f"OpenRouter key must be a JSON object, got {type(payload).__name__}. " | ||
| "Expected an object with 'api_key' (and optionally 'provider_order'); " | ||
| "see conf/models/openrouter.json for the full schema.", | ||
| retryable=False, | ||
| ) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)key_utils\.py$|(^|/)chat_model\.py$|^rag/llm/key_utils\.py$|^rag/llm/chat_model\.py$' || true
echo "== key_utils outline =="
ast-grep outline rag/llm/key_utils.py --view expanded || true
echo "== key_utils relevant lines =="
nl -ba rag/llm/key_utils.py | sed -n '1,140p'
echo "== logging usage in rag/llm.py == (sample)"
for f in rag/llm/*.py; do
if rg -q 'logging\.' "$f"; then
echo "--- $f"
rg -n 'logging\.' "$f" | head -20
fi
doneRepository: infiniflow/ragflow
Length of output: 494
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== key_utils.py =="
awk '{printf "%5d: %s\n", NR, $0}' rag/llm/key_utils.py
echo "== logging imports/usages in rag/llm/key_utils.py =="
python3 - <<'PY'
from pathlib import Path
p = Path("rag/llm/key_utils.py")
text = p.read_text()
print("imports", [s.strip() for s in text.splitlines() if s.lstrip().startswith("import ") or s.lstrip().startswith("from ")])
print("logging count", text.count("logging"))
PY
echo "== sample error logging pattern in rag/llm/chat_model.py =="
awk '{printf "%5d: %s\n", NR, $0}' rag/llm/chat_model.py | sed -n '1,220p'Repository: infiniflow/ragflow
Length of output: 15139
Add logging for the OpenRouter key validation failures.
These are new failure flows: raise when the key type is unsupported, and raise again when parsed JSON is not an object. Add a log line before each ModelException so malformed key inputs are visible in server logs.
🪵 Proposed fix
else:
+ logging.error("OpenRouter key must be a string or dict, got %s", type(key).__name__)
raise ModelException(
f"OpenRouter key must be a string or dict, got {type(key).__name__}. See conf/models/openrouter.json for the full schema.",
retryable=False,
)
if not isinstance(payload, dict):
+ logging.error("OpenRouter key JSON top-level type must be an object, got %s", type(payload).__name__)
raise ModelException(
f"OpenRouter key must be a JSON object, got {type(payload).__name__}. "
"Expected an object with 'api_key' (and optionally 'provider_order'); "
"see conf/models/openrouter.json for the full schema.",
retryable=False,
)📝 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
| if isinstance(key, dict): | |
| payload = key | |
| elif isinstance(key, str): | |
| try: | |
| payload = json.loads(key) | |
| except (json.JSONDecodeError, TypeError): | |
| # Plain (non-JSON) string key. Pre-fix behavior: use the | |
| # string as-is for ``api_key`` and default ``provider_order`` | |
| # to empty. Preserve. | |
| return {"api_key": key, "provider_order": ""} | |
| else: | |
| raise ModelException( | |
| f"OpenRouter key must be a string or dict, got {type(key).__name__}. See conf/models/openrouter.json for the full schema.", | |
| retryable=False, | |
| ) | |
| if not isinstance(payload, dict): | |
| # JSON parsed but the top-level type is not an object. The pre-fix | |
| # code did ``payload.get("api_key", "")`` here and crashed with | |
| # AttributeError on lists/strings/numbers/booleans/null. Surface a | |
| # clear error naming the actual type and the required schema. | |
| raise ModelException( | |
| f"OpenRouter key must be a JSON object, got {type(payload).__name__}. " | |
| "Expected an object with 'api_key' (and optionally 'provider_order'); " | |
| "see conf/models/openrouter.json for the full schema.", | |
| retryable=False, | |
| ) | |
| if isinstance(key, dict): | |
| payload = key | |
| elif isinstance(key, str): | |
| try: | |
| payload = json.loads(key) | |
| except (json.JSONDecodeError, TypeError): | |
| # Plain (non-JSON) string key. Pre-fix behavior: use the | |
| # string as-is for ``api_key`` and default ``provider_order`` | |
| # to empty. Preserve. | |
| return {"api_key": key, "provider_order": ""} | |
| else: | |
| logging.error("OpenRouter key must be a string or dict, got %s", type(key).__name__) | |
| raise ModelException( | |
| f"OpenRouter key must be a string or dict, got {type(key).__name__}. See conf/models/openrouter.json for the full schema.", | |
| retryable=False, | |
| ) | |
| if not isinstance(payload, dict): | |
| logging.error("OpenRouter key JSON top-level type must be an object, got %s", type(payload).__name__) | |
| raise ModelException( | |
| f"OpenRouter key must be a JSON object, got {type(payload).__name__}. " | |
| "Expected an object with 'api_key' (and optionally 'provider_order'); " | |
| "see conf/models/openrouter.json for the full schema.", | |
| retryable=False, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rag/llm/key_utils.py` around lines 73 - 99, Add logging immediately before
both ModelException raises in the OpenRouter key validation flow: the
unsupported input-type branch and the parsed non-dict payload branch. Use the
existing logging mechanism and include the relevant actual type and validation
context, while preserving the current exception messages and retryable=False
behavior.
Source: Coding guidelines
This was referenced Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #17458. The OpenRouter provider in
rag/llm/parses its API key as JSON, looking forapi_keyandprovider_order. The three call sites had a partialtry: ... except JSONDecodeError:block added by PR #15776 (commit8e4fba6cd):except JSONDecodeErroronly catches the case where the string fails to parse as JSON. A user pasting a JSON string that parses but is not an object (e.g."[1,2,3]","42",'"hi"',"true","null") would crash on the subsequent.get(...)call withAttributeError: 'list' object has no attribute 'get'from insiderag/llminternals — no indication of what the user did wrong. The embedding site does not crash (it has anisinstance(payload, dict)check) but silently uses the raw JSON string as the API key, which then fails at the OpenAI client with a less-actionable 401 Unauthorized.This is the same class of bug PR #17215 fixed for Azure, PR #17377 fixed for Bedrock, PR #17390 fixed for BaiduYiyan, and PR #17457 fixed for VolcEngine/Ark. After this PR, no remaining LLM-provider JSON-decode sites in
rag/llm/crash withAttributeErroron JSON non-object keys.Changes
1 helper, 3 call sites, 1 test file:
rag/llm/key_utils.py— add_resolve_openrouter_credentials(key). Returns a dict{"api_key": str, "provider_order": str}.{"api_key": key, "provider_order": ""}so the existingexcept JSONDecodeErrorfallback semantics are preserved.{"api_key": payload["api_key"], "provider_order": payload["provider_order"]}(both default to"").ModelException(retryable=False)naming the actual type and pointing atconf/models/openrouter.json.rag/llm/chat_model.py—LiteLLMBase.__init__(OpenRouter branch) calls the helper instead ofjson.loads(key).get(...)× 2.rag/llm/cv_model.py—OpenRouterCV.__init__calls the helper.rag/llm/embedding_model.py—OpenRouterEmbed.__init__calls the helper (replaces the silentisinstance(payload, dict)fallback with a clearModelException).test/unit_test/rag/llm/test_openrouter_json_key_fallback.py— 27 p0 regression tests:LiteLLMBaseis too heavy to build in a unit test)Behavior unchanged for the existing happy paths
api_key = key, provider_order = ""(unchanged)api_key = parsed["api_key"], provider_order = parsed["provider_order"](unchanged)api_keyorprovider_order→ defaults to""(unchanged)ModelExceptionfor consistency with the chat and CV sites.No public API change. No data-model change. No migration.
Testing
Plus the existing 14
test_embedding_model.py+ 23test_bedrock_rerank.pytests pass unchanged (64 / 64 in the combined run).Cross-references
8e4fba6cd, June 2026) that addedtry/except JSONDecodeErrorbut did not address theAttributeErrorcase. This PR closes the gap.rag/llm/crash withAttributeErroron JSON non-object keys. The only places left inrag/llm/with unguardedjson.loads(key)are insideLiteLLMBasefor providers that pass through litellm (which is fine — litellm handles its own key parsing).