rebenitez1802 commented on code in PR #43772:
URL: https://github.com/apache/superset/pull/43772#discussion_r3916363366
##########
superset/db_engine_specs/gsheets.py:
##########
@@ -393,7 +393,22 @@ def validate_parameters(
# On create the encrypted credentials are a string,
# at all other times they are a dict
if isinstance(encrypted_credentials, str):
- encrypted_credentials = json.loads(encrypted_credentials)
+ try:
+ encrypted_credentials = json.loads(encrypted_credentials)
+ except json.JSONDecodeError:
+ errors.append(
+ SupersetError(
+ message=(
+ "The service account credentials are not valid
JSON. "
+ "Please check that the field contains a valid
service "
+ "account key."
+ ),
+
error_type=SupersetErrorType.INVALID_PAYLOAD_FORMAT_ERROR,
+ level=ErrorLevel.ERROR,
+ extra={"invalid": ["service_account_info"]},
+ ),
+ )
+ return errors
Review Comment:
🟡 Medium (non-blocking) — the twin `json.loads` on `masked_encrypted_extra`
still 500s, and the "validated upstream" rationale is only half-right.
_(Anchoring on the new `service_account_info` guard — the fix below targets
lines 436-438 of this file, which sit ~25 lines down and outside this PR's
diff, so GitHub won't attach an applyable suggestion directly to them.)_
`encrypted_extra_validator` skips falsy values (`if value:`,
`superset/databases/schemas.py:246`) and this field is `allow_none=True`
(`schemas.py:490`), so `masked_encrypted_extra: ""` or `: null` passes
marshmallow and reaches the still-unguarded call at lines 436-438. `.get(...,
"{}")` only defaults when the key is *absent*, so a present-but-empty value
flows straight in: `json.loads("")` raises `JSONDecodeError` and
`json.loads(None)` raises `TypeError` → uncaught → the same opaque 500 this PR
set out to remove, in the same method and endpoint. Not a security issue (the
route requires `can_write` on `Database`), so this is a robustness/completeness
gap rather than a blocker — but it's the same bug-class one screen down.
Suggested fix — replace lines 436-438 with a guarded version (`or "{}"`
collapses both `""` and `None`, killing the `TypeError` path too; the
`try/except` mirrors the guard this PR adds above):
```python
try:
secure_extra = json.loads(
properties.get("masked_encrypted_extra") or "{}"
)
except json.JSONDecodeError:
secure_extra = {}
oauth2_config_in_secure_extra =
secure_extra.get("oauth2_client_info")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
