Skip to content

Commit 3edff7a

Browse files
irvingpopclaude
andauthored
Harden /slack/events and report real client IP to Sentry (#466)
* Harden /slack/events and report real client IP to Sentry - Return 400 for a /slack/events body that is not a JSON object. Before, json.loads raised and the handler returned an unhandled 500 (Sentry issue 7822478670, an empty curl POST). - Use payload.get() in the url_verification path so a payload with no token or challenge cannot raise KeyError. - Add a Sentry before_send hook that sets REMOTE_ADDR from the X-Forwarded-For entry the ALB appends. The aiohttp integration uses request.remote, so every event showed the ALB's private IP. Hop count is configurable with SENTRY_TRUSTED_PROXY_HOPS (default 1). - poetry update: sentry-sdk 2.74.0, zipcodes 3.0.1, coverage 7.17.2, ruff 0.17.4, pytest-asyncio 1.4.1.post1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Fall back to 1 for invalid SENTRY_TRUSTED_PROXY_HOPS before_send ran int() on the env var. A bad value raised, and Sentry drops the event when before_send raises. Now a value that is not a positive integer logs a warning and uses 1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Reject missing or malformed Slack signature headers cleanly validate_request_signature read X-Slack-Request-Timestamp and X-Slack-Signature with headers[...] and int(). A missing header raised KeyError, and a non-numeric timestamp raised ValueError. Callers catch only InvalidSlackSignature and InvalidTimestamp, so a POST without Slack headers still gave an unhandled 500 and a Sentry error. Now both cases raise InvalidSlackSignature, so every caller returns its normal rejection status (401 for events, 500 for url_verification). Tests cover missing headers and a bad timestamp at the unit level and through /slack/events, and fail on the old code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Detect unhandled errors by log record, not message text The url_verification test matched aiohttp's 'Error handling request' text. Check for an aiohttp.server record with exc_info instead, so a reworded message cannot hide an unhandled exception. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 775a383 commit 3edff7a

7 files changed

Lines changed: 482 additions & 161 deletions

File tree

‎poetry.lock‎

Lines changed: 153 additions & 153 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

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

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,13 @@
1919

2020
async def incoming_event(request):
2121
slack = request.app.plugins["slack"]
22-
payload = await request.json()
22+
try:
23+
payload = await request.json()
24+
except ValueError:
25+
LOG.info("Rejected /slack/events request with a body that is not JSON")
26+
return Response(status=400)
27+
if not isinstance(payload, dict):
28+
return Response(status=400)
2329
LOG.log(5, "Incoming event payload: %s", payload)
2430

2531
if payload.get("type") == "url_verification":
@@ -29,11 +35,11 @@ async def incoming_event(request):
2935
validate_request_signature(
3036
raw_payload.decode("utf-8"), request.headers, slack.signing_secret
3137
)
32-
return Response(body=payload["challenge"])
38+
return Response(body=payload.get("challenge", ""))
3339
except (InvalidSlackSignature, InvalidTimestamp):
3440
return Response(status=500)
35-
elif payload["token"] == slack.verify:
36-
return Response(body=payload["challenge"])
41+
elif slack.verify and payload.get("token") == slack.verify:
42+
return Response(body=payload.get("challenge", ""))
3743
else:
3844
return Response(status=500)
3945

‎pybot/_vendor/slack/sansio.py‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -367,17 +367,26 @@ def validate_request_signature(body: str, headers: MutableMapping, signing_secre
367367
:class:`slack.exceptions.InvalidTimestamp`: when incoming request timestamp is more than 5 minutes old
368368
"""
369369

370-
request_timestamp = int(headers["X-Slack-Request-Timestamp"])
370+
raw_timestamp = headers.get("X-Slack-Request-Timestamp")
371+
slack_signature = headers.get("X-Slack-Signature")
372+
373+
# The second argument below is a marker string, not a computed signature.
374+
if raw_timestamp is None or slack_signature is None:
375+
raise exceptions.InvalidSlackSignature(slack_signature or "", "missing-header")
376+
377+
try:
378+
request_timestamp = int(raw_timestamp)
379+
except ValueError:
380+
raise exceptions.InvalidSlackSignature(slack_signature, "invalid-timestamp") from None
371381

372382
if (int(time.time()) - request_timestamp) > (60 * 5):
373383
raise exceptions.InvalidTimestamp(timestamp=request_timestamp)
374384

375-
slack_signature = headers["X-Slack-Signature"]
376385
calculated_signature = (
377386
"v0="
378387
+ hmac.new(
379388
signing_secret.encode("utf-8"),
380-
f"""v0:{headers["X-Slack-Request-Timestamp"]}:{body}""".encode(),
389+
f"v0:{raw_timestamp}:{body}".encode(),
381390
digestmod=hashlib.sha256,
382391
).hexdigest()
383392
)

‎pybot/sentry.py‎

Lines changed: 66 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,69 @@ def traces_sampler(sampling_context):
9494
return config("SENTRY_TRACES_SAMPLE_RATE", default=1.0, cast=float)
9595

9696

97+
def client_ip_from_forwarded_for(header, hops=1):
98+
"""
99+
Return the client IP that the trusted proxy appended to X-Forwarded-For.
100+
101+
The AWS ALB appends the address of the peer it received the request from,
102+
so the real client is ``hops`` entries from the end. Entries further left
103+
come from the client and can be forged.
104+
"""
105+
addresses = [addr.strip() for addr in str(header).split(",") if addr.strip()]
106+
if hops < 1 or len(addresses) < hops:
107+
return None
108+
return addresses[-hops]
109+
110+
111+
def trusted_proxy_hops():
112+
"""
113+
Return SENTRY_TRUSTED_PROXY_HOPS, or 1 if it is not a positive integer.
114+
115+
This runs inside before_send. If it raised, Sentry would drop the event,
116+
so a bad value must never raise.
117+
"""
118+
raw = config("SENTRY_TRUSTED_PROXY_HOPS", default="1")
119+
try:
120+
hops = int(raw)
121+
except TypeError, ValueError:
122+
hops = 0
123+
if hops < 1:
124+
logging.getLogger(__name__).warning("Invalid SENTRY_TRUSTED_PROXY_HOPS=%r, using 1", raw)
125+
return 1
126+
return hops
127+
128+
129+
def set_client_ip_from_forwarded_for(event):
130+
"""
131+
Replace the ALB's IP with the real client IP in the event.
132+
133+
The aiohttp integration sets REMOTE_ADDR to the TCP peer, which is the ALB.
134+
Only rewrites when REMOTE_ADDR is already present, so PII settings apply.
135+
"""
136+
request = event.get("request") or {}
137+
env = request.get("env") or {}
138+
if "REMOTE_ADDR" not in env:
139+
return event
140+
141+
headers = request.get("headers") or {}
142+
forwarded_for = next(
143+
(value for key, value in headers.items() if key.lower() == "x-forwarded-for"),
144+
None,
145+
)
146+
if not forwarded_for:
147+
return event
148+
149+
client_ip = client_ip_from_forwarded_for(forwarded_for, trusted_proxy_hops())
150+
if client_ip:
151+
env["REMOTE_ADDR"] = client_ip
152+
return event
153+
154+
155+
def before_send(event, hint): # noqa: ARG001
156+
"""Adjust error events before sending to Sentry."""
157+
return set_client_ip_from_forwarded_for(event)
158+
159+
97160
def before_send_transaction(event, hint): # noqa: ARG001
98161
"""
99162
Filter transactions before sending to Sentry.
@@ -107,7 +170,7 @@ def before_send_transaction(event, hint): # noqa: ARG001
107170
if event.get("contexts", {}).get("response", {}).get("status_code") == 404:
108171
return None
109172

110-
return event
173+
return set_client_ip_from_forwarded_for(event)
111174

112175

113176
def init_sentry():
@@ -134,6 +197,8 @@ def init_sentry():
134197
release=config("VERSION", default="1.0.0"),
135198
# Performance Monitoring (Tracing) - use custom sampler to filter health checks
136199
traces_sampler=traces_sampler,
200+
# Report the real client IP from X-Forwarded-For instead of the ALB's IP
201+
before_send=before_send,
137202
# Filter transactions before sending (e.g., drop 404s)
138203
before_send_transaction=before_send_transaction,
139204
# Set profiles_sample_rate to 1.0 to profile 100% of sampled transactions.

‎tests/endpoints/slack/test_slack_events.py‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
import inspect
99
import logging
10+
import time
1011
from unittest.mock import AsyncMock, MagicMock, patch
1112

1213
import pytest
@@ -215,3 +216,105 @@ async def test_get_backend_auth_headers_returns_headers_on_success(self):
215216

216217
assert "Authorization" in result
217218
assert "Bearer" in result["Authorization"]
219+
220+
221+
@pytest.mark.parametrize(
222+
"body",
223+
[b"", b"not json", b"[1, 2]"],
224+
ids=["empty", "invalid", "not-an-object"],
225+
)
226+
async def test_events_endpoint_rejects_bad_body(bot, aiohttp_client, body):
227+
"""A body that is not a JSON object returns 400, not an unhandled 500."""
228+
client = await aiohttp_client(bot)
229+
230+
response = await client.post(
231+
"/slack/events", data=body, headers={"Content-Type": "application/json"}
232+
)
233+
assert response.status == 400
234+
235+
236+
async def test_url_verification_without_token_returns_500(bot, aiohttp_client):
237+
"""A url_verification payload with no token is refused, not a KeyError."""
238+
client = await aiohttp_client(bot)
239+
240+
response = await client.post(
241+
"/slack/events", json={"type": "url_verification", "challenge": "abc"}
242+
)
243+
assert response.status == 500
244+
245+
246+
async def test_url_verification_with_valid_token(bot, aiohttp_client):
247+
client = await aiohttp_client(bot)
248+
249+
response = await client.post(
250+
"/slack/events",
251+
json={"type": "url_verification", "token": "supersecuretoken", "challenge": "abc"},
252+
)
253+
assert response.status == 200
254+
assert await response.text() == "abc"
255+
256+
257+
@pytest.fixture
258+
async def signed_bot():
259+
"""A bot whose Slack plugin has a signing_secret set (production mode)."""
260+
import aiohttp
261+
262+
from pybot._vendor.sirbot import SirBot
263+
from pybot._vendor.sirbot.plugins.slack import SlackPlugin
264+
265+
b = SirBot()
266+
b["http_session"] = aiohttp.ClientSession()
267+
slack = SlackPlugin(
268+
token="token",
269+
signing_secret="signing-secret",
270+
bot_user_id="bot_user_id",
271+
bot_id="bot_id",
272+
)
273+
endpoints.slack.create_endpoints(slack)
274+
b.load_plugin(slack)
275+
await slack._initialize_api(b)
276+
yield b
277+
await b["http_session"].close()
278+
279+
280+
BAD_SIGNATURE_HEADERS = [
281+
pytest.param({}, id="no-headers"),
282+
pytest.param({"X-Slack-Request-Timestamp": "abc", "X-Slack-Signature": "v0=x"}, id="bad-ts"),
283+
pytest.param({"X-Slack-Request-Timestamp": str(int(time.time()))}, id="missing-signature"),
284+
pytest.param({"X-Slack-Signature": "v0=x"}, id="missing-timestamp"),
285+
]
286+
287+
288+
@pytest.mark.parametrize("headers", BAD_SIGNATURE_HEADERS)
289+
async def test_signed_events_endpoint_rejects_bad_headers(signed_bot, aiohttp_client, headers):
290+
"""Missing or malformed Slack signature headers return 401, not a 500."""
291+
client = await aiohttp_client(signed_bot)
292+
293+
response = await client.post("/slack/events", json={}, headers=headers)
294+
assert response.status == 401
295+
296+
297+
@pytest.mark.parametrize("headers", BAD_SIGNATURE_HEADERS)
298+
async def test_signed_url_verification_rejects_bad_headers(
299+
signed_bot, aiohttp_client, headers, caplog
300+
):
301+
"""url_verification keeps its 500 for a failed signature, without an unhandled exception."""
302+
client = await aiohttp_client(signed_bot)
303+
caplog.set_level(logging.ERROR, logger="aiohttp.server")
304+
305+
response = await client.post(
306+
"/slack/events", json={"type": "url_verification", "challenge": "abc"}, headers=headers
307+
)
308+
assert response.status == 500
309+
# aiohttp logs an unhandled handler exception on "aiohttp.server" with exc_info.
310+
# Check the record, not the message text, so a reworded message cannot hide it.
311+
unhandled = [r for r in caplog.records if r.name == "aiohttp.server" and r.exc_info]
312+
assert not unhandled
313+
314+
315+
@pytest.mark.parametrize("path", ["/slack/commands", "/slack/actions"])
316+
async def test_signed_commands_and_actions_reject_missing_headers(signed_bot, aiohttp_client, path):
317+
client = await aiohttp_client(signed_bot)
318+
319+
response = await client.post(path, data={"payload": "{}"})
320+
assert response.status == 401
Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
"""Tests for replacing the ALB IP with the real client IP in Sentry events."""
2+
3+
import pytest
4+
5+
from pybot.sentry import (
6+
before_send,
7+
before_send_transaction,
8+
client_ip_from_forwarded_for,
9+
)
10+
11+
ALB_IP = "172.20.15.250"
12+
13+
14+
def make_event(headers, env=None):
15+
return {
16+
"request": {
17+
"headers": headers,
18+
"env": {"REMOTE_ADDR": ALB_IP} if env is None else env,
19+
}
20+
}
21+
22+
23+
@pytest.mark.parametrize(
24+
("header", "hops", "expected"),
25+
[
26+
("203.0.113.7", 1, "203.0.113.7"),
27+
("1.1.1.1, 203.0.113.7", 1, "203.0.113.7"), # leftmost entry is client-forged
28+
("1.1.1.1, 203.0.113.7", 2, "1.1.1.1"),
29+
(" 203.0.113.7 , ", 1, "203.0.113.7"),
30+
("203.0.113.7", 2, None),
31+
("", 1, None),
32+
("203.0.113.7", 0, None),
33+
],
34+
)
35+
def test_client_ip_from_forwarded_for(header, hops, expected):
36+
assert client_ip_from_forwarded_for(header, hops) == expected
37+
38+
39+
def test_before_send_uses_forwarded_for():
40+
event = make_event({"X-Forwarded-For": "198.51.100.1, 203.0.113.7"})
41+
assert before_send(event, {})["request"]["env"]["REMOTE_ADDR"] == "203.0.113.7"
42+
43+
44+
def test_before_send_header_lookup_is_case_insensitive():
45+
event = make_event({"x-forwarded-for": "203.0.113.7"})
46+
assert before_send(event, {})["request"]["env"]["REMOTE_ADDR"] == "203.0.113.7"
47+
48+
49+
def test_before_send_without_header_keeps_remote_addr():
50+
event = make_event({"User-Agent": "curl/8.7.1"})
51+
assert before_send(event, {})["request"]["env"]["REMOTE_ADDR"] == ALB_IP
52+
53+
54+
def test_before_send_does_not_add_ip_when_remote_addr_absent():
55+
"""When PII settings removed REMOTE_ADDR, do not add the IP back."""
56+
event = make_event({"X-Forwarded-For": "203.0.113.7"}, env={})
57+
assert "REMOTE_ADDR" not in before_send(event, {})["request"]["env"]
58+
59+
60+
def test_before_send_without_request():
61+
assert before_send({"message": "hi"}, {}) == {"message": "hi"}
62+
63+
64+
def test_before_send_respects_trusted_hops(monkeypatch):
65+
monkeypatch.setenv("SENTRY_TRUSTED_PROXY_HOPS", "2")
66+
event = make_event({"X-Forwarded-For": "198.51.100.1, 203.0.113.7"})
67+
assert before_send(event, {})["request"]["env"]["REMOTE_ADDR"] == "198.51.100.1"
68+
69+
70+
def test_before_send_transaction_uses_forwarded_for():
71+
event = make_event({"X-Forwarded-For": "203.0.113.7"})
72+
assert before_send_transaction(event, {})["request"]["env"]["REMOTE_ADDR"] == "203.0.113.7"
73+
74+
75+
def test_before_send_transaction_still_drops_404():
76+
event = make_event({"X-Forwarded-For": "203.0.113.7"})
77+
event["contexts"] = {"response": {"status_code": 404}}
78+
assert before_send_transaction(event, {}) is None
79+
80+
81+
@pytest.mark.parametrize("value", ["abc", "", "0", "-2", "1.5"])
82+
def test_before_send_invalid_hops_falls_back_to_one(monkeypatch, value):
83+
"""A bad SENTRY_TRUSTED_PROXY_HOPS must not raise, or Sentry drops the event."""
84+
monkeypatch.setenv("SENTRY_TRUSTED_PROXY_HOPS", value)
85+
event = make_event({"X-Forwarded-For": "198.51.100.1, 203.0.113.7"})
86+
assert before_send(event, {})["request"]["env"]["REMOTE_ADDR"] == "203.0.113.7"

0 commit comments

Comments
 (0)