From 7c7be6d24a5201abe25328c830fb31f021177408 Mon Sep 17 00:00:00 2001 From: prasanna8585 Date: Tue, 1 Sep 2026 23:21:02 +0530 Subject: [PATCH] fix(revisions): escape component/link_text/link_url in format_revision_list format_revision_list built HTML via raw string concatenation and .format() with zero escaping of component, link_text, and link_url -- the same untrusted-content boundary the recently-hardened deps_to_revisions_dict/get_component_revisions_dict already documents: these values originate from a DEPS file's own dependency keys and 'rev'/'url' string literals. The AST-based parser that replaced exec() there restricts what *operations* a DEPS file can perform, not what *content* its string literals hold, so an attacker who can shape a processed DEPS file's dependency name, 'rev', or 'url' fields can still place arbitrary text in these values. This function's output is returned directly by the happy path of _get_revision_range_html in show.py -- the same function whose empty fallback string was just escaped -- and is bound with inner-h-t-m-l on the testcase detail page. The happy path (component revisions found) was left unescaped by that fix. Confirmed with the real, unmodified function: a crafted component name reaches the page as a live ', + 'link_text': 'irrelevant', + }]) + self.assertNotIn('', result) + self.assertIn( + '"><script>alert(document.cookie)</script>', + result) + + def test_escapes_malicious_link_text(self): + """A crafted 'rev' value must not inject markup via link_text.""" + result = revisions.format_revision_list([{ + 'component': 'src/v8', + 'link_text': '">', + }]) + self.assertNotIn('', result) + self.assertIn('"><img src=x onerror=alert(1)>', result) + + def test_escapes_malicious_link_url_breakout(self): + """A crafted 'url' value must not be able to break out of the href + attribute it is placed in.""" + result = revisions.format_revision_list([{ + 'component': 'src/v8', + 'link_text': 'irrelevant', + 'link_url': 'https://x/" onmouseover="alert(1)', + }]) + self.assertNotIn('" onmouseover="alert(1)', result) + self.assertIn('" onmouseover="alert(1)', result) + + def test_escapes_ampersand_in_legitimate_url(self): + """A real query string, e.g. Gitiles' '?pretty=fuller&n=10000', must be + escaped to & -- browsers decode this correctly when following the + link, so this does not break legitimate URLs.""" + result = revisions.format_revision_list([{ + 'component': 'V8', + 'link_text': 'abc123:def456', + 'link_url': ('https://chromium.googlesource.com/v8/v8/+log/' + 'abc123..def456?pretty=fuller&n=10000'), + }]) + self.assertIn('pretty=fuller&n=10000', result) + self.assertNotIn('pretty=fuller&n=10000', result) + + def test_use_html_false_is_unaffected(self): + """The plain-text path (use_html=False) is for non-HTML consumers and + must not be escaped.""" + result = revisions.format_revision_list( + [{ + 'component': 'src/v8', + 'link_text': 'abc123', + }], use_html=False) + self.assertEqual(result, 'src/v8: abc123\n') + + def test_legitimate_case_still_renders_a_working_link(self): + """Confirms the fix does not break normal, non-malicious output.""" + result = revisions.format_revision_list([{ + 'component': 'src/v8', + 'link_text': 'abc123..def456', + 'link_url': 'https://chromium.googlesource.com/v8/v8/+log/abc123..def456', + }]) + self.assertEqual( + result, 'src/v8: ' + 'abc123..def456
') + + @test_utils.with_cloud_emulators('datastore') class RevisionsTestcase(unittest.TestCase): """Revisions tests."""