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

* Fix 23 correctness and security issues from full-codebase review

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

* Add tests for new branches and restore 100% coverage

- 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)

* Address review observations

- 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

Author: vsdudakov

fastadmin/api/frameworks/django/app/api.py

@@ -3,6 +3,7 @@
 from dataclasses import asdict
 from datetime import datetime, time
 from functools import wraps
+from typing import Any
 from uuid import UUID
 
 from django.core.files.uploadedfile import UploadedFile
@@ -59,6 +60,20 @@ async def wrapped_view(*args, **kwargs):
     return wraps(view_func)(wrapped_view)
 
 
+def _load_json_body(request: HttpRequest, schema: type | None = None) -> Any:
+    """Parse the JSON request body, returning a clean 422 (not a 500) on bad input.
+
+    Unlike FastAPI/Flask, the Django views parse the body by hand, so malformed
+    JSON or (when ``schema`` is given) a missing/extra field would otherwise
+    surface as an unhandled 500. Raising AdminApiException keeps it a 422.
+    """
+    try:
+        data = json.loads(request.body)
+        return schema(**data) if schema is not None else data
+    except (json.JSONDecodeError, TypeError) as e:
+        raise AdminApiException(422, detail="Invalid request body.") from e
+
+
 @csrf_exempt
 async def sign_in(request: HttpRequest) -> JsonResponse:
     """This method is used to sign in.
@@ -70,7 +85,7 @@ async def sign_in(request: HttpRequest) -> JsonResponse:
     if request.method != "POST":
         return JsonResponse({"detail": "Method not allowed"}, status=405)
     try:
-        payload = SignInInputSchema(**json.loads(request.body))
+        payload = _load_json_body(request, SignInInputSchema)
         session_id = await api_service.sign_in(
             request.COOKIES.get(settings.ADMIN_SESSION_ID_KEY, None),
             payload,
@@ -120,12 +135,12 @@ async def me(request: HttpRequest) -> JsonResponse:
     :return: A user object.
     """
     if request.method != "GET":
-        return JsonResponse({"error": "Method not allowed"}, status=405)
+        return JsonResponse({"detail": "Method not allowed"}, status=405)
     try:
         user_id = await get_user_id_from_session_id(
             request.COOKIES.get(settings.ADMIN_SESSION_ID_KEY, None),
         )
-        if not user_id:
+        if user_id is None:
             raise AdminApiException(401, "User is not authenticated.")
         obj = await api_service.get(
             request.COOKIES.get(settings.ADMIN_SESSION_ID_KEY, None),
@@ -152,7 +167,7 @@ async def list_objs(request: HttpRequest, model: str) -> JsonResponse:
     :return: A list of objects.
     """
     if request.method != "GET":
-        return JsonResponse({"error": "Method not allowed"}, status=405)
+        return JsonResponse({"detail": "Method not allowed"}, status=405)
     try:
         search = request.GET.get("search") or None
         sort_by = request.GET.get("sort_by") or None
@@ -195,9 +210,9 @@ async def get(request: HttpRequest, model: str, id: UUID | int | str) -> JsonRes
     :return: An object.
     """
     if request.method != "GET":
-        return JsonResponse({"error": "Method not allowed"}, status=405)
+        return JsonResponse({"detail": "Method not allowed"}, status=405)
     if not is_valid_id(id):
-        return JsonResponse({"error": "Invalid id. It must be a UUID, an integer, or a non-empty string."}, status=422)
+        return JsonResponse({"detail": "Invalid id. It must be a UUID, an integer, or a non-empty string."}, status=422)
     try:
         obj = await api_service.get(
             request.COOKIES.get(settings.ADMIN_SESSION_ID_KEY, None),
@@ -220,12 +235,12 @@ async def add(request: HttpRequest, model: str) -> JsonResponse:
     :return: An object.
     """
     if request.method != "POST":
-        return JsonResponse({"error": "Method not allowed"}, status=405)
+        return JsonResponse({"detail": "Method not allowed"}, status=405)
     try:
         obj = await api_service.add(
             request.COOKIES.get(settings.ADMIN_SESSION_ID_KEY, None),
             model,
-            json.loads(request.body),
+            _load_json_body(request),
             request=request,
         )
         return JsonResponse(obj)
@@ -242,14 +257,14 @@ async def change_password(request: HttpRequest, id: UUID | int | str) -> JsonRes
     :return: An object.
     """
     if request.method != "PATCH":
-        return JsonResponse({"error": "Method not allowed"}, status=405)
+        return JsonResponse({"detail": "Method not allowed"}, status=405)
     if not is_valid_id(id):
-        return JsonResponse({"error": "Invalid id. It must be a UUID, an integer, or a non-empty string."}, status=422)
+        return JsonResponse({"detail": "Invalid id. It must be a UUID, an integer, or a non-empty string."}, status=422)
     try:
         await api_service.change_password(
             request.COOKIES.get(settings.ADMIN_SESSION_ID_KEY, None),
             id,
-            json.loads(request.body),
+            _load_json_body(request),
             request=request,
         )
         return JsonResponse(id, safe=False)
@@ -268,15 +283,15 @@ async def change(request: HttpRequest, model: str, id: UUID | int | str) -> Json
     :return: An object.
     """
     if request.method != "PATCH":
-        return JsonResponse({"error": "Method not allowed"}, status=405)
+        return JsonResponse({"detail": "Method not allowed"}, status=405)
     if not is_valid_id(id):
-        return JsonResponse({"error": "Invalid id. It must be a UUID, an integer, or a non-empty string."}, status=422)
+        return JsonResponse({"detail": "Invalid id. It must be a UUID, an integer, or a non-empty string."}, status=422)
     try:
         obj = await api_service.change(
             request.COOKIES.get(settings.ADMIN_SESSION_ID_KEY, None),
             model,
             id,
-            json.loads(request.body),
+            _load_json_body(request),
             request=request,
         )
         return JsonResponse(obj)
@@ -300,11 +315,11 @@ async def upload_file(
     """
 
     if request.method != "POST":
-        return JsonResponse({"error": "Method not allowed"}, status=405)
+        return JsonResponse({"detail": "Method not allowed"}, status=405)
     try:
         file: UploadedFile = request.FILES.get("file")
         if not file:
-            return JsonResponse({"error": "File not found"}, status=400)
+            return JsonResponse({"detail": "File not found"}, status=400)
         file_name = file.name
         file_content = file.read()
         obj_id = request.GET.get("id") or None
@@ -334,7 +349,7 @@ async def export(request: HttpRequest, model: str) -> JsonResponse | StreamingHt
     :return: A stream of export data.
     """
     if request.method != "POST":
-        return JsonResponse({"error": "Method not allowed"}, status=405)
+        return JsonResponse({"detail": "Method not allowed"}, status=405)
     search = request.GET.get("search") or None
     sort_by = request.GET.get("sort_by") or None
     list_filters = parse_list_filters_from_query_params(
@@ -343,7 +358,7 @@ async def export(request: HttpRequest, model: str) -> JsonResponse | StreamingHt
         exclude={"search", "sort_by", "offset", "limit"},
     )
     try:
-        payload = ExportInputSchema(**json.loads(request.body))
+        payload = _load_json_body(request, ExportInputSchema)
         file_name, content_type, stream = await api_service.export(
             request.COOKIES.get(settings.ADMIN_SESSION_ID_KEY, None),
             model,
@@ -374,9 +389,9 @@ async def delete(
     :return: An id of object.
     """
     if request.method != "DELETE":
-        return JsonResponse({"error": "Method not allowed"}, status=405)
+        return JsonResponse({"detail": "Method not allowed"}, status=405)
     if not is_valid_id(id):
-        return JsonResponse({"error": "Invalid id. It must be a UUID, an integer, or a non-empty string."}, status=422)
+        return JsonResponse({"detail": "Invalid id. It must be a UUID, an integer, or a non-empty string."}, status=422)
     try:
         deleted_id = await api_service.delete(
             request.COOKIES.get(settings.ADMIN_SESSION_ID_KEY, None),
@@ -404,9 +419,9 @@ async def action(
     :return: action result.
     """
     if request.method != "POST":
-        return JsonResponse({"error": "Method not allowed"}, status=405)
+        return JsonResponse({"detail": "Method not allowed"}, status=405)
     try:
-        payload = ActionInputSchema(**json.loads(request.body))
+        payload = _load_json_body(request, ActionInputSchema)
         response = await api_service.action(
             request.COOKIES.get(settings.ADMIN_SESSION_ID_KEY, None),
             model,
@@ -438,17 +453,17 @@ async def widget_action(
     :return: widget action result.
     """
     if request.method != "POST":
-        return JsonResponse({"error": "Method not allowed"}, status=405)
+        return JsonResponse({"detail": "Method not allowed"}, status=405)
     try:
-        payload = WidgetActionInputSchema(**json.loads(request.body))
+        payload = _load_json_body(request, WidgetActionInputSchema)
         response = await api_service.widget_action(
             request.COOKIES.get(settings.ADMIN_SESSION_ID_KEY, None),
             model,
             widget_action,
             payload,
             request=request,
         )
-        return JsonResponse(asdict(response))
+        return JsonResponse(asdict(response) if response is not None else {})
     except AdminApiException as e:
         return JsonResponse({"detail": e.detail}, status=e.status_code)
 
@@ -461,7 +476,7 @@ async def configuration(request: HttpRequest) -> JsonResponse:
     :return: A configuration.
     """
     if request.method != "GET":
-        return JsonResponse({"error": "Method not allowed"}, status=405)
+        return JsonResponse({"detail": "Method not allowed"}, status=405)
 
     obj = await api_service.get_configuration(
         request.COOKIES.get(settings.ADMIN_SESSION_ID_KEY, None),

fastadmin/api/frameworks/fastapi/api.py

@@ -398,7 +398,7 @@ async def widget_action(
             payload,
             request=request,
         )
-        return asdict(response)
+        return asdict(response) if response is not None else {}
     except AdminApiException as e:
         raise HTTPException(e.status_code, detail=e.detail) from None
 
@@ -412,7 +412,10 @@ async def configuration(
     :params user_id: an id of user.
     :return: A configuration.
     """
-    return await api_service.get_configuration(
-        request.cookies.get(settings.ADMIN_SESSION_ID_KEY, None),
-        request=request,
-    )
+    try:
+        return await api_service.get_configuration(
+            request.cookies.get(settings.ADMIN_SESSION_ID_KEY, None),
+            request=request,
+        )
+    except AdminApiException as e:
+        raise HTTPException(e.status_code, detail=e.detail) from None

fastadmin/api/frameworks/flask/api.py

@@ -412,7 +412,7 @@ async def widget_action(
             payload,
             request=request,
         )
-        return make_response(asdict(response))
+        return make_response(asdict(response) if response is not None else {})
     except AdminApiException as e:
         http_exception = HTTPException(e.detail)
         http_exception.code = e.status_code
@@ -426,8 +426,13 @@ async def configuration() -> dict:
     :params user_id: an id of user.
     :return: A configuration.
     """
-    obj = await api_service.get_configuration(
-        request.cookies.get(settings.ADMIN_SESSION_ID_KEY, None),
-        request=request,
-    )
+    try:
+        obj = await api_service.get_configuration(
+            request.cookies.get(settings.ADMIN_SESSION_ID_KEY, None),
+            request=request,
+        )
+    except AdminApiException as e:
+        http_exception = HTTPException(e.detail)
+        http_exception.code = e.status_code
+        raise http_exception from e
     return asdict(obj)

fastadmin/api/helpers.py

@@ -88,9 +88,12 @@ def sanitize_filter_key(key: str, fields: list[ModelFieldWidgetSchema]) -> tuple
     :param fields: A list of fields.
     :return: A tuple of sanitized key and condition.
     """
-    if "__" not in key:
-        key += "__exact"
     field_name, _, condition = key.partition("__")
+    # No suffix ("name") or a trailing "__" with an empty condition ("name__")
+    # both mean an exact lookup; without this the empty condition reaches the ORM
+    # as a broken lookup (e.g. Django ``name__``) and 500s the request.
+    if not condition:
+        condition = "exact"
     field: ModelFieldWidgetSchema | None = next((field for field in fields if field.name == field_name), None)
     if field and field.filter_widget_props.get("parentModel") and not field.is_m2m:
         field_name += "_id"

fastadmin/api/service.py

@@ -59,6 +59,11 @@ async def get_user_id_from_session_id(session_id: str | None) -> UUID | int | No
     if not admin_model:
         return None
 
+    # An empty/unset secret makes HS256 signatures trivially forgeable, so refuse
+    # to validate any token rather than accept one signed with a blank key.
+    if not settings.ADMIN_SECRET_KEY:
+        return None
+
     try:
         token_payload = jwt.decode(session_id, settings.ADMIN_SECRET_KEY, algorithms=["HS256"])
     except jwt.PyJWTError:
@@ -72,7 +77,7 @@ async def get_user_id_from_session_id(session_id: str | None) -> UUID | int | No
         return None
 
     user_id = token_payload.get("user_id")
-    if not user_id:
+    if user_id is None:
         return None
 
     if not await admin_model.get_obj(user_id):
@@ -166,6 +171,9 @@ async def sign_in(
         if not admin_model:
             raise AdminApiException(401, detail=f"{model} model is not registered.")
 
+        if not settings.ADMIN_SECRET_KEY:
+            raise AdminApiException(500, detail="Server misconfiguration: ADMIN_SECRET_KEY is not set.")
+
         if inspect.iscoroutinefunction(admin_model.authenticate):
             authenticate_fn = admin_model.authenticate
         else:
@@ -174,7 +182,7 @@ async def sign_in(
         self._bind_admin_context(admin_model, request=request, user=None)
         user_id = await authenticate_fn(payload.username, payload.password)
 
-        if not user_id or not isinstance(user_id, int | UUID):
+        if isinstance(user_id, bool) or not isinstance(user_id, int | UUID):
             raise AdminApiException(401, detail="Invalid credentials.")
 
         now = datetime.now(UTC)
@@ -449,6 +457,11 @@ async def export(
         self._bind_admin_context(admin_model, request=request, user=current_user)
         await self._require_permission(admin_model, "has_export_permission", current_user_id)
 
+        # Reject an unsupported/null format up front: otherwise get_export returns
+        # None and the framework layer wraps None in a StreamingResponse and 500s.
+        if payload.format not in (ExportFormat.CSV, ExportFormat.JSON):
+            raise AdminApiException(422, detail="Unsupported export format.")
+
         # validations
         fields = set(admin_model.get_fields_for_serialize())
 
@@ -475,12 +488,11 @@ async def export(
                 if not is_allowed_field_or_path(ordering_field.strip("-"), fields):
                     raise AdminApiException(422, detail=f"Sort by {ordering_field} is not allowed")
 
-        content_type = "text/plain"
-        file_name = f"{model}.txt"
+        # payload.format is guaranteed to be CSV or JSON by the guard above.
         if payload.format == ExportFormat.CSV:
             content_type = "text/csv"
             file_name = f"{model}.csv"
-        elif payload.format == ExportFormat.JSON:
+        else:
             content_type = "text/plain"
             file_name = f"{model}.json"
         return (
@@ -528,12 +540,16 @@ async def action(
         payload: ActionInputSchema,
         request: Any | None = None,
     ) -> ActionResponseSchema | None:
-        _current_user_id, current_user = await self._get_authenticated_user(session_id)
+        current_user_id, current_user = await self._get_authenticated_user(session_id)
 
         admin_model = get_admin_or_admin_inline_model(model)
         if not admin_model:
             raise AdminApiException(404, detail=f"{model} model is not registered.")
         self._bind_admin_context(admin_model, request=request, user=current_user)
+        # Actions run bulk mutations over the selected ids, so they must be gated
+        # server-side — otherwise a read-only admin could mutate records through a
+        # registered action. has_action_permission defaults to has_change_permission.
+        await self._require_permission(admin_model, "has_action_permission", current_user_id)
 
         if action not in admin_model.actions:
             raise AdminApiException(422, detail=f"{action} action is not in actions setting.")