diff --git a/changedetectionio/blueprint/__init__.py b/changedetectionio/blueprint/__init__.py index e69de29b..87c7deca 100644 --- a/changedetectionio/blueprint/__init__.py +++ b/changedetectionio/blueprint/__init__.py @@ -0,0 +1,20 @@ +from flask import make_response + + +def plaintext_response(message, status): + """ + Error response for a body that may contain caller-supplied text. + + Flask's make_response() defaults to Content-Type: text/html, so any user input echoed + into an error body becomes reflected XSS. That is the failure behind + GHSA-23mp-8222-96fr (/diff//download-patch), CVE-2026-27645 (/rss/watch/) and + CVE-2026-29038 (/rss/tag/) - three instances of one pattern. Forcing text/plain means + the browser will not parse the body as markup even if input does reach it. + + Prefer validating input over relying on this, but use it on error paths regardless: + exception text routinely carries values nobody audited (selectors, timestamps, + filesystem paths from snapshot reads). + """ + response = make_response(message, status) + response.headers['Content-Type'] = 'text/plain; charset=utf-8' + return response diff --git a/changedetectionio/blueprint/browser_steps/__init__.py b/changedetectionio/blueprint/browser_steps/__init__.py index 07dbf589..a8264f60 100644 --- a/changedetectionio/blueprint/browser_steps/__init__.py +++ b/changedetectionio/blueprint/browser_steps/__init__.py @@ -17,6 +17,7 @@ from flask import Blueprint, request, make_response import os from changedetectionio.store import ChangeDetectionStore +from changedetectionio.blueprint import plaintext_response from changedetectionio.flask_app import login_optionally_required from changedetectionio.validate_url import validate_fetch_url_async from loguru import logger @@ -400,8 +401,10 @@ def construct_blueprint(datastore: ChangeDetectionStore): except Exception as e: logger.error(f"Exception when calling step operation {step_operation} {str(e)}") - # Try to find something of value to give back to the user - return make_response(str(e).splitlines()[0], 401) + # Try to find something of value to give back to the user. + # text/plain: the message can contain the user's own selectors/values, so it + # must not be parsed as HTML by the browser (GHSA-23mp-8222-96fr pattern). + return plaintext_response(str(e).splitlines()[0], 401) # Screenshots and other info only needed on requesting a step (POST) try: @@ -418,7 +421,7 @@ def construct_blueprint(datastore: ChangeDetectionStore): watch.save_xpath_data(data=xpath_data) except Exception as e: - return make_response(f"Error fetching screenshot and element data - {str(e)}", 401) + return plaintext_response(f"Error fetching screenshot and element data - {str(e)}", 401) # SEND THIS BACK TO THE BROWSER output = { diff --git a/changedetectionio/blueprint/ui/diff.py b/changedetectionio/blueprint/ui/diff.py index 26218bbf..e7133c24 100644 --- a/changedetectionio/blueprint/ui/diff.py +++ b/changedetectionio/blueprint/ui/diff.py @@ -14,6 +14,7 @@ from changedetectionio.diff import ( CHANGED_INTO_PLACEMARKER_OPEN, CHANGED_INTO_PLACEMARKER_CLOSED ) from changedetectionio.store import ChangeDetectionStore +from changedetectionio.blueprint import plaintext_response from changedetectionio.auth_decorator import login_optionally_required @@ -516,20 +517,36 @@ def construct_blueprint(datastore: ChangeDetectionStore): try: watch = datastore.data['watching'][uuid] except KeyError: - return make_response('Watch not found', 404) + return plaintext_response('Watch not found', 404) dates = list(watch.history.keys()) if len(dates) < 2: - return make_response('Not enough history', 400) + return plaintext_response('Not enough history', 400) from_version = request.args.get('from_version', dates[-2]) to_version = request.args.get('to_version', dates[-1]) + # Validate before use. get_history_snapshot() does a plain dict lookup, so an + # unknown key raises KeyError whose str() carries the raw query parameter - which + # the error path below used to reflect into a text/html response, giving reflected + # XSS (GHSA-23mp-8222-96fr). This mirrors the check the API resource already + # performs for the same operation in api/Watch.py. + for timestamp in (from_version, to_version): + if timestamp not in watch.history: + return plaintext_response('Snapshot not found', 404) + try: from_text = watch.get_history_snapshot(timestamp=from_version) to_text = watch.get_history_snapshot(timestamp=to_version) except Exception as e: - return make_response(f'Could not read snapshots: {e}', 500) + # Never reflect the exception text. Besides the KeyError guarded above, this + # also catches brotli decode failures and the data-dir containment guard in + # get_history_snapshot(), whose messages carry filesystem paths. Send the + # detail to the log and keep the response text/plain regardless, matching the + # pattern already applied to rss/single_watch.py, so nothing that reaches the + # body can be parsed as HTML by the browser. + logger.error(f"download-patch: could not read snapshots for watch {uuid}: {e}") + return plaintext_response('Could not read snapshots', 500) diff_lines = list(difflib.unified_diff( from_text.splitlines(keepends=True), diff --git a/changedetectionio/tests/test_diff_download_patch_xss.py b/changedetectionio/tests/test_diff_download_patch_xss.py new file mode 100644 index 00000000..fd561ad0 --- /dev/null +++ b/changedetectionio/tests/test_diff_download_patch_xss.py @@ -0,0 +1,86 @@ +#!/usr/bin/env python3 +""" +Reflected XSS regression tests for /diff//download-patch (GHSA-23mp-8222-96fr). + +get_history_snapshot() does a plain dict lookup on the history, so an unknown timestamp +raised KeyError whose str() carries the raw from_version/to_version query parameter. The +error path interpolated that into make_response(), which defaults to text/html - so the +parameter was reflected unescaped and executed in the victim's browser. + +Two independent guarantees are asserted here, because either alone would have prevented +this bug and both are worth keeping: + 1. Unknown timestamps are rejected up front (404), so the exception never fires. + 2. Whatever the error path returns is text/plain and never echoes the input. + +This is the third instance of the same pattern, after CVE-2026-27645 (/rss/watch/) and +CVE-2026-29038 (/rss/tag/). +""" + +import os +from flask import url_for +from .util import wait_for_all_checks + +XSS_PAYLOADS = [ + '', + '">', + "'>", +] + + +def _setup_watch_with_history(client, datastore_path): + """A watch needs >=2 snapshots before download-patch will get as far as the lookup.""" + with open(os.path.join(datastore_path, "endpoint-content.txt"), "w") as f: + f.write("first content") + + test_url = url_for('test_endpoint', _external=True) + uuid = client.application.config.get('DATASTORE').add_watch(url=test_url) + client.post(url_for("ui.form_watch_checknow"), follow_redirects=True) + wait_for_all_checks(client) + + with open(os.path.join(datastore_path, "endpoint-content.txt"), "w") as f: + f.write("second content, now different") + client.post(url_for("ui.form_watch_checknow"), follow_redirects=True) + wait_for_all_checks(client) + + return uuid + + +def test_download_patch_does_not_reflect_unknown_timestamp(client, live_server, measure_memory_usage, datastore_path): + uuid = _setup_watch_with_history(client, datastore_path) + + for payload in XSS_PAYLOADS: + # from_version and to_version are separate code paths; both must be validated + for param in ('from_version', 'to_version'): + res = client.get(url_for("ui.ui_diff.download_patch", uuid=uuid, **{param: payload})) + + assert payload.encode() not in res.data, \ + f"{param}={payload!r} was reflected into the response body" + + # An unknown timestamp is a client error, not a 500 from an unhandled KeyError + assert res.status_code == 404, \ + f"{param}={payload!r} should be rejected up front, got {res.status_code}" + + # Even if a future refactor lets the exception path run again, the body must + # not be parseable as HTML + if res.status_code >= 400: + assert 'text/html' not in res.headers.get('Content-Type', ''), \ + f"error response for {param}={payload!r} should not be text/html" + + +def test_download_patch_still_works_for_valid_timestamps(client, live_server, measure_memory_usage, datastore_path): + """The validation must not break the legitimate path.""" + uuid = _setup_watch_with_history(client, datastore_path) + watch = client.application.config.get('DATASTORE').data['watching'][uuid] + dates = list(watch.history.keys()) + assert len(dates) >= 2 + + res = client.get(url_for("ui.ui_diff.download_patch", uuid=uuid, + from_version=dates[-2], to_version=dates[-1])) + assert res.status_code == 200 + assert 'text/plain' in res.headers.get('Content-Type', '') + assert b'second content' in res.data, "Patch should contain the changed text" + + # And with no params at all it should default to the last two snapshots + res = client.get(url_for("ui.ui_diff.download_patch", uuid=uuid)) + assert res.status_code == 200 + assert b'second content' in res.data