From patchwork Fri Sep 30 16:19:21 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stephen Finucane X-Patchwork-Id: 1684927 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=lists.ozlabs.org (client-ip=2404:9400:2:0:216:3eff:fee1:b9f1; helo=lists.ozlabs.org; envelope-from=patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org; receiver=) Authentication-Results: legolas.ozlabs.org; dkim=fail reason="key not found in DNS" header.d=that.guru header.i=@that.guru header.a=rsa-sha256 header.s=x header.b=rSTU1dGZ; dkim-atps=neutral Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2404:9400:2:0:216:3eff:fee1:b9f1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384)) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4MfFnz2v6dz1ypH for ; Sat, 1 Oct 2022 02:20:39 +1000 (AEST) Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4MfFnz2Pw5z3c96 for ; Sat, 1 Oct 2022 02:20:39 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="key not found in DNS" header.d=that.guru header.i=@that.guru header.a=rsa-sha256 header.s=x header.b=rSTU1dGZ; dkim-atps=neutral X-Original-To: patchwork@lists.ozlabs.org Delivered-To: patchwork@lists.ozlabs.org Authentication-Results: lists.ozlabs.org; spf=none (no SPF record) smtp.mailfrom=that.guru (client-ip=136.175.108.160; helo=mail-108-mta160.mxroute.com; envelope-from=stephen@that.guru; receiver=) Authentication-Results: lists.ozlabs.org; dkim=fail reason="key not found in DNS" header.d=that.guru header.i=@that.guru header.a=rsa-sha256 header.s=x header.b=rSTU1dGZ; dkim-atps=neutral Received: from mail-108-mta160.mxroute.com (mail-108-mta160.mxroute.com [136.175.108.160]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4MfFmz2FYWz3cGC for ; Sat, 1 Oct 2022 02:19:46 +1000 (AEST) Received: from mail-111-mta2.mxroute.com ([136.175.111.2] filter006.mxroute.com) (Authenticated sender: mN4UYu2MZsgR) by mail-108-mta160.mxroute.com (ZoneMTA) with ESMTPSA id 1838f322d660002b7a.003 for (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES128-GCM-SHA256); Fri, 30 Sep 2022 16:19:33 +0000 X-Zone-Loop: 9a666af2462c4ed1585a0cd77aebcaa2e75340bba1a7 X-Originating-IP: [136.175.111.2] DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=that.guru; s=x; h=Content-Transfer-Encoding:MIME-Version:References:In-Reply-To: Message-Id:Date:Subject:Cc:To:From:Sender:Reply-To:Content-Type:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=zq249odPk7+M/T51JgO3fJgkXzEVUuPiSLyf3EhEAlY=; b=rSTU1dGZ5lOJ/zFvHh6cUPkGz2 AobByRKWW05Bz9vlwC3cjQK4RkelP780RAq1fswH19FX187tTRtk9Chd+tMYG0T5YtXh5oRFoLmV5 zLqRXKWNJTSL4sYGSoZWmk7q/629QZcYJSQ4sOd02MZzt2bUdinOvVjzrvSxiYDZUMOSZWvM9a+WT jQr7xzDoLvYee4qK+JP4vxNdu4dHQw522MXC8Xm8cTEDLSgXLt1cDz4ttsiIeAYuo1WNMCAl5j0ns E5VdM0eVftf8wJ0fbSs0jd+l2tvJUAaZu7lYgYpnDhidyYtgQgSdmPEkkHhdApn43VLQCblHMH9ad TrIwE2IA==; From: Stephen Finucane To: patchwork@lists.ozlabs.org Subject: [PATCH 10/10] urls: Encode slashes in message IDs Date: Fri, 30 Sep 2022 17:19:21 +0100 Message-Id: <20220930161921.266633-10-stephen@that.guru> In-Reply-To: <20220930161921.266633-1-stephen@that.guru> References: <20220930161921.266633-1-stephen@that.guru> MIME-Version: 1.0 X-Authenticated-Id: stephen@that.guru X-BeenThere: patchwork@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Patchwork development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: siddhesh@gotplt.org Errors-To: patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: "Patchwork" We were attempting to work around the fact that message IDs could contain slashes which in some cases broke our ability to generate meaningful URLs. Rather than doing this, insist that users encode these slashes so that we can distinguish between semantically meaningful slashes and those that form the URL. This is a slightly breaking change, but the current behavior is already broken (see the linked bug) so this seems reasonable. Signed-off-by: Stephen Finucane Closes: #433 Cc: dja@axtens.net Cc: siddhesh@gotplt.org --- notes/issue-433-5f048abbe3789556.yaml | 19 +++++++++++++++ patchwork/models.py | 23 ++++++++++++++----- .../patchwork/partials/download-buttons.html | 6 ++--- .../patchwork/partials/patch-list.html | 2 +- patchwork/templates/patchwork/submission.html | 8 +++---- patchwork/tests/api/test_cover.py | 2 +- patchwork/tests/api/test_patch.py | 2 +- patchwork/tests/views/test_bundles.py | 8 +++---- patchwork/tests/views/test_cover.py | 10 ++++---- patchwork/tests/views/test_patch.py | 22 +++++++++--------- patchwork/urls.py | 20 +++++----------- patchwork/views/comment.py | 4 ++-- patchwork/views/cover.py | 4 ++-- patchwork/views/patch.py | 8 +++---- 14 files changed, 80 insertions(+), 58 deletions(-) create mode 100644 notes/issue-433-5f048abbe3789556.yaml diff --git notes/issue-433-5f048abbe3789556.yaml notes/issue-433-5f048abbe3789556.yaml new file mode 100644 index 00000000..1d0c1553 --- /dev/null +++ notes/issue-433-5f048abbe3789556.yaml @@ -0,0 +1,19 @@ +--- +fixes: + - | + Message IDs containing slashes will now have these slashes percent-encoded. + Previously, attempts to access submissions whose Message IDs contained + slashes would result in a HTTP 404 on some Django versions. If you wish to + access such a submission, you must now percent-encode the slashes first. + For example, to access a patch, cover letter or comment with the following + message ID: + + bug-28101-10460-NydYNmfPGz@http.sourceware.org/bugzilla/ + + You should now use: + + bug-28101-10460-NydYNmfPGz@http.sourceware.org%2Dbugzilla%2D + + Both the web UI and REST API have been updated to generate URLs in this + format so this should only be noticable to users manually generating such + URLs. diff --git patchwork/models.py patchwork/models.py index d2507d4f..20ec9f06 100644 --- patchwork/models.py +++ patchwork/models.py @@ -369,12 +369,24 @@ class EmailMixin(models.Model): @property def url_msgid(self): - """A trimmed messageid, suitable for inclusion in URLs""" + """A trimmed Message ID, suitable for inclusion in URLs""" if settings.DEBUG: assert self.msgid[0] == '<' and self.msgid[-1] == '>' return self.msgid.strip('<>') + @property + def encoded_msgid(self): + """Like 'url_msgid' but with slashes percentage encoded.""" + # We don't want to encode all characters (i.e. use urllib.parse.quote) + # because that would result in us encoding the '@' present in all + # message IDs. Instead we only percent-encode any slashes present [1]. + # These are not common so this is very much expected to be an edge + # case. + # + # [1] https://datatracker.ietf.org/doc/html/rfc3986.html#section-2 + return self.url_msgid.replace('/', '%2F') + def save(self, *args, **kwargs): # Modifying a submission via admin interface changes '\n' newlines in # message content to '\r\n'. We need to fix them to avoid problems, @@ -436,7 +448,7 @@ class Cover(SubmissionMixin): 'cover-detail', kwargs={ 'project_id': self.project.linkname, - 'msgid': self.url_msgid, + 'msgid': self.encoded_msgid, }, ) @@ -445,7 +457,7 @@ class Cover(SubmissionMixin): 'cover-mbox', kwargs={ 'project_id': self.project.linkname, - 'msgid': self.url_msgid, + 'msgid': self.encoded_msgid, }, ) @@ -671,7 +683,7 @@ class Patch(SubmissionMixin): 'patch-detail', kwargs={ 'project_id': self.project.linkname, - 'msgid': self.url_msgid, + 'msgid': self.encoded_msgid, }, ) @@ -680,7 +692,7 @@ class Patch(SubmissionMixin): 'patch-mbox', kwargs={ 'project_id': self.project.linkname, - 'msgid': self.url_msgid, + 'msgid': self.encoded_msgid, }, ) @@ -760,7 +772,6 @@ class CoverComment(EmailMixin, models.Model): class PatchComment(EmailMixin, models.Model): - # parent patch = models.ForeignKey( Patch, diff --git patchwork/templates/patchwork/partials/download-buttons.html patchwork/templates/patchwork/partials/download-buttons.html index 149bbc62..34c5f8fc 100644 --- patchwork/templates/patchwork/partials/download-buttons.html +++ patchwork/templates/patchwork/partials/download-buttons.html @@ -4,16 +4,16 @@ {{ submission.id }} {% if submission.diff %} - diff - mbox {% else %} - mbox diff --git patchwork/templates/patchwork/partials/patch-list.html patchwork/templates/patchwork/partials/patch-list.html index a9a262eb..a882cd9d 100644 --- patchwork/templates/patchwork/partials/patch-list.html +++ patchwork/templates/patchwork/partials/patch-list.html @@ -186,7 +186,7 @@ $(document).ready(function() { {% endif %} - + {{ patch.name|default:"[no subject]"|truncatechars:100 }} diff --git patchwork/templates/patchwork/submission.html patchwork/templates/patchwork/submission.html index 266744d9..6ebd8415 100644 --- patchwork/templates/patchwork/submission.html +++ patchwork/templates/patchwork/submission.html @@ -72,7 +72,7 @@ {% if cover == submission %} {{ cover.name|default:"[no subject]"|truncatechars:100 }} {% else %} - + {{ cover.name|default:"[no subject]"|truncatechars:100 }} {% endif %} @@ -84,7 +84,7 @@ {% if sibling == submission %} {{ sibling.name|default:"[no subject]"|truncatechars:100 }} {% else %} - + {{ sibling.name|default:"[no subject]"|truncatechars:100 }} {% endif %} @@ -105,7 +105,7 @@ {% for sibling in related_same_project %}
  • {% if sibling.id != submission.id %} - + {{ sibling.name|default:"[no subject]"|truncatechars:100 }} {% endif %} @@ -116,7 +116,7 @@
  • - + {{ sibling.name|default:"[no subject]"|truncatechars:100 }} (in {{ sibling.project }})
    • diff --git patchwork/tests/api/test_cover.py patchwork/tests/api/test_cover.py index 126b3af1..44ae2ebf 100644 --- patchwork/tests/api/test_cover.py +++ patchwork/tests/api/test_cover.py @@ -116,7 +116,7 @@ class TestCoverAPI(utils.APITestCase): """Filter covers by msgid.""" cover = create_cover() - resp = self.client.get(self.api_url(), {'msgid': cover.url_msgid}) + resp = self.client.get(self.api_url(), {'msgid': cover.encoded_msgid}) self.assertEqual([cover.id], [x['id'] for x in resp.data]) # empty response if nothing matches diff --git patchwork/tests/api/test_patch.py patchwork/tests/api/test_patch.py index 0ba3042b..03b5be11 100644 --- patchwork/tests/api/test_patch.py +++ patchwork/tests/api/test_patch.py @@ -218,7 +218,7 @@ class TestPatchAPI(utils.APITestCase): """Filter patches by msgid.""" patch = self._create_patch() - resp = self.client.get(self.api_url(), {'msgid': patch.url_msgid}) + resp = self.client.get(self.api_url(), {'msgid': patch.encoded_msgid}) self.assertEqual([patch.id], [x['id'] for x in resp.data]) # empty response if nothing matches diff --git patchwork/tests/views/test_bundles.py patchwork/tests/views/test_bundles.py index b26badc8..b730bdf3 100644 --- patchwork/tests/views/test_bundles.py +++ patchwork/tests/views/test_bundles.py @@ -496,7 +496,7 @@ class BundleCreateFromPatchTest(BundleTestBase): 'patch-detail', kwargs={ 'project_id': patch.project.linkname, - 'msgid': patch.url_msgid, + 'msgid': patch.encoded_msgid, }, ), params, @@ -519,7 +519,7 @@ class BundleCreateFromPatchTest(BundleTestBase): 'patch-detail', kwargs={ 'project_id': patch.project.linkname, - 'msgid': patch.url_msgid, + 'msgid': patch.encoded_msgid, }, ), params, @@ -655,7 +655,7 @@ class BundleAddFromPatchTest(BundleTestBase): 'patch-detail', kwargs={ 'project_id': patch.project.linkname, - 'msgid': patch.url_msgid, + 'msgid': patch.encoded_msgid, }, ), params, @@ -680,7 +680,7 @@ class BundleAddFromPatchTest(BundleTestBase): 'patch-detail', kwargs={ 'project_id': patch.project.linkname, - 'msgid': patch.url_msgid, + 'msgid': patch.encoded_msgid, }, ), params, diff --git patchwork/tests/views/test_cover.py patchwork/tests/views/test_cover.py index f33a6238..ee1f205f 100644 --- patchwork/tests/views/test_cover.py +++ patchwork/tests/views/test_cover.py @@ -19,14 +19,14 @@ class CoverViewTest(TestCase): 'patch-detail', kwargs={ 'project_id': cover.project.linkname, - 'msgid': cover.url_msgid, + 'msgid': cover.encoded_msgid, }, ) redirect_url = reverse( 'cover-detail', kwargs={ 'project_id': cover.project.linkname, - 'msgid': cover.url_msgid, + 'msgid': cover.encoded_msgid, }, ) @@ -43,7 +43,7 @@ class CoverViewTest(TestCase): 'cover-detail', kwargs={ 'project_id': cover.project.linkname, - 'msgid': cover.url_msgid, + 'msgid': cover.encoded_msgid, }, ) @@ -60,7 +60,7 @@ class CoverViewTest(TestCase): 'cover-mbox', kwargs={ 'project_id': cover.project.linkname, - 'msgid': cover.url_msgid, + 'msgid': cover.encoded_msgid, }, ) @@ -98,7 +98,7 @@ class CommentRedirectTest(TestCase): 'cover-detail', kwargs={ 'project_id': cover.project.linkname, - 'msgid': cover.url_msgid, + 'msgid': cover.encoded_msgid, }, ), comment_id, diff --git patchwork/tests/views/test_patch.py patchwork/tests/views/test_patch.py index d1de8ec9..70a2c836 100644 --- patchwork/tests/views/test_patch.py +++ patchwork/tests/views/test_patch.py @@ -212,14 +212,14 @@ class PatchViewTest(TestCase): 'cover-detail', kwargs={ 'project_id': patch.project.linkname, - 'msgid': patch.url_msgid, + 'msgid': patch.encoded_msgid, }, ) redirect_url = reverse( 'patch-detail', kwargs={ 'project_id': patch.project.linkname, - 'msgid': patch.url_msgid, + 'msgid': patch.encoded_msgid, }, ) @@ -238,7 +238,7 @@ class PatchViewTest(TestCase): 'patch-detail', kwargs={ 'project_id': patch.project.linkname, - 'msgid': patch.url_msgid, + 'msgid': patch.encoded_msgid, }, ), comment_id, @@ -257,7 +257,7 @@ class PatchViewTest(TestCase): 'patch-detail', kwargs={ 'project_id': patch.project.linkname, - 'msgid': patch.url_msgid, + 'msgid': patch.encoded_msgid, }, ) @@ -274,7 +274,7 @@ class PatchViewTest(TestCase): 'patch-mbox', kwargs={ 'project_id': patch.project.linkname, - 'msgid': patch.url_msgid, + 'msgid': patch.encoded_msgid, }, ) @@ -291,7 +291,7 @@ class PatchViewTest(TestCase): 'patch-raw', kwargs={ 'project_id': patch.project.linkname, - 'msgid': patch.url_msgid, + 'msgid': patch.encoded_msgid, }, ) @@ -315,7 +315,7 @@ class PatchViewTest(TestCase): 'patch-detail', kwargs={ 'project_id': patch.project.linkname, - 'msgid': patch.url_msgid, + 'msgid': patch.encoded_msgid, }, ) response = self.client.get(requested_url) @@ -358,7 +358,7 @@ class PatchViewTest(TestCase): 'patch-detail', kwargs={ 'project_id': patch.project.linkname, - 'msgid': patch.url_msgid, + 'msgid': patch.encoded_msgid, }, ) response = self.client.get(requested_url) @@ -511,7 +511,7 @@ class UTF8PatchViewTest(TestCase): response = self.client.get( reverse( 'patch-detail', - args=[self.patch.project.linkname, self.patch.url_msgid], + args=[self.patch.project.linkname, self.patch.encoded_msgid], ) ) self.assertContains(response, self.patch.name) @@ -520,7 +520,7 @@ class UTF8PatchViewTest(TestCase): response = self.client.get( reverse( 'patch-mbox', - args=[self.patch.project.linkname, self.patch.url_msgid], + args=[self.patch.project.linkname, self.patch.encoded_msgid], ) ) self.assertEqual(response.status_code, 200) @@ -530,7 +530,7 @@ class UTF8PatchViewTest(TestCase): response = self.client.get( reverse( 'patch-raw', - args=[self.patch.project.linkname, self.patch.url_msgid], + args=[self.patch.project.linkname, self.patch.encoded_msgid], ) ) self.assertEqual(response.status_code, 200) diff --git patchwork/urls.py patchwork/urls.py index ab606f1c..f4d67aa7 100644 --- patchwork/urls.py +++ patchwork/urls.py @@ -47,29 +47,21 @@ urlpatterns = [ name='project-detail', ), # patch views - # NOTE(dja): Per the RFC, msgids can contain slashes. There doesn't seem - # to be an easy way to tell Django to urlencode the slash when generating - # URLs, so instead we must use a permissive regex (.+ rather than [^/]+). - # This also means we need to put the raw and mbox URLs first, otherwise the - # patch-detail regex will just greedily grab those parts into a massive and - # wrong msgid. - # - # This does mean that message-ids that end in '/raw/' or '/mbox/' will not - # work, but it is RECOMMENDED by the RFC that the right hand side of the @ - # contains a domain, so I think breaking on messages that have "domains" - # ending in /raw/ or /mbox/ is good enough. + # NOTE(stephenfin): Per the RFC, msgids can contain slashes. Users are + # required to percent-encode any slashes present to generate valid URLs. + # The API does this automatically. path( - 'project//patch//raw/', + 'project//patch//raw/', patch_views.patch_raw, name='patch-raw', ), path( - 'project//patch//mbox/', + 'project//patch//mbox/', patch_views.patch_mbox, name='patch-mbox', ), path( - 'project//patch//', + 'project//patch//', patch_views.patch_detail, name='patch-detail', ), diff --git patchwork/views/comment.py patchwork/views/comment.py index 4f699224..98232a9e 100644 --- patchwork/views/comment.py +++ patchwork/views/comment.py @@ -29,7 +29,7 @@ def comment(request, comment_id): 'patch-detail', kwargs={ 'project_id': patch.project.linkname, - 'msgid': patch.url_msgid, + 'msgid': patch.encoded_msgid, }, ) else: # cover @@ -37,7 +37,7 @@ def comment(request, comment_id): 'cover-detail', kwargs={ 'project_id': cover.project.linkname, - 'msgid': cover.url_msgid, + 'msgid': cover.encoded_msgid, }, ) diff --git patchwork/views/cover.py patchwork/views/cover.py index 3368186b..15013a89 100644 --- patchwork/views/cover.py +++ patchwork/views/cover.py @@ -71,7 +71,7 @@ def cover_by_id(request, cover_id): 'cover-detail', kwargs={ 'project_id': cover.project.linkname, - 'msgid': cover.url_msgid, + 'msgid': cover.encoded_msgid, }, ) @@ -85,7 +85,7 @@ def cover_mbox_by_id(request, cover_id): 'cover-mbox', kwargs={ 'project_id': cover.project.linkname, - 'msgid': cover.url_msgid, + 'msgid': cover.encoded_msgid, }, ) diff --git patchwork/views/patch.py patchwork/views/patch.py index 75705720..9f1bb415 100644 --- patchwork/views/patch.py +++ patchwork/views/patch.py @@ -40,7 +40,7 @@ def patch_list(request, project_id): def patch_detail(request, project_id, msgid): project = get_object_or_404(Project, linkname=project_id) - db_msgid = '<%s>' % msgid + db_msgid = f"<{msgid.replace('%2F', '/')}>" # redirect to cover letters where necessary try: @@ -190,7 +190,7 @@ def patch_by_id(request, patch_id): 'patch-detail', kwargs={ 'project_id': patch.project.linkname, - 'msgid': patch.url_msgid, + 'msgid': patch.encoded_msgid, }, ) @@ -204,7 +204,7 @@ def patch_mbox_by_id(request, patch_id): 'patch-mbox', kwargs={ 'project_id': patch.project.linkname, - 'msgid': patch.url_msgid, + 'msgid': patch.encoded_msgid, }, ) @@ -218,7 +218,7 @@ def patch_raw_by_id(request, patch_id): 'patch-raw', kwargs={ 'project_id': patch.project.linkname, - 'msgid': patch.url_msgid, + 'msgid': patch.encoded_msgid, }, )