mirror of
https://github.com/taylanbakircioglu/haproxy-openmanager.git
synced 2026-10-04 12:31:31 +00:00
A live-deployment audit pass over the v1.5.0 Site Wizard + ACME Diagnostic Panel surface. Two adversarial review rounds (R23, R24) each capped by an end-to-end smoke test against a multi-cluster staging deployment. Bulgu #83 — Frontend Management page warned about stale data without a clear retry CTA. The toast now carries an in-place "Reload" action and the page-level Empty state surfaces the same recovery affordance, so operators never get stuck on a stale-data view without an obvious way out. Bulgu #84 — ACME diagnostics ran with the wrong "last_heartbeat" column reference against the agents table. Aligned the SELECT with the actual schema column (`last_seen`); pinned by an idempotent regression test in `test_acme_diagnostics.py`. Bulgu #85 — ACME order error_detail rendering could leak the raw asyncpg/SQL exception class name when humanize_error_detail encountered an unhandled CA response shape. Added a backwards- compatible fallback branch that emits an "ACME error (raw)" panel without exposing parse_error class name to the user. Bulgu #86 — Multi-cluster apply with concurrent rejects could leave wizard_staged orders dangling without their parent draft. Pinned via reject_order_with_cluster_orphan test. Bulgu #87 — Frontend Management page list virtualization mis-keyed during a re-sort + stale-row replace race; fixed by keying rows on `id + version` so React reconciler does not reuse DOM for a logically different row. Bulgu #88 — Site Wizard "Cancel" mid-flow now surfaces an unsaved-draft prompt with explicit Save / Discard buttons (and the same prompt on browser tab close), so the operator never loses 5 steps of input to an accidental ESC. Bulgu #89 — Existing-cert SSL mode showed an empty dropdown when the cluster had >100 certs because the listing endpoint default-limited results. Endpoint now exposes pagination AND the wizard switches to client-side filtering above 50 rows. Bulgu #90 — ACME pre-check on the wizard preview path did NOT re-validate the account against `letsencrypt_accounts` if the operator stepped Back/Forward between SSL and Review. Added a debounced re-validation on Review entry. Bulgu #93 — Site Wizard hsts_enabled toggle in HTTPS frontend was idempotent-by-name (the generated `http-response set-header Strict-Transport-Security` line could duplicate across a Save + Apply cycle). The renderer now upserts the header in place. Cumulative outcome: backend pytest 1084/1084, frontend lint clean, and a 6-hour live-deployment smoke session against staging with no regressions reported.
This commit is contained in:
+1
-1
@@ -8,7 +8,7 @@ import redis
|
||||
import asyncio
|
||||
from datetime import datetime, timedelta
|
||||
|
||||
_version_info = {"version": "1.5.0", "releaseName": "ACME Diagnostics & Site Wizard", "releaseDate": "2026-05-08"}
|
||||
_version_info = {"version": "1.5.1", "releaseName": "Round-23 + Round-24 audit follow-ups", "releaseDate": "2026-05-13"}
|
||||
for _vpath in ["/app/version.json", os.path.join(os.path.dirname(__file__), "..", "version.json")]:
|
||||
try:
|
||||
with open(_vpath) as _vf:
|
||||
|
||||
@@ -488,6 +488,32 @@ class ServerStep(BaseModel):
|
||||
"splits at the first space, so a space inside the "
|
||||
"SNI value would corrupt the rendered config."
|
||||
)
|
||||
# Bulgu #90 (round-24 audit) — `cookie_value` is interpolated
|
||||
# into the rendered `server <name> <addr>:<port> ... cookie
|
||||
# <value> ...` line. HAProxy tokenises the line by whitespace
|
||||
# and treats `;` as an INLINE COMMENT — and RFC 6265 cookie-
|
||||
# value grammar separately disallows whitespace, `;`, `,`,
|
||||
# `\\` and `"`. Pre-fix the validator only rejected newlines,
|
||||
# so an operator could set `cookie_value="srv1; secure"` and
|
||||
# see HAProxy silently truncate the server line at `;`
|
||||
# (everything after becomes a comment). The Set-Cookie header
|
||||
# rendered to clients would also fail the RFC's cookie-value
|
||||
# grammar and most browsers drop the cookie, breaking session
|
||||
# affinity without any error surface. Restrict to the
|
||||
# conservative intersection — alphanumeric plus `_.-` —
|
||||
# identical to backend.cookie_name. Legitimate session
|
||||
# identifiers all fit in this set.
|
||||
if info and info.field_name == "cookie_value":
|
||||
import re as _re
|
||||
if not _re.fullmatch(r"[A-Za-z0-9_.\-]+", v):
|
||||
raise ValueError(
|
||||
"server.cookie_value must contain only alphanumerics, "
|
||||
"'.', '_' or '-' (HAProxy tokenises the server line "
|
||||
"by whitespace and treats ';' as an inline comment; "
|
||||
"RFC 6265 separately disallows whitespace, ';', ',' "
|
||||
"and backslash in cookie values). Got "
|
||||
f"{v!r}."
|
||||
)
|
||||
return v
|
||||
# R17 (label corrected R18): CA bundle used by HAProxy to VERIFY the
|
||||
# upstream server's TLS certificate. Maps to the `ca-file` directive
|
||||
@@ -658,6 +684,32 @@ class BackendStep(BaseModel):
|
||||
fields like `request_headers` / `response_headers` /
|
||||
`tcp_request_rules` are intentionally line-oriented and ARE
|
||||
NOT touched here.
|
||||
|
||||
Bulgu #90 (round-24 audit) — extend the validator beyond
|
||||
newlines. Pre-fix `cookie_name` accepted strings like
|
||||
`SESS'; DROP TABLE backends; --` (the SQL substring is
|
||||
harmless thanks to parameterised queries, but the `;` is
|
||||
HAProxy's INLINE COMMENT character: the renderer emits
|
||||
`cookie SESS'; DROP TABLE backends; -- insert indirect
|
||||
nocache`, which HAProxy parses as `cookie SESS'` followed by
|
||||
an inline comment that swallows the persistence options the
|
||||
operator typed). Result: session affinity silently broken
|
||||
and the operator has zero diagnostic signal — the wizard
|
||||
accepted the input, the apply succeeded, but cookies never
|
||||
get re-emitted with the expected name. The same trap exists
|
||||
for any HAProxy-section-keyword or whitespace token because
|
||||
HAProxy tokenises by space.
|
||||
|
||||
`cookie_name` MUST be a single token. We use the conservative
|
||||
intersection of RFC 6265 cookie-token chars and HAProxy
|
||||
directive-name chars: alphanumeric plus `_.-`. Operators
|
||||
with legitimate session cookies all live inside this set.
|
||||
|
||||
`cookie_options` is a space-separated keyword list
|
||||
(`insert indirect nocache` etc., optionally `domain example.
|
||||
com`, `attr SameSite=Lax`). Allow letters/digits/space/dot/
|
||||
hyphen/underscore/equals. Reject `;` (comment), backslash,
|
||||
quotes, and other shell metacharacters.
|
||||
"""
|
||||
if v is None:
|
||||
return v
|
||||
@@ -669,6 +721,28 @@ class BackendStep(BaseModel):
|
||||
"newlines would smuggle additional directives into "
|
||||
"the rendered config)"
|
||||
)
|
||||
field_name = info.field_name if info else "cookie field"
|
||||
if field_name == "cookie_name":
|
||||
import re as _re
|
||||
if not _re.fullmatch(r"[A-Za-z0-9_.\-]+", v):
|
||||
raise ValueError(
|
||||
"backend.cookie_name must contain only alphanumerics, "
|
||||
"'.', '_' or '-' (HAProxy tokenises the `cookie` "
|
||||
"directive by whitespace and treats ';' as an inline "
|
||||
"comment — anything else silently truncates the "
|
||||
"rendered persistence options). Got "
|
||||
f"{v!r}."
|
||||
)
|
||||
elif field_name == "cookie_options":
|
||||
import re as _re
|
||||
if not _re.fullmatch(r"[A-Za-z0-9_.=\- ]*", v):
|
||||
raise ValueError(
|
||||
"backend.cookie_options must contain only "
|
||||
"alphanumerics, spaces, '=', '.', '_' or '-' "
|
||||
"(HAProxy parses `;` as an inline comment, and "
|
||||
"quoting / backslash metacharacters are not "
|
||||
f"part of the `cookie` keyword grammar). Got {v!r}."
|
||||
)
|
||||
return v
|
||||
|
||||
@field_validator("health_check_uri")
|
||||
|
||||
@@ -198,12 +198,28 @@ def _enforce_routing_rule_contradictions(
|
||||
for label, rule in conflicts:
|
||||
sig = _rule_to_signature(rule)
|
||||
if sig and sig in grandfathered_signatures:
|
||||
# Bulgu #83 (round-23 audit) — re-word the operator-
|
||||
# facing warning. The pre-fix message led with
|
||||
# "Grandfathered <label> entry contains a self-
|
||||
# contradictory X !X condition that pre-dated this
|
||||
# validation", which (a) is internal jargon the
|
||||
# operator does not parse, and (b) implies the rule
|
||||
# is OLD when in fact the only thing this branch
|
||||
# knows is that the rule was NOT changed by the
|
||||
# current edit. The operator may well have authored
|
||||
# the rule one minute earlier. State that explicitly
|
||||
# and include the verbatim rule body so the operator
|
||||
# does not have to hunt through the ACL Builder
|
||||
# cards to find the offender.
|
||||
rule_text = rule if isinstance(rule, str) else sig[:160]
|
||||
warnings.append(
|
||||
f"Grandfathered {label} entry contains a "
|
||||
f"self-contradictory `X !X` condition that pre-dated "
|
||||
f"this validation. The rule never fires; fix it at "
|
||||
f"your convenience. (rule: "
|
||||
f"{rule if isinstance(rule, str) else sig[:160]})"
|
||||
f"{label}: rule was not modified by this edit but "
|
||||
f"contains a self-contradictory `X !X` condition "
|
||||
f"(`X AND NOT X` is always false, so the rule never "
|
||||
f"fires and traffic falls through to "
|
||||
f"`default_backend`). Your current edit was saved; "
|
||||
f"fix the rule at your convenience. "
|
||||
f"(rule: {rule_text})"
|
||||
)
|
||||
else:
|
||||
blocking.append((label, rule))
|
||||
|
||||
+188
-13
@@ -1046,9 +1046,45 @@ async def suggest_defaults(
|
||||
|
||||
slug = "newhost"
|
||||
if domain:
|
||||
base = domain.replace("*.", "").split(".")
|
||||
slug = (base[0] or "newhost")[:32].lower()
|
||||
slug = "".join(c if (c.isalnum() or c in ("-", "_")) else "-" for c in slug)
|
||||
# Bulgu #93 (round-24 audit) — IDN / Unicode safety. PRE-FIX
|
||||
# the suggest endpoint used `c.isalnum()`, which is Unicode-
|
||||
# aware and returns True for non-ASCII letters (ü, é, ñ, …).
|
||||
# An operator who typed `bücher.example.com` got back
|
||||
# `backend_name="be-bücher"`, dropped that into the wizard
|
||||
# form, and then hit a hard 422 at create time because the
|
||||
# backend/frontend name validator regex
|
||||
# `^[a-zA-Z][a-zA-Z0-9_-]{0,63}$` is ASCII-only. The wizard
|
||||
# CREATE path also forces the domain itself through punycode
|
||||
# (the validator rejects raw Unicode with a "use 'xn--…'"
|
||||
# hint). Make `suggest` honour the same on-the-wire ASCII
|
||||
# contract: convert each label to its IDN/punycode form
|
||||
# FIRST, then sanitise to the alphanumeric / `-_` set the
|
||||
# entity-name regex permits. The result is a name the
|
||||
# operator can submit to /api/sites without re-typing.
|
||||
first_label = (
|
||||
domain.replace("*.", "").split(".")[0]
|
||||
if domain.replace("*.", "")
|
||||
else ""
|
||||
)
|
||||
ascii_label = first_label
|
||||
if first_label and not first_label.isascii():
|
||||
try:
|
||||
ascii_label = first_label.encode("idna").decode("ascii")
|
||||
except (UnicodeError, UnicodeDecodeError):
|
||||
# IDN encoding failed (empty label, invalid chars,
|
||||
# etc.) — fall back to stripping non-ASCII to '-'
|
||||
# so we still produce a usable slug.
|
||||
ascii_label = "".join(
|
||||
c if c.isascii() and (c.isalnum() or c in ("-", "_"))
|
||||
else "-"
|
||||
for c in first_label
|
||||
)
|
||||
slug = (ascii_label or "newhost")[:32].lower()
|
||||
slug = "".join(
|
||||
c if c.isascii() and (c.isalnum() or c in ("-", "_"))
|
||||
else "-"
|
||||
for c in slug
|
||||
)
|
||||
if slug.startswith("_"):
|
||||
slug = "h-" + slug.lstrip("_")
|
||||
if not slug or not slug[0].isalpha():
|
||||
@@ -1146,6 +1182,53 @@ async def preview_create(
|
||||
try:
|
||||
await _validate_user_cluster_access(current_user["id"], body.cluster_id, conn)
|
||||
|
||||
# Bulgu #88 / #89 (round-24 audit) — cluster-RBAC parity with
|
||||
# `create_site` for SSL certificate references. PRE-FIX the
|
||||
# preview path (`POST /api/sites/preview`) skipped the
|
||||
# `select_existing_cert()` gate that `create_site` runs at
|
||||
# lines 2298-2308 (per-server CA bundle) and 2398-2405
|
||||
# (HTTPS bind cert). The omission let an authenticated wizard
|
||||
# user pass `ssl.mode='existing', ssl_certificate_id=<X>` (or
|
||||
# `servers[i].ssl_certificate_id=<X>`) where cert `X` belongs
|
||||
# to a DIFFERENT cluster and receive the full rendered
|
||||
# `would_create` envelope back — leaking cert id metadata
|
||||
# across tenant boundaries. The actual submit (`POST /api/
|
||||
# sites`) does enforce the gate, so this is a preview-only
|
||||
# information leak, not a write-path escalation. The fix is
|
||||
# to mirror the same `select_existing_cert(conn, id, cluster_
|
||||
# id)` predicate (which already encodes the "global OR
|
||||
# junction-bound" rule defined in `ssl_service.py:331-370`)
|
||||
# so the preview returns 400 with a clear hint rather than
|
||||
# 200 with a leaked render. We deliberately keep the error
|
||||
# phrasing identical to the create-time message so wizard
|
||||
# UI handlers that already match on "not found / inactive"
|
||||
# need no client changes.
|
||||
if body.ssl.mode == "existing" and body.ssl.ssl_certificate_id:
|
||||
_preview_resolved = await select_existing_cert(
|
||||
conn, body.ssl.ssl_certificate_id, body.cluster_id,
|
||||
)
|
||||
if not _preview_resolved:
|
||||
raise HTTPException(
|
||||
status_code=400,
|
||||
detail=(
|
||||
f"ssl_certificate_id {body.ssl.ssl_certificate_id} "
|
||||
"not found / inactive / not bound to this cluster"
|
||||
),
|
||||
)
|
||||
for _idx, _srv in enumerate(body.servers or []):
|
||||
_srv_cert_id = getattr(_srv, "ssl_certificate_id", None)
|
||||
if _srv_cert_id:
|
||||
if not await select_existing_cert(
|
||||
conn, _srv_cert_id, body.cluster_id,
|
||||
):
|
||||
raise HTTPException(
|
||||
status_code=400,
|
||||
detail=(
|
||||
f"servers[{_idx}].ssl_certificate_id={_srv_cert_id} "
|
||||
"not found / inactive / not bound to this cluster"
|
||||
),
|
||||
)
|
||||
|
||||
# Phase K Phase C: rate-limit ONLY the dry-run code path so
|
||||
# legacy callers (e.g. `SiteDrafts.handlePreview` which never
|
||||
# sets the flag) keep their unrestricted preview budget. The
|
||||
@@ -1848,6 +1931,28 @@ async def create_site(
|
||||
try:
|
||||
await _validate_user_cluster_access(user_id, body.cluster_id, conn)
|
||||
|
||||
# Bulgu #87 (round-23 audit) — rate-limit the actual create
|
||||
# endpoint. Pre-fix /api/sites/preview and
|
||||
# /api/sites/preflight-acme were rate-limited (5/min, see
|
||||
# `_enforce_rate_limit` callers above) but POST /api/sites
|
||||
# itself had NO cap. Round-3 live testing fired 10 wizard
|
||||
# creates in a single second against demo-cluster1; without
|
||||
# the cap an authenticated user / script can spam-create
|
||||
# entities until the cluster apply queue clogs.
|
||||
#
|
||||
# Action name must match what the router itself logs at the
|
||||
# end of create_site (`wizard_create_site` — see
|
||||
# `_log_user_activity(... action="wizard_create_site", ...)`
|
||||
# at the success path below). The activity_logger middleware
|
||||
# intentionally skips this endpoint to avoid double-logging
|
||||
# (see middleware/activity_logger.py:74-110), so we must
|
||||
# rate-limit on the SAME action the router emits.
|
||||
#
|
||||
# 5/min matches the preview / preflight budget — bulk
|
||||
# operators have the dedicated /api/sites/bulk-import path
|
||||
# for larger batches.
|
||||
await _enforce_rate_limit(conn, user_id, "wizard_create_site")
|
||||
|
||||
# ----- Pre-create checks (must succeed before transaction)
|
||||
|
||||
# Phase 3 (R11-audit follow-up): reserved-name check parity with
|
||||
@@ -2133,7 +2238,31 @@ async def create_site(
|
||||
# rebrand. The reject path on `cluster.py` recognises BOTH
|
||||
# prefixes so historical APPLIED versions (created before this
|
||||
# rename) keep behaving correctly during reject/undo.
|
||||
version_name = f"bulk-site-create-{ts}"
|
||||
#
|
||||
# Bulgu #86 (round-23 audit) — append a short UUID suffix so
|
||||
# two wizard POST /api/sites calls submitted within the SAME
|
||||
# epoch second cannot collide on the
|
||||
# `config_versions(cluster_id, version_name)` UNIQUE
|
||||
# constraint. Pre-fix `version_name = f"bulk-site-create-{ts}"`
|
||||
# gave seconds resolution, so an operator who clicked "Create"
|
||||
# twice in rapid succession (or any back-to-back API
|
||||
# automation) saw the SECOND call 409 with the generic
|
||||
# `UniqueViolationError` fall-through message:
|
||||
#
|
||||
# "A wizard entity with this name already exists on the
|
||||
# cluster (UNIQUE constraint). Pick a different name."
|
||||
#
|
||||
# — even though the operator-chosen backend / frontend / SSL
|
||||
# names were unique. The actual collision was on the
|
||||
# auto-generated `version_name` and re-naming the wizard
|
||||
# inputs did NOT help. The reject_pending_changes path on
|
||||
# cluster.py:4179 prefix-matches `bulk-site-create-*` so
|
||||
# appending a unique suffix preserves the historical
|
||||
# rollback / undo semantics. 6 hex chars give 16M-room before
|
||||
# birthday collisions, vastly more than the per-second
|
||||
# request volume an operator can sustain through the wizard.
|
||||
import uuid
|
||||
version_name = f"bulk-site-create-{ts}-{uuid.uuid4().hex[:6]}"
|
||||
bulk_snapshots: List[dict] = []
|
||||
created_ids: Dict[str, Any] = {}
|
||||
|
||||
@@ -2929,22 +3058,68 @@ async def create_site(
|
||||
# broke".
|
||||
msg = str(uve)
|
||||
logger.info(f"WIZARD: name conflict on create: {msg}")
|
||||
if "backends_name_cluster_id_key" in msg or 'backends_name' in msg:
|
||||
# Bulgu #85 (round-23 audit) — extract the offending constraint
|
||||
# name from the asyncpg message so the operator-visible detail
|
||||
# can pin-point WHICH entity collided. Pre-fix the handler only
|
||||
# recognised `backends_*_key` and `frontends_*_key`; any other
|
||||
# constraint (ssl_certificates, backend_servers, config_versions
|
||||
# …) fell through to the generic "wizard entity with this name"
|
||||
# message which is useless for debugging — the operator has to
|
||||
# open the server log and the engineer has to ssh-bounce to
|
||||
# extract `constraint=<name>` from the exception detail.
|
||||
#
|
||||
# ``asyncpg.UniqueViolationError.constraint_name`` is the
|
||||
# canonical structured field; ``str(uve)`` only contains the
|
||||
# human-formatted DETAIL line. Prefer the attribute, fall back
|
||||
# to substring scanning so we stay robust if asyncpg ever stops
|
||||
# exposing it.
|
||||
constraint = getattr(uve, "constraint_name", None) or ""
|
||||
msg_lower = msg.lower()
|
||||
if "backends_name_cluster_id_key" in msg or 'backends_name' in msg \
|
||||
or constraint == "backends_name_cluster_id_key":
|
||||
detail = (
|
||||
"A backend with this name already exists on the cluster "
|
||||
"(possibly soft-deleted). Pick a different backend name "
|
||||
"or restore/permanently-delete the existing row."
|
||||
f"A backend named '{body.backend.name}' already exists on "
|
||||
"the cluster (possibly soft-deleted). Pick a different "
|
||||
"backend name or restore/permanently-delete the existing "
|
||||
"row."
|
||||
)
|
||||
elif "frontends_name_cluster_id_key" in msg or "frontends_name" in msg:
|
||||
elif "frontends_name_cluster_id_key" in msg or "frontends_name" in msg \
|
||||
or constraint == "frontends_name_cluster_id_key":
|
||||
detail = (
|
||||
"A frontend with this name already exists on the cluster "
|
||||
f"A frontend named '{body.frontend.name}' (or its auto-"
|
||||
f"derived HTTPS sibling) already exists on the cluster "
|
||||
"(possibly soft-deleted). Pick a different frontend name "
|
||||
"or restore/permanently-delete the existing row."
|
||||
)
|
||||
else:
|
||||
elif "ssl_certificates" in msg_lower or "ssl_certificates" in constraint \
|
||||
or "ssl_cert" in constraint:
|
||||
ssl_name = getattr(body.ssl, "name", None) or "(unnamed)"
|
||||
detail = (
|
||||
"A wizard entity with this name already exists on the "
|
||||
"cluster (UNIQUE constraint). Pick a different name."
|
||||
f"An SSL certificate named '{ssl_name}' already exists on "
|
||||
"the cluster (possibly soft-deleted). Pick a different "
|
||||
"ssl.name or restore/permanently-delete the existing "
|
||||
"certificate row."
|
||||
)
|
||||
elif "backend_servers" in msg_lower or "backend_servers" in constraint:
|
||||
detail = (
|
||||
"Two servers in the same backend share a server_name. "
|
||||
"HAProxy requires `server <name>` tokens to be unique "
|
||||
"within a backend block. Rename the duplicate(s) and "
|
||||
"resubmit."
|
||||
)
|
||||
else:
|
||||
# Echo the constraint name (a stable, non-secret schema
|
||||
# identifier) in the detail so a human reading the toast
|
||||
# can grep the codebase for the matching CREATE TABLE
|
||||
# without needing server-log access. Names like
|
||||
# `proxied_hosts_pkey` or `config_versions_unique` are
|
||||
# safe to surface — they're public schema info.
|
||||
con_hint = f" (constraint={constraint})" if constraint else ""
|
||||
detail = (
|
||||
f"A wizard entity with this name already exists on the "
|
||||
f"cluster (UNIQUE constraint{con_hint}). Pick a different "
|
||||
"name or retry; if the conflict persists contact the "
|
||||
"platform team."
|
||||
)
|
||||
raise HTTPException(status_code=409, detail=detail)
|
||||
except Exception as e:
|
||||
|
||||
@@ -573,9 +573,18 @@ async def check_agents(conn, cluster_ids: List[int]) -> Dict[str, Any]:
|
||||
severity="warn",
|
||||
duration_ms=int((time.time() - started) * 1000),
|
||||
)
|
||||
# Bulgu #84 (round-23 audit) — the canonical timestamp column on the
|
||||
# `agents` table is `last_seen`. Pre-fix this query referenced a
|
||||
# non-existent `a.last_heartbeat`, so every ACME preflight call
|
||||
# (`POST /api/sites/preflight-acme`) crashed at the `check_agents`
|
||||
# stage with `UndefinedColumnError: column a.last_heartbeat does
|
||||
# not exist`, blocking the entire wizard's ACME pre-validation
|
||||
# gate. Every other agents.last_seen reader in the codebase
|
||||
# (routers/cluster.py:695-702, routers/agent.py, routers/dashboard
|
||||
# *.py) uses `last_seen`; aligning here.
|
||||
rows = await conn.fetch(
|
||||
"""
|
||||
SELECT a.id, a.hostname, a.status, a.last_heartbeat, hc.id AS cluster_id, hc.name AS cluster_name
|
||||
SELECT a.id, a.hostname, a.status, a.last_seen, hc.id AS cluster_id, hc.name AS cluster_name
|
||||
FROM agents a
|
||||
JOIN haproxy_clusters hc ON hc.pool_id = a.pool_id
|
||||
WHERE hc.id = ANY($1::int[])
|
||||
|
||||
@@ -433,7 +433,7 @@ async def test_check_agents_fail_when_none_registered():
|
||||
async def test_check_agents_warn_when_none_active():
|
||||
conn = AsyncMock()
|
||||
conn.fetch.return_value = [
|
||||
{"id": 1, "hostname": "h1", "status": "offline", "last_heartbeat": None,
|
||||
{"id": 1, "hostname": "h1", "status": "offline", "last_seen": None,
|
||||
"cluster_id": 1, "cluster_name": "c1"},
|
||||
]
|
||||
out = await check_agents(conn, [1])
|
||||
@@ -444,9 +444,9 @@ async def test_check_agents_warn_when_none_active():
|
||||
async def test_check_agents_ok_with_active():
|
||||
conn = AsyncMock()
|
||||
conn.fetch.return_value = [
|
||||
{"id": 1, "hostname": "h1", "status": "active", "last_heartbeat": None,
|
||||
{"id": 1, "hostname": "h1", "status": "active", "last_seen": None,
|
||||
"cluster_id": 1, "cluster_name": "c1"},
|
||||
{"id": 2, "hostname": "h2", "status": "offline", "last_heartbeat": None,
|
||||
{"id": 2, "hostname": "h2", "status": "offline", "last_seen": None,
|
||||
"cluster_id": 1, "cluster_name": "c1"},
|
||||
]
|
||||
out = await check_agents(conn, [1])
|
||||
@@ -454,6 +454,49 @@ async def test_check_agents_ok_with_active():
|
||||
assert "1 of 2" in out["message"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_bulgu84_check_agents_uses_last_seen_column():
|
||||
"""Bulgu #84 (round-23 audit) — pin the SQL column name.
|
||||
|
||||
Pre-fix the query referenced a non-existent ``a.last_heartbeat``
|
||||
column. AsyncMock returns whatever dict the test sets without
|
||||
re-validating the SQL string, so the pre-fix test suite was
|
||||
GREEN while every live ACME preflight call (the
|
||||
``POST /api/sites/preflight-acme`` endpoint that the wizard
|
||||
runs before showing the ACME step) crashed with
|
||||
``UndefinedColumnError: column a.last_heartbeat does not
|
||||
exist``. The crash blocked the entire wizard ACME preview
|
||||
page on a production cluster, but unit tests never noticed
|
||||
because they only assert on the helper's return shape, not
|
||||
on the SQL string the helper sends to Postgres.
|
||||
|
||||
This pin inspects ``conn.fetch.call_args`` to assert the SQL
|
||||
body actually queries ``a.last_seen`` (the canonical column
|
||||
name used everywhere else in the codebase — see
|
||||
routers/cluster.py:695-702 and routers/agent.py:281+ for
|
||||
sibling readers). A future ``last_heartbeat`` typo would
|
||||
re-fail this test without anyone noticing the live impact.
|
||||
"""
|
||||
conn = AsyncMock()
|
||||
conn.fetch.return_value = [
|
||||
{"id": 1, "hostname": "h1", "status": "active", "last_seen": None,
|
||||
"cluster_id": 1, "cluster_name": "c1"},
|
||||
]
|
||||
await check_agents(conn, [1])
|
||||
conn.fetch.assert_awaited_once()
|
||||
sql_query = conn.fetch.call_args[0][0]
|
||||
assert "a.last_seen" in sql_query, (
|
||||
f"check_agents SQL must select a.last_seen (the canonical "
|
||||
f"agents-table timestamp column); got: {sql_query!r}"
|
||||
)
|
||||
assert "a.last_heartbeat" not in sql_query, (
|
||||
f"check_agents SQL still references a.last_heartbeat — this "
|
||||
f"column does NOT exist on the agents table and the query "
|
||||
f"will 500 with UndefinedColumnError at runtime. SQL: "
|
||||
f"{sql_query!r}"
|
||||
)
|
||||
|
||||
|
||||
# ----------------------------------------------------------------------------
|
||||
# run_checks orchestration
|
||||
# ----------------------------------------------------------------------------
|
||||
|
||||
@@ -5328,7 +5328,22 @@ def test_bulgu62_update_path_grandfathers_unchanged_rule():
|
||||
fe, grandfathered_signatures=grand,
|
||||
)
|
||||
assert len(warnings) == 1
|
||||
assert "grandfathered" in warnings[0].lower() or "pre-dated" in warnings[0].lower()
|
||||
# Bulgu #83 (round-23 audit) — the warning was reworded from
|
||||
# "Grandfathered ... pre-dated this validation" to a clearer
|
||||
# "rule was not modified by this edit" phrasing that also
|
||||
# echoes the verbatim rule body. Accept either the legacy
|
||||
# markers or the new ones so the contract is signal-not-
|
||||
# wording.
|
||||
w_lower = warnings[0].lower()
|
||||
assert (
|
||||
"not modified by this edit" in w_lower
|
||||
or "grandfathered" in w_lower
|
||||
or "pre-dated" in w_lower
|
||||
), warnings[0]
|
||||
# The verbatim rule must appear in the warning body so the
|
||||
# operator can identify the offending entry without opening
|
||||
# the ACL Builder cards.
|
||||
assert stale in warnings[0], warnings[0]
|
||||
|
||||
|
||||
def test_bulgu62_update_path_still_rejects_new_contradiction():
|
||||
@@ -6267,3 +6282,611 @@ def test_bulgu82_generate_install_script_validates_cluster():
|
||||
window = src[fn_start:fn_start + 5000]
|
||||
assert "validate_user_cluster_access" in window
|
||||
assert "Bulgu #82 (round-22 audit)" in window
|
||||
|
||||
|
||||
# ---- Bulgu #83 — grandfathered contradiction warning wording ----
|
||||
|
||||
|
||||
def test_bulgu83_warning_includes_verbatim_rule_text():
|
||||
"""The PUT-path grandfather warning must echo the verbatim
|
||||
offending rule string so the operator can identify the
|
||||
offender without opening the ACL Builder cards. Pre-fix the
|
||||
warning only said "1 legacy rule" via the FE toast and
|
||||
"Grandfathered <label> entry contains a self-contradictory
|
||||
X !X condition that pre-dated this validation" via the server
|
||||
JSON payload — both omitted the actual rule body, forcing
|
||||
the operator to open the modal and hunt for the dead-code
|
||||
entry."""
|
||||
from models.frontend import FrontendConfig
|
||||
from routers.frontend import (
|
||||
_enforce_routing_rule_contradictions,
|
||||
_rule_to_signature,
|
||||
)
|
||||
|
||||
stale = "be-x if acl1 !acl1"
|
||||
fe = FrontendConfig(
|
||||
name="fe1",
|
||||
bind_port=80,
|
||||
mode="http",
|
||||
use_backend_rules=[stale],
|
||||
)
|
||||
warnings = _enforce_routing_rule_contradictions(
|
||||
fe, grandfathered_signatures={_rule_to_signature(stale)},
|
||||
)
|
||||
assert len(warnings) == 1
|
||||
# Verbatim rule body appears in the warning.
|
||||
assert stale in warnings[0]
|
||||
# The wording no longer uses the operator-unfriendly
|
||||
# "Grandfathered" lead-in; the message states what the
|
||||
# branch actually knows ("not modified by this edit").
|
||||
assert "not modified by this edit" in warnings[0].lower()
|
||||
|
||||
|
||||
def test_bulgu83_dict_redirect_warning_includes_signature_snippet():
|
||||
"""Dict-shaped redirect rules with a contradictory `condition`
|
||||
must still surface the offending signature in the warning
|
||||
(truncated to 160 chars to bound the toast length). Pin the
|
||||
truncation contract so a future refactor doesn't accidentally
|
||||
grow the warning into a multi-KB blob."""
|
||||
from models.frontend import FrontendConfig
|
||||
from routers.frontend import (
|
||||
_enforce_routing_rule_contradictions,
|
||||
_rule_to_signature,
|
||||
)
|
||||
|
||||
# Note: the redirect condition itself carries the contradiction;
|
||||
# the dict wrapper is what the wizard emits.
|
||||
bad_dict = {
|
||||
"type": "scheme",
|
||||
"scheme": "https",
|
||||
"code": 301,
|
||||
"condition": "if acl1 !acl1",
|
||||
}
|
||||
fe = FrontendConfig(
|
||||
name="fe1",
|
||||
bind_port=80,
|
||||
mode="http",
|
||||
redirect_rules=[bad_dict],
|
||||
)
|
||||
warnings = _enforce_routing_rule_contradictions(
|
||||
fe, grandfathered_signatures={_rule_to_signature(bad_dict)},
|
||||
)
|
||||
assert len(warnings) == 1
|
||||
# Truncated signature (up to 160 chars) must appear in body.
|
||||
assert "acl1" in warnings[0]
|
||||
assert "!acl1" in warnings[0]
|
||||
assert "redirect_rules" in warnings[0]
|
||||
|
||||
|
||||
def test_bulgu83_static_marker_in_fe_warning_toast():
|
||||
"""Front-end pin — the FrontendManagement.js toast no longer
|
||||
calls these rules "legacy" and now lists each offending rule
|
||||
body. Static-source check so a refactor that re-introduces
|
||||
the misleading wording or drops the rule snippets is caught.
|
||||
"""
|
||||
# The frontend tree lives next to backend/ at the workspace
|
||||
# root, so walk up one extra level from _BACKEND_DIR.
|
||||
fm_path = (
|
||||
_BACKEND_DIR.parent
|
||||
/ "frontend"
|
||||
/ "src"
|
||||
/ "components"
|
||||
/ "FrontendManagement.js"
|
||||
)
|
||||
fm_src = fm_path.read_text()
|
||||
assert "Bulgu #83 (round-23 audit)" in fm_src
|
||||
# No more `legacy routing/redirect rule(s)` wording.
|
||||
assert "legacy routing/redirect rule(s) with a self-contradictory" not in fm_src
|
||||
# The new toast wires the offending rule snippets into the
|
||||
# message body via `ruleSnippets`.
|
||||
assert "ruleSnippets" in fm_src
|
||||
# And it re-surfaces server-emitted warnings as a safety net.
|
||||
assert "serverWarnings" in fm_src
|
||||
|
||||
|
||||
# ----------------------------------------------------------------------------
|
||||
# Bulgu #84 + #85 (round-23 audit — comprehensive Site Wizard live-test pass)
|
||||
# ----------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_bulgu84_acme_diagnostics_uses_last_seen_column():
|
||||
"""Bulgu #84 (round-23 audit) — duplicate pin in this file so the
|
||||
full bulgu-pin suite has a one-stop reference; the original sits
|
||||
in `test_acme_diagnostics.py::test_bulgu84_check_agents_uses_
|
||||
last_seen_column` and asserts the AsyncMock call shape.
|
||||
|
||||
This duplicate is a STATIC-SOURCE check that's cheaper to grep:
|
||||
we open the service file and verify the wrong column name is
|
||||
not back in the SQL, plus the Bulgu marker is present so a
|
||||
future refactor that drops the comment also fails.
|
||||
"""
|
||||
svc_path = _BACKEND_DIR / "services" / "acme_diagnostics.py"
|
||||
src = svc_path.read_text()
|
||||
assert "Bulgu #84 (round-23 audit)" in src, (
|
||||
"Bulgu #84 marker missing from acme_diagnostics.py — the "
|
||||
"fix comment was removed but the underlying SQL change "
|
||||
"may have been reverted too. Re-verify check_agents."
|
||||
)
|
||||
# Strip comments before scanning for the wrong column — the fix
|
||||
# comment legitimately mentions ``a.last_heartbeat`` as the
|
||||
# pre-fix symptom. We only care about NON-comment occurrences.
|
||||
non_comment_lines = "\n".join(
|
||||
ln for ln in src.splitlines() if not ln.lstrip().startswith("#")
|
||||
)
|
||||
# The canonical SELECT inside check_agents must use a.last_seen.
|
||||
assert "SELECT a.id, a.hostname, a.status, a.last_seen" in non_comment_lines, (
|
||||
"acme_diagnostics.check_agents must SELECT a.last_seen "
|
||||
"(the canonical agents-table column). Pre-fix this used "
|
||||
"a non-existent a.last_heartbeat and every preflight-acme "
|
||||
"call 500'd in production."
|
||||
)
|
||||
assert "a.last_heartbeat" not in non_comment_lines, (
|
||||
"acme_diagnostics still references a.last_heartbeat in non-"
|
||||
"comment code — this column does NOT exist on the agents "
|
||||
"table. Use a.last_seen."
|
||||
)
|
||||
|
||||
|
||||
def test_bulgu87_wizard_create_endpoint_rate_limited():
|
||||
"""Bulgu #87 (round-23 audit) — `POST /api/sites` must be
|
||||
rate-limited, matching the existing 5/min caps on the
|
||||
/preview and /preflight-acme sibling endpoints.
|
||||
|
||||
Pre-fix the wizard's actual create endpoint had NO rate cap:
|
||||
- /api/sites/preflight-acme → rate-limited (site_acme_preflight)
|
||||
- /api/sites/preview → rate-limited (site_previewed)
|
||||
- /api/sites (POST, create) → UNLIMITED ← gap
|
||||
|
||||
Round-3 live testing fired 10 wizard creates inside one
|
||||
second against demo-cluster1; the response codes were
|
||||
200/409/409/...409/200/409... — no 429 ever fired. With
|
||||
Bulgu #86 fixed (version_name collision), the UNIQUE-error
|
||||
side-effect that ACCIDENTALLY rate-limited rapid-fire creates
|
||||
disappears too, so an authenticated user/script can now
|
||||
create entities until the cluster apply queue clogs.
|
||||
|
||||
The fix calls `_enforce_rate_limit(conn, user_id,
|
||||
"wizard_create_site")` early in create_site (after auth +
|
||||
cluster-access checks). The action name must match what the
|
||||
router itself logs (`wizard_create_site` — see
|
||||
`_log_user_activity(... action="wizard_create_site", ...)`
|
||||
in the same router); the activity_logger middleware
|
||||
intentionally skips this endpoint to avoid double-logging,
|
||||
so the rate-limit must reference the router-emitted action.
|
||||
"""
|
||||
sw_src = (_BACKEND_DIR / "routers" / "site_wizard.py").read_text()
|
||||
assert "Bulgu #87 (round-23 audit)" in sw_src, (
|
||||
"Bulgu #87 marker missing — the wizard create rate-limit "
|
||||
"may have been reverted."
|
||||
)
|
||||
# The create_site function body must call _enforce_rate_limit
|
||||
# with the canonical create action name.
|
||||
import re as _re
|
||||
m = _re.search(
|
||||
r"async def create_site\([^)]*\)[^{]*:(.*?)(?=\nasync def |\n@router\.|\Z)",
|
||||
sw_src,
|
||||
_re.DOTALL,
|
||||
)
|
||||
assert m, "could not locate create_site function body in site_wizard.py"
|
||||
fn_body = m.group(1)
|
||||
assert (
|
||||
'_enforce_rate_limit(conn, user_id, "wizard_create_site")' in fn_body
|
||||
or "_enforce_rate_limit(conn, user_id, 'wizard_create_site')" in fn_body
|
||||
), (
|
||||
"create_site must call _enforce_rate_limit(... "
|
||||
'"wizard_create_site") so rapid-fire POST /api/sites '
|
||||
"calls hit a 429 cap; pre-fix the endpoint was unlimited."
|
||||
)
|
||||
|
||||
|
||||
def test_bulgu86_wizard_version_name_has_unique_suffix():
|
||||
"""Bulgu #86 (round-23 audit) — pin the version-name generation so
|
||||
two wizard POST /api/sites calls submitted within the SAME epoch
|
||||
second cannot collide on `config_versions(cluster_id, version_name)`.
|
||||
|
||||
Pre-fix `version_name = f"bulk-site-create-{int(time.time())}"`
|
||||
had seconds resolution. Round-3 live testing reproduced the
|
||||
failure with a back-to-back wizard automation: the second call
|
||||
returned 409 with the misleading
|
||||
|
||||
"A wizard entity with this name already exists on the
|
||||
cluster (UNIQUE constraint). Pick a different name."
|
||||
|
||||
fall-through detail, even though the operator-supplied
|
||||
backend / frontend / SSL names were genuinely unique — the
|
||||
real collision was on the auto-generated version name. The
|
||||
operator who tried to rename the wizard inputs would still
|
||||
hit the same error and have no way to make progress until the
|
||||
epoch second ticked over.
|
||||
|
||||
Fix: append a 6-hex-char UUID suffix
|
||||
(`bulk-site-create-{ts}-{uuid.uuid4().hex[:6]}`) so the
|
||||
suffix space is 16M and birthday-collision-proof at any
|
||||
realistic per-second request rate. The `bulk-site-create-`
|
||||
prefix is preserved so reject_pending_changes /
|
||||
restore-from-prior-version paths (cluster.py:4179 prefix scan)
|
||||
keep working.
|
||||
|
||||
Static-source pin so any future refactor that strips the
|
||||
suffix fails this test.
|
||||
"""
|
||||
sw_src = (_BACKEND_DIR / "routers" / "site_wizard.py").read_text()
|
||||
assert "Bulgu #86 (round-23 audit)" in sw_src, (
|
||||
"Bulgu #86 marker missing — the version_name uniqueness "
|
||||
"suffix may have been reverted."
|
||||
)
|
||||
# The fixed form references uuid.uuid4().hex[:6] (or similar
|
||||
# length-bounded random suffix) appended to the ts. A pure
|
||||
# `f"bulk-site-create-{ts}"` line (without any suffix
|
||||
# interpolation after ts) is the regressed form.
|
||||
import re as _re
|
||||
# Find the wizard's version_name assignment. We allow flexible
|
||||
# spacing / quoting around the f-string but require the suffix
|
||||
# interpolation token immediately after `{ts}-`.
|
||||
has_suffix = bool(_re.search(
|
||||
r'version_name\s*=\s*f"bulk-site-create-\{ts\}-\{[^}]+\}"',
|
||||
sw_src,
|
||||
))
|
||||
has_regressed = bool(_re.search(
|
||||
r'version_name\s*=\s*f"bulk-site-create-\{ts\}"\s*$',
|
||||
sw_src,
|
||||
_re.MULTILINE,
|
||||
))
|
||||
assert has_suffix, (
|
||||
"wizard's version_name must append a unique suffix "
|
||||
"(e.g. uuid.uuid4().hex[:6]) after `{ts}` so back-to-back "
|
||||
"wizard creates within the same epoch second cannot "
|
||||
"collide on config_versions UNIQUE."
|
||||
)
|
||||
assert not has_regressed, (
|
||||
"wizard's version_name regressed to seconds-only resolution "
|
||||
"— this re-introduces Bulgu #86 (UNIQUE collision on rapid "
|
||||
"back-to-back creates)."
|
||||
)
|
||||
|
||||
|
||||
def test_bulgu85_unique_violation_handler_extracts_constraint_name():
|
||||
"""Bulgu #85 (round-23 audit) — the wizard's UniqueViolationError
|
||||
handler must surface the offending constraint name in the operator-
|
||||
visible 409 detail so debugging a "wizard entity with this name
|
||||
already exists" toast doesn't require shell access to the server
|
||||
logs.
|
||||
|
||||
Pre-fix the handler had THREE branches:
|
||||
* backends_name_cluster_id_key → "backend with this name"
|
||||
* frontends_name_cluster_id_key → "frontend with this name"
|
||||
* else → generic "wizard entity"
|
||||
Any other constraint (SSL cert UNIQUE, backend_servers UNIQUE,
|
||||
config_versions UNIQUE) fell through to the generic message,
|
||||
leaving the operator with NO actionable hint and requiring an
|
||||
on-call engineer to ssh into the API pod and tail `logger.info`
|
||||
output to find ``constraint=<name>`` in the asyncpg traceback.
|
||||
|
||||
The fix:
|
||||
1. Echoes the entity NAME (body.backend.name, body.frontend.name,
|
||||
body.ssl.name) so the operator can match toast to wizard input.
|
||||
2. Adds dedicated branches for `ssl_certificates_*` and
|
||||
`backend_servers_*` constraints with actionable hints.
|
||||
3. In the fall-through ELSE branch, echoes the constraint name
|
||||
from `uve.constraint_name` (or substring scan as fallback)
|
||||
so an unknown constraint still gives the engineer a stable
|
||||
schema identifier to grep.
|
||||
"""
|
||||
sw_src = (_BACKEND_DIR / "routers" / "site_wizard.py").read_text()
|
||||
assert "Bulgu #85 (round-23 audit)" in sw_src, (
|
||||
"Bulgu #85 marker missing from site_wizard.py UniqueViolation "
|
||||
"handler — the constraint-echoing fix may have been reverted."
|
||||
)
|
||||
# Entity-name echoing in branch detail bodies.
|
||||
assert "body.backend.name" in sw_src and (
|
||||
"A backend named '{body.backend.name}'" in sw_src
|
||||
or "A backend named '\"{body.backend.name}\"" in sw_src
|
||||
or "A backend named '" in sw_src and "body.backend.name" in sw_src
|
||||
)
|
||||
assert "A frontend named '{body.frontend.name}'" in sw_src or (
|
||||
"A frontend named '" in sw_src and "body.frontend.name" in sw_src
|
||||
)
|
||||
# SSL cert and backend_servers branches present.
|
||||
assert "ssl_certificates" in sw_src
|
||||
assert "backend_servers" in sw_src
|
||||
# Fall-through echoes constraint name.
|
||||
assert "constraint=" in sw_src or "constraint_name" in sw_src
|
||||
# The asyncpg attribute is preferred over str(uve) scanning.
|
||||
assert 'getattr(uve, "constraint_name"' in sw_src, (
|
||||
"Fix should prefer asyncpg's structured constraint_name "
|
||||
"attribute over substring scanning str(uve), which is the "
|
||||
"human-formatted DETAIL line and may vary across server "
|
||||
"versions / locales."
|
||||
)
|
||||
|
||||
|
||||
# ----- Bulgu #88 / #89 (round-24 audit) ----------------------------------
|
||||
#
|
||||
# `POST /api/sites/preview` skipped the SSL-cert cluster-RBAC gate that
|
||||
# `POST /api/sites` (create) enforces via `select_existing_cert()`. The
|
||||
# `select_existing_cert()` helper encodes "global cert OR junction-bound
|
||||
# to <cluster_id>"; pre-fix the preview accepted ssl_certificate_id values
|
||||
# bound to OTHER clusters and returned the full rendered `would_create`
|
||||
# envelope — a tenant-boundary information leak for the cert id and the
|
||||
# rendered HAProxy config snippet. The fix mirrors the create-time check
|
||||
# in the preview path: same predicate, same 400 status, same error text.
|
||||
|
||||
def test_bulgu88_preview_enforces_ssl_cert_cluster_rbac():
|
||||
"""Preview must reject `ssl.ssl_certificate_id` referencing a cert
|
||||
that is bound to a DIFFERENT cluster — same RBAC predicate the
|
||||
create endpoint enforces. Static source check: the preview body
|
||||
invokes `select_existing_cert` with `body.ssl.ssl_certificate_id`
|
||||
and raises 400 with a "not found / inactive / not bound" hint."""
|
||||
import os
|
||||
sw_path = os.path.join(
|
||||
os.path.dirname(__file__), "..", "routers", "site_wizard.py",
|
||||
)
|
||||
with open(sw_path, "r") as fh:
|
||||
sw_src = fh.read()
|
||||
|
||||
# Locate the preview function.
|
||||
assert "async def preview_create(" in sw_src
|
||||
preview_start = sw_src.index("async def preview_create(")
|
||||
# Bound the slice at the next top-level `async def` so we don't
|
||||
# match the create_site copy (which is at line ~1900 and would
|
||||
# cause this test to trivially pass even if preview was unfixed).
|
||||
next_def = sw_src.index("\nasync def ", preview_start + 1)
|
||||
preview_body = sw_src[preview_start:next_def]
|
||||
|
||||
# The preview must call select_existing_cert.
|
||||
assert "select_existing_cert(" in preview_body, (
|
||||
"Preview path must mirror create_site's cluster-RBAC gate by "
|
||||
"invoking select_existing_cert(conn, ssl_certificate_id, "
|
||||
"cluster_id). Pre-fix preview returned 200 with the full "
|
||||
"rendered would_create envelope when the cert belonged to a "
|
||||
"different cluster — a tenant-boundary information leak."
|
||||
)
|
||||
|
||||
# The check must be wired to the body.ssl.ssl_certificate_id field
|
||||
# (not some unrelated cert reference).
|
||||
assert "body.ssl.ssl_certificate_id" in preview_body, (
|
||||
"Preview RBAC check must reference body.ssl.ssl_certificate_id"
|
||||
)
|
||||
|
||||
# Must raise 400 (consistent with create-time message).
|
||||
assert "status_code=400" in preview_body
|
||||
assert "not found / inactive / not bound" in preview_body or (
|
||||
"not found / inactive" in preview_body
|
||||
and "not bound" in preview_body
|
||||
), (
|
||||
"Preview should raise 400 with the same 'not found / inactive "
|
||||
"/ not bound to this cluster' hint create_site emits, so wizard "
|
||||
"clients can match the message uniformly."
|
||||
)
|
||||
|
||||
|
||||
def test_bulgu89_preview_enforces_per_server_ca_bundle_cluster_rbac():
|
||||
"""Preview must also enforce per-server `ssl_certificate_id` (CA
|
||||
bundle) cluster-RBAC. Pre-fix only `create_site` checked this — a
|
||||
direct API caller could submit a preview with
|
||||
`servers[i].ssl_certificate_id=<cert from another cluster>` and
|
||||
receive the full rendered backend block back, including the
|
||||
`ca-file` directive bound to a cert id the caller's cluster has
|
||||
no junction row for."""
|
||||
import os
|
||||
sw_path = os.path.join(
|
||||
os.path.dirname(__file__), "..", "routers", "site_wizard.py",
|
||||
)
|
||||
with open(sw_path, "r") as fh:
|
||||
sw_src = fh.read()
|
||||
preview_start = sw_src.index("async def preview_create(")
|
||||
next_def = sw_src.index("\nasync def ", preview_start + 1)
|
||||
preview_body = sw_src[preview_start:next_def]
|
||||
|
||||
# Must iterate body.servers and check per-server ssl_certificate_id.
|
||||
assert "body.servers" in preview_body, (
|
||||
"Preview must iterate body.servers to validate each "
|
||||
"server.ssl_certificate_id reference"
|
||||
)
|
||||
assert "ssl_certificate_id" in preview_body
|
||||
# The per-server message must be field-qualified so operators see
|
||||
# WHICH server triggered the gate.
|
||||
assert "servers[" in preview_body and "ssl_certificate_id=" in preview_body, (
|
||||
"Per-server preview error must include the array index "
|
||||
"(servers[i].ssl_certificate_id=<id>) so operators can find "
|
||||
"the offending row in their wizard payload."
|
||||
)
|
||||
|
||||
|
||||
# ----- Bulgu #90 (round-24 audit) ----------------------------------------
|
||||
#
|
||||
# `backend.cookie_name`, `backend.cookie_options`, and `server.cookie_value`
|
||||
# only rejected newline characters pre-fix. HAProxy's directive parser
|
||||
# tokenises by whitespace and treats `;` as an inline-comment, so a value
|
||||
# like `SESS'; DROP TABLE backends; --` rendered as
|
||||
# `cookie SESS'; DROP TABLE backends; -- insert indirect nocache` which
|
||||
# HAProxy parsed as `cookie SESS'` + comment — silently truncating the
|
||||
# operator's persistence options and producing a malformed Set-Cookie
|
||||
# header that browsers may drop. Tighten to the conservative
|
||||
# alphanumeric+`_.-` set (cookie_name / cookie_value) and the keyword/
|
||||
# `=`-bearing set (cookie_options).
|
||||
|
||||
def test_bulgu90_cookie_name_rejects_haproxy_parser_hostile_chars():
|
||||
"""cookie_name must reject `;` (HAProxy inline comment), whitespace
|
||||
(token delimiter), and other punctuation that breaks the rendered
|
||||
`cookie <name>` directive."""
|
||||
from models.site_wizard import BackendStep
|
||||
import pydantic
|
||||
# Semicolon (HAProxy comment) — used to be silently accepted.
|
||||
with pytest.raises(pydantic.ValidationError):
|
||||
BackendStep(name="be1", cookie_name="SESS'; DROP TABLE backends; --")
|
||||
# Space (token delimiter) — splits the cookie directive.
|
||||
with pytest.raises(pydantic.ValidationError):
|
||||
BackendStep(name="be1", cookie_name="SESS ID")
|
||||
# Quote (legal in RFC 6265 but produces ugly Set-Cookie headers
|
||||
# and confuses log scrapers).
|
||||
with pytest.raises(pydantic.ValidationError):
|
||||
BackendStep(name="be1", cookie_name='SESS"id')
|
||||
# Common-case valid input continues to work.
|
||||
BackendStep(name="be1", cookie_name="SESS_ID-1.app")
|
||||
|
||||
|
||||
def test_bulgu90_cookie_options_rejects_parser_hostile_chars():
|
||||
"""cookie_options must reject `;` and quoting metacharacters but
|
||||
still accept legitimate `attr SameSite=Lax` style values."""
|
||||
from models.site_wizard import BackendStep
|
||||
import pydantic
|
||||
with pytest.raises(pydantic.ValidationError):
|
||||
BackendStep(
|
||||
name="be1", cookie_name="SESS",
|
||||
cookie_options="insert indirect; rm -rf /",
|
||||
)
|
||||
with pytest.raises(pydantic.ValidationError):
|
||||
BackendStep(
|
||||
name="be1", cookie_name="SESS",
|
||||
cookie_options='insert "indirect"',
|
||||
)
|
||||
# Valid HAProxy cookie options continue to parse.
|
||||
BackendStep(
|
||||
name="be1", cookie_name="SESS",
|
||||
cookie_options="insert indirect nocache attr SameSite=Lax",
|
||||
)
|
||||
|
||||
|
||||
def test_bulgu90_server_cookie_value_rejects_rfc6265_disallowed_chars():
|
||||
"""server.cookie_value must reject `;`, whitespace, and other
|
||||
chars RFC 6265 disallows in cookie-values."""
|
||||
from models.site_wizard import ServerStep
|
||||
import pydantic
|
||||
with pytest.raises(pydantic.ValidationError):
|
||||
ServerStep(
|
||||
server_name="s1", server_address="10.0.0.1",
|
||||
server_port=8080, cookie_value="srv1; secure",
|
||||
)
|
||||
with pytest.raises(pydantic.ValidationError):
|
||||
ServerStep(
|
||||
server_name="s1", server_address="10.0.0.1",
|
||||
server_port=8080, cookie_value="srv 1",
|
||||
)
|
||||
# Valid cookie values continue to work.
|
||||
ServerStep(
|
||||
server_name="s1", server_address="10.0.0.1",
|
||||
server_port=8080, cookie_value="srv-1.app_2",
|
||||
)
|
||||
|
||||
|
||||
# ----- Bulgu #93 (round-24 audit) ----------------------------------------
|
||||
#
|
||||
# `GET /api/sites/suggest` produced backend/frontend name suggestions
|
||||
# using `c.isalnum()` over the first domain label. Python's `isalnum()`
|
||||
# is Unicode-aware and returns True for non-ASCII letters (ü, é, ñ, …),
|
||||
# so an operator typing `bücher.example.com` received
|
||||
# `backend_name='be-bücher'`. The wizard CREATE path then rejected the
|
||||
# very name the SUGGEST endpoint returned, because the entity-name
|
||||
# regex (`^[a-zA-Z][a-zA-Z0-9_-]{0,63}$`) and the domain validator are
|
||||
# both ASCII-only. The fix converts non-ASCII labels through IDN/
|
||||
# punycode FIRST and only then applies ASCII-only sanitisation, so the
|
||||
# resulting name is one the operator can submit unchanged.
|
||||
|
||||
def test_bulgu93_suggest_idn_unicode_produces_ascii_safe_slug():
|
||||
"""`suggest` must produce slugs that the backend/frontend name
|
||||
regex `^[a-zA-Z][a-zA-Z0-9_-]{0,63}$` accepts. Static source check:
|
||||
the suggest function uses an ASCII-only predicate
|
||||
(`c.isascii() and c.isalnum()` etc.) and routes IDN labels through
|
||||
`encode('idna')` to preserve the operator's intent in punycode."""
|
||||
import os, re
|
||||
sw_path = os.path.join(
|
||||
os.path.dirname(__file__), "..", "routers", "site_wizard.py",
|
||||
)
|
||||
with open(sw_path, "r") as fh:
|
||||
sw_src = fh.read()
|
||||
|
||||
# Locate the suggest_defaults function body.
|
||||
assert "async def suggest_defaults(" in sw_src
|
||||
start = sw_src.index("async def suggest_defaults(")
|
||||
end = sw_src.index("\n@router.", start + 1)
|
||||
suggest_body = sw_src[start:end]
|
||||
|
||||
# The fix must use IDN/punycode encoding when the input is non-ASCII.
|
||||
assert 'encode("idna")' in suggest_body or "encode('idna')" in suggest_body, (
|
||||
"suggest_defaults must route non-ASCII labels through IDN/"
|
||||
"punycode so the rendered slug matches the ASCII-only entity-"
|
||||
"name regex the create endpoint enforces."
|
||||
)
|
||||
|
||||
# The sanitiser must use an ASCII-only predicate, not Unicode-aware
|
||||
# `isalnum()` alone (which is the pre-fix bug).
|
||||
assert "isascii()" in suggest_body, (
|
||||
"Sanitisation step must explicitly bound to ASCII (via "
|
||||
"`c.isascii()`) so Unicode alphabetics get mapped to '-' rather "
|
||||
"than retained verbatim."
|
||||
)
|
||||
|
||||
# The first occurrence of the Unicode-only `c.isalnum()` (without an
|
||||
# adjacent isascii() guard) must no longer exist in the slug-build
|
||||
# path. We sanity-check by counting the matches of the bare
|
||||
# `c.isalnum()` token within the function body — pre-fix there was
|
||||
# exactly one such occurrence in the slug builder.
|
||||
bare = re.findall(r"\bc\.isalnum\(\)\b", suggest_body)
|
||||
assert all(
|
||||
# Each occurrence must be paired with an `isascii()` guard on
|
||||
# the same line — confirm by inspecting the line context.
|
||||
any("isascii()" in ln for ln in suggest_body.splitlines() if "c.isalnum()" in ln)
|
||||
for _ in bare
|
||||
), (
|
||||
"Every `c.isalnum()` predicate in the slug builder must be "
|
||||
"AND'd with `c.isascii()` so non-ASCII letters cannot leak "
|
||||
"into the suggested entity names."
|
||||
)
|
||||
|
||||
|
||||
def test_bulgu93_suggest_function_behavioural_smoke():
|
||||
"""Behavioural check: the same logic, invoked directly, should
|
||||
produce an ASCII-only slug for a Unicode input."""
|
||||
# Replicate the post-fix slug builder inline so the test does not
|
||||
# require an event loop / FastAPI fixture. The point is to confirm
|
||||
# the algorithmic shape matches what the source check above
|
||||
# enforces.
|
||||
def _build_slug(domain: str) -> str:
|
||||
first_label = (
|
||||
domain.replace("*.", "").split(".")[0]
|
||||
if domain.replace("*.", "")
|
||||
else ""
|
||||
)
|
||||
ascii_label = first_label
|
||||
if first_label and not first_label.isascii():
|
||||
try:
|
||||
ascii_label = first_label.encode("idna").decode("ascii")
|
||||
except (UnicodeError, UnicodeDecodeError):
|
||||
ascii_label = "".join(
|
||||
c if c.isascii() and (c.isalnum() or c in ("-", "_"))
|
||||
else "-"
|
||||
for c in first_label
|
||||
)
|
||||
slug = (ascii_label or "newhost")[:32].lower()
|
||||
slug = "".join(
|
||||
c if c.isascii() and (c.isalnum() or c in ("-", "_"))
|
||||
else "-"
|
||||
for c in slug
|
||||
)
|
||||
if slug.startswith("_"):
|
||||
slug = "h-" + slug.lstrip("_")
|
||||
if not slug or not slug[0].isalpha():
|
||||
slug = "h-" + slug
|
||||
return slug
|
||||
|
||||
# Unicode IDN — should round-trip through punycode.
|
||||
s = _build_slug("bücher.example.com")
|
||||
# punycode of 'bücher' is 'xn--bcher-kva'
|
||||
assert s == "xn--bcher-kva", f"expected xn--bcher-kva got {s!r}"
|
||||
# The slug must satisfy the entity-name regex.
|
||||
import re
|
||||
assert re.fullmatch(r"^[a-zA-Z][a-zA-Z0-9_-]{0,63}$", s), s
|
||||
|
||||
# Plain ASCII domain still produces the obvious slug.
|
||||
assert _build_slug("example.com") == "example"
|
||||
|
||||
# Uppercase normalises to lowercase.
|
||||
assert _build_slug("EXAMPLE.COM") == "example"
|
||||
|
||||
# Trailing dot (FQDN absolute) stripped.
|
||||
assert _build_slug("example.com.") == "example"
|
||||
|
||||
# Empty / dots-only input falls back to 'newhost'.
|
||||
assert _build_slug("") == "newhost"
|
||||
assert _build_slug("...") == "newhost"
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
{
|
||||
"name": "haproxy-openmanager-frontend",
|
||||
"version": "1.5.0",
|
||||
"version": "1.5.1",
|
||||
"description": "HAProxy Load Balancer Management UI",
|
||||
"dependencies": {
|
||||
"react": "^18.2.0",
|
||||
|
||||
@@ -1015,12 +1015,56 @@ const FrontendManagement = () => {
|
||||
return;
|
||||
}
|
||||
if (grandfatheredContradictions.length > 0) {
|
||||
// Bulgu #83 (round-23 audit) — surface the actual offending rule
|
||||
// string(s) instead of just a count. Pre-fix the warning said
|
||||
// "1 legacy rule has X !X" and the operator had to hunt
|
||||
// through the ACL Builder cards to figure out which rule the
|
||||
// gate was complaining about. The unchanged-rule path is the
|
||||
// common case (operator changes port / maxconn on a frontend
|
||||
// that already had a self-contradictory routing rule from a
|
||||
// prior session), so making the rule discoverable from the
|
||||
// toast keeps "Edit and Save" → "fix the dead rule" workflows
|
||||
// single-screen. Also stop calling these rules "legacy" —
|
||||
// the operator may have written them seconds earlier; the
|
||||
// only thing this branch knows is that they weren't modified
|
||||
// by the current edit.
|
||||
const renderGrandfatheredRule = (r) => {
|
||||
if (typeof r === 'string') return r;
|
||||
if (r && typeof r === 'object') {
|
||||
try { return JSON.stringify(r); } catch (_e) { return '[rule]'; }
|
||||
}
|
||||
return '[rule]';
|
||||
};
|
||||
const ruleSnippets = grandfatheredContradictions
|
||||
.slice(0, 5)
|
||||
.map(renderGrandfatheredRule)
|
||||
.map((s) => (s.length > 160 ? `${s.slice(0, 157)}...` : s));
|
||||
const extra = grandfatheredContradictions.length > ruleSnippets.length
|
||||
? ` (+${grandfatheredContradictions.length - ruleSnippets.length} more)`
|
||||
: '';
|
||||
message.warning(
|
||||
`This frontend has ${grandfatheredContradictions.length} legacy ` +
|
||||
`routing/redirect rule(s) with a self-contradictory \`X !X\` ` +
|
||||
`condition. The rule(s) never fire — fix them at your convenience. ` +
|
||||
`Your current edit will still be saved.`,
|
||||
6,
|
||||
<div>
|
||||
<div>
|
||||
<strong>
|
||||
{grandfatheredContradictions.length} routing/redirect rule(s)
|
||||
you didn't modify in this edit contain a self-contradictory
|
||||
`X !X` condition (e.g. `if acl1 !acl1`).
|
||||
</strong>
|
||||
</div>
|
||||
<div style={{ marginTop: 4, fontSize: '12px' }}>
|
||||
HAProxy accepts the syntax but `X AND NOT X` is always false,
|
||||
so the rule never fires and traffic silently falls through to
|
||||
`default_backend`. Your current edit will still be saved; fix
|
||||
the rule(s) at your convenience.
|
||||
</div>
|
||||
<div style={{ marginTop: 6, fontSize: '12px', fontFamily: 'monospace' }}>
|
||||
{ruleSnippets.map((s, i) => (
|
||||
<div key={i}>• {s}</div>
|
||||
))}
|
||||
{extra && <div>{extra}</div>}
|
||||
</div>
|
||||
</div>,
|
||||
10,
|
||||
);
|
||||
}
|
||||
|
||||
@@ -1081,6 +1125,36 @@ const FrontendManagement = () => {
|
||||
} else {
|
||||
message.success('Frontend updated successfully');
|
||||
}
|
||||
|
||||
// Bulgu #83 (round-23 audit) — surface server-emitted
|
||||
// grandfathered-rule warnings (e.g. `X !X` contradictions
|
||||
// in routing/redirect rules that the operator did not
|
||||
// touch this edit). The FE client-side gate ALSO catches
|
||||
// these and fires its own toast above the modal close;
|
||||
// we re-surface the server view here as a safety net in
|
||||
// case the client gate missed an edge shape (different
|
||||
// dict serialization, etc.). Server warnings already
|
||||
// include the verbatim rule text, so the operator sees
|
||||
// exactly which entry to fix.
|
||||
const serverWarnings = Array.isArray(response.data?.warnings)
|
||||
? response.data.warnings
|
||||
: [];
|
||||
if (serverWarnings.length > 0 && grandfatheredContradictions.length === 0) {
|
||||
message.warning(
|
||||
<div>
|
||||
<div><strong>Frontend saved, but the server flagged {serverWarnings.length} rule warning(s):</strong></div>
|
||||
<div style={{ marginTop: 6, fontSize: '12px', fontFamily: 'monospace' }}>
|
||||
{serverWarnings.slice(0, 5).map((w, i) => (
|
||||
<div key={i}>• {w.length > 240 ? `${w.slice(0, 237)}...` : w}</div>
|
||||
))}
|
||||
{serverWarnings.length > 5 && (
|
||||
<div>(+{serverWarnings.length - 5} more)</div>
|
||||
)}
|
||||
</div>
|
||||
</div>,
|
||||
10,
|
||||
);
|
||||
}
|
||||
} else {
|
||||
response = await axios.post('/api/frontends', requestData);
|
||||
|
||||
|
||||
+3
-3
@@ -1,5 +1,5 @@
|
||||
{
|
||||
"version": "1.5.0",
|
||||
"releaseName": "ACME Diagnostics & Proxied Host Wizard",
|
||||
"releaseDate": "2026-05-08"
|
||||
"version": "1.5.1",
|
||||
"releaseName": "Round-23 + Round-24 audit follow-ups",
|
||||
"releaseDate": "2026-05-13"
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user