fix(revisions): escape component/link_text/link_url in format_revision_list - #5445
Open
prasanna8585 wants to merge 1 commit into
Open
fix(revisions): escape component/link_text/link_url in format_revision_list#5445prasanna8585 wants to merge 1 commit into
prasanna8585 wants to merge 1 commit into
Conversation
…n_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 <script> tag, and a crafted link_url breaks out of the href attribute to inject an event handler attribute, with no HTML-escaping applied anywhere in the concatenation. Escapes all three values with html.escape(..., quote=True) when use_html is True, matching the pattern already used in show.py's fallback string. The use_html=False path (plain-text consumers) is left unescaped, since it was never HTML and escaping it would corrupt that output. Updates the two golden-file tests (chromium_expected_html.txt, clank_expected_html.txt), whose real Gitiles URLs contain legitimate '&' characters in their query strings (e.g. '?pretty=fuller&n=10000') that now correctly escape to '&' -- verified this is exact by reconstructing one golden-file entry and confirming byte-for-byte match against the real, patched function's output. Escaping '&' to '&' does not change what the link navigates to; browsers decode it back to '&' when following the href. Adds a dedicated FormatRevisionListTest covering: a malicious component name, a malicious link_text, a link_url attribute breakout, the legitimate-ampersand case above, the use_html=False path staying unescaped, and a normal non-malicious case rendering unchanged. Verification note: this sandbox could not fully bootstrap the project's test harness (the production dependency chain -- aiohttp, google-cloud-*, googleapiclient, etc. -- goes well beyond what's practical to stub in an isolated environment). All fix and test logic was instead verified by extracting the exact function source via ast.get_source_segment (not reimplemented) and executing it directly: confirmed the vulnerability against the pre-fix code, confirmed the fix closes it, confirmed the golden-file update is an exact byte-for-byte match, and confirmed all six new test assertions pass against the real, patched function. Recommend running `butler.py py_unittest -t core -p revisions_test.py` in a properly bootstrapped dev environment (via `local/install_deps.bash`) to additionally confirm the CI harness itself is clean before merging.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 <script> tag, and a crafted link_url breaks out of the href attribute to inject an event handler attribute, with no HTML-escaping applied anywhere in the concatenation.
Escapes all three values with html.escape(..., quote=True) when use_html is True, matching the pattern already used in show.py's fallback string. The use_html=False path (plain-text consumers) is left unescaped, since it was never HTML and escaping it would corrupt that output.
Updates the two golden-file tests (chromium_expected_html.txt, clank_expected_html.txt), whose real Gitiles URLs contain legitimate '&' characters in their query strings (e.g. '?pretty=fuller&n=10000') that now correctly escape to '&' -- verified this is exact by reconstructing one golden-file entry and confirming byte-for-byte match against the real, patched function's output. Escaping '&' to '&' does not change what the link navigates to; browsers decode it back to '&' when following the href.
Adds a dedicated FormatRevisionListTest covering: a malicious component name, a malicious link_text, a link_url attribute breakout, the legitimate-ampersand case above, the use_html=False path staying unescaped, and a normal non-malicious case rendering unchanged.
Verification note: this sandbox could not fully bootstrap the project's test harness (the production dependency chain -- aiohttp, google-cloud-*, googleapiclient, etc. -- goes well beyond what's practical to stub in an isolated environment). All fix and test logic was instead verified by extracting the exact function source via ast.get_source_segment (not reimplemented) and executing it directly: confirmed the vulnerability against the pre-fix code, confirmed the fix closes it, confirmed the golden-file update is an exact byte-for-byte match, and confirmed all six new test assertions pass against the real, patched function. Recommend running
butler.py py_unittest -t core -p revisions_test.pyin a properly bootstrapped dev environment (vialocal/install_deps.bash) to additionally confirm the CI harness itself is clean before merging.