From eb47e1262f317649a777e7a82c8636c18928131a Mon Sep 17 00:00:00 2001 From: Vlad Doloman Date: Wed, 24 Jun 2026 11:40:11 +0300 Subject: [PATCH] fix: address 7 consistency findings from code review - Counter overflow: drop len==2 guard in build_ovpn so suffixes grow naturally beyond _99 instead of silently resetting to _00 - TOGGLE_ALL: preserve checked set and cursor across A-key view switches by threading saved_checked/saved_cursor through ScreenResult and back into show_main_screen on the next call - CLI Cryptgeon fallback: skip email/eml when Cryptgeon fails instead of embedding the raw plaintext password in the email body - --show-eml silent skip: warn explicitly when --show-eml is requested but no email address is available - URL truncation: show full one-time URL in TUI confirmation dialog - _fatal(): remove misleading alias; inline self._error() + return at the sole call site so termination is explicit - argparse: replace store_true/store_false with store_const for the three-state --send-email / --no-send-email / neither pattern Co-Authored-By: Claude Sonnet 4.6 --- openvpncertupdate.py | 74 ++++++++++++++++++++++++++++---------------- 1 file changed, 47 insertions(+), 27 deletions(-) diff --git a/openvpncertupdate.py b/openvpncertupdate.py index 812477b..102e343 100644 --- a/openvpncertupdate.py +++ b/openvpncertupdate.py @@ -277,7 +277,7 @@ def build_ovpn( int(d.name[len(prefix):]) for d in base.iterdir() if d.is_dir() and d.name.startswith(prefix) - and d.name[len(prefix):].isdigit() and len(d.name[len(prefix):]) == 2 + and d.name[len(prefix):].isdigit() ] if base.exists() else [] next_n = max(existing) + 1 if existing else 0 out_dir = base / f"{prefix}{next_n:02d}" @@ -661,6 +661,8 @@ class Action(Enum): class ScreenResult: action: Action selected_cns: list[str] = field(default_factory=list) + saved_checked: list[str] = field(default_factory=list) + saved_cursor: int = 0 _MENU_NEW = "Create New Certificate" @@ -678,7 +680,9 @@ def _expiry_label(c: CertInfo) -> str: def show_main_screen( - stdscr, certs: list[CertInfo], show_all: bool = False + stdscr, certs: list[CertInfo], show_all: bool = False, + initial_checked: Optional[set[str]] = None, + initial_cursor: int = 0, ) -> ScreenResult: """Display cert selection list. Returns when user activates an action.""" init_colors() @@ -686,8 +690,8 @@ def show_main_screen( n_certs = len(certs) n_items = n_certs + len(_MENU_ITEMS) - checked: set[str] = set() - cursor = 0 + checked: set[str] = set(initial_checked) if initial_checked else set() + cursor = clamp(initial_cursor, 0, max(0, n_items - 1)) def is_cert(idx): return idx < n_certs def menu_item(idx): return _MENU_ITEMS[idx - n_certs] if idx >= n_certs else None @@ -786,7 +790,9 @@ def show_main_screen( # _MENU_NEW when checked → grayed, ignore elif key in (ord("a"), ord("A")): - return ScreenResult(action=Action.TOGGLE_ALL) + return ScreenResult(action=Action.TOGGLE_ALL, + saved_checked=sorted(checked), + saved_cursor=cursor) elif key in (ord("r"), ord("R")) and is_cert(cursor): cert = certs[cursor] @@ -811,6 +817,8 @@ class CursesApp: init_colors() show_all = False + saved_checked: list[str] = [] + saved_cursor: int = 0 while True: try: @@ -820,15 +828,21 @@ class CursesApp: else load_expiring_certs(EASYRSA_PKI_DIR, DAYS_PAST, DAYS_AHEAD) ) except FileNotFoundError as exc: - self._fatal(stdscr, f"Cannot read PKI index.txt:\n{exc}") + self._error(stdscr, f"Cannot read PKI index.txt:\n{exc}") return - result = show_main_screen(stdscr, certs, show_all=show_all) + result = show_main_screen(stdscr, certs, show_all=show_all, + initial_checked=set(saved_checked), + initial_cursor=saved_cursor) + saved_checked = [] + saved_cursor = 0 if result.action == Action.EXIT: return elif result.action == Action.TOGGLE_ALL: show_all = not show_all + saved_checked = result.saved_checked + saved_cursor = result.saved_cursor elif result.action == Action.REGEN_CRL: self._regen_crl(stdscr) elif result.action == Action.REVOKE: @@ -900,7 +914,7 @@ class CursesApp: stdscr, f"Send email to {form.email}?\n\n" f"Attachment : {os.path.basename(ovpn_path)}\n" - f"Password URL: {one_time_url[:50]}…", + f"Password URL: {one_time_url}", ): try: send_email(form.email, final_cn, one_time_url, ovpn_path, @@ -980,9 +994,6 @@ class CursesApp: stdscr.refresh() stdscr.getch() - def _fatal(self, stdscr, text: str) -> None: - self._error(stdscr, text) - def _show_result( self, stdscr, cn: str, ovpn_path: str, password: str, one_time_url: str, ) -> None: @@ -1083,22 +1094,31 @@ class CliRunner: except CryptgeonError as exc: print(f"warning: Cryptgeon failed: {exc}", file=sys.stderr) - if show_eml and email_addr: - try: - msg = build_mime_message(email_addr, cn, one_time_url or password, - ovpn_path, MAIL_FROM, MAIL_SUBJECT, - EMAIL_TEMPLATE_PATH) - print(base64.b64encode(msg.as_bytes()).decode()) - except Exception as exc: - print(f"warning: could not build .eml: {exc}", file=sys.stderr) + if show_eml: + if not email_addr: + print("warning: --show-eml skipped (no email address)", file=sys.stderr) + elif not one_time_url: + print("warning: --show-eml skipped (Cryptgeon failed)", file=sys.stderr) + else: + try: + msg = build_mime_message(email_addr, cn, one_time_url, + ovpn_path, MAIL_FROM, MAIL_SUBJECT, + EMAIL_TEMPLATE_PATH) + print(base64.b64encode(msg.as_bytes()).decode()) + except Exception as exc: + print(f"warning: could not build .eml: {exc}", file=sys.stderr) if email_addr and send_email_flag: - try: - send_email(email_addr, cn, one_time_url or password, ovpn_path, - MAIL_FROM, MAIL_SUBJECT, EMAIL_TEMPLATE_PATH, MAIL_BINARY) - print(f"Email sent to {email_addr}", file=sys.stderr) - except RuntimeError as exc: - print(f"warning: email failed: {exc}", file=sys.stderr) + if not one_time_url: + print("warning: email skipped (Cryptgeon failed; use password from stdout)", + file=sys.stderr) + else: + try: + send_email(email_addr, cn, one_time_url, ovpn_path, + MAIL_FROM, MAIL_SUBJECT, EMAIL_TEMPLATE_PATH, MAIL_BINARY) + print(f"Email sent to {email_addr}", file=sys.stderr) + except RuntimeError as exc: + print(f"warning: email failed: {exc}", file=sys.stderr) print(f"config: {ovpn_path}") if one_time_url: @@ -1119,9 +1139,9 @@ def _build_parser() -> argparse.ArgumentParser: group.add_argument("--gen-crl", action="store_true", help="Regenerate and copy CRL") parser.add_argument("--email", metavar="EMAIL", help="Email address (required for --create; optional for --reissue)") - parser.add_argument("--send-email", dest="send_email", action="store_true", + parser.add_argument("--send-email", dest="send_email", action="store_const", const=True, default=None, help="Send email after issuing cert (default unless --show-eml)") - parser.add_argument("--no-send-email", dest="send_email", action="store_false", + parser.add_argument("--no-send-email", dest="send_email", action="store_const", const=False, help="Skip email delivery; print URL to stdout instead") parser.add_argument("--show-eml", action="store_true", help="Print the generated email as base64-encoded .eml to stdout "