[PATCH 10/10] urls: Encode slashes in message IDs
Stephen Finucane
stephen at that.guru
Sat Oct 1 02:19:21 AEST 2022
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 at that.guru>
Closes: #433
Cc: dja at axtens.net
Cc: siddhesh at 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 at http.sourceware.org/bugzilla/
+
+ You should now use:
+
+ bug-28101-10460-NydYNmfPGz at 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 }}
</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 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() {
</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 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 %}
- <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 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/<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 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,
},
)
--
2.37.3
More information about the Patchwork
mailing list