From e0a63898ec634531b2ec76798f1b4c005bf6be8c Mon Sep 17 00:00:00 2001 From: Greg V <6913307+gregv@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:21:04 +0200 Subject: [PATCH 1/8] security: register auth-gated routes correctly, gate the newsletter relay, constant-time tokens, JSON 413 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two routes were public: @auth.* sat ABOVE @bp.route on GET /hackathon///checkins (full volunteer docs incl. email/phone) and PATCH /api/problem-statements/events, so Flask registered the undecorated function. @bp.route is now outermost; test/common/test_view_decorator_order.py AST-scans every *_views.py and fails on a repeat (it named exactly these two). api/newsletters: send_newsletter / preview_newsletter / GET / had auth commented out (open Gmail relay) and the module built its own init_auth (not importable under test). They are volunteer.admin-gated via common.auth now; POST // stays per-user on purpose. Route-level proof uses the new rejecting auth stub (test/common/auth_stubs.py): no Authorization -> 401, no X-Org-Id -> 403 — the pass-through stub cannot see a dropped decorator. api/messages/tests/test_route_gates.py covers checkins, problem-statement link, newsletter, the news/praise X-Api-Key checks (hmac.compare_digest; unset env never matches) and GET /news?limit= (400 on non-int, clamped 1..200). The create-hackathon views map None -> 404. MAX_CONTENT_LENGTH = 32 MiB with a JSON 413 handler (the app forces JSON content-type and frontend callers do res.json()). Co-Authored-By: Claude Fable 5.1 --- api/__init__.py | 4 + api/exception_views.py | 10 ++ api/messages/messages_views.py | 66 +++++--- api/messages/tests/test_route_gates.py | 147 ++++++++++++++++++ api/newsletters/newsletter_views.py | 25 +-- .../problem_statement_views.py | 2 +- api/store/store_views.py | 3 +- test/__init__.py | 0 test/common/__init__.py | 0 test/common/auth_stubs.py | 91 +++++++++++ test/common/test_payload_too_large.py | 28 ++++ test/common/test_view_decorator_order.py | 56 +++++++ 12 files changed, 396 insertions(+), 36 deletions(-) create mode 100644 api/messages/tests/test_route_gates.py create mode 100644 test/__init__.py create mode 100644 test/common/__init__.py create mode 100644 test/common/auth_stubs.py create mode 100644 test/common/test_payload_too_large.py create mode 100644 test/common/test_view_decorator_order.py diff --git a/api/__init__.py b/api/__init__.py index 5378b85..c06884e 100644 --- a/api/__init__.py +++ b/api/__init__.py @@ -98,6 +98,10 @@ def create_app(): ########################################## app = Flask(__name__, instance_relative_config=True) + # Hard cap on request bodies (the largest legitimate upload is a 10 MB + # planning attachment; videos go straight to GCS via signed URLs). + # Over-limit requests get the JSON 413 from api/exception_views.py. + app.config["MAX_CONTENT_LENGTH"] = 32 * 1024 * 1024 logger.info("Started Flask") diff --git a/api/exception_views.py b/api/exception_views.py index f7bcd1b..f6010dc 100644 --- a/api/exception_views.py +++ b/api/exception_views.py @@ -21,3 +21,13 @@ def _handle_not_found_error(ex): return {"message": "Not Found"}, ex.code else: return ex + + +@bp.app_errorhandler(exceptions.RequestEntityTooLarge) +def _handle_payload_too_large(ex): + # The app forces a JSON content-type on every response, so werkzeug's HTML + # 413 page breaks frontend res.json() callers — answer in JSON on /api/. + if request.path.startswith('/api/'): + from flask import current_app + return {"error": "payload_too_large", "max_bytes": current_app.config.get("MAX_CONTENT_LENGTH")}, 413 + return ex diff --git a/api/messages/messages_views.py b/api/messages/messages_views.py index c8157fc..16198b8 100644 --- a/api/messages/messages_views.py +++ b/api/messages/messages_views.py @@ -1,4 +1,5 @@ -import os +import os +import hmac from common.log import get_logger, debug, error import json from common.auth import auth, auth_user @@ -91,6 +92,16 @@ def getOrgId(req): # Get the org_id from the req return req.headers.get("X-Org-Id") +NEWS_LIMIT_MAX = 200 + + +def _api_key_matches(provided, expected): + """Constant-time X-Api-Key check; an unset env secret never matches.""" + if not expected or not provided: + return False + return hmac.compare_digest(str(expected), str(provided)) + + def get_authenticated_user_id(): """Helper function to get authenticated user ID with proper error handling""" if not auth_user or not auth_user.user_id: @@ -287,9 +298,9 @@ def add_single_hacker(event_id): if auth_user and auth_user.user_id: return vars(single_add_volunteer(event_id, request.get_json(), "hacker", auth_user.user_id)) +@bp.route("/hackathon///checkins", methods=["GET"]) @auth.require_user @auth.require_org_member_with_permission("volunteer.admin", req_to_org_id=getOrgId) -@bp.route("/hackathon///checkins", methods=["GET"]) def get_volunteers_checked_in_by_event_api(event_id, volunteer_type): logger.info(f"GET /hackathon/{event_id}/{volunteer_type}/checked_in called") return (get_volunteer_checked_in_by_event(event_id, volunteer_type)) @@ -491,11 +502,9 @@ def store_news(): # if token is valid, store news # else return 401 token = request.headers.get("X-Api-Key") - # Check BACKEND_NEWS_TOKEN - if token == None or token != os.getenv("BACKEND_NEWS_TOKEN"): + if not _api_key_matches(token, os.getenv("BACKEND_NEWS_TOKEN")): return "Unauthorized", 401 - else: - return vars(save_news(request.get_json())) + return vars(save_news(request.get_json())) @bp.route("/news", methods=["GET"]) def read_news(): @@ -504,10 +513,13 @@ def read_news(): # Log logger.info(f"Processing problem statements list with limit: {limit_arg}") - # If limit is set, convert to int - limit=3 + limit = 3 if limit_arg: - limit = int(limit_arg) + try: + limit = int(limit_arg) + except (TypeError, ValueError): + return {"error": "invalid_limit"}, 400 + limit = max(1, min(limit, NEWS_LIMIT_MAX)) return vars(get_news(news_limit=limit, news_id=None)) # Pass the 'limit' parameter to the get_news() function @@ -598,8 +610,7 @@ def store_praise(): sender_id = json_data.get("praise_sender") receiver_id = json_data.get("praise_receiver") - # Check BACKEND_NEWS_TOKEN - if token == None or token != os.getenv("BACKEND_PRAISE_TOKEN"): + if not _api_key_matches(token, os.getenv("BACKEND_PRAISE_TOKEN")): return "Unauthorized", 401 elif sender_id == receiver_id: return "You cannot write a praise about yourself", 400 @@ -798,12 +809,18 @@ def submit_create_hackathon(): @bp.route("/create-hackathon/", methods=["GET"]) def get_submitted_hackathon(request_id): logger.info(f"GET /create-hackathon/{request_id} called") - return get_hackathon_request_by_id(request_id) + result = get_hackathon_request_by_id(request_id) + if result is None: + return {"error": "not_found"}, 404 + return result @bp.route("/create-hackathon/", methods=["PATCH"]) def update_submitted_hackathon(request_id): logger.info(f"PATCH /create-hackathon/{request_id} called") - return update_hackathon_request(request_id, request.get_json()) + result = update_hackathon_request(request_id, request.get_json(silent=True)) + if result is None: + return {"error": "not_found"}, 404 + return result @bp.route("/upload-image", methods=["POST"]) @@ -817,18 +834,23 @@ def upload_image(): from api.messages.messages_service import upload_image_to_cdn if auth_user and auth_user.user_id: - # teams//... directories are only writable by that team's members - # (or an admin) — the team-project thumbnail validator trusts that - # prefix. Everything else is unchanged. - from services.hackathon_planning_service import is_admin - from api.submissions.submissions_service import authorize_team_upload_directory - - blocked = authorize_team_upload_directory( - auth_user.user_id, request.form.get("directory"), admin=is_admin(auth_user) + # teams//... is member-only (the project-thumbnail validator + # trusts that prefix); other shared directories are admin-only and + # non-admins may never overwrite an existing file. + from services import hackathon_planning_service as planning + from api.submissions.submissions_service import authorize_upload_directory + + admin = bool(planning.is_admin(auth_user)) + blocked = authorize_upload_directory( + auth_user.user_id, + request.form.get("directory"), + admin=admin, + # planning editors (non-admin) may attach files to their event's cards + plan_editor_check=lambda event_id: planning.can_write_plan_for_event(auth_user, event_id), ) if blocked: return blocked - return upload_image_to_cdn(request) + return upload_image_to_cdn(request, allow_overwrite=admin) else: error(logger, "Could not obtain user details for POST /upload-image") return {"error": "Unauthorized"}, 401 diff --git a/api/messages/tests/test_route_gates.py b/api/messages/tests/test_route_gates.py new file mode 100644 index 0000000..e67e72e --- /dev/null +++ b/api/messages/tests/test_route_gates.py @@ -0,0 +1,147 @@ +""" +Route-level proof that admin/secret-gated routes actually reject anonymous +callers, using the REJECTING common.auth stub (no Authorization -> 401, no +X-Org-Id -> 403). Also covers the X-Api-Key token checks and the news +`limit` parsing on the legacy messages blueprint. +""" +import importlib +import os +import sys + +os.environ.setdefault("ENVIRONMENT", "test") + +import pytest +from flask import Flask + +from test.common.auth_stubs import rejecting_auth_module + +MODULES = ( + "api.messages.messages_views", + "api.problemstatements.problem_statement_views", + "api.newsletters.newsletter_views", +) +ADMIN_HEADERS = {"Authorization": "Bearer x", "X-Org-Id": "org-1"} + + +@pytest.fixture +def load(monkeypatch): + monkeypatch.setitem(sys.modules, "common.auth", rejecting_auth_module()) + loaded = [] + + def _load(module_name): + sys.modules.pop(module_name, None) + views = importlib.import_module(module_name) + loaded.append(module_name) + app = Flask(__name__) + app.register_blueprint(views.bp) + return views, app.test_client() + + yield _load + for name in loaded: + sys.modules.pop(name, None) + + +# --- 1.1 decorator order -------------------------------------------------- + +def test_checkins_list_requires_auth(load, monkeypatch): + """Was 200 for anonymous callers: the auth decorators sat above @bp.route.""" + views, client = load("api.messages.messages_views") + monkeypatch.setattr(views, "get_volunteer_checked_in_by_event", lambda e, t: {"data": []}) + assert client.get("/api/messages/hackathon/x/hacker/checkins").status_code == 401 + assert client.get("/api/messages/hackathon/x/hacker/checkins", headers=ADMIN_HEADERS).status_code == 200 + + +def test_problem_statement_events_patch_requires_auth(load, monkeypatch): + """Was reachable anonymously for the same decorator-order reason.""" + views, client = load("api.problemstatements.problem_statement_views") + monkeypatch.setattr(views.service, "link_problem_statements_to_events", lambda body: None) + assert client.patch("/api/problem-statements/events", json={}).status_code == 401 + assert client.patch("/api/problem-statements/events", json={}, headers={"Authorization": "Bearer x"}).status_code == 403 + + +# --- 1.2 newsletter open relay -------------------------------------------- + +def test_send_newsletter_requires_admin(load, monkeypatch): + """Before: the module built its own PropelAuth client at import (ValueError + under the stub / test env), and send_newsletter had its auth commented out + so anyone could POST arbitrary addresses+body (open mail relay, 200).""" + views, client = load("api.newsletters.newsletter_views") + sent = [] + monkeypatch.setattr(views, "send_newsletters", lambda **kw: sent.append(kw)) + body = {"addresses": ["a@x"], "body": "hi", "subject": "s", "role": "r"} + assert client.post("/api/newsletter/send_newsletter", json=body).status_code == 401 + assert client.post("/api/newsletter/send_newsletter", json=body, headers={"Authorization": "Bearer x"}).status_code == 403 + assert sent == [] + assert client.post("/api/newsletter/send_newsletter", json=body, headers=ADMIN_HEADERS).status_code == 200 + assert len(sent) == 1 + + +def test_newsletter_preview_and_lookup_require_admin(load, monkeypatch): + views, client = load("api.newsletters.newsletter_views") + monkeypatch.setattr(views, "check_subscription_list", lambda **kw: "ok") + assert client.post("/api/newsletter/preview_newsletter", json={"body": "x"}).status_code == 401 + assert client.get("/api/newsletter/some-user").status_code == 401 + assert client.get("/api/newsletter/some-user", headers=ADMIN_HEADERS).status_code == 200 + + +# --- 1.10 token checks + news limit --------------------------------------- + +@pytest.fixture +def news_client(load, monkeypatch): + monkeypatch.setenv("BACKEND_NEWS_TOKEN", "right-token") + monkeypatch.setenv("BACKEND_PRAISE_TOKEN", "praise-token") + views, client = load("api.messages.messages_views") + calls = {"save": [], "get": [], "praise": []} + + class _R: + pass + + def _save(body): + calls["save"].append(body) + return _R() + + def _get(news_limit, news_id): + calls["get"].append(news_limit) + return _R() + + def _praise(body): + calls["praise"].append(body) + return _R() + + monkeypatch.setattr(views, "save_news", _save) + monkeypatch.setattr(views, "get_news", _get) + monkeypatch.setattr(views, "save_praise", _praise) + client.calls = calls + return client + + +def test_news_post_token(news_client): + assert news_client.post("/api/messages/news", json={}).status_code == 401 + assert news_client.post("/api/messages/news", json={}, headers={"X-Api-Key": "wrong"}).status_code == 401 + assert news_client.calls["save"] == [] + assert news_client.post("/api/messages/news", json={"a": 1}, headers={"X-Api-Key": "right-token"}).status_code == 200 + assert news_client.calls["save"] == [{"a": 1}] + + +def test_news_post_rejects_when_env_token_unset(news_client, monkeypatch): + monkeypatch.delenv("BACKEND_NEWS_TOKEN") + assert news_client.post("/api/messages/news", json={}, headers={"X-Api-Key": ""}).status_code == 401 + assert news_client.post("/api/messages/news", json={}, headers={"X-Api-Key": "None"}).status_code == 401 + + +def test_praise_post_token(news_client): + body = {"praise_sender": "a", "praise_receiver": "b"} + assert news_client.post("/api/messages/praise", json=body).status_code == 401 + assert news_client.post("/api/messages/praise", json=body, headers={"X-Api-Key": "wrong"}).status_code == 401 + assert news_client.post("/api/messages/praise", json=body, headers={"X-Api-Key": "praise-token"}).status_code == 200 + assert len(news_client.calls["praise"]) == 1 + + +def test_news_limit_parsing(news_client): + """Was int(limit) with no guard -> 500 on ?limit=abc, and unbounded.""" + assert news_client.get("/api/messages/news?limit=abc").status_code == 400 + assert news_client.get("/api/messages/news?limit=abc").get_json() == {"error": "invalid_limit"} + assert news_client.get("/api/messages/news?limit=99999").status_code == 200 + assert news_client.get("/api/messages/news?limit=0").status_code == 200 + assert news_client.get("/api/messages/news").status_code == 200 + assert news_client.calls["get"] == [200, 1, 3] diff --git a/api/newsletters/newsletter_views.py b/api/newsletters/newsletter_views.py index 507c3fd..0f87c29 100644 --- a/api/newsletters/newsletter_views.py +++ b/api/newsletters/newsletter_views.py @@ -8,11 +8,11 @@ request ) -from propelauth_flask import init_auth, current_user -auth = init_auth( - os.getenv("PROPEL_AUTH_URL"), - os.getenv("PROPEL_AUTH_KEY"), -) +from common.auth import auth + + +def getOrgId(req): + return req.headers.get("X-Org-Id") bp_name = 'api-newsletter' @@ -24,20 +24,20 @@ @bp.route("/") @auth.require_user -@auth.require_org_member_with_permission("admin_permissions") +@auth.require_org_member_with_permission("volunteer.admin", req_to_org_id=getOrgId) def newsletter(): return get_subscription_list() @bp.route("/") -# @auth.require_user -# @auth.require_org_member_with_permission("admin_permissions") +@auth.require_user +@auth.require_org_member_with_permission("volunteer.admin", req_to_org_id=getOrgId) def check_sub(user_id): return check_subscription_list(user_id=user_id) @bp.route("/send_newsletter", methods=["POST"]) -# @auth.require_user -# @auth.require_org_member_with_permission("admin_permissions") +@auth.require_user +@auth.require_org_member_with_permission("volunteer.admin", req_to_org_id=getOrgId) def send_newsletter(): data = request.get_json() try: @@ -49,6 +49,8 @@ def send_newsletter(): return "True" @bp.route("/preview_newsletter", methods=["POST"]) +@auth.require_user +@auth.require_org_member_with_permission("volunteer.admin", req_to_org_id=getOrgId) def preview_newsletter(): debug(logger, "Sending newsletter") data = request.get_json() @@ -62,8 +64,7 @@ def preview_newsletter(): @bp.route("//", methods=["POST"]) -@auth.require_user -# @auth.require_org_member_with_permission("admin_permissions") +@auth.require_user # per-user self-service (subscribe/verify/unsubscribe) — deliberately not admin-gated def newsletter_signup(subscribe, doc_id): debug(logger, "User authorized") if subscribe == "subscribe": diff --git a/api/problemstatements/problem_statement_views.py b/api/problemstatements/problem_statement_views.py index aceb200..c8cc554 100644 --- a/api/problemstatements/problem_statement_views.py +++ b/api/problemstatements/problem_statement_views.py @@ -85,9 +85,9 @@ def get_problem_statement_helpers(id): logger.error(f"Error in get_problem_statement_helpers: {str(e)}") return jsonify({"error": "Internal server error"}), 500 +@bp.route("/events", methods=["PATCH"]) @auth.require_user @auth.require_org_member_with_permission("volunteer.admin", req_to_org_id=getOrgId) -@bp.route("/events", methods=["PATCH"]) def update_problem_statement_events_link(): res = service.link_problem_statements_to_events(request.get_json()) diff --git a/api/store/store_views.py b/api/store/store_views.py index 0d7ead2..a22a94e 100644 --- a/api/store/store_views.py +++ b/api/store/store_views.py @@ -1,4 +1,5 @@ from flask import Blueprint, jsonify, request +import hmac import os from common.log import get_logger from common.auth import auth @@ -22,7 +23,7 @@ def verify_webhook_secret(req): """Verify the webhook shared secret from the request header.""" expected = os.environ.get('STORE_WEBHOOK_SECRET', '') provided = req.headers.get('X-Webhook-Secret', '') - return expected and provided and expected == provided + return bool(expected and provided and hmac.compare_digest(str(expected), str(provided))) @bp.route("/store/orders", methods=["POST"]) diff --git a/test/__init__.py b/test/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/test/common/__init__.py b/test/common/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/test/common/auth_stubs.py b/test/common/auth_stubs.py new file mode 100644 index 0000000..335897d --- /dev/null +++ b/test/common/auth_stubs.py @@ -0,0 +1,91 @@ +""" +Shared `common.auth` stand-ins for route-level tests. + +Route tests import a views module against a bare Flask app with +`sys.modules["common.auth"]` replaced (the real module builds a PropelAuth +client at import, which raises under ENVIRONMENT=test). Two flavours: + +- `passthrough_auth_module()` — every decorator lets the request through and + sets the fake user (the pattern api/messages/tests/test_upload_image_gate.py + started with). +- `rejecting_auth_module()` — behaves like PropelAuth at the edge: no + `Authorization` header -> 401, no `X-Org-Id` header -> 403. Use it to prove + a route is actually gated (a decorator placed ABOVE `@bp.route` is silently + dropped by Flask, so the route answers 200 to anonymous callers). +""" +import functools +import types + +from flask import g, request +from werkzeug.local import LocalProxy + +DEFAULT_FAKE_USER = types.SimpleNamespace(user_id="fake-propel-uuid", email="fake@example.com") + + +def _module(auth_ns): + stub = types.ModuleType("common.auth") + stub.auth = auth_ns + stub.auth_user = LocalProxy(lambda: getattr(g, "propelauth_current_user", None)) + return stub + + +def passthrough_auth_module(user=DEFAULT_FAKE_USER): + def _passthrough(*_args, **_kwargs): + def decorator(fn): + @functools.wraps(fn) + def wrapper(*args, **kwargs): + g.propelauth_current_user = user + return fn(*args, **kwargs) + + return wrapper + + return decorator + + return _module( + types.SimpleNamespace( + require_org_member_with_permission=_passthrough, + require_user=_passthrough(), + optional_user=_passthrough(), + ) + ) + + +def rejecting_auth_module(user=DEFAULT_FAKE_USER): + def require_user(fn): + @functools.wraps(fn) + def wrapper(*args, **kwargs): + if not request.headers.get("Authorization"): + return {"error": "unauthorized"}, 401 + g.propelauth_current_user = user + return fn(*args, **kwargs) + + return wrapper + + def require_org_member_with_permission(_perm, req_to_org_id=None): + def decorator(fn): + @functools.wraps(fn) + def wrapper(*args, **kwargs): + if not request.headers.get("X-Org-Id"): + return {"error": "forbidden"}, 403 + return fn(*args, **kwargs) + + return wrapper + + return decorator + + def optional_user(fn): + @functools.wraps(fn) + def wrapper(*args, **kwargs): + if request.headers.get("Authorization"): + g.propelauth_current_user = user + return fn(*args, **kwargs) + + return wrapper + + return _module( + types.SimpleNamespace( + require_user=require_user, + require_org_member_with_permission=require_org_member_with_permission, + optional_user=optional_user, + ) + ) diff --git a/test/common/test_payload_too_large.py b/test/common/test_payload_too_large.py new file mode 100644 index 0000000..449f8b6 --- /dev/null +++ b/test/common/test_payload_too_large.py @@ -0,0 +1,28 @@ +"""An over-limit body must get a JSON 413 on /api/ paths (frontend callers +do res.json(); werkzeug's default is an HTML page).""" +import os + +os.environ.setdefault("ENVIRONMENT", "test") + +from flask import Flask, request + +from api import exception_views + + +def test_413_is_json(): + app = Flask(__name__) + app.config["MAX_CONTENT_LENGTH"] = 10 + app.register_blueprint(exception_views.bp) + + @app.route("/api/x", methods=["POST"]) + def _x(): + return {"keys": list(request.form)} + + resp = app.test_client().post("/api/x", data={"f": "x" * 100}) + assert resp.status_code == 413 + assert resp.get_json() == {"error": "payload_too_large", "max_bytes": 10} + + +def test_app_sets_max_content_length(): + src = open(os.path.join(os.path.dirname(__file__), "..", "..", "api", "__init__.py")).read() + assert 'app.config["MAX_CONTENT_LENGTH"] = 32 * 1024 * 1024' in src diff --git a/test/common/test_view_decorator_order.py b/test/common/test_view_decorator_order.py new file mode 100644 index 0000000..360e69f --- /dev/null +++ b/test/common/test_view_decorator_order.py @@ -0,0 +1,56 @@ +""" +Flask registers whatever function `@bp.route` sees. An auth decorator placed +ABOVE `@bp.route` wraps the already-registered function, so the live route +runs WITHOUT the check — two admin routes were publicly readable/writable +this way (checkins list, problem-statement events PATCH). This walks every +views module and requires the route decorator to be the outermost one on +any auth-decorated function. +""" +import ast +import pathlib + +REPO_ROOT = pathlib.Path(__file__).resolve().parents[2] + + +def _decorator_name(node): + target = node.func if isinstance(node, ast.Call) else node + parts = [] + while isinstance(target, ast.Attribute): + parts.append(target.attr) + target = target.value + if isinstance(target, ast.Name): + parts.append(target.id) + return ".".join(reversed(parts)) + + +def _is_auth(name): + return name.startswith("auth.require_") or name == "auth.optional_user" + + +def _is_route(node): + return isinstance(node, ast.Call) and _decorator_name(node).endswith(".route") + + +def find_misordered(): + offenders = [] + for path in sorted((REPO_ROOT / "api").rglob("*_views.py")): + if "/tests/" in path.as_posix(): + continue + tree = ast.parse(path.read_text(), filename=str(path)) + for fn in ast.walk(tree): + if not isinstance(fn, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + decs = fn.decorator_list + if not any(_is_auth(_decorator_name(d)) for d in decs): + continue + if not decs or not _is_route(decs[0]): + offenders.append(f"{path.relative_to(REPO_ROOT)}:{fn.name}") + return offenders + + +def test_route_decorator_is_outermost_on_auth_gated_views(): + offenders = find_misordered() + assert offenders == [], ( + "@bp.route must be the FIRST (outermost) decorator, otherwise the auth " + "decorators above it are never applied: " + ", ".join(offenders) + ) From 2df72f5e0bf45da9f0eee96b8fec9d6bc8d8f1ce Mon Sep 17 00:00:00 2001 From: Greg V <6913307+gregv@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:21:04 +0200 Subject: [PATCH 2/8] security: upload-image directory allowlist for non-admins, no overwrite, planning-editor allowance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #289 gated teams// only; any logged-in user could still write — and overwrite — ohack.dev/logos, hackathons//photos, nonprofits, news/... authorize_upload_directory (api/submissions/submissions_service.py) is now the single gate the view calls: teams/... delegates to the existing membership gate; non-admins may otherwise write only the single-segment application photo directories (images hackers volunteers mentors judges sponsors uploads) or hackathons//planning/... when can_write_plan_for_event admits them (card attachments); admins anywhere; '..' or odd characters -> 400. upload_image_to_cdn(request, allow_overwrite=False) answers 409 file_exists for non-admins via the new cdn.blob_exists() — upload_to_cdn itself still overwrites because certificates/hearts/openai rely on that. Tests: test_upload_image_gate.py (403 for shared dirs — was 200; planning editor case), test_upload_overwrite.py. Co-Authored-By: Claude Fable 5.1 --- api/messages/messages_service.py | 22 ++++-- api/messages/tests/test_upload_image_gate.py | 70 ++++++++++++++++++-- api/messages/tests/test_upload_overwrite.py | 57 ++++++++++++++++ api/submissions/submissions_service.py | 40 +++++++++++ common/utils/cdn.py | 8 +++ services/hackathon_planning_service.py | 11 +++ 6 files changed, 194 insertions(+), 14 deletions(-) create mode 100644 api/messages/tests/test_upload_overwrite.py diff --git a/api/messages/messages_service.py b/api/messages/messages_service.py index 4b147d4..9078fb1 100644 --- a/api/messages/messages_service.py +++ b/api/messages/messages_service.py @@ -438,17 +438,20 @@ def get_user_by_id_old(id): return get_profile_by_db_id(id) or {} -def upload_image_to_cdn(request): +def upload_image_to_cdn(request, allow_overwrite=True): """ Upload an image to CDN. Accepts binary data, base64, or standard image formats. Returns the CDN URL of the uploaded image. + + allow_overwrite=False (non-admin uploads) makes the multipart path answer + 409 file_exists instead of replacing an existing blob. """ import base64 import tempfile import mimetypes from werkzeug.utils import secure_filename - from common.utils.cdn import upload_to_cdn - + from common.utils import cdn + logger.info("Starting image upload to CDN") try: @@ -473,6 +476,11 @@ def upload_image_to_cdn(request): if not _is_image_file(filename): logger.warning(f"Upload failed: File is not an image: {filename}") return {"success": False,"error": "File must be an image"}, 400 + + # temp basename == filename, so this is the exact blob upload_to_cdn writes. + if not allow_overwrite and cdn.blob_exists(_directory, filename): + logger.warning(f"Upload refused: {_directory}/{filename} already exists") + return {"success": False, "error": "file_exists"}, 409 # Create a properly named temporary file import tempfile @@ -490,7 +498,7 @@ def upload_image_to_cdn(request): try: # Upload to CDN using the properly named temp file logger.info(f"Uploading {filename} to CDN from {temp_filepath}") - cdn_url = upload_to_cdn(_directory, temp_filepath, destination_filename) + cdn_url = cdn.upload_to_cdn(_directory, temp_filepath, destination_filename) logger.info(f"Successfully uploaded image to CDN: {cdn_url}") return {"success": True, "url": cdn_url, "message": "Image uploaded successfully"} @@ -547,7 +555,7 @@ def upload_image_to_cdn(request): try: # Upload to CDN using the properly named temp file logger.info(f"Uploading base64 image {filename} to CDN from {temp_filepath}") - cdn_url = upload_to_cdn("images", temp_filepath, destination_filename) + cdn_url = cdn.upload_to_cdn("images", temp_filepath, destination_filename) logger.info(f"Successfully uploaded base64 image to CDN: {cdn_url}") return {"success": True, "url": cdn_url, "message": "Image uploaded successfully"} @@ -593,7 +601,7 @@ def upload_image_to_cdn(request): try: # Upload to CDN using the properly named temp file logger.info(f"Uploading binary image {filename} to CDN from {temp_filepath}") - cdn_url = upload_to_cdn("images", temp_filepath, destination_filename) + cdn_url = cdn.upload_to_cdn("images", temp_filepath, destination_filename) logger.info(f"Successfully uploaded binary image to CDN: {cdn_url}") return {"success": True, "url": cdn_url, "message": "Image uploaded successfully"} @@ -632,7 +640,7 @@ def upload_image_to_cdn(request): try: # Upload to CDN using the properly named temp file logger.info(f"Uploading raw image {filename} to CDN from {temp_filepath}") - cdn_url = upload_to_cdn("images", temp_filepath, destination_filename) + cdn_url = cdn.upload_to_cdn("images", temp_filepath, destination_filename) logger.info(f"Successfully uploaded raw image to CDN: {cdn_url}") return { diff --git a/api/messages/tests/test_upload_image_gate.py b/api/messages/tests/test_upload_image_gate.py index 57ae82e..695efa2 100644 --- a/api/messages/tests/test_upload_image_gate.py +++ b/api/messages/tests/test_upload_image_gate.py @@ -47,16 +47,21 @@ def client(monkeypatch): views = importlib.import_module(VIEWS_MODULE) uploads = [] - monkeypatch.setattr( - "api.messages.messages_service.upload_image_to_cdn", - lambda request: uploads.append(request.form.get("directory")) or {"success": True, "url": "https://cdn.test/x.png"}, - ) + overwrite_flags = [] + + def _fake_upload(request, allow_overwrite=True): + uploads.append(request.form.get("directory")) + overwrite_flags.append(allow_overwrite) + return {"success": True, "url": "https://cdn.test/x.png"} + + monkeypatch.setattr("api.messages.messages_service.upload_image_to_cdn", _fake_upload) monkeypatch.setattr("services.hackathon_planning_service.is_admin", lambda user: False) app = Flask(__name__) app.register_blueprint(views.bp) test_client = app.test_client() test_client.uploads = uploads + test_client.overwrite_flags = overwrite_flags yield test_client sys.modules.pop(VIEWS_MODULE, None) @@ -84,8 +89,59 @@ def test_member_can_upload_into_their_teams_directory(client, monkeypatch): assert client.uploads == ["teams/my-team/project"] -def test_other_directories_are_unaffected(client, monkeypatch): +def test_non_admin_cannot_write_shared_site_directories(client, monkeypatch): + """Was 200: any logged-in user could (over)write site assets like + ohack.dev/logos or an event's photo gallery. (Replaces the old + `test_other_directories_are_unaffected`, which asserted exactly that.)""" monkeypatch.setattr("api.teams.teams_service.user_is_on_team", lambda propel, team_id: False) - response = _post(client, "nonprofits") + for directory in ("ohack.dev/logos", "hackathons/x/photos", "nonprofits", "images/nested"): + response = _post(client, directory) + assert response.status_code == 403, directory + assert response.get_json()["error"] == "directory_not_allowed" + assert client.uploads == [] + + +def test_non_admin_can_write_application_photo_directories(client): + for directory in ("hackers", "mentors", "judges", "volunteers", "sponsors", "images", "uploads"): + assert _post(client, directory).status_code == 200, directory + assert client.overwrite_flags == [False] * 7 + + +def test_missing_directory_defaults_to_images(client): + response = client.post( + "/api/messages/upload-image", + data={"file": (io.BytesIO(b"x"), "thumb.png")}, + content_type="multipart/form-data", + ) assert response.status_code == 200 - assert client.uploads == ["nonprofits"] + + +def test_admin_may_write_any_valid_directory_and_overwrite(client, monkeypatch): + monkeypatch.setattr("services.hackathon_planning_service.is_admin", lambda user: True) + assert _post(client, "ohack.dev/logos").status_code == 200 + assert client.overwrite_flags == [True] + + +def test_traversal_and_odd_characters_rejected(client, monkeypatch): + monkeypatch.setattr("services.hackathon_planning_service.is_admin", lambda user: True) + for directory in ("../x", "hackers/../teams/t", "a b", "x;rm"): + response = _post(client, directory) + assert response.status_code == 400, directory + assert response.get_json()["error"] == "invalid_directory" + assert client.uploads == [] + + +def test_planning_editor_can_attach_files_to_their_events_cards(client, monkeypatch): + """Non-admin planning editors upload card attachments under + hackathons//planning/cards/; the per-event editor check + (services.hackathon_planning_service.can_write_plan_for_event) admits them. + Other subtrees of the same event stay admin-only.""" + monkeypatch.setattr( + "services.hackathon_planning_service.can_write_plan_for_event", + lambda user, event_id: event_id == "2026_fall", + ) + assert _post(client, "hackathons/2026_fall/planning/cards/abc").status_code == 200 + assert _post(client, "hackathons/other_event/planning/cards/abc").status_code == 403 + assert _post(client, "hackathons/2026_fall/photos").status_code == 403 + assert client.uploads == ["hackathons/2026_fall/planning/cards/abc"] + assert client.overwrite_flags == [False] diff --git a/api/messages/tests/test_upload_overwrite.py b/api/messages/tests/test_upload_overwrite.py new file mode 100644 index 0000000..22e9f4e --- /dev/null +++ b/api/messages/tests/test_upload_overwrite.py @@ -0,0 +1,57 @@ +""" +upload_image_to_cdn(allow_overwrite=False) must refuse to replace an existing +blob — non-admin uploads used to silently overwrite whatever lived at +/ (upload_to_cdn always overwrites by design; it's +shared with intentional overwriters, so the check lives in the caller). +""" +import io +import os + +os.environ.setdefault("ENVIRONMENT", "test") + +import pytest +from flask import Flask, request + +import api.messages.messages_service as svc + + +@pytest.fixture +def app(monkeypatch): + monkeypatch.setattr(svc, "_optimize_image_for_web", lambda path: None, raising=False) + return Flask(__name__) + + +def _call(app, monkeypatch, exists, allow_overwrite): + uploaded, checked = [], [] + monkeypatch.setattr("common.utils.cdn.blob_exists", lambda d, f: checked.append((d, f)) or exists) + monkeypatch.setattr( + "common.utils.cdn.upload_to_cdn", + lambda d, src, dest=None: uploaded.append((d, dest)) or f"https://cdn.test/{d}/{dest}", + ) + with app.test_request_context( + "/api/messages/upload-image", + method="POST", + data={"file": (io.BytesIO(b"x"), "pic.png"), "directory": "hackers"}, + content_type="multipart/form-data", + ): + return svc.upload_image_to_cdn(request, allow_overwrite=allow_overwrite), uploaded, checked + + +def test_existing_blob_is_409_without_overwrite(app, monkeypatch): + result, uploaded, checked = _call(app, monkeypatch, exists=True, allow_overwrite=False) + assert result == ({"success": False, "error": "file_exists"}, 409) + assert uploaded == [] + assert checked == [("hackers", "pic.png")] + + +def test_new_blob_uploads_without_overwrite(app, monkeypatch): + result, uploaded, _ = _call(app, monkeypatch, exists=False, allow_overwrite=False) + assert result["success"] is True + assert uploaded == [("hackers", "pic.png")] + + +def test_overwrite_allowed_skips_the_check(app, monkeypatch): + result, uploaded, checked = _call(app, monkeypatch, exists=True, allow_overwrite=True) + assert result["success"] is True + assert uploaded == [("hackers", "pic.png")] + assert checked == [] diff --git a/api/submissions/submissions_service.py b/api/submissions/submissions_service.py index 6d4ba73..a0fd3bf 100644 --- a/api/submissions/submissions_service.py +++ b/api/submissions/submissions_service.py @@ -25,6 +25,7 @@ """ import logging import os +import re from datetime import datetime, timedelta, timezone from db.db import get_db @@ -280,6 +281,45 @@ def authorize_team_upload_directory(propel_user_id, directory, admin=False): return None +# Single-segment directories any logged-in user may upload into (the +# application-form photo pickers + generic images). Everything else — site +# assets, event galleries, nonprofit logos, blog/planning media — is +# admin-only; teams//... keeps its own membership gate. +USER_UPLOAD_DIRECTORIES = frozenset({"images", "hackers", "volunteers", "mentors", "judges", "sponsors", "uploads"}) +_UPLOAD_DIRECTORY_RE = re.compile(r"^[A-Za-z0-9_.\-/]+$") # "." for ohack.dev/...; ".." rejected separately + + +def authorize_upload_directory(propel_user_id, directory, admin=False, plan_editor_check=None): + """Gate for every POST /api/messages/upload-image `directory`. + + None = proceed, else a ready-to-return (payload, status): + - 400 invalid_directory — characters outside [A-Za-z0-9_.-/] or a `..` + - 403 not_team_member — teams//... (see authorize_team_upload_directory) + - 403 directory_not_allowed — non-admin writing outside USER_UPLOAD_DIRECTORIES + Empty/None means the service default, `images`. + `plan_editor_check(event_id) -> bool` (optional) lets a non-admin planning + editor write under hackathons//planning/... (card attachments). + """ + raw = str(directory).replace("\\", "/") if directory else "images" + if ".." in raw or not _UPLOAD_DIRECTORY_RE.match(raw): + return {"error": "invalid_directory"}, 400 + parts = [p for p in raw.split("/") if p not in ("", ".")] + if not parts: + return {"error": "invalid_directory"}, 400 + if parts[0] == "teams": + return authorize_team_upload_directory(propel_user_id, directory, admin=admin) + if admin: + return None + if len(parts) == 1 and parts[0] in USER_UPLOAD_DIRECTORIES: + return None + if ( + parts[0] == "hackathons" and len(parts) >= 3 and parts[2] == "planning" + and callable(plan_editor_check) and plan_editor_check(parts[1]) + ): + return None + return {"error": "directory_not_allowed"}, 403 + + def validate_project_payload(payload, team_id, existing=None): """(clean, errors[{field, reason}]) — a partial update: only keys present in `payload` are validated/returned. `existing` is the team's current doc diff --git a/common/utils/cdn.py b/common/utils/cdn.py index a627dd8..d0004f0 100644 --- a/common/utils/cdn.py +++ b/common/utils/cdn.py @@ -89,6 +89,14 @@ def get_blob_metadata(path): return {"exists": True, "size": blob.size, "content_type": blob.content_type} +def blob_exists(directory, filename): + """True when / already exists — the same path + upload_to_cdn writes to. Lets callers refuse an overwrite without changing + upload_to_cdn (which some callers rely on to overwrite).""" + bucket = _get_bucket() + return bucket.blob(f"{directory}/{filename}").exists() + + def delete_from_cdn(path): """Best-effort delete of a blob path. Returns True when deleted.""" try: diff --git a/services/hackathon_planning_service.py b/services/hackathon_planning_service.py index 81d6853..978fcc0 100644 --- a/services/hackathon_planning_service.py +++ b/services/hackathon_planning_service.py @@ -68,6 +68,17 @@ def can_write_plan(propel_user, hackathon_doc) -> bool: return propel_user.user_id in editors +def can_write_plan_for_event(propel_user, event_id) -> bool: + """can_write_plan() resolved by event_id; False on any lookup problem. + Used by the upload-image directory gate for hackathons//planning/...""" + try: + if not isinstance(event_id, str) or not event_id.strip(): + return False + return can_write_plan(propel_user, get_hackathon_by_event_id(event_id)) + except Exception: + return False + + def can_comment(propel_user) -> bool: """Any logged-in user can comment on an enabled plan.""" return bool(propel_user and getattr(propel_user, "user_id", None)) From 82026498e1e752af83f9df2f412f959d4d625512 Mon Sep 17 00:00:00 2001 From: Greg V <6913307+gregv@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:21:04 +0200 Subject: [PATCH 3/8] =?UTF-8?q?security:=20hackathon-request=20edit=20link?= =?UTF-8?q?=20=E2=80=94=20field=20allowlist,=20404=20before=20email,=20con?= =?UTF-8?q?firm=20to=20the=20stored=20contact?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PATCH /api/messages/create-hackathon/ is the anonymous requester's edit link (public by design). It did doc.update(json) with no allowlist (status was settable) and emailed the BODY's contactEmail before checking the doc existed (missing doc -> 500). Now: body filtered to HACKATHON_REQUEST_EDITABLE_FIELDS (= the frontend HackathonRequestForm formData keys, lockstep-tested), None when the doc is missing (view -> 404) before any email, confirmation to the STORED contact, adminNotes stripped from the public GET and PATCH responses, new ids uuid4. admin_update_hackathon_request is untouched. Tests: api/messages/tests/test_hackathon_requests.py (+7). Co-Authored-By: Claude Fable 5.1 --- api/messages/tests/test_hackathon_requests.py | 119 ++++++++++++++++++ services/hackathons_service.py | 57 ++++++--- 2 files changed, 161 insertions(+), 15 deletions(-) diff --git a/api/messages/tests/test_hackathon_requests.py b/api/messages/tests/test_hackathon_requests.py index bc74946..ba5d0e8 100644 --- a/api/messages/tests/test_hackathon_requests.py +++ b/api/messages/tests/test_hackathon_requests.py @@ -400,3 +400,122 @@ def test_custom_theme_and_dates_humanized(self): }) assert "Custom — AI for accessibility" in html assert "February" in html and "2027" in html + + +# The frontend form's initial `formData` keys — copied from +# frontend-ohack.dev/src/components/HackathonRequest/HackathonRequestForm.js +# (useState(initialData || {...})). Keep in lockstep: a key missing from the +# backend allowlist is silently dropped from requester edits. +FRONTEND_FORM_KEYS = [ + "companyName", "organizationType", "contactName", "contactEmail", "contactPhone", + "employeeCount", "participantType", "hackathonTheme", "customTheme", + "expectedHackathonDate", "preferredDate", "alternateDate", "location", "eventFormat", + "hasNonprofitList", "nonprofitDetails", "hasWorkedWithNonprofitsBefore", + "nonprofitSource", "preferredNonprofitLocation", "specificRegion", + "responsibilities", "budget", "donationPercentage", "additionalInfo", + "agreeToContact", "agreeToTimeline", +] + + +def _request_doc(mock_db, stored): + mock_doc_ref = MagicMock() + snapshot = MagicMock() + snapshot.exists = stored is not None + snapshot.to_dict.return_value = stored + mock_doc_ref.get.return_value = snapshot + mock_db.return_value.collection.return_value.document.return_value = mock_doc_ref + return mock_doc_ref + + +class TestUpdateHackathonRequest: + """Public requester edit (capability link) — must not be a raw doc.update(json).""" + + def test_form_keys_are_all_editable(self): + from services.hackathons_service import HACKATHON_REQUEST_EDITABLE_FIELDS + assert set(FRONTEND_FORM_KEYS) <= set(HACKATHON_REQUEST_EDITABLE_FIELDS) + for staff_key in ("status", "adminNotes", "created", "id", "updated"): + assert staff_key not in HACKATHON_REQUEST_EDITABLE_FIELDS + + @patch('services.hackathons_service.send_hackathon_request_email') + @patch('services.hackathons_service.send_slack_audit') + @patch('services.hackathons_service._get_db') + def test_body_filtered_and_email_goes_to_stored_contact(self, mock_db, mock_audit, mock_email): + """Before: doc.update got the whole body (status/adminNotes writable by + anyone with the link) and the email went to body.contactEmail.""" + ref = _request_doc(mock_db, {"companyName": "old", "contactName": "Owner", "contactEmail": "owner@x", "status": "pending"}) + + result = update_hackathon_request("req-1", { + "status": "approved", "adminNotes": "pwned", "id": "other", "created": "x", + "companyName": "x", + }) + + ref.update.assert_called_once() + written = ref.update.call_args[0][0] + assert set(written) == {"companyName", "updated"} + assert written["companyName"] == "x" + datetime.fromisoformat(written["updated"]) + mock_email.assert_called_once() + assert mock_email.call_args[0][0] == "Owner" + assert mock_email.call_args[0][1] == "owner@x" + assert result is not None + + @patch('services.hackathons_service.send_hackathon_request_email') + @patch('services.hackathons_service.send_slack_audit') + @patch('services.hackathons_service._get_db') + def test_contact_email_change_does_not_redirect_confirmation(self, mock_db, mock_audit, mock_email): + ref = _request_doc(mock_db, {"contactName": "Owner", "contactEmail": "owner@x"}) + update_hackathon_request("req-1", {"contactEmail": "attacker@x", "companyName": "x"}) + assert mock_email.call_args[0][1] == "owner@x" + + @patch('services.hackathons_service.send_hackathon_request_email') + @patch('services.hackathons_service.send_slack_audit') + @patch('services.hackathons_service._get_db') + def test_missing_doc_returns_none_without_email(self, mock_db, mock_audit, mock_email): + """Before: emailed body.contactEmail, then crashed on None.update/None dict.""" + ref = _request_doc(mock_db, None) + assert update_hackathon_request("nope", {"contactName": "A", "contactEmail": "a@x"}) is None + mock_email.assert_not_called() + ref.update.assert_not_called() + + +class TestHackathonRequestPublicRoutes: + @pytest.fixture + def client(self, monkeypatch): + import importlib, sys + from flask import Flask + from test.common.auth_stubs import passthrough_auth_module + monkeypatch.setitem(sys.modules, "common.auth", passthrough_auth_module()) + sys.modules.pop("api.messages.messages_views", None) + views = importlib.import_module("api.messages.messages_views") + app = Flask(__name__) + app.register_blueprint(views.bp) + yield views, app.test_client() + sys.modules.pop("api.messages.messages_views", None) + + def test_patch_missing_request_is_404(self, client, monkeypatch): + """Before: the view returned the service's None verbatim -> 500.""" + views, c = client + monkeypatch.setattr(views, "update_hackathon_request", lambda rid, body: None) + resp = c.patch("/api/messages/create-hackathon/nope", json={"companyName": "x"}) + assert resp.status_code == 404 + assert resp.get_json() == {"error": "not_found"} + + +class TestPublicGetAndCreate: + @patch('services.hackathons_service.send_slack_audit') + @patch('services.hackathons_service._get_db') + def test_public_get_strips_admin_notes(self, mock_db, mock_audit): + _request_doc(mock_db, {"companyName": "A", "adminNotes": "internal"}) + result = get_hackathon_request_by_id("req-1") + assert result["companyName"] == "A" + assert "adminNotes" not in result + + @patch('services.hackathons_service.send_slack') + @patch('services.hackathons_service.send_hackathon_request_email') + @patch('services.hackathons_service.send_slack_audit') + @patch('services.hackathons_service._get_db') + def test_create_uses_random_uuid4_ids(self, mock_db, mock_audit, mock_email, mock_slack): + """uuid1 ids embed the host MAC + timestamp (guessable capability link).""" + import uuid + result = create_hackathon({"companyName": "A"}) + assert uuid.UUID(hex=result["id"]).version == 4 diff --git a/services/hackathons_service.py b/services/hackathons_service.py index 1d38fd7..6c09ba8 100644 --- a/services/hackathons_service.py +++ b/services/hackathons_service.py @@ -1085,7 +1085,7 @@ def create_hackathon(json): logger.debug("Hackathon Create") send_slack_audit(action="create_hackathon", message="Creating", payload=json) - doc_id = uuid.uuid1().hex + doc_id = uuid.uuid4().hex collection = db.collection('hackathon_requests') json["created"] = datetime.now().isoformat() json["status"] = "pending" @@ -1110,29 +1110,56 @@ def get_hackathon_request_by_id(doc_id): db = _get_db() logger.debug("Hackathon Request Get") doc = db.collection('hackathon_requests').document(doc_id) - if doc: - doc_dict = doc.get().to_dict() - send_slack_audit(action="get_hackathon_request_by_id", message="Getting", payload=doc_dict) - return doc_dict - else: + doc_dict = doc.get().to_dict() + if doc_dict is None: return None + send_slack_audit(action="get_hackathon_request_by_id", message="Getting", payload=doc_dict) + # Public capability-link read: staff-only notes never leave the admin API. + doc_dict.pop("adminNotes", None) + return doc_dict + + +# Fields an (anonymous) requester may change through the emailed edit link — +# exactly the frontend HackathonRequestForm's `formData` keys. Staff fields +# (status, adminNotes, created, id, ...) are only writable via +# admin_update_hackathon_request. Keep in lockstep with the form +# (api/messages/tests/test_hackathon_requests.py::FRONTEND_FORM_KEYS). +HACKATHON_REQUEST_EDITABLE_FIELDS = frozenset({ + "companyName", "organizationType", "contactName", "contactEmail", "contactPhone", + "employeeCount", "participantType", "hackathonTheme", "customTheme", + "expectedHackathonDate", "preferredDate", "alternateDate", "location", "eventFormat", + "hasNonprofitList", "nonprofitDetails", "hasWorkedWithNonprofitsBefore", + "nonprofitSource", "preferredNonprofitLocation", "specificRegion", + "responsibilities", "budget", "donationPercentage", "additionalInfo", + "agreeToContact", "agreeToTimeline", +}) def update_hackathon_request(doc_id, json): + """Public requester edit. Returns None when the request doesn't exist.""" db = _get_db() logger.debug("Hackathon Request Update") doc = db.collection('hackathon_requests').document(doc_id) - if doc: - doc_dict = doc.get().to_dict() - send_slack_audit(action="update_hackathon_request", message="Updating", payload=doc_dict) - send_hackathon_request_email(json["contactName"], json["contactEmail"], doc_id, request_data=json) - doc_dict["updated"] = datetime.now().isoformat() - - doc.update(json) - return doc_dict - else: + stored = doc.get().to_dict() + if stored is None: return None + body = json if isinstance(json, dict) else {} + updates = {k: v for k, v in body.items() if k in HACKATHON_REQUEST_EDITABLE_FIELDS} + updates["updated"] = datetime.now().isoformat() + send_slack_audit(action="update_hackathon_request", message="Updating", payload=updates) + + doc.update(updates) + merged = {**stored, **updates} + + # Confirm to the contact ON FILE — never to an address supplied in the + # body, or the edit link becomes a way to redirect the request's email. + if stored.get("contactEmail"): + send_hackathon_request_email(stored.get("contactName"), stored["contactEmail"], doc_id, request_data=merged) + + merged.pop("adminNotes", None) + return merged + def get_all_hackathon_requests(): db = _get_db() From 1e07dedcc06072d8df129f605b8d54f247aa9acc Mon Sep 17 00:00:00 2001 From: Greg V <6913307+gregv@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:21:05 +0200 Subject: [PATCH 4/8] security: strip team internals and member PII from public team payloads; admin-only full-doc route MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit admin_notes, nonprofit_rankings, comments and communication_history shipped on GET /api/messages/team/, /teams, the unauthenticated event payload, /api/team//me and GET /api/team/ — the last one also returned full member user docs (email_address) to any logged-in user. services/teams_service.py::public_team_view strips PUBLIC_TEAM_STRIPPED_FIELDS in every public/member getter; the hackathon list route trims member docs to {id,user_id,name,nickname,profile_image} for non-admins and keeps the full payload for admins (TeamManagement, judging admin views). New GET /api/team/admin/ (volunteer.admin) returns the full doc; the frontend's adminTeamApi prefers it and falls back on 404. mentor_* fields are public by design and untouched. Tests: api/teams/tests/test_public_team_projection.py (11). Co-Authored-By: Claude Fable 5.1 --- api/teams/teams_service.py | 25 +- api/teams/teams_views.py | 21 +- .../tests/test_public_team_projection.py | 246 ++++++++++++++++++ services/hackathons_service.py | 5 + services/teams_service.py | 39 ++- 5 files changed, 326 insertions(+), 10 deletions(-) create mode 100644 api/teams/tests/test_public_team_projection.py diff --git a/api/teams/teams_service.py b/api/teams/teams_service.py index 130821b..172f828 100644 --- a/api/teams/teams_service.py +++ b/api/teams/teams_service.py @@ -3,7 +3,7 @@ from datetime import datetime from db.db import get_db, get_user_doc_reference from api.messages.messages_service import get_problem_statement_from_id_old -from services.teams_service import get_teams_list, get_team +from services.teams_service import get_teams_list, get_team, public_team_view from services.nonprofits_service import get_single_npo from common.utils.firestore_helpers import clear_all_caches as clear_cache from services.users_service import ( @@ -647,7 +647,7 @@ def get_my_teams_by_event_id(propel_id, event_id): del team_data["users"] if "problem_statements" in team_data: del team_data["problem_statements"] - teams.append(team_data) + teams.append(public_team_view(team_data)) break # No need to check other users in this team logger.debug("Teams data: %s", teams) @@ -795,7 +795,6 @@ def get_teams_by_hackathon_id(hackathon_id): team_data.pop(field, None) team_data["team_members"] = users - #logger.debug("Team data: %s", team_data) teams.append(team_data) logger.info(f"Retrieved {len(teams)} teams for hackathon {hackathon_id}") @@ -806,6 +805,26 @@ def get_teams_by_hackathon_id(hackathon_id): return {"teams": []} +# Same slim member shape services.teams_service._enrich_team_users produces. +_SLIM_MEMBER_FIELDS = ("id", "user_id", "name", "nickname", "profile_image") + + +def public_hackathon_teams_view(payload): + """Non-admin projection of get_teams_by_hackathon_id's payload: team + internals stripped and each member trimmed to the slim public profile + (the full user docs carry email_address etc.).""" + teams = [] + for team in (payload or {}).get("teams") or []: + view = public_team_view(team) + if isinstance(view, dict) and isinstance(view.get("team_members"), list): + view["team_members"] = [ + {k: m.get(k) for k in _SLIM_MEMBER_FIELDS} if isinstance(m, dict) else m + for m in view["team_members"] + ] + teams.append(view) + return {**(payload or {}), "teams": teams} + + def _normalize_repo_link(link): return (link or "").strip().rstrip("/").lower() diff --git a/api/teams/teams_views.py b/api/teams/teams_views.py index d96b80b..a62ac8b 100644 --- a/api/teams/teams_views.py +++ b/api/teams/teams_views.py @@ -13,6 +13,7 @@ remove_team_member, remove_team, get_teams_by_hackathon_id, + public_hackathon_teams_view, get_my_teams_by_event_id, send_team_message, toggle_completion_item, @@ -38,7 +39,13 @@ def get_teams_by_hackathon_id_api(hackathon_id): """ logger.info(f"GET /team/{hackathon_id} called") if auth_user and auth_user.user_id: - return get_teams_by_hackathon_id(hackathon_id) + payload = get_teams_by_hackathon_id(hackathon_id) + # Admins (TeamManagement, judging admin) keep the full payload; anyone + # else gets team internals stripped and slim member profiles. + from services import hackathon_planning_service + if hackathon_planning_service.is_admin(auth_user): + return payload + return public_hackathon_teams_view(payload) logger.error("Could not obtain user details for GET /team/") return {"error": "Unauthorized"}, 401 @@ -237,6 +244,18 @@ def approve_team_assignment(): logger.error("Could not obtain user details for POST /team/approve") return {"error": "Unauthorized"}, 401 +@bp.route("/admin/", methods=["GET"]) +@auth.require_user +@auth.require_org_member_with_permission("volunteer.admin", req_to_org_id=getOrgId) +def get_team_admin_api(teamid): + """Admin-only full team doc (admin_notes, nonprofit_rankings, comments, + communication_history) — the public team routes strip those.""" + from services import teams_service as services_teams + team = services_teams.get_team_admin(teamid) + if not team: + return {"error": "not_found"}, 404 + return {"team": team} + @bp.route("/admin//message", methods=["POST"]) @auth.require_user @auth.require_org_member_with_permission("volunteer.admin", req_to_org_id=getOrgId) diff --git a/api/teams/tests/test_public_team_projection.py b/api/teams/tests/test_public_team_projection.py new file mode 100644 index 0000000..8a54d2d --- /dev/null +++ b/api/teams/tests/test_public_team_projection.py @@ -0,0 +1,246 @@ +""" +Team docs carry staff-only internals (admin_notes, nonprofit_rankings, +comments, communication_history). Every public/member-facing getter must strip +them via services.teams_service.public_team_view; mentor_* fields stay public +by design. Admins read the full doc through GET /api/team/admin/, and +GET /api/team/ keeps the full payload only for admins. +""" +import functools +import importlib +import os +import sys +import types +from unittest.mock import MagicMock + +os.environ.setdefault("ENVIRONMENT", "test") + +import flask +import pytest +from flask import Flask, g +from werkzeug.local import LocalProxy + +import services.hackathons_service as hs +import services.teams_service as ts + +PRIVATE = ("admin_notes", "nonprofit_rankings", "comments", "communication_history") + + +def _full_team(team_id="team-1"): + return { + "id": team_id, + "name": "Rockets", + "admin_notes": "flaky team", + "nonprofit_rankings": ["npo-a"], + "comments": "we want npo-a", + "communication_history": [{"text": "hi"}], + "mentor_ratings": [{"criterion": "scope", "rating": 3}], + "mentor_notes": [{"text": "good"}], + "users": [], + } + + +def test_public_team_view_strips_private_keys_and_is_none_safe(): + team = _full_team() + view = ts.public_team_view(team) + for key in PRIVATE: + assert key not in view + assert view["mentor_ratings"] == team["mentor_ratings"] + assert "admin_notes" in team # input untouched + assert ts.public_team_view(None) is None + + +def _db_with_team(team): + doc = MagicMock() + doc.exists = True + doc.id = team["id"] + db = MagicMock() + db.collection.return_value.document.return_value.get.return_value = doc + db.collection.return_value.where.return_value.stream.return_value = [doc] + db.collection.return_value.stream.return_value = [doc] + return db + + +def test_get_team_omits_private_fields_but_keeps_mentor_data(monkeypatch): + team = _full_team() + monkeypatch.setattr(ts, "get_db", lambda: _db_with_team(team)) + monkeypatch.setattr(ts, "doc_to_json", lambda docid, doc: dict(team)) + monkeypatch.setattr(ts, "_enrich_team_users", lambda data, db: data) + + payload = ts.get_team("team-1")["team"] + + for key in PRIVATE: + assert key not in payload + assert payload["mentor_ratings"] and payload["mentor_notes"] + + +def test_get_teams_list_and_batch_omit_private_fields(monkeypatch): + team = _full_team() + monkeypatch.setattr(ts, "get_db", lambda: _db_with_team(team)) + monkeypatch.setattr(ts, "doc_to_json", lambda docid, doc: dict(team)) + + single = ts.get_teams_list("team-1") + [listed] = ts.get_teams_list()["teams"] + [batched] = ts.get_teams_batch({"team_ids": ["team-1"]}) + + for payload in (single, listed, batched): + for key in PRIVATE: + assert key not in payload + assert payload["mentor_ratings"] + + +def test_get_team_admin_returns_full_doc(monkeypatch): + team = _full_team() + monkeypatch.setattr(ts, "get_db", lambda: _db_with_team(team)) + monkeypatch.setattr(ts, "doc_to_json", lambda docid, doc: dict(team)) + monkeypatch.setattr(ts, "_enrich_team_users", lambda data, db: data) + + assert ts.get_team_admin("team-1")["admin_notes"] == "flaky team" + + +def test_single_hackathon_event_teams_omit_private_fields(monkeypatch): + hs.get_single_hackathon_event.cache_clear() + team_ref = types.SimpleNamespace(id="team-1") + monkeypatch.setattr( + hs, + "get_hackathon_by_event_id", + lambda event_id: {"id": "doc-1", "event_id": "event-1", "nonprofits": [], "teams": [team_ref]}, + ) + monkeypatch.setattr(hs, "doc_to_json", lambda doc=None, docid=None: _full_team(docid)) + monkeypatch.setattr(hs, "_enrich_teams_users_batch", lambda teams, db: teams) + monkeypatch.setattr(hs, "_get_db", lambda: MagicMock()) + + [team] = hs.get_single_hackathon_event("event-1")["teams"] + + for key in PRIVATE: + assert key not in team + assert team["mentor_ratings"] + hs.get_single_hackathon_event.cache_clear() + + +# ---- route level ----------------------------------------------------------- + +VIEWS_MODULE = "api.teams.teams_views" +FAKE_USER = types.SimpleNamespace(user_id="propel-uuid", email="u@example.com") + + +def _passthrough(*_args, **_kwargs): + def decorator(fn): + @functools.wraps(fn) + def wrapper(*args, **kwargs): + g.propelauth_current_user = FAKE_USER + return fn(*args, **kwargs) + + return wrapper + + return decorator + + +def _rejecting_require_user(fn): + @functools.wraps(fn) + def wrapper(*args, **kwargs): + if not flask.request.headers.get("Authorization"): + return {"error": "unauthorized"}, 401 + g.propelauth_current_user = FAKE_USER + return fn(*args, **kwargs) + + return wrapper + + +def _full_list_payload(): + team = _full_team() + team.pop("users") + team["team_members"] = [{ + "id": "user-doc-1", + "user_id": "oauth2|slack|T1-U1", + "name": "Ada", + "nickname": "ada", + "profile_image": "https://img/ada.png", + "email_address": "ada@example.com", + "phone_number": "555", + }] + return {"teams": [team]} + + +@pytest.fixture +def client(monkeypatch): + stub = types.ModuleType("common.auth") + stub.auth = types.SimpleNamespace( + require_org_member_with_permission=_passthrough, + require_user=_rejecting_require_user, + optional_user=_passthrough(), + ) + stub.auth_user = LocalProxy(lambda: g.get("propelauth_current_user")) + monkeypatch.setitem(sys.modules, "common.auth", stub) + sys.modules.pop(VIEWS_MODULE, None) + views = importlib.import_module(VIEWS_MODULE) + monkeypatch.setattr(views, "get_teams_by_hackathon_id", lambda hackathon_id: _full_list_payload()) + monkeypatch.setattr("services.hackathon_planning_service.is_admin", lambda user: False) + + app = Flask(__name__) + app.register_blueprint(views.bp) + yield app.test_client() + sys.modules.pop(VIEWS_MODULE, None) + + +AUTH = {"Authorization": "Bearer x"} + + +def test_non_admin_team_list_is_public_view_with_slim_members(client): + response = client.get("/api/team/hack-1", headers=AUTH) + assert response.status_code == 200 + [team] = response.get_json()["teams"] + for key in PRIVATE: + assert key not in team + assert team["mentor_ratings"] + [member] = team["team_members"] + assert set(member) == {"id", "user_id", "name", "nickname", "profile_image"} + + +def test_admin_team_list_keeps_full_payload(client, monkeypatch): + monkeypatch.setattr("services.hackathon_planning_service.is_admin", lambda user: True) + [team] = client.get("/api/team/hack-1", headers=AUTH).get_json()["teams"] + assert team["admin_notes"] == "flaky team" + assert team["team_members"][0]["email_address"] == "ada@example.com" + + +def test_admin_team_detail_route_returns_full_doc(client, monkeypatch): + monkeypatch.setattr("services.teams_service.get_team_admin", lambda team_id: _full_team(team_id)) + response = client.get("/api/team/admin/team-1", headers=AUTH) + assert response.status_code == 200 + assert response.get_json()["team"]["admin_notes"] == "flaky team" + + +def test_admin_team_detail_route_404s_missing_team(client, monkeypatch): + monkeypatch.setattr("services.teams_service.get_team_admin", lambda team_id: None) + response = client.get("/api/team/admin/nope", headers=AUTH) + assert response.status_code == 404 + assert response.get_json() == {"error": "not_found"} + + +def test_admin_team_detail_route_requires_login(client): + assert client.get("/api/team/admin/team-1").status_code == 401 + + +def test_my_teams_omit_private_fields(monkeypatch): + import api.teams.teams_service as api_ts + + user_ref = types.SimpleNamespace(id="user-doc-1") + team_doc = MagicMock() + team_doc.exists = True + team_doc.id = "team-1" + team_doc.to_dict.return_value = {**_full_team(), "users": [user_ref]} + user_doc = MagicMock() + user_doc.exists = True + user_doc.id = "user-doc-1" + user_doc.to_dict.return_value = {"user_id": "oauth2|slack|T1-U1"} + db = MagicMock() + db.get_all.side_effect = [[team_doc], [user_doc]] + + monkeypatch.setattr(api_ts, "get_propel_user_details_by_id", lambda pid: (None, "oauth2|slack|T1-U1", None, None, "Ada", None)) + monkeypatch.setattr(api_ts, "get_hackathon_by_event_id", lambda eid: {"teams": [object()]}) + monkeypatch.setattr(api_ts, "get_db", lambda: db) + + [team] = api_ts.get_my_teams_by_event_id("propel-uuid", "event-1")["teams"] + for key in PRIVATE: + assert key not in team + assert team["mentor_ratings"] diff --git a/services/hackathons_service.py b/services/hackathons_service.py index 6c09ba8..9e63e57 100644 --- a/services/hackathons_service.py +++ b/services/hackathons_service.py @@ -612,6 +612,11 @@ def get_single_hackathon_event(hackathon_id): # per-team routes instead. for t in result["teams"]: t.pop("project_story", None) + # Staff-only team internals (admin_notes, nonprofit_rankings, ...) + # never ship on this unauthenticated payload. Imported here: + # services.teams_service imports this module lazily too. + from services.teams_service import public_team_view + result["teams"] = [public_team_view(t) for t in result["teams"]] else: result["teams"] = [] diff --git a/services/teams_service.py b/services/teams_service.py index cdf648c..1fe209a 100644 --- a/services/teams_service.py +++ b/services/teams_service.py @@ -31,6 +31,19 @@ def _clear_cache(): clear_cache() +# Staff-only internals on a team doc. Every public / member-facing team getter +# goes through public_team_view; admins read the full doc via get_team_admin +# (GET /api/team/admin/). mentor_* fields are public BY DESIGN. +PUBLIC_TEAM_STRIPPED_FIELDS = ("admin_notes", "nonprofit_rankings", "comments", "communication_history") + + +def public_team_view(team): + """Shallow copy of `team` without PUBLIC_TEAM_STRIPPED_FIELDS. None-safe.""" + if not isinstance(team, dict): + return team + return {k: v for k, v in team.items() if k not in PUBLIC_TEAM_STRIPPED_FIELDS} + + @limits(calls=2000, period=THIRTY_SECONDS) def get_teams_list(id=None): logger.debug(f"Teams List Start team_id={id}") @@ -41,9 +54,9 @@ def get_teams_list(id=None): if doc is None: return {} else: - logger.info(f"Teams List team_id={id} | End (with result):{doc_to_json(docid=doc.id, doc=doc)}") + result = public_team_view(doc_to_json(docid=doc.id, doc=doc)) logger.debug(f"Teams List team_id={id} | End") - return doc_to_json(docid=doc.id, doc=doc) + return result else: logger.debug("Teams List | Start") docs = db.collection('teams').stream() @@ -53,9 +66,9 @@ def get_teams_list(id=None): else: results = [] for doc in docs: - results.append(doc_to_json(docid=doc.id, doc=doc)) + results.append(public_team_view(doc_to_json(docid=doc.id, doc=doc))) - logger.debug(f"Found {len(results)} results {results}") + logger.debug(f"Found {len(results)} results") return { "teams": results } @@ -145,7 +158,7 @@ def get_team(id): team_data = _enrich_team_users(team_data, db) logger.info(f"Successfully retrieved team with id={id}") return { - "team" : team_data + "team" : public_team_view(team_data) } except Exception as e: @@ -156,6 +169,20 @@ def get_team(id): logger.debug(f"get_team operation completed for id={id}") +def get_team_admin(id): + """Full team doc (incl. PUBLIC_TEAM_STRIPPED_FIELDS) for admin views, or None.""" + if not id: + return None + db = get_db() + doc = db.collection('teams').document(id).get() + if not doc.exists: + return None + team_data = doc_to_json(docid=doc.id, doc=doc) + if team_data is None: + return None + return _enrich_team_users(team_data, db) + + def get_teams_by_event_id(event_id): """Get teams for a specific hackathon event (for admin judging assignment)""" logger.debug(f"Getting teams for event_id={event_id}") @@ -204,7 +231,7 @@ def get_teams_batch(json): results = [] for doc in docs: team_data = doc_to_json(docid=doc.id, doc=doc) - results.append(team_data) + results.append(public_team_view(team_data)) logger.debug(f"get_teams_batch end (with {len(results)} results)") return results From beee98f525f26c65032b95b3930c8dd049f9014b Mon Sep 17 00:00:00 2001 From: Greg V <6913307+gregv@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:21:05 +0200 Subject: [PATCH 5/8] security: hacker directory allowlist + require_user; deposit webhook never marks an underpaid session paid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /api/hacker/applications/ was optional_user with a 4-key denylist, leaking phone, deposit bookkeeping, sent_emails and free-text answers for every applicant. It is require_user and projected to HACKER_DIRECTORY_FIELDS — every key findteam.js reads, plus teamCode and isSelected (peer votes). _handle_checkout_session_completed compares amount_total with the event's default_amount_cents BEFORE the already-paid shortcut; short -> deposit_status 'underpaid' + Slack audit, never 'paid'; missing config / lookup error fails open. Best-effort — PaymentIntent verification on submit is the follow-up. Tests: test_hacker_applications_projection.py, test_deposit_webhook_amount.py. Co-Authored-By: Claude Fable 5.1 --- .../tests/test_deposit_webhook_amount.py | 89 +++++++++++++ .../test_hacker_applications_projection.py | 118 ++++++++++++++++++ api/volunteers/volunteers_views.py | 2 +- services/volunteers_service.py | 85 ++++++++++++- 4 files changed, 291 insertions(+), 3 deletions(-) create mode 100644 api/volunteers/tests/test_deposit_webhook_amount.py create mode 100644 api/volunteers/tests/test_hacker_applications_projection.py diff --git a/api/volunteers/tests/test_deposit_webhook_amount.py b/api/volunteers/tests/test_deposit_webhook_amount.py new file mode 100644 index 0000000..40bf3b4 --- /dev/null +++ b/api/volunteers/tests/test_deposit_webhook_amount.py @@ -0,0 +1,89 @@ +""" +checkout.session.completed must never mark a hacker deposit `paid` when the +Stripe amount is below the event's constraints.hacker_deposit.default_amount_cents. +Missing/unreadable config fails open (today's behaviour). +""" +import os +from unittest.mock import MagicMock + +os.environ.setdefault("ENVIRONMENT", "test") + +import pytest + +import services.volunteers_service as vs + + +def _session(amount_total, pi="pi_123"): + return { + "metadata": {"kind": "hacker_deposit", "event_id": "event-1", "hacker_email": "h@example.com"}, + "payment_intent": pi, + "amount_total": amount_total, + } + + +@pytest.fixture +def harness(monkeypatch): + db = MagicMock() + doc = {"user_id": "u1", "deposit_status": None} + monkeypatch.setattr(vs, "get_db", lambda: db) + monkeypatch.setattr(vs, "_find_hacker_by_email_and_event", lambda email, event_id: ("vol-1", dict(doc))) + monkeypatch.setattr(vs, "_clear_volunteer_caches", lambda *a, **k: None) + audits = [] + monkeypatch.setattr("common.utils.slack.send_slack_audit", lambda **kw: audits.append(kw)) + + def set_default(cents): + event = {} if cents is None else {"constraints": {"hacker_deposit": {"enabled": True, "default_amount_cents": cents}}} + monkeypatch.setattr("services.hackathons_service.get_single_hackathon_event", lambda event_id: event) + + def written(): + return db.collection.return_value.document.return_value.update.call_args[0][0] + + return set_default, written, audits, doc + + +def test_underpaid_session_is_not_marked_paid(harness): + set_default, written, audits, _ = harness + set_default(2500) + ok, _msg = vs._handle_checkout_session_completed(_session(100)) + assert ok + update = written() + assert update["deposit_status"] == "underpaid" + assert update["deposit_amount_cents"] == 100 + assert update["stripe_payment_intent_id"] == "pi_123" + assert audits + + +def test_underpaid_check_runs_before_idempotent_paid_shortcut(harness, monkeypatch): + set_default, written, _audits, _ = harness + set_default(2500) + monkeypatch.setattr( + vs, "_find_hacker_by_email_and_event", + lambda email, event_id: ("vol-1", {"deposit_status": "paid", "stripe_payment_intent_id": "pi_123"}), + ) + vs._handle_checkout_session_completed(_session(100)) + assert written()["deposit_status"] == "underpaid" + + +def test_missing_constraint_fails_open_to_paid(harness): + set_default, written, _audits, _ = harness + set_default(None) + vs._handle_checkout_session_completed(_session(100)) + assert written()["deposit_status"] == "paid" + + +def test_event_lookup_error_fails_open_to_paid(harness, monkeypatch): + _set_default, written, _audits, _ = harness + + def boom(event_id): + raise RuntimeError("firestore down") + + monkeypatch.setattr("services.hackathons_service.get_single_hackathon_event", boom) + vs._handle_checkout_session_completed(_session(100)) + assert written()["deposit_status"] == "paid" + + +def test_exact_amount_is_paid(harness): + set_default, written, _audits, _ = harness + set_default(2500) + vs._handle_checkout_session_completed(_session(2500)) + assert written()["deposit_status"] == "paid" diff --git a/api/volunteers/tests/test_hacker_applications_projection.py b/api/volunteers/tests/test_hacker_applications_projection.py new file mode 100644 index 0000000..8f4e8b8 --- /dev/null +++ b/api/volunteers/tests/test_hacker_applications_projection.py @@ -0,0 +1,118 @@ +""" +GET /api/hacker/applications/ backs findteam.js matchmaking. It used +to strip only email/ageRange/shirtSize/dietaryRestrictions and was reachable +anonymously, so phone numbers, deposit bookkeeping, sent-email logs, etc. went +out to anyone. The service now projects to HACKER_DIRECTORY_FIELDS and the +route requires a logged-in user. +""" +import functools +import importlib +import os +import sys +import types +from unittest.mock import MagicMock, patch + +os.environ.setdefault("ENVIRONMENT", "test") + +import flask +import pytest +from flask import Flask, g +from werkzeug.local import LocalProxy + +import services.volunteers_service as vs + +VIEWS_MODULE = "api.volunteers.volunteers_views" +FAKE_USER = types.SimpleNamespace(user_id="hacker-propel-uuid", email="hacker@example.com") + +HACKER_DOC = { + "user_id": "propel-1", + "name": "Ada", + "github": "ada", + "teamStatus": "I'd like to be matched with a team", + "teamCode": "ROCKET", + "isSelected": True, + "skills": "python", + "email": "ada@example.com", + "phone": "+1 555 0100", + "additionalInfo": "private note", + "deposit_amount_cents": 2500, + "sent_emails": [{"subject": "hi"}], + "ageRange": "18-24", +} + + +def _mock_db_returning(docs): + snaps = [] + for d in docs: + s = MagicMock() + s.to_dict.return_value = dict(d) + s.id = "vol-1" + snaps.append(s) + db = MagicMock() + db.collection.return_value.where.return_value.where.return_value.stream.return_value = snaps + return db + + +def test_hackers_are_projected_to_directory_allowlist(): + with patch.object(vs, "get_db", return_value=_mock_db_returning([HACKER_DOC])): + [hacker] = vs.get_all_hackers_by_event_id("event-1") + + assert set(hacker) <= vs.HACKER_DIRECTORY_FIELDS + for key in ("user_id", "teamStatus", "isSelected", "teamCode"): + assert key in hacker + for key in ("phone", "additionalInfo", "deposit_amount_cents", "sent_emails", "email", "ageRange"): + assert key not in hacker + + +def _passthrough(*_args, **_kwargs): + def decorator(fn): + @functools.wraps(fn) + def wrapper(*args, **kwargs): + g.propelauth_current_user = FAKE_USER + return fn(*args, **kwargs) + + return wrapper + + return decorator + + +def _rejecting_require_user(fn): + @functools.wraps(fn) + def wrapper(*args, **kwargs): + if not flask.request.headers.get("Authorization"): + return {"error": "unauthorized"}, 401 + g.propelauth_current_user = FAKE_USER + return fn(*args, **kwargs) + + return wrapper + + +@pytest.fixture +def client(monkeypatch): + stub = types.ModuleType("common.auth") + stub.auth = types.SimpleNamespace( + require_org_member_with_permission=_passthrough, + require_user=_rejecting_require_user, + optional_user=_passthrough(), + ) + stub.auth_user = LocalProxy(lambda: g.get("propelauth_current_user")) + stub.getOrgId = lambda req: req.headers.get("X-Org-Id") + monkeypatch.setitem(sys.modules, "common.auth", stub) + sys.modules.pop(VIEWS_MODULE, None) + views = importlib.import_module(VIEWS_MODULE) + monkeypatch.setattr(views, "get_all_hackers_by_event_id", lambda event_id: [{"name": "Ada"}]) + + app = Flask(__name__) + app.register_blueprint(views.bp) + yield app.test_client() + sys.modules.pop(VIEWS_MODULE, None) + + +def test_anonymous_request_is_rejected(client): + response = client.get("/api/hacker/applications/event-1") + assert response.status_code == 401 + + +def test_logged_in_request_gets_directory(client): + response = client.get("/api/hacker/applications/event-1", headers={"Authorization": "Bearer x"}) + assert response.status_code == 200 diff --git a/api/volunteers/volunteers_views.py b/api/volunteers/volunteers_views.py index 1411e7c..64bfbf4 100644 --- a/api/volunteers/volunteers_views.py +++ b/api/volunteers/volunteers_views.py @@ -574,7 +574,7 @@ def get_hacker_application(event_id): return _error_response(f"Failed to retrieve application: {str(e)}") @bp.route('/hacker/applications/', methods=['GET']) -@auth.optional_user +@auth.require_user def get_hacker_applications(event_id): """Get all hacker applications for a specific event. Only if teamStatus is 'I'd like to be matched with a team'. using get_all_hackers_by_event_id""" user = auth_user diff --git a/services/volunteers_service.py b/services/volunteers_service.py index fa9c8ad..ed8f067 100644 --- a/services/volunteers_service.py +++ b/services/volunteers_service.py @@ -1517,6 +1517,21 @@ def handle_stripe_hacker_deposit_event( return True, f"Ignored event type {event_type}" +def _expected_hacker_deposit_cents(event_id: str) -> Optional[int]: + """The event's constraints.hacker_deposit.default_amount_cents, or None + (missing config / lookup error — callers fail open).""" + try: + from services.hackathons_service import get_single_hackathon_event + event = get_single_hackathon_event(event_id) or {} + cents = ((event.get('constraints') or {}).get('hacker_deposit') or {}).get('default_amount_cents') + if isinstance(cents, bool) or not isinstance(cents, int): + return None + return cents + except Exception as e: + warning(logger, "Could not resolve hacker deposit amount", event_id=event_id, error=str(e)) + return None + + def _handle_checkout_session_completed(session: Dict[str, Any]) -> Tuple[bool, str]: """Self-heal the volunteer doc when Stripe confirms a checkout payment. @@ -1565,6 +1580,51 @@ def _handle_checkout_session_completed(session: Dict[str, Any]) -> Tuple[bool, s vol_id, doc = match if doc.get('deposit_status') == 'refunded': return True, "Already refunded — not regressing" + + # Best-effort amount check: never mark a session that paid less than the + # event's configured deposit as 'paid'. Runs BEFORE the idempotent + # already-paid shortcut so an underpaid session can't ride on a doc the + # client-side submit already stamped 'paid'. Unknown config fails open. + expected_cents = _expected_hacker_deposit_cents(event_id) + if ( + isinstance(expected_cents, int) + and isinstance(amount_total, int) + and amount_total < expected_cents + ): + db = get_db() + db.collection('volunteers').document(vol_id).update({ + 'deposit_status': 'underpaid', + 'deposit_amount_cents': amount_total, + 'stripe_payment_intent_id': payment_intent_id, + 'updated_timestamp': _get_current_timestamp(), + }) + _clear_volunteer_caches(doc.get('user_id'), email, event_id, 'hacker') + try: + from common.utils.slack import send_slack_audit + send_slack_audit( + action="hacker_deposit_underpaid", + message=( + f"Hacker deposit underpaid for volunteer {vol_id}: " + f"{amount_total} < {expected_cents} cents" + ), + payload={ + 'volunteer_id': vol_id, + 'event_id': event_id, + 'payment_intent_id': payment_intent_id, + 'amount_total': amount_total, + 'expected_cents': expected_cents, + }, + ) + except Exception as e: # audit is best-effort + warning(logger, "Underpaid deposit audit failed", error=str(e)) + warning( + logger, + "Webhook: underpaid hacker deposit — not marking paid", + volunteer_id=vol_id, + payment_intent_id=payment_intent_id, + ) + return True, f"Underpaid deposit recorded for {vol_id}" + if ( doc.get('deposit_status') == 'paid' and doc.get('stripe_payment_intent_id') == payment_intent_id @@ -1648,6 +1708,23 @@ def _handle_charge_refunded(charge: Dict[str, Any]) -> Tuple[bool, str]: return True, f"Recorded refund for {volunteer_id}" +# Allowlist for the hacker directory (GET /api/hacker/applications/). +# Every key frontend findteam.js reads, plus teamCode (hacker form "Find your +# team" picker) and isSelected (api/peer_votes/peer_votes_service.py reads it). +# Anything else on a hacker application (phone, deposit bookkeeping, sent-email +# logs, free-text notes, ...) must never ship through this route. +HACKER_DIRECTORY_FIELDS = frozenset({ + "id", "user_id", "name", "github", "slack_user_id", "pronouns", + "linkedin", "linkedinProfile", "photoUrl", "portfolio", + "experienceLevel", "participationCount", "participantType", + "schoolOrganization", "willContinue", "inPerson", "isInPerson", + "shortBio", "bio", "socialCauses", "primaryRoles", "skills", + "teamNeededSkills", "teamMatchingPreferences", + "teamMatchingPreferredCauses", "teamMatchingPreferredSkills", + "teamMatchingPreferredSize", "teamStatus", "teamCode", "isSelected", +}) + + def get_all_hackers_by_event_id(event_id: str) -> List[Dict[str, Any]]: """ Get all hackers for a specific event ID. @@ -1662,8 +1739,12 @@ def get_all_hackers_by_event_id(event_id: str) -> List[Dict[str, Any]]: hackers = db.collection('volunteers').where('event_id', '==', event_id).where('volunteer_type', '==', 'hacker').stream() hacker_dict = [hacker.to_dict() for hacker in hackers] if hackers else [] - # Remove sensitive info from the fields like: volunteers = [{k: v for k, v in volunteer.items() if k != "email" and k != "ageRange" and k != "shirtSize" and k != "dietaryRestrictions"} for volunteer in volunteers] - hacker_dict = [{k: v for k, v in hacker.items() if k != "email" and k != "ageRange" and k != "shirtSize" and k != "dietaryRestrictions"} for hacker in hacker_dict] + # Project to the directory allowlist (was a 4-key denylist that leaked + # phone, deposit fields, sent_emails, ...). + hacker_dict = [ + {k: v for k, v in hacker.items() if k in HACKER_DIRECTORY_FIELDS} + for hacker in hacker_dict + ] return hacker_dict From 76e886e00ec1256b0f95a6f28f0bda613619bd70 Mon Sep 17 00:00:00 2001 From: Greg V <6913307+gregv@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:21:05 +0200 Subject: [PATCH 6/8] docs: CLAUDE.md invariants for the PR-A security changes Co-Authored-By: Claude Fable 5.1 --- CLAUDE.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 013b934..36f4eab 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -286,3 +286,17 @@ Gunicorn runs `--workers 2`, and every `TTLCache` in this codebase is per-proces ## CDN URL prefix contract (Sep 2026) `common/utils/cdn.py::cdn_server()` is the ONE way to get the public CDN origin: read at call time (not import), `rstrip("/")`, default `https://cdn.ohack.dev`. `upload_to_cdn` / `generate_signed_upload_url` build URLs with it and `api/submissions/submissions_service.py::_cdn_server` delegates to it, so a producer and a validator can never disagree on the prefix (the "Thumbnail: must be an ohack CDN URL under teams//" bug — a trailing slash or unset `CDN_SERVER` on one side). `_validate_own_cdn_image` logs the rejected URL + expected prefix at WARNING so the next mismatch is diagnosable from Render logs. `api/jobs` and `users_service` still carry their own `os.getenv("CDN_SERVER", …)` copies — migrate them to `cdn_server()` when touched. + +## Hardening stack (Sep 2026) — PR-A security + +Plan + evidence live in the frontend repo: `frontend-ohack.dev/docs/plans/hardening-security-seo-reliability-2026-09.md`. Every change below landed test-first; the tests are the contract. + +- **`@bp.route` must be the OUTERMOST decorator.** Two routes (`GET /hackathon///checkins`, `PATCH /api/problem-statements/events`) had `@auth.*` above `@bp.route`, so Flask registered the undecorated function and served volunteer PII (email, phone) to anyone. `test/common/test_view_decorator_order.py` AST-scans every `api/**/*_views.py` and fails the build on a repeat. Route-level proof uses the **rejecting** auth stub in `test/common/auth_stubs.py` (`rejecting_auth_module`: no `Authorization` → 401, no `X-Org-Id` → 403) — the pass-through stub can't detect a dropped decorator. +- **Newsletter routes** are `volunteer.admin`-gated (they were an open Gmail relay). `POST //` stays `require_user` on purpose (per-user self-service). The module imports `common.auth` like every other view (its own `init_auth` made it un-importable under test). +- **Hackathon requests** (public capability-link flow, `/hack/request/`): `update_hackathon_request` filters the body to `HACKATHON_REQUEST_EDITABLE_FIELDS` (= the frontend `HackathonRequestForm` `formData` keys; lockstep test in `api/messages/tests/test_hackathon_requests.py::FRONTEND_FORM_KEYS`), 404s before any email when the doc is missing, and confirms to the STORED `contactEmail` (never the body's). The public GET strips `adminNotes`. New ids are `uuid4`. Admin edits use the separate `admin_update_hackathon_request`. +- **Uploads (`POST /api/messages/upload-image`)**: one gate, `api/submissions/submissions_service.py::authorize_upload_directory` — `teams//…` delegates to the #289 membership gate; non-admins may otherwise only write single-segment `USER_UPLOAD_DIRECTORIES` (`images hackers volunteers mentors judges sponsors uploads`) or `hackathons//planning/…` when `hackathon_planning_service.can_write_plan_for_event` says they edit that plan; everything else (site assets, event galleries, nonprofit logos, blog media) is admin-only (403 `directory_not_allowed`); `..`/odd characters → 400 `invalid_directory`. Non-admins can't overwrite: `upload_image_to_cdn(request, allow_overwrite=False)` answers 409 `file_exists` (multipart path; `cdn.blob_exists` — `upload_to_cdn` itself still overwrites because certificates/hearts/openai rely on it). `MAX_CONTENT_LENGTH` = 32 MiB with a JSON 413 (`api/exception_views.py`) because the app forces JSON content-type and frontend callers do `res.json()`. +- **Hacker directory (`GET /api/hacker/applications/`)** is `require_user` and projected to `HACKER_DIRECTORY_FIELDS` (`services/volunteers_service.py`) — every key `findteam.js` reads + `teamCode` + `isSelected` (peer votes). Add to the allowlist if the finder needs a new field; phone/deposit/sent_emails must never ship. +- **Team payloads:** `services/teams_service.py::public_team_view` strips `PUBLIC_TEAM_STRIPPED_FIELDS` (`admin_notes nonprofit_rankings comments communication_history`) in `get_team`, `get_teams_list`, `get_teams_batch`, `get_single_hackathon_event`, `/me`, and (non-admins only) `GET /api/team/` whose member docs are also trimmed to `{id,user_id,name,nickname,profile_image}` (full user docs incl. email were shipping to any logged-in user). Admins keep full payloads there; the full single doc is `GET /api/team/admin/` (`get_team_admin`), which the frontend's `adminTeamApi` prefers (404 → public fallback). `mentor_*` fields are public BY DESIGN — don't strip them. +- **Deposit webhook**: `_handle_checkout_session_completed` compares `amount_total` with the event's `default_amount_cents` BEFORE the already-paid shortcut; short → `deposit_status="underpaid"` + Slack audit, never `paid`; missing config/lookup error fails open. Best-effort only (the submit path still accepts deposit fields by design); PaymentIntent verification is the follow-up. +- Shared-secret headers (`BACKEND_NEWS_TOKEN`, `BACKEND_PRAISE_TOKEN`, store `X-Webhook-Secret`) compare with `hmac.compare_digest` and never match when the env var is unset. `GET /news?limit=` → 400 on non-int, clamped to 1..200. +- **Running tests locally:** `ENVIRONMENT=test SLACK_WEBHOOK= SLACK_BOT_TOKEN= RESEND_API_KEY= FIREBASE_CERT_CONFIG='' python -m pytest ` — Slack vars EMPTY (dummies make `send_slack_audit` raise on `requests.post`), and `common/utils/firebase.py` builds `credentials.Certificate` at import, so `.env.example`'s placeholder cert blocks collection of almost every suite. `api/messages/tests` files run individually. `api/certificates/tests` hangs on network — exclude it from gated runs. From d3304bed01b596944d4f63ee37f05257469a7e0c Mon Sep 17 00:00:00 2001 From: Greg V <6913307+gregv@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:48:57 +0200 Subject: [PATCH 7/8] security: explicit jsonify on the upload gate's error response; 413 test route no longer echoes form keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both were CodeQL 'reflected XSS' findings on the PR. Flask already serialises returned dicts as JSON and the payloads are constant strings, so neither was exploitable — making the JSON explicit (and not echoing request data in a test-only route) removes the taint path the analyser follows. Co-Authored-By: Claude Fable 5.1 --- api/messages/messages_views.py | 9 +++++++-- test/common/test_payload_too_large.py | 6 +++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/api/messages/messages_views.py b/api/messages/messages_views.py index 16198b8..ca33605 100644 --- a/api/messages/messages_views.py +++ b/api/messages/messages_views.py @@ -7,7 +7,8 @@ from flask import ( Blueprint, request, - g + g, + jsonify ) from api.messages.messages_service import ( @@ -849,7 +850,11 @@ def upload_image(): plan_editor_check=lambda event_id: planning.can_write_plan_for_event(auth_user, event_id), ) if blocked: - return blocked + # Explicit JSON so the (constant) error payload is never mistaken + # for reflected request input (CodeQL) — Flask would jsonify the + # dict anyway. + payload, status = blocked + return jsonify(payload), status return upload_image_to_cdn(request, allow_overwrite=admin) else: error(logger, "Could not obtain user details for POST /upload-image") diff --git a/test/common/test_payload_too_large.py b/test/common/test_payload_too_large.py index 449f8b6..ee5e4c1 100644 --- a/test/common/test_payload_too_large.py +++ b/test/common/test_payload_too_large.py @@ -16,7 +16,11 @@ def test_413_is_json(): @app.route("/api/x", methods=["POST"]) def _x(): - return {"keys": list(request.form)} + # Werkzeug enforces MAX_CONTENT_LENGTH when the body is READ, so the + # route must touch request.form; the response stays constant (echoing + # the form keys here tripped CodeQL's reflected-XSS check). + request.form # noqa: B018 — forces body parsing -> 413 + return {"ok": True} resp = app.test_client().post("/api/x", data={"f": "x" * 100}) assert resp.status_code == 413 From 093890bb7550e7ec007ef491ae2992af258f49a6 Mon Sep 17 00:00:00 2001 From: Greg V <6913307+gregv@users.noreply.github.com> Date: Wed, 30 Sep 2026 15:18:01 -0400 Subject: [PATCH 8/8] security: upload denial responses come from a module constant table CodeQL kept following request.form['directory'] through the gate's return value into the response even after jsonify(). The gate still returns (payload, status) for its own tests; the view now maps the error code to a constant response, so no request-derived object is ever returned. Co-Authored-By: Claude Fable 5.1 --- api/messages/messages_views.py | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/api/messages/messages_views.py b/api/messages/messages_views.py index ca33605..dde43a0 100644 --- a/api/messages/messages_views.py +++ b/api/messages/messages_views.py @@ -95,6 +95,15 @@ def getOrgId(req): NEWS_LIMIT_MAX = 200 +# Constant responses for POST /upload-image directory denials (see +# api/submissions/submissions_service.py::authorize_upload_directory). +UPLOAD_DENIAL_RESPONSES = { + "invalid_directory": ({"error": "invalid_directory"}, 400), + "not_team_member": ({"error": "not_team_member"}, 403), + "directory_not_allowed": ({"error": "directory_not_allowed"}, 403), + "forbidden": ({"error": "forbidden"}, 403), +} + def _api_key_matches(provided, expected): """Constant-time X-Api-Key check; an unset env secret never matches.""" @@ -850,10 +859,12 @@ def upload_image(): plan_editor_check=lambda event_id: planning.can_write_plan_for_event(auth_user, event_id), ) if blocked: - # Explicit JSON so the (constant) error payload is never mistaken - # for reflected request input (CodeQL) — Flask would jsonify the - # dict anyway. - payload, status = blocked + # The gate's payloads are constant, but static analysis follows the + # `directory` argument into its return value — answer from a module + # constant keyed by the error code so no request data can reach the + # response. + code = (blocked[0] or {}).get("error") + payload, status = UPLOAD_DENIAL_RESPONSES.get(code, UPLOAD_DENIAL_RESPONSES["forbidden"]) return jsonify(payload), status return upload_image_to_cdn(request, allow_overwrite=admin) else: