refactor(fl): allowlist the app bundle — ship only app/ plus the backend's root file - #1008
refactor(fl): allowlist the app bundle — ship only app/ plus the backend's root file#1008atriaybagur wants to merge 6 commits into
Conversation
list_local_base_files walks FL_APP_BASE_DIR and mirrors it 1:1 into the app
bucket, filtering nothing but symlinks. In dev the repo's fl-apps/ tree is
bind-mounted into flip-api, so whatever a developer's tooling leaves inside a
template directory is bundled and uploaded to every trust.
It is not hypothetical: fl-apps/flower/{standard,evaluation} currently carry a
.venv and a .ruff_cache, and a live e2e smoke uploaded 35 files for the xray
app of which 19 were artefacts:
.venv/pyvenv.cfg .venv/.lock .venv/bin/activate{,.bat,.csh,.fish,.nu,.ps1}
.venv/lib/python3.12/site-packages/_virtualenv.{py,pth}
.ruff_cache/CACHEDIR.TAG .ruff_cache/0.14.7/... ...
Prune a denylist of known dev/tooling directory names during the walk (in
place, so a virtualenv's file tree is never descended into), plus *.pyc/*.pyo
and .DS_Store. Against the current tree this takes flower/standard from 26
files to 6 and flower/evaluation from 24 to 5; nvflare/standard, which is
clean, is unchanged at 6.
Deliberately a denylist and not a 'skip dotfiles' rule: the tutorials ship a
real .env.app that must still reach the trusts. Covered by a test.
Production never saw this - the baked tree comes from a clean CI checkout - but
FL_APP_BASE_DIR is documented as overridable to an operator-provided tree, which
has the same exposure as dev.
Signed-off-by: at24_bioeng625-pc <alexandre.triay_bagur@kcl.ac.uk>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Both inline comments addressed in eae004f: each bundling path now verifies the backend's root file (meta.json / pyproject.toml) is actually present in the walked template, after the existing empty-list guard, and fails with a FileNotFoundError naming the missing file and the template dir. Unit tests added for both backends; flip-api ruff + mypy + unit suite green (1582 passed). |
…son guard CI tests the PR merge ref, where develop's fail-loudly missing-config.json check (#1056) runs before the base-template walk: the new missing-root-file tests now supply config.json (with is_valid_job_type patched, as every config-bearing bundler test does), and develop's empty-manifest test gets the meta.json its base tree needs to reach the manifest check it pins. Signed-off-by: at24_bioeng625-pc <alexandre.triay_bagur@kcl.ac.uk>
| skipped.append(entry.name) | ||
| continue | ||
| if entry.is_dir(): | ||
| if entry.name != BUNDLED_APP_DIR_NAME: |
There was a problem hiding this comment.
This hardcodes recursion into a directory literally named app, but bundle_nvflare_application's own downstream comment a few dozen lines below (Find app folders (top-level directories that start with app, e.g. app, app_site1, etc), around line 685) and fl-services/nvflare/fl-api-base/fl_api/utils/upload.py both document app_site-1/app_site-2 as a supported multi-site base-template layout. With this change, any such folder in a local base template -- baked-in or an operator-provided FL_APP_BASE_DIR override -- is silently excluded (only a DEBUG log records it), with no test covering the scenario. Was retiring multi-site base templates intentional? If so it should be called out explicitly and the stale docs/comments updated; if not, the allowlist needs to recognize app_site-/app_ directories too.
| skipped.append(entry.name) | ||
|
|
||
| if skipped: | ||
| logger.debug(f"Excluded from the {base_dir.name} bundle (not app/ or an allowed root file): {sorted(skipped)}") |
There was a problem hiding this comment.
The docstring promises every skipped path is logged at debug level, but a symlinked directory nested inside app/ is excluded via os.walk(followlinks=False) without ever being appended to skipped in _walk_app_dir -- unlike a symlinked directory at the template root, which is logged. Minor inconsistency between the documented guarantee and actual behavior for this one nested case.
garciadias
left a comment
There was a problem hiding this comment.
The allowlist rework is a well-reasoned, well-tested fix for a real production incident (dev/tooling artefacts shipped to trusts). However it silently and untestedly narrows a separate, documented capability: NVFLARE's multi-site app_site-N base-template folders (still referenced in this same file's bundle_nvflare_application comments and in fl-api-base/fl_api/utils/upload.py) are no longer picked up by list_local_base_files, with only a DEBUG log recording the omission. Please either widen the allowlist to recognize app_site-/app_ folders, or explicitly retire that capability with updated docs/comments and a test proving the new intentional behavior. Two additional minor nits left inline.
|
One more finding that GitHub wouldn't let me anchor as an inline comment (the line falls outside this PR's diff hunks) — noting it here instead: flip-api/src/flip_api/fl_services/services/fl_service.py:692 (Minor) — the |
What
list_local_base_fileswalksFL_APP_BASE_DIRand mirrors it 1:1 into the app bucket, filtering nothing but symlinks. This replaces that with a positive allowlist: a bundle is everything underapp/plus one root file per backend, and nothing else.pyproject.tomlmust stay at the root because[tool.flwr.app.components]resolvesapp.server_app:apprelative to it —flwr run .executes there.Why a denylist wasn't enough
A template directory is also a live uv project root — it is where
pyproject.tomlsits — so it is exactly whereuv sync,ruff,pytestand friends write their caches. In dev that tree is bind-mounted into flip-api, so a denylist has to keep pace with every tool a developer might run, and one miss ships the artefact to every trust. The denylist this replaces already omitted.tox,.nox,htmlcov,.idea,dist/and*.egg-info.Allowlisting is closed by construction: a
.venvis excluded because it is notapp/, not because someone remembered to name it.This is not hypothetical. It cost a full afternoon of e2e debugging on 2026-08-26. A
.venvbuilt against python3.14 sat infl-apps/flower/{standard,evaluation}, was bundled to S3, and was downloaded into every Flower run directory — where the ServerApp found an interpreter-less venv, died instantly, and left[flwr-serverapp] <defunct>. The hub reported onlystatus=INITIATEDuntil timeout. Four consecutive runs failed this way before the cause was traced.Note those
.venvdirs are gitignored — and gitignored is not excluded from the bundler, which walks the filesystem, not git.It also drops real files that don't belong at a trust
A denylist could never catch these, because they aren't artefacts:
recipe.pyfullextra — flip + nvflare + torch)". It would otherwise sit dead in every trust's app directory as executable Python.README.mdrequired_files.jsonfl-apps/<backend>/required_files.jsonEffect
nvflare/standardflower/standardflower/standard, with a developer's.venvDesign notes
__pycache__/*.pyc/*.pyostay named explicitly. Compiled Python is the one artefact that appears insideapp/, where position alone cannot exclude it. Pruned in place ondirnamesduringos.walk, so those trees are never descended into.app*/custom/, or diverted toserver_checkpoints/so a large file does not collapse NVFLARE's app-deploy. They never pass throughlist_local_base_files.FL_APP_BASE_DIRmay point at an operator-provided tree, where an allowlist drops files it does not recognise. Every excluded path is logged at debug level, and the "base application files missing" error now names the rule, so a visibly non-empty template directory reporting "missing" stays diagnosable..env.appis no longer preserved. The previous denylist avoided a "skip dotfiles" rule to protect it — but there are zero.env.appfiles underfl-apps; all 8 live infl-tutorials, which this walk never touches. Even there it is a simulator-time file.Behaviour change
A template with no
app/directory now yields no bundleable files at all, so it is rejected at the first check (Base application files missing …) rather than by the later app-folder scan.test_bundle_nvflare_application_no_app_foldersis updated accordingly; the error text now names the rule and the debug log lists what was excluded.Tests
flip-api/tests/unit/fl_services/— 200 passed. Nine cover this walk directly:.venv,.ruff_cache,.DS_Store).tox,.nox,htmlcov,.idea,dist,*.egg-info) — the property a denylist cannot have__pycache__/.pyc/.pyoinsideapp/recipe.py,README.md,required_files.jsondroppedapp/