Skip to content

Commit 4285eb8

Browse files
mekarpelesmekarpeles
committed
fix(templates): safe attribute construction for data-ol-link-track
modal_links.html, title_and_author.html, onboarding_card.html, and header_dropdown.html each built the tracking attribute via %-string formatting spliced into the tag unquoted. Templetor's bare $var escapes <, >, &, and " but not whitespace, so a tracking value containing a space injects a real new HTML attribute (e.g. onmouseover=...) since the escaped quote characters become inert &quot; text, not real delimiters. Confirmed exploitable against the prior code with a real render + injection payload before applying this fix. Mirrors the fix already applied to Follow.html in internetarchive#12985: place the value inside a static, template-source quote character via $if var: attr="$var" instead of building the whole fragment in Python.
1 parent eeaf157 commit 4285eb8

6 files changed

Lines changed: 105 additions & 15 deletions

File tree

‎openlibrary/templates/home/onboarding_card.html‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,15 +9,12 @@
99
$# ol_link_track : str : Event name for analytics
1010
$# attribution : str : Attribution for the image used in the card
1111

12-
$ data_ol_link_track = ''
13-
$if ol_link_track:
14-
$ data_ol_link_track = 'data-ol-link-track="%s"' % ol_link_track
15-
1612
<div class="carousel__item tutorial__item">
1713
<a
1814
href="$href"
1915
tabindex="0"
20-
$data_ol_link_track
16+
$if ol_link_track:
17+
data-ol-link-track="$ol_link_track"
2118
>
2219
<img src="$img_src"
2320
alt=""

‎openlibrary/templates/lib/header_dropdown.html‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,11 @@
1717
<div class="$(props['name'])-component header-dropdown">
1818
$if singleton:
1919
$ link = props['links'][0]
20-
$ tracker = 'data-ol-link-track=%s|%s' % (track_prefix, link['track']) if (track_prefix and link.get('track')) else ''
21-
<a href="$link['href']" $tracker>$(props['label'])</a>
20+
$ tracker_value = '%s|%s' % (track_prefix, link['track']) if (track_prefix and link.get('track')) else ''
21+
<a href="$link['href']"
22+
$if tracker_value:
23+
data-ol-link-track="$tracker_value"
24+
>$(props['label'])</a>
2225
$else:
2326
<details>
2427
<summary $:cond(props['name'] == 'hamburger', 'data-ol-link-track="HeaderBar|Hamburger"')>
@@ -45,7 +48,7 @@
4548
>
4649
<ul class="dropdown-menu $(props['name'])-dropdown-menu">
4750
$for link in props['links']:
48-
$ tracker = 'data-ol-link-track=%s|%s' % (track_prefix, link['track']) if (track_prefix and link.get('track')) else ''
51+
$ tracker_value = '%s|%s' % (track_prefix, link['track']) if (track_prefix and link.get('track')) else ''
4952
$if 'subsection' in link:
5053
<li class="subsection">
5154
$(link['subsection'])
@@ -60,13 +63,19 @@
6063
$if 'post' in link:
6164
$ name = 'name=%s' % link['name'] if 'name' in link else ''
6265
<form $name action="$(link['post'])" method="post">
63-
<button $(tracker)>$(link["text"])</button>
66+
<button
67+
$if tracker_value:
68+
data-ol-link-track="$tracker_value"
69+
>$(link["text"])</button>
6470
</form>
6571
$else:
6672
$if is_privileged_user:
6773
$if 'badge' in link and cached_get_counts_by_mode(mode='open') > 0:
6874
<div class="app-drawer__badge">$(cached_get_counts_by_mode(mode='open'))</div>
69-
<a href="$(link['href'])" $(tracker)>
75+
<a href="$(link['href'])"
76+
$if tracker_value:
77+
data-ol-link-track="$tracker_value"
78+
>
7079
$if 'new' in link and props['name'] == 'hamburger':
7180
<span>$(link['text'])</span><span class="app-drawer__badge">$_('New!')</span>
7281
$else:

‎openlibrary/templates/tests/icon_link_pattern_check.html‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
$def with (ga_data)
2+
$# Test-only fixture. Mirrors the icon_link() attribute-construction pattern
3+
$# duplicated in type/edition/modal_links.html and type/edition/title_and_author.html.
4+
$# Those files' real call sites only ever pass hardcoded literal ga_data, so there's
5+
$# no top-level parameter to drive an end-to-end injection test through them directly.
6+
$# See openlibrary/tests/test_link_track_attribute_escaping.py.
7+
<a href="javascript:;" class="icon-link"
8+
$if ga_data:
9+
data-ol-link-track="$ga_data"
10+
>link</a>

‎openlibrary/templates/type/edition/modal_links.html‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,10 @@
1212
$ link_class = '%s--redirect js-login-intent' % link_class
1313
$ href = '/account/login?redirect=%s' % ctx.path
1414

15-
$ ga_attr = 'data-ol-link-track=%s' % ga_data if ga_data else ''
16-
17-
<a href="$href" class="$link_class icon-link" $ga_attr $:data_attr>
15+
<a href="$href" class="$link_class icon-link"
16+
$if ga_data:
17+
data-ol-link-track="$ga_data"
18+
$:data_attr>
1819
<img class="icon-link__image" src="$img_src" alt="">
1920
<div class="icon-link__text">$link_text</div>
2021
</a>

‎openlibrary/templates/type/edition/title_and_author.html‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,10 @@
33
$def icon_link(link_class, img_src, link_text, login_redirect=True, ga_data=None):
44
$ href = 'javascript:;'
55

6-
$ ga_attr = 'data-ol-link-track=%s' % ga_data if ga_data else ''
7-
<a href="$href" class="$link_class icon-link" $ga_attr>
6+
<a href="$href" class="$link_class icon-link"
7+
$if ga_data:
8+
data-ol-link-track="$ga_data"
9+
>
810
<img class="icon-link__image" src="$img_src" alt="">
911
<div class="icon-link__text">$link_text</div>
1012
</a>

‎openlibrary/tests/test_link_track_attribute_escaping.py‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
"""Regression tests for issue #13186 (follow-up to PR #12985's Follow.html fix).
2+
3+
Templetor's bare `$var` substitution HTML-escapes `<`, `>`, `&`, `"` but not
4+
whitespace. Before this fix, onboarding_card.html, header_dropdown.html,
5+
modal_links.html, and title_and_author.html each built the tracking attribute
6+
as a single pre-formatted "attr=value" string spliced into the tag unquoted --
7+
so a value containing a space (e.g. "x onmouseover=alert(1)//") injects a real
8+
new HTML attribute, because the escaped `"` characters become inert `&quot;`
9+
text rather than real quote delimiters. The fix places the value inside a
10+
*static*, template-source quote character instead, which can't be broken out
11+
of regardless of what the value contains.
12+
"""
13+
14+
from bs4 import BeautifulSoup
15+
16+
INJECTION_PAYLOAD = "x onmouseover=alert(document.cookie)//"
17+
18+
19+
def test_onboarding_card_blocks_attribute_injection(render_template, request_context_fixture):
20+
request_context_fixture(lang="en")
21+
html = str(
22+
render_template(
23+
"home/onboarding_card",
24+
"Title",
25+
"Body",
26+
"/img.png",
27+
"/href",
28+
ol_link_track=INJECTION_PAYLOAD,
29+
)
30+
)
31+
a = BeautifulSoup(html, "lxml").find("a")
32+
assert a.get("onmouseover") is None
33+
assert a["data-ol-link-track"] == INJECTION_PAYLOAD
34+
35+
36+
def test_header_dropdown_singleton_blocks_attribute_injection(render_template, request_context_fixture):
37+
request_context_fixture(lang="en")
38+
props = {"name": "test-dropdown", "label": "Test", "links": [{"href": "/x", "track": INJECTION_PAYLOAD}]}
39+
html = str(render_template("lib/header_dropdown", props, track_prefix="Test"))
40+
a = BeautifulSoup(html, "lxml").find("a")
41+
assert a.get("onmouseover") is None
42+
assert a["data-ol-link-track"] == f"Test|{INJECTION_PAYLOAD}"
43+
44+
45+
def test_header_dropdown_menu_item_blocks_attribute_injection(render_template, request_context_fixture):
46+
request_context_fixture(lang="en")
47+
props = {
48+
"name": "test-dropdown",
49+
"label": "Test",
50+
"links": [
51+
{"href": "/x", "track": INJECTION_PAYLOAD, "text": "Item one"},
52+
{"href": "/y", "track": INJECTION_PAYLOAD, "text": "Item two"},
53+
],
54+
}
55+
html = str(render_template("lib/header_dropdown", props, track_prefix="Test"))
56+
links = BeautifulSoup(html, "lxml").find_all("a", href=True)
57+
menu_links = [a for a in links if a["href"] in ("/x", "/y")]
58+
assert len(menu_links) == 2
59+
for a in menu_links:
60+
assert a.get("onmouseover") is None
61+
assert a["data-ol-link-track"] == f"Test|{INJECTION_PAYLOAD}"
62+
63+
64+
def test_icon_link_pattern_blocks_attribute_injection(render_template, request_context_fixture):
65+
"""Covers the icon_link() pattern duplicated in modal_links.html and
66+
title_and_author.html -- see openlibrary/templates/tests/icon_link_pattern_check.html."""
67+
request_context_fixture(lang="en")
68+
html = str(render_template("tests/icon_link_pattern_check", ga_data=INJECTION_PAYLOAD))
69+
a = BeautifulSoup(html, "lxml").find("a")
70+
assert a.get("onmouseover") is None
71+
assert a["data-ol-link-track"] == INJECTION_PAYLOAD

0 commit comments

Comments
 (0)