Fix six audit findings: dead code, missing status records, missing fallback, unsafe JSON read
create_task.py: - Remove dead make_client() call before the verify branch; it was called unconditionally with host=None/key=None, creating a broken client that was immediately discarded or overwritten. - Remove "verify" from the dispatch dict; it was never reached through dispatch (the if/cmd==verify branch above it handled it). Dispatch now only contains commands that actually use a client. - Wrap json.load() in try/except (OSError, JSONDecodeError) so a corrupt or partially-written status file produces a useful message instead of a traceback. sitecustomize.py: - Call _record_status() on the early-return paths in both _patch_b2 and _patch_restic. Without this, if TrueNAS natively supports B2 or the patch is already applied, the status file was never written and `verify` always reported failure even when everything was fine. - Add idempotency guard to _record_status(): first call wins; duplicate calls for the same module are ignored so the entry count stays accurate. - Make B2 get_restic_config a @staticmethod. The method never used self; the noqa comment was suppressing the evidence of a design mismatch. Removing the unused parameter makes the intent explicit. - Add NamedTuple._replace() fallback after dataclasses.replace() in the restic wrapper. If ResticConfig is ever refactored to a NamedTuple, the TypeError from dataclasses.replace() would have surfaced as a backup job failure rather than a graceful recovery.
This commit is contained in:
+17
-12
@@ -92,8 +92,14 @@ def cmd_verify(_client, _args):
|
||||
print(f" Expected: {_STATUS_FILE}")
|
||||
sys.exit(1)
|
||||
|
||||
with open(_STATUS_FILE, encoding="utf-8") as fh:
|
||||
status = json.load(fh)
|
||||
try:
|
||||
with open(_STATUS_FILE, encoding="utf-8") as fh:
|
||||
status = json.load(fh)
|
||||
except (OSError, json.JSONDecodeError) as exc:
|
||||
print(f"Could not read status file: {exc}")
|
||||
print(f" Path: {_STATUS_FILE}")
|
||||
print("Try restarting middlewared to regenerate it.")
|
||||
sys.exit(1)
|
||||
|
||||
print(f"Hook status (recorded at {status.get('patched_at', 'unknown')})")
|
||||
print()
|
||||
@@ -225,22 +231,21 @@ def main():
|
||||
help="Create the task in a disabled state")
|
||||
|
||||
args = p.parse_args()
|
||||
client = make_client(args.host, args.api_key, args.insecure)
|
||||
|
||||
if args.cmd == "verify":
|
||||
cmd_verify(None, args)
|
||||
return
|
||||
|
||||
if not args.host or not args.api_key:
|
||||
p.error("--host and --api-key are required for this command")
|
||||
|
||||
client = make_client(args.host, args.api_key, args.insecure)
|
||||
dispatch = {
|
||||
"verify": cmd_verify,
|
||||
"list-credentials": cmd_list_credentials,
|
||||
"list-tasks": cmd_list_tasks,
|
||||
"create": cmd_create,
|
||||
}
|
||||
|
||||
if args.cmd == "verify":
|
||||
cmd_verify(None, args)
|
||||
else:
|
||||
if not args.host or not args.api_key:
|
||||
p.error("--host and --api-key are required for this command")
|
||||
client = make_client(args.host, args.api_key, args.insecure)
|
||||
dispatch[args.cmd](client, args)
|
||||
dispatch[args.cmd](client, args)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
|
||||
+15
-5
@@ -121,9 +121,12 @@ def _patch_b2(module):
|
||||
|
||||
if hasattr(cls, "get_restic_config"):
|
||||
# A future TrueNAS version already added native B2 restic support.
|
||||
_record_status("middlewared.rclone.remote.b2", ok=True,
|
||||
detail="native support present; patch not needed")
|
||||
return
|
||||
|
||||
def get_restic_config(self, task): # noqa: ARG001
|
||||
@staticmethod
|
||||
def get_restic_config(task):
|
||||
p = task["credentials"]["provider"]
|
||||
return "", {
|
||||
"B2_ACCOUNT_ID": p["account"],
|
||||
@@ -138,6 +141,8 @@ def _patch_b2(module):
|
||||
|
||||
def _patch_restic(module):
|
||||
if getattr(module.get_restic_config, "_truecloud_patched", False):
|
||||
_record_status("middlewared.plugins.cloud_backup.restic", ok=True,
|
||||
detail="already patched in this process")
|
||||
return
|
||||
|
||||
import dataclasses
|
||||
@@ -163,10 +168,13 @@ def _patch_restic(module):
|
||||
m = _broken_url.match(part)
|
||||
if m:
|
||||
cmd[i] = f"{m.group(1)}:{m.group(2)}"
|
||||
# dataclasses.replace passes all other fields through, so
|
||||
# new ResticConfig fields added in future TrueNAS versions
|
||||
# are preserved automatically.
|
||||
return dataclasses.replace(result, cmd=cmd)
|
||||
# Prefer dataclasses.replace (passes unknown future fields
|
||||
# through automatically). Fall back to NamedTuple._replace
|
||||
# in case ResticConfig is refactored.
|
||||
try:
|
||||
return dataclasses.replace(result, cmd=cmd)
|
||||
except TypeError:
|
||||
return result._replace(cmd=cmd)
|
||||
break # -r arg present and already correct
|
||||
return result # URL was fine; return original unchanged
|
||||
|
||||
@@ -190,6 +198,8 @@ def _record_status(fullname: str, ok: bool, detail: str = "") -> None:
|
||||
import os
|
||||
import time
|
||||
|
||||
if fullname in _hook_status:
|
||||
return # idempotent: first call wins
|
||||
_hook_status[fullname] = {"ok": ok, "detail": detail}
|
||||
if len(_hook_status) < len(_PATCHES):
|
||||
return # wait until all patches have reported
|
||||
|
||||
Reference in New Issue
Block a user