diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index b43b4a2c..cb2185e2 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -11,7 +11,7 @@ name: Claude Code Review # the safe path. If you add steps here, preserve this invariant. on: pull_request_target: - types: [opened, synchronize, ready_for_review, reopened] + types: [opened, synchronize, ready_for_review, reopened, labeled] # Optional: Only run on specific file changes # paths: # - "src/**/*.ts" @@ -21,11 +21,13 @@ on: jobs: claude-review: - # Optional: Filter by PR author - # if: | - # github.event.pull_request.user.login == 'external-contributor' || - # github.event.pull_request.user.login == 'new-developer' || - # github.event.pull_request.author_association == 'FIRST_TIME_CONTRIBUTOR' + # Maintainers apply this label to explicitly approve privileged review on fork PRs. + if: | + github.event.pull_request.head.repo.full_name == github.repository || + github.event.pull_request.author_association == 'OWNER' || + github.event.pull_request.author_association == 'MEMBER' || + github.event.pull_request.author_association == 'COLLABORATOR' || + contains(github.event.pull_request.labels.*.name, 'safe-to-run-claude-review') runs-on: ubuntu-latest permissions: @@ -36,13 +38,13 @@ jobs: steps: - name: Checkout repository - uses: actions/checkout@v4 + uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 with: fetch-depth: 1 - name: Run Claude Code Review id: claude-review - uses: anthropics/claude-code-action@v1 + uses: anthropics/claude-code-action@51ea8ea73a139f2a74ff649e3092c25a904aed7e with: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} plugin_marketplaces: 'https://github.com/anthropics/claude-code.git' diff --git a/clients/desktop/tests/conftest.py b/clients/desktop/tests/conftest.py index a72e4eb8..48080789 100644 --- a/clients/desktop/tests/conftest.py +++ b/clients/desktop/tests/conftest.py @@ -1,10 +1,14 @@ import pytest -import wx @pytest.fixture(scope="session") def wx_app(): """Ensure a wx App exists for dialog tests.""" + try: + import wx + except ModuleNotFoundError: + pytest.skip("wxPython is not installed") + if not wx.App.IsDisplayAvailable(): pytest.skip("GUI display is not available for wx dialogs") app = wx.App(False) diff --git a/clients/desktop/tests/test_markdown_viewer_security.py b/clients/desktop/tests/test_markdown_viewer_security.py new file mode 100644 index 00000000..9dd55913 --- /dev/null +++ b/clients/desktop/tests/test_markdown_viewer_security.py @@ -0,0 +1,253 @@ +from importlib.util import module_from_spec, spec_from_file_location +from pathlib import Path +import re +import sys +import types + + +def _load_markdown_viewer_dialog_module(): + if "markdown" not in sys.modules: + markdown_module = types.ModuleType("markdown") + + def _markdown_to_html(text, extensions=None): + text = re.sub( + r"!\[([^\]]*)\]\(([^)]+)\)", + lambda match: f'{match.group(1)}', + text, + ) + text = re.sub( + r"\[([^\]]+)\]\(([^)]+)\)", + lambda match: f'{match.group(1)}', + text, + ) + blocks = [block.strip() for block in text.split("\n\n") if block.strip()] + html_blocks = [] + for block in blocks: + if block.startswith("<"): + html_blocks.append(block) + else: + html_blocks.append(f"

{block}

") + return "\n".join(html_blocks) + + markdown_module.markdown = _markdown_to_html + sys.modules["markdown"] = markdown_module + + if "nh3" not in sys.modules: + nh3_module = types.ModuleType("nh3") + + def _clean_html(html, tags=None, attributes=None, url_schemes=None, link_rel=None): + cleaned = re.sub(r".*?", "", html, flags=re.IGNORECASE | re.DOTALL) + cleaned = re.sub(r'\s+on\w+="[^"]*"', "", cleaned) + cleaned = re.sub(r'\s+on\w+=\'[^\']*\'', "", cleaned) + cleaned = re.sub(r'(href|src)="javascript:[^"]*"', "", cleaned, flags=re.IGNORECASE) + if link_rel: + cleaned = re.sub(r"]*\brel=)", f']*>", "", cleaned) + return cleaned + + nh3_module.clean = _clean_html + sys.modules["nh3"] = nh3_module + + wx_module = types.ModuleType("wx") + + class _Dialog: + def __init__(self, *args, **kwargs): + pass + + def Bind(self, *args, **kwargs): + pass + + def SetAcceleratorTable(self, *args, **kwargs): + pass + + def SetSize(self, *args, **kwargs): + pass + + def CenterOnScreen(self): + pass + + def EndModal(self, *args, **kwargs): + pass + + class _Panel: + def __init__(self, *args, **kwargs): + pass + + def SetSizer(self, *args, **kwargs): + pass + + class _BoxSizer: + def __init__(self, *args, **kwargs): + pass + + def Add(self, *args, **kwargs): + pass + + class _Button: + def __init__(self, *args, **kwargs): + pass + + def Bind(self, *args, **kwargs): + pass + + class _AcceleratorTable: + def __init__(self, *args, **kwargs): + pass + + class _AcceleratorEntry: + def __init__(self, *args, **kwargs): + pass + + class _Color: + def Red(self): + return 0 + + def Green(self): + return 0 + + def Blue(self): + return 0 + + class _SystemSettings: + @staticmethod + def GetColour(_color_id): + return _Color() + + wx_module.Dialog = _Dialog + wx_module.Panel = _Panel + wx_module.BoxSizer = _BoxSizer + wx_module.Button = _Button + wx_module.AcceleratorTable = _AcceleratorTable + wx_module.AcceleratorEntry = _AcceleratorEntry + wx_module.SystemSettings = _SystemSettings + wx_module.DEFAULT_DIALOG_STYLE = 0 + wx_module.RESIZE_BORDER = 0 + wx_module.VERTICAL = 0 + wx_module.EXPAND = 0 + wx_module.ALL = 0 + wx_module.ALIGN_RIGHT = 0 + wx_module.ID_CLOSE = 0 + wx_module.EVT_BUTTON = object() + wx_module.EVT_CLOSE = object() + wx_module.ACCEL_NORMAL = 0 + wx_module.WXK_ESCAPE = 27 + wx_module.SYS_COLOUR_WINDOW = 0 + wx_module.SYS_COLOUR_WINDOWTEXT = 1 + wx_module.SYS_COLOUR_GRAYTEXT = 2 + wx_module.SYS_COLOUR_HOTLIGHT = 3 + + wx_html2_module = types.ModuleType("wx.html2") + + class _WebView: + last_created = None + + def __init__(self): + self.bound_events = [] + self.set_page_calls = [] + + @staticmethod + def New(*args, **kwargs): + web_view = _WebView() + _WebView.last_created = web_view + return web_view + + def Bind(self, *args, **kwargs): + self.bound_events.append((args, kwargs)) + + def SetPage(self, html, base_url): + self.set_page_calls.append((html, base_url)) + + def SetFocus(self): + pass + + def RunScript(self, *args, **kwargs): + pass + + wx_html2_module.WebView = _WebView + wx_html2_module.EVT_WEBVIEW_LOADED = object() + wx_html2_module.EVT_WEBVIEW_NAVIGATING = object() + + wx_module.html2 = wx_html2_module + sys.modules["wx"] = wx_module + sys.modules["wx.html2"] = wx_html2_module + + module_path = Path(__file__).resolve().parents[1] / "ui" / "markdown_viewer_dialog.py" + spec = spec_from_file_location("test_markdown_viewer_dialog", module_path) + module = module_from_spec(spec) + assert spec is not None and spec.loader is not None + spec.loader.exec_module(module) + return module + + +markdown_viewer_dialog = _load_markdown_viewer_dialog_module() +MARKDOWN_DOCUMENT_BASE_URL = markdown_viewer_dialog.MARKDOWN_DOCUMENT_BASE_URL +MarkdownViewerDialog = markdown_viewer_dialog.MarkdownViewerDialog +sanitize_markdown = markdown_viewer_dialog.sanitize_markdown +should_allow_navigation = markdown_viewer_dialog.should_allow_navigation + + +def test_sanitize_markdown_strips_scripts_and_event_handlers(): + html = sanitize_markdown("[x](javascript:alert(1))\n\n\n\nok") + + assert "javascript:" not in html + assert " """ +MARKDOWN_DOCUMENT_BASE_URL = "playpalace://document/viewer" + + +def sanitize_markdown(markdown_content: str) -> str: + """Convert untrusted Markdown to sanitized HTML for document viewing.""" + html_body = markdown.markdown( + markdown_content, + extensions=_MD_EXTENSIONS, + ) + return nh3.clean( + html_body, + tags=_ALLOWED_TAGS, + attributes=_ALLOWED_ATTRIBUTES, + url_schemes=_ALLOWED_URL_SCHEMES, + link_rel="noopener noreferrer", + ) + + +def should_allow_navigation(url: str) -> bool: + """Allow only navigation that stays within the rendered document.""" + base_url, _fragment = urldefrag(url) + return base_url == MARKDOWN_DOCUMENT_BASE_URL + class MarkdownViewerDialog(wx.Dialog): """Modal dialog for viewing rendered Markdown content. @@ -154,20 +177,7 @@ def _create_ui(self, markdown_content): panel = wx.Panel(self) sizer = wx.BoxSizer(wx.VERTICAL) - # Convert markdown to HTML, then sanitize to prevent XSS. - # Documents are user-editable and transmitted over the network; - # python-markdown passes raw HTML through by default. - html_body = markdown.markdown( - markdown_content, - extensions=_MD_EXTENSIONS, - ) - html_body = nh3.clean( - html_body, - tags=_ALLOWED_TAGS, - attributes=_ALLOWED_ATTRIBUTES, - url_schemes=_ALLOWED_URL_SCHEMES, - link_rel="noopener noreferrer", - ) + html_body = sanitize_markdown(markdown_content) # Build full HTML page with system colours bg = _sys_color_hex(wx.SYS_COLOUR_WINDOW) @@ -203,7 +213,11 @@ def _create_ui(self, markdown_content): # WebView for rendered content self.web_view = wx.html2.WebView.New(panel) - self.web_view.SetPage(full_html, "") + self.web_view.Bind( + wx.html2.EVT_WEBVIEW_NAVIGATING, + self._on_webview_navigating, + ) + self.web_view.SetPage(full_html, MARKDOWN_DOCUMENT_BASE_URL) sizer.Add(self.web_view, 1, wx.EXPAND | wx.ALL, 5) # Close button @@ -234,6 +248,13 @@ def _on_webview_loaded(self, event): # Help screen readers pick up the content self.web_view.RunScript("document.body.focus();") + def _on_webview_navigating(self, event): + """Prevent untrusted document links from navigating the embedded browser.""" + if should_allow_navigation(event.GetURL()): + event.Skip() + return + event.Veto() + def _on_close(self, event): """Close the dialog.""" self.EndModal(wx.ID_CLOSE) diff --git a/clients/web/bootstrap.js b/clients/web/bootstrap.js new file mode 100644 index 00000000..e103b13a --- /dev/null +++ b/clients/web/bootstrap.js @@ -0,0 +1,35 @@ +(function () { + const cacheBust = Date.now(); + + function loadScript(src, { type = "text/javascript", optional = false } = {}) { + return new Promise((resolve, reject) => { + const script = document.createElement("script"); + script.src = src; + script.type = type; + script.onload = () => resolve(); + script.onerror = () => { + if (optional) { + resolve(); + } else { + reject(new Error(`Failed to load script: ${src}`)); + } + }; + document.body.appendChild(script); + }); + } + + (async () => { + await loadScript(`./version.js?${cacheBust}`, { optional: true }); + await loadScript(`./config.sample.js?${cacheBust}`, { optional: true }); + await loadScript(`./config.js?${cacheBust}`, { optional: true }); + await loadScript(`./libs/marked.min.js?${cacheBust}`, { optional: true }); + await loadScript(`./libs/purify.min.js?${cacheBust}`, { optional: true }); + + const version = String(window.PLAYPALACE_WEB_VERSION || "2026.02.08.1").trim(); + await loadScript(`./app.js?v=${encodeURIComponent(version)}&${cacheBust}`, { + type: "module", + }); + })().catch((error) => { + console.error(error); + }); +})(); diff --git a/clients/web/index.html b/clients/web/index.html index 260e8101..8b2bb7e8 100644 --- a/clients/web/index.html +++ b/clients/web/index.html @@ -5,6 +5,10 @@ PlayPalace Web Client +
@@ -156,46 +160,6 @@

Chat

- + diff --git a/clients/web/tests/markdown_viewer_security.test.mjs b/clients/web/tests/markdown_viewer_security.test.mjs new file mode 100644 index 00000000..38d755db --- /dev/null +++ b/clients/web/tests/markdown_viewer_security.test.mjs @@ -0,0 +1,117 @@ +import assert from "node:assert/strict"; +import test from "node:test"; +import { renderMarkdownHtml } from "../ui/markdown_viewer.js"; + +const fakeMarked = { + parse(markdown) { + return String(markdown) + .replace("SCRIPT", "") + .replace("IMG", '') + .replace("AUDIO", '') + .replace("VIDEO", '') + .replace("SOURCE", '') + .replace("BUTTON", "") + .replace("TEXTAREA", "") + .replace("SELECT", '') + .replace("DETAILS", "
openbody
") + .replace("DIALOG", "modal") + .replace("FIELDSET", '
') + .replace("BACKGROUND", '
x
') + .replace("JSURL", '
bad') + .replace("OKURL", 'ok'); + }, +}; + +const fakePurifier = { + sanitize(html, config) { + const expectedForbiddenAttrs = ["style", "srcset", "background"]; + const expectedForbiddenTags = [ + "img", + "svg", + "math", + "iframe", + "object", + "embed", + "form", + "style", + "audio", + "video", + "source", + "track", + "picture", + "input", + "button", + "link", + "meta", + "textarea", + "select", + "option", + "optgroup", + "details", + "summary", + "dialog", + "fieldset", + "label", + "datalist", + "legend", + "output", + ]; + + assert.deepEqual(config?.USE_PROFILES, { html: true }); + assert.equal(config?.ALLOWED_URI_REGEXP?.test("https://example.com"), true); + assert.equal(config?.ALLOWED_URI_REGEXP?.test("mailto:test@example.com"), true); + assert.equal(config?.ALLOWED_URI_REGEXP?.test("javascript:alert(1)"), false); + for (const attr of expectedForbiddenAttrs) { + assert.equal(config?.FORBID_ATTR?.includes(attr), true, `missing forbidden attr ${attr}`); + } + for (const tag of expectedForbiddenTags) { + assert.equal(config?.FORBID_TAGS?.includes(tag), true, `missing forbidden tag ${tag}`); + } + return html + .replace(//gi, "") + .replace(/]*>/gi, "") + .replace(//gi, "") + .replace(//gi, "") + .replace(/]*>/gi, "") + .replace(//gi, "") + .replace(//gi, "") + .replace(//gi, "") + .replace(//gi, "") + .replace(//gi, "") + .replace(//gi, "") + .replace(/\sbackground="[^"]*"/gi, "") + .replace(/href="javascript:[^"]*"/gi, ""); + }, +}; + +test("renderMarkdownHtml removes executable and remote-resource HTML", () => { + const html = renderMarkdownHtml( + "SCRIPT\nIMG\nAUDIO\nVIDEO\nSOURCE\nBUTTON\nTEXTAREA\nSELECT\nDETAILS\nDIALOG\nFIELDSET\nBACKGROUND\nJSURL\nOKURL", + fakeMarked, + fakePurifier, + ); + + assert.equal(html.includes(" { + const html = renderMarkdownHtml("", null, null); + + assert.equal(html.includes("