Harden /slack/events and report real client IP to Sentry

pybot/_vendor/sirbot/plugins/slack/endpoints.py

@@ -19,7 +19,13 @@
 
 async def incoming_event(request):
     slack = request.app.plugins["slack"]
-    payload = await request.json()
+    try:
+        payload = await request.json()
+    except ValueError:
+        LOG.info("Rejected /slack/events request with a body that is not JSON")
+        return Response(status=400)
+    if not isinstance(payload, dict):
+        return Response(status=400)
     LOG.log(5, "Incoming event payload: %s", payload)
 
     if payload.get("type") == "url_verification":
@@ -29,11 +35,11 @@ async def incoming_event(request):
                 validate_request_signature(
                     raw_payload.decode("utf-8"), request.headers, slack.signing_secret
                 )
-                return Response(body=payload["challenge"])
+                return Response(body=payload.get("challenge", ""))
             except (InvalidSlackSignature, InvalidTimestamp):
                 return Response(status=500)
-        elif payload["token"] == slack.verify:
-            return Response(body=payload["challenge"])
+        elif slack.verify and payload.get("token") == slack.verify:
+            return Response(body=payload.get("challenge", ""))
         else:
             return Response(status=500)
 

pybot/_vendor/slack/sansio.py

@@ -367,17 +367,26 @@ def validate_request_signature(body: str, headers: MutableMapping, signing_secre
         :class:`slack.exceptions.InvalidTimestamp`: when incoming request timestamp is more than 5 minutes old
     """
 
-    request_timestamp = int(headers["X-Slack-Request-Timestamp"])
+    raw_timestamp = headers.get("X-Slack-Request-Timestamp")
+    slack_signature = headers.get("X-Slack-Signature")
+
+    # The second argument below is a marker string, not a computed signature.
+    if raw_timestamp is None or slack_signature is None:
+        raise exceptions.InvalidSlackSignature(slack_signature or "", "missing-header")
+
+    try:
+        request_timestamp = int(raw_timestamp)
+    except ValueError:
+        raise exceptions.InvalidSlackSignature(slack_signature, "invalid-timestamp") from None
 
     if (int(time.time()) - request_timestamp) > (60 * 5):
         raise exceptions.InvalidTimestamp(timestamp=request_timestamp)
 
-    slack_signature = headers["X-Slack-Signature"]
     calculated_signature = (
         "v0="
         + hmac.new(
             signing_secret.encode("utf-8"),
-            f"""v0:{headers["X-Slack-Request-Timestamp"]}:{body}""".encode(),
+            f"v0:{raw_timestamp}:{body}".encode(),
             digestmod=hashlib.sha256,
         ).hexdigest()
     )

pybot/sentry.py

@@ -94,6 +94,69 @@ def traces_sampler(sampling_context):
     return config("SENTRY_TRACES_SAMPLE_RATE", default=1.0, cast=float)
 
 
+def client_ip_from_forwarded_for(header, hops=1):
+    """
+    Return the client IP that the trusted proxy appended to X-Forwarded-For.
+
+    The AWS ALB appends the address of the peer it received the request from,
+    so the real client is ``hops`` entries from the end. Entries further left
+    come from the client and can be forged.
+    """
+    addresses = [addr.strip() for addr in str(header).split(",") if addr.strip()]
+    if hops < 1 or len(addresses) < hops:
+        return None
+    return addresses[-hops]
+
+
+def trusted_proxy_hops():
+    """
+    Return SENTRY_TRUSTED_PROXY_HOPS, or 1 if it is not a positive integer.
+
+    This runs inside before_send. If it raised, Sentry would drop the event,
+    so a bad value must never raise.
+    """
+    raw = config("SENTRY_TRUSTED_PROXY_HOPS", default="1")
+    try:
+        hops = int(raw)
+    except TypeError, ValueError:
+        hops = 0
+    if hops < 1:
+        logging.getLogger(__name__).warning("Invalid SENTRY_TRUSTED_PROXY_HOPS=%r, using 1", raw)
+        return 1
+    return hops
+
+
+def set_client_ip_from_forwarded_for(event):
+    """
+    Replace the ALB's IP with the real client IP in the event.
+
+    The aiohttp integration sets REMOTE_ADDR to the TCP peer, which is the ALB.
+    Only rewrites when REMOTE_ADDR is already present, so PII settings apply.
+    """
+    request = event.get("request") or {}
+    env = request.get("env") or {}
+    if "REMOTE_ADDR" not in env:
+        return event
+
+    headers = request.get("headers") or {}
+    forwarded_for = next(
+        (value for key, value in headers.items() if key.lower() == "x-forwarded-for"),
+        None,
+    )
+    if not forwarded_for:
+        return event
+
+    client_ip = client_ip_from_forwarded_for(forwarded_for, trusted_proxy_hops())
+    if client_ip:
+        env["REMOTE_ADDR"] = client_ip
+    return event
+
+
+def before_send(event, hint):  # noqa: ARG001
+    """Adjust error events before sending to Sentry."""
+    return set_client_ip_from_forwarded_for(event)
+
+
 def before_send_transaction(event, hint):  # noqa: ARG001
     """
     Filter transactions before sending to Sentry.
@@ -107,7 +170,7 @@ def before_send_transaction(event, hint):  # noqa: ARG001
     if event.get("contexts", {}).get("response", {}).get("status_code") == 404:
         return None
 
-    return event
+    return set_client_ip_from_forwarded_for(event)
 
 
 def init_sentry():
@@ -134,6 +197,8 @@ def init_sentry():
         release=config("VERSION", default="1.0.0"),
         # Performance Monitoring (Tracing) - use custom sampler to filter health checks
         traces_sampler=traces_sampler,
+        # Report the real client IP from X-Forwarded-For instead of the ALB's IP
+        before_send=before_send,
         # Filter transactions before sending (e.g., drop 404s)
         before_send_transaction=before_send_transaction,
         # Set profiles_sample_rate to 1.0 to profile 100% of sampled transactions.

tests/endpoints/slack/test_slack_events.py

@@ -7,6 +7,7 @@
 
 import inspect
 import logging
+import time
 from unittest.mock import AsyncMock, MagicMock, patch
 
 import pytest
@@ -215,3 +216,105 @@ async def test_get_backend_auth_headers_returns_headers_on_success(self):
 
         assert "Authorization" in result
         assert "Bearer" in result["Authorization"]
+
+
+@pytest.mark.parametrize(
+    "body",
+    [b"", b"not json", b"[1, 2]"],
+    ids=["empty", "invalid", "not-an-object"],
+)
+async def test_events_endpoint_rejects_bad_body(bot, aiohttp_client, body):
+    """A body that is not a JSON object returns 400, not an unhandled 500."""
+    client = await aiohttp_client(bot)
+
+    response = await client.post(
+        "/slack/events", data=body, headers={"Content-Type": "application/json"}
+    )
+    assert response.status == 400
+
+
+async def test_url_verification_without_token_returns_500(bot, aiohttp_client):
+    """A url_verification payload with no token is refused, not a KeyError."""
+    client = await aiohttp_client(bot)
+
+    response = await client.post(
+        "/slack/events", json={"type": "url_verification", "challenge": "abc"}
+    )
+    assert response.status == 500
+
+
+async def test_url_verification_with_valid_token(bot, aiohttp_client):
+    client = await aiohttp_client(bot)
+
+    response = await client.post(
+        "/slack/events",
+        json={"type": "url_verification", "token": "supersecuretoken", "challenge": "abc"},
+    )
+    assert response.status == 200
+    assert await response.text() == "abc"
+
+
+@pytest.fixture
+async def signed_bot():
+    """A bot whose Slack plugin has a signing_secret set (production mode)."""
+    import aiohttp
+
+    from pybot._vendor.sirbot import SirBot
+    from pybot._vendor.sirbot.plugins.slack import SlackPlugin
+
+    b = SirBot()
+    b["http_session"] = aiohttp.ClientSession()
+    slack = SlackPlugin(
+        token="token",
+        signing_secret="signing-secret",
+        bot_user_id="bot_user_id",
+        bot_id="bot_id",
+    )
+    endpoints.slack.create_endpoints(slack)
+    b.load_plugin(slack)
+    await slack._initialize_api(b)
+    yield b
+    await b["http_session"].close()
+
+
+BAD_SIGNATURE_HEADERS = [
+    pytest.param({}, id="no-headers"),
+    pytest.param({"X-Slack-Request-Timestamp": "abc", "X-Slack-Signature": "v0=x"}, id="bad-ts"),
+    pytest.param({"X-Slack-Request-Timestamp": str(int(time.time()))}, id="missing-signature"),
+    pytest.param({"X-Slack-Signature": "v0=x"}, id="missing-timestamp"),
+]
+
+
+@pytest.mark.parametrize("headers", BAD_SIGNATURE_HEADERS)
+async def test_signed_events_endpoint_rejects_bad_headers(signed_bot, aiohttp_client, headers):
+    """Missing or malformed Slack signature headers return 401, not a 500."""
+    client = await aiohttp_client(signed_bot)
+
+    response = await client.post("/slack/events", json={}, headers=headers)
+    assert response.status == 401
+
+
+@pytest.mark.parametrize("headers", BAD_SIGNATURE_HEADERS)
+async def test_signed_url_verification_rejects_bad_headers(
+    signed_bot, aiohttp_client, headers, caplog
+):
+    """url_verification keeps its 500 for a failed signature, without an unhandled exception."""
+    client = await aiohttp_client(signed_bot)
+    caplog.set_level(logging.ERROR, logger="aiohttp.server")
+
+    response = await client.post(
+        "/slack/events", json={"type": "url_verification", "challenge": "abc"}, headers=headers
+    )
+    assert response.status == 500
+    # aiohttp logs an unhandled handler exception on "aiohttp.server" with exc_info.
+    # Check the record, not the message text, so a reworded message cannot hide it.
+    unhandled = [r for r in caplog.records if r.name == "aiohttp.server" and r.exc_info]
+    assert not unhandled
+
+
+@pytest.mark.parametrize("path", ["/slack/commands", "/slack/actions"])
+async def test_signed_commands_and_actions_reject_missing_headers(signed_bot, aiohttp_client, path):
+    client = await aiohttp_client(signed_bot)
+
+    response = await client.post(path, data={"payload": "{}"})
+    assert response.status == 401

tests/unit/test_sentry_client_ip.py

@@ -0,0 +1,86 @@
+"""Tests for replacing the ALB IP with the real client IP in Sentry events."""
+
+import pytest
+
+from pybot.sentry import (
+    before_send,
+    before_send_transaction,
+    client_ip_from_forwarded_for,
+)
+
+ALB_IP = "172.20.15.250"
+
+
+def make_event(headers, env=None):
+    return {
+        "request": {
+            "headers": headers,
+            "env": {"REMOTE_ADDR": ALB_IP} if env is None else env,
+        }
+    }
+
+
+@pytest.mark.parametrize(
+    ("header", "hops", "expected"),
+    [
+        ("203.0.113.7", 1, "203.0.113.7"),
+        ("1.1.1.1, 203.0.113.7", 1, "203.0.113.7"),  # leftmost entry is client-forged
+        ("1.1.1.1, 203.0.113.7", 2, "1.1.1.1"),
+        (" 203.0.113.7 , ", 1, "203.0.113.7"),
+        ("203.0.113.7", 2, None),
+        ("", 1, None),
+        ("203.0.113.7", 0, None),
+    ],
+)
+def test_client_ip_from_forwarded_for(header, hops, expected):
+    assert client_ip_from_forwarded_for(header, hops) == expected
+
+
+def test_before_send_uses_forwarded_for():
+    event = make_event({"X-Forwarded-For": "198.51.100.1, 203.0.113.7"})
+    assert before_send(event, {})["request"]["env"]["REMOTE_ADDR"] == "203.0.113.7"
+
+
+def test_before_send_header_lookup_is_case_insensitive():
+    event = make_event({"x-forwarded-for": "203.0.113.7"})
+    assert before_send(event, {})["request"]["env"]["REMOTE_ADDR"] == "203.0.113.7"
+
+
+def test_before_send_without_header_keeps_remote_addr():
+    event = make_event({"User-Agent": "curl/8.7.1"})
+    assert before_send(event, {})["request"]["env"]["REMOTE_ADDR"] == ALB_IP
+
+
+def test_before_send_does_not_add_ip_when_remote_addr_absent():
+    """When PII settings removed REMOTE_ADDR, do not add the IP back."""
+    event = make_event({"X-Forwarded-For": "203.0.113.7"}, env={})
+    assert "REMOTE_ADDR" not in before_send(event, {})["request"]["env"]
+
+
+def test_before_send_without_request():
+    assert before_send({"message": "hi"}, {}) == {"message": "hi"}
+
+
+def test_before_send_respects_trusted_hops(monkeypatch):
+    monkeypatch.setenv("SENTRY_TRUSTED_PROXY_HOPS", "2")
+    event = make_event({"X-Forwarded-For": "198.51.100.1, 203.0.113.7"})
+    assert before_send(event, {})["request"]["env"]["REMOTE_ADDR"] == "198.51.100.1"
+
+
+def test_before_send_transaction_uses_forwarded_for():
+    event = make_event({"X-Forwarded-For": "203.0.113.7"})
+    assert before_send_transaction(event, {})["request"]["env"]["REMOTE_ADDR"] == "203.0.113.7"
+
+
+def test_before_send_transaction_still_drops_404():
+    event = make_event({"X-Forwarded-For": "203.0.113.7"})
+    event["contexts"] = {"response": {"status_code": 404}}
+    assert before_send_transaction(event, {}) is None
+
+
+@pytest.mark.parametrize("value", ["abc", "", "0", "-2", "1.5"])
+def test_before_send_invalid_hops_falls_back_to_one(monkeypatch, value):
+    """A bad SENTRY_TRUSTED_PROXY_HOPS must not raise, or Sentry drops the event."""
+    monkeypatch.setenv("SENTRY_TRUSTED_PROXY_HOPS", value)
+    event = make_event({"X-Forwarded-For": "198.51.100.1, 203.0.113.7"})
+    assert before_send(event, {})["request"]["env"]["REMOTE_ADDR"] == "203.0.113.7"