Skip to content

Commit 7dcb2c0

Browse files
committed
Fix review findings: sitemap Content-Type and search DOM injection
- P2: the FastAPI catch-all wrapped every route() response in HTMLResponse, discarding AppResponse.headers — deployed /sitemap.xml shipped as text/html. The catch-all now preserves headers when the route sets them. Regression tests exercise the handler through the stubbed-main harness (extracted MainModuleHarness) and fail when the fix is reverted. - P3: search results were rendered via innerHTML with interpolated catalog fields. Result nodes are now built with createElement/ textContent and replaceChildren, the slug is URI-encoded in the href, and the ranking check asserts search.js contains no innerHTML use. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012tBECRGRPmM2dj5b3FFJGo
1 parent 6060c96 commit 7dcb2c0

7 files changed

Lines changed: 108 additions & 15 deletions

File tree

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,24 +50,41 @@ function wireSearch() {
5050

5151
function hideResults() {
5252
results.hidden = true;
53-
results.innerHTML = '';
53+
results.replaceChildren();
54+
}
55+
56+
// Result nodes are built with textContent so catalog fields can never
57+
// be parsed as markup.
58+
function resultNode(entry) {
59+
const item = document.createElement('li');
60+
const link = document.createElement('a');
61+
link.href = `/examples/${encodeURIComponent(entry.slug)}`;
62+
const title = document.createElement('strong');
63+
title.textContent = entry.title;
64+
const meta = document.createElement('span');
65+
meta.className = 'meta';
66+
meta.textContent = ` · ${entry.section}`;
67+
link.append(title, meta);
68+
item.appendChild(link);
69+
return item;
5470
}
5571

5672
function renderResults() {
5773
if (!entries) return;
5874
const matches = rankExamples(input.value, entries);
5975
if (!matches.length) {
6076
if (input.value.trim()) {
61-
results.innerHTML = '<li class="search-empty">No matching examples.</li>';
77+
const empty = document.createElement('li');
78+
empty.className = 'search-empty';
79+
empty.textContent = 'No matching examples.';
80+
results.replaceChildren(empty);
6281
results.hidden = false;
6382
} else {
6483
hideResults();
6584
}
6685
return;
6786
}
68-
results.innerHTML = matches
69-
.map((entry) => `<li><a href="/examples/${entry.slug}"><strong>${entry.title}</strong><span class="meta"> · ${entry.section}</span></a></li>`)
70-
.join('');
87+
results.replaceChildren(...matches.map(resultNode));
7188
results.hidden = false;
7289
}
7390

public/search.js

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,24 +50,41 @@ function wireSearch() {
5050

5151
function hideResults() {
5252
results.hidden = true;
53-
results.innerHTML = '';
53+
results.replaceChildren();
54+
}
55+
56+
// Result nodes are built with textContent so catalog fields can never
57+
// be parsed as markup.
58+
function resultNode(entry) {
59+
const item = document.createElement('li');
60+
const link = document.createElement('a');
61+
link.href = `/examples/${encodeURIComponent(entry.slug)}`;
62+
const title = document.createElement('strong');
63+
title.textContent = entry.title;
64+
const meta = document.createElement('span');
65+
meta.className = 'meta';
66+
meta.textContent = ` · ${entry.section}`;
67+
link.append(title, meta);
68+
item.appendChild(link);
69+
return item;
5470
}
5571

5672
function renderResults() {
5773
if (!entries) return;
5874
const matches = rankExamples(input.value, entries);
5975
if (!matches.length) {
6076
if (input.value.trim()) {
61-
results.innerHTML = '<li class="search-empty">No matching examples.</li>';
77+
const empty = document.createElement('li');
78+
empty.className = 'search-empty';
79+
empty.textContent = 'No matching examples.';
80+
results.replaceChildren(empty);
6281
results.hidden = false;
6382
} else {
6483
hideResults();
6584
}
6685
return;
6786
}
68-
results.innerHTML = matches
69-
.map((entry) => `<li><a href="/examples/${entry.slug}"><strong>${entry.title}</strong><span class="meta"> · ${entry.section}</span></a></li>`)
70-
.join('');
87+
results.replaceChildren(...matches.map(resultNode));
7188
results.hidden = false;
7289
}
7390

scripts/check_search_ranking.mjs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,11 @@ function expect(condition, message) {
1414
if (!condition) failures.push(message);
1515
}
1616

17+
// Result nodes must be built with textContent, never innerHTML, so
18+
// catalog fields can never be parsed as markup (DOM injection guard).
19+
const searchSource = readFileSync(path.join(root, 'public', 'search.js'), 'utf8');
20+
expect(!searchSource.includes('innerHTML'), 'search.js must not use innerHTML');
21+
1722
const closures = rankExamples('closures', entries);
1823
expect(closures[0]?.slug === 'closures', `exact title should rank first, got ${closures[0]?.slug}`);
1924

src/asset_manifest.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
11
# Generated by scripts/fingerprint_assets.py. Do not edit by hand.
2-
ASSET_PATHS = {'SITE_CSS': '/site.8804b14e63bc.css', 'SYNTAX_JS': '/syntax-highlight.305edd86ffcd.js', 'EDITOR_JS': '/editor.bbf94cd1abda.js', 'SEARCH_JS': '/search.7f410703da25.js', 'SEARCH_INDEX': '/search-index.f08e8599474a.json'}
3-
HTML_CACHE_VERSION = '43f8ba99437d'
2+
ASSET_PATHS = {'SITE_CSS': '/site.8804b14e63bc.css', 'SYNTAX_JS': '/syntax-highlight.305edd86ffcd.js', 'EDITOR_JS': '/editor.bbf94cd1abda.js', 'SEARCH_JS': '/search.ab0effeac6ce.js', 'SEARCH_INDEX': '/search-index.f08e8599474a.json'}
3+
HTML_CACHE_VERSION = '1a58594ea2d2'

src/main.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -390,6 +390,10 @@ def provide_worker_code():
390390
@app.get("/{path:path}")
391391
async def not_found(path: str, request: Request):
392392
response = route(str(request.url), method="GET")
393+
# Non-HTML routes (such as /sitemap.xml) set their own Content-Type;
394+
# wrapping them in HTMLResponse would discard it.
395+
if response.headers:
396+
return Response(response.body, status_code=response.status, headers=response.headers)
393397
return _html(response.body, response.status)
394398

395399

tests/test_main_observability.py

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,13 @@
1010
SRC = ROOT / "src"
1111

1212

13-
class MainObservabilityTests(unittest.TestCase):
13+
class MainModuleHarness(unittest.TestCase):
14+
"""Imports src/main.py with stubbed FastAPI/Workers modules.
15+
16+
Subclass this to test main.py handlers directly; the stub Response
17+
classes record content, status, and headers for assertions.
18+
"""
19+
1420
def setUp(self):
1521
self.saved_modules = {
1622
name: sys.modules.get(name)
@@ -83,8 +89,11 @@ class RequestValidationError(Exception):
8389
fastapi_responses = types.ModuleType("fastapi.responses")
8490

8591
class Response:
86-
def __init__(self, *args, **kwargs):
87-
pass
92+
def __init__(self, content=None, status_code=200, headers=None, media_type=None, **kwargs):
93+
self.content = content
94+
self.status_code = status_code
95+
self.headers = headers or {}
96+
self.media_type = media_type
8897

8998
class HTMLResponse(Response):
9099
pass
@@ -129,6 +138,8 @@ def import_main(self):
129138
sys.modules.pop("main", None)
130139
return importlib.import_module("main")
131140

141+
142+
class MainObservabilityTests(MainModuleHarness):
132143
def test_run_example_marks_dynamic_worker_http_500_as_worker_error(self):
133144
main = self.import_main()
134145
destroyed = []

tests/test_main_routes.py

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
import asyncio
2+
from types import SimpleNamespace
3+
4+
from tests.test_main_observability import MainModuleHarness
5+
6+
7+
class CatchAllResponseHeaderTests(MainModuleHarness):
8+
"""The FastAPI catch-all must preserve route()'s response headers.
9+
10+
/sitemap.xml is served through the catch-all; wrapping every
11+
response in HTMLResponse would discard its XML Content-Type.
12+
"""
13+
14+
def request(self, url):
15+
return SimpleNamespace(url=url)
16+
17+
def test_sitemap_keeps_xml_content_type_through_the_catch_all(self):
18+
main = self.import_main()
19+
response = asyncio.run(
20+
main.not_found("sitemap.xml", self.request("https://example.test/sitemap.xml"))
21+
)
22+
self.assertEqual(response.headers["Content-Type"], "application/xml; charset=utf-8")
23+
self.assertIn("<urlset", response.content)
24+
self.assertEqual(response.status_code, 200)
25+
26+
def test_journey_pages_stay_html_through_the_catch_all(self):
27+
main = self.import_main()
28+
response = asyncio.run(
29+
main.not_found("journeys", self.request("https://example.test/journeys"))
30+
)
31+
self.assertTrue(response.headers["Content-Type"].startswith("text/html"))
32+
self.assertEqual(response.status_code, 200)
33+
34+
def test_unknown_paths_fall_back_to_html(self):
35+
main = self.import_main()
36+
response = asyncio.run(
37+
main.not_found("no-such-page", self.request("https://example.test/no-such-page"))
38+
)
39+
self.assertEqual(response.status_code, 404)

0 commit comments

Comments
 (0)