fix(llm): guard GoogleCV (Google Vertex vision) key against non-JSON / JSON non-object - #17464
Conversation
…niflow#17463) The GoogleCV (Google Cloud Vertex vision) provider in rag/llm parses its API key as JSON to extract the service-account fields. The constructor had no try/except around the bare `json.loads(key)` call: # rag/llm/cv_model.py:1319 (GoogleCV.__init__) key = json.loads(key) access_token = json.loads(base64.b64decode(key.get("google_service_account_key", ""))) project_id = key.get("google_project_id", "") region = key.get("google_region", "") The Google Cloud Vertex provider REQUIRES a JSON object key (per the schema in api/apps/llm_app.py:291 and the test fixture in test/testcases/test_web_api/test_llm_app/test_llm_list_unit.py:635-640): { "google_project_id": "<project-id>", "google_region": "<region>", "google_service_account_key": "<base64-encoded-service-account-json>" } A user pasting a non-JSON string or a JSON non-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. Fix: add `_resolve_google_service_account_key` to rag/llm/key_utils and wire `GoogleCV.__init__` through it. The helper: - Accepts a dict (returned verbatim) or a JSON-string-encoded dict (parsed and returned) with all three fields extracted (`google_project_id`, `google_region`, `google_service_account_key`). - On non-JSON input, on a JSON top-level type that is not a dict (list, string, number, bool, null), or on any other input type (None, int, list, ...), raises a clear `ModelException(retryable=False)` naming the required object shape and pointing at the schema in `api/apps/llm_app.py:291`. Unlike the Bedrock/BaiduYiyan/VolcEngine/OpenRouter helpers, the Google Vertex helper does NOT have a plain-string-key fallback -- the provider genuinely requires the full object. The pre-existing `access_token = json.loads(base64.b64decode(...))` line still crashes with JSONDecodeError if `google_service_account_key` is missing, matching the pre-fix behavior; that is a separate missing-field bug, intentionally left out of scope. The chat counterpart `GoogleChat` (rag/llm/chat_model.py:1284) has the same shape of bug but is owned by @glu000 (PR infiniflow#15994) actively working on that class. The chat-side fix is intentionally OUT OF SCOPE for this PR to avoid a conflicting change. After this PR, no remaining LLM-provider JSON-decode sites in rag/llm/ that we own crash with `AttributeError` on JSON non-object keys. The only remaining site is the chat-side GoogleChat (rag/llm/chat_model.py:1284), which is owned by @glu000 via infiniflow#15994. Fixes infiniflow#17463.
…w#17463) 19 p0 tests in test/unit_test/rag/llm/test_googlecv_json_key_fallback.py: - TestResolveGoogleServiceAccountKey (16 tests on the helper directly): Python dict passes through, JSON dict with all/missing/empty fields is tolerated, plain string raises ModelException (the provider requires a JSON object), empty string raises, malformed JSON raises, 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. - TestGoogleCVCallSite (3 tests on the GoogleCV constructor): JSON array raises ModelException, JSON number raises ModelException, plain string raises ModelException. The Google SDK modules (google.oauth2.service_account, google.auth.transport.requests) are stubbed via sys.modules monkeypatch because GoogleCV.__init__ imports them lazily inside the function body. All 19 pass against the fix. The helper raises ModelException (retryable=False) on every non-JSON-object input with a message that names the required fields and points at the schema in api/apps/llm_app.py:291. Fixes infiniflow#17463.
📝 WalkthroughWalkthroughGoogle Vertex AI service-account keys are now normalized through a shared resolver. Invalid JSON, non-object JSON, and unsupported input types raise non-retryable ChangesGoogle service-account key validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/unit_test/rag/llm/test_googlecv_json_key_fallback.py (1)
242-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
KEY_VALID_JSONis defined but unused.Neither
TestResolveGoogleServiceAccountKeynorTestGoogleCVCallSitereferencesKEY_VALID_JSON. Either use it in a happy-pathGoogleCVconstructor test (verifyingproject_id/region/access_tokenresolve correctly end-to-end — currently only failure paths are exercised through the constructor), or remove it.As per path instructions: "Remove dead tests, commented-out code, stale documentation, and 'move later' notes instead of preserving them."
🤖 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 `@test/unit_test/rag/llm/test_googlecv_json_key_fallback.py` around lines 242 - 333, Remove the unused KEY_VALID_JSON fixture from the test module, since neither TestResolveGoogleServiceAccountKey nor TestGoogleCVCallSite uses it and the current tests intentionally cover only failure paths.Source: Path instructions
rag/llm/key_utils.py (1)
65-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider logging before raising in the new validation flow.
None of the three failure branches (malformed JSON, wrong input type, non-object JSON) emit a log line before raising
ModelException, making it harder to correlate provider misconfiguration with server logs.As per path instructions: "Add logging for new flows" (
**/*.py).🤖 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 65 - 94, Update the validation branches in the key-parsing flow to log the provider misconfiguration immediately before raising ModelException: malformed JSON in the string parser, unsupported input types, and non-object JSON payloads. Use the module’s existing logging mechanism and include the relevant error context, while preserving the current exception messages and retryable=False behavior.Source: Path instructions
🤖 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 68-74: Update the JSON parsing exception handler around json.loads
in the key utility to bind the caught exception and raise ModelException with
explicit exception chaining using from exc. Preserve the existing error message
and retryable=False behavior.
In `@test/unit_test/rag/llm/test_googlecv_json_key_fallback.py`:
- Around line 116-122: The test_json_dict_missing_google_service_account_key
case currently duplicates the missing-region scenario. Update its input to
include google_project_id and google_region while omitting only
google_service_account_key, and assert that the resolver preserves those values
and returns an empty service-account key.
---
Nitpick comments:
In `@rag/llm/key_utils.py`:
- Around line 65-94: Update the validation branches in the key-parsing flow to
log the provider misconfiguration immediately before raising ModelException:
malformed JSON in the string parser, unsupported input types, and non-object
JSON payloads. Use the module’s existing logging mechanism and include the
relevant error context, while preserving the current exception messages and
retryable=False behavior.
In `@test/unit_test/rag/llm/test_googlecv_json_key_fallback.py`:
- Around line 242-333: Remove the unused KEY_VALID_JSON fixture from the test
module, since neither TestResolveGoogleServiceAccountKey nor
TestGoogleCVCallSite uses it and the current tests intentionally cover only
failure paths.
🪄 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: 09f336b9-7840-4646-9e9c-44e1dcae4df0
📒 Files selected for processing (3)
rag/llm/cv_model.pyrag/llm/key_utils.pytest/unit_test/rag/llm/test_googlecv_json_key_fallback.py
| try: | ||
| payload = json.loads(key) | ||
| except (json.JSONDecodeError, TypeError): | ||
| raise ModelException( | ||
| "Google Vertex AI key is not valid JSON. Expected an object with 'google_project_id', 'google_region', and 'google_service_account_key' (see api/apps/llm_app.py:291 for the schema).", | ||
| retryable=False, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Preserve exception chain with from exc.
The except block re-raises ModelException without chaining to the original JSON error, dropping the underlying cause from the traceback.
🔧 Proposed fix
elif isinstance(key, str):
try:
payload = json.loads(key)
- except (json.JSONDecodeError, TypeError):
+ except (json.JSONDecodeError, TypeError) as exc:
raise ModelException(
"Google Vertex AI key is not valid JSON. Expected an object with 'google_project_id', 'google_region', and 'google_service_account_key' (see api/apps/llm_app.py:291 for the schema).",
retryable=False,
- )
+ ) from excBased on learnings (Repo: infiniflow/ragflow PR: 17390, file rag/llm/key_utils.py): "use Python exception chaining: raise ModelException(...) from exc... rather than losing it by re-raising without from exc." This also matches the Ruff B904 hint at lines 71-74.
📝 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.
| try: | |
| payload = json.loads(key) | |
| except (json.JSONDecodeError, TypeError): | |
| raise ModelException( | |
| "Google Vertex AI key is not valid JSON. Expected an object with 'google_project_id', 'google_region', and 'google_service_account_key' (see api/apps/llm_app.py:291 for the schema).", | |
| retryable=False, | |
| ) | |
| try: | |
| payload = json.loads(key) | |
| except (json.JSONDecodeError, TypeError) as exc: | |
| raise ModelException( | |
| "Google Vertex AI key is not valid JSON. Expected an object with 'google_project_id', 'google_region', and 'google_service_account_key' (see api/apps/llm_app.py:291 for the schema).", | |
| retryable=False, | |
| ) from exc |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 71-74: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 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 68 - 74, Update the JSON parsing exception
handler around json.loads in the key utility to bind the caught exception and
raise ModelException with explicit exception chaining using from exc. Preserve
the existing error message and retryable=False behavior.
Sources: Learnings, Linters/SAST tools
| def test_json_dict_missing_google_service_account_key(self): | ||
| out = _resolve_google_service_account_key(json.dumps({"google_project_id": "p"})) | ||
| assert out == { | ||
| "google_project_id": "p", | ||
| "google_region": "", | ||
| "google_service_account_key": "", | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Duplicate test body — doesn't actually test the missing-google_service_account_key-only case.
This test is byte-for-byte identical to test_json_dict_missing_google_region above it (same input, same assertion), so it provides no coverage of the case where only google_service_account_key is absent.
🔧 Proposed fix
def test_json_dict_missing_google_service_account_key(self):
- out = _resolve_google_service_account_key(json.dumps({"google_project_id": "p"}))
+ out = _resolve_google_service_account_key(
+ json.dumps({"google_project_id": "p", "google_region": "us-central1"})
+ )
assert out == {
"google_project_id": "p",
- "google_region": "",
+ "google_region": "us-central1",
"google_service_account_key": "",
}📝 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.
| def test_json_dict_missing_google_service_account_key(self): | |
| out = _resolve_google_service_account_key(json.dumps({"google_project_id": "p"})) | |
| assert out == { | |
| "google_project_id": "p", | |
| "google_region": "", | |
| "google_service_account_key": "", | |
| } | |
| def test_json_dict_missing_google_service_account_key(self): | |
| out = _resolve_google_service_account_key( | |
| json.dumps({"google_project_id": "p", "google_region": "us-central1"}) | |
| ) | |
| assert out == { | |
| "google_project_id": "p", | |
| "google_region": "us-central1", | |
| "google_service_account_key": "", | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 116-116: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"google_project_id": "p"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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 `@test/unit_test/rag/llm/test_googlecv_json_key_fallback.py` around lines 116 -
122, The test_json_dict_missing_google_service_account_key case currently
duplicates the missing-region scenario. Update its input to include
google_project_id and google_region while omitting only
google_service_account_key, and assert that the resolver preserves those values
and returns an empty service-account key.
1 participant
Summary
Closes #17463. The
GoogleCV(Google Cloud Vertex vision) provider inrag/llm/parses its API key as JSON to extract the service-account fields. The constructor had no try/except around the barejson.loads(key)call:The Google Cloud Vertex provider REQUIRES a JSON object key (per the schema in
api/apps/llm_app.py:291and the test fixture intest/testcases/test_web_api/test_llm_app/test_llm_list_unit.py:635-640):{ "google_project_id": "<project-id>", "google_region": "<region>", "google_service_account_key": "<base64-encoded-service-account-json>" }A user pasting a non-JSON string or a JSON non-object — e.g.
"[1,2,3]","42",'"hi"',"true","null"— parses fine and then crashes on the subsequent.get(...)call withAttributeError: 'list' object has no attribute 'get'from insiderag/llminternals — no indication of what the user did wrong.This is the same class of bug PR #17215 fixed for Azure, PR #17377 fixed for Bedrock, PR #17390 fixed for BaiduYiyan, PR #17457 fixed for VolcEngine/Ark, and PR #17459 fixed for OpenRouter. After this PR, no remaining LLM-provider JSON-decode sites in
rag/llm/that we own crash withAttributeErroron JSON non-object keys.Changes
1 helper, 1 call site, 1 test file:
rag/llm/key_utils.py— add_resolve_google_service_account_key(key). Returns a dict{"google_project_id": str, "google_region": str, "google_service_account_key": str}.""if missing).ModelException(retryable=False)naming the required schema and pointing atapi/apps/llm_app.py:291. Unlike the Bedrock/BaiduYiyan/VolcEngine/OpenRouter helpers, this one does NOT have a plain-key fallback — the Google Vertex provider genuinely requires the full object.ModelException(retryable=False)naming the actual type and the required schema.ModelException.rag/llm/cv_model.py—GoogleCV.__init__calls the helper instead of the barejson.loads(key).get(...)calls.test/unit_test/rag/llm/test_googlecv_json_key_fallback.py— 19 p0 regression tests:GoogleCVconstructor (JSON array, JSON number, plain string). The Google SDK modules (google.oauth2.service_account,google.auth.transport.requests) are stubbed viasys.modulesmonkeypatch becauseGoogleCV.__init__imports them lazily inside the function body.Out of scope
GoogleChat(rag/llm/chat_model.py:1284-1289) has the same shape of bug, but PR fix(llm): implement async chat methods for GoogleChat (Vertex AI Gemini) #15994 is open by @glu000 actively working on that class. The chat-side JSON-decode fix is intentionally OUT OF SCOPE here to avoid a conflicting PR.rag/llm/embedding_model.py:1014) has the same shape — filed separately.access_token = json.loads(base64.b64decode(...))line still raisesJSONDecodeErrorifgoogle_service_account_keyis missing (matching the pre-fix behavior). That is a separate missing-field bug, intentionally out of scope.Behavior unchanged for the existing happy path
project_id,region, andaccess_tokenpopulate correctly (unchanged).""(unchanged from the pre-fixdict.get(k, "")calls).No public API change. No data-model change. No migration.
Testing
Cross-references
rag/llm/that we own crash withAttributeErroron JSON non-object keys. The only remaining site is the chat-sideGoogleChat(rag/llm/chat_model.py:1284), which is owned by @glu000 via fix(llm): implement async chat methods for GoogleChat (Vertex AI Gemini) #15994.