merge: bring the element-style work onto main's api_v3 package split

main moved 16 commits while this branch was open, and one of them split
api_v3.py into a package. A rebase would have conflicted on that file in
each of the 15 commits that touch it; merging resolves it once.

Reapplied into the package, unchanged in substance:

- _schema_type_is / _schema_allows_null and the nullable-field handling
  (api_v3/__init__.py, where the form parser now lives)
- the indexed-array recombiner's union-type fix and its all-blank-means-null
  case (api_v3/plugins.py)
- scalable / native_size on the font catalog, and the removal of the three
  font-override stubs (api_v3/fonts.py)

Two places took main's version rather than mine, deliberately:

- pages_v3 now reads the schema through SchemaManager *and* keeps
  resolve_under for the raw-read fallback, so the containment guard main
  added in the CodeQL triage survives.
- serve_plugin_widget moves off os.path.basename onto safe_path_component
  and resolve_under. main dropped `import os` from pages_v3 in favour of
  those helpers, and they reject a bad component rather than mangling it
  into a valid-looking one.

The url-map snapshot loses exactly the three font-override routes. Edited
rather than regenerated: that test exists to catch a route lost by accident
during a move, and regenerating it wholesale is how such a loss would be
papered over.

Verified against a fresh run of current main: same failure set, 4692
passing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

test/web_interface/test_api_v3_backup_restore.py

@@ -139,6 +139,58 @@ class TestRequestValidation:
         assert post(client, options="{}").status_code == 200
         assert restore.call_args[0][2].restore_config is True
 
+    def test_unknown_option_key_is_refused(self, client, restore):
+        # Regression: opts_dict.get('restore_secrets', True) silently
+        # ignores a typo'd/renamed key like "restoreSecrets" and keeps the
+        # True default, restoring secrets a caller's request clearly meant
+        # to exclude -- with no indication anything was wrong.
+        response = post(client, options=json.dumps({"restoreSecrets": False}))
+        assert response.status_code == 400
+        assert "Unknown restore option" in response.get_json()["message"]
+        assert "restoreSecrets" in response.get_json()["message"]
+        restore.assert_not_called()
+
+    def test_known_and_unknown_keys_together_are_refused(self, client, restore):
+        response = post(client, options=json.dumps({
+            "restore_secrets": False, "restore_everything": True}))
+        assert response.status_code == 400
+        restore.assert_not_called()
+
+
+class TestOptionsAreBooleanAware:
+    """Regression: bool("false") is True in Python.
+
+    Every restore flag used bare bool() coercion, so a caller that sends its
+    options as JSON strings rather than real booleans -- a form field, a
+    hand-built request -- had `{"restore_secrets": "false"}` restore secrets
+    anyway, the opposite of what was asked. Fixed with the same
+    string-aware `_coerce_to_bool` already used for checkbox-style config
+    fields elsewhere in this package (config.py, plugins.py).
+    """
+
+    @pytest.mark.parametrize("raw,expected", [
+        ("false", False), ("False", False), ("FALSE", False),
+        ("0", False),
+        ("true", True), ("True", True), ("1", True),
+    ])
+    def test_string_valued_flags_are_parsed_not_just_truthy(
+            self, client, restore, raw, expected):
+        post(client, options=json.dumps({"restore_secrets": raw}))
+        assert restore.call_args[0][2].restore_secrets is expected
+
+    def test_a_string_false_does_not_restore_secrets(self, client, restore):
+        # The exact shape of the bug: a truthy non-empty string coerced by
+        # bare bool() to True regardless of its contents.
+        post(client, options=json.dumps({"restore_secrets": "false"}))
+        assert restore.call_args[0][2].restore_secrets is False
+
+    def test_real_json_booleans_still_work(self, client, restore):
+        post(client, options=json.dumps({"restore_secrets": False,
+                                          "restore_config": True}))
+        options = restore.call_args[0][2]
+        assert options.restore_secrets is False
+        assert options.restore_config is True
+
 
 class TestSuccess:
     def test_success_returns_the_result(self, client, restore):
@@ -230,7 +282,7 @@ class TestPluginReinstall:
     def test_missing_store_manager_is_reported_per_plugin(self, client, restore):
         restore.return_value = FakeResult(plugins_to_install=[{"plugin_id": "clock"}])
         api_v3.plugin_store_manager = None
-        with patch("web_interface.blueprints.api_v3.plugin_store_manager", None):
+        with patch("web_interface.blueprints.api_v3.backup.plugin_store_manager", None):
             body = post(client).get_json()
         assert body["data"]["plugins_failed"][0]["error"] == "Store manager unavailable"
 

test/web_interface/test_calendar_oauth_endpoints.py

@@ -77,7 +77,10 @@ class TestTheRoutesExistAtAll:
         oauth = Path(project_root) / 'web_interface/static/v3/js/widgets/google-oauth.js'
         assert '/api/v3/plugins/calendar/list-calendars' in picker.read_text(encoding='utf-8')
         assert '/api/v3/plugins/calendar/authenticate' in oauth.read_text(encoding='utf-8')
-        source = (Path(project_root) / 'web_interface/blueprints/api_v3.py').read_text(encoding='utf-8')
+        # api_v3 is a package now, so the route strings are spread across its
+        # modules; read the whole directory rather than one file.
+        pkg = Path(project_root) / 'web_interface/blueprints/api_v3'
+        source = "\n".join(f.read_text(encoding='utf-8') for f in sorted(pkg.glob('*.py')))
         assert "'/plugins/calendar/list-calendars'" in source
         assert "'/plugins/calendar/authenticate'" in source
 
@@ -360,6 +363,22 @@ class TestDiagnosticsAreRedacted:
         assert 'hunter2' not in error, error
         assert '<redacted>' in error, error
 
+    def test_script_stderr_is_redacted_in_the_log_too(self, tmp_path, caplog):
+        # Regression: the return value went through redact_text (asserted
+        # above), but the logger.error call right next to it logged `raw`
+        # verbatim -- a script that handles OAuth client secrets and can
+        # quote them in its stderr, landing unredacted in the log (CWE-532).
+        script = tmp_path / 'calendar_registration.py'
+        script.write_text(
+            'import sys\n'
+            'sys.stderr.write("boom client_secret=hunter2 more\\n")\n',
+            encoding='utf-8')
+        with caplog.at_level('ERROR', logger=mod.logger.name):
+            mod._run_calendar_registration(tmp_path, '')
+        logged = '\n'.join(r.getMessage() for r in caplog.records)
+        assert 'hunter2' not in logged, logged
+        assert '<redacted>' in logged, logged
+
     def test_a_failing_script_payload_is_redacted(self, client):
         (client.plugin_dir / 'credentials.json').write_text('{}', encoding='utf-8')
         (client.plugin_dir / 'calendar_registration.py').write_text(

test/web_interface/test_config_logging_omits_secrets.py

@@ -10,8 +10,8 @@ from pathlib import Path
 
 import pytest
 
-SOURCE = (Path(__file__).resolve().parents[2]
-          / "web_interface" / "blueprints" / "api_v3.py")
+API_V3_PKG = (Path(__file__).resolve().parents[2]
+              / "web_interface" / "blueprints" / "api_v3")
 
 #: Objects that still hold submitted secret values at the point these log
 #: calls run. Interpolating one whole into a log message leaks credentials.
@@ -19,7 +19,7 @@ UNREDACTED = ("plugin_config", "secrets_config", "current_secrets")
 
 
 def _logging_lines():
-    for number, line in enumerate(SOURCE.read_text(encoding="utf-8").splitlines(), 1):
+    for number, line in enumerate("\n".join(p.read_text(encoding="utf-8") for p in sorted(API_V3_PKG.glob("*.py"))).splitlines(), 1):
         stripped = line.strip()
         if stripped.startswith("#"):
             continue

test/web_interface/test_secret_separation_parity.py

@@ -15,8 +15,18 @@ from pathlib import Path
 
 from src.web_interface.secret_helpers import find_secret_fields, separate_secrets
 
-API_V3_PATH = (Path(__file__).resolve().parents[2]
-               / "web_interface" / "blueprints" / "api_v3.py")
+API_V3_PKG = (Path(__file__).resolve().parents[2]
+              / "web_interface" / "blueprints" / "api_v3")
+
+
+def _api_v3_source() -> str:
+    """Every module of the api_v3 package as one string.
+
+    It used to be a single file; the inline copies this guards against could
+    now reappear in any module of the package.
+    """
+    return "\n".join(p.read_text(encoding="utf-8")
+                     for p in sorted(API_V3_PKG.glob("*.py")))
 
 # The migration is complete: any inline reimplementation is a regression.
 EXPECTED_INLINE_COPIES = 0
@@ -24,7 +34,7 @@ EXPECTED_INLINE_COPIES = 0
 
 class TestNoInlineCopies:
     def _count(self, name: str) -> int:
-        source = API_V3_PATH.read_text(encoding="utf-8")
+        source = _api_v3_source()
         return len(re.findall(rf"^\s*def {name}\(", source, flags=re.MULTILINE))
 
     def test_no_inline_find_secret_fields(self):
@@ -46,7 +56,7 @@ class TestNoInlineCopies:
     def test_canonical_import_present(self):
         # Tripwire: the endpoints still need the helpers, so removing the
         # import means either dead secret handling or a new local copy.
-        source = API_V3_PATH.read_text(encoding="utf-8")
+        source = _api_v3_source()
         assert re.search(
             r"from src\.web_interface\.secret_helpers import .*find_secret_fields",
             source,

test/web_interface/test_starlark_pixlet_routes.py

@@ -63,14 +63,14 @@ class TestRoutesAreRegistered:
 
 class TestInstallPixlet:
     def test_it_does_not_404(self, client):
-        with patch('web_interface.blueprints.api_v3.subprocess.run') as run:
+        with patch('web_interface.blueprints.api_v3.starlark.subprocess.run') as run:
             run.return_value = MagicMock(returncode=0, stdout="ok", stderr="")
             resp = client.post('/api/v3/starlark/install-pixlet')
         assert resp.status_code != 404, "the route is still missing"
         assert resp.get_json().get('message') != 'Resource not found'
 
     def test_success_is_reported_in_the_shape_the_button_reads(self, client):
-        with patch('web_interface.blueprints.api_v3.subprocess.run') as run:
+        with patch('web_interface.blueprints.api_v3.starlark.subprocess.run') as run:
             run.return_value = MagicMock(returncode=0, stdout="done", stderr="")
             resp = client.post('/api/v3/starlark/install-pixlet')
         body = resp.get_json()
@@ -78,7 +78,7 @@ class TestInstallPixlet:
         assert 'message' in body, "the JS shows data.message on success"
 
     def test_a_failed_download_says_why(self, client):
-        with patch('web_interface.blueprints.api_v3.subprocess.run') as run:
+        with patch('web_interface.blueprints.api_v3.starlark.subprocess.run') as run:
             run.return_value = MagicMock(returncode=1, stdout="", stderr="no such release")
             resp = client.post('/api/v3/starlark/install-pixlet')
         body = resp.get_json()
@@ -88,7 +88,7 @@ class TestInstallPixlet:
 
     def test_a_timeout_is_reported_rather_than_hanging(self, client):
         import subprocess as sp
-        with patch('web_interface.blueprints.api_v3.subprocess.run',
+        with patch('web_interface.blueprints.api_v3.starlark.subprocess.run',
                    side_effect=sp.TimeoutExpired(cmd='x', timeout=300)):
             resp = client.post('/api/v3/starlark/install-pixlet')
         assert resp.get_json()['status'] == 'error'
@@ -395,6 +395,164 @@ class TestTheManifestStaysRelocatable:
         assert self._install(tmp_path)['star_file'] == 'demo.star'
 
 
+class TestManifestLockPreventsLostUpdates:
+    """The standalone manifest fallback (no plugin instance loaded) reads,
+    mutates and writes manifest.json with no coordination across requests.
+    Each write is atomic on its own (temp file + rename), but two concurrent
+    read-modify-write cycles can still race: both read the same starting
+    manifest, and the second write silently discards whatever the first one
+    added. _starlark_manifest_lock closes that window -- mirrors
+    StarlarkAppsPlugin._update_manifest_safe, which already does this when
+    the plugin instance is loaded.
+    """
+
+    @pytest.fixture
+    def starlark_dir(self, tmp_path, monkeypatch):
+        from web_interface.blueprints import api_v3 as module
+        apps_dir = tmp_path / "starlark-apps"
+        apps_dir.mkdir()
+        monkeypatch.setattr(module, '_STARLARK_APPS_DIR', apps_dir)
+        monkeypatch.setattr(module, '_STARLARK_MANIFEST_FILE', apps_dir / 'manifest.json')
+        monkeypatch.setattr(module, '_STARLARK_MANIFEST_LOCK_FILE', apps_dir / 'manifest.json.lock')
+        module._write_starlark_manifest({'apps': {}})
+        return apps_dir
+
+    def test_the_locked_file_survives_a_manifest_write(self, starlark_dir):
+        """_write_starlark_manifest replaces manifest.json with a fresh inode
+        on every write (temp file + rename). If the lock were taken on that
+        same file, a second locker's fresh os.open() right after the rename
+        would land on the new inode -- unguarded, because only the old,
+        now-orphaned inode was ever locked -- and two writers could race
+        despite each believing it "held the lock" (see the docstring on
+        _starlark_manifest_lock). Locking a sidecar path that no write ever
+        touches or renames over closes that: the inode identity of what gets
+        locked must not change across writes.
+        """
+        import os
+
+        from web_interface.blueprints import api_v3 as module
+
+        with module._starlark_manifest_lock():
+            manifest = module._read_starlark_manifest()
+        lock_ino_before = os.stat(module._STARLARK_MANIFEST_LOCK_FILE).st_ino
+
+        for app_id in ('one', 'two', 'three'):
+            with module._starlark_manifest_lock():
+                manifest = module._read_starlark_manifest()
+                manifest.setdefault('apps', {})[app_id] = {'enabled': True}
+                assert module._write_starlark_manifest(manifest)
+
+        lock_ino_after = os.stat(module._STARLARK_MANIFEST_LOCK_FILE).st_ino
+        assert lock_ino_after == lock_ino_before, (
+            "the locked file's inode changed across writes -- a locker that "
+            "opened it before this write and one that opens it after would "
+            "no longer contend for the same lock")
+
+    def test_two_concurrent_updates_are_both_kept(self, starlark_dir):
+        import threading
+        import time as _time
+
+        from web_interface.blueprints import api_v3 as module
+
+        def add_app(app_id):
+            with module._starlark_manifest_lock():
+                manifest = module._read_starlark_manifest()
+                # Widen the window between read and write. Without the lock
+                # both threads read here before either writes, and whichever
+                # writes second overwrites the other's addition; with the
+                # lock, the second thread cannot even start its read until
+                # the first has written and released.
+                _time.sleep(0.05)
+                manifest.setdefault('apps', {})[app_id] = {'enabled': True}
+                module._write_starlark_manifest(manifest)
+
+        threads = [threading.Thread(target=add_app, args=(app_id,))
+                   for app_id in ('a', 'b')]
+        for t in threads:
+            t.start()
+        for t in threads:
+            t.join(timeout=5)
+
+        manifest = module._read_starlark_manifest()
+        assert set(manifest['apps']) == {'a', 'b'}, (
+            "a concurrent update was lost: %r" % (manifest,))
+
+    def test_the_lock_is_reentrant_safe_across_sequential_calls(self, starlark_dir):
+        """Not reentrant within one thread -- just that using it twice in a
+        row (the ordinary case: one request, then the next) works cleanly
+        and does not leak the lock file descriptor or leave it locked."""
+        from web_interface.blueprints import api_v3 as module
+
+        for app_id in ('first', 'second'):
+            with module._starlark_manifest_lock():
+                manifest = module._read_starlark_manifest()
+                manifest.setdefault('apps', {})[app_id] = {'enabled': True}
+                module._write_starlark_manifest(manifest)
+
+        manifest = module._read_starlark_manifest()
+        assert set(manifest['apps']) == {'first', 'second'}
+
+
+class TestConfigAndManifestStayInSync:
+    """Standalone-mode PUT /starlark/apps/<id>/config (no plugin instance
+    loaded) writes config.json and then the manifest. If the manifest write
+    fails after config.json was already written, the two disagree about
+    what was saved unless config.json is rolled back.
+    """
+
+    @pytest.fixture
+    def app_dir(self, tmp_path, monkeypatch):
+        from web_interface.blueprints import api_v3 as module
+        apps_dir = tmp_path / "starlark-apps"
+        apps_dir.mkdir()
+        monkeypatch.setattr(module, '_STARLARK_APPS_DIR', apps_dir)
+        monkeypatch.setattr(module, '_STARLARK_MANIFEST_FILE', apps_dir / 'manifest.json')
+        one_app_dir = apps_dir / 'demo'
+        one_app_dir.mkdir()
+        module._write_starlark_manifest({'apps': {'demo': {'name': 'Demo', 'enabled': True}}})
+        return one_app_dir
+
+    def test_manifest_write_failure_rolls_back_an_existing_config_json(self, client, app_dir):
+        config_file = app_dir / 'config.json'
+        config_file.write_text(json.dumps({'existing': 'value'}))
+
+        with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=None), \
+             patch('web_interface.blueprints.api_v3._write_starlark_manifest', return_value=False):
+            resp = client.put('/api/v3/starlark/apps/demo/config',
+                              json={'new_field': 'x'})
+
+        assert resp.status_code == 500
+        assert json.loads(config_file.read_text()) == {'existing': 'value'}, (
+            "config.json kept the new value even though the manifest write "
+            "that was supposed to follow it failed")
+
+    def test_manifest_write_failure_removes_a_freshly_created_config_json(self, client, app_dir):
+        config_file = app_dir / 'config.json'
+        assert not config_file.exists()
+
+        with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=None), \
+             patch('web_interface.blueprints.api_v3._write_starlark_manifest', return_value=False):
+            resp = client.put('/api/v3/starlark/apps/demo/config',
+                              json={'new_field': 'x'})
+
+        assert resp.status_code == 500
+        assert not config_file.exists(), (
+            "config.json was left behind even though the manifest write "
+            "that was supposed to follow it failed")
+
+    def test_success_updates_both_config_and_manifest(self, client, app_dir):
+        from web_interface.blueprints import api_v3 as module
+
+        with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=None):
+            resp = client.put('/api/v3/starlark/apps/demo/config',
+                              json={'new_field': 'x'})
+
+        assert resp.status_code == 200, resp.get_json()
+        assert json.loads((app_dir / 'config.json').read_text())['new_field'] == 'x'
+        manifest = json.loads(module._STARLARK_MANIFEST_FILE.read_text())
+        assert manifest['apps']['demo']['config']['new_field'] == 'x'
+
+
 # ---------------------------------------------------------------------------
 # The store loaded, then stopped loading, and nothing anywhere said why.
 #
@@ -748,3 +906,66 @@ class TestTheStoreUsesTheTokenTheUserConfigured:
             client.get('/api/v3/starlark/repository/browse')
 
         repo.assert_called_once_with(github_token='ghp_configured')
+
+
+class TestPixletEditorHostDefaultsButDoesNotOverride:
+    """PIXLET_EDITOR_HOST must default to 0.0.0.0, never force it.
+
+    A browser reaching the editor is remote by definition, so a session with
+    nothing configured has to bind more than loopback to be reachable at
+    all -- but an operator who has deliberately pinned PIXLET_EDITOR_HOST to
+    loopback (e.g. in the systemd unit's Environment=, to edit only over an
+    SSH tunnel) must keep that setting. The previous unconditional
+    ``env['PIXLET_EDITOR_HOST'] = '0.0.0.0'`` overrode it every time,
+    always exposing the unauthenticated ``pixlet serve`` dev process on the
+    LAN regardless (CodeQL CWE-1188).
+    """
+
+    @pytest.fixture
+    def app_dir(self, tmp_path):
+        d = tmp_path / "demo_app"
+        d.mkdir()
+        (d / "demo_app.star").write_text("def main():\n    pass\n")
+        return d
+
+    def _start(self, client, app_dir, tmp_path, operator_host):
+        from web_interface.blueprints.api_v3 import starlark as mod
+
+        script = tmp_path / "pixlet_config_editor.sh"
+        script.write_text("#!/bin/bash\n")
+        state_file = tmp_path / "pixlet_editor_state.json"
+        captured = {}
+
+        class FakeProcess:
+            pid = 424242
+
+        def fake_popen(cmd, *args, env=None, **kwargs):
+            if env is not None:
+                captured['env'] = env
+            return FakeProcess()
+
+        with patch.object(mod, '_validate_starlark_app_path',
+                          return_value=(app_dir, None)), \
+             patch.object(mod, '_PIXLET_EDITOR_SCRIPT', script), \
+             patch.object(mod, '_PIXLET_EDITOR_STATE', state_file), \
+             patch.object(mod, '_find_pixlet_binary', return_value='/usr/bin/pixlet'), \
+             patch.object(mod.subprocess, 'Popen', side_effect=fake_popen), \
+             patch.dict(os.environ):
+            if operator_host is None:
+                os.environ.pop('PIXLET_EDITOR_HOST', None)
+            else:
+                os.environ['PIXLET_EDITOR_HOST'] = operator_host
+            resp = client.post('/api/v3/starlark/editor/start',
+                                json={'app_id': app_dir.name})
+
+        assert resp.status_code == 200, resp.get_json()
+        assert 'env' in captured, "subprocess.Popen was never called"
+        return captured['env']
+
+    def test_defaults_to_0_0_0_0_when_operator_set_nothing(self, client, app_dir, tmp_path):
+        env = self._start(client, app_dir, tmp_path, operator_host=None)
+        assert env['PIXLET_EDITOR_HOST'] == '0.0.0.0'
+
+    def test_keeps_an_operator_configured_loopback_host(self, client, app_dir, tmp_path):
+        env = self._start(client, app_dir, tmp_path, operator_host='127.0.0.1')
+        assert env['PIXLET_EDITOR_HOST'] == '127.0.0.1'

test/web_interface/test_systemctl_sudoers_alignment.py

@@ -19,7 +19,12 @@ import re
 from pathlib import Path
 
 PROJECT_ROOT = Path(__file__).resolve().parents[2]
-API_V3 = PROJECT_ROOT / "web_interface" / "blueprints" / "api_v3.py"
+# api_v3 is a package; a sudo systemctl call can live in any of its modules.
+API_V3_PKG = PROJECT_ROOT / "web_interface" / "blueprints" / "api_v3"
+
+
+def _api_v3_source() -> str:
+    return "\n".join(p.read_text() for p in sorted(API_V3_PKG.glob("*.py")))
 SUDOERS_SCRIPT = PROJECT_ROOT / "scripts" / "install" / "configure_web_sudo.sh"
 
 
@@ -51,7 +56,7 @@ def _granted_systemctl_rules(script: str) -> set[tuple[str, str]]:
 
 
 def test_every_sudo_systemctl_call_is_granted() -> None:
-    calls = _sudo_systemctl_calls(API_V3.read_text())
+    calls = _sudo_systemctl_calls(_api_v3_source())
     rules = _granted_systemctl_rules(SUDOERS_SCRIPT.read_text())
 
     assert calls, "expected to find sudo systemctl calls in api_v3.py"
@@ -68,7 +73,7 @@ def test_every_sudo_systemctl_call_is_granted() -> None:
 def test_units_are_fully_qualified() -> None:
     """Privileged systemctl calls must name the unit as <name>.service so they
     match the sudoers grants, which use the fully-qualified unit name."""
-    calls = _sudo_systemctl_calls(API_V3.read_text())
+    calls = _sudo_systemctl_calls(_api_v3_source())
     unqualified = {(v, u) for v, u in calls if not u.endswith(".service")}
     assert not unqualified, (
         "sudo systemctl calls must use fully-qualified .service unit names: "