From b75863ba5114d52083232a0a2a741f0b1093c1c1 Mon Sep 17 00:00:00 2001 From: Maxim Shulga Date: Sun, 2 Aug 2026 17:29:51 -0700 Subject: [PATCH 1/2] Read the local library list as JSON instead of splitting columns get_local_libraries unpacked "line.rsplit(maxsplit=3)" into three names, but maxsplit=3 yields up to four fields. seaf-cli prints "name id path" separated by whitespace, so any library whose name contains a space produced a fourth field and raised ValueError, which nothing caught. The failure only appeared after the first run: the list is empty until a library has been synced, so a container would work when it was set up and then crash on every restart. seaf-cli list --json returns name, id and path as structured fields, which display output cannot represent unambiguously for names or paths containing whitespace. Its exit code is now checked and unreadable output is reported as a daemon error instead of a traceback. The image smoke check asserts that the installed seaf-cli offers --json and --token, so a future version bump that drops either fails the build rather than the container. --- Dockerfile | 2 ++ IMPROVEMENTS.md | 5 +++- dsc/client.py | 19 ++++++++------ tests/test_commands.py | 56 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 9 deletions(-) diff --git a/Dockerfile b/Dockerfile index 5eec5f0..3646f90 100644 --- a/Dockerfile +++ b/Dockerfile @@ -68,6 +68,8 @@ RUN chmod +x /dsc/start.py && \ # Smoke checks: seaf-cli has no --version flag, so assert its subcommands, and # confirm the app's own imports resolve. RUN seaf-cli --help | grep -q 'download-by-name' && \ + seaf-cli sync --help | grep -q '\--token' && \ + seaf-cli list --help | grep -q '\--json' && \ python3 -c "import seafile, pysearpc, dsc.client, dsc.misc" VOLUME /dsc/seafile-data diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index f300cbe..2330bb4 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -152,7 +152,10 @@ Acceptance criteria: server errors separately. - [ ] Make TLS configuration consistent between Python requests and Seafile. - [ ] Support a custom CA bundle; keep certificate disabling as a last resort. -- [ ] Prefer structured JSON output from `seaf-cli` over parsing display text. +- [x] Prefer structured JSON output from `seaf-cli` over parsing display text. + `seaf-cli list --json` is used instead of splitting the display columns, + which could not represent a library name or a path containing whitespace. + `list-remote` still uses the HTTP API rather than the CLI. - [ ] Reject ambiguous library names and an empty requested library set. ### Runtime hardening diff --git a/dsc/client.py b/dsc/client.py index 757414b..2965f74 100644 --- a/dsc/client.py +++ b/dsc/client.py @@ -1,4 +1,5 @@ import argparse +import json import logging import os import subprocess @@ -271,16 +272,18 @@ class SeafileClient: prev_status[library] = cur_status[library] def get_local_libraries(self) -> set: - cmd = ["seaf-cli", "list"] + # The display output is whitespace separated, so a library name or a + # path containing a space cannot be parsed back apart. Ask for JSON. + cmd = ["seaf-cli", "list", "--json"] _lg.info("Listing local libraries: %s", " ".join(cmd)) - out = subprocess.check_output(self.__gen_cmd(cmd)) - out = out.decode().splitlines()[1:] # first line is a header + out = self.__run(cmd, stdout=subprocess.PIPE).stdout - local_libs = set() - for line in out: - lib_name, lib_id, lib_path = line.rsplit(maxsplit=3) - local_libs.add(lib_id) - return local_libs + try: + libraries = json.loads(out.decode()) + except (UnicodeDecodeError, ValueError) as err: + raise DaemonError(f"Cannot read the library list from seaf-cli: {err}") + + return {lib["id"] for lib in libraries} def configure(self, args: argparse.Namespace, check_for_daemon: bool = True): need_restart = False diff --git a/tests/test_commands.py b/tests/test_commands.py index 497c913..48edf8f 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -1,12 +1,14 @@ """Every subprocess must be an argument list, never a shell string.""" import argparse +import json import subprocess import pytest from dsc import client as client_module from dsc import const +from dsc.errors import DaemonError from dsc.client import SeafileClient from dsc.misc import hide_secrets, user_cmd @@ -157,3 +159,57 @@ def test_configure_passes_option_values_as_arguments(client, calls, monkeypatch) assert prefix + ["-k", "upload_limit", "-v", "100"] in argvs # An option that seaf-cli does not know about is never forwarded. assert not any("ignored" in argv for argv in argvs) + + +def run_returning(monkeypatch, stdout=b"", returncode=0): + """Run seaf-cli commands that produce the given output.""" + calls = [] + + def fake_run(argv, **kwargs): + calls.append(argv) + return subprocess.CompletedProcess(argv, returncode, stdout=stdout) + + monkeypatch.setattr(subprocess, "run", fake_run) + return calls + + +def test_local_libraries_are_read_as_json(client, monkeypatch): + out = json.dumps([ + {"name": "Docs", "id": "id-1", "path": "/dsc/seafile/Docs"}, + ]).encode() + calls = run_returning(monkeypatch, out) + + assert client.get_local_libraries() == {"id-1"} + assert "--json" in calls[-1] + + +@pytest.mark.parametrize("name, path", [ + # Display output is whitespace separated, so these used to be unparseable. + ("My Docs", "/dsc/seafile/My_Docs"), + ("Docs", "/mnt/my libraries/Docs"), + ("My Docs", "/mnt/my libraries/My Docs"), + ("Ünïcödé ☂", "/dsc/seafile/Ünïcödé_☂"), + ("tab\tname", "/dsc/seafile/tab_name"), +]) +def test_local_libraries_survive_whitespace_in_names_and_paths(client, monkeypatch, name, path): + out = json.dumps([{"name": name, "id": "id-1", "path": path}]).encode() + run_returning(monkeypatch, out) + + assert client.get_local_libraries() == {"id-1"} + + +def test_no_local_libraries_gives_an_empty_set(client, monkeypatch): + run_returning(monkeypatch, b"[]") + assert client.get_local_libraries() == set() + + +def test_local_libraries_reports_unreadable_output(client, monkeypatch): + run_returning(monkeypatch, b"not json at all") + with pytest.raises(DaemonError): + client.get_local_libraries() + + +def test_local_libraries_reports_a_failed_command(client, monkeypatch): + run_returning(monkeypatch, b"", returncode=1) + with pytest.raises(DaemonError): + client.get_local_libraries() From 2e18a774f2f9918da504d3c9567f9f73da3499f3 Mon Sep 17 00:00:00 2001 From: Maxim Shulga Date: Sun, 2 Aug 2026 17:33:45 -0700 Subject: [PATCH 2/2] Fail the same way on JSON of an unexpected shape json.loads succeeding does not mean the result is a list of objects with an id. A dict, an entry without an id, or a scalar raised TypeError or KeyError, which escaped unhandled and defeated the point of the change. Both cases now raise DaemonError, and the original exception is chained so the cause is visible. --- dsc/client.py | 13 +++++++++++-- tests/test_commands.py | 22 ++++++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/dsc/client.py b/dsc/client.py index 2965f74..62c56f0 100644 --- a/dsc/client.py +++ b/dsc/client.py @@ -281,9 +281,18 @@ class SeafileClient: try: libraries = json.loads(out.decode()) except (UnicodeDecodeError, ValueError) as err: - raise DaemonError(f"Cannot read the library list from seaf-cli: {err}") + raise DaemonError( + f"Cannot read the library list from seaf-cli: {err}" + ) from err - return {lib["id"] for lib in libraries} + # Valid JSON is not necessarily the expected shape, and an unexpected + # one must fail the same way rather than as a bare TypeError. + try: + return {lib["id"] for lib in libraries} + except (KeyError, TypeError) as err: + raise DaemonError( + f"Unexpected library list from seaf-cli: {err}" + ) from err def configure(self, args: argparse.Namespace, check_for_daemon: bool = True): need_restart = False diff --git a/tests/test_commands.py b/tests/test_commands.py index 48edf8f..082f16f 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -213,3 +213,25 @@ def test_local_libraries_reports_a_failed_command(client, monkeypatch): run_returning(monkeypatch, b"", returncode=1) with pytest.raises(DaemonError): client.get_local_libraries() + + +@pytest.mark.parametrize("payload", [ + b'{"name": "Docs"}', # an object where a list is expected + b'[{"name": "Docs"}]', # an entry with no id + b'["Docs"]', # entries that are not objects + b'[null]', + b'null', + b'42', +]) +def test_local_libraries_reports_unexpected_json_shapes(client, monkeypatch, payload): + """Valid JSON of the wrong shape must not escape as a raw TypeError.""" + run_returning(monkeypatch, payload) + with pytest.raises(DaemonError): + client.get_local_libraries() + + +def test_local_libraries_error_keeps_the_original_cause(client, monkeypatch): + run_returning(monkeypatch, b"not json at all") + with pytest.raises(DaemonError) as err: + client.get_local_libraries() + assert err.value.__cause__ is not None