urls: Encode slashes in message IDs

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 <stephen@that.guru>
Closes: #433
Cc: dja@axtens.net
Cc: siddhesh@gotplt.org
(cherry picked from commit 2653fdbb2f33a9c762936ca97e3c6888b185cb4b)

diff --git a/notes/issue-433-5f048abbe3789556.yaml b/notes/issue-433-5f048abbe3789556.yaml
new file mode 100644 (file)
index 0000000..1d0c155
--- /dev/null
+++ b/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 a/patchwork/models.py b/patchwork/models.py
index 264af532e94268e62374eef2d24825c24bd214e3..deac33f84199525245ea903f88f10aaf906537ce 100644 (file)
--- a/patchwork/models.py
+++ b/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 a/patchwork/templates/patchwork/partials/download-buttons.html b/patchwork/templates/patchwork/partials/download-buttons.html
index 149bbc626a7e09209ae805b526da4433ad105629..34c5f8fc63d642b918be05e217ba6d799e7c44dc 100644 (file)
--- a/patchwork/templates/patchwork/partials/download-buttons.html
+++ b/patchwork/templates/patchwork/partials/download-buttons.html
@@ -4,16 +4,16 @@
     {{ submission.id }}
   </button>
 {% if submission.diff %}
-  <a href="{% url 'patch-raw' project_id=project.linkname msgid=submission.url_msgid %}"
+  <a href="{% url 'patch-raw' project_id=project.linkname msgid=submission.encoded_msgid %}"
       class="btn btn-default" role="button" title="Download patch diff">
     diff
   </a>
-  <a href="{% url 'patch-mbox' project_id=project.linkname msgid=submission.url_msgid %}"
+  <a href="{% url 'patch-mbox' project_id=project.linkname msgid=submission.encoded_msgid %}"
       class="btn btn-default" role="button" title="Download patch mbox">
     mbox
   </a>
 {% else %}
-  <a href="{% url 'cover-mbox' project_id=project.linkname msgid=submission.url_msgid %}"
+  <a href="{% url 'cover-mbox' project_id=project.linkname msgid=submission.encoded_msgid %}"
       class="btn btn-default" role="button" title="Download cover mbox">
     mbox
   </a>

diff --git a/patchwork/templates/patchwork/partials/patch-list.html b/patchwork/templates/patchwork/partials/patch-list.html
index a9a262ebd81349c561d2babcff89ff983fbca95e..a882cd9d82653d0b71addc89cb09e7ff01600de2 100644 (file)
--- a/patchwork/templates/patchwork/partials/patch-list.html
+++ b/patchwork/templates/patchwork/partials/patch-list.html
@@ -186,7 +186,7 @@ $(document).ready(function() {
         </td>
 {% endif %}
         <td>
-          <a href="{% url 'patch-detail' project_id=project.linkname msgid=patch.url_msgid %}">
+          <a href="{% url 'patch-detail' project_id=project.linkname msgid=patch.encoded_msgid %}">
             {{ patch.name|default:"[no subject]"|truncatechars:100 }}
           </a>
         </td>

diff --git a/patchwork/templates/patchwork/submission.html b/patchwork/templates/patchwork/submission.html
index 266744d9535f42f64347bb0f138f09b63f98b848..6ebd8415237b838d6cd927420480b698b9e665e1 100644 (file)
--- a/patchwork/templates/patchwork/submission.html
+++ b/patchwork/templates/patchwork/submission.html
@@ -72,7 +72,7 @@
 {% if cover == submission %}
                 {{ cover.name|default:"[no subject]"|truncatechars:100 }}
 {% else %}
-              <a href="{% url 'cover-detail' project_id=project.linkname msgid=cover.url_msgid %}">
+              <a href="{% url 'cover-detail' project_id=project.linkname msgid=cover.encoded_msgid %}">
                 {{ cover.name|default:"[no subject]"|truncatechars:100 }}
               </a>
 {% endif %}
@@ -84,7 +84,7 @@
 {% if sibling == submission %}
                 {{ sibling.name|default:"[no subject]"|truncatechars:100 }}
 {% else %}
-              <a href="{% url 'patch-detail' project_id=project.linkname msgid=sibling.url_msgid %}">
+              <a href="{% url 'patch-detail' project_id=project.linkname msgid=sibling.encoded_msgid %}">
                 {{ sibling.name|default:"[no subject]"|truncatechars:100 }}
               </a>
 {% endif %}
@@ -105,7 +105,7 @@
 {% for sibling in related_same_project %}
           <li>
 {% if sibling.id != submission.id %}
-            <a href="{% url 'patch-detail' project_id=project.linkname msgid=sibling.url_msgid %}">
+            <a href="{% url 'patch-detail' project_id=project.linkname msgid=sibling.encoded_msgid %}">
               {{ sibling.name|default:"[no subject]"|truncatechars:100 }}
             </a>
 {% endif %}
@@ -116,7 +116,7 @@
           <div id="related-outside" class="submission-list" style="display:none;">
 {% for sibling in related_outside %}
             <li>
-              <a href="{% url 'patch-detail' project_id=sibling.project.linkname msgid=sibling.url_msgid %}">
+              <a href="{% url 'patch-detail' project_id=sibling.project.linkname msgid=sibling.encoded_msgid %}">
                 {{ sibling.name|default:"[no subject]"|truncatechars:100 }}
               </a> (in {{ sibling.project }})
             </li>

diff --git a/patchwork/tests/api/test_cover.py b/patchwork/tests/api/test_cover.py
index 126b3af18991156fbe50046b47ed3f292308b66b..44ae2ebfe981c38cd7fa2a8a6e05979fafe7ff27 100644 (file)
--- a/patchwork/tests/api/test_cover.py
+++ b/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 a/patchwork/tests/api/test_patch.py b/patchwork/tests/api/test_patch.py
index 0ba3042bcf122825df3d64bacd23ed8826a90910..03b5be11fc1c1ee3ff6acd7285351d9bf5e3dc27 100644 (file)
--- a/patchwork/tests/api/test_patch.py
+++ b/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 a/patchwork/tests/views/test_bundles.py b/patchwork/tests/views/test_bundles.py
index b26badc8d9d544ad4be2b3000129fb420f2148fe..b730bdf369741e8be57a1e3d1d1379aabf384582 100644 (file)
--- a/patchwork/tests/views/test_bundles.py
+++ b/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 a/patchwork/tests/views/test_cover.py b/patchwork/tests/views/test_cover.py
index f33a6238d46de1915768c9fcbeac8856dbb04c1f..ee1f205f0b39693cd7b6d0acc81a9c355a042265 100644 (file)
--- a/patchwork/tests/views/test_cover.py
+++ b/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 a/patchwork/tests/views/test_patch.py b/patchwork/tests/views/test_patch.py
index 97b0e97e1d873e4b6d9d22de9be2578241b8183a..f3990d4264acc374e2128556780670f95ce6c448 100644 (file)
--- a/patchwork/tests/views/test_patch.py
+++ b/patchwork/tests/views/test_patch.py
@@ -211,14 +211,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,
             },
         )
 
@@ -237,7 +237,7 @@ class PatchViewTest(TestCase):
                 'patch-detail',
                 kwargs={
                     'project_id': patch.project.linkname,
-                    'msgid': patch.url_msgid,
+                    'msgid': patch.encoded_msgid,
                 },
             ),
             comment_id,
@@ -256,7 +256,7 @@ class PatchViewTest(TestCase):
             'patch-detail',
             kwargs={
                 'project_id': patch.project.linkname,
-                'msgid': patch.url_msgid,
+                'msgid': patch.encoded_msgid,
             },
         )
 
@@ -273,7 +273,7 @@ class PatchViewTest(TestCase):
             'patch-mbox',
             kwargs={
                 'project_id': patch.project.linkname,
-                'msgid': patch.url_msgid,
+                'msgid': patch.encoded_msgid,
             },
         )
 
@@ -290,7 +290,7 @@ class PatchViewTest(TestCase):
             'patch-raw',
             kwargs={
                 'project_id': patch.project.linkname,
-                'msgid': patch.url_msgid,
+                'msgid': patch.encoded_msgid,
             },
         )
 
@@ -314,7 +314,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)
@@ -357,7 +357,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)
@@ -502,7 +502,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)
@@ -511,7 +511,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)
@@ -521,7 +521,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 a/patchwork/urls.py b/patchwork/urls.py
index ab606f1c48a92470ae3019a0b9e8ee207517a6a8..f4d67aa707423c5dad33ce1e31dec9bfbcf101f4 100644 (file)
--- a/patchwork/urls.py
+++ b/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/<project_id>/patch/<path:msgid>/raw/',
+        'project/<project_id>/patch/<str:msgid>/raw/',
         patch_views.patch_raw,
         name='patch-raw',
     ),
     path(
-        'project/<project_id>/patch/<path:msgid>/mbox/',
+        'project/<project_id>/patch/<str:msgid>/mbox/',
         patch_views.patch_mbox,
         name='patch-mbox',
     ),
     path(
-        'project/<project_id>/patch/<path:msgid>/',
+        'project/<project_id>/patch/<str:msgid>/',
         patch_views.patch_detail,
         name='patch-detail',
     ),

diff --git a/patchwork/views/comment.py b/patchwork/views/comment.py
index 4f699224df601f1df5f5dbfe651ebbe0e15166d3..98232a9e2fadd02def92733e951a3b80b130bbfd 100644 (file)
--- a/patchwork/views/comment.py
+++ b/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 a/patchwork/views/cover.py b/patchwork/views/cover.py
index 3368186bf5411ab73ff45e8b0f41cda71a2b3a9c..15013a89e1cecbed99c06f981bf8c7f9a5850769 100644 (file)
--- a/patchwork/views/cover.py
+++ b/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 a/patchwork/views/patch.py b/patchwork/views/patch.py
index 757057206febf618131ffd447b7a3c9f4e0edb86..9f1bb415f662b39e9755cd29b2324d347a18d410 100644 (file)
--- a/patchwork/views/patch.py
+++ b/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,
         },
     )