P0 hardening: delivery auth, trusted proxy IP extraction, root cleanup#40
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 150c0f78f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if "/" in candidate: | ||
| networks.append(ipaddress.ip_network(candidate, strict=False)) | ||
| else: | ||
| networks.append(ipaddress.ip_network(f"{candidate}/32", strict=False)) |
There was a problem hiding this comment.
When RATE_LIMIT_TRUSTED_PROXIES contains a bare IPv6 address (for example 2001:db8::1), this branch appends /32, which expands that single host into a very large network (2001:db8::/32). In _is_trusted_proxy, any client from that range will then be treated as a trusted proxy, so attacker-controlled forwarded headers can be accepted and IP-based limiting can be bypassed in IPv6 deployments.
Useful? React with 👍 / 👎.
| max_keepalive_connections=5, | ||
| read_timeout=30.0, | ||
| ) | ||
| self.alerts_api_key = os.getenv("ALERTS_API_KEY", "").strip() |
There was a problem hiding this comment.
The backend auth dependency accepts either ALERTS_API_KEY or ALERTS_API_KEYS, but the web client only reads ALERTS_API_KEY here. If an environment is configured with only ALERTS_API_KEYS (which this change explicitly supports), /api/alerts/digest-deliver will send no auth header and the backend returns 401, breaking digest delivery from the web UI.
Useful? React with 👍 / 👎.
f1sherFM
force-pushed
the
chore/p0-security-and-root-cleanup
branch
from
ad43fdf to
c5cb76f
Compare
1 participant
Summary
This PR includes the first implementation batch from code review issue #39.
Included changes
/alerts/telegram/send/alerts/check-current-and-deliver/alerts/digest/daily-and-deliverX-Forwarded-For/X-Real-IPonly when request comes from trusted proxy networks.magnitogorsk_current.jsonmagnitogorsk_forecast.jsonhealth_status.jsonIMPLEMENTATION_SUMMARY.mdCRITICAL_FIXES.mdMEDIUM_LOW_PRIORITY_FIXES.mdRollout notes (required env)
Set these env vars before enabling delivery endpoints in production:
ALERTS_API_KEY(single key)ALERTS_API_KEYS(comma-separated list of keys)RATE_LIMIT_TRUSTED_PROXIES(comma-separated IP/CIDR allowlist for proxy hops)Examples:
Behavior:
503(fail-closed).401.Validation
Executed targeted tests covering changed behavior:
All passed.
Closes #39 (partial).