Skip to content

playwright xss tests for renderMarkdownSafe#151

Open
clean6378-max-it wants to merge 2 commits into
masterfrom
test/browser-xss-execution
Open

playwright xss tests for renderMarkdownSafe#151
clean6378-max-it wants to merge 2 commits into
masterfrom
test/browser-xss-execution

Conversation

@clean6378-max-it

@clean6378-max-it clean6378-max-it commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Closes #142

The grep tests in test_xss_sanitization.py only prove the source still mentions DOMPurify. they do not run the frontend. this adds a headless chromium test that loads a real page (marked + dompurify + app.js from base.html), calls renderMarkdownSafe, and drops the result into innerHTML the same way workspace does.

Four payloads from the issue: img onerror, script tag, javascript: link markdown, svg onload. we set window.__xssProbe and check the dom sink for leftover onerror, script nodes, javascript: hrefs, and svg onload. production pages ship with csp now, so inline handlers often will not fire even if markup slips through. the assertions target stripped markup, not alert dialogs. there is a negative control that runs bare marked.parse without sanitize and expects dangerous attributes to still show up in the tree.

playwright 1.50.0 is in dev deps. ci gets a ubuntu-only browser-xss job: install chromium with deps, pytest tests/test_xss_browser.py, then the existing unittest grep backstop. pyinstaller steps stay on the unittest matrix job.

locally: pip install -e ".[dev]", playwright install chromium, pytest tests/test_xss_browser.py. comment out DOMPurify.sanitize in renderMarkdownSafe and the safe-path tests should fail.

Summary by CodeRabbit

  • Bug Fixes

    • Improved Markdown rendering protection by adding browser-based verification that common XSS payloads (scripts, event handlers, and dangerous links) are neutralized.
    • Ensures the “safe” rendering path blocks executable/unsafe DOM outcomes, with a negative-control check for comparison.
  • Tests

    • Added Playwright headless browser regression tests covering the production rendering flow.
    • Extended static XSS sanitization verification to reduce the risk of reintroducing unsafe Markdown parsing behavior.

@clean6378-max-it clean6378-max-it self-assigned this Jul 21, 2026
@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 253ae471-d64b-40c9-9291-d9e58229f781

📥 Commits

Reviewing files that changed from the base of the PR and between a830cce and c5c4f6a.

📒 Files selected for processing (1)
  • tests/test_xss_browser.py

📝 Walkthrough

Walkthrough

Adds Playwright browser regression tests for the production Markdown sanitization path, a bare marked.parse negative control, pinned Playwright configuration, and CI execution of browser and static XSS checks.

Changes

Browser XSS validation

Layer / File(s) Summary
Playwright harness and rendering setup
pyproject.toml, tests/test_xss_browser.py
Adds pinned Playwright configuration, a pytest marker, XSS vectors, Chromium and live-server fixtures, and page readiness checks for renderMarkdownSafe() and DOMPurify.
Sanitization and negative-control assertions
tests/test_xss_browser.py, tests/test_xss_sanitization.py
Tests sanitized rendering against script, event-handler, URI, and SVG vectors, verifies sanitizer invocation and dangerous artifacts, and confirms bare marked.parse() leaves dangerous output.
Continuous integration browser gate
.github/workflows/tests.yml
Adds an Ubuntu browser-xss job that installs Playwright and Chromium, runs browser tests, and executes static sanitization checks.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Pytest
  participant LiveServer
  participant BrowserPage
  participant renderMarkdownSafe
  participant DOMPurify
  Pytest->>LiveServer: start application
  BrowserPage->>LiveServer: navigate to root page
  BrowserPage->>renderMarkdownSafe: render XSS payload
  renderMarkdownSafe->>DOMPurify: sanitize rendered Markdown
  DOMPurify-->>BrowserPage: return sanitized DOM
  BrowserPage->>BrowserPage: assert no execution or dangerous artifacts
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed It concisely names the main change: Playwright-based XSS tests for renderMarkdownSafe.
Linked Issues check ✅ Passed It adds headless-browser coverage for the real render path, XSS vectors, a negative control, pinned Playwright, CI, and keeps static backstop tests.
Out of Scope Changes check ✅ Passed The changes stay focused on browser XSS verification, dependency pinning, CI wiring, and the existing sanitization backstop.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch test/browser-xss-execution

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/test_xss_browser.py`:
- Around line 53-57: Update the safe-render test branch around
renderMarkdownSafe to track or wrap DOMPurify.sanitize before rendering, then
assert that it was called at least once after processing the vectors. Keep the
existing renderMarkdownSafe invocation and assertions, while ensuring the test
fails if sanitization is removed or throws and the escaped-text fallback handles
the inputs instead.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 22dcdf94-0e2f-4b7d-a0c6-644f83f2cfa3

📥 Commits

Reviewing files that changed from the base of the PR and between 8e9ee40 and a830cce.

📒 Files selected for processing (4)
  • .github/workflows/tests.yml
  • pyproject.toml
  • tests/test_xss_browser.py
  • tests/test_xss_sanitization.py

Comment thread tests/test_xss_browser.py Outdated
Comment thread tests/test_xss_browser.py

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional data: URI vector (app.js mentions it)

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.

cppa-cursor-browser: browser-level test that a rendered payload does not execute

2 participants