diff --git a/api_schemas/ad_schema.py b/api_schemas/ad_schema.py index 1b52ef96..1875b1e9 100644 --- a/api_schemas/ad_schema.py +++ b/api_schemas/ad_schema.py @@ -3,6 +3,7 @@ from api_schemas.base_schema import BaseSchema from helpers.constants import MAX_BOOK_AUTHOR, MAX_BOOK_TITLE + class AdRead(BaseSchema): ad_id: int title: str @@ -12,17 +13,17 @@ class AdRead(BaseSchema): user_id: int selling: bool condition: int - - + + class AdCreate(BaseSchema): title: Annotated[str, StringConstraints(max_length=MAX_BOOK_TITLE)] author: Annotated[str, StringConstraints(max_length=MAX_BOOK_AUTHOR)] | None price: int | None course: Annotated[str, StringConstraints(max_length=MAX_BOOK_TITLE)] | None - user_id: int selling: bool condition: int - + + class AdUpdate(BaseSchema): title: Annotated[str, StringConstraints(max_length=MAX_BOOK_TITLE)] | None = None author: Annotated[str, StringConstraints(max_length=MAX_BOOK_AUTHOR)] | None = None diff --git a/helpers/csv_response_factory.py b/helpers/csv_response_factory.py index 6d397e12..3b94b0e2 100644 --- a/helpers/csv_response_factory.py +++ b/helpers/csv_response_factory.py @@ -6,9 +6,29 @@ from api_schemas.csv_schemas.base_csv_schema import BaseCsvSchema - T = TypeVar("T", bound=BaseCsvSchema) +# Characters which make Excel/LibreOffice treat a cell as a formula instead of text. +# Leading tab/CR are included because spreadsheet apps strip them before parsing. +FORMULA_TRIGGERS = ("=", "+", "-", "@", "\t", "\r") + + +def escape_csv_value(value: str) -> str: + """ + Neutralize spreadsheet formula injection (CWE-1236). + + Much of what we export is free text written by ordinary members (names, + food preferences, motivations). Without this, a member can set their food + preference to something like `=cmd|'/c calc'!A1` and get it executed on the + machine of whichever admin opens the exported file. + + Prefixing with a single quote makes the spreadsheet render the literal text. + This is pretty standard, as per CWE-1236. Only the first char matters. + """ + if value.startswith(FORMULA_TRIGGERS): + return f"'{value}" + return value + class CsvResponseFactory(Generic[T]): def __init__(self, none_str: str = "") -> None: @@ -22,7 +42,7 @@ def append(self, row: T) -> None: dump = row.model_dump(by_alias=True) for k in self.__columns.keys(): - self.__columns[k].append(str(dump.get(k) or self.none_str)) + self.__columns[k].append(escape_csv_value(str(dump.get(k) or self.none_str))) def __initialize_headers(self, row: T) -> None: model_fields = { diff --git a/routes/access_serve_router.py b/routes/access_serve_router.py index 1b1c1737..509c4e64 100644 --- a/routes/access_serve_router.py +++ b/routes/access_serve_router.py @@ -6,7 +6,6 @@ from typing import get_args import datetime - access_serve_router = APIRouter() @@ -78,9 +77,6 @@ def get_all_access_ids(door: str, db: DB_dependency) -> list[str]: all_access_ids = sorted(set(direct_access_ids + post_access_ids)) # Remove all stil-ids which are not alphanumeric with dashes, - # just a failsafe if stil_id is not set properly since we will be putting these in html - for stil_id in all_access_ids: - if not stil_id or not stil_id.replace("-", "").isalnum(): - all_access_ids.remove(stil_id) - - return all_access_ids + # just a failsafe if stil_id is not set properly since we will be putting these in html. + # Build a new list to avoid bugs related to skipping elements. + return [stil_id for stil_id in all_access_ids if stil_id and stil_id.replace("-", "").isalnum()] diff --git a/routes/ad_router.py b/routes/ad_router.py index 04799829..8579f0f5 100644 --- a/routes/ad_router.py +++ b/routes/ad_router.py @@ -17,12 +17,12 @@ def get_all_ads(db: DB_dependency): @ad_router.post("/", response_model=AdRead) -def create_ad(data: AdCreate, db: DB_dependency): +def create_ad(data: AdCreate, current_user: Annotated[User_DB, Permission.member()], db: DB_dependency): ad = BookAd_DB( title=data.title, course=data.course, author=data.author, - user_id=data.user_id, + user_id=current_user.id, selling=data.selling, condition=data.condition, price=data.price, diff --git a/routes/post_router.py b/routes/post_router.py index 333182ee..a3a98ec5 100644 --- a/routes/post_router.py +++ b/routes/post_router.py @@ -124,7 +124,7 @@ def get_post(post_id: int, db: DB_dependency): return post -@post_router.get("/users/{post_id}", response_model=list[SimpleUserRead]) +@post_router.get("/users/{post_id}", response_model=list[SimpleUserRead], dependencies=[Permission.member()]) def get_all_users_with_post(post_id: int, db: DB_dependency): posts = db.query(Post_DB).filter_by(id=post_id).one_or_none() if posts is None: diff --git a/tests/test_security_regressions.py b/tests/test_security_regressions.py new file mode 100644 index 00000000..ec1af548 --- /dev/null +++ b/tests/test_security_regressions.py @@ -0,0 +1,192 @@ +# type: ignore +""" +Regression tests for security fixes. +""" + +import datetime + +import pytest + +from db_models.council_model import Council_DB +from db_models.event_model import Event_DB +from db_models.user_door_access_model import UserDoorAccess_DB + +from .basic_factories import auth_headers + + +@pytest.fixture() +def simple_event(db_session): + """A minimal event with signups unconfirmed.""" + council = Council_DB( + name_sv="EventUtskott", + description_sv="beskrivning", + name_en="EventCouncil", + description_en="description", + ) + db_session.add(council) + db_session.commit() + + now = datetime.datetime.now(datetime.timezone.utc) + event = Event_DB( + council_id=council.id, + starts_at=now + datetime.timedelta(days=2), + ends_at=now + datetime.timedelta(days=3), + signup_start=now - datetime.timedelta(days=1), + signup_end=now + datetime.timedelta(days=1), + title_sv="Testevent", + title_en="Test event", + description_sv="beskrivning", + description_en="description", + location="LC", + dress_code="Cool", + price=0, + ) + db_session.add(event) + db_session.commit() + db_session.refresh(event) + return event + + +#################################################################### +# Posts: the member roster of a post was world-readable +#################################################################### + + +def test_post_users_requires_authentication(client, member_post): + """ + Listing who holds a post discloses guild members' names. The sibling route + GET /posts/ already requires membership; this one must match. + """ + res = client.get(f"/posts/users/{member_post.id}") + + assert res.status_code == 401, res.text + + +def test_post_users_allowed_for_member(client, member_token, member_post, membered_user): + """Members must still be able to read a post's roster.""" + res = client.get( + f"/posts/users/{member_post.id}", + headers=auth_headers(member_token), + ) + + assert res.status_code == 200, res.text + assert membered_user.id in [user["id"] for user in res.json()] + + +#################################################################### +# Door access serving: malformed stil-ids must never be served +#################################################################### + + +def _make_user_with_door_access(db_session, client, email, stil_id, door="Arkivet"): + from .basic_factories import create_membered_user + + user = create_membered_user(client, db_session, email=email, first_name="Door", last_name="User") + user.stil_id = stil_id + db_session.commit() + + now = datetime.datetime.now(datetime.timezone.utc) + access = UserDoorAccess_DB( + user_id=user.id, + door=door, + starttime=now - datetime.timedelta(days=1), + endtime=now + datetime.timedelta(days=1), + ) + db_session.add(access) + db_session.commit() + return user + + +def test_access_serve_filters_all_malformed_stil_ids(client, db_session): + """ + The endpoint's output is interpolated into HTML by the frontend (after more html removal there), + so the non-alphanumeric failsafe is a security control. It previously mutated the + list while iterating over it, which silently skipped entries: with two + adjacent bad ids, the second one survived and was served. + """ + # Chosen so that, sorted, the two malformed ids land next to each other. + _make_user_with_door_access(db_session, client, "door_a@example.com", "aaa