From 21ef1f257060a192337a0c4d52969f3f22d560be Mon Sep 17 00:00:00 2001 From: Howard Enos Date: Thu, 25 Jun 2026 11:56:32 -0700 Subject: [PATCH] synology: fix --confirm arg position + verify write path live - Fix argparse: --confirm/--vault were only accepted BEFORE the subcommand, so every documented gated-write (e.g. `call X set k=v --confirm`) failed. Moved to a shared parent parser (SUPPRESS defaults) -> both flags work in either position. - Verified the CSRF write path live on cascadesDS: Share create -> verify -> delete -> verify gone. Both mutating calls succeeded; device left pristine. - SKILL.md: write/setter path marked VERIFIED; confirmed share-create signature. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/skills/synology/SKILL.md | 14 +++-- .../skills/synology/scripts/syno_client.py | 55 ++++++++++++------- 2 files changed, 43 insertions(+), 26 deletions(-) diff --git a/.claude/skills/synology/SKILL.md b/.claude/skills/synology/SKILL.md index eefa89d3..6497991e 100644 --- a/.claude/skills/synology/SKILL.md +++ b/.claude/skills/synology/SKILL.md @@ -129,16 +129,20 @@ VPN-down connect error surfaced to the user, a method refused for lack of --conf --confirm`. (`ls ` now catches this and prints the SSH hint.) Windows note: a bare `ls /Public` arg is also rewritten by MSYS path-conversion — prefer `MSYS_NO_PATHCONV=1` or the `call ... folder_path=/x` form, but the 407 denial blocks it regardless. -- **WRITE/SETTER PATH NOT YET EXERCISED LIVE.** Session-expiry re-login (106/119), the mutating-verb - gate, and read-modify-write setters are coded to spec but no mutating call has been made against - the device yet. Gate is verified only on the read path. First real change will confirm CSRF on a - write end-to-end. +- **WRITE/SETTER PATH VERIFIED 2026-06-25.** Live round-trip on cascadesDS: `SYNO.Core.Share create` + (hidden throwaway share) -> confirmed in `shares` -> `SYNO.Core.Share delete` -> confirmed gone. + Both mutating calls passed CSRF (synotoken) end-to-end and returned success; device left pristine. + The mutating-verb gate correctly blocks without `--confirm` and allows with it. (Session-expiry + re-login 106/119 still not forced live, but the auth/CSRF/setter plumbing is now proven.) + Share create signature confirmed: `name= 'shareinfo:={"name":..,"vol_path":"/volume1",..}'`. - Built originally from a 5-agent scan of the DSM 7 help tree + authoritative API sources (kwent `_full.json`, N4S4/synology-api, Synology Web API guides) — see `references/dsm-api.md`. - **Live fixes from this run (2026-06-25):** `services` used `SYNO.Core.Service.list` which 103s on DSM 7.2.1 — corrected to `get` (returns `{"service":[...]}`). `apis | head` raised a `BrokenPipeError` traceback — now caught and exits cleanly. `ls ` now degrades to an SSH - hint on the FileStation 400/407 denial. + hint on the FileStation 400/407 denial. **`--confirm`/`--vault` after the subcommand were rejected + by argparse** (every documented gated-write example, e.g. `call X set k=v --confirm`, would have + failed) — moved to a shared parent parser so both flags now work before AND after the subcommand. - **DSM 7.2.x reboot/shutdown 103:** the Web-API `reboot`/`shutdown` can return error 103 on some builds. Fallback is the SSH `reboot|shutdown --confirm` recipe (`synoshutdown -r|-s`). - **Param-schema caveat:** method names + versions in the reference are device-authoritative, but many diff --git a/.claude/skills/synology/scripts/syno_client.py b/.claude/skills/synology/scripts/syno_client.py index b446be85..efcc5ac1 100644 --- a/.claude/skills/synology/scripts/syno_client.py +++ b/.claude/skills/synology/scripts/syno_client.py @@ -256,27 +256,38 @@ def _kv(pairs): def main(argv): - ap = argparse.ArgumentParser(prog="syno", description="Synology DSM Web API client") - ap.add_argument("--vault", default=DEFAULT_VAULT, help="SOPS vault entry for the NAS") - ap.add_argument("--confirm", action="store_true", help="authorize a mutating call") + # --vault / --confirm live on a shared parent so they are accepted BOTH before the + # subcommand and after it (the SKILL docs put --confirm at the end, e.g. + # `call X set k=v --confirm`). SUPPRESS defaults stop the subparser copy from + # clobbering a value parsed by the main parser. + common = argparse.ArgumentParser(add_help=False) + common.add_argument("--vault", default=argparse.SUPPRESS, help="SOPS vault entry for the NAS") + common.add_argument("--confirm", action="store_true", default=argparse.SUPPRESS, + help="authorize a mutating call") + + ap = argparse.ArgumentParser(prog="syno", parents=[common], + description="Synology DSM Web API client") sub = ap.add_subparsers(dest="cmd", required=True) - sub.add_parser("test", help="login and report DSM identity") - pa = sub.add_parser("apis", help="list the device's API map (what you can control)") + def add(name, **kw): + return sub.add_parser(name, parents=[common], **kw) + + add("test", help="login and report DSM identity") + pa = add("apis", help="list the device's API map (what you can control)") pa.add_argument("filter", nargs="?", help="case-insensitive substring filter") - sub.add_parser("sysinfo", help="model/serial/RAM/temp/uptime (SYNO.Core.System)") - sub.add_parser("util", help="live CPU/mem/disk/net (SYNO.Core.System.Utilization)") - sub.add_parser("storage", help="volumes/disks/RAID/usage (SYNO.Storage.CGI.Storage)") - sub.add_parser("shares", help="shared folders (SYNO.Core.Share)") - sub.add_parser("users", help="local users (SYNO.Core.User)") - sub.add_parser("groups", help="local groups (SYNO.Core.Group)") - sub.add_parser("packages", help="installed packages (SYNO.Core.Package)") - sub.add_parser("services", help="services (SYNO.Core.Service)") - sub.add_parser("connections", help="current connections (SYNO.Core.CurrentConnection)") - pl = sub.add_parser("ls", help="FileStation list (no path = shares)") + add("sysinfo", help="model/serial/RAM/temp/uptime (SYNO.Core.System)") + add("util", help="live CPU/mem/disk/net (SYNO.Core.System.Utilization)") + add("storage", help="volumes/disks/RAID/usage (SYNO.Storage.CGI.Storage)") + add("shares", help="shared folders (SYNO.Core.Share)") + add("users", help="local users (SYNO.Core.User)") + add("groups", help="local groups (SYNO.Core.Group)") + add("packages", help="installed packages (SYNO.Core.Package)") + add("services", help="services (SYNO.Core.Service)") + add("connections", help="current connections (SYNO.Core.CurrentConnection)") + pl = add("ls", help="FileStation list (no path = shares)") pl.add_argument("path", nargs="?") - pc = sub.add_parser("call", help="generic: call ANY API method (the power tool)") + pc = add("call", help="generic: call ANY API method (the power tool)") pc.add_argument("api"); pc.add_argument("method") pc.add_argument("--version", type=int) pc.add_argument("--post", action="store_true") @@ -285,19 +296,21 @@ def main(argv): # gated convenience writes for name, helptxt in (("pkg-start", "start a package by id"), ("pkg-stop", "stop a package by id")): - pp = sub.add_parser(name, help=helptxt + " (needs --confirm)") + pp = add(name, help=helptxt + " (needs --confirm)") pp.add_argument("id") - sub.add_parser("reboot", help="reboot the NAS (needs --confirm)") - sub.add_parser("shutdown", help="shut down the NAS (needs --confirm)") + add("reboot", help="reboot the NAS (needs --confirm)") + add("shutdown", help="shut down the NAS (needs --confirm)") args = ap.parse_args(argv) + vault = getattr(args, "vault", DEFAULT_VAULT) + confirm = getattr(args, "confirm", False) def guard(method): - if not args.confirm: + if not confirm: raise SynoError(f"'{method}' mutates the device -- re-run with --confirm") try: - c = SynoClient(vault=args.vault) + c = SynoClient(vault=vault) if args.cmd == "test": c.login() d = c.call("SYNO.Core.System", "info")