Skip to content

Fix 23 correctness and security issues from full-codebase review - #142

Merged
vsdudakov merged 3 commits into
mainfrom
fix/full-codebase-review-findings
Jul 10, 2026
Merged

Fix 23 correctness and security issues from full-codebase review#142
vsdudakov merged 3 commits into
mainfrom
fix/full-codebase-review-findings

Conversation

@vsdudakov

Copy link
Copy Markdown
Owner

Summary

A full-codebase review of the shipped fastadmin/ package surfaced 137 verified candidate issues; this PR applies the 23 highest-value correctness and security fixes. All 2005 tests pass; ruff lint/format and ty type-check are clean.

Security

  • Authorization bypass on action — bulk actions now enforce has_change_permission, matching the change/delete endpoints. Default hooks return True, so default deployments are unaffected; only deliberately restricted (read-only) admins gain the intended restriction.
  • Blank password overwrite — an empty password field on edit no longer re-hashes to hash("") (which would lock out the user and let anyone sign in with an empty password).
  • Empty/unset ADMIN_SECRET_KEY — the JWT sign/verify paths refuse a blank key (closing the empty-string HS256 forgery) and return a clear config error instead of a 500.

Correctness

  • Falsy-zero PK/user-id traps — auth guards and orm_save_obj across all five ORM adapters (SQLAlchemy, Pony, Tortoise, Django, Yara) now use is not None, so a legitimate primary key / user id of 0 no longer turns an update into an insert or rejects a valid session.
  • exclude wins over list_display in serialization, so a field hidden via exclude is not re-added to responses.
  • Cleaner 4xx instead of 500 — malformed date/datetime values, empty-condition filters (name__), unsupported/null export format, malformed sign-in bodies, and None widget-action responses now return proper 4xx / empty responses.
  • SQLAlchemy — real PK-name fallback for non-autoincrement PKs (UUID/String); cast-to-text for contains/icontains (no more int-LIKE errors on Postgres); PK excluded from required; no post-commit expired-attribute read (expire_on_commit safe).
  • Field metadata — falsy DB defaults (0/False/"") no longer force a field to required (Tortoise/Django/Yara); Tortoise enum options emit the scalar .value; Django choice option label/value un-swapped.
  • Framework integrations — Django error responses unified under the detail key (matching FastAPI/Flask and the frontend); configuration endpoint wraps AdminApiException in FastAPI/Flask.
  • Settings — safe int parsing for ADMIN_QUERY_MAX_LIMIT and ADMIN_SESSION_EXPIRED_AT so a blank/garbage env value no longer crashes the package at import.

Notes / out of scope

  • Pony m2m filtering, non-text search fields, and nested-path ordering (ORM-specific query construction) were left for a Pony-tested change.
  • Inline resolution by prefixed name was intentionally not changed — it conflicts with the tested behavior that a prefixed inline must not shadow a deleted top-level admin.
  • Actions are gated on has_change_permission; a dedicated has_action_permission hook could be added if read-only admins need to run non-mutating actions.

Security:
- Enforce has_change_permission on the action endpoint (authz bypass)
- Skip empty password values on edit so a blank field no longer overwrites
  the stored hash with hash("")
- Refuse to sign/verify JWTs with an unset/empty ADMIN_SECRET_KEY

Correctness:
- Fix falsy-zero PK/user-id traps in auth guards and orm_save_obj across all
  five ORM adapters (a legitimate id of 0 no longer misroutes)
- exclude now wins over list_display in serialization
- Malformed date/datetime and empty-condition filters return 422, not 500
- SQLAlchemy: real PK-name fallback for non-autoincrement PKs, cast-to-text
  for contains/icontains, PK excluded from required, no post-commit expired
  attribute read
- Falsy DB defaults no longer force required (Tortoise/Django/Yara); Tortoise
  enum options emit .value; Django choice label/value un-swapped
- Reject unsupported/null export format up front; add widget_action and
  configuration None/exception guards (FastAPI/Flask/Django); unify Django
  error responses under detail; 422 on malformed sign-in body
- Safe int parsing for ADMIN_QUERY_MAX_LIMIT and ADMIN_SESSION_EXPIRED_AT

@vsdudakov vsdudakov left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fable 5 review

Reviewed the full diff against main (12 files, +161/−69). Verdict: looks good to merge. The changes are localized, well-commented, and cross-consistent across the five ORM adapters. CI aside, locally the whole suite (2005 tests) passes and ruff + ty are clean.

Verified closely

  • if id is not None across all five adapters + SQLAlchemy m2m guard — correct fix for the falsy-0 PK trap; the update-vs-insert branch is preserved.
  • SQLAlchemy insert path — reading the generated PK after flush() and before commit() is the right way to stay safe under the default expire_on_commit=True; the update branch reuses the incoming id, so no post-commit attribute read remains.
  • required logic — the default is None (vs falsy) change plus not is_pk now matches the Yara reference in Tortoise/Django/SQLAlchemy; Django's NOT_PROVIDED sentinel handling is preserved.
  • get_fields_for_serialize reorder — subtracting exclude last makes it win over list_display without changing how list_display bypasses the fields allowlist. Good.
  • Enum/choice mappings — Tortoise v.value and the Django {label: v, value: k} swap are both correct against the underlying data shapes.
  • action authz gate — placed after context binding, before dispatch; default hooks return True so existing deployments are unaffected.

Non-blocking observations

  1. action is gated on has_change_permission only (upload_file / widget_action still auth-only). This is the right minimal fix for the confirmed bulk-mutation bypass, but it also means a read-only admin can no longer run a non-mutating registered action. If that's a supported use case, a dedicated has_action_permission hook would be cleaner. Fine to defer — just calling it out as a behavior change.
  2. Export text/plain/.txt default is now dead code — with the up-front format not in (CSV, JSON) guard, the initial content_type = "text/plain" / f"{model}.txt" can never survive. Harmless, but could be tidied.
  3. widget_action returns {} on a None response — strictly better than the previous asdict(None) 500, but worth a quick confirm that the frontend tolerates an empty object for a widget action that intentionally returns nothing.
  4. Malformed-body 422 was applied to sign_in only (the unauthenticated entry point); the other Django handlers that json.loads(request.body) still surface a 500 on garbage input. Reasonable scoping given they're authenticated, but the pattern is repeated if you want to sweep it later.

Correctly scoped out

Reverting the prefixed-inline lookup was the right call — the test suite encodes that a prefixed inline must not shadow a deleted top-level admin's 404. Deferring the Pony m2m/search/ordering fixes until they can be exercised against a live Pony backend is also sensible; a wrong query rewrite there would trade a loud 500 for silently wrong results.

No correctness regressions found in the diff.

- Cover the empty ADMIN_SECRET_KEY guards in sign_in and
  get_user_id_from_session_id
- Cover the unsupported/null export format 422
- Cover the invalid Date/DateTime deserialize 422 branches
- Cover the Django malformed sign-in body 422 and the FastAPI/Flask
  configuration AdminApiException handling
- Cover the _env_int blank/garbage fallbacks
- Revert the proactive SQLAlchemy m2m falsy-id guard (not a reported
  finding; kept the original truthiness check to preserve behavior)
- Add a has_action_permission hook (defaults to has_change_permission) and
  gate the action endpoint on it, so a read-only admin can be allowed to run
  a non-mutating action without granting change permission
- Remove the now-unreachable text/plain export default (format is guaranteed
  CSV or JSON by the up-front guard)
- Sweep the malformed-body 422 handling across all Django handlers via a
  shared _load_json_body helper (was previously only on sign_in)
- Tests: action 403 without change permission
@vsdudakov

Copy link
Copy Markdown
Owner Author

Addressed the review observations

  1. action permission granularity — added a dedicated has_action_permission hook on BaseModelAdmin that defaults to has_change_permission, and gated the action endpoint on it. Default behavior is unchanged (read-only admins are still blocked), but an admin can now override it to allow a read-only user to run a non-mutating action. Added a test asserting the gate returns 403 when change permission is denied.
  2. Dead export default — removed the unreachable text/plain/.txt initialization; payload.format is guaranteed CSV or JSON by the up-front guard, so it's now a clean if/else.
  3. widget_action {} on None — confirmed the frontend tolerates it: dashboard-chart-widget reads data?.data || [], so an empty object degrades to an empty chart rather than crashing. No change needed.
  4. Malformed-body 422 sweep — extracted a shared _load_json_body helper and applied it to every Django handler (sign_in, add, change, change_password, export, action, widget_action), so malformed JSON / bad payloads uniformly return 422 instead of 500.

Coverage stays at 100%; ruff, ruff format, and ty are clean.

@vsdudakov
vsdudakov merged commit e3b405a into main Jul 10, 2026
12 checks passed
@vsdudakov
vsdudakov deleted the fix/full-codebase-review-findings branch July 10, 2026 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant