From 9cc5d21613524d1a5af23256b27ab6f393df913d Mon Sep 17 00:00:00 2001 From: Seva D Date: Fri, 10 Jul 2026 12:30:44 +0400 Subject: [PATCH 1/3] 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 --- fastadmin/api/frameworks/django/app/api.py | 43 ++++++++++++---------- fastadmin/api/frameworks/fastapi/api.py | 13 ++++--- fastadmin/api/frameworks/flask/api.py | 15 +++++--- fastadmin/api/helpers.py | 7 +++- fastadmin/api/service.py | 23 ++++++++++-- fastadmin/models/base.py | 23 +++++++++--- fastadmin/models/orms/django.py | 23 +++++++----- fastadmin/models/orms/ponyorm.py | 2 +- fastadmin/models/orms/sqlalchemy.py | 38 ++++++++++++++----- fastadmin/models/orms/tortoise.py | 16 +++++--- fastadmin/models/orms/yaraorm.py | 8 ++-- fastadmin/settings.py | 19 +++++++++- 12 files changed, 161 insertions(+), 69 deletions(-) diff --git a/fastadmin/api/frameworks/django/app/api.py b/fastadmin/api/frameworks/django/app/api.py index db145bf..c440365 100644 --- a/fastadmin/api/frameworks/django/app/api.py +++ b/fastadmin/api/frameworks/django/app/api.py @@ -70,7 +70,10 @@ 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)) + try: + payload = SignInInputSchema(**json.loads(request.body)) + except (json.JSONDecodeError, TypeError) as e: + raise AdminApiException(422, detail="Invalid request body.") from e session_id = await api_service.sign_in( request.COOKIES.get(settings.ADMIN_SESSION_ID_KEY, None), payload, @@ -120,12 +123,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 +155,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 +198,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,7 +223,7 @@ 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), @@ -242,9 +245,9 @@ 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), @@ -268,9 +271,9 @@ 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), @@ -300,11 +303,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 +337,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( @@ -374,9 +377,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,7 +407,7 @@ 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)) response = await api_service.action( @@ -438,7 +441,7 @@ 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)) response = await api_service.widget_action( @@ -448,7 +451,7 @@ async def 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 +464,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), diff --git a/fastadmin/api/frameworks/fastapi/api.py b/fastadmin/api/frameworks/fastapi/api.py index 827dadd..2b5cd03 100644 --- a/fastadmin/api/frameworks/fastapi/api.py +++ b/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 diff --git a/fastadmin/api/frameworks/flask/api.py b/fastadmin/api/frameworks/flask/api.py index f77e0ad..6e660df 100644 --- a/fastadmin/api/frameworks/flask/api.py +++ b/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) diff --git a/fastadmin/api/helpers.py b/fastadmin/api/helpers.py index 8c3c1b4..b6c476a 100644 --- a/fastadmin/api/helpers.py +++ b/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" diff --git a/fastadmin/api/service.py b/fastadmin/api/service.py index 6a3108f..7a47db1 100644 --- a/fastadmin/api/service.py +++ b/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()) @@ -528,12 +541,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 honor the + # same change-permission gate as the change/delete endpoints — otherwise a + # read-only admin could mutate records through a registered action. + await self._require_permission(admin_model, "has_change_permission", current_user_id) if action not in admin_model.actions: raise AdminApiException(422, detail=f"{action} action is not in actions setting.") diff --git a/fastadmin/models/base.py b/fastadmin/models/base.py index 3e580db..cf65609 100644 --- a/fastadmin/models/base.py +++ b/fastadmin/models/base.py @@ -12,6 +12,7 @@ from asgiref.sync import sync_to_async from fastadmin.api.encoders import apply_custom_encoders +from fastadmin.api.exceptions import AdminApiException from fastadmin.api.schemas import ExportFormat from fastadmin.models.schemas import ModelFieldWidgetSchema, WidgetType @@ -367,10 +368,13 @@ def get_fields_for_serialize(self) -> set[str]: fields_for_serialize = {field.name for field in fields} if self.fields: fields_for_serialize &= set(self.fields) - if self.exclude: - fields_for_serialize -= set(self.exclude) if self.list_display: fields_for_serialize |= set(self.list_display) + # exclude must win over list_display: a field the admin hid via exclude + # must not be re-added (and serialized) just because it also appears in + # list_display, so subtract exclude last. + if self.exclude: + fields_for_serialize -= set(self.exclude) return fields_for_serialize def get_writable_field_names(self) -> set[str]: @@ -504,9 +508,15 @@ def deserialize_value(self, field: ModelFieldWidgetSchema, value: Any) -> Any: except ValueError: return datetime.time.fromisoformat(value) case WidgetType.DatePicker: - return datetime.datetime.fromisoformat(value).date() + try: + return datetime.datetime.fromisoformat(value).date() + except ValueError as e: + raise AdminApiException(422, detail=f"Invalid date value for {field.name}.") from e case WidgetType.DateTimePicker: - return datetime.datetime.fromisoformat(value) + try: + return datetime.datetime.fromisoformat(value) + except ValueError as e: + raise AdminApiException(422, detail=f"Invalid datetime value for {field.name}.") from e case _: return value @@ -811,7 +821,10 @@ async def save_model(self, id: UUID | int | str | None, payload: dict) -> dict | # hashing here overwrites it — the column never keeps a plaintext value. pk_name = self.get_model_pk_name(self.model_cls) pk = obj[pk_name] - password_values = [payload[field] for field in password_fields if field in payload] + # Only (re)hash when a non-empty password was submitted. An empty field + # on the edit form means "leave the password unchanged"; hashing "" here + # would overwrite the stored hash with hash("") and let anyone sign in. + password_values = [payload[field] for field in password_fields if payload.get(field)] if password_values: await self.change_password(pk, password_values[0]) return obj diff --git a/fastadmin/models/orms/django.py b/fastadmin/models/orms/django.py index 189750c..7ddbcf1 100644 --- a/fastadmin/models/orms/django.py +++ b/fastadmin/models/orms/django.py @@ -51,11 +51,14 @@ def get_model_fields_with_widget_types( is_pk or getattr(orm_model_field, "auto_now", False) or getattr(orm_model_field, "auto_now_add", False) ) and field_name not in self.readonly_fields - has_default = getattr(orm_model_field, "default", False) - if hasattr(has_default, "__name__") and has_default.__name__ == "NOT_PROVIDED": - has_default = False - - required = not getattr(orm_model_field, "null", False) and not has_default and not is_m2m + default = getattr(orm_model_field, "default", None) + # Django signals "no default" with the NOT_PROVIDED sentinel; a real + # default of 0/False/"" must still count as provided (otherwise the + # field is wrongly marked required). + is_not_provided = hasattr(default, "__name__") and default.__name__ == "NOT_PROVIDED" + has_default = default is not None and not is_not_provided + + required = not getattr(orm_model_field, "null", False) and not has_default and not is_m2m and not is_pk choices = ( {item[0]: item[1] for item in orm_model_field.choices} if getattr(orm_model_field, "choices", None) @@ -77,8 +80,10 @@ def get_model_fields_with_widget_types( match field_type: case "CharField": if choices is not None: - form_widget_props["options"] = [{"label": k, "value": v} for k, v in choices.items()] - filter_widget_props["options"] = [{"label": k, "value": v} for k, v in choices.items()] + # choices maps db_value -> human_label, so the option label + # is the human label (v) and the stored value is the db key (k). + form_widget_props["options"] = [{"label": v, "value": k} for k, v in choices.items()] + filter_widget_props["options"] = [{"label": v, "value": k} for k, v in choices.items()] if field_name in self.radio_fields: form_widget_type = WidgetType.RadioGroup filter_widget_type = WidgetType.CheckboxGroup @@ -296,7 +301,7 @@ def orm_save_obj(self, id: UUID | Any | None, payload: dict) -> Any: :params payload: a dict of payload. :return: An object. """ - if id: + if id is not None: obj = self.model_cls.objects.filter(**{self.get_model_pk_name(self.model_cls): id}).first() if not obj: return None @@ -304,7 +309,7 @@ def orm_save_obj(self, id: UUID | Any | None, payload: dict) -> Any: setattr(obj, k, v) else: obj = self.model_cls(**payload) - obj.save(update_fields=payload.keys() if id else None) + obj.save(update_fields=payload.keys() if id is not None else None) return obj @sync_to_async diff --git a/fastadmin/models/orms/ponyorm.py b/fastadmin/models/orms/ponyorm.py index 20f34b5..bdf70a3 100644 --- a/fastadmin/models/orms/ponyorm.py +++ b/fastadmin/models/orms/ponyorm.py @@ -382,7 +382,7 @@ def orm_save_obj(self, id: UUID | Any | None, payload: dict) -> Any: :params payload: a dict of payload. :return: An object. """ - if id: + if id is not None: obj = self.model_cls.select(**{self.get_model_pk_name(self.model_cls): id}).first() if not obj: return None diff --git a/fastadmin/models/orms/sqlalchemy.py b/fastadmin/models/orms/sqlalchemy.py index 240f07f..987998f 100644 --- a/fastadmin/models/orms/sqlalchemy.py +++ b/fastadmin/models/orms/sqlalchemy.py @@ -2,7 +2,7 @@ from typing import Any from uuid import UUID -from sqlalchemy import BIGINT, Integer, and_, func, inspect, or_, select +from sqlalchemy import BIGINT, Integer, String, and_, cast, func, inspect, or_, select from sqlalchemy.orm import selectinload from fastadmin.models.base import InlineModelAdmin, ModelAdmin @@ -90,7 +90,13 @@ def get_model_pk_name(orm_model_cls: Any) -> str: :return: A str. """ - return getattrs(orm_model_cls, "__table__.primary_key._autoincrement_column.name", default="id") + # _autoincrement_column is None for non-autoincrement PKs (UUID, String, + # or explicit autoincrement=False), so fall back to the actual primary-key + # column rather than a hardcoded "id" that then AttributeErrors downstream. + pk_columns = list(getattrs(orm_model_cls, "__table__.primary_key.columns", default=[])) + if pk_columns: + return pk_columns[0].name + return "id" def get_model_fields_with_widget_types( self, @@ -141,7 +147,7 @@ def get_model_fields_with_widget_types( fk_column = next((c for c in mapper.c if c.key == column_name), None) if fk_column is not None: nullable = getattr(fk_column, "nullable", False) - required = not nullable and not getattr(orm_model_field, "default", False) and not is_m2m + required = not nullable and not getattr(orm_model_field, "default", False) and not is_m2m and not is_pk choices = ( orm_model_field.type._object_lookup if hasattr(orm_model_field, "type") and hasattr(orm_model_field.type, "_object_lookup") @@ -358,7 +364,12 @@ def order_column(ordering_field: str): q.append(model_field.has(match_expr)) continue - if condition != "in" and isinstance(model_field.expression.type, BIGINT | Integer): + # Only numeric comparisons coerce to int; contains/icontains + # stay string substring matches (coercing them would build a + # LIKE against an int and error on Postgres). + if condition in ("exact", "lt", "lte", "gt", "gte") and isinstance( + model_field.expression.type, BIGINT | Integer + ): with contextlib.suppress(ValueError, TypeError): value = int(value) @@ -379,9 +390,9 @@ def order_column(ordering_field: str): value = [int(x) for x in value] # ty: ignore[not-iterable] q.append(model_field.in_(value)) case "contains": - q.append(model_field.like(f"%{_escape_like(value)}%", escape="\\")) + q.append(cast(model_field, String).like(f"%{_escape_like(value)}%", escape="\\")) case "icontains": - q.append(model_field.ilike(f"%{_escape_like(value)}%", escape="\\")) + q.append(cast(model_field, String).ilike(f"%{_escape_like(value)}%", escape="\\")) qs = qs.where(and_(*q)) search_fields = list(self.search_fields) @@ -460,7 +471,10 @@ async def orm_save_obj(self, id: UUID | Any | None, payload: dict) -> Any: sessionmaker = self.get_sessionmaker() async with sessionmaker() as session: - if id: + pk_name = self.get_model_pk_name(self.model_cls) + # `is not None` (not truthiness) so a legitimate primary key of 0 still + # updates instead of falling through to an insert. + if id is not None: obj = await session.get(self.model_cls, id) if not obj: return None @@ -468,11 +482,17 @@ async def orm_save_obj(self, id: UUID | Any | None, payload: dict) -> Any: setattr(obj, k, v) await session.merge(obj) await session.commit() + pk_value = id else: obj = self.model_cls(**payload) session.add(obj) + # Read the generated pk after flush but before commit: with the + # SQLAlchemy default expire_on_commit=True, reading an attribute + # after commit triggers a sync refresh that fails in async context. + await session.flush() + pk_value = getattr(obj, pk_name) await session.commit() - return await session.get(self.model_cls, getattr(obj, self.get_model_pk_name(self.model_cls))) + return await session.get(self.model_cls, pk_value) async def orm_delete_obj(self, id: UUID | int | str) -> None: """This method is used to delete orm/db model object. @@ -529,7 +549,7 @@ async def orm_save_m2m_ids(self, obj: Any, field: str, ids: list[int | str | UUI values = [] id_key = self.get_model_pk_name(self.model_cls) obj_id = getattr(obj, id_key) - if not obj_id: + if obj_id is None: return obj_field_name = orm_model_field.synchronize_pairs[0][1].key rel_field_name = orm_model_field.secondary_synchronize_pairs[0][1].key diff --git a/fastadmin/models/orms/tortoise.py b/fastadmin/models/orms/tortoise.py index 7dc0ea1..64b6deb 100644 --- a/fastadmin/models/orms/tortoise.py +++ b/fastadmin/models/orms/tortoise.py @@ -85,8 +85,11 @@ def get_model_fields_with_widget_types( ) and field_name not in self.readonly_fields required = ( not getattr(orm_model_field, "null", False) - and not getattr(orm_model_field, "default", False) + # `is None` (not falsy) so a valid falsy default (0, False, "") + # is still recognized as a default and the field is not required. + and getattr(orm_model_field, "default", None) is None and not is_m2m + and not is_pk ) choices = ( orm_model_field.enum_type._member_map_ @@ -147,8 +150,11 @@ def get_model_fields_with_widget_types( filter_widget_props["format"] = settings.ADMIN_TIME_FORMAT filter_widget_props["showTime"] = True case "CharEnumFieldInstance" | "IntEnumFieldInstance": - form_widget_props["options"] = [{"label": k, "value": v} for k, v in choices.items()] - filter_widget_props["options"] = [{"label": k, "value": v} for k, v in choices.items()] + # choices maps name -> Enum member; emit the member's scalar + # .value (as the Yara adapter does) so the option value is the + # stored value and is JSON-serializable. + form_widget_props["options"] = [{"label": k, "value": v.value} for k, v in choices.items()] + filter_widget_props["options"] = [{"label": k, "value": v.value} for k, v in choices.items()] if field_name in self.radio_fields: form_widget_type = WidgetType.RadioGroup filter_widget_type = WidgetType.CheckboxGroup @@ -306,7 +312,7 @@ async def orm_save_obj(self, id: UUID | Any | None, payload: dict) -> Any: :params payload: a dict of payload. :return: An object. """ - if id: + if id is not None: obj = await self.model_cls.filter(**{self.get_model_pk_name(self.model_cls): id}).first() if not obj: return None @@ -314,7 +320,7 @@ async def orm_save_obj(self, id: UUID | Any | None, payload: dict) -> Any: setattr(obj, k, v) else: obj = self.model_cls(**payload) - await obj.save(update_fields=payload.keys() if id else None) + await obj.save(update_fields=payload.keys() if id is not None else None) return obj async def orm_delete_obj(self, id: UUID | int | str) -> None: diff --git a/fastadmin/models/orms/yaraorm.py b/fastadmin/models/orms/yaraorm.py index b22c7a5..98fddee 100644 --- a/fastadmin/models/orms/yaraorm.py +++ b/fastadmin/models/orms/yaraorm.py @@ -118,7 +118,9 @@ def get_model_fields_with_widget_types( ) and field_name not in self.readonly_fields required = ( not getattr(orm_model_field, "null", False) - and not getattr(orm_model_field, "default", None) + # `is None` (not falsy) so a valid falsy default (0, False, "") + # is still recognized as a default and the field is not required. + and getattr(orm_model_field, "default", None) is None and not is_pk and not is_m2m ) @@ -288,7 +290,7 @@ async def orm_save_obj(self, id: UUID | Any | None, payload: dict) -> Any: :params payload: a dict of payload. :return: An object. """ - if id: + if id is not None: obj = await self.model_cls.filter(**{self.get_model_pk_name(self.model_cls): id}).first() if not obj: return None @@ -296,7 +298,7 @@ async def orm_save_obj(self, id: UUID | Any | None, payload: dict) -> Any: setattr(obj, k, v) else: obj = self.model_cls(**payload) - await obj.save(update_fields=list(payload.keys()) if id else None) + await obj.save(update_fields=list(payload.keys()) if id is not None else None) return obj async def orm_delete_obj(self, id: UUID | int | str) -> None: diff --git a/fastadmin/settings.py b/fastadmin/settings.py index c122bb4..9526c97 100644 --- a/fastadmin/settings.py +++ b/fastadmin/settings.py @@ -4,6 +4,21 @@ ROOT_DIR = Path(__file__).resolve().parent +def _env_int(name: str, default: int) -> int: + """Read an int env var, falling back to the default when it is unset, blank, or non-numeric. + + ``int(os.getenv(...))`` crashes the whole package at import time on an empty + (``NAME=``) or garbage value, so parse defensively instead. + """ + raw = os.getenv(name) + if raw is None or raw.strip() == "": + return default + try: + return int(raw) + except ValueError: + return default + + class Settings: """Settings""" @@ -29,7 +44,7 @@ class Settings: ADMIN_SESSION_ID_KEY: str = os.getenv("ADMIN_SESSION_ID_KEY", "admin_session_id") # This value is the expired_at period (in sec) for session id. - ADMIN_SESSION_EXPIRED_AT: int = int(os.getenv("ADMIN_SESSION_EXPIRED_AT", 144000)) # in sec + ADMIN_SESSION_EXPIRED_AT: int = _env_int("ADMIN_SESSION_EXPIRED_AT", 144000) # in sec # Set the Secure flag on the session cookie so it is only sent over HTTPS. # Enabled by default; set ADMIN_SESSION_COOKIE_SECURE=false for local HTTP dev. @@ -41,7 +56,7 @@ class Settings: # Hard upper bound on the number of rows a single list/export request may # return. Caps memory/CPU use from a crafted limit=100000000 request. - ADMIN_QUERY_MAX_LIMIT: int = int(os.getenv("ADMIN_QUERY_MAX_LIMIT", 1000)) + ADMIN_QUERY_MAX_LIMIT: int = _env_int("ADMIN_QUERY_MAX_LIMIT", 1000) # This value is the date format for JS widgets. ADMIN_DATE_FORMAT: str = os.getenv("ADMIN_DATE_FORMAT", "YYYY-MM-DD") From 3b488a229394ca1e77389a95d376ee911f359f31 Mon Sep 17 00:00:00 2001 From: Seva D Date: Fri, 10 Jul 2026 12:49:26 +0400 Subject: [PATCH 2/3] 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) --- fastadmin/models/orms/sqlalchemy.py | 2 +- tests/api/frameworks/django/test_app.py | 12 ++++++++++++ tests/api/frameworks/fastapi/test_app.py | 24 ++++++++++++++++++++++++ tests/api/frameworks/flask/test_app.py | 22 ++++++++++++++++++++++ tests/api/test_export.py | 9 +++++++++ tests/api/test_helpers.py | 15 +++++++++++++++ tests/api/test_service.py | 20 ++++++++++++++++++++ tests/models/test_base.py | 21 +++++++++++++++++++++ 8 files changed, 124 insertions(+), 1 deletion(-) diff --git a/fastadmin/models/orms/sqlalchemy.py b/fastadmin/models/orms/sqlalchemy.py index 987998f..2ad29f5 100644 --- a/fastadmin/models/orms/sqlalchemy.py +++ b/fastadmin/models/orms/sqlalchemy.py @@ -549,7 +549,7 @@ async def orm_save_m2m_ids(self, obj: Any, field: str, ids: list[int | str | UUI values = [] id_key = self.get_model_pk_name(self.model_cls) obj_id = getattr(obj, id_key) - if obj_id is None: + if not obj_id: return obj_field_name = orm_model_field.synchronize_pairs[0][1].key rel_field_name = orm_model_field.secondary_synchronize_pairs[0][1].key diff --git a/tests/api/frameworks/django/test_app.py b/tests/api/frameworks/django/test_app.py index 247058f..ce15460 100644 --- a/tests/api/frameworks/django/test_app.py +++ b/tests/api/frameworks/django/test_app.py @@ -180,3 +180,15 @@ async def test_django_upload_file_success(): id=None, request=request, ) + + +async def test_django_sign_in_invalid_body_422(): + """sign_in returns 422 (not 500) when the request body is not valid JSON.""" + from fastadmin.api.frameworks.django.app.api import sign_in + + request = MagicMock() + request.method = "POST" + request.body = b"{" # malformed JSON + + response = await sign_in(request) + assert response.status_code == 422 diff --git a/tests/api/frameworks/fastapi/test_app.py b/tests/api/frameworks/fastapi/test_app.py index 1e69fee..2ffe2a0 100644 --- a/tests/api/frameworks/fastapi/test_app.py +++ b/tests/api/frameworks/fastapi/test_app.py @@ -90,3 +90,27 @@ async def test_fastapi_widget_action_admin_exception(): payload, request=request, ) + + +async def test_fastapi_configuration_admin_exception(): + """FastAPI configuration endpoint converts AdminApiException to HTTPException.""" + from fastapi import HTTPException + + from fastadmin.api.exceptions import AdminApiException + from fastadmin.api.frameworks.fastapi import api as fastapi_api + from fastadmin.settings import settings + + request = MagicMock() + request.cookies = {settings.ADMIN_SESSION_ID_KEY: "sid"} + + with ( + pytest.raises(HTTPException) as exc_info, + patch.object( + fastapi_api.api_service, + "get_configuration", + AsyncMock(side_effect=AdminApiException(418, detail="boom")), + ), + ): + await fastapi_api.configuration(request) + + assert exc_info.value.status_code == 418 diff --git a/tests/api/frameworks/flask/test_app.py b/tests/api/frameworks/flask/test_app.py index 8f9c139..3101e73 100644 --- a/tests/api/frameworks/flask/test_app.py +++ b/tests/api/frameworks/flask/test_app.py @@ -218,3 +218,25 @@ async def test_flask_upload_file_admin_exception(): await flask_api.upload_file("Event", "file") assert exc_info.value.code == 500 assert "upload failed" in str(exc_info.value.description) + + +async def test_flask_configuration_admin_exception(): + """Flask configuration endpoint re-raises AdminApiException as HTTPException.""" + from unittest.mock import AsyncMock, patch + + from fastadmin.api.exceptions import AdminApiException + from fastadmin.api.frameworks.flask import api as flask_api + from tests.environment.flask_app.dev import app as flask_app + + with ( + flask_app.test_request_context(path="/api/configuration", method="GET"), + patch.object( + flask_api.api_service, + "get_configuration", + AsyncMock(side_effect=AdminApiException(418, detail="boom")), + ), + pytest.raises(HTTPException) as exc_info, + ): + await flask_api.configuration() + + assert exc_info.value.code == 418 diff --git a/tests/api/test_export.py b/tests/api/test_export.py index 27fcf6a..61045fe 100644 --- a/tests/api/test_export.py +++ b/tests/api/test_export.py @@ -29,6 +29,15 @@ async def test_export_401(event, client): assert r.status_code == 401, r.text +async def test_export_invalid_format_422(session_id, event, client): + assert session_id + r = await client.post( + f"/api/export/{event.get_model_name()}", + json={"format": None}, + ) + assert r.status_code == 422, r.text + + async def test_export_404(session_id, admin_models, event, client): assert session_id del admin_models[event.__class__] diff --git a/tests/api/test_helpers.py b/tests/api/test_helpers.py index 1de0024..d1f15c0 100644 --- a/tests/api/test_helpers.py +++ b/tests/api/test_helpers.py @@ -209,3 +209,18 @@ async def test_get_template(tmp_path): template.write_text("Hello {{name}}, count={{count}}") out = get_template(template, {"name": "World", "count": 42}) assert out == "Hello World, count=42" + + +def test_env_int_falls_back_on_blank_or_garbage(monkeypatch): + from fastadmin.settings import _env_int + + monkeypatch.delenv("FASTADMIN_TEST_INT", raising=False) + assert _env_int("FASTADMIN_TEST_INT", 7) == 7 # unset + monkeypatch.setenv("FASTADMIN_TEST_INT", "") + assert _env_int("FASTADMIN_TEST_INT", 7) == 7 # blank + monkeypatch.setenv("FASTADMIN_TEST_INT", " ") + assert _env_int("FASTADMIN_TEST_INT", 7) == 7 # whitespace + monkeypatch.setenv("FASTADMIN_TEST_INT", "not-an-int") + assert _env_int("FASTADMIN_TEST_INT", 7) == 7 # non-numeric + monkeypatch.setenv("FASTADMIN_TEST_INT", "42") + assert _env_int("FASTADMIN_TEST_INT", 7) == 42 # valid diff --git a/tests/api/test_service.py b/tests/api/test_service.py index e75dea6..7507f6d 100644 --- a/tests/api/test_service.py +++ b/tests/api/test_service.py @@ -39,6 +39,26 @@ async def test_get_user_id_from_session_id_without_user_id(monkeypatch): assert await get_user_id_from_session_id(token) is None +async def test_get_user_id_from_session_id_empty_secret(monkeypatch): + """An empty ADMIN_SECRET_KEY refuses to validate any token (forgery guard).""" + admin_model = SimpleNamespace(get_obj=AsyncMock(return_value={"id": 1})) + monkeypatch.setattr("fastadmin.api.service.get_admin_model", lambda _model: admin_model) + monkeypatch.setattr(settings, "ADMIN_SECRET_KEY", "") + + assert await get_user_id_from_session_id("any-token") is None + + +async def test_sign_in_empty_secret_raises(monkeypatch): + """sign_in refuses to issue a token when ADMIN_SECRET_KEY is unset/empty.""" + admin_model = SimpleNamespace(authenticate=AsyncMock(return_value=1)) + monkeypatch.setattr("fastadmin.api.service.get_admin_model", lambda _model: admin_model) + monkeypatch.setattr(settings, "ADMIN_SECRET_KEY", "") + + with pytest.raises(AdminApiException) as exc: + await ApiService().sign_in(None, SignInInputSchema(username="u", password="p")) + assert exc.value.status_code == 500 + + async def test_sign_in_converts_uuid_to_string(monkeypatch): user_id = uuid4() admin_model = SimpleNamespace(authenticate=AsyncMock(return_value=user_id)) diff --git a/tests/models/test_base.py b/tests/models/test_base.py index d6d877e..8331ddc 100644 --- a/tests/models/test_base.py +++ b/tests/models/test_base.py @@ -4,6 +4,7 @@ import pytest from fastadmin import ModelAdmin +from fastadmin.api.exceptions import AdminApiException from fastadmin.api.schemas import ExportFormat from fastadmin.models.base import BaseModelAdmin from fastadmin.models.schemas import ModelFieldWidgetSchema, WidgetType @@ -600,9 +601,29 @@ def test_deserialize_value_timepicker_fallback_and_datetime(): filter_widget_type=WidgetType.Input, filter_widget_props={}, ) + field_date = ModelFieldWidgetSchema( + name="d", + column_name="d", + is_m2m=False, + is_pk=False, + is_immutable=False, + form_widget_type=WidgetType.DatePicker, + form_widget_props={}, + filter_widget_type=WidgetType.Input, + filter_widget_props={}, + ) base = ModelAdmin(type("Model", (), {})) assert base.deserialize_value(field_time, "12:34:56").isoformat() == "12:34:56" assert base.deserialize_value(field_dt, "2026-02-19T12:34:56").isoformat() == "2026-02-19T12:34:56" + assert base.deserialize_value(field_date, "2026-02-19").isoformat() == "2026-02-19" + + # A malformed Date/DateTime value raises a clean 422 instead of an unhandled 500. + with pytest.raises(AdminApiException) as exc_date: + base.deserialize_value(field_date, "not-a-date") + assert exc_date.value.status_code == 422 + with pytest.raises(AdminApiException) as exc_dt: + base.deserialize_value(field_dt, "not-a-datetime") + assert exc_dt.value.status_code == 422 async def test_save_model_excludes_password_flow(): From 3da5e1753bd473edb85b2b39d6c1b0fa9b05e9fa Mon Sep 17 00:00:00 2001 From: Seva D Date: Fri, 10 Jul 2026 12:57:49 +0400 Subject: [PATCH 3/3] 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 --- fastadmin/api/frameworks/django/app/api.py | 32 +++++++++++++++------- fastadmin/api/service.py | 13 ++++----- fastadmin/models/base.py | 12 ++++++++ tests/api/test_action.py | 17 ++++++++++++ 4 files changed, 57 insertions(+), 17 deletions(-) diff --git a/fastadmin/api/frameworks/django/app/api.py b/fastadmin/api/frameworks/django/app/api.py index c440365..98b59c2 100644 --- a/fastadmin/api/frameworks/django/app/api.py +++ b/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,10 +85,7 @@ async def sign_in(request: HttpRequest) -> JsonResponse: if request.method != "POST": return JsonResponse({"detail": "Method not allowed"}, status=405) try: - try: - payload = SignInInputSchema(**json.loads(request.body)) - except (json.JSONDecodeError, TypeError) as e: - raise AdminApiException(422, detail="Invalid request body.") from e + payload = _load_json_body(request, SignInInputSchema) session_id = await api_service.sign_in( request.COOKIES.get(settings.ADMIN_SESSION_ID_KEY, None), payload, @@ -228,7 +240,7 @@ async def add(request: HttpRequest, model: str) -> JsonResponse: 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) @@ -252,7 +264,7 @@ async def change_password(request: HttpRequest, id: UUID | int | str) -> JsonRes 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) @@ -279,7 +291,7 @@ async def change(request: HttpRequest, model: str, id: UUID | int | str) -> Json request.COOKIES.get(settings.ADMIN_SESSION_ID_KEY, None), model, id, - json.loads(request.body), + _load_json_body(request), request=request, ) return JsonResponse(obj) @@ -346,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, @@ -409,7 +421,7 @@ async def action( if request.method != "POST": 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, @@ -443,7 +455,7 @@ async def widget_action( if request.method != "POST": 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, diff --git a/fastadmin/api/service.py b/fastadmin/api/service.py index 7a47db1..7530173 100644 --- a/fastadmin/api/service.py +++ b/fastadmin/api/service.py @@ -488,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 ( @@ -547,10 +546,10 @@ async def action( 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 honor the - # same change-permission gate as the change/delete endpoints — otherwise a - # read-only admin could mutate records through a registered action. - await self._require_permission(admin_model, "has_change_permission", current_user_id) + # 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.") diff --git a/fastadmin/models/base.py b/fastadmin/models/base.py index cf65609..e171ecf 100644 --- a/fastadmin/models/base.py +++ b/fastadmin/models/base.py @@ -736,6 +736,18 @@ async def has_export_permission(self, user_id: UUID | int | None = None) -> bool """ return True + async def has_action_permission(self, user_id: UUID | int | None = None) -> bool: + """This method is used to check if user has permission to run an action. + + Defaults to the change permission because actions run bulk mutations over + the selected ids; override it to let a read-only user run a non-mutating + action independently of the change permission. + + :param user_id: The user id. + :return: A boolean value. + """ + return await self.has_change_permission(user_id=user_id) + class InlineModelAdmin(BaseModelAdmin): """This class is used to create admin inline model class.""" diff --git a/tests/api/test_action.py b/tests/api/test_action.py index 8256dfe..2300efc 100644 --- a/tests/api/test_action.py +++ b/tests/api/test_action.py @@ -19,6 +19,23 @@ async def test_action(session_id, admin_models, event, client): assert updated_event["is_active"] +async def test_action_403_without_change_permission(session_id, admin_models, event, client, monkeypatch): + """A user without change permission cannot run an action (has_action_permission delegates to it).""" + assert session_id + event_admin_model = admin_models[event.__class__] + event_admin_model.actions = ("make_is_active",) + + async def _denied(user_id=None): + return False + + monkeypatch.setattr(event_admin_model, "has_change_permission", _denied) + r = await client.post( + f"/api/action/{event.get_model_name()}/make_is_active", + json={"ids": [event.id]}, + ) + assert r.status_code == 403, r.text + + async def test_action_405(session_id, event, client): assert session_id r = await client.get(