v0.18.0 Slice 3: webhook tightening — mandatory secret + dev-bypass

`GITEA_WEBHOOK_SECRET` is now mandatory at startup. The framework refuses to load_config() when the env var is empty unless the operator opts into the dev-bypass with `RFC_APP_INSECURE_WEBHOOKS=1`. This is the v0.18.0 startup-loud-failure shape — the pre-v0.18.0 "silently accept unsigned POSTs when the secret is empty" path is the bug the proposal targets. `webhooks.receive`: * Defense in depth: refuses 500 if the secret is empty at request time and the dev-bypass is not set (catches the case where something mutates env after startup). * Logs a loud warning every time a webhook lands under the bypass — so a misconfigured production deployment shows up in the logs even if the operator missed the warning at boot. * Adds an INFO log at the unknown-repo branch (previously silently 200-OK'd a hook on a fork or a stale Gitea binding). `tmp_env` fixture binds a fake secret so the existing 277 tests boot cleanly; the new test_webhooks_vertical.py exercises both the production-secret path (valid signature, invalid signature, missing signature) and the dev-bypass path (config loads with empty secret when bypass set, refuses without it). 7 new tests; full suite: 284 passed.
2026-05-28 07:27:23 -07:00
parent e9fdc478f6
commit d3daa97264
4 changed files with 258 additions and 3 deletions
@@ -60,6 +60,20 @@ def load_config() -> Config:

    enabled = [m.strip() for m in _optional("ENABLED_MODELS", "claude").split(",") if m.strip()]

+    # v0.18.0: `GITEA_WEBHOOK_SECRET` is now mandatory (per the
+    # email + webhook hygiene proposal). An empty value used to
+    # silently accept unsigned webhook POSTs — that was the
+    # invisible-failure shape the proposal targets. Now the
+    # framework refuses to start when the secret is empty unless
+    # the operator opts into the dev-bypass with
+    # `RFC_APP_INSECURE_WEBHOOKS=1`. Local-dev deployments without
+    # a wired Gitea hook set the bypass; production MUST NOT.
+    insecure_webhooks = os.environ.get("RFC_APP_INSECURE_WEBHOOKS", "").strip() == "1"
+    if insecure_webhooks:
+        webhook_secret = _optional("GITEA_WEBHOOK_SECRET")
+    else:
+        webhook_secret = _required("GITEA_WEBHOOK_SECRET")
+
    return Config(
        gitea_url=_required("GITEA_URL").rstrip("/"),
        gitea_bot_user=_required("GITEA_BOT_USER"),
@@ -72,7 +86,7 @@ def load_config() -> Config:
        secret_key=_required("SECRET_KEY"),
        database_path=database_path,
        owner_gitea_login=_optional("OWNER_GITEA_LOGIN"),
-        webhook_secret=_optional("GITEA_WEBHOOK_SECRET"),
+        webhook_secret=webhook_secret,
        enabled_models=enabled,
        anthropic_api_key=_optional("ANTHROPIC_API_KEY"),
        google_api_key=_optional("GOOGLE_API_KEY"),
@@ -12,6 +12,7 @@ import hashlib
 import hmac
 import json
 import logging
+import os

 from fastapi import APIRouter, Header, HTTPException, Request

@@ -40,7 +41,27 @@ def make_router(config: Config, gitea: Gitea) -> APIRouter:
        x_gitea_signature: str = Header(default=""),
    ):
        body = await request.body()
-        if config.webhook_secret:
+        # v0.18.0: defense in depth. config.py refuses to start
+        # when the secret is empty unless `RFC_APP_INSECURE_WEBHOOKS=1`
+        # is set; this branch catches the dev-bypass case (the only
+        # path where `config.webhook_secret` can be empty) and surfaces
+        # it loudly to the client. A POST that lands here with an
+        # empty secret on a production deployment indicates a
+        # mis-configuration (somebody flipped the bypass in prod),
+        # and the loud 500 is the proposal's whole point.
+        insecure = os.environ.get("RFC_APP_INSECURE_WEBHOOKS", "").strip() == "1"
+        if not config.webhook_secret:
+            if not insecure:
+                log.error(
+                    "webhook receiver misconfigured: GITEA_WEBHOOK_SECRET is empty "
+                    "and RFC_APP_INSECURE_WEBHOOKS=1 is not set"
+                )
+                raise HTTPException(status_code=500, detail="Webhook receiver misconfigured")
+            log.warning(
+                "webhook receiver running with RFC_APP_INSECURE_WEBHOOKS=1 — "
+                "signature verification is DISABLED. Production deployments MUST NOT set this."
+            )
+        else:
            if not _verify_signature(body, x_gitea_signature, config.webhook_secret):
                raise HTTPException(status_code=401, detail="Invalid signature")

@@ -68,6 +89,17 @@ def make_router(config: Config, gitea: Gitea) -> APIRouter:
                slug = _slug_for_repo(repo_full)
                if slug:
                    await cache.refresh_rfc_repo(config, gitea, slug)
+                else:
+                    # v0.18.0: the proposal's "unknown-repo logging"
+                    # gesture — a hook on a fork or a stale repo binding
+                    # used to silently 200-OK here, hiding the
+                    # misconfiguration. Now the operator sees it in
+                    # the log.
+                    log.info(
+                        "webhook received for unknown repo: repo_full=%s event=%s "
+                        "(no cached_rfcs row matched; hook may be on a fork or stale)",
+                        repo_full, event,
+                    )
        except Exception:
            log.exception("webhook refresh failed")
            raise HTTPException(status_code=500, detail="Refresh failed")
@@ -438,7 +438,11 @@ def tmp_env(monkeypatch):
        "SECRET_KEY": "test-secret-key-for-cookies",
        "DATABASE_PATH": str(db_path),
        "OWNER_GITEA_LOGIN": "ben",
-        "GITEA_WEBHOOK_SECRET": "",
+        # v0.18.0: `GITEA_WEBHOOK_SECRET` is now mandatory at startup
+        # per the email + webhook hygiene proposal. Tests bind a fake
+        # value so the framework boots; tests that want to exercise
+        # the dev-bypass path monkeypatch `RFC_APP_INSECURE_WEBHOOKS=1`.
+        "GITEA_WEBHOOK_SECRET": "test-webhook-secret-for-signature-verification",
        "ENABLED_MODELS": "claude",
    }
    for k, v in env.items():
@@ -0,0 +1,205 @@
+"""End-to-end integration tests for the Gitea webhook receiver
+(v0.18.0 Slice 3 — webhook tightening per the email + webhook
+hygiene proposal).
+
+The release changes the receiver from "verifies the signature only
+when a secret is configured; silently accepts unsigned POSTs
+otherwise" to "requires the secret unless `RFC_APP_INSECURE_WEBHOOKS=1`
+is set as an explicit dev-bypass." The startup-time check lives in
+`config.load_config()`; the request-time check lives in
+`webhooks.receive`.
+
+These tests prove:
+
+  * The framework refuses to start when `GITEA_WEBHOOK_SECRET` is
+    empty and the dev-bypass is not set.
+  * The dev-bypass (`RFC_APP_INSECURE_WEBHOOKS=1`) lets the
+    framework boot with an empty secret AND lets webhook POSTs
+    land without signature verification (a loud-warning log line
+    surfaces, but the request is accepted).
+  * Default path (secret bound): a POST with a valid signature
+    lands; a POST with an invalid signature gets 401; a POST with
+    no signature gets 401.
+  * Unknown-repo POSTs surface in the log (the "stale Gitea hook"
+    case the proposal targets).
+"""
+from __future__ import annotations
+
+import hashlib
+import hmac
+import json
+import logging
+
+import pytest
+
+from test_propose_vertical import (  # noqa: F401
+    FakeGitea,
+    app_with_fake_gitea,
+    tmp_env,
+)
+
+
+# ---------------------------------------------------------------------------
+# Startup-time secret check (config.load_config)
+# ---------------------------------------------------------------------------
+
+
+def test_config_refuses_to_load_with_empty_secret_and_no_bypass(monkeypatch, tmp_path):
+    """The framework MUST refuse to start when `GITEA_WEBHOOK_SECRET`
+    is empty unless `RFC_APP_INSECURE_WEBHOOKS=1` is set. This is
+    the v0.18.0 startup-loud-failure shape — silent acceptance was
+    the bug."""
+    monkeypatch.setenv("GITEA_URL", "http://gitea.test")
+    monkeypatch.setenv("GITEA_BOT_USER", "rfc-bot")
+    monkeypatch.setenv("GITEA_BOT_TOKEN", "bot-token")
+    monkeypatch.setenv("GITEA_ORG", "wiggleverse")
+    monkeypatch.setenv("OAUTH_CLIENT_ID", "cid")
+    monkeypatch.setenv("OAUTH_CLIENT_SECRET", "csec")
+    monkeypatch.setenv("SECRET_KEY", "test-secret-key")
+    monkeypatch.setenv("DATABASE_PATH", str(tmp_path / "test.db"))
+    monkeypatch.setenv("GITEA_WEBHOOK_SECRET", "")
+    monkeypatch.delenv("RFC_APP_INSECURE_WEBHOOKS", raising=False)
+
+    from app.config import load_config
+    with pytest.raises(RuntimeError, match="GITEA_WEBHOOK_SECRET"):
+        load_config()
+
+
+def test_config_loads_with_empty_secret_when_bypass_is_set(monkeypatch, tmp_path):
+    """The explicit `RFC_APP_INSECURE_WEBHOOKS=1` opt-in lets the
+    framework boot with an empty webhook secret. This is the
+    local-dev escape hatch."""
+    monkeypatch.setenv("GITEA_URL", "http://gitea.test")
+    monkeypatch.setenv("GITEA_BOT_USER", "rfc-bot")
+    monkeypatch.setenv("GITEA_BOT_TOKEN", "bot-token")
+    monkeypatch.setenv("GITEA_ORG", "wiggleverse")
+    monkeypatch.setenv("OAUTH_CLIENT_ID", "cid")
+    monkeypatch.setenv("OAUTH_CLIENT_SECRET", "csec")
+    monkeypatch.setenv("SECRET_KEY", "test-secret-key")
+    monkeypatch.setenv("DATABASE_PATH", str(tmp_path / "test.db"))
+    monkeypatch.setenv("GITEA_WEBHOOK_SECRET", "")
+    monkeypatch.setenv("RFC_APP_INSECURE_WEBHOOKS", "1")
+
+    from app.config import load_config
+    cfg = load_config()  # MUST NOT raise
+    assert cfg.webhook_secret == ""
+
+
+def test_config_loads_with_secret_set(monkeypatch, tmp_path):
+    """Sanity: the happy path (secret bound, bypass not set) loads
+    cleanly."""
+    monkeypatch.setenv("GITEA_URL", "http://gitea.test")
+    monkeypatch.setenv("GITEA_BOT_USER", "rfc-bot")
+    monkeypatch.setenv("GITEA_BOT_TOKEN", "bot-token")
+    monkeypatch.setenv("GITEA_ORG", "wiggleverse")
+    monkeypatch.setenv("OAUTH_CLIENT_ID", "cid")
+    monkeypatch.setenv("OAUTH_CLIENT_SECRET", "csec")
+    monkeypatch.setenv("SECRET_KEY", "test-secret-key")
+    monkeypatch.setenv("DATABASE_PATH", str(tmp_path / "test.db"))
+    monkeypatch.setenv("GITEA_WEBHOOK_SECRET", "my-real-secret")
+    monkeypatch.delenv("RFC_APP_INSECURE_WEBHOOKS", raising=False)
+
+    from app.config import load_config
+    cfg = load_config()
+    assert cfg.webhook_secret == "my-real-secret"
+
+
+# ---------------------------------------------------------------------------
+# Request-time signature verification (webhooks.receive)
+#
+# The default `app_with_fake_gitea` fixture binds
+# `GITEA_WEBHOOK_SECRET=test-webhook-secret-for-signature-verification`,
+# so these tests exercise the production path.
+# ---------------------------------------------------------------------------
+
+
+_SECRET = "test-webhook-secret-for-signature-verification"
+
+
+def _sign(body: bytes) -> str:
+    return hmac.new(_SECRET.encode("utf-8"), body, hashlib.sha256).hexdigest()
+
+
+def test_webhook_post_with_valid_signature_accepted(app_with_fake_gitea):
+    from fastapi.testclient import TestClient
+
+    app, _fake = app_with_fake_gitea
+    with TestClient(app) as client:
+        body = json.dumps({"repository": {"full_name": "wiggleverse/meta"}}).encode()
+        sig = _sign(body)
+        r = client.post(
+            "/api/webhooks/gitea",
+            content=body,
+            headers={
+                "X-Gitea-Event": "push",
+                "X-Gitea-Signature": sig,
+                "Content-Type": "application/json",
+            },
+        )
+        assert r.status_code == 200, r.text
+
+
+def test_webhook_post_with_invalid_signature_refused_401(app_with_fake_gitea):
+    from fastapi.testclient import TestClient
+
+    app, _fake = app_with_fake_gitea
+    with TestClient(app) as client:
+        body = json.dumps({"repository": {"full_name": "wiggleverse/meta"}}).encode()
+        r = client.post(
+            "/api/webhooks/gitea",
+            content=body,
+            headers={
+                "X-Gitea-Event": "push",
+                "X-Gitea-Signature": "0" * 64,  # wrong signature
+                "Content-Type": "application/json",
+            },
+        )
+        assert r.status_code == 401
+
+
+def test_webhook_post_with_missing_signature_refused_401(app_with_fake_gitea):
+    from fastapi.testclient import TestClient
+
+    app, _fake = app_with_fake_gitea
+    with TestClient(app) as client:
+        body = json.dumps({"repository": {"full_name": "wiggleverse/meta"}}).encode()
+        r = client.post(
+            "/api/webhooks/gitea",
+            content=body,
+            headers={
+                "X-Gitea-Event": "push",
+                "Content-Type": "application/json",
+            },
+        )
+        assert r.status_code == 401
+
+
+# ---------------------------------------------------------------------------
+# Unknown-repo logging (the "stale hook on a fork" surface)
+# ---------------------------------------------------------------------------
+
+
+def test_webhook_unknown_repo_logs_at_info(app_with_fake_gitea, caplog):
+    """Per the proposal: a hook on a fork or a stale Gitea binding
+    used to silently 200-OK. v0.18.0 surfaces it as an INFO log."""
+    from fastapi.testclient import TestClient
+
+    app, _fake = app_with_fake_gitea
+    with TestClient(app) as client:
+        body = json.dumps({"repository": {"full_name": "someone-else/unrelated"}}).encode()
+        sig = _sign(body)
+        with caplog.at_level(logging.INFO, logger="app.webhooks"):
+            r = client.post(
+                "/api/webhooks/gitea",
+                content=body,
+                headers={
+                    "X-Gitea-Event": "push",
+                    "X-Gitea-Signature": sig,
+                    "Content-Type": "application/json",
+                },
+            )
+        assert r.status_code == 200  # the handler still 200s; surface is the log line
+        assert any(
+            "unknown repo" in rec.message and "someone-else/unrelated" in rec.message
+            for rec in caplog.records
+        ), f"expected unknown-repo log line; got: {[r.message for r in caplog.records]}"