Block client props in Dashboard creation and PATCH requests (#5290)

### Description

Dashboard client properties can supply provisioning paths that cause
server-local files to be included in downloadable kits. Reject client
creation and PATCH requests containing `props` with HTTP 400 before
changing any stored data, for both client creators and project admins,
regardless of approval state or the supplied value.

Client-kit generation also ignores previously stored `Client.props`,
including malformed JSON, so legacy records cannot continue injecting
provisioning properties after the API restriction takes effect. Client
capacity and operator-managed properties from `properties.yml` or
`properties.json` remain supported. Existing rows do not require
migration.

API integrations that submit `props` must omit it. The current Dashboard
UI's client name and capacity requests continue to work.

### Types of changes

- [x] Breaking change: client API requests containing `props` are now
rejected.
- [x] New tests cover creation and PATCH, creator and admin access,
pending and approved clients, empty/null/invalid values, and rejection
without partial updates.
- [x] Kit-generation regression tests seed legacy properties directly in
the database and download the actual ZIP as the client creator. They
verify that the target file is excluded, stored connection security is
ignored, and client capacity and operator-managed transport properties
still reach the kit.

### Validation

- `./.venv/bin/python -m pytest tests/unit_test/dashboard -q
--disable-warnings`: 73 passed; existing SQLAlchemy and JWT warnings
remain.
- Before the provisioning-boundary fix, the new legacy-path regression
failed because the downloaded kit contained `customRootCA.pem`; both
legacy-path and malformed-JSON cases pass after the fix.
- Scoped equivalent of the project style check passed: Black on each
changed file, isort, flake8, agent skill checks, and `git diff --check`.
Black's combined file check stalled, so formatting was checked per file.

Author: IsaacYangSLA

nvflare/dashboard/application/blob.py

@@ -154,10 +154,9 @@ def gen_client_blob(key, id):
 def _prepare_client(prop_mgr: PropertyManager, prov_project: ProvProject, client_id):
     client = Client.query.get(client_id)
     inc_dl(Client, client_id)
-    if client.props:
-        props = json.loads(client.props)
-    else:
-        props = {}
+    # Legacy client.props may contain user-supplied provisioning paths. Never use
+    # them in kits; additional properties must come from operator configuration.
+    props = {}
 
     if client.capacity:
         props[PropKey.CAPACITY] = json.loads(client.capacity)

nvflare/dashboard/application/clients.py

@@ -27,6 +27,8 @@
 def create_one_client():
     creator = get_jwt_identity()
     req = request.json
+    if "props" in req:
+        return jsonify({"status": "error", "message": "Client props cannot be supplied through the Dashboard API"}), 400
     result = Store.create_client(req, creator)
     if result is not None:
         return jsonify(result), 201
@@ -66,6 +68,11 @@ def update_client(id):
 
     if request.method == "PATCH":
         req = request.json
+        if "props" in req:
+            return (
+                jsonify({"status": "error", "message": "Client props cannot be supplied through the Dashboard API"}),
+                400,
+            )
         if p:
             result = Store.patch_client_by_project_admin(id, req)
         elif c:

tests/unit_test/dashboard/clients_test.py

@@ -11,9 +11,17 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
+import io
+import json
+from uuid import uuid4
+from zipfile import ZipFile
+
 import pytest
 
+from nvflare.dashboard.application import db
 from nvflare.dashboard.application.constants import FLARE_DASHBOARD_NAMESPACE
+from nvflare.dashboard.application.models import Client, Project
+from nvflare.dashboard.application.store import Store
 
 CLIENT1 = {"name": "site-1", "organization": "test.com", "capacity": {"num_gpus": 16, "mem_per_gpu_in_GiB": 64}}
 
@@ -69,3 +77,140 @@ def test_update_client(self, client, client_ids, auth_header):
 
         assert response.status_code == 200
         assert response.json["client"]["organization"] == NEW_ORG
+
+
+class TestClientPropsRejection:
+    @pytest.fixture(scope="class")
+    @classmethod
+    def creator_header(cls, client, auth_header):
+        email = f"client-creator-{uuid4().hex}@test.com"
+        response = client.post(
+            FLARE_DASHBOARD_NAMESPACE + "/api/v1/users",
+            json={"email": email, "password": "test-password", "organization": "test.com", "role": "member"},
+        )
+        assert response.status_code == 201
+        user_id = response.json["user"]["id"]
+        response = client.post(
+            FLARE_DASHBOARD_NAMESPACE + "/api/v1/login", json={"email": email, "password": "test-password"}
+        )
+        assert response.status_code == 200
+        yield {"Authorization": "Bearer " + response.json["access_token"]}
+        response = client.delete(FLARE_DASHBOARD_NAMESPACE + f"/api/v1/users/{user_id}", headers=auth_header)
+        assert response.status_code == 200
+
+    @pytest.mark.parametrize("as_admin", [False, True])
+    @pytest.mark.parametrize("props", [{"custom_ca_cert": "/server/private-file"}, {}, None, "invalid", []])
+    def test_create_rejects_props(self, client, auth_header, creator_header, as_admin, props):
+        url = FLARE_DASHBOARD_NAMESPACE + "/api/v1/clients"
+        before = client.get(url, headers=auth_header).json["client_list"]
+        response = client.post(
+            url,
+            json={**CLIENT1, "name": f"props-{uuid4().hex}", "props": props},
+            headers=auth_header if as_admin else creator_header,
+        )
+        try:
+            assert response.status_code == 400
+            assert response.json["status"] == "error"
+            assert "props" in response.json["message"]
+            assert client.get(url, headers=auth_header).json["client_list"] == before
+        finally:
+            if response.status_code == 201:
+                client.delete(url + f"/{response.json['client']['id']}", headers=auth_header)
+
+    @pytest.mark.parametrize("as_admin", [False, True])
+    @pytest.mark.parametrize("approval_state", [0, 100, 200])
+    @pytest.mark.parametrize("props", [{"custom_ca_cert": "/server/private-file"}, {}, None, "invalid", []])
+    def test_patch_rejects_props(self, client, auth_header, creator_header, as_admin, approval_state, props):
+        response = client.post(
+            FLARE_DASHBOARD_NAMESPACE + "/api/v1/clients",
+            json={**CLIENT1, "name": f"props-{uuid4().hex}"},
+            headers=creator_header,
+        )
+        assert response.status_code == 201
+        url = FLARE_DASHBOARD_NAMESPACE + f"/api/v1/clients/{response.json['client']['id']}"
+        try:
+            response = client.patch(url, json={"approval_state": approval_state}, headers=auth_header)
+            assert response.status_code == 200
+            before = client.get(url, headers=auth_header).json["client"]
+            response = client.patch(
+                url,
+                json={"props": props, "name": "changed-name", "approval_state": -1},
+                headers=auth_header if as_admin else creator_header,
+            )
+            assert response.status_code == 400
+            assert response.json["status"] == "error"
+            assert "props" in response.json["message"]
+            assert client.get(url, headers=auth_header).json["client"] == before
+
+            # Ordinary client fields remain editable when props is absent.
+            capacity = {"num_of_gpus": 2, "mem_per_gpu_in_GiB": 16}
+            response = client.patch(url, json={"capacity": capacity}, headers=creator_header)
+            assert response.status_code == 200
+            assert response.json["client"]["capacity"] == capacity
+        finally:
+            response = client.delete(url, headers=auth_header)
+            assert response.status_code == 200
+
+    @pytest.mark.parametrize("malformed_props", [False, True])
+    def test_blob_ignores_legacy_props(
+        self, app, client, auth_header, creator_header, tmp_path, monkeypatch, malformed_props
+    ):
+        secret = b"legacy-client-props-must-not-disclose-this-file"
+        target = tmp_path / "private-file"
+        target.write_bytes(secret)
+        legacy_props = (
+            "invalid JSON"
+            if malformed_props
+            else json.dumps({"custom_ca_cert": str(target), "connection_security": "clear"})
+        )
+        # Operator-managed properties and the client's capacity must still reach the kit.
+        monkeypatch.setenv("NVFL_WEB_ROOT", str(tmp_path))
+        (tmp_path / "properties.json").write_text(json.dumps({"client": {"use_aio": True}}))
+        capacity = {"num_of_gpus": 2, "mem_per_gpu_in_GiB": 16}
+        response = client.post(
+            FLARE_DASHBOARD_NAMESPACE + "/api/v1/clients",
+            json={"name": f"legacy-{uuid4().hex}", "organization": "testorg", "capacity": capacity},
+            headers=creator_header,
+        )
+        assert response.status_code == 201
+        client_id = response.json["client"]["id"]
+        url = FLARE_DASHBOARD_NAMESPACE + f"/api/v1/clients/{client_id}"
+        with app.app_context():
+            project = Project.query.first()
+            original = {key: getattr(project, key) for key in ("short_name", "server1", "root_key", "root_cert")}
+            project.short_name = "legacy-test"
+            project.server1 = "server.test.com"
+            Store.build_project(project)
+            legacy_client = db.session.get(Client, client_id)
+            legacy_client.props = legacy_props
+            legacy_client.approval_state = 200
+            db.session.commit()
+
+        try:
+            response = client.post(url + "/blob", json={"pin": "1234"}, headers=creator_header)
+            assert response.status_code == 200
+            assert response.headers["Content-Type"] == "zip"
+            with ZipFile(io.BytesIO(response.data)) as kit:
+                assert not any(name.endswith("customRootCA.pem") for name in kit.namelist())
+                for name in kit.namelist():
+                    if not name.endswith("/"):
+                        assert secret not in kit.read(name, pwd=b"1234")
+                resources = json.loads(kit.read("local/resources.json.default", pwd=b"1234"))
+                resource_manager = next(c for c in resources["components"] if c["id"] == "resource_manager")
+                for key, value in capacity.items():
+                    assert resource_manager["args"][key] == value
+                config = json.loads(kit.read("startup/fed_client.json", pwd=b"1234"))
+                assert config["client"]["connection_security"] == "mtls"
+                assert config["servers"][0]["service"]["scheme"] == "agrpc"
+
+            with app.app_context():
+                # Safety comes from ignoring stored props, without depending on a migration.
+                assert db.session.get(Client, client_id).props == legacy_props
+        finally:
+            response = client.delete(url, headers=auth_header)
+            assert response.status_code == 200
+            with app.app_context():
+                project = Project.query.first()
+                for key, value in original.items():
+                    setattr(project, key, value)
+                db.session.commit()