From 5f59752e60e6570e36b67dd16ad585f68841f8a7 Mon Sep 17 00:00:00 2001 From: Jelterminator FennemAI-Neo Date: Sun, 25 Oct 2026 18:21:27 +0100 Subject: [PATCH 1/3] Only allow modifying patches through POST requests Changing the status of a patch, closing or moving it, becoming reviewer or committer, and (un)subscribing were all done through plain links, i.e. GET requests. Crawlers follow all of those links on every patch page, and since they're not logged in each of them resulted in a redirect to the login page, generating a lot of useless load. It also meant that anyone could trick a logged in user into modifying a patch by getting them to click a link, since GET requests aren't protected against CSRF. These buttons are now forms that POST, with a CSRF token. A GET request to one of these URLs now redirects to the patch page without doing anything, also when not logged in, so crawlers that still have the old URLs don't end up at the login page. The staff-only `importUser` AJAX call, which creates a user, now also uses POST. Fixes #116 --- media/commitfest/js/commitfest.js | 6 ++- pgcommitfest/commitfest/ajax.py | 4 +- pgcommitfest/commitfest/templates/patch.html | 11 ++++-- .../commitfest/templates/patch_commands.inc | 39 +++++++++++-------- .../commitfest/tests/test_post_only.py | 30 ++++++++++++++ .../commitfest/tests/test_remove_reviewer.py | 4 +- pgcommitfest/commitfest/views.py | 26 +++++++++++++ 7 files changed, 94 insertions(+), 26 deletions(-) create mode 100644 pgcommitfest/commitfest/tests/test_post_only.py diff --git a/media/commitfest/js/commitfest.js b/media/commitfest/js/commitfest.js index 24c7b66b..a353aef2 100644 --- a/media/commitfest/js/commitfest.js +++ b/media/commitfest/js/commitfest.js @@ -260,7 +260,9 @@ function flagCommitted(committer) { ); return; } - document.location.href = `close/committed/?c=${c}`; + const form = $("#commitForm"); + form.attr("action", `close/committed/?c=${encodeURIComponent(c)}`); + form.submit(); }); return false; } @@ -317,7 +319,7 @@ function search_and_store_user() { } /* Create this user locally */ - $.get("/ajax/importUser/", { + $.post("/ajax/importUser/", { u: $("#searchUserList").val(), }) .success((data) => { diff --git a/pgcommitfest/commitfest/ajax.py b/pgcommitfest/commitfest/ajax.py index 1592e800..9e0d92e4 100644 --- a/pgcommitfest/commitfest/ajax.py +++ b/pgcommitfest/commitfest/ajax.py @@ -306,8 +306,8 @@ def importUser(request): if not request.user.is_staff: raise Http404() - if request.GET.get("u", ""): - u = user_search(userid=request.GET["u"]) + if request.POST.get("u", ""): + u = user_search(userid=request.POST["u"]) if len(u) != 1: return "Internal error, duplicate user found" diff --git a/pgcommitfest/commitfest/templates/patch.html b/pgcommitfest/commitfest/templates/patch.html index 2de090b0..b49734d9 100644 --- a/pgcommitfest/commitfest/templates/patch.html +++ b/pgcommitfest/commitfest/templates/patch.html @@ -125,14 +125,16 @@ Reviewers {{patch.reviewers_string}} - {%if is_reviewer%}Remove from reviewers{%else%}Become reviewer{%endif%} - {%if patch.can_remove_all_reviewers and patch.reviewers.all%}Remove all reviewers{%endif%} +
{%csrf_token%} + {%if is_reviewer%}{%else%}{%endif%} + {%if patch.can_remove_all_reviewers and patch.reviewers.all%}{%endif%} +
Committer {%if patch.committer%}{{patch.committer.fullname}}{%endif%} - {%if is_committer%}{{is_this_committer|yesno:"Unclaim as committer,Claim as committer"}}{%endif%} + {%if is_committer%}
{%csrf_token%}
{%endif%} @@ -225,7 +227,7 @@

Annotations

{%if user.is_authenticated%} - {{is_subscribed|yesno:"Unsubscribe from patch update emails,Subscribe to patch update emails"}} +
{%csrf_token%}
{%endif%} @@ -260,6 +262,7 @@

Flag as committed

Close Flag as committed +
{%csrf_token%}
diff --git a/pgcommitfest/commitfest/templates/patch_commands.inc b/pgcommitfest/commitfest/templates/patch_commands.inc index 2811a20b..5b31ac62 100644 --- a/pgcommitfest/commitfest/templates/patch_commands.inc +++ b/pgcommitfest/commitfest/templates/patch_commands.inc @@ -1,39 +1,46 @@ -
+
{%csrf_token%} Edit - + +
+ + +
@@ -50,4 +57,4 @@
{%endif%} -
+ diff --git a/pgcommitfest/commitfest/tests/test_post_only.py b/pgcommitfest/commitfest/tests/test_post_only.py new file mode 100644 index 00000000..209c5832 --- /dev/null +++ b/pgcommitfest/commitfest/tests/test_post_only.py @@ -0,0 +1,30 @@ +from datetime import datetime + +import pytest + +from pgcommitfest.commitfest.models import Patch, PatchOnCommitFest + +pytestmark = pytest.mark.django_db + + +@pytest.mark.parametrize("logged_in", [False, True]) +def test_get_does_not_modify_patch(client, open_cf, alice, logged_in): + patch = Patch.objects.create(name="Test patch") + poc = PatchOnCommitFest.objects.create( + patch=patch, + commitfest=open_cf, + enterdate=datetime.now(), + status=PatchOnCommitFest.STATUS_REVIEW, + ) + if logged_in: + client.force_login(alice) + + response = client.get(f"/patch/{patch.id}/status/author/") + assert response.status_code == 302 + assert response["Location"] == f"/patch/{patch.id}/" + poc.refresh_from_db() + assert poc.status == PatchOnCommitFest.STATUS_REVIEW + + response = client.post(f"/patch/{patch.id}/status/author/") + poc.refresh_from_db() + assert (poc.status == PatchOnCommitFest.STATUS_AUTHOR) == logged_in diff --git a/pgcommitfest/commitfest/tests/test_remove_reviewer.py b/pgcommitfest/commitfest/tests/test_remove_reviewer.py index a4d34e89..8535cb05 100644 --- a/pgcommitfest/commitfest/tests/test_remove_reviewer.py +++ b/pgcommitfest/commitfest/tests/test_remove_reviewer.py @@ -32,7 +32,7 @@ def test_remove_all_reviewers(client, open_cf, alice, bob, charlie): ) client.force_login(alice) - response = client.get(f"/patch/{patch.id}/reviewer/remove_all/") + response = client.post(f"/patch/{patch.id}/reviewer/remove_all/") assert response.status_code == 302 assert list(patch.reviewers.all()) == [] assert ( @@ -54,6 +54,6 @@ def test_remove_all_reviewers_forbidden_without_tag(client, open_cf, alice, bob) ) client.force_login(alice) - response = client.get(f"/patch/{patch.id}/reviewer/remove_all/") + response = client.post(f"/patch/{patch.id}/reviewer/remove_all/") assert response.status_code == 403 assert bob in patch.reviewers.all() diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index ef0a3831..27787b4b 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -88,6 +88,25 @@ def wrapper(*args, **kwargs): return wrapper +def patch_modification(view_func): + """Only allow POST requests for views that modify a patch. + + Other requests are redirected to the patch page instead of being rejected, + so that someone logging in after clicking one of these buttons ends up + somewhere useful. This has to sit outside login_required, so that crawlers + that still have these URLs from when they were plain links don't cause a + login redirect. + """ + + @functools.wraps(view_func) + def wrapper(request, patchid, *args, **kwargs): + if request.method != "POST": + return HttpResponseRedirect(f"/patch/{patchid}/") + return view_func(request, patchid, *args, **kwargs) + + return wrapper + + def get_tags_data(): """Generate JSON data for enhanced selectize dropdown with tag colors and descriptions.""" return json.dumps( @@ -1106,6 +1125,7 @@ def newpatch(request, cfid): ) +@patch_modification @login_required @transaction.atomic def status(request, patchid, status): @@ -1134,6 +1154,7 @@ def status(request, patchid, status): return HttpResponseRedirect("/patch/%s/" % (poc.patch.id)) +@patch_modification @login_required @transaction.atomic def close(request, patchid, status): @@ -1228,6 +1249,7 @@ def int_param_or_none(request, param): return None +@patch_modification @login_required @transaction.atomic def move(request, patchid): @@ -1253,6 +1275,7 @@ def move(request, patchid): return HttpResponseRedirect(f"/patch/{patchid}/") +@patch_modification @login_required @transaction.atomic def reviewer(request, patchid, status): @@ -1279,6 +1302,7 @@ def reviewer(request, patchid, status): return HttpResponseRedirect("../../") +@patch_modification @login_required @transaction.atomic def remove_all_reviewers(request, patchid): @@ -1304,6 +1328,7 @@ def remove_all_reviewers(request, patchid): return HttpResponseRedirect(f"/patch/{patchid}/") +@patch_modification @login_required @transaction.atomic def committer(request, patchid, status): @@ -1337,6 +1362,7 @@ def committer(request, patchid, status): return HttpResponseRedirect("../../") +@patch_modification @login_required @transaction.atomic def subscribe(request, patchid, sub): From c8ce287b81578450277d76386dba9acae94a8c0b Mon Sep 17 00:00:00 2001 From: Jelterminator FennemAI-Neo Date: Sun, 25 Oct 2026 22:05:07 +0100 Subject: [PATCH 2/3] Use require_POST and send logged out users to login first The previous commit added a custom `patch_modification` decorator that redirected GET requests to the patch page, instead of using Django's `require_POST`. The only reason for that was a logged out user clicking one of the new POST buttons: `login_required` would then send them to the login page with the action URL as `next`, and after logging in the browser would do a GET to that URL. Instead, logged out users now don't get these POST buttons at all. The patch command bar shows a "Log in to edit" button (next to the Review button) and the reviewers row shows "Log in to become reviewer", both of which log in and return to the patch page. That allows using the standard `require_POST`, which returns a 405 for GET requests before the login check, so crawlers with the old URLs still don't end up at the login page. This also restores the Review button that opens the review popup, which was accidentally replaced by the old Comment/Review dropdown when merging main. --- pgcommitfest/commitfest/templates/patch.html | 12 +- .../commitfest/templates/patch_commands.inc | 104 +++++++++--------- .../commitfest/tests/test_post_only.py | 3 +- pgcommitfest/commitfest/views.py | 34 ++---- 4 files changed, 70 insertions(+), 83 deletions(-) diff --git a/pgcommitfest/commitfest/templates/patch.html b/pgcommitfest/commitfest/templates/patch.html index b49734d9..25e36c28 100644 --- a/pgcommitfest/commitfest/templates/patch.html +++ b/pgcommitfest/commitfest/templates/patch.html @@ -125,10 +125,14 @@ Reviewers {{patch.reviewers_string}} -
{%csrf_token%} - {%if is_reviewer%}{%else%}{%endif%} - {%if patch.can_remove_all_reviewers and patch.reviewers.all%}{%endif%} -
+ {%if not user.is_authenticated%} + Log in to become reviewer + {%else%} +
{%csrf_token%} + {%if is_reviewer%}{%else%}{%endif%} + {%if patch.can_remove_all_reviewers and patch.reviewers.all%}{%endif%} +
+ {%endif%} diff --git a/pgcommitfest/commitfest/templates/patch_commands.inc b/pgcommitfest/commitfest/templates/patch_commands.inc index 5b31ac62..a19bbf80 100644 --- a/pgcommitfest/commitfest/templates/patch_commands.inc +++ b/pgcommitfest/commitfest/templates/patch_commands.inc @@ -1,60 +1,62 @@
{%csrf_token%} - Edit + {%if not user.is_authenticated%} + {%comment%} + Most of these commands are POSTs, which can't be replayed after logging in, + so send anonymous users to log in first instead of letting them submit. + {%endcomment%} + Log in to edit + + {%else%} + Edit + -
- - -
- -
- -
    -
  • Open statuses
    • -
    • -
    • -
    • -
    • -
  • Closed statuses
    • -
    • -
    • -
    • -
    • -
    • -
  • Move to
    • - {%if not cf.is_open_regular %} -
  • - -
    • - {%endif%} - {%if not cf.is_open_draft %} -
  • - -
    • - {%endif%} -
-
- - {%if request.user.is_staff%}
- +
    -
  • Send mail to authors
    • -
  • Send mail to reviewers
    • -
  • Send mail to authors and reviewers
    • +
  • Open statuses
    • +
    • +
    • +
    • +
    • +
  • Closed statuses
    • +
    • +
    • +
    • +
    • +
    • +
  • Move to
    • + {%if not cf.is_open_regular %} +
  • + +
    • + {%endif%} + {%if not cf.is_open_draft %} +
  • + +
    • + {%endif%}
+ + {%if request.user.is_staff%} +
+ + +
+ {%endif%} {%endif%}
diff --git a/pgcommitfest/commitfest/tests/test_post_only.py b/pgcommitfest/commitfest/tests/test_post_only.py index 209c5832..1c26e6c0 100644 --- a/pgcommitfest/commitfest/tests/test_post_only.py +++ b/pgcommitfest/commitfest/tests/test_post_only.py @@ -20,8 +20,7 @@ def test_get_does_not_modify_patch(client, open_cf, alice, logged_in): client.force_login(alice) response = client.get(f"/patch/{patch.id}/status/author/") - assert response.status_code == 302 - assert response["Location"] == f"/patch/{patch.id}/" + assert response.status_code == 405 poc.refresh_from_db() assert poc.status == PatchOnCommitFest.STATUS_REVIEW diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index c31a0fe3..d36ac503 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -15,6 +15,7 @@ from django.shortcuts import get_object_or_404, render from django.utils import timezone from django.views.decorators.csrf import csrf_exempt +from django.views.decorators.http import require_POST import collections import functools @@ -90,25 +91,6 @@ def wrapper(*args, **kwargs): return wrapper -def patch_modification(view_func): - """Only allow POST requests for views that modify a patch. - - Other requests are redirected to the patch page instead of being rejected, - so that someone logging in after clicking one of these buttons ends up - somewhere useful. This has to sit outside login_required, so that crawlers - that still have these URLs from when they were plain links don't cause a - login redirect. - """ - - @functools.wraps(view_func) - def wrapper(request, patchid, *args, **kwargs): - if request.method != "POST": - return HttpResponseRedirect(f"/patch/{patchid}/") - return view_func(request, patchid, *args, **kwargs) - - return wrapper - - def get_tags_data(): """Generate JSON data for enhanced selectize dropdown with tag colors and descriptions.""" return json.dumps( @@ -1132,7 +1114,7 @@ def newpatch(request, cfid): ) -@patch_modification +@require_POST @login_required @transaction.atomic def status(request, patchid, status): @@ -1161,7 +1143,7 @@ def status(request, patchid, status): return HttpResponseRedirect("/patch/%s/" % (poc.patch.id)) -@patch_modification +@require_POST @login_required @transaction.atomic def close(request, patchid, status): @@ -1256,7 +1238,7 @@ def int_param_or_none(request, param): return None -@patch_modification +@require_POST @login_required @transaction.atomic def move(request, patchid): @@ -1282,7 +1264,7 @@ def move(request, patchid): return HttpResponseRedirect(f"/patch/{patchid}/") -@patch_modification +@require_POST @login_required @transaction.atomic def reviewer(request, patchid, status): @@ -1309,7 +1291,7 @@ def reviewer(request, patchid, status): return HttpResponseRedirect("../../") -@patch_modification +@require_POST @login_required @transaction.atomic def remove_all_reviewers(request, patchid): @@ -1335,7 +1317,7 @@ def remove_all_reviewers(request, patchid): return HttpResponseRedirect(f"/patch/{patchid}/") -@patch_modification +@require_POST @login_required @transaction.atomic def committer(request, patchid, status): @@ -1369,7 +1351,7 @@ def committer(request, patchid, status): return HttpResponseRedirect("../../") -@patch_modification +@require_POST @login_required @transaction.atomic def subscribe(request, patchid, sub): From 96c4a3a379fe4a27d31cd2262fac1b51a989877e Mon Sep 17 00:00:00 2001 From: Jelterminator FennemAI-Neo Date: Sun, 25 Oct 2026 22:09:30 +0100 Subject: [PATCH 3/3] Remove test_post_only.py Now that these views use Django's `require_POST` and `login_required` directly, this test was mostly testing Django's own behaviour. POST requests to the patch modification views are already exercised by `test_remove_reviewer.py`. --- .../commitfest/tests/test_post_only.py | 29 ------------------- 1 file changed, 29 deletions(-) delete mode 100644 pgcommitfest/commitfest/tests/test_post_only.py diff --git a/pgcommitfest/commitfest/tests/test_post_only.py b/pgcommitfest/commitfest/tests/test_post_only.py deleted file mode 100644 index 1c26e6c0..00000000 --- a/pgcommitfest/commitfest/tests/test_post_only.py +++ /dev/null @@ -1,29 +0,0 @@ -from datetime import datetime - -import pytest - -from pgcommitfest.commitfest.models import Patch, PatchOnCommitFest - -pytestmark = pytest.mark.django_db - - -@pytest.mark.parametrize("logged_in", [False, True]) -def test_get_does_not_modify_patch(client, open_cf, alice, logged_in): - patch = Patch.objects.create(name="Test patch") - poc = PatchOnCommitFest.objects.create( - patch=patch, - commitfest=open_cf, - enterdate=datetime.now(), - status=PatchOnCommitFest.STATUS_REVIEW, - ) - if logged_in: - client.force_login(alice) - - response = client.get(f"/patch/{patch.id}/status/author/") - assert response.status_code == 405 - poc.refresh_from_db() - assert poc.status == PatchOnCommitFest.STATUS_REVIEW - - response = client.post(f"/patch/{patch.id}/status/author/") - poc.refresh_from_db() - assert (poc.status == PatchOnCommitFest.STATUS_AUTHOR) == logged_in