portal: guard against an implausible member list; redact the webhook token from logs
Two fixes, both filed rather than deferred. 1. member-portal#2 — the sweep deactivates every member whose email is missing from the Open Collective member list, and the existing fail-safe only covers a fetch that RAISES. A fetch that SUCCEEDS while returning nothing or far less than usual is unguarded, and is what a narrowed token scope, an expired credential with partial access, or a renamed Membership tier produces (the fetch filters on the tier's name). That path deactivates the membership instead of failing. fetch_members() now checks the result against the local active count before any caller can act on it: empty, or at or below half of local, raises — so the sweep's fail-safe takes over and members stay active. Skipped when we have no active members of our own (fresh install). Correction to the issue as filed: listmonk_reconcile reads the local members table, not this fetch, so the newsletter path was never exposed to it. The sweep is the only destructive consumer. 2. ops/security#2 — OC's generic webhooks cannot send headers, so the webhook secret must travel in the URL path, and uvicorn wrote it into the access log on every delivery. A logging filter on uvicorn.access and on the root handler replaces the token with <redacted>. The token cannot be kept out of the URL; it can be kept out of the retention. Both pieces are unit-tested in isolation before shipping: the guard's decision function over eight cases, and the filter against a real access-log record, an unrelated path (no over-redaction), and a missing dependency (must not break logging).
This commit is contained in:
1 parent
c2369034ee
commit
452a27a778
1 file changed
+82
+82
@@ -36,6 +36,40 @@ from slowapi.errors import RateLimitExceeded
|
||||
logging.basicConfig(level=logging.INFO)
|
||||
logger = logging.getLogger("member-portal")
|
||||
|
||||
# SECURITY (ops/security#2): OC's generic webhooks cannot send headers, so the shared
|
||||
# secret must travel in the URL path — which means uvicorn writes it into the access
|
||||
# log on every delivery, and the log is read by more things than the secret is. The
|
||||
# token cannot be kept out of the URL; it can be kept out of the retention. Filters
|
||||
# sit on the access logger and on the root handler so both uvicorn's line and any
|
||||
# string the app itself logs get scrubbed.
|
||||
_WEBHOOK_TOKEN_RE = re.compile(r"(/webhook/opencollective/)[^\s\"'?&]+")
|
||||
|
||||
|
||||
def _redact_webhook_path(value):
|
||||
if isinstance(value, str) and "/webhook/opencollective/" in value:
|
||||
return _WEBHOOK_TOKEN_RE.sub(r"\1<redacted>", value)
|
||||
return value
|
||||
|
||||
|
||||
class _RedactWebhookPath(logging.Filter):
|
||||
"""Replace the webhook token in any logged request path with <redacted>."""
|
||||
|
||||
def filter(self, record: logging.LogRecord) -> bool:
|
||||
try:
|
||||
record.msg = _redact_webhook_path(record.msg)
|
||||
if isinstance(record.args, tuple):
|
||||
record.args = tuple(_redact_webhook_path(a) for a in record.args)
|
||||
elif isinstance(record.args, dict):
|
||||
record.args = {k: _redact_webhook_path(v) for k, v in record.args.items()}
|
||||
except Exception:
|
||||
pass # a logging filter must never break logging
|
||||
return True
|
||||
|
||||
|
||||
logging.getLogger("uvicorn.access").addFilter(_RedactWebhookPath())
|
||||
for _handler in logging.getLogger().handlers:
|
||||
_handler.addFilter(_RedactWebhookPath())
|
||||
|
||||
app = FastAPI(title="Inference Cooperative Member Portal")
|
||||
|
||||
# --- Rate limiting (control plane only, not the member inference path) ---
|
||||
@@ -811,6 +845,51 @@ async def fetch_all_member_nodes() -> list[dict]:
|
||||
raise RuntimeError("OC member pagination exceeded 20 pages")
|
||||
|
||||
|
||||
_MEMBER_SET_MIN_RATIO = 0.5
|
||||
|
||||
|
||||
def _member_set_verdict(fetched: int, local_active: int) -> bool:
|
||||
"""Is this member list plausible enough to act on? (member-portal#2)
|
||||
|
||||
Pure decision so it can be tested without a database.
|
||||
|
||||
Guards the case a raise-only fail-safe cannot see: a fetch that SUCCEEDS while
|
||||
returning nothing, or far less than usual. A narrowed token scope, an expired
|
||||
credential with partial access, or a renamed Membership tier on Open Collective
|
||||
all produce that, and the sweep's response to it is to deactivate the membership
|
||||
rather than to fail. An empty collective is legitimate only when we have no
|
||||
active members ourselves.
|
||||
"""
|
||||
if local_active <= 0:
|
||||
return True
|
||||
# Strictly above: a halving of the membership is implausible enough, and refusing
|
||||
# costs a loud error (no action taken) where allowing it costs the membership.
|
||||
return fetched > 0 and fetched > local_active * _MEMBER_SET_MIN_RATIO
|
||||
|
||||
|
||||
def _member_set_guard(fetched: int, label: str) -> None:
|
||||
"""Raise when the fetched member list is implausible, so callers fail safe."""
|
||||
try:
|
||||
conn = get_db()
|
||||
local_active = conn.execute(
|
||||
"SELECT COUNT(*) FROM members WHERE active = 1"
|
||||
).fetchone()[0]
|
||||
conn.close()
|
||||
except Exception as e: # fresh install / unreadable DB: nothing to compare against
|
||||
logger.warning("member-set guard: cannot read the local member count (%s); skipping", e)
|
||||
return
|
||||
if _member_set_verdict(fetched, local_active):
|
||||
logger.info("member-set guard OK (%s): fetched %d, %d active locally",
|
||||
label, fetched, local_active)
|
||||
return
|
||||
raise RuntimeError(
|
||||
f"implausible member list from Open Collective ({label}): fetched {fetched} while "
|
||||
f"{local_active} members are active locally. Refusing to reconcile — the sweep "
|
||||
"fail-safe keeps members active instead. Check the OC token's scopes and expiry, "
|
||||
"and that the Membership tier still exists (fetch_members filters on its name)."
|
||||
)
|
||||
|
||||
|
||||
async def fetch_members() -> list[dict]:
|
||||
"""Return the full active-member list: email, name, slug, role, tier.
|
||||
|
||||
@@ -848,6 +927,9 @@ async def fetch_members() -> list[dict]:
|
||||
"role": role,
|
||||
"tier": tier,
|
||||
})
|
||||
# Guard before ANY caller acts on this: the sweep deactivates members missing
|
||||
# from the list, so an implausible list must fail rather than propagate.
|
||||
_member_set_guard(len(result), "oc members query")
|
||||
return result
|
||||
|
||||
|
||||
|
||||
Reference in new issue
Block a user