diff --git a/changedetectionio/blueprint/ui/browser_config.py b/changedetectionio/blueprint/ui/browser_config.py index 09c8925f3..e8199ae4a 100644 --- a/changedetectionio/blueprint/ui/browser_config.py +++ b/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)) diff --git a/changedetectionio/blueprint/ui/templates/_browser_config_fields.html b/changedetectionio/blueprint/ui/templates/_browser_config_fields.html index f5e31cf43..c1f53b4fe 100644 --- a/changedetectionio/blueprint/ui/templates/_browser_config_fields.html +++ b/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 %}
@@ -13,18 +18,18 @@ {{ render_field(form.label) }} {{ _('A friendly name, e.g. “Mobile” or “German desktop”.') }} - {% if caps.supports_browser_type %} + {% if 'browser_type' in applicable %}
{{ render_field(form.browser_type) }} {{ _('Which browser engine to launch locally.') }}
{% endif %} - {% if caps.supports_delete_created_files %} + {% if 'delete_created_files' in applicable %}
{{ render_checkbox_field(form.delete_created_files) }}
{% endif %}
-{% if caps.supports_screenshots %} +{% if 'viewport_width' in applicable %}
{{ _('Screen size') }}
@@ -59,16 +64,16 @@
{% endif %} -{% if caps.supports_request_timeout or caps.supports_custom_user_agent %} +{% if 'timeout' in applicable or 'user_agent' in applicable %}
{{ _('HTTP request options') }} - {% if caps.supports_request_timeout %} + {% if 'timeout' in applicable %}
{{ render_field(form.timeout) }} {{ _('Maximum seconds to wait for the response (plain HTTP client only).') }}
{% endif %} - {% if caps.supports_custom_user_agent %} + {% if 'user_agent' in applicable %}
{{ render_field(form.user_agent) }} {{ _('Sent as the User-Agent header for requests using this browser.') }} @@ -77,7 +82,7 @@
{% endif %} -{% if caps.supports_request_blocking %} +{% if 'block_resource_types' in applicable %}
{{ _('Skip downloads (save bandwidth)') }}
@@ -95,7 +100,7 @@
{% endif %} -{% if caps.supports_screenshots %} +{% if 'screenshot_format' in applicable %}
{{ render_field(form.screenshot_format) }}
diff --git a/changedetectionio/model/browser_config.py b/changedetectionio/model/browser_config.py index a2bfa801e..75294eed9 100644 --- a/changedetectionio/model/browser_config.py +++ b/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): diff --git a/changedetectionio/tests/test_browser_config.py b/changedetectionio/tests/test_browser_config.py index edadfaa4e..e463ab8ca 100644 --- a/changedetectionio/tests/test_browser_config.py +++ b/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"