From 33dbdc9be945bfc6d908eab5c0e58660708c0309 Mon Sep 17 00:00:00 2001 From: Chris Nestrud Date: Sat, 16 May 2026 14:30:26 -0500 Subject: [PATCH 01/12] Harden fork PR review workflow --- .github/workflows/claude-code-review.yml | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index b43b4a2c..a3eff291 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -21,11 +21,12 @@ 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' + 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 +37,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' From 67c38543d0ffae5d06085b757ab3aa6c927c10e4 Mon Sep 17 00:00:00 2001 From: Chris Nestrud Date: Sat, 16 May 2026 14:33:18 -0500 Subject: [PATCH 02/12] Trigger Claude review on label approval --- .github/workflows/claude-code-review.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index a3eff291..ce7d7904 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" @@ -26,6 +26,7 @@ jobs: github.event.pull_request.author_association == 'OWNER' || github.event.pull_request.author_association == 'MEMBER' || github.event.pull_request.author_association == 'COLLABORATOR' || + # Maintainers apply this label to explicitly approve privileged review on fork PRs. contains(github.event.pull_request.labels.*.name, 'safe-to-run-claude-review') runs-on: ubuntu-latest From 289a72379335993ed84af8067a782426594e02d6 Mon Sep 17 00:00:00 2001 From: Chris Nestrud Date: Sat, 16 May 2026 14:35:06 -0500 Subject: [PATCH 03/12] Fix Claude review gate syntax --- .github/workflows/claude-code-review.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index ce7d7904..cb2185e2 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -21,12 +21,12 @@ on: jobs: claude-review: + # 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' || - # Maintainers apply this label to explicitly approve privileged review on fork PRs. contains(github.event.pull_request.labels.*.name, 'safe-to-run-claude-review') runs-on: ubuntu-latest From 030261ca19a37601d4fca776be56a4caeedd2d8e Mon Sep 17 00:00:00 2001 From: Chris Nestrud Date: Sat, 16 May 2026 14:41:00 -0500 Subject: [PATCH 04/12] Harden desktop markdown viewer --- .../tests/test_markdown_viewer_security.py | 17 +++++++ clients/desktop/ui/markdown_viewer_dialog.py | 45 ++++++++++++------- 2 files changed, 46 insertions(+), 16 deletions(-) create mode 100644 clients/desktop/tests/test_markdown_viewer_security.py 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..b664d57f --- /dev/null +++ b/clients/desktop/tests/test_markdown_viewer_security.py @@ -0,0 +1,17 @@ +from ui.markdown_viewer_dialog import sanitize_markdown + + +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 " 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", + ) + + class MarkdownViewerDialog(wx.Dialog): """Modal dialog for viewing rendered Markdown content. @@ -146,6 +159,7 @@ def __init__(self, parent, title, markdown_content): style=wx.DEFAULT_DIALOG_STYLE | wx.RESIZE_BORDER, ) + self._allow_initial_load = True self._create_ui(markdown_content) self.SetSize(750, 550) self.CenterOnScreen() @@ -154,20 +168,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) @@ -221,6 +222,10 @@ def _create_ui(self, markdown_content): wx.html2.EVT_WEBVIEW_LOADED, self._on_webview_loaded, ) + self.web_view.Bind( + wx.html2.EVT_WEBVIEW_NAVIGATING, + self._on_webview_navigating, + ) # Escape key accelerator accel = wx.AcceleratorTable( @@ -234,6 +239,14 @@ 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 self._allow_initial_load: + self._allow_initial_load = False + event.Skip() + return + event.Veto() + def _on_close(self, event): """Close the dialog.""" self.EndModal(wx.ID_CLOSE) From 703094bdd2f17aace3903441e0ed74df9c409667 Mon Sep 17 00:00:00 2001 From: Chris Nestrud Date: Sat, 16 May 2026 14:46:55 -0500 Subject: [PATCH 05/12] Fix desktop markdown navigation guard --- .../tests/test_markdown_viewer_security.py | 126 +++++++++++++++++- clients/desktop/ui/markdown_viewer_dialog.py | 23 +++- 2 files changed, 141 insertions(+), 8 deletions(-) diff --git a/clients/desktop/tests/test_markdown_viewer_security.py b/clients/desktop/tests/test_markdown_viewer_security.py index b664d57f..af2a70de 100644 --- a/clients/desktop/tests/test_markdown_viewer_security.py +++ b/clients/desktop/tests/test_markdown_viewer_security.py @@ -1,4 +1,121 @@ -from ui.markdown_viewer_dialog import sanitize_markdown +from importlib.util import module_from_spec, spec_from_file_location +from pathlib import Path +import sys +import types + + +def _load_markdown_viewer_dialog_module(): + if "wx" not in sys.modules: + wx_module = types.ModuleType("wx") + + class _Dialog: + 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: + @staticmethod + def New(*args, **kwargs): + return _WebView() + + def Bind(self, *args, **kwargs): + pass + + def SetPage(self, *args, **kwargs): + pass + + 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() +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(): @@ -15,3 +132,10 @@ def test_sanitize_markdown_strips_remote_images(): assert " """ +_INTERNAL_WEBVIEW_URLS = {"", "about:blank"} + def sanitize_markdown(markdown_content: str) -> str: """Convert untrusted Markdown to sanitized HTML for document viewing.""" @@ -142,6 +144,13 @@ def sanitize_markdown(markdown_content: str) -> str: ) +def should_allow_navigation(url: str, document_loaded: bool) -> bool: + """Allow only the initial internal document load for the embedded WebView.""" + if document_loaded: + return False + return url in _INTERNAL_WEBVIEW_URLS + + class MarkdownViewerDialog(wx.Dialog): """Modal dialog for viewing rendered Markdown content. @@ -159,7 +168,7 @@ def __init__(self, parent, title, markdown_content): style=wx.DEFAULT_DIALOG_STYLE | wx.RESIZE_BORDER, ) - self._allow_initial_load = True + self._document_loaded = False self._create_ui(markdown_content) self.SetSize(750, 550) self.CenterOnScreen() @@ -204,6 +213,10 @@ def _create_ui(self, markdown_content): # WebView for rendered content self.web_view = wx.html2.WebView.New(panel) + self.web_view.Bind( + wx.html2.EVT_WEBVIEW_NAVIGATING, + self._on_webview_navigating, + ) self.web_view.SetPage(full_html, "") sizer.Add(self.web_view, 1, wx.EXPAND | wx.ALL, 5) @@ -222,10 +235,6 @@ def _create_ui(self, markdown_content): wx.html2.EVT_WEBVIEW_LOADED, self._on_webview_loaded, ) - self.web_view.Bind( - wx.html2.EVT_WEBVIEW_NAVIGATING, - self._on_webview_navigating, - ) # Escape key accelerator accel = wx.AcceleratorTable( @@ -235,14 +244,14 @@ def _create_ui(self, markdown_content): def _on_webview_loaded(self, event): """Set focus to the web view body once content is loaded.""" + self._document_loaded = True self.web_view.SetFocus() # 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 self._allow_initial_load: - self._allow_initial_load = False + if should_allow_navigation(event.GetURL(), self._document_loaded): event.Skip() return event.Veto() From a3a80effdcdd6c0ca89778b0bba66dcc184a38e7 Mon Sep 17 00:00:00 2001 From: Chris Nestrud Date: Sat, 16 May 2026 14:50:20 -0500 Subject: [PATCH 06/12] Stabilize desktop markdown document URL --- .../tests/test_markdown_viewer_security.py | 44 +++++++++++++++++-- clients/desktop/ui/markdown_viewer_dialog.py | 19 ++++---- 2 files changed, 49 insertions(+), 14 deletions(-) diff --git a/clients/desktop/tests/test_markdown_viewer_security.py b/clients/desktop/tests/test_markdown_viewer_security.py index af2a70de..a52d3335 100644 --- a/clients/desktop/tests/test_markdown_viewer_security.py +++ b/clients/desktop/tests/test_markdown_viewer_security.py @@ -114,6 +114,8 @@ def RunScript(self, *args, **kwargs): 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 @@ -134,8 +136,42 @@ def test_sanitize_markdown_strips_remote_images(): assert "tracker.example" not in html -def test_should_allow_navigation_only_for_initial_internal_load(): - assert should_allow_navigation("", document_loaded=False) - assert should_allow_navigation("about:blank", document_loaded=False) +def test_should_allow_navigation_allows_internal_document_url(): + assert should_allow_navigation(MARKDOWN_DOCUMENT_BASE_URL) + + +def test_should_allow_navigation_allows_internal_document_fragment(): + assert should_allow_navigation(f"{MARKDOWN_DOCUMENT_BASE_URL}#section-1") + + +def test_should_allow_navigation_blocks_external_url(): assert not should_allow_navigation("https://example.com", document_loaded=False) - assert not should_allow_navigation("https://example.com", document_loaded=True) + + +def test_on_webview_navigating_skips_internal_urls_and_vetoes_external_urls(): + dialog = MarkdownViewerDialog.__new__(MarkdownViewerDialog) + + class _Event: + def __init__(self, url): + self._url = url + self.skipped = False + self.vetoed = False + + def GetURL(self): + return self._url + + def Skip(self): + self.skipped = True + + def Veto(self): + self.vetoed = True + + internal_event = _Event(f"{MARKDOWN_DOCUMENT_BASE_URL}#top") + MarkdownViewerDialog._on_webview_navigating(dialog, internal_event) + assert internal_event.skipped + assert not internal_event.vetoed + + external_event = _Event("https://example.com") + MarkdownViewerDialog._on_webview_navigating(dialog, external_event) + assert external_event.vetoed + assert not external_event.skipped diff --git a/clients/desktop/ui/markdown_viewer_dialog.py b/clients/desktop/ui/markdown_viewer_dialog.py index 827a3a9d..84c8aae0 100644 --- a/clients/desktop/ui/markdown_viewer_dialog.py +++ b/clients/desktop/ui/markdown_viewer_dialog.py @@ -1,5 +1,7 @@ """Markdown viewer dialog for displaying rendered document content.""" +from urllib.parse import urldefrag + import wx import wx.html2 import markdown @@ -126,7 +128,7 @@ def _sys_color_hex(color_id): """ -_INTERNAL_WEBVIEW_URLS = {"", "about:blank"} +MARKDOWN_DOCUMENT_BASE_URL = "playpalace://document/viewer" def sanitize_markdown(markdown_content: str) -> str: @@ -144,11 +146,10 @@ def sanitize_markdown(markdown_content: str) -> str: ) -def should_allow_navigation(url: str, document_loaded: bool) -> bool: - """Allow only the initial internal document load for the embedded WebView.""" - if document_loaded: - return False - return url in _INTERNAL_WEBVIEW_URLS +def should_allow_navigation(url: str, document_loaded: bool | None = None) -> 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): @@ -168,7 +169,6 @@ def __init__(self, parent, title, markdown_content): style=wx.DEFAULT_DIALOG_STYLE | wx.RESIZE_BORDER, ) - self._document_loaded = False self._create_ui(markdown_content) self.SetSize(750, 550) self.CenterOnScreen() @@ -217,7 +217,7 @@ def _create_ui(self, markdown_content): wx.html2.EVT_WEBVIEW_NAVIGATING, self._on_webview_navigating, ) - self.web_view.SetPage(full_html, "") + self.web_view.SetPage(full_html, MARKDOWN_DOCUMENT_BASE_URL) sizer.Add(self.web_view, 1, wx.EXPAND | wx.ALL, 5) # Close button @@ -244,14 +244,13 @@ def _create_ui(self, markdown_content): def _on_webview_loaded(self, event): """Set focus to the web view body once content is loaded.""" - self._document_loaded = True self.web_view.SetFocus() # 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(), self._document_loaded): + if should_allow_navigation(event.GetURL()): event.Skip() return event.Veto() From 7e245acbc658aff987e93c8dd12e5b2c0007ec63 Mon Sep 17 00:00:00 2001 From: Chris Nestrud Date: Sat, 16 May 2026 14:54:14 -0500 Subject: [PATCH 07/12] Add desktop markdown SetPage regression --- .../tests/test_markdown_viewer_security.py | 232 ++++++++++-------- clients/desktop/ui/markdown_viewer_dialog.py | 2 +- 2 files changed, 133 insertions(+), 101 deletions(-) diff --git a/clients/desktop/tests/test_markdown_viewer_security.py b/clients/desktop/tests/test_markdown_viewer_security.py index a52d3335..a45f7f2b 100644 --- a/clients/desktop/tests/test_markdown_viewer_security.py +++ b/clients/desktop/tests/test_markdown_viewer_security.py @@ -5,105 +5,128 @@ def _load_markdown_viewer_dialog_module(): - if "wx" not in sys.modules: - wx_module = types.ModuleType("wx") - - class _Dialog: - 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: - @staticmethod - def New(*args, **kwargs): - return _WebView() - - def Bind(self, *args, **kwargs): - pass - - def SetPage(self, *args, **kwargs): - pass - - 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 + 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) @@ -145,7 +168,7 @@ def test_should_allow_navigation_allows_internal_document_fragment(): def test_should_allow_navigation_blocks_external_url(): - assert not should_allow_navigation("https://example.com", document_loaded=False) + assert not should_allow_navigation("https://example.com") def test_on_webview_navigating_skips_internal_urls_and_vetoes_external_urls(): @@ -175,3 +198,12 @@ def Veto(self): MarkdownViewerDialog._on_webview_navigating(dialog, external_event) assert external_event.vetoed assert not external_event.skipped + + +def test_create_ui_sets_page_with_explicit_internal_base_url(): + dialog = MarkdownViewerDialog(None, "Security", "hello") + + set_page_calls = dialog.web_view.set_page_calls + assert set_page_calls + _html, base_url = set_page_calls[-1] + assert base_url == MARKDOWN_DOCUMENT_BASE_URL diff --git a/clients/desktop/ui/markdown_viewer_dialog.py b/clients/desktop/ui/markdown_viewer_dialog.py index 84c8aae0..54894986 100644 --- a/clients/desktop/ui/markdown_viewer_dialog.py +++ b/clients/desktop/ui/markdown_viewer_dialog.py @@ -146,7 +146,7 @@ def sanitize_markdown(markdown_content: str) -> str: ) -def should_allow_navigation(url: str, document_loaded: bool | None = None) -> bool: +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 From ebddf3a2724f8bdf8e4a0003cb664d6a26b0b6c4 Mon Sep 17 00:00:00 2001 From: Chris Nestrud Date: Sat, 16 May 2026 14:58:13 -0500 Subject: [PATCH 08/12] Fix desktop markdown test collection --- clients/desktop/tests/conftest.py | 6 ++- .../tests/test_markdown_viewer_security.py | 44 +++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) 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 index a45f7f2b..9dd55913 100644 --- a/clients/desktop/tests/test_markdown_viewer_security.py +++ b/clients/desktop/tests/test_markdown_viewer_security.py @@ -1,10 +1,54 @@ 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: From be88e5685b245c26a51d95242fdfe339428f69cd Mon Sep 17 00:00:00 2001 From: Chris Nestrud Date: Sat, 16 May 2026 15:01:27 -0500 Subject: [PATCH 09/12] Harden web markdown rendering --- .../tests/markdown_viewer_security.test.mjs | 40 +++++++++++++++++ clients/web/ui/markdown_viewer.js | 43 +++++++++++-------- 2 files changed, 66 insertions(+), 17 deletions(-) create mode 100644 clients/web/tests/markdown_viewer_security.test.mjs 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..1c24ecc5 --- /dev/null +++ b/clients/web/tests/markdown_viewer_security.test.mjs @@ -0,0 +1,40 @@ +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("JSURL", 'bad') + .replace("OKURL", 'ok'); + }, +}; + +const fakePurifier = { + sanitize(html) { + return html + .replace(//gi, "") + .replace(/]*>/gi, "") + .replace(/href="javascript:[^"]*"/gi, ""); + }, +}; + +test("renderMarkdownHtml removes executable and remote-resource HTML", () => { + const html = renderMarkdownHtml("SCRIPT\nIMG\nJSURL\nOKURL", fakeMarked, fakePurifier); + + assert.equal(html.includes(" { + const html = renderMarkdownHtml("", null, null); + + assert.equal(html.includes("") .replace("IMG", '') + .replace("AUDIO", '') + .replace("VIDEO", '') + .replace("SOURCE", '') + .replace("BUTTON", "") .replace("JSURL", 'bad') .replace("OKURL", 'ok'); }, }; const fakePurifier = { - sanitize(html) { + sanitize(html, config) { + 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); + assert.equal(config?.FORBID_ATTR?.includes("style"), true); + assert.equal(config?.FORBID_ATTR?.includes("srcset"), true); + for (const tag of [ + "img", + "svg", + "math", + "iframe", + "object", + "embed", + "form", + "style", + "audio", + "video", + "source", + "track", + "picture", + "input", + "button", + "link", + "meta", + ]) { + 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(/href="javascript:[^"]*"/gi, ""); }, }; test("renderMarkdownHtml removes executable and remote-resource HTML", () => { - const html = renderMarkdownHtml("SCRIPT\nIMG\nJSURL\nOKURL", fakeMarked, fakePurifier); + const html = renderMarkdownHtml("SCRIPT\nIMG\nAUDIO\nVIDEO\nSOURCE\nBUTTON\nJSURL\nOKURL", fakeMarked, fakePurifier); assert.equal(html.includes(" Date: Sat, 16 May 2026 15:07:58 -0500 Subject: [PATCH 11/12] Complete web markdown denylist hardening --- .../tests/markdown_viewer_security.test.mjs | 56 ++++++++++++++++--- clients/web/ui/markdown_viewer.js | 14 ++++- 2 files changed, 60 insertions(+), 10 deletions(-) diff --git a/clients/web/tests/markdown_viewer_security.test.mjs b/clients/web/tests/markdown_viewer_security.test.mjs index d778855a..38d755db 100644 --- a/clients/web/tests/markdown_viewer_security.test.mjs +++ b/clients/web/tests/markdown_viewer_security.test.mjs @@ -11,6 +11,12 @@ const fakeMarked = { .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'); }, @@ -18,13 +24,8 @@ const fakeMarked = { const fakePurifier = { sanitize(html, config) { - 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); - assert.equal(config?.FORBID_ATTR?.includes("style"), true); - assert.equal(config?.FORBID_ATTR?.includes("srcset"), true); - for (const tag of [ + const expectedForbiddenAttrs = ["style", "srcset", "background"]; + const expectedForbiddenTags = [ "img", "svg", "math", @@ -42,7 +43,28 @@ const fakePurifier = { "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 @@ -52,12 +74,22 @@ const fakePurifier = { .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\nJSURL\nOKURL", fakeMarked, fakePurifier); + const html = renderMarkdownHtml( + "SCRIPT\nIMG\nAUDIO\nVIDEO\nSOURCE\nBUTTON\nTEXTAREA\nSELECT\nDETAILS\nDIALOG\nFIELDSET\nBACKGROUND\nJSURL\nOKURL", + fakeMarked, + fakePurifier, + ); assert.equal(html.includes(" { assert.equal(html.includes(" Date: Sat, 16 May 2026 15:10:31 -0500 Subject: [PATCH 12/12] Add web client CSP --- clients/web/bootstrap.js | 35 ++++++++++++++++++++++++++++++ clients/web/index.html | 46 +++++----------------------------------- 2 files changed, 40 insertions(+), 41 deletions(-) create mode 100644 clients/web/bootstrap.js 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

- +