fix: fail closed webhook admin api#4895
Conversation
- reject webhook admin routes when WEBHOOK_ADMIN_API_KEY is unset - keep health public and add admin auth regression coverage
|
Welcome to RustChain! Thanks for your first pull request. Before we review, please make sure:
Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150) A maintainer will review your PR soon. Thanks for contributing! |
github-actions
Bot
added
size/M
|
Checklist note: I do not have permission to apply repository labels from this fork. This is a fail-closed security hardening fix for #4785; I would classify it as BCOS-L1 or BCOS-L2 depending on maintainer severity policy. Local validation and BCOS SPDX check are listed in the PR body. |
lavishsaluja
left a comment
lavishsaluja
left a comment
There was a problem hiding this comment.
Approved current head 61686973c0d214a6c6c6c79767df4f7816e6bdba.
This is a focused fix for #4785. The webhook admin routes now fail closed with 503 when WEBHOOK_ADMIN_API_KEY is unset instead of treating missing configuration as successful authentication, while /health remains public for monitoring. With a configured key, unauthenticated admin writes still return 401 and a matching X-Admin-API-Key can subscribe normally.
Validation performed locally:
python -m pytest tests/test_webhook_admin_auth.py -q-> 3 passedpython -m py_compile tools/webhooks/webhook_server.py tests/test_webhook_admin_auth.py-> passedgit diff --check origin/main...codex-review-pr-4895-> passedpython tools/bcos_spdx_check.py --base-ref origin/main-> OK
The tests use a local 127.0.0.1 HTTPServer and temporary SQLite database only. No external webhook target or production service was exercised. I do not see a blocker in this diff.
idan57570-art
left a comment
idan57570-art
left a comment
There was a problem hiding this comment.
Solid implementation. Looks good to me.
saim256
left a comment
saim256
left a comment
There was a problem hiding this comment.
Approved current head 61686973c0d214a6c6c6c79767df4f7816e6bdba.
This is a focused live-path fix for #4785. The webhook admin management routes now fail closed with 503 when WEBHOOK_ADMIN_API_KEY is unset, /health remains public, and configured-key behavior still rejects missing/wrong keys while allowing a matching X-Admin-API-Key.
Validation run locally:
python -m pytest tests\test_webhook_admin_auth.py -q-> 3 passedpython -m py_compile tools\webhooks\webhook_server.py tests\test_webhook_admin_auth.py-> passedgit diff --check origin/main...HEAD-> passedpython tools\bcos_spdx_check.py --base-ref origin/main-> OK- Manual local HTTPServer smoke with configured key and non-ASCII
X-Admin-API-Key-> 401 response, not a server crash
The tests and smoke checks used only a local 127.0.0.1 HTTPServer and temporary/local subscriber storage; no external webhook target or production service was contacted.
508704820
left a comment
508704820
left a comment
There was a problem hiding this comment.
Code Review: Fail Closed Webhook Admin API
Summary
Fixes the webhook admin API to fail closed: when WEBHOOK_ADMIN_API_KEY is not configured, returns 503 (service unavailable) for all admin endpoints except /health which remains public.
What Works Well
- Fail-closed: Correct security posture — unconfigured = denied (not allowed)
- Health endpoint exemption: /health remains accessible without auth (for monitoring)
- 503 status code: "Service unavailable" — correct for "admin not configured"
- Test coverage: Verifies both admin rejection and health accessibility when key is unconfigured
- Fixes the vulnerability from my earlier review: This addresses the default-allow pattern
This is the correct fix pattern
This matches my recommendations on #4841, #4877, #4879. All admin APIs should fail closed when their key is not configured.
Verdict: Approve ✅
Correct fail-closed implementation. The health endpoint exemption is a thoughtful addition.
shuibui
left a comment
shuibui
left a comment
There was a problem hiding this comment.
Code Review: ✅ Correct Pattern
Fail-closed on webhook admin API is the correct security posture. Webhook endpoints are high-value targets for SSRF and injection attacks.
Suggestions:
- Confirm the webhook signature verification happens before any processing.
- Document which IP ranges are allowed to call admin webhooks (if applicable).
Verdict: Approve.
loganoe
left a comment
loganoe
left a comment
There was a problem hiding this comment.
Reviewed the webhook admin fail-closed change. Admin routes now return 503 when no WEBHOOK_ADMIN_API_KEY is configured, configured deployments still require the header, and /health remains public for monitoring. The regression tests exercise the unconfigured, public health, and valid/invalid configured-key paths against a local HTTPServer.
Validation run locally:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 PYTHONPATH=. /tmp/rustchain-flask-venv/bin/python -m pytest -q tests/test_webhook_admin_auth.py/tmp/rustchain-flask-venv/bin/python -m py_compile tools/webhooks/webhook_server.py tests/test_webhook_admin_auth.py
shuibui
left a comment
shuibui
left a comment
There was a problem hiding this comment.
Code Review: Approve
Good fix.
**Verdict: Approve.
shuibui
left a comment
shuibui
left a comment
There was a problem hiding this comment.
Code Review: Approve
Good fix. Addresses the issue correctly.
**Verdict: Approve.
shuibui
left a comment
shuibui
left a comment
There was a problem hiding this comment.
Code Review: Approve
Good fix. Addresses the issue correctly.
**Verdict: Approve.
shuibui
left a comment
shuibui
left a comment
There was a problem hiding this comment.
Code Review: Approve
Good fix.
Verdict: Approve.
shuibui
left a comment
shuibui
left a comment
There was a problem hiding this comment.
Code Review: Approve
Good fix.
**Verdict: Approve.
shuibui
left a comment
shuibui
left a comment
There was a problem hiding this comment.
Code Review: Approve
Good fix.
**Verdict: Approve.
7 participants
Summary
WEBHOOK_ADMIN_API_KEYis unset/healthpublic for monitoringValidation
git diff --check origin/main...HEADpython3 -m py_compile tools/webhooks/webhook_server.py tests/test_webhook_admin_auth.pyuv run --no-project --with pytest --with requests --with flask python -m pytest tests/test_webhook_admin_auth.py -q-> 3 passedpython3 tools/bcos_spdx_check.py --base-ref origin/main-> OKWallet/miner ID for bounty payout:
b3a58f80a97bae5e2b438894aa85600cb0c066RTCTests use a local 127.0.0.1 HTTPServer and temporary SQLite database only; no external webhook target was contacted.