@@ -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,
@@ -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>
@@ -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>
@@ -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>
@@ -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
@@ -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
@@ -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,
@@ -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,
@@ -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)
@@ -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',
),
@@ -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,
},
)
@@ -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,
},
)
@@ -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,
},
)
new file mode 100644
@@ -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.
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 --- 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 +++---- .../notes/issue-433-5f048abbe3789556.yaml | 19 +++++++++++++++ 14 files changed, 80 insertions(+), 58 deletions(-) create mode 100644 releasenotes/notes/issue-433-5f048abbe3789556.yaml