From c8d144ca9d8ef9fc8cb522814c036d47b49ab88b Mon Sep 17 00:00:00 2001 From: taylanbakircioglu Date: Fri, 3 Jul 2026 02:07:43 +0300 Subject: [PATCH] fix(acme): remove stray paren breaking ACME completion task (v1.8.5, Issue #35) The background order-completion task (complete_pending_acme_orders, 60s cycle) failed on EVERY cycle since v1.8.0 with: [ACME-COMPLETE] Error in completion task: syntax error at or near ")" Root cause: the bounded DNS-01 retry OR-arm added to the atomic order-claim query in v1.8.0 (13b65d9) carried one extra closing parenthesis, making the whole SELECT invalid PostgreSQL. The claim is the task's first statement, so the generic except swallowed it each minute and NO background ACME work ever ran on v1.8.0-v1.8.4: - orders were never claimed for finalize -> download -> save (http-01 too); - advance_dns01_order never ran, so the DNS-01 TXT record was never published - DNS-01 with an automated provider (e.g. Cloudflare) could never validate (exactly the report in Issue #35); - wizard-staged orders never left wizard_staged (same try block); - retry_invalid_dns01 / reconcile_dns01_cleanup never executed; - hourly-created renewal orders could never complete in the background. Fix: drop the stray ')' (one line). Query semantics are unchanged. Why the suite missed it: the unit tests mock asyncpg, so raw SQL never reaches a real parser. Added a regression test that AST-scans the ACME modules' SQL string literals (comments/quoted literals stripped) and fails on unbalanced parentheses - it is red on the pre-fix tree and would have caught the v1.8.0 regression at commit time. Scanned all six ACME modules: this query was the only unbalanced SQL. Verification: full backend suite in docker green (1062 passed, 151 skipped); the fixed query EXPLAINs cleanly on postgres:15; live localtest run shows zero completion-task errors and a seeded pending order is claimed ("[ACME-COMPLETE] Claimed 1 order(s)"). Backend-only, no schema/API/agent changes; fully backward compatible. Reported-by: @tkkost (GitHub Issue #35) --- README.md | 1 + backend/main.py | 1 - backend/tests/test_dns01.py | 108 ++++++++++++++++++++++++++++++++++++ frontend/package.json | 2 +- version.json | 6 +- 5 files changed, 113 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index d82e2a1..65e68d5 100644 --- a/README.md +++ b/README.md @@ -2415,6 +2415,7 @@ Developed with ❤️ for the HAProxy community ## Release Notes +- **v1.8.5** (2026-07-03) — **ACME completion-task SQL fix** (Issue #35 follow-up): the background order-completion task (`complete_pending_acme_orders`, runs every 60s) died on **every cycle** with `syntax error at or near ")"` — an extra closing parenthesis introduced in v1.8.0's bounded DNS-01 retry claim query. Because that query is the task's first database call, **no background ACME work ran at all from v1.8.0 through v1.8.4**: orders were never claimed for finalize/download, the DNS-01 TXT record was never published (so DNS-01 with an automated provider such as Cloudflare could never validate), Site Wizard staged orders never left `wizard_staged`, and DNS-01 retry/TXT-cleanup never executed. The stray parenthesis is removed and a regression test now scans all ACME modules' SQL for unbalanced parentheses (the unit suite mocks the database, which is why a raw-SQL syntax error could slip through). One-line backend query fix; no schema, API, or agent changes — fully backward compatible. - **v1.8.4** (2026-06-27) — **Agent installer self-kill fix** (Issue #31): the Linux/macOS agent installer could abort during "pre-installation cleanup" (terminal showed `Killing processes matching: haproxy-agent` then `Killed`) when the install script's own filename contained "haproxy-agent". The cleanup killed processes by matching the bare string "haproxy-agent" against full command lines, which also matched the running installer (and a `sudo`/PAM ancestor the self-exclusion did not cover), so the installer terminated itself. Cleanup now targets only the installed agent (the `$INSTALL_DIR/haproxy-agent` binary and the agent service), never the bare string, and the UI now names the downloaded scripts `install-agent-.sh` / `uninstall-agent-.sh`. Installer-only change; the running agent and its privilege model (it runs as root for HAProxy reload, config writes, keepalived, and self-upgrade) are unchanged. - **v1.8.3** (2026-06-25) — **Agent heartbeat JSON fix** (Issue #31): a self-hosted agent could fail every heartbeat with `HTTP 400 Invalid JSON: Expecting property name enclosed in double quotes` when the system-info block it collects came back empty on an unusual host, leaving a stray comma in the hand-built heartbeat JSON. The agent script now substitutes a valid placeholder when that block is empty so it can no longer emit a stray comma, and the backend heartbeat endpoint now parses valid payloads as-is and, only when a body fails to parse, tolerates that specific malformed pattern (a leading or doubled comma) so an already-deployed agent recovers on its next heartbeat after this build is deployed. Backend + agent-script only; healthy agents of every version are byte-for-byte unaffected. - **v1.8.2** (2026-06-25) — **ACME nonce fix** (Issue #35 follow-up): the ACME client now scopes the anti-replay nonce **per certificate authority** so a nonce issued by one CA is never sent to another. This fixes ZeroSSL/Google account registration failing with `malformed: The Replay Nonce could not be base64url-decoded` (the client previously shared one nonce across CAs and only auto-retried on `badNonce`). Account registration now always uses a fresh nonce from the target CA, and the retry covers this case too. Backend-only; HTTP-01 and Let's Encrypt are unaffected. diff --git a/backend/main.py b/backend/main.py index bab3fd1..e916e6a 100644 --- a/backend/main.py +++ b/backend/main.py @@ -318,7 +318,6 @@ async def complete_pending_acme_orders(): OR dns01_last_attempt_at < NOW() - ( (CASE COALESCE(dns01_attempts, 0) WHEN 0 THEN 15 WHEN 1 THEN 30 ELSE 60 END) || ' minutes')::INTERVAL - ) ) ) ) diff --git a/backend/tests/test_dns01.py b/backend/tests/test_dns01.py index cabe871..43d2d20 100644 --- a/backend/tests/test_dns01.py +++ b/backend/tests/test_dns01.py @@ -110,3 +110,111 @@ def test_nonce_scoped_per_directory(): assert got == "NONCE_A" # returns THIS CA's nonce assert svc._nonce_by_dir.get("https://a.example/dir") is None # consumed (single-use) assert svc._nonce_by_dir.get("https://b.example/dir") == "NONCE_B" # the other CA is untouched + + +def _sql_paren_depth(sql: str): + """Parenthesis depth of a SQL string, counting only OUTSIDE '...' literals (with '' + escapes), `--` line comments and /* */ block comments. Single-pass state machine so a + `--` inside a literal or a `'` inside a comment cannot corrupt the count. Dollar-quoted + strings are out of scope (not used in this codebase). Returns (final_depth, min_depth). + """ + depth = 0 + min_depth = 0 + state = "normal" + i, n = 0, len(sql) + while i < n: + ch = sql[i] + nxt = sql[i + 1] if i + 1 < n else "" + if state == "normal": + if ch == "'": + state = "string" + elif ch == "-" and nxt == "-": + state = "line_comment" + i += 1 + elif ch == "/" and nxt == "*": + state = "block_comment" + i += 1 + elif ch == "(": + depth += 1 + elif ch == ")": + depth -= 1 + min_depth = min(min_depth, depth) + elif state == "string": + if ch == "'": + if nxt == "'": + i += 1 # escaped '' stays inside the literal + else: + state = "normal" + elif state == "line_comment": + if ch == "\n": + state = "normal" + else: # block_comment + if ch == "*" and nxt == "/": + state = "normal" + i += 1 + i += 1 + return depth, min_depth + + +def test_acme_sql_parentheses_balanced(): + """Issue #35 v1.8.5: the completion task's order-claim query shipped (v1.8.0-v1.8.4) with an + extra closing parenthesis, so EVERY 60s cycle died with `syntax error at or near ")"` and no + background ACME work (claim/finalize/download, DNS-01 publish, wizard-staged promotion, + retry, TXT cleanup) ever ran. The suite never caught it because the DB layer is mocked and + raw SQL never reaches a real parser. This guard scans the ACME modules' SQL string literals + for unbalanced parentheses. + + Guard scope is deliberately conservative to avoid false positives on production changes: + keyword matching is case-sensitive (SQL is uppercase in this codebase; prose in docstrings + is not) and f-string fragments are excluded (they split at `{`, so a fragment may be + legitimately unbalanced). + """ + import ast + import re + + backend_dir = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + modules = [ + "main.py", + os.path.join("services", "dns01_orchestrator.py"), + os.path.join("services", "acme_service.py"), + os.path.join("services", "letsencrypt_service.py"), + os.path.join("routers", "letsencrypt.py"), + os.path.join("routers", "acme_diagnostics.py"), + ] + problems = [] + for rel in modules: + with open(os.path.join(backend_dir, rel), encoding="utf-8") as fh: + tree = ast.parse(fh.read()) + fstring_parts = { + id(const) + for joined in ast.walk(tree) if isinstance(joined, ast.JoinedStr) + for const in ast.walk(joined) if isinstance(const, ast.Constant) + } + for node in ast.walk(tree): + if not (isinstance(node, ast.Constant) and isinstance(node.value, str)): + continue + if id(node) in fstring_parts: + continue + sql = node.value + if not re.search(r"\b(SELECT|INSERT|UPDATE|DELETE)\b", sql): + continue + if not re.search(r"\b(FROM|INTO|SET|WHERE)\b", sql): + continue + depth, min_depth = _sql_paren_depth(sql) + if depth != 0 or min_depth < 0: + problems.append(f"{rel}:{node.lineno} (paren depth {depth:+d}, min {min_depth})") + assert not problems, f"Unbalanced parentheses in SQL literal(s): {problems}" + + +def test_sql_paren_depth_scanner(): + # The guard's scanner itself: parens in literals/comments must not count; '' escapes and + # block comments handled; an extra ')' is reported via min_depth even if a later '(' would + # re-balance the total. + assert _sql_paren_depth("SELECT (1)") == (0, 0) + assert _sql_paren_depth("SELECT (1))") == (-1, -1) # the v1.8.0 bug shape + assert _sql_paren_depth("SELECT ')' , '((' FROM t") == (0, 0) # literals ignored + assert _sql_paren_depth("SELECT 'it''s ))' FROM t") == (0, 0) # '' escape stays inside + assert _sql_paren_depth("SELECT 1 -- comment ) (\nFROM t") == (0, 0) # line comment ignored + assert _sql_paren_depth("SELECT 1 /* ) */ FROM t") == (0, 0) # block comment ignored + assert _sql_paren_depth("SELECT 'a--b' AND (x=1\n)") == (0, 0) # -- inside literal is data + assert _sql_paren_depth("WHERE x) AND (y") == (0, -1) # net 0 but went negative diff --git a/frontend/package.json b/frontend/package.json index 9b222b3..b8960e1 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "haproxy-openmanager-frontend", - "version": "1.8.4", + "version": "1.8.5", "description": "HAProxy Load Balancer Management UI", "license": "AGPL-3.0-or-later", "dependencies": { diff --git a/version.json b/version.json index eeee28c..922d8f0 100644 --- a/version.json +++ b/version.json @@ -1,5 +1,5 @@ { - "version": "1.8.4", - "releaseName": "Agent installer self-kill fix", - "releaseDate": "2026-06-27" + "version": "1.8.5", + "releaseName": "ACME completion-task SQL fix", + "releaseDate": "2026-07-03" }