Skip to content

Commit 106242a

Browse files
stephenfinstephenfin
committed
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 <[email protected]> Closes: #433 Cc: [email protected] Cc: [email protected] (cherry picked from commit 2653fdb)
1 parent 8a0031b commit 106242a

File tree

14 files changed

+80
-58
lines changed

14 files changed

+80
-58
lines changed

‎notes/issue-433-5f048abbe3789556.yaml‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
---
2+
fixes:
3+
- |
4+
Message IDs containing slashes will now have these slashes percent-encoded.
5+
Previously, attempts to access submissions whose Message IDs contained
6+
slashes would result in a HTTP 404 on some Django versions. If you wish to
7+
access such a submission, you must now percent-encode the slashes first.
8+
For example, to access a patch, cover letter or comment with the following
9+
message ID:
10+
11+
[email protected]/bugzilla/
12+
13+
You should now use:
14+
15+
[email protected]%2Dbugzilla%2D
16+
17+
Both the web UI and REST API have been updated to generate URLs in this
18+
format so this should only be noticable to users manually generating such
19+
URLs.

‎patchwork/models.py‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -369,12 +369,24 @@ def patch_responses(self):
369369

370370
@property
371371
def url_msgid(self):
372-
"""A trimmed messageid, suitable for inclusion in URLs"""
372+
"""A trimmed Message ID, suitable for inclusion in URLs"""
373373
if settings.DEBUG:
374374
assert self.msgid[0] == '<' and self.msgid[-1] == '>'
375375

376376
return self.msgid.strip('<>')
377377

378+
@property
379+
def encoded_msgid(self):
380+
"""Like 'url_msgid' but with slashes percentage encoded."""
381+
# We don't want to encode all characters (i.e. use urllib.parse.quote)
382+
# because that would result in us encoding the '@' present in all
383+
# message IDs. Instead we only percent-encode any slashes present [1].
384+
# These are not common so this is very much expected to be an edge
385+
# case.
386+
#
387+
# [1] https://datatracker.ietf.org/doc/html/rfc3986.html#section-2
388+
return self.url_msgid.replace('/', '%2F')
389+
378390
def save(self, *args, **kwargs):
379391
# Modifying a submission via admin interface changes '\n' newlines in
380392
# message content to '\r\n'. We need to fix them to avoid problems,
@@ -436,7 +448,7 @@ def get_absolute_url(self):
436448
'cover-detail',
437449
kwargs={
438450
'project_id': self.project.linkname,
439-
'msgid': self.url_msgid,
451+
'msgid': self.encoded_msgid,
440452
},
441453
)
442454

@@ -445,7 +457,7 @@ def get_mbox_url(self):
445457
'cover-mbox',
446458
kwargs={
447459
'project_id': self.project.linkname,
448-
'msgid': self.url_msgid,
460+
'msgid': self.encoded_msgid,
449461
},
450462
)
451463

@@ -671,7 +683,7 @@ def get_absolute_url(self):
671683
'patch-detail',
672684
kwargs={
673685
'project_id': self.project.linkname,
674-
'msgid': self.url_msgid,
686+
'msgid': self.encoded_msgid,
675687
},
676688
)
677689

@@ -680,7 +692,7 @@ def get_mbox_url(self):
680692
'patch-mbox',
681693
kwargs={
682694
'project_id': self.project.linkname,
683-
'msgid': self.url_msgid,
695+
'msgid': self.encoded_msgid,
684696
},
685697
)
686698

@@ -760,7 +772,6 @@ class Meta:
760772

761773

762774
class PatchComment(EmailMixin, models.Model):
763-
# parent
764775

765776
patch = models.ForeignKey(
766777
Patch,

‎patchwork/templates/patchwork/partials/download-buttons.html‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,16 +4,16 @@
44
{{ submission.id }}
55
</button>
66
{% if submission.diff %}
7-
<a href="{% url 'patch-raw' project_id=project.linkname msgid=submission.url_msgid %}"
7+
<a href="{% url 'patch-raw' project_id=project.linkname msgid=submission.encoded_msgid %}"
88
class="btn btn-default" role="button" title="Download patch diff">
99
diff
1010
</a>
11-
<a href="{% url 'patch-mbox' project_id=project.linkname msgid=submission.url_msgid %}"
11+
<a href="{% url 'patch-mbox' project_id=project.linkname msgid=submission.encoded_msgid %}"
1212
class="btn btn-default" role="button" title="Download patch mbox">
1313
mbox
1414
</a>
1515
{% else %}
16-
<a href="{% url 'cover-mbox' project_id=project.linkname msgid=submission.url_msgid %}"
16+
<a href="{% url 'cover-mbox' project_id=project.linkname msgid=submission.encoded_msgid %}"
1717
class="btn btn-default" role="button" title="Download cover mbox">
1818
mbox
1919
</a>

‎patchwork/templates/patchwork/partials/patch-list.html‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@
186186
</td>
187187
{% endif %}
188188
<td>
189-
<a href="{% url 'patch-detail' project_id=project.linkname msgid=patch.url_msgid %}">
189+
<a href="{% url 'patch-detail' project_id=project.linkname msgid=patch.encoded_msgid %}">
190190
{{ patch.name|default:"[no subject]"|truncatechars:100 }}
191191
</a>
192192
</td>

‎patchwork/templates/patchwork/submission.html‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ <h1>{{ submission.name }}</h1>
7272
{% if cover == submission %}
7373
{{ cover.name|default:"[no subject]"|truncatechars:100 }}
7474
{% else %}
75-
<a href="{% url 'cover-detail' project_id=project.linkname msgid=cover.url_msgid %}">
75+
<a href="{% url 'cover-detail' project_id=project.linkname msgid=cover.encoded_msgid %}">
7676
{{ cover.name|default:"[no subject]"|truncatechars:100 }}
7777
</a>
7878
{% endif %}
@@ -84,7 +84,7 @@ <h1>{{ submission.name }}</h1>
8484
{% if sibling == submission %}
8585
{{ sibling.name|default:"[no subject]"|truncatechars:100 }}
8686
{% else %}
87-
<a href="{% url 'patch-detail' project_id=project.linkname msgid=sibling.url_msgid %}">
87+
<a href="{% url 'patch-detail' project_id=project.linkname msgid=sibling.encoded_msgid %}">
8888
{{ sibling.name|default:"[no subject]"|truncatechars:100 }}
8989
</a>
9090
{% endif %}
@@ -105,7 +105,7 @@ <h1>{{ submission.name }}</h1>
105105
{% for sibling in related_same_project %}
106106
<li>
107107
{% if sibling.id != submission.id %}
108-
<a href="{% url 'patch-detail' project_id=project.linkname msgid=sibling.url_msgid %}">
108+
<a href="{% url 'patch-detail' project_id=project.linkname msgid=sibling.encoded_msgid %}">
109109
{{ sibling.name|default:"[no subject]"|truncatechars:100 }}
110110
</a>
111111
{% endif %}
@@ -116,7 +116,7 @@ <h1>{{ submission.name }}</h1>
116116
<div id="related-outside" class="submission-list" style="display:none;">
117117
{% for sibling in related_outside %}
118118
<li>
119-
<a href="{% url 'patch-detail' project_id=sibling.project.linkname msgid=sibling.url_msgid %}">
119+
<a href="{% url 'patch-detail' project_id=sibling.project.linkname msgid=sibling.encoded_msgid %}">
120120
{{ sibling.name|default:"[no subject]"|truncatechars:100 }}
121121
</a> (in {{ sibling.project }})
122122
</li>

‎patchwork/tests/api/test_cover.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ def test_list_filter_msgid(self):
116116
"""Filter covers by msgid."""
117117
cover = create_cover()
118118

119-
resp = self.client.get(self.api_url(), {'msgid': cover.url_msgid})
119+
resp = self.client.get(self.api_url(), {'msgid': cover.encoded_msgid})
120120
self.assertEqual([cover.id], [x['id'] for x in resp.data])
121121

122122
# empty response if nothing matches

‎patchwork/tests/api/test_patch.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ def test_list_filter_msgid(self):
218218
"""Filter patches by msgid."""
219219
patch = self._create_patch()
220220

221-
resp = self.client.get(self.api_url(), {'msgid': patch.url_msgid})
221+
resp = self.client.get(self.api_url(), {'msgid': patch.encoded_msgid})
222222
self.assertEqual([patch.id], [x['id'] for x in resp.data])
223223

224224
# empty response if nothing matches

‎patchwork/tests/views/test_bundles.py‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -496,7 +496,7 @@ def test_create_non_empty_bundle(self):
496496
'patch-detail',
497497
kwargs={
498498
'project_id': patch.project.linkname,
499-
'msgid': patch.url_msgid,
499+
'msgid': patch.encoded_msgid,
500500
},
501501
),
502502
params,
@@ -519,7 +519,7 @@ def test_create_with_existing_name(self):
519519
'patch-detail',
520520
kwargs={
521521
'project_id': patch.project.linkname,
522-
'msgid': patch.url_msgid,
522+
'msgid': patch.encoded_msgid,
523523
},
524524
),
525525
params,
@@ -655,7 +655,7 @@ def test_add_to_empty_bundle(self):
655655
'patch-detail',
656656
kwargs={
657657
'project_id': patch.project.linkname,
658-
'msgid': patch.url_msgid,
658+
'msgid': patch.encoded_msgid,
659659
},
660660
),
661661
params,
@@ -680,7 +680,7 @@ def test_add_to_non_empty_bundle(self):
680680
'patch-detail',
681681
kwargs={
682682
'project_id': patch.project.linkname,
683-
'msgid': patch.url_msgid,
683+
'msgid': patch.encoded_msgid,
684684
},
685685
),
686686
params,

‎patchwork/tests/views/test_cover.py‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,14 +19,14 @@ def test_redirect(self):
1919
'patch-detail',
2020
kwargs={
2121
'project_id': cover.project.linkname,
22-
'msgid': cover.url_msgid,
22+
'msgid': cover.encoded_msgid,
2323
},
2424
)
2525
redirect_url = reverse(
2626
'cover-detail',
2727
kwargs={
2828
'project_id': cover.project.linkname,
29-
'msgid': cover.url_msgid,
29+
'msgid': cover.encoded_msgid,
3030
},
3131
)
3232

@@ -43,7 +43,7 @@ def test_old_detail_url(self):
4343
'cover-detail',
4444
kwargs={
4545
'project_id': cover.project.linkname,
46-
'msgid': cover.url_msgid,
46+
'msgid': cover.encoded_msgid,
4747
},
4848
)
4949

@@ -60,7 +60,7 @@ def test_old_mbox_url(self):
6060
'cover-mbox',
6161
kwargs={
6262
'project_id': cover.project.linkname,
63-
'msgid': cover.url_msgid,
63+
'msgid': cover.encoded_msgid,
6464
},
6565
)
6666

@@ -98,7 +98,7 @@ def test_cover_redirect(self):
9898
'cover-detail',
9999
kwargs={
100100
'project_id': cover.project.linkname,
101-
'msgid': cover.url_msgid,
101+
'msgid': cover.encoded_msgid,
102102
},
103103
),
104104
comment_id,

‎patchwork/tests/views/test_patch.py‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -211,14 +211,14 @@ def test_redirect(self):
211211
'cover-detail',
212212
kwargs={
213213
'project_id': patch.project.linkname,
214-
'msgid': patch.url_msgid,
214+
'msgid': patch.encoded_msgid,
215215
},
216216
)
217217
redirect_url = reverse(
218218
'patch-detail',
219219
kwargs={
220220
'project_id': patch.project.linkname,
221-
'msgid': patch.url_msgid,
221+
'msgid': patch.encoded_msgid,
222222
},
223223
)
224224

@@ -237,7 +237,7 @@ def test_comment_redirect(self):
237237
'patch-detail',
238238
kwargs={
239239
'project_id': patch.project.linkname,
240-
'msgid': patch.url_msgid,
240+
'msgid': patch.encoded_msgid,
241241
},
242242
),
243243
comment_id,
@@ -256,7 +256,7 @@ def test_old_detail_url(self):
256256
'patch-detail',
257257
kwargs={
258258
'project_id': patch.project.linkname,
259-
'msgid': patch.url_msgid,
259+
'msgid': patch.encoded_msgid,
260260
},
261261
)
262262

@@ -273,7 +273,7 @@ def test_old_mbox_url(self):
273273
'patch-mbox',
274274
kwargs={
275275
'project_id': patch.project.linkname,
276-
'msgid': patch.url_msgid,
276+
'msgid': patch.encoded_msgid,
277277
},
278278
)
279279

@@ -290,7 +290,7 @@ def test_old_raw_url(self):
290290
'patch-raw',
291291
kwargs={
292292
'project_id': patch.project.linkname,
293-
'msgid': patch.url_msgid,
293+
'msgid': patch.encoded_msgid,
294294
},
295295
)
296296

@@ -314,7 +314,7 @@ def test_escaping(self):
314314
'patch-detail',
315315
kwargs={
316316
'project_id': patch.project.linkname,
317-
'msgid': patch.url_msgid,
317+
'msgid': patch.encoded_msgid,
318318
},
319319
)
320320
response = self.client.get(requested_url)
@@ -357,7 +357,7 @@ def test_patch_with_checks(self):
357357
'patch-detail',
358358
kwargs={
359359
'project_id': patch.project.linkname,
360-
'msgid': patch.url_msgid,
360+
'msgid': patch.encoded_msgid,
361361
},
362362
)
363363
response = self.client.get(requested_url)
@@ -502,7 +502,7 @@ def test_patch_view(self):
502502
response = self.client.get(
503503
reverse(
504504
'patch-detail',
505-
args=[self.patch.project.linkname, self.patch.url_msgid],
505+
args=[self.patch.project.linkname, self.patch.encoded_msgid],
506506
)
507507
)
508508
self.assertContains(response, self.patch.name)
@@ -511,7 +511,7 @@ def test_mbox_view(self):
511511
response = self.client.get(
512512
reverse(
513513
'patch-mbox',
514-
args=[self.patch.project.linkname, self.patch.url_msgid],
514+
args=[self.patch.project.linkname, self.patch.encoded_msgid],
515515
)
516516
)
517517
self.assertEqual(response.status_code, 200)
@@ -521,7 +521,7 @@ def test_raw_view(self):
521521
response = self.client.get(
522522
reverse(
523523
'patch-raw',
524-
args=[self.patch.project.linkname, self.patch.url_msgid],
524+
args=[self.patch.project.linkname, self.patch.encoded_msgid],
525525
)
526526
)
527527
self.assertEqual(response.status_code, 200)

0 commit comments

Comments
 (0)