[eschutho]

SUMMARY

superset/databases/schemas.py::extra_validator — the shared marshmallow validate=
callable for the extra field on DatabasePostSchema, DatabasePutSchema,
DatabaseTestConnectionSchema and friends — could let a raw AttributeError escape as
an opaque HTTP 500 instead of returning a validation error.

Problem

extra_ = json.loads(value)          # accepts ANY valid JSON, not just objects
...
for key in extra_.get("metadata_params", {}):   # <-- assumes a mapping

json.loads() happily returns non-mapping values: json.loads("123") → int,
"null" → None, "[1,2]" → list, "true" → bool, '"abc"' → str. None of
those have .get(), so the next line raises
AttributeError: 'int' object has no attribute 'get'.

Nothing along the path turns that into a validation error:

• marshmallow only translates ValidationError raised by a field validator; anything
  else propagates straight out of schema.load();
• superset/databases/api.py post() / put() wrap self.add_model_schema.load(...) /
  self.edit_model_schema.load(...) in except ValidationError only — nothing broader
  (statsd_metrics catches Exception purely to emit a metric and re-raises);
• it is ultimately swallowed by the catch-all @app.errorhandler(Exception) in
  superset/views/error_handling.py, which renders it as a SIP-40
  GENERIC_BACKEND_ERROR 500 whose message is the raw
  'int' object has no attribute 'get'.

So any authenticated user with database create/edit permission sending
POST /api/v1/database/ or PUT /api/v1/database/<pk>/ with {"extra": "123"} (or any
other non-object JSON string) gets a 500 leaking an internal type error, where a 4xx
validation error is the correct response. This is a robustness/error-handling
bug, not a privilege issue: the request already requires the database write capability,
so no principal gains anything they were not entitled to.

Fix

Guard the decoded value with the same isinstance + ValidationError pattern that
already exists a few lines below in the very same function for metadata_cache_timeout:

if not isinstance(extra_, dict):
    raise ValidationError(
        [_("Extra field must be a mapping from string keys to values.")]
    )

The check is placed after the json.JSONDecodeError handler and before any .get() /
in use of extra_, so malformed JSON still reports the existing decode message and
dict-valued extra behaves exactly as before.

This follows the same "raw exception → proper Superset exception" cleanup as
#42366 and #42401.

Behavior change: a request that previously returned an unhandled 500 now returns a
proper 4xx validation error. Strict improvement — no tradeoff to disclose. Valid
extra payloads are unaffected.

One incidental note: adding the branch pushed extra_validator from McCabe complexity
10 to 11, so the function carries a # noqa: C901. Its complexity is a flat sequence of
independent field validations rather than nesting, and this matches ~144 existing
# noqa: C901 usages under superset/. Extracting the neighboring
metadata_cache_timeout block into a helper would also work if reviewers prefer that,
but it would enlarge an otherwise 5-line diff.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A — API error-handling change, no UI surface.

TESTING INSTRUCTIONS

Automated: pytest tests/unit_tests/databases/schema_tests.py

A new parametrized regression test,
test_extra_validator_rejects_non_dict_top_level_value, drives the real code path
through DatabasePostSchema().load(...) (not a direct call to the validator, so
marshmallow's error handling is exercised) for all five non-mapping JSON shapes
(123, null, [1, 2], true, "abc").

Verified both ways:

• against unfixed code: all 5 parametrizations fail with
  AttributeError: 'str' object has no attribute 'get' at schemas.py:278
• with the fix: 5 passed
• whole file after the fix: 44 passed, including the pre-existing
  metadata_params, metadata_cache_timeout and JSON-decode-message tests — the new
  check does not fire ahead of the JSONDecodeError handler and does not change
  behavior for dict-valued extra.

Manual:

curl -X POST "$SUPERSET/api/v1/database/" -H "Authorization: Bearer $TOKEN" \
  -H "Content-Type: application/json" \
  -d '{"database_name": "test_db", "sqlalchemy_uri": "sqlite://", "extra": "123"}'

Before: 500 (GENERIC_BACKEND_ERROR, message 'int' object has no attribute 'get').
After: a 422 validation error naming the extra field.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[bito-code-review]

Code Review Agent Run #eff883

Actionable Suggestions - 0

Review Details

• Files reviewed - 2 · Commit Range: 997d6a8..997d6a8

  • superset/databases/schemas.py
  • tests/unit_tests/databases/schema_tests.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.08%. Comparing base (078915f) to head (997d6a8).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #44092       +/-   ##
===========================================
+ Coverage   57.47%   80.08%   +22.60%     
===========================================
  Files        2922     2922               
  Lines      171962   171964        +2     
  Branches    39907    39908        +1     
===========================================
+ Hits        98837   137719    +38882     
+ Misses      72197    31664    -40533     
- Partials      928     2581     +1653     

Flag	Coverage Δ
hive	37.40% <33.33%> (-0.01%)	⬇️
mysql	56.98% <33.33%> (?)
postgres	57.01% <33.33%> (?)
presto	39.29% <33.33%> (-0.01%)	⬇️
python	84.49% <100.00%> (+45.14%)	⬆️
sqlite	56.71% <33.33%> (?)
unit	75.72% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[rebenitez1802]

Approve — correct, well-tested fix and a strict improvement: a raw AttributeError→500 becomes a proper 4xx validation error. Security model intact — POST/PUT /api/v1/database already requires can_write on Database, so this is robustness, not a privilege change, and the constant error message leaks nothing. One optional, pre-existing gap on an adjacent line is worth considering while you're here.

🟢 Low — Sibling metadata_params still 500s on a non-iterable value

The new guard fully closes the top-level case, but one level down at superset/databases/schemas.py:278, for key in extra_.get("metadata_params", {}) carries the same unguarded-iteration bug this PR targets. A payload whose top level is a dict (so it passes the new guard) but whose metadata_params is a non-iterable scalar — {"metadata_params": 123}, null, true, 1.5 — raises TypeError: 'int' object is not iterable. That isn't a ValidationError, so it propagates to Flask's catch-all and surfaces as the exact SIP-40 GENERIC_BACKEND_ERROR 500 this PR set out to eliminate. (List/string values happen to iterate and yield a handled-but-misleading ValidationError.) Pre-existing on master and outside this PR's title scope, so non-blocking — but it's the same bug class on the adjacent line and cheap to close in the same diff:

metadata_params = extra_.get("metadata_params", {})
if not isinstance(metadata_params, dict):
    raise ValidationError(
        [_("The metadata_params in Extra field must be a mapping from string keys to values.")]
    )
metadata_signature = inspect.signature(MetaData)
for key in metadata_params:
    ...