From 3e6c2961550a25bafb3113fecc2d2fc28dd0cc37 Mon Sep 17 00:00:00 2001 From: 8rulerstar <190670431+8rulerstar@users.noreply.github.com> Date: Wed, 30 Sep 2026 10:30:51 +0900 Subject: [PATCH] fix(local): retire the pre-host egress grants in revision 0027 --- docs/architecture/data-model.md | 7 +- docs/architecture/egress.md | 1 - .../0027_retire_pre_host_egress_grants.py | 58 ++++++++ .../tests/integration/test_migration_0027.py | 130 ++++++++++++++++++ 4 files changed, 193 insertions(+), 3 deletions(-) create mode 100644 surfsense_local/backend/alembic/versions/0027_retire_pre_host_egress_grants.py create mode 100644 surfsense_local/backend/tests/integration/test_migration_0027.py diff --git a/docs/architecture/data-model.md b/docs/architecture/data-model.md index c0ea05e60e..c2b11f296b 100644 --- a/docs/architecture/data-model.md +++ b/docs/architecture/data-model.md @@ -157,7 +157,7 @@ Connections are in [`connections.md`](connections.md); selection and onboarding | `license_state` | `id`, `certificate`, `imported_at`, `clock_watermark` | a singleton; plan and expiry are re-derived from the certificate on every read, and `clock_watermark` is the highest instant ever seen | | `egress_destinations` | `destination`, `enabled`, `last_call_at` | one row per host; `enabled` defaults to false | -A destination is `host:`: `host:huggingface.co` for model search and downloads, and one per remote host, shared by every connection to it; a loopback endpoint needs none. Rows under the earlier names `model_download`, `model_search` and `image_model_pull` are no longer read; revision `0012` renamed `ollama_pull` to `model_download` before that change. See [`license/app.md`](license/app.md) and [`egress.md`](egress.md). +A destination is `host:`: `host:huggingface.co` for model search and downloads, and one per remote host, shared by every connection to it; a loopback endpoint needs none. Revision `0027` removes rows under the earlier names `model_download`, `model_search` and `image_model_pull`, carrying a grant to `host:huggingface.co` only where search and a download were both allowed and the host had no answer of its own. See [`license/app.md`](license/app.md) and [`egress.md`](egress.md). ### `plugin_runs` @@ -369,18 +369,21 @@ erDiagram | `0014` | `0014_connection_catalog_provider.py` | `provider_connections.catalog_provider`, `custom` for every existing connection; downgrading drops the column in place, because a table rebuild would cascade into `selected_models` | | `0015` | `0015_image_selection_by_build.py` | a local `image_gen` selection is renamed from the old list's name to its curated build's id (`stable-diffusion-1.5` to `v1-5-pruned_Q4_0`, and the two SDXL models); the map is frozen in the migration, and downgrading reverses it | | `0016` | `0016_local_audio_provider.py` | `selected_models` rebuilt so `audiocpp` may hold `audio_gen`, and only that, without a connection; downgrading drops an `audiocpp` selection | +| `0017` | `0017_keyword_index_keeps_combining_marks.py` | the keyword index rebuilt from `chunks` with combining marks kept inside a word | +| `0018` | `0018_sdcpp_edit_and_video.py` | `selected_models` rebuilt so `sdcpp` may hold `image_edit` and `video_gen`; downgrading drops such a selection | | `0019` | `0019_selection_settings.py` | `selected_models.settings`, a nullable JSON column added in place; its first entry is a server audio model's `voices` | | `0020` | `0020_plugin_runs.py` | `plugin_runs` | | `0022` | `0022_embedding_indexes.py` | `embedding_indexes` and `documents.embedding_index_id`; an existing library gets a bge-small row and its documents are stamped. The column is added by a plain `ALTER TABLE`, since a batch rebuild of `documents` would cascade to every chunk | | `0024` | `0024_thread_source_scope.py` | `chat_threads.source_scope`, by a plain `ALTER TABLE`; no row is backfilled, so every older thread uses every source | | `0025` | `0025_source_roots_and_folders.py` | `source_roots` and `folders`; a Library root and root folder per workspace; `documents.folder_id` by a plain `ALTER TABLE`; every `FILE` and `NOTE` filed, an imported `folder_path` becoming a chain of folders (case and normalization variants merged, levels past 8 joined with " / " into the eighth); `content_hash` filled from `dedup_key`; the dedup index swapped to per folder by `DROP` and `CREATE INDEX` | | `0026` | `0026_chat_history_start.py` | `chat_threads.history_start_message_id`, by a plain `ALTER TABLE`; no row is backfilled, so a thread sends its whole history until it first outgrows its budget | +| `0027` | `0027_retire_pre_host_egress_grants.py` | egress rows under `model_download`, `model_search` and `image_model_pull` removed; `host:huggingface.co` allowed only where search and a download were both allowed and the host had no row; downgrading changes nothing | - Before the API applies a pending revision to an existing database, `upgrade_to_head` writes a `VACUUM INTO` copy to `/backups/-.db` (via `.partial`, renamed once complete) and keeps the newest two ([`migration_snapshot.py`](../../surfsense_local/backend/shared/migration_snapshot.py)). A new database or one at head writes none. If the copy fails, nothing migrates and the API start fails with the reason. To restore: quit SurfSense, replace `surfsense.db` with the snapshot, and start the app. - Migrations run on every API start and are idempotent. Autogenerate is off: it renders a rename as a drop plus an add, which deletes a column's data silently, and `env.py` carries no `target_metadata`, so it cannot be used by accident. - SQLite cannot alter a CHECK constraint in place, so `0004`, `0009`, `0012` and `0013` copy `selected_models` into a new table. - A revision that touches a table already holding rows should read the live schema first (`op.get_bind()`, `sa.inspect`) rather than assume its shape. -- [`tests/integration/test_migrations.py`](../../surfsense_local/backend/tests/integration/test_migrations.py) fails when the models and the migration history disagree, when a second upgrade is not a no-op, and when a failed migration leaves anything behind. `test_migration_0012.py`, `test_migration_0013.py` and `test_migration_0014.py` beside it test what those revisions change. +- [`tests/integration/test_migrations.py`](../../surfsense_local/backend/tests/integration/test_migrations.py) fails when the models and the migration history disagree, when a second upgrade is not a no-op, and when a failed migration leaves anything behind. `test_migration_0012.py`, `test_migration_0013.py`, `test_migration_0014.py` and `test_migration_0022.py` beside it test what those revisions change. ## Known gaps diff --git a/docs/architecture/egress.md b/docs/architecture/egress.md index dfea7e6d44..e3f03aa858 100644 --- a/docs/architecture/egress.md +++ b/docs/architecture/egress.md @@ -71,4 +71,3 @@ Settings › Network lists the App updates row and every destination with its ho - The Office Studio formats run model-written code in the worker, and that code can open connections of its own ([studio](studio.md)). - The Office pack's download is allowed by its host, `downloadarchive.documentfoundation.org`, which redirects to one of TDF's mirrors; the mirror sees the user's IP address and is not asked about ([office pack](office-pack.md)). -- Grants stored under the earlier destination names, `model_download`, `model_search` and `image_model_pull`, are not carried over to `host:huggingface.co`. Nothing reads them any more, so someone who had allowed model downloads is asked again, and the old rows stay in the table; revision 0012 still turns an Ollama-era `ollama_pull` grant into `model_download`. diff --git a/surfsense_local/backend/alembic/versions/0027_retire_pre_host_egress_grants.py b/surfsense_local/backend/alembic/versions/0027_retire_pre_host_egress_grants.py new file mode 100644 index 0000000000..0aadc0b347 --- /dev/null +++ b/surfsense_local/backend/alembic/versions/0027_retire_pre_host_egress_grants.py @@ -0,0 +1,58 @@ +"""Retire the egress grants stored under the earlier destination names. + +`host:huggingface.co` covers search as well as downloads, so, as in 0012, only +a user who allowed both is carried across. An answer already given for the +host is kept. Downgrading changes nothing: 0026 reads only host names. + +Revision ID: 0027 +Revises: 0026 +""" + +from collections.abc import Sequence + +import sqlalchemy as sa + +from alembic import op +from modules.egress.service import HUGGINGFACE + +revision: str = "0027" +down_revision: str | Sequence[str] | None = "0026" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + +SEARCH = "model_search" +DOWNLOADS = ("model_download", "image_model_pull") + + +def upgrade() -> None: + bind = op.get_bind() + if not sa.inspect(bind).has_table("egress_destinations"): + return + rows = dict( + bind.execute( + sa.text( + "SELECT destination, enabled FROM egress_destinations " + "WHERE destination IN :names" + ).bindparams(sa.bindparam("names", expanding=True)), + {"names": [HUGGINGFACE, SEARCH, *DOWNLOADS]}, + ).all() + ) + allowed_both = rows.get(SEARCH) and any(rows.get(name) for name in DOWNLOADS) + if allowed_both and HUGGINGFACE not in rows: + bind.execute( + sa.text( + "INSERT INTO egress_destinations(destination, enabled) " + "VALUES (:destination, 1)" + ), + {"destination": HUGGINGFACE}, + ) + bind.execute( + sa.text( + "DELETE FROM egress_destinations WHERE destination IN :names" + ).bindparams(sa.bindparam("names", expanding=True)), + {"names": [SEARCH, *DOWNLOADS]}, + ) + + +def downgrade() -> None: + pass diff --git a/surfsense_local/backend/tests/integration/test_migration_0027.py b/surfsense_local/backend/tests/integration/test_migration_0027.py new file mode 100644 index 0000000000..bd21a2c681 --- /dev/null +++ b/surfsense_local/backend/tests/integration/test_migration_0027.py @@ -0,0 +1,130 @@ +"""Revision 0027: grants stored under the earlier destination names. + +`host:huggingface.co` covers search as well as downloads, so only a user who +allowed both is carried across. Every old row is removed either way. +""" + +from pathlib import Path + +import pytest +from alembic.config import Config +from sqlalchemy import Engine, text + +from alembic import command +from shared.db import create_db_engine + +pytestmark = pytest.mark.integration + +HUGGINGFACE = "host:huggingface.co" + + +def _config(engine: Engine) -> Config: + config = Config() + config.set_main_option( + "script_location", str(Path(__file__).parents[2] / "alembic") + ) + config.attributes["engine"] = engine + return config + + +def upgraded(tmp_path: Path, rows: dict[str, bool]) -> Engine: + """A database seeded at 0026 and then upgraded to 0027.""" + engine = create_db_engine(tmp_path / "surfsense.db") + config = _config(engine) + command.upgrade(config, "0026") + with engine.begin() as connection: + for destination, enabled in rows.items(): + connection.execute( + text( + "INSERT INTO egress_destinations(destination, enabled) " + "VALUES (:destination, :enabled)" + ), + {"destination": destination, "enabled": enabled}, + ) + command.upgrade(config, "0027") + return engine + + +def destinations(engine: Engine) -> dict[str, int]: + """Every egress row, as destination to enabled.""" + with engine.connect() as connection: + return dict( + connection.execute( + text("SELECT destination, enabled FROM egress_destinations") + ).all() + ) + + +@pytest.mark.parametrize("download", ["model_download", "image_model_pull"]) +def test_a_download_grant_alone_is_removed_not_carried( + tmp_path: Path, download: str +) -> None: + """The host also receives what the user types in search, which a download + grant never covered, so the user is asked once instead.""" + engine = upgraded(tmp_path, {download: True}) + + assert destinations(engine) == {} + + +@pytest.mark.parametrize("download", ["model_download", "image_model_pull"]) +def test_search_and_a_download_together_carry_across( + tmp_path: Path, download: str +) -> None: + """Both halves of the host's consent were given, so it is not asked again.""" + engine = upgraded(tmp_path, {"model_search": True, download: True}) + + assert destinations(engine) == {HUGGINGFACE: 1} + + +def test_a_refused_half_carries_nothing(tmp_path: Path) -> None: + """A refusal is a half that was not given.""" + engine = upgraded(tmp_path, {"model_search": True, "model_download": False}) + + assert destinations(engine) == {} + + +def test_an_answer_already_given_for_the_host_wins(tmp_path: Path) -> None: + """From v2.0.3 the host can be answered directly; an older grant must not + overturn that answer.""" + engine = upgraded( + tmp_path, + {HUGGINGFACE: False, "model_search": True, "model_download": True}, + ) + + assert destinations(engine) == {HUGGINGFACE: 0} + + +def test_other_hosts_are_untouched(tmp_path: Path) -> None: + """Only the three old names are this revision's business.""" + engine = upgraded(tmp_path, {"host:api.provider.example": True}) + + assert destinations(engine) == {"host:api.provider.example": 1} + + +def test_downgrade_keeps_the_host_grant(tmp_path: Path) -> None: + """The app at 0026 reads only host names, and a merged row cannot say + which old name it came from.""" + engine = upgraded(tmp_path, {"model_search": True, "model_download": True}) + + command.downgrade(_config(engine), "0026") + + assert destinations(engine) == {HUGGINGFACE: 1} + + +def test_an_install_upgrading_from_v2_0_2_is_left_clean(tmp_path: Path) -> None: + """v2.0.2 stored `ollama_pull` and `image_model_pull`; 0012 renames the + first, and neither covered search.""" + engine = create_db_engine(tmp_path / "surfsense.db") + config = _config(engine) + command.upgrade(config, "0011") + with engine.begin() as connection: + connection.execute( + text( + "INSERT INTO egress_destinations(destination, enabled) " + "VALUES ('ollama_pull', 1), ('image_model_pull', 1)" + ) + ) + + command.upgrade(config, "head") + + assert destinations(engine) == {}