GHSA-56fq-63vj-9992-improved-URL-limitations

changedetectionio/blueprint/add_watch_ui/__init__.py

@@ -4,6 +4,7 @@ from loguru import logger
 from changedetectionio import forms
 from changedetectionio.auth_decorator import login_optionally_required
 from changedetectionio.store import ChangeDetectionStore
+from changedetectionio.validate_url import is_fetch_url_allowed
 
 
 def construct_blueprint(datastore: ChangeDetectionStore):
@@ -47,9 +48,18 @@ def construct_blueprint(datastore: ChangeDetectionStore):
         # Opportunistically sweep snapshots that were fetched but never saved.
         datastore.cleanup_temporary_watches()
 
+        # This endpoint makes the server-side browser fetch an arbitrary URL and returns the
+        # rendered result (screenshot + xpath element data) straight back in the HTTP response, so
+        # it is a direct SSRF-with-exfiltration primitive if left unvalidated. It used to only
+        # check startswith('http://', 'https://'), which skipped the private-IP gate AND the
+        # backslash/parser-differential rejection of GHSA-rph4-96w6-q594 (GHSA-56fq-63vj-9992).
+        # Note this fetch never reaches difference_detection_processor.call_browser(), so it gets
+        # no gating from there - it has to validate for itself.
         url = (request.args.get('url') or '').strip()
-        if not url or not url.lower().startswith(('http://', 'https://')):
-            return make_response('Please enter a valid http(s):// URL', 400)
+        ok, reason = is_fetch_url_allowed(url)
+        if not ok:
+            logger.warning(f"Add-watch snapshot: refused '{url}' - {reason}")
+            return make_response(reason, 400)
 
         # Use whatever fetcher the application is configured to use by default
         # (e.g. CloakBrowser, Playwright/sockpuppet) so the preview matches real checks.

changedetectionio/blueprint/browser_steps/__init__.py

@@ -18,6 +18,7 @@ import os
 
 from changedetectionio.store import ChangeDetectionStore
 from changedetectionio.flask_app import login_optionally_required
+from changedetectionio.validate_url import validate_fetch_url_async
 from loguru import logger
 
 browsersteps_sessions = {}
@@ -266,6 +267,13 @@ def construct_blueprint(datastore: ChangeDetectionStore):
         # Resolve the fetcher backend for this watch so we can ask it to launch its own browser
         # if it supports that (e.g. CloakBrowser, which runs locally rather than via CDP)
         watch = datastore.data['watching'][watch_uuid]
+
+        # Live preview sessions also return rendered screenshots to the caller and never pass
+        # through difference_detection_processor.call_browser(), so validate before we even spend
+        # a browser on it - otherwise a watch pointed at a private address is refused at real check
+        # time but happily previewed (and exfiltrated) here.
+        await validate_fetch_url_async(watch.link)
+
         fetcher_name = watch.get_fetch_backend or 'system'
         if fetcher_name == 'system':
             fetcher_name = datastore.data['settings']['application'].get('fetch_backend', 'html_requests')

changedetectionio/browser_steps/browser_steps.py

@@ -7,6 +7,7 @@ from loguru import logger
 from changedetectionio.content_fetchers import SCREENSHOT_MAX_HEIGHT_DEFAULT
 from changedetectionio.content_fetchers.base import manage_user_agent
 from changedetectionio.jinja2_custom import render as jinja_render
+from changedetectionio.validate_url import validate_fetch_url_async
 
 def browser_steps_get_valid_steps(browser_steps: list):
     if browser_steps is not None and len(browser_steps):
@@ -135,7 +136,15 @@ class steppable_browser_interface():
         if not value:
             logger.warning("No URL provided for goto_url action")
             return None
-            
+
+        # Every browser navigation we initiate funnels through here - the "Goto URL" step, the
+        # "Goto site" step, the live Browser Steps UI and the Add Watch snapshot preview - so this
+        # is the one place that has to enforce the fetch rules. Step values are plain user-supplied
+        # strings (forms.SingleBrowserStep.optional_value, or the browser_steps[] API field) and are
+        # NOT covered by the watch URL validation, which is what made file:///etc/passwd readable
+        # and private-IP SSRF possible via a browser step (GHSA-hm22-wg2m-35v4).
+        await validate_fetch_url_async(value)
+
         now = time.time()
         response = await self.page.goto(value, timeout=0, wait_until='load')
         logger.debug(f"Time to goto URL {time.time()-now:.2f}s")

changedetectionio/content_fetchers/base.py

@@ -219,7 +219,11 @@ class Fetcher():
                                                       optional_value=optional_value)
                     await self.screenshot_step(step_n)
                     await self.save_step_html(step_n)
-                except (Error, TimeoutError) as e:
+                except (Error, TimeoutError, ValueError) as e:
+                    # ValueError is what validate_fetch_url_async() raises when a step's URL is
+                    # refused (file://, private IP, bad scheme) - report it against the offending
+                    # step number like any other step failure, rather than failing the whole watch
+                    # with an opaque error.
                     logger.debug(str(e))
                     # Stop processing here
                     raise BrowserStepsStepException(step_n=step_n, original_e=e)

changedetectionio/content_fetchers/requests.py

@@ -9,7 +9,7 @@ import asyncio
 from changedetectionio import strtobool
 from changedetectionio.content_fetchers.exceptions import BrowserStepsInUnsupportedFetcher, EmptyReply, Non200ErrorCodeReceived
 from changedetectionio.content_fetchers.base import Fetcher
-from changedetectionio.validate_url import is_private_hostname, is_url_private_or_parser_confused
+from changedetectionio.validate_url import is_fetch_url_allowed, is_private_hostname, is_url_private_or_parser_confused
 
 
 # "html_requests" is listed as the default fetcher in store.py!
@@ -87,13 +87,12 @@ class fetcher(Fetcher):
 
         try:
             # Fresh DNS check at fetch time — catches DNS rebinding regardless of add-time cache.
-            # Validates every hostname both urlparse and urllib3 see, so parser-differential
-            # payloads (GHSA-rph4-96w6-q594) cannot smuggle an internal target past the gate.
-            if not allow_iana_restricted:
-                if is_url_private_or_parser_confused(url):
-                    raise Exception(f"Fetch blocked: '{url}' resolves to a private/reserved IP address "
-                                    f"or contains a parser-differential payload. "
-                                    f"Set ALLOW_IANA_RESTRICTED_ADDRESSES=true to allow.")
+            # Shared with every other fetch entry point, so the scheme allowlist and the
+            # parser-differential rejection (GHSA-rph4-96w6-q594) stay in step here too.
+            # Per-redirect-hop re-validation is done separately in the loop below.
+            ok, reason = is_fetch_url_allowed(url)
+            if not ok:
+                raise Exception(reason)
 
             r = session.request(method=request_method,
                                 data=request_body.encode('utf-8') if type(request_body) is str else request_body,

changedetectionio/processors/base.py

@@ -1,11 +1,8 @@
-import asyncio
-import re
 import hashlib
 
 from changedetectionio.browser_steps.browser_steps import browser_steps_get_valid_steps
 from changedetectionio.content_fetchers.base import Fetcher
-from changedetectionio.strtobool import strtobool
-from changedetectionio.validate_url import is_private_hostname, is_url_private_or_parser_confused
+from changedetectionio.validate_url import validate_fetch_url_async
 from copy import deepcopy
 from abc import abstractmethod
 import os
@@ -97,22 +94,19 @@ class difference_detection_processor():
             logger.warning(f"Failed to read checksum file for {self.watch_uuid}: {e}")
             self.last_raw_content_checksum = None
 
-    async def validate_iana_url(self):
-        """Pre-flight SSRF check — runs DNS lookup in executor to avoid blocking the event loop.
-        Covers all fetchers (requests, playwright, puppeteer, plugins) since every fetch goes
-        through call_browser().
+    async def validate_url_is_fetchable(self):
+        """Pre-flight fetch gate for the regular check path (all fetchers, since they all come
+        through call_browser()). The scheme/file:///private-IP rules live in
+        validate_url.is_fetch_url_allowed() so that the fetch paths which do NOT come through
+        here - the live Browser Steps UI, the Add Watch snapshot preview, and individual
+        'Goto URL' browser steps - enforce exactly the same rules.
         """
-        if strtobool(os.getenv('ALLOW_IANA_RESTRICTED_ADDRESSES', 'false')):
-            return
-        loop = asyncio.get_running_loop()
-        # Use the parser-agnostic check so urlparse/urllib3 differentials (GHSA-rph4-96w6-q594)
-        # can't slip a private/internal hostname past this pre-flight gate.
-        if await loop.run_in_executor(None, is_url_private_or_parser_confused, self.watch.link):
-            raise Exception(
-                f"Fetch blocked: '{self.watch.link}' resolves to a private/reserved IP address "
-                f"or contains a parser-differential payload. "
-                f"Set ALLOW_IANA_RESTRICTED_ADDRESSES=true to allow."
-            )
+        try:
+            await validate_fetch_url_async(self.watch.link)
+        except ValueError as e:
+            # Re-raised as a plain Exception so it lands in the watch's last_error like every other
+            # fetch failure, instead of looking like an internal type error.
+            raise Exception(str(e)) from e
 
     def _consume_preloaded_fetch(self):
         """One-shot: if the Add Watch page parked a freshly-fetched snapshot for this
@@ -178,14 +172,8 @@ class difference_detection_processor():
 
         url = self.watch.link
 
-        # Protect against file:, file:/, file:// access, check the real "link" without any meta "source:" etc prepended.
-        if re.search(r'^file:', url.strip(), re.IGNORECASE):
-            if not strtobool(os.getenv('ALLOW_FILE_URI', 'false')):
-                raise Exception(
-                    "file:// type access is denied for security reasons."
-                )
-
-        await self.validate_iana_url()
+        # Scheme allowlist (file:// etc), parser-differential and private/reserved IP checks.
+        await self.validate_url_is_fetchable()
 
         # Proxy ID "key"
         preferred_proxy_id = preferred_proxy_id if preferred_proxy_id else self.datastore.get_preferred_proxy_for_watch(

changedetectionio/validate_url.py

@@ -133,6 +133,106 @@ def is_url_private_or_parser_confused(url):
     return False
 
 
+def is_fetch_url_allowed(url):
+    """THE single gate for "is the server allowed to fetch this URL?".
+
+    Returns (ok: bool, reason: str) — `reason` is safe to show the user.
+
+    Call this from EVERY entry point that causes a server-side fetch. The checks used to live
+    inline in difference_detection_processor.call_browser(), on the documented assumption that
+    "every fetch goes through call_browser()". That stopped being true once the live Browser
+    Steps UI and the Add Watch snapshot preview grew their own fetch paths — each silently
+    skipped both the file:// and the private-IP gate (GHSA-hm22-wg2m-35v4, GHSA-56fq-63vj-9992).
+    Rather than re-assert that invariant, every fetch path now calls this function.
+
+    Layers, in order:
+      1. Render Jinja2 and strip the 'source:' meta prefix, so what gets checked is what the
+         browser/requests library will actually be handed. Stripping is load-bearing, not
+         cosmetic: urlparse('source:http://127.0.0.1/') reports NO hostname at all, so an
+         unstripped value sails straight past the private-IP check in step 5.
+      2. file:// refused unless ALLOW_FILE_URI=true. Checked explicitly rather than leaning on
+         is_safe_valid_url()'s scheme allowlist, because an operator who loosened
+         SAFE_PROTOCOL_REGEX for some other scheme should not silently get local file reads too.
+      3. Backslash rejection (GHSA-rph4-96w6-q594) — unconditional, including when the operator
+         has opted into private addresses.
+      4. is_safe_valid_url() — scheme allowlist, '<>' rejection, validators.url().
+      5. Private / loopback / link-local / reserved IP rejection, unless
+         ALLOW_IANA_RESTRICTED_ADDRESSES=true.
+
+    Step 5 performs DNS resolution and therefore blocks. From async code call
+    validate_fetch_url_async() instead so the event loop keeps turning.
+
+    Note this validates one URL, not a redirect chain. content_fetchers/requests.py follows
+    redirects manually and re-checks each hop; the Chromium-based fetchers cannot do that yet,
+    so an open redirect on a public host remains a known gap for those backends.
+    """
+    import os
+    import re
+    from changedetectionio.strtobool import strtobool
+    from changedetectionio.jinja2_custom import render as jinja_render
+
+    if not url or not isinstance(url, str) or not url.strip():
+        return False, "No URL specified."
+
+    url = url.strip()
+
+    # Jinja2 first — the fetch uses the rendered value, so the rendered value is what must pass.
+    if '{%' in url or '{{' in url:
+        try:
+            url = jinja_render(template_str=url).strip()
+        except Exception as e:
+            logger.error(f"URL '{url}' is not valid Jinja2? {str(e)}")
+            return False, "The URL contains invalid Jinja2 template syntax."
+
+    # 'source:' is our own meta prefix meaning "return the raw source"; it is not part of the
+    # URL that gets fetched. Must be removed before any hostname parsing happens - see step 1 above.
+    url = re.sub(r'^source:', '', url, flags=re.IGNORECASE).strip()
+
+    if re.match(r'^file:', url, re.IGNORECASE) and not strtobool(os.getenv('ALLOW_FILE_URI', 'false')):
+        logger.warning(f"Fetch blocked: file:// access is disabled (ALLOW_FILE_URI) - '{url}'")
+        return False, "file:// type access is denied for security reasons."
+
+    # Checked here in its own right, not left to is_safe_valid_url()/is_url_private_or_parser_confused():
+    # a backslash is never legitimate in a URL, so it must be refused even when the operator has
+    # opted into private addresses with ALLOW_IANA_RESTRICTED_ADDRESSES (GHSA-rph4-96w6-q594).
+    if '\\' in url:
+        logger.warning(f"Fetch blocked: '{url}' contains a backslash (parser-differential SSRF vector).")
+        return False, f"Fetch blocked: '{url}' contains a parser-differential payload (backslash)."
+
+    if not is_safe_valid_url(url):
+        return False, "The URL is invalid or uses an unsupported protocol."
+
+    if not strtobool(os.getenv('ALLOW_IANA_RESTRICTED_ADDRESSES', 'false')):
+        if is_url_private_or_parser_confused(url):
+            return False, (
+                f"Fetch blocked: '{url}' resolves to a private/reserved IP address "
+                f"or contains a parser-differential payload. "
+                f"Set ALLOW_IANA_RESTRICTED_ADDRESSES=true to allow."
+            )
+
+    return True, ''
+
+
+def validate_fetch_url(url):
+    """is_fetch_url_allowed() as an assertion - raises ValueError with the reason.
+
+    Use at fetch entry points that should abort loudly (the message surfaces to the user as a
+    watch error or an HTTP 400). Blocks on DNS; from async code use validate_fetch_url_async().
+    """
+    ok, reason = is_fetch_url_allowed(url)
+    if not ok:
+        raise ValueError(reason)
+
+
+async def validate_fetch_url_async(url):
+    """validate_fetch_url() with the DNS lookup pushed to a thread so the event loop isn't blocked."""
+    import asyncio
+    loop = asyncio.get_running_loop()
+    ok, reason = await loop.run_in_executor(None, is_fetch_url_allowed, url)
+    if not ok:
+        raise ValueError(reason)
+
+
 def is_llm_api_base_safe(api_base):
     """SSRF guard for the LLM `api_base` setting (GHSA-jrxm-qjfh-g54f).