Fix: Sanitize text/html artifacts to prevent stored XSS (Related #5514) - #6813
Open
ParzivalHack wants to merge 3 commits into
Open
Fix: Sanitize text/html artifacts to prevent stored XSS (Related #5514)#6813ParzivalHack wants to merge 3 commits into
ParzivalHack wants to merge 3 commits into
Conversation
Add a function to sanitize HTML artifacts before returning them.
Add unit tests for HTML sanitization to block XSS and preserve safe HTML.
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.
Link to Issue or Description of Change
I reported this unpatched variant of the Stored XSS, related to Issue #5514, yesterday to Google OSS VRP (https://issuetracker.google.com/issues/548415067), and even though its a duplicate, they still told me to open a PR here to fix it, and report back to them once merged.
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
Problem:
The ADK Web dev UI previews
text/htmlartifacts by opening them in a new tab as a same-originblob:document. The artifact body is served as an HTML document with no sanitization, so HTML artifacts can contain executable script that runs in the dev server's origin when previewed. The existing fix for this issue class (PR #5515) only coversimage/svg+xml; thetext/htmlpath is a distinct sink that remains unhandled.Solution:
Sanitize
text/htmlartifact payloads server-side before they are returned to the browser, mirroring the SVG sanitization approach in #5515. Add_sanitize_html_artifact()inapi_server.py, which runs the artifact body throughnh3.clean()to strip executable content (script tags, event-handler attributes, andjavascript:URLs) while preserving safe markup, and apply it at the two artifact-serving endpoints (load_artifact_versionandload_artifact). Addnh3>=0.3,<1to the dependencies.Testing Plan
Unit Tests:
Summary of passed
pytestresults:tests/unittests/cli/test_html_sanitization.py— 8 passed (XSS payloads are neutralized: script tags, event handlers,iframe,javascript:URLs; safe HTML is preserved; other MIME types pass through unchanged)tests/unittests/cli/test_dns_rebinding_protection.py,test_cors_regex.py,test_path_normalizer.py,test_adk_web_server_import_isolation.py,test_adk_web_server_tests.py— 114 passedtests/unittests/cli/test_fast_api.py— 90 passed; the remaining failures/errors are pre-existing and unrelated to this change (TracerProvider/fixture teardown and missing optional extras; verified identical on the unmodified baseline)Manual End-to-End (E2E) Tests:
Reproduction: create a session containing a
text/htmlartifact with a simple JS alert, startadk web, open the dev UI, navigate to that session, and click "Preview in new tab" on the artifact, in the Artifacts tab.Verified locally with the same repro.
Checklist
Additional context
This is the
text/htmlcounterpart of the SVG sanitization in PR #5515 (which fixes theimage/svg+xmlsink). Both share issue #5514. The fix reuses the same sanitizer library (nh3) already proposed in #5515 to avoid adding a second dependency.