Skip to content

Fix: Sanitize text/html artifacts to prevent stored XSS (Related #5514) - #6813

Open
ParzivalHack wants to merge 3 commits into
google:mainfrom
ParzivalHack:fix-html-artifact-xss
Open

Fix: Sanitize text/html artifacts to prevent stored XSS (Related #5514)#6813
ParzivalHack wants to merge 3 commits into
google:mainfrom
ParzivalHack:fix-html-artifact-xss

Conversation

@ParzivalHack

Copy link
Copy Markdown

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/html artifacts by opening them in a new tab as a same-origin blob: 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 covers image/svg+xml; the text/html path is a distinct sink that remains unhandled.

Solution:
Sanitize text/html artifact payloads server-side before they are returned to the browser, mirroring the SVG sanitization approach in #5515. Add _sanitize_html_artifact() in api_server.py, which runs the artifact body through nh3.clean() to strip executable content (script tags, event-handler attributes, and javascript: URLs) while preserving safe markup, and apply it at the two artifact-serving endpoints (load_artifact_version and load_artifact). Add nh3>=0.3,<1 to the dependencies.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

Summary of passed pytest results:

  • tests/unittests/cli/test_html_sanitization.py8 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.py114 passed
  • tests/unittests/cli/test_fast_api.py90 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/html artifact with a simple JS alert, start adk web, open the dev UI, navigate to that session, and click "Preview in new tab" on the artifact, in the Artifacts tab.

  • Before the fix: the artifact preview executes embedded script in the dev server's origin.
  • After the fix: the artifact serving endpoints return sanitized HTML and the preview no longer executes script.

Verified locally with the same repro.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end.
  • Any dependent changes have been merged and published in downstream modules.

Additional context

This is the text/html counterpart of the SVG sanitization in PR #5515 (which fixes the image/svg+xml sink). Both share issue #5514. The fix reuses the same sanitizer library (nh3) already proposed in #5515 to avoid adding a second dependency.

Add a function to sanitize HTML artifacts before returning them.
Add unit tests for HTML sanitization to block XSS and preserve safe HTML.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants