Browser configs - only store settings the base engine can actually honour

A browser config could carry fields its engine ignores, and in two cases a *wrong*
value: saving a plain-HTTP-client variation wrote browser_type='chromium' (the
unrendered SelectField's own default) and delete_created_files=False (an unchecked,
unrendered BooleanField), because the form mapped every field regardless of what the
engine can honour.

The capability a field needs now rides on the field itself - _needs() wraps
Field(json_schema_extra={'capability': ...}) - so there is one declaration site
instead of a parallel name->flag table to keep in step. FetcherConfig.applicable_fields()
reads it back and drives BOTH halves: which fields _browser_config_fields.html renders,
and which ones FetcherConfig.from_submitted() accepts from a POST. So a field can no
longer be rendered-but-unsaveable or hidden-but-written-from-its-widget-default, and a
crafted POST cannot put a setting on a browser that ignores it.

Also rejects control characters in the per-profile User-Agent at the model, because that
value is written straight into an outbound request header. urllib3 would refuse it at
request time anyway, but now it can never be persisted in browsers.json at all.

Reads stay deliberately tolerant (pydantic extra='ignore' plus the coercion pass in
BrowserConfigStore.all()), so a browsers.json written by another version still loads.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

changedetectionio/blueprint/ui/browser_config.py

@@ -80,6 +80,13 @@ def _caps_for(base_name):
     return FetcherCapabilities.from_fetcher(getattr(content_fetchers, base_name, None)).model_dump()
 
 
+def _applicable_fields_for(capabilities):
+    """The FetcherConfig fields an engine with these capabilities may carry - the one set that
+    both the form template and the save path use, so they cannot drift apart."""
+    from changedetectionio.model.browser_config import FetcherConfig
+    return FetcherConfig.applicable_fields(capabilities)
+
+
 def _entry_to_formdata(entry):
     """Flatten a browsers.json entry into flat form field values (base is contextual, not a field)."""
     data = {'label': entry.get('label')}
@@ -127,11 +134,16 @@ def construct_blueprint(datastore: ChangeDetectionStore):
                 return True
         return False
 
-    def _validate_and_build_config(form):
-        """Return a validated FetcherConfig, or None (with errors attached to form)."""
+    def _validate_and_build_config(form, capabilities):
+        """Return a validated FetcherConfig, or None (with errors attached to form).
+
+        `capabilities` is the base engine's capability set and acts as the allowlist: only fields
+        that engine can honour are taken from the submitted form (FetcherConfig.from_submitted),
+        so nothing this browser ignores can be written into browsers.json.
+        """
         from changedetectionio.model.browser_config import FetcherConfig
         try:
-            return FetcherConfig(**form.to_fetcher_config_dict())
+            return FetcherConfig.from_submitted(form.to_fetcher_config_dict(), capabilities)
         except ValidationError as e:
             for err in e.errors():
                 loc = err['loc'][0] if err['loc'] else ''
@@ -167,7 +179,7 @@ def construct_blueprint(datastore: ChangeDetectionStore):
             if _label_is_taken(form.label.data):
                 form.label.errors.append(gettext("A browser with this name already exists"))
             else:
-                cfg = _validate_and_build_config(form)
+                cfg = _validate_and_build_config(form, caps)
                 if cfg is not None:
                     datastore.browser_config_store.add(
                         label=form.label.data,
@@ -179,7 +191,8 @@ def construct_blueprint(datastore: ChangeDetectionStore):
 
         locale_choices, timezone_choices = _autocomplete_choices()
         return render_template("browser-config-form.html", form=form, mode='add',
-                               base_fetcher=base_fetcher, base_label=base_label, caps=caps.model_dump(),
+                               base_fetcher=base_fetcher, base_label=base_label,
+                               applicable=_applicable_fields_for(caps),
                                locale_choices=locale_choices, timezone_choices=timezone_choices,
                                form_action=url_for('ui.browser_config.browser_config_add', base_fetcher=base_fetcher))
 
@@ -211,7 +224,7 @@ def construct_blueprint(datastore: ChangeDetectionStore):
                 if (not is_builtin) and _label_is_taken(form.label.data, exclude_id=config_id):
                     form.label.errors.append(gettext("A browser with this name already exists"))
                 else:
-                    cfg = _validate_and_build_config(form)
+                    cfg = _validate_and_build_config(form, _caps_for(base) if base else None)
                     if cfg is not None:
                         datastore.browser_config_store.upsert(
                             config_id,
@@ -228,7 +241,7 @@ def construct_blueprint(datastore: ChangeDetectionStore):
         return render_template("browser-config-form.html", form=form, mode='edit',
                                config_id=config_id, is_builtin=is_builtin,
                                base_fetcher=base, base_label=base_label,
-                               caps=_caps_for(base) if base else {},
+                               applicable=_applicable_fields_for(_caps_for(base) if base else None),
                                locale_choices=locale_choices, timezone_choices=timezone_choices,
                                form_action=url_for('ui.browser_config.browser_config_edit', config_id=config_id))
 

changedetectionio/blueprint/ui/templates/_browser_config_fields.html

@@ -1,7 +1,12 @@
-{# Browser config form fields for the add/edit page. Expects `form`, `caps` (the base engine's
-   capability dict) and `base_label` (read-only base engine name). The base engine is fixed by
-   the page (chosen via the "Add variation" link, or the existing config's base), so it's shown
-   read-only, not as a picker. Fields are gated server-side by `caps`. #}
+{# Browser config form fields for the add/edit page. Expects `form`, `applicable` (the set of
+   FetcherConfig fields this base engine can honour - FetcherConfig.applicable_fields()) and
+   `base_label` (read-only base engine name). The base engine is fixed by the page (chosen via
+   the "Add variation" link, or the existing config's base), so it's shown read-only, not as a
+   picker.
+
+   Gating on `applicable` rather than on capability flags directly is deliberate: it is the same
+   set the save path allows (FetcherConfig.from_submitted), so a field can never be rendered but
+   unsaveable, or hidden but written from its widget default. #}
 {% from '_helpers.html' import render_field, render_checkbox_field %}
 
 <fieldset>
@@ -13,18 +18,18 @@
         {{ render_field(form.label) }}
         <span class="pure-form-message-inline">{{ _('A friendly name, e.g. “Mobile” or “German desktop”.') }}</span>
     </div>
-    {% if caps.supports_browser_type %}
+    {% if 'browser_type' in applicable %}
     <div class="pure-control-group">
         {{ render_field(form.browser_type) }}
         <span class="pure-form-message-inline">{{ _('Which browser engine to launch locally.') }}</span>
     </div>
     {% endif %}
-    {% if caps.supports_delete_created_files %}
+    {% if 'delete_created_files' in applicable %}
     <div class="pure-control-group">{{ render_checkbox_field(form.delete_created_files) }}</div>
     {% endif %}
 </fieldset>
 
-{% if caps.supports_screenshots %}
+{% if 'viewport_width' in applicable %}
 <fieldset>
     <legend>{{ _('Screen size') }}</legend>
     <div class="pure-control-group">
@@ -59,16 +64,16 @@
 </fieldset>
 {% endif %}
 
-{% if caps.supports_request_timeout or caps.supports_custom_user_agent %}
+{% if 'timeout' in applicable or 'user_agent' in applicable %}
 <fieldset>
     <legend>{{ _('HTTP request options') }}</legend>
-    {% if caps.supports_request_timeout %}
+    {% if 'timeout' in applicable %}
     <div class="pure-control-group">
         {{ render_field(form.timeout) }}
         <span class="pure-form-message-inline">{{ _('Maximum seconds to wait for the response (plain HTTP client only).') }}</span>
     </div>
     {% endif %}
-    {% if caps.supports_custom_user_agent %}
+    {% if 'user_agent' in applicable %}
     <div class="pure-control-group">
         {{ render_field(form.user_agent) }}
         <span class="pure-form-message-inline">{{ _('Sent as the User-Agent header for requests using this browser.') }}</span>
@@ -77,7 +82,7 @@
 </fieldset>
 {% endif %}
 
-{% if caps.supports_request_blocking %}
+{% if 'block_resource_types' in applicable %}
 <fieldset>
     <legend>{{ _('Skip downloads (save bandwidth)') }}</legend>
     <div class="pure-control-group">
@@ -95,7 +100,7 @@
 </fieldset>
 {% endif %}
 
-{% if caps.supports_screenshots %}
+{% if 'screenshot_format' in applicable %}
 <fieldset>
     <div class="pure-control-group">{{ render_field(form.screenshot_format) }}</div>
 </fieldset>

changedetectionio/model/browser_config.py

@@ -26,7 +26,7 @@ orphans the watches that reference it.
 import os
 import uuid as uuid_builder
 from os import path
-from typing import List, Optional
+from typing import ClassVar, Dict, List, Optional
 
 from loguru import logger
 from pydantic import BaseModel, Field, ValidationError, field_validator
@@ -69,6 +69,19 @@ def _available_timezones():
         return _TZ_CACHE
 
 
+def _needs(capability, **field_kwargs):
+    """A FetcherConfig field only the engines with `capability` may carry.
+
+    The capability rides on the field itself rather than in a separate name->flag table, so
+    there is exactly one place to declare a field and no second list to keep in step. Read back
+    by FetcherConfig.applicable_fields(), which drives BOTH which fields the /browsers form
+    renders and which ones may be saved.
+    """
+    if 'default_factory' not in field_kwargs:
+        field_kwargs.setdefault('default', None)
+    return Field(json_schema_extra={'capability': capability}, **field_kwargs)
+
+
 class FetcherConfig(BaseModel):
     """Engine-agnostic per-instance browser behaviour.
 
@@ -79,29 +92,66 @@ class FetcherConfig(BaseModel):
     Keep every field optional with a sensible default.
     """
     # Rendering / device
-    viewport_width: Optional[int] = None       # px; None -> engine default
-    viewport_height: Optional[int] = None
+    viewport_width: Optional[int] = _needs('supports_screenshots')   # px; None -> engine default
+    viewport_height: Optional[int] = _needs('supports_screenshots')
     # Identity / locale
-    locale: Optional[str] = None               # e.g. 'de-DE' -> Accept-Language + navigator.language
-    timezone_id: Optional[str] = None          # e.g. 'Europe/Berlin'
+    locale: Optional[str] = _needs('supports_screenshots')           # e.g. 'de-DE' -> Accept-Language + navigator.language
+    timezone_id: Optional[str] = _needs('supports_screenshots')      # e.g. 'Europe/Berlin'
     # Screenshot
-    screenshot_format: str = 'JPEG'
+    screenshot_format: str = _needs('supports_screenshots', default='JPEG')
     # Cost / bandwidth - block assets (capability-gated by supports_request_blocking)
-    block_resource_types: List[str] = Field(default_factory=list)  # e.g. ['image', 'font', 'media']
-    block_url_patterns: List[str] = Field(default_factory=list)    # globs, e.g. ['*.ttf', '*/analytics/*']
+    block_resource_types: List[str] = _needs('supports_request_blocking', default_factory=list)  # e.g. ['image', 'font', 'media']
+    block_url_patterns: List[str] = _needs('supports_request_blocking', default_factory=list)    # globs, e.g. ['*.ttf', '*/analytics/*']
     # Local-launch engines only (capability-gated by supports_browser_type)
-    browser_type: Optional[str] = None         # 'chromium' | 'firefox' | 'webkit'
+    browser_type: Optional[str] = _needs('supports_browser_type')    # 'chromium' | 'firefox' | 'webkit'
     # Delete the per-fetch temp profile after use (capability-gated by supports_delete_created_files)
-    delete_created_files: bool = True
+    delete_created_files: bool = _needs('supports_delete_created_files', default=True)
     # timeout: plain HTTP client only (capability-gated by supports_request_timeout).
     # Defaults to DEFAULT_REQUEST_TIMEOUT_SECONDS (45s) so a fresh install / built-in html_requests
     # config has a sane, browser-like read timeout without relying on any global setting. An
     # existing install's previous settings.requests.timeout is carried onto its html_requests
     # config by update_35 (the migration hook), so upgrades keep whatever the user had.
-    timeout: Optional[int] = DEFAULT_REQUEST_TIMEOUT_SECONDS   # request timeout in seconds
+    timeout: Optional[int] = _needs('supports_request_timeout', default=DEFAULT_REQUEST_TIMEOUT_SECONDS)   # request timeout in seconds
     # user_agent: honoured by every engine (capability supports_custom_user_agent) via the
     # request_headers User-Agent channel.
-    user_agent: Optional[str] = None           # overrides the User-Agent header for this profile
+    user_agent: Optional[str] = _needs('supports_custom_user_agent') # overrides the User-Agent header for this profile
+
+    @classmethod
+    def applicable_fields(cls, capabilities):
+        """The field names an engine with these `capabilities` may carry.
+
+        `capabilities` is a FetcherCapabilities or its .model_dump() dict (the blueprint holds
+        one of each). A field declared without _needs() applies to every engine; an unknown/None
+        capability set yields only those, so a made-up base engine can never widen what is
+        storable.
+        """
+        def _has(flag):
+            if capabilities is None:
+                return False
+            if isinstance(capabilities, dict):
+                return bool(capabilities.get(flag))
+            return bool(getattr(capabilities, flag, False))
+
+        out = set()
+        for name, field in cls.model_fields.items():
+            extra = field.json_schema_extra if isinstance(field.json_schema_extra, dict) else {}
+            capability = extra.get('capability')
+            if capability is None or _has(capability):
+                out.add(name)
+        return out
+
+    @classmethod
+    def from_submitted(cls, data, capabilities):
+        """Build from untrusted input (a form POST), dropping every field this engine cannot
+        honour. The engine's capabilities are the allowlist, so a crafted POST - or an
+        unrendered field falling back to its own widget default - cannot put a setting on a
+        browser that ignores it.
+
+        Deliberately NOT used when loading browsers.json: reads stay tolerant so a file written
+        by another version still parses (see the module docstring).
+        """
+        allowed = cls.applicable_fields(capabilities)
+        return cls(**{k: v for k, v in (data or {}).items() if k in allowed})
 
     @field_validator('timeout')
     @classmethod
@@ -141,6 +191,17 @@ class FetcherConfig(BaseModel):
         """This profile's request timeout, else the caller's default (plain HTTP client only)."""
         return self.timeout or default
 
+    @field_validator('user_agent')
+    @classmethod
+    def _validate_user_agent(cls, v):
+        # This value is written straight into an outbound request header (apply_user_agent), so
+        # CR/LF or other control characters have no legitimate use here and are exactly what a
+        # header-splitting attempt looks like. The HTTP clients would reject them anyway; refusing
+        # at the model means it can never be persisted in browsers.json in the first place.
+        if v and any(ord(c) < 0x20 or ord(c) == 0x7f for c in v):
+            raise ValueError("User-Agent cannot contain control characters")
+        return v
+
     @field_validator('browser_type')
     @classmethod
     def _validate_browser_type(cls, v):

changedetectionio/tests/test_browser_config.py

@@ -691,3 +691,69 @@ def test_locked_browser_config_blocks_mutations(client, live_server, measure_mem
     # Clean up so the shared datastore doesn't leak this config into other tests
     monkeypatch.delenv('LOCKED_BROWSER_CONFIG', raising=False)
     datastore.browser_config_store.delete(cid)
+
+
+def test_config_only_stores_fields_the_engine_honours(client, live_server, measure_memory_usage, datastore_path):
+    """A browser config may only carry settings its base engine can actually honour.
+
+    Two ways junk used to reach browsers.json: an unrendered WTForms field falling back to its
+    own widget default (the plain HTTP client stored browser_type='chromium' and
+    delete_created_files=False, neither of which it honours), and a crafted POST naming fields
+    the form never rendered. FetcherConfig.applicable_fields() is the allowlist for both, and is
+    the same set the form template renders from.
+    """
+    datastore = client.application.config.get('DATASTORE')
+
+    # A variation of the plain HTTP client: no screenshots, no local launch
+    res = _add_browser(client, label="Fast plain client", base_fetcher="html_requests", timeout=30)
+    assert b"Fast plain client" in res.data
+
+    cid = next(c for c, e in datastore.browser_config_store.all().items()
+               if e.get('label') == "Fast plain client")
+    stored = datastore.browser_config_store.get(cid)['browser_config']
+    assert stored.get('timeout') == 30, "a field this engine honours is kept"
+    # The store re-expands every FetcherConfig key with its default on write, so what matters is
+    # that no inapplicable field carries a *value*: browser_type came back as 'chromium' (the
+    # SelectField default) and delete_created_files as False (unchecked box) before the filter.
+    assert stored.get('browser_type') is None
+    assert stored.get('delete_created_files') is True, "must stay at the model default, not the unrendered widget's"
+    for untouched in ('viewport_width', 'viewport_height', 'locale', 'timezone_id'):
+        assert stored.get(untouched) is None, f"{untouched} does not apply to html_requests"
+    assert not stored.get('block_resource_types')
+
+    # Same route, but the POST names browser-only fields the form never rendered for this engine
+    res = _add_browser(client, label="Crafted", base_fetcher="html_requests", timeout=30,
+                       viewport_width=1920, viewport_height=1080, locale='de-DE',
+                       browser_type='firefox')
+    assert b"Crafted" in res.data
+    cid = next(c for c, e in datastore.browser_config_store.all().items()
+               if e.get('label') == "Crafted")
+    stored = datastore.browser_config_store.get(cid)['browser_config']
+    assert stored.get('timeout') == 30
+    assert stored.get('viewport_width') is None and stored.get('browser_type') is None \
+        and stored.get('locale') is None
+
+    # And the fields a real browser DOES honour still save
+    res = _add_browser(client, label="German desktop", base_fetcher="html_webdriver",
+                       viewport_width=1920, viewport_height=1080, locale='de-DE')
+    assert b"German desktop" in res.data
+    cid = next(c for c, e in datastore.browser_config_store.all().items()
+               if e.get('label') == "German desktop")
+    stored = datastore.browser_config_store.get(cid)['browser_config']
+    assert stored.get('viewport_width') == 1920 and stored.get('locale') == 'de-DE'
+
+
+def test_user_agent_cannot_carry_control_characters(client, live_server, measure_memory_usage, datastore_path):
+    """The per-profile User-Agent lands in an outbound header, so CR/LF can't be stored."""
+    from pydantic import ValidationError
+    from changedetectionio.model.browser_config import FetcherConfig
+
+    with pytest.raises(ValidationError):
+        FetcherConfig(user_agent="Mozilla/5.0\r\nX-Injected: 1")
+
+    datastore = client.application.config.get('DATASTORE')
+    before = len(datastore.browser_config_store.all())
+    res = _add_browser(client, label="Header splitter", base_fetcher="html_webdriver",
+                       user_agent="Mozilla/5.0\r\nX-Injected: 1")
+    assert b"Header splitter" not in res.data or b"control characters" in res.data
+    assert len(datastore.browser_config_store.all()) == before, "must not have been saved"