From 3e48d894256a25792638734eded6eb73cf4c3ea2 Mon Sep 17 00:00:00 2001 From: Igor Benav Date: Mon, 24 Aug 2026 08:38:26 -0300 Subject: [PATCH 1/5] Fix admin auth bypass from loose login and static path matching --- SECURITY.md | 9 ++ crudadmin/admin_interface/middleware/auth.py | 16 +- crudadmin/admin_interface/model_view.py | 12 ++ tests/auth/test_middleware_path_bypass.py | 161 +++++++++++++++++++ 4 files changed, 192 insertions(+), 6 deletions(-) create mode 100644 tests/auth/test_middleware_path_bypass.py diff --git a/SECURITY.md b/SECURITY.md index f40176e..909022a 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -122,6 +122,15 @@ Stay informed about security updates: 3. Subscribe to our security mailing list 4. Monitor our release notes +## Acknowledgments + +We are grateful to the researchers who report vulnerabilities responsibly. Reporters +of fixed issues are listed here with their permission. + +| Reporter | Issue | Fixed in | +| -------- | ----- | -------- | +| [Ubaid Ur Rehman](mailto:arifubaid0345@gmail.com), using [shadowaudit](https://gitlab.com/theredhacker0345/shadowaudit) | Authentication bypass in `AdminAuthMiddleware`: the login-page and static-asset exemptions matched by path suffix and substring, so a crafted URL could reach admin read endpoints without a session | v0.5.1 | + ## License This security policy is part of the CRUDAdmin project and is subject to the same license terms. diff --git a/crudadmin/admin_interface/middleware/auth.py b/crudadmin/admin_interface/middleware/auth.py index 8bacf4b..e9613a2 100644 --- a/crudadmin/admin_interface/middleware/auth.py +++ b/crudadmin/admin_interface/middleware/auth.py @@ -46,8 +46,10 @@ async def dispatch(self, request: Request, call_next): if not request.url.path.startswith(expected_prefix): return await call_next(request) - is_login_path = request.url.path.endswith("/login") - is_static_path = "/static/" in request.url.path + url_prefix = self.admin_instance.get_url_prefix() + + is_login_path = request.url.path.rstrip("/") == f"{url_prefix}/login" + is_static_path = request.url.path.startswith(f"{url_prefix}/static/") if is_login_path or is_static_path: response = await call_next(request) @@ -63,7 +65,9 @@ async def dispatch(self, request: Request, call_next): if not session_id: logger.debug("Missing session_id") - login_url = f"{self.admin_instance.get_url_prefix()}/login?error=Please+log+in+to+access+this+page" + login_url = ( + f"{url_prefix}/login?error=Please+log+in+to+access+this+page" + ) return RedirectResponse( url=login_url, status_code=303, @@ -78,7 +82,7 @@ async def dispatch(self, request: Request, call_next): if not session_data: logger.debug("Invalid or expired session") - login_url = f"{self.admin_instance.get_url_prefix()}/login?error=Session+expired" + login_url = f"{url_prefix}/login?error=Session+expired" return RedirectResponse( url=login_url, status_code=303, @@ -91,7 +95,7 @@ async def dispatch(self, request: Request, call_next): if not user: logger.debug("User not found for session") - login_url = f"{self.admin_instance.get_url_prefix()}/login?error=User+not+found" + login_url = f"{url_prefix}/login?error=User+not+found" return RedirectResponse( url=login_url, status_code=303, @@ -115,7 +119,7 @@ async def dispatch(self, request: Request, call_next): or "/crud/" in request.url.path ): raise - login_url = f"{self.admin_instance.get_url_prefix()}/login?error=Authentication+error" + login_url = f"{url_prefix}/login?error=Authentication+error" return RedirectResponse( url=login_url, status_code=303, diff --git a/crudadmin/admin_interface/model_view.py b/crudadmin/admin_interface/model_view.py index c25ae83..efc235e 100644 --- a/crudadmin/admin_interface/model_view.py +++ b/crudadmin/admin_interface/model_view.py @@ -541,6 +541,12 @@ def setup_routes(self) -> None: view.setup_routes() # Only creates view/create/update routes ``` """ + auth_dependencies = ( + [Depends(self.admin_site.admin_authentication.get_current_user())] + if self.admin_site is not None + else [] + ) + if "create" in self.allowed_actions: self.router.add_api_route( "/form_create", @@ -554,6 +560,7 @@ def setup_routes(self) -> None: self.get_model_create_page(template="admin/model/create.html"), methods=["GET"], include_in_schema=False, + dependencies=auth_dependencies, response_model=None, ) @@ -563,6 +570,7 @@ def setup_routes(self) -> None: self.get_model_admin_page(), methods=["GET"], include_in_schema=False, + dependencies=auth_dependencies, response_model=None, ) self.router.add_api_route( @@ -572,6 +580,7 @@ def setup_routes(self) -> None: ), methods=["GET"], include_in_schema=False, + dependencies=auth_dependencies, response_model=None, ) @@ -590,6 +599,7 @@ def setup_routes(self) -> None: self.get_model_update_page(template="admin/model/update.html"), methods=["GET"], include_in_schema=False, + dependencies=auth_dependencies, response_model=None, ) self.router.add_api_route( @@ -606,6 +616,7 @@ def setup_routes(self) -> None: self.get_related_data_endpoint(), methods=["GET"], include_in_schema=False, + dependencies=auth_dependencies, response_model=None, ) self.router.add_api_route( @@ -613,6 +624,7 @@ def setup_routes(self) -> None: self.get_relationship_options_endpoint(), methods=["GET"], include_in_schema=False, + dependencies=auth_dependencies, response_model=None, ) diff --git a/tests/auth/test_middleware_path_bypass.py b/tests/auth/test_middleware_path_bypass.py new file mode 100644 index 0000000..a444dcf --- /dev/null +++ b/tests/auth/test_middleware_path_bypass.py @@ -0,0 +1,161 @@ +"""Regression tests for the AdminAuthMiddleware path allowlist. + +The middleware used to skip authentication for any path *ending* in ``/login`` +and any path *containing* ``/static/``, so URLs like +``/admin/Article/update/login`` or ``/admin/Author/related/static/books`` +reached admin endpoints with no session at all. + +Reported by Ubaid Ur Rehman, found with shadowaudit +(https://gitlab.com/theredhacker0345/shadowaudit). +""" + +from contextlib import asynccontextmanager + +import pytest +from fastapi import FastAPI +from fastapi.testclient import TestClient +from pydantic import BaseModel +from sqlalchemy import Column, ForeignKey, Integer, String +from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine +from sqlalchemy.orm import DeclarativeBase, relationship + +from crudadmin import CRUDAdmin + + +class Base(DeclarativeBase): + pass + + +class Article(Base): + __tablename__ = "bypass_articles" + slug = Column(String, primary_key=True) + body = Column(String) + + +class Author(Base): + __tablename__ = "bypass_authors" + id = Column(Integer, primary_key=True) + name = Column(String) + books = relationship("BypassBook", back_populates="author", lazy="selectin") + + +class BypassBook(Base): + __tablename__ = "bypass_books" + id = Column(Integer, primary_key=True) + title = Column(String) + author_id = Column(Integer, ForeignKey("bypass_authors.id")) + author = relationship("Author", back_populates="books", lazy="selectin") + + +class ArticleCreate(BaseModel): + slug: str + body: str + + +class ArticleUpdate(BaseModel): + body: str + + +class AuthorCreate(BaseModel): + name: str + + +class AuthorUpdate(BaseModel): + name: str + + +SECRET_BODY = "article-body-that-must-not-leak" + + +@pytest.fixture(scope="module") +def admin_client(tmp_path_factory): + """A running admin app with a record whose primary key is literally 'login'. + + Module-scoped: CRUDAdmin registers its admin models on a shared declarative + base, so only one instance can be built per process. + """ + tmp_path = tmp_path_factory.mktemp("bypass") + engine = create_async_engine(f"sqlite+aiosqlite:///{tmp_path}/app.db") + session_factory = async_sessionmaker(engine, expire_on_commit=False) + + async def get_session(): + async with session_factory() as session: + yield session + + admin = CRUDAdmin( + session=get_session, + SECRET_KEY="x" * 32, + admin_db_url=f"sqlite+aiosqlite:///{tmp_path}/admin.db", + secure_cookies=False, + ) + admin.add_view( + model=Article, create_schema=ArticleCreate, update_schema=ArticleUpdate + ) + admin.add_view( + model=Author, + create_schema=AuthorCreate, + update_schema=AuthorUpdate, + display_field="name", + ) + + @asynccontextmanager + async def lifespan(app): + async with engine.begin() as conn: + await conn.run_sync(Base.metadata.create_all) + async with session_factory() as session: + session.add(Article(slug="login", body=SECRET_BODY)) + session.add(Author(id=1, name="author-name")) + session.add(BypassBook(id=1, title="book-title", author_id=1)) + await session.commit() + await admin.initialize() + yield + + app = FastAPI(lifespan=lifespan) + app.mount("/admin", admin.app) + + with TestClient( + app, follow_redirects=False, raise_server_exceptions=False + ) as client: + yield client + + +@pytest.mark.parametrize( + "path", + [ + "/admin/Article/update/login", + "/admin/Author/update/login", + "/admin/Author/related/1/login", + "/admin/Author/related/static/books", + "/admin/Author/relationship-options/login", + ], +) +def test_crafted_path_does_not_bypass_auth(admin_client, path): + """A path that merely ends in /login or contains /static/ must still 401/redirect.""" + response = admin_client.get(path) + + assert response.status_code in (303, 401), response.status_code + if response.status_code == 303: + assert response.headers["location"].startswith("/admin/login") + assert SECRET_BODY not in response.text + + +def test_crafted_login_path_leaks_no_record_data(admin_client): + """The string-PK record keyed 'login' must not be rendered to an anonymous user.""" + response = admin_client.get("/admin/Article/update/login") + + assert SECRET_BODY not in response.text + + +def test_real_login_page_is_still_reachable(admin_client): + """The genuine login page stays open to unauthenticated visitors.""" + response = admin_client.get("/admin/login") + + assert response.status_code == 200 + assert " Date: Mon, 24 Aug 2026 08:41:31 -0300 Subject: [PATCH 2/5] version bump --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 25bc429..5aef622 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "crudadmin" -version = "0.5.0" +version = "0.5.1" description = "FastAPI-based admin interface with authentication, event logging and CRUD operations" readme = "README.md" requires-python = ">=3.10" From ebb9a727b08c8b058d44ed55556f0993dbc3b940 Mon Sep 17 00:00:00 2001 From: Igor Benav Date: Mon, 24 Aug 2026 08:43:23 -0300 Subject: [PATCH 3/5] Publish the security policy on the docs site --- crudadmin/admin_interface/model_view.py | 4 +++- docs/community/SECURITY.md | 1 + docs/community/overview.md | 8 ++++++++ zensical.toml | 1 + 4 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 docs/community/SECURITY.md diff --git a/crudadmin/admin_interface/model_view.py b/crudadmin/admin_interface/model_view.py index efc235e..e8eec26 100644 --- a/crudadmin/admin_interface/model_view.py +++ b/crudadmin/admin_interface/model_view.py @@ -529,7 +529,9 @@ def setup_routes(self) -> None: - Update: /update/{id} (GET), /form_update/{id} (POST) Routes are configured based on the allowed_actions set provided during initialization. - All routes use appropriate templates and include required dependencies. + All routes use appropriate templates and include required dependencies. Every route + declares an authentication dependency in addition to the admin auth middleware, so a + request that reaches an endpoint without a valid session is rejected there as well. Example: ```python diff --git a/docs/community/SECURITY.md b/docs/community/SECURITY.md new file mode 100644 index 0000000..390dbe3 --- /dev/null +++ b/docs/community/SECURITY.md @@ -0,0 +1 @@ +--8<-- "SECURITY.md" \ No newline at end of file diff --git a/docs/community/overview.md b/docs/community/overview.md index ef9cb1b..29c9827 100644 --- a/docs/community/overview.md +++ b/docs/community/overview.md @@ -5,6 +5,7 @@ Welcome to the project's community hub. Here, you'll find essential resources an ## Table of Contents - [Contributing](#contributing) - [Code of Conduct](#code-of-conduct) +- [Security Policy](#security-policy) - [License](#license) --- @@ -23,6 +24,13 @@ The Code of Conduct outlines the standards and behaviors expected of our communi --- +## Security Policy +[View the Security Policy](SECURITY.md) + +The security policy explains which versions receive security updates and how to report a vulnerability privately. If you believe you have found a security issue, please follow this process rather than opening a public issue. It also acknowledges the researchers whose reports have led to fixes. + +--- + ## License [View the License](LICENSE.md) diff --git a/zensical.toml b/zensical.toml index 95e0aa8..f49e60c 100644 --- a/zensical.toml +++ b/zensical.toml @@ -90,6 +90,7 @@ toggle.name = "Switch to light mode" { "Overview" = "community/overview.md" }, { "Contributing" = "community/CONTRIBUTING.md" }, { "Code of Conduct" = "community/CODE_OF_CONDUCT.md" }, + { "Security Policy" = "community/SECURITY.md" }, { "License" = "community/LICENSE.md" }, ] From e145b7d91b8756ac82d34fb51fa49bc2333f684d Mon Sep 17 00:00:00 2001 From: Igor Benav Date: Mon, 24 Aug 2026 08:59:01 -0300 Subject: [PATCH 4/5] Fix admin auth dependencies that were registered uncalled --- crudadmin/admin_interface/admin_site.py | 4 +- crudadmin/admin_interface/auth.py | 21 +++++--- crudadmin/admin_interface/crud_admin.py | 8 +-- tests/auth/test_middleware_path_bypass.py | 63 +++++++++++++++++++++++ 4 files changed, 80 insertions(+), 16 deletions(-) diff --git a/crudadmin/admin_interface/admin_site.py b/crudadmin/admin_interface/admin_site.py index c4b24ca..80ddfa2 100644 --- a/crudadmin/admin_interface/admin_site.py +++ b/crudadmin/admin_interface/admin_site.py @@ -201,7 +201,7 @@ def setup_routes(self) -> None: self.dashboard_content(), methods=["GET"], include_in_schema=False, - dependencies=[Depends(self.admin_authentication.get_current_user)], + dependencies=[Depends(self.admin_authentication.get_current_user())], response_model=None, ) self.router.add_api_route( @@ -209,7 +209,7 @@ def setup_routes(self) -> None: self.dashboard_page(), methods=["GET"], include_in_schema=False, - dependencies=[Depends(self.admin_authentication.get_current_user)], + dependencies=[Depends(self.admin_authentication.get_current_user())], response_model=None, ) diff --git a/crudadmin/admin_interface/auth.py b/crudadmin/admin_interface/auth.py index 0166651..d443063 100644 --- a/crudadmin/admin_interface/auth.py +++ b/crudadmin/admin_interface/auth.py @@ -1,5 +1,5 @@ import logging -from typing import Optional +from typing import Any, Callable, Optional from fastapi import Cookie, Depends, Request from fastapi.security import OAuth2PasswordBearer @@ -39,6 +39,7 @@ def __init__( self.auth_models = {} self.event_integration = event_integration self.session_manager = session_manager + self._current_user_dependency: Optional[Callable[..., Any]] = None self.auth_models[self.db_config.AdminUser.__name__] = { "model": self.db_config.AdminUser, @@ -58,7 +59,16 @@ def __init__( "delete_schema": None, } - def get_current_user(self): + def get_current_user(self) -> Callable[..., Any]: + """Return the dependency that resolves the session cookie to an admin user. + + The dependency is built once and reused, so routes that declare it at more + than one level (router and route) share a single callable and FastAPI's + dependency cache resolves it once per request. + """ + if self._current_user_dependency is not None: + return self._current_user_dependency + async def get_current_user_inner( request: Request, db: AsyncSession = Depends(self.db_config.get_admin_db), @@ -67,12 +77,6 @@ async def get_current_user_inner( if not session_id: raise UnauthorizedException("Not authenticated") - is_valid_session = await self.session_manager.validate_session( - session_id=session_id - ) - if not is_valid_session: - raise UnauthorizedException("Could not validate credentials") - session_data = await self.session_manager.validate_session( session_id=session_id ) @@ -93,6 +97,7 @@ async def get_current_user_inner( logger.debug("User not found") raise UnauthorizedException("User not authenticated") + self._current_user_dependency = get_current_user_inner return get_current_user_inner async def get_current_superuser(self, current_user: AdminUserRead) -> AdminUserRead: diff --git a/crudadmin/admin_interface/crud_admin.py b/crudadmin/admin_interface/crud_admin.py index 9f8d638..f1cfcbd 100644 --- a/crudadmin/admin_interface/crud_admin.py +++ b/crudadmin/admin_interface/crud_admin.py @@ -846,9 +846,7 @@ def setup( allowed_actions=allowed_actions, ) - get_user_dependency = cast( - Callable[..., AsyncSession], self.admin_authentication.get_current_user - ) + get_user_dependency = self.admin_authentication.get_current_user() self.router.add_api_route( "/management/health", @@ -1162,9 +1160,7 @@ class Config: if self.track_events and self.event_integration: admin_view.event_integration = self.event_integration - current_user_dep = cast( - Callable[..., Any], self.admin_site.admin_authentication.get_current_user - ) + current_user_dep = self.admin_site.admin_authentication.get_current_user() self.app.include_router( admin_view.router, prefix=f"/{model_key}", diff --git a/tests/auth/test_middleware_path_bypass.py b/tests/auth/test_middleware_path_bypass.py index a444dcf..02f5e7e 100644 --- a/tests/auth/test_middleware_path_bypass.py +++ b/tests/auth/test_middleware_path_bypass.py @@ -112,6 +112,10 @@ async def lifespan(app): app = FastAPI(lifespan=lifespan) app.mount("/admin", admin.app) + # Mounted a second time at a path that does not match the configured mount_path. + # The middleware's outer prefix check does not match here and lets the request + # through, which leaves the per-route auth dependencies as the only defense. + app.mount("/backoffice", admin.app) with TestClient( app, follow_redirects=False, raise_server_exceptions=False @@ -159,3 +163,62 @@ def test_login_page_reachable_with_trailing_slash(admin_client): response = admin_client.get("/admin/login/") assert response.status_code != 303 + + +@pytest.mark.parametrize( + "path", + [ + "/backoffice/", + "/backoffice/Article/", + "/backoffice/Article/update/login", + "/backoffice/management/health", + ], +) +def test_route_dependencies_hold_when_the_middleware_is_skipped(admin_client, path): + """Auth must not depend on the middleware alone. + + Mounting the admin app somewhere other than its configured ``mount_path`` + makes the middleware's prefix check miss, so these requests reach the + endpoints directly. The per-route dependencies have to reject them. + """ + response = admin_client.get(path) + + assert response.status_code == 401, response.status_code + assert SECRET_BODY not in response.text + + +def test_protected_routes_declare_a_real_auth_dependency(admin_client): + """Guard against ``Depends(factory)`` being passed instead of ``Depends(factory())``. + + ``AdminAuthentication.get_current_user`` is a factory. Passing it uncalled + yields a dependency that takes no arguments, returns a function and never + raises, so the route looks protected while enforcing nothing. + """ + import inspect + + app = admin_client.app + admin_app = next( + route.app for route in app.routes if getattr(route, "path", None) == "/admin" + ) + + protected = [ + "/", + "/dashboard-content", + "/management/health", + "/Article/", + "/Article/update/{id}", + ] + seen = set() + for route in admin_app.routes: + path = getattr(route, "path", None) + dependant = getattr(route, "dependant", None) + if path not in protected or dependant is None: + continue + calls = [dep.call for dep in dependant.dependencies if dep.call is not None] + assert any(inspect.iscoroutinefunction(call) for call in calls), ( + f"{path} declares no awaitable auth dependency: " + f"{[getattr(c, '__name__', c) for c in calls]}" + ) + seen.add(path) + + assert seen == set(protected), f"routes not checked: {set(protected) - seen}" From 937c0badd8849865ada411879215ca2a5b5d8cb3 Mon Sep 17 00:00:00 2001 From: Igor Benav Date: Mon, 24 Aug 2026 09:05:34 -0300 Subject: [PATCH 5/5] Stop gitignoring the tracked uv.lock and sync it to 0.5.1 --- .gitignore | 2 -- uv.lock | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/.gitignore b/.gitignore index 535dc00..c23ed74 100644 --- a/.gitignore +++ b/.gitignore @@ -175,7 +175,5 @@ cython_debug/ .ruff_cache -uv.lock - local_test crudadmin_data/ diff --git a/uv.lock b/uv.lock index 04e8a0f..2d3a6f1 100644 --- a/uv.lock +++ b/uv.lock @@ -411,7 +411,7 @@ toml = [ [[package]] name = "crudadmin" -version = "0.5.0" +version = "0.5.1" source = { editable = "." } dependencies = [ { name = "aiosqlite" },