From 717acb8efeb3b644365ce2bceb25c36a5ef7ecc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yannik=20R=C3=B6del?= Date: Thu, 4 Apr 2024 17:47:55 +0200 Subject: [PATCH 1/5] Validate Email regex before submitting forms --- cgi-bin/form.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/cgi-bin/form.py b/cgi-bin/form.py index 8087cc0..5ca8b69 100755 --- a/cgi-bin/form.py +++ b/cgi-bin/form.py @@ -34,6 +34,9 @@ except IOError: HONEYPOT_FIELD_NAME = "addressline1" +# This regex merely validates what the in-browser form validation already checks and +# isn't all too strict. +EMAIL_REGEX = re.compile(r"^[^ ]+@[^ ]+\.[^ ]+$") SITE_DIRECTORY = os.environ.get("SITE_DIRECTORY", "") request_uri = os.environ.get("REQUEST_URI", "").lower().rstrip("/") @@ -183,7 +186,11 @@ if not isinstance(hcaptcha_data, Mapping) or not hcaptcha_data.get("success", Fa # Extract all the actually provided form data. This is different from form to # form (see the match block below). contact_name = get_form_value("contactname") + contact_email = get_form_value("contactemail") +if not EMAIL_REGEX.fullmatch(contact_email): + fail("400 Bad Request", "Invalid Email address") + message = get_form_value("message", "[Keine Nachricht hinterlassen]") attachment: Optional[tuple[str, bytes]] = None From e2623df390d08ade6c3f6fc6234bc23f251557f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yannik=20R=C3=B6del?= Date: Thu, 4 Apr 2024 17:48:16 +0200 Subject: [PATCH 2/5] Fix typing errors in form.py --- cgi-bin/form.py | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/cgi-bin/form.py b/cgi-bin/form.py index 5ca8b69..9fd4268 100755 --- a/cgi-bin/form.py +++ b/cgi-bin/form.py @@ -131,15 +131,16 @@ if form_disabled: form = cgi.FieldStorage() @overload -def get_form_value(name: str, default: Optional[str], cast: type[str] = str) -> str: ... +def get_form_value(name: str, default: None = ..., *, cast: type[bytes]) -> tuple[str, bytes]:... @overload -def get_form_value(name: str, default: Optional[int], cast: type[int]) -> int:... +def get_form_value(name: str, default: Optional[int] = ..., *, cast: type[int]) -> int:... @overload -def get_form_value(name: str, default: None = ..., cast: type[bytes] = ...) -> tuple[str, bytes]:... +def get_form_value(name: str, default: Optional[str] = ..., *, cast: type[str] = ...) -> str: ... def get_form_value( name: str, default: Any = None, - cast: type[str] | type[int] | type[io.BytesIO] = str, + *, + cast: type[str] | type[int] | type[bytes] = str, ) -> Any: if name not in form: if default is None: @@ -166,7 +167,7 @@ def get_form_value( # constant-time string comparison here. given_csrf_token = get_form_value("csrftoken") if not hmac.compare_digest(csrf_token, given_csrf_token): - fail("400 Bad Request", f"Invalid CSRF token") + fail("400 Bad Request", "Invalid CSRF token") # If the honeypot field was not empty, back off. @@ -194,7 +195,7 @@ if not EMAIL_REGEX.fullmatch(contact_email): message = get_form_value("message", "[Keine Nachricht hinterlassen]") attachment: Optional[tuple[str, bytes]] = None -ticket_details = collections.OrderedDict() +ticket_details = collections.OrderedDict[str, str | int]() ticket_details["Kontaktperson"] = contact_name ticket_details["Email"] = contact_email @@ -220,9 +221,9 @@ match request_uri: ticket_details["Adresse"] = get_form_value("addressline") ticket_details["PLZ"] = get_form_value("postalcode") ticket_details["Stadt"] = get_form_value("city") - ticket_details["Anzahl Desktops"] = get_form_value("desktopcount", 0, int) - ticket_details["Anzahl Laptops"] = get_form_value("laptopcount", 0, int) - ticket_details["Anzahl Drucker"] = get_form_value("printercount", 0, int) + ticket_details["Anzahl Desktops"] = get_form_value("desktopcount", 0, cast=int) + ticket_details["Anzahl Laptops"] = get_form_value("laptopcount", 0, cast=int) + ticket_details["Anzahl Drucker"] = get_form_value("printercount", 0, cast=int) case "/computer-beantragen/privat": form_name = "Computerantrag (privat)" @@ -256,13 +257,13 @@ match request_uri: ticket_details["Teilnehmenden-Name"] = get_form_value("participantname", "-") ticket_details["Telefonnummer"] = get_form_value("contactphone", "-") ticket_details["Fotos?"] = get_form_value("photos") - + case "/party": form_name = "CoderDojo Minecraft LAN" form_group = "CoderDojo" ticket_details["Java-Spielername"] = get_form_value("javaname", "") ticket_details["Bedrock-Spielername"] = get_form_value("bedrockname", "") - + case "/freizeit": form_name = "CoderCamp Umfrage" form_group = "CoderDojo" From 8a43f29bc4bd3d3e88c3b507886e8379abd389ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yannik=20R=C3=B6del?= Date: Thu, 4 Apr 2024 18:02:14 +0200 Subject: [PATCH 3/5] Explicitly create users in Zammad before using them Closes #40. Closes #141. For some reason, Zammad behaves differently when the corresponding user doesn't exit yet. This also allows us to explicitly set a name for users (at least for new ones). --- cgi-bin/form.py | 36 ++++++++++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/cgi-bin/form.py b/cgi-bin/form.py index 9fd4268..2452cd6 100755 --- a/cgi-bin/form.py +++ b/cgi-bin/form.py @@ -158,7 +158,10 @@ def get_form_value( return (value_object.filename or "upload"), value_object.file.read() else: try: - return cast(form.getfirst(name)) + result = cast(form.getfirst(name)) + if isinstance(result, str): + result = result.strip() + return result except (TypeError, ValueError): fail("400 Bad Request", f"Invalid value for field: {name}") @@ -187,6 +190,7 @@ if not isinstance(hcaptcha_data, Mapping) or not hcaptcha_data.get("success", Fa # Extract all the actually provided form data. This is different from form to # form (see the match block below). contact_name = get_form_value("contactname") +contact_names = contact_name.split(" ") contact_email = get_form_value("contactemail") if not EMAIL_REGEX.fullmatch(contact_email): @@ -289,13 +293,41 @@ ZAMMAD_TOKEN = os.environ.get("ZAMMAD_TOKEN", "") session.headers.update(Authorization=f"Token token={ZAMMAD_TOKEN}") try: + # Create a new user for the client. For some reason, using "guess:{email}" as the + # customer_id when creating the ticket doesn't really work, as described in the + # Zammad documentation [1]. Instead, we sometimes need to explictily create the + # user beforehand. See this discussion [2] for more details. + # [1]: https://docs.zammad.org/en/latest/api/ticket/index.html#create + # [2]: https://codeberg.org/angestoepselt/homepage/issues/141 + response = session.post( + f"{ZAMMAD_URL}/api/v1/users", + json=dict( + # Yes, yes... This goes against pretty much all best practices for parsing + # names. But: it's only internal and we save the name verbatim again below + # so we're going to go ahead and do it anyway. + firstname=" ".join(contact_names[:-1]) if len(contact_names) >= 2 else "?", + lastname=contact_names[-1], + email=contact_email, + ) + ) + if response.status_code == 422: + # This email address is already in use by another user. + customer_id = f"guess:{contact_email}" + else: + response.raise_for_status() + customer_id = response.json()["id"] + assert isinstance(customer_id, (str, int)) + # Add the actual ticket to the system. response = session.post( f"{ZAMMAD_URL}/api/v1/tickets", + headers={ + "X-On-Behalf-Of": contact_email, + }, json=dict( title=f"Kontaktformular {contact_name} – {form_name}", group=form_group, - customer_id=f"guess:{contact_email}", + customer_id=customer_id, article=dict( type="web", internal=True, From 81e41a2f2d6f34a2e3eff3fa249a9328f6a9e00e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yannik=20R=C3=B6del?= Date: Thu, 4 Apr 2024 18:13:04 +0200 Subject: [PATCH 4/5] Set a form category corresponding to the request Closes #120. --- cgi-bin/form.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/cgi-bin/form.py b/cgi-bin/form.py index 2452cd6..7f08fa6 100755 --- a/cgi-bin/form.py +++ b/cgi-bin/form.py @@ -38,6 +38,17 @@ HONEYPOT_FIELD_NAME = "addressline1" # isn't all too strict. EMAIL_REGEX = re.compile(r"^[^ ]+@[^ ]+\.[^ ]+$") +# Mapping from site-defined devices (see sites/angestoepselt/_data/config.json in this +# repository) to the corresponding Zammad categories: +# https://codeberg.org/angestoepselt/homepage/issues/120#issuecomment-1727768 +# This is a (str | int) -> str map because some keys we check against below might be +# integers and it's just easier to type this way. +FORM_CATEGORY_MAP: dict[str | int, str] = { + "Laptop": "laptop", + "Laptop ohne Akku": "laptop-battery-missing", + "Desktop-Computer": "desktop", +} + SITE_DIRECTORY = os.environ.get("SITE_DIRECTORY", "") request_uri = os.environ.get("REQUEST_URI", "").lower().rstrip("/") serializer = itsdangerous.URLSafeSerializer("secret key", "salt") @@ -204,6 +215,7 @@ ticket_details["Kontaktperson"] = contact_name ticket_details["Email"] = contact_email form_group = "csw-Allgemein" +form_category: str | None = None match request_uri: case "/kontakt": @@ -232,7 +244,10 @@ match request_uri: case "/computer-beantragen/privat": form_name = "Computerantrag (privat)" form_group = "csw-Anfragen" + ticket_details["Gewünschte Hardware"] = get_form_value("hardware", default="Unbekannt") + form_category = FORM_CATEGORY_MAP.get(ticket_details["Gewünschte Hardware"], None) + ticket_details["Adresse"] = get_form_value("addressline") ticket_details["PLZ"] = get_form_value("postalcode") ticket_details["Stadt"] = get_form_value("city") From 7a75e38451c4d1e769f018564678bcac6e7d69c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yannik=20R=C3=B6del?= Date: Thu, 4 Apr 2024 19:04:17 +0200 Subject: [PATCH 5/5] Correctly join URLs like a pro instead of stiching them together --- cgi-bin/form.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/cgi-bin/form.py b/cgi-bin/form.py index 7f08fa6..f047682 100755 --- a/cgi-bin/form.py +++ b/cgi-bin/form.py @@ -1,17 +1,17 @@ #!/usr/bin/env python import base64 -import io import cgi import collections from collections.abc import Mapping import hmac -import mimetypes -import re -import os -import secrets import json +import mimetypes +import os +import re +import secrets from typing import Any, Optional, overload +from urllib.parse import urljoin import itsdangerous import requests @@ -303,7 +303,7 @@ ticket_details["Kontaktformular"] = form_name # testing). form_group = os.environ.get("ZAMMAD_GROUP", "") or form_group -ZAMMAD_URL = os.environ.get("ZAMMAD_URL", "").rstrip("/") +ZAMMAD_URL = os.environ.get("ZAMMAD_URL", "") ZAMMAD_TOKEN = os.environ.get("ZAMMAD_TOKEN", "") session.headers.update(Authorization=f"Token token={ZAMMAD_TOKEN}") @@ -315,7 +315,7 @@ try: # [1]: https://docs.zammad.org/en/latest/api/ticket/index.html#create # [2]: https://codeberg.org/angestoepselt/homepage/issues/141 response = session.post( - f"{ZAMMAD_URL}/api/v1/users", + urljoin(ZAMMAD_URL, "api/v1/users"), json=dict( # Yes, yes... This goes against pretty much all best practices for parsing # names. But: it's only internal and we save the name verbatim again below @@ -335,7 +335,7 @@ try: # Add the actual ticket to the system. response = session.post( - f"{ZAMMAD_URL}/api/v1/tickets", + urljoin(ZAMMAD_URL, "api/v1/tickets"), headers={ "X-On-Behalf-Of": contact_email, }, @@ -364,7 +364,7 @@ try: # Add a second article to the ticket that contains all the other information # from the contact form. response = session.post( - f"{ZAMMAD_URL}/api/v1/ticket_articles", + urljoin(ZAMMAD_URL, "api/v1/ticket_articles"), json=dict( ticket_id=ticket_id, type="note", @@ -379,7 +379,7 @@ try: # Add a tag to the ticket, denoting which contact form it came from. response = session.post( - f"{ZAMMAD_URL}/api/v1/tags/add", + urljoin(ZAMMAD_URL, "api/v1/tags/add"), json=dict( object="Ticket", o_id=ticket_id,