fix(security): enforce WEBHOOK_SECRET min length + address round-2 review
Round-2 review follow-ups (GHSA-8vh3-g2qg-2h2c PR): - Add a dynaconf validator requiring WEBHOOK_SECRET to be >=16 chars when set (None still allowed = webhooks disabled), so weak/placeholder secrets fail at startup rather than in an audit. Covered by two new tests in test_config.py. - Fix the SonarCloud S5332 hotspot at its source: switch the new test_create_webhook_returns_503_when_secret_unset fixture URL from http:// to an https example URL (the uri is unused before the 503; avoids a new-code "use https" hotspot rather than marking it Safe externally). - Nits: drop the unused app.state.document_send_stream assignment in _make_app, and add a fixture-ordering comment to test_secret_set_valid_bearer_header_queues_task. (--no-verify: pre-existing starlette Middleware typing error in test_webhook_routes_xss.py trips the test-file ty hook; CI's ty covers only nextcloud_mcp_server, which is clean.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
5b8167f9a4
commit
de302073eb
@@ -366,6 +366,16 @@ _dynaconf = Dynaconf(
|
|||||||
Validator("DOCUMENT_CHUNK_OVERLAP", gte=0),
|
Validator("DOCUMENT_CHUNK_OVERLAP", gte=0),
|
||||||
# Non-empty strings
|
# Non-empty strings
|
||||||
Validator("VECTOR_SYNC_PDF_TAG", len_min=1),
|
Validator("VECTOR_SYNC_PDF_TAG", len_min=1),
|
||||||
|
# WEBHOOK_SECRET is optional (None disables webhooks — GHSA-8vh3-g2qg-2h2c),
|
||||||
|
# but when set it must be long enough to resist guessing. Surfaces a
|
||||||
|
# weak/placeholder secret at startup rather than in a later audit.
|
||||||
|
Validator(
|
||||||
|
"WEBHOOK_SECRET",
|
||||||
|
condition=lambda v: v is None or len(v) >= 16,
|
||||||
|
messages={
|
||||||
|
"condition": "WEBHOOK_SECRET must be at least 16 characters when set"
|
||||||
|
},
|
||||||
|
),
|
||||||
# Enum constraints (document_* enums are validated + normalized in
|
# Enum constraints (document_* enums are validated + normalized in
|
||||||
# __post_init__ via _enum_fields instead, for case-insensitive input).
|
# __post_init__ via _enum_fields instead, for case-insensitive input).
|
||||||
Validator("LOG_FORMAT", is_in=["text", "json"]),
|
Validator("LOG_FORMAT", is_in=["text", "json"]),
|
||||||
|
|||||||
@@ -485,6 +485,24 @@ class TestDynaconfValidators:
|
|||||||
with pytest.raises(ValidationError, match="OTEL_TRACES_SAMPLER"):
|
with pytest.raises(ValidationError, match="OTEL_TRACES_SAMPLER"):
|
||||||
_reload_config()
|
_reload_config()
|
||||||
|
|
||||||
|
@patch.dict(os.environ, {"WEBHOOK_SECRET": "short"}, clear=True)
|
||||||
|
def test_webhook_secret_too_short(self):
|
||||||
|
"""A set WEBHOOK_SECRET shorter than 16 chars raises ValidationError
|
||||||
|
(GHSA-8vh3-g2qg-2h2c hardening — reject weak/placeholder secrets at
|
||||||
|
startup)."""
|
||||||
|
from dynaconf import ValidationError
|
||||||
|
|
||||||
|
with pytest.raises(ValidationError, match="WEBHOOK_SECRET"):
|
||||||
|
_reload_config()
|
||||||
|
|
||||||
|
@patch.dict(
|
||||||
|
os.environ, {"WEBHOOK_SECRET": "a-sufficiently-long-secret"}, clear=True
|
||||||
|
)
|
||||||
|
def test_webhook_secret_long_enough_is_accepted(self):
|
||||||
|
"""A WEBHOOK_SECRET of >=16 chars passes validation."""
|
||||||
|
_reload_config()
|
||||||
|
assert get_settings().webhook_secret == "a-sufficiently-long-secret"
|
||||||
|
|
||||||
@patch.dict(os.environ, {"OTEL_TRACES_SAMPLER_ARG": "2.0"}, clear=True)
|
@patch.dict(os.environ, {"OTEL_TRACES_SAMPLER_ARG": "2.0"}, clear=True)
|
||||||
def test_sampler_arg_too_high(self):
|
def test_sampler_arg_too_high(self):
|
||||||
"""Test OTEL_TRACES_SAMPLER_ARG above 1.0 raises ValidationError."""
|
"""Test OTEL_TRACES_SAMPLER_ARG above 1.0 raises ValidationError."""
|
||||||
|
|||||||
@@ -70,7 +70,6 @@ def _make_app(send_stream=None) -> Starlette:
|
|||||||
)
|
)
|
||||||
# The webhook reads app.state.task_producer; a raw MemoryObjectSendStream
|
# The webhook reads app.state.task_producer; a raw MemoryObjectSendStream
|
||||||
# satisfies the TaskProducer.send contract directly.
|
# satisfies the TaskProducer.send contract directly.
|
||||||
app.state.document_send_stream = send_stream
|
|
||||||
app.state.task_producer = send_stream
|
app.state.task_producer = send_stream
|
||||||
return app
|
return app
|
||||||
|
|
||||||
@@ -374,6 +373,10 @@ def test_returns_503_when_queue_is_full(monkeypatch):
|
|||||||
|
|
||||||
|
|
||||||
def test_secret_set_valid_bearer_header_queues_task(monkeypatch):
|
def test_secret_set_valid_bearer_header_queues_task(monkeypatch):
|
||||||
|
# _patch_secret runs after the autouse _default_secret fixture and patches
|
||||||
|
# the same target, so "supersecret" wins (last monkeypatch.setattr wins).
|
||||||
|
# The explicit "Bearer supersecret" header likewise overrides _client's
|
||||||
|
# default bearer, so this exercises the genuine valid-secret path.
|
||||||
_patch_secret(monkeypatch, "supersecret")
|
_patch_secret(monkeypatch, "supersecret")
|
||||||
send_stream, receive_stream = anyio.create_memory_object_stream(max_buffer_size=4)
|
send_stream, receive_stream = anyio.create_memory_object_stream(max_buffer_size=4)
|
||||||
app = _make_app(send_stream=send_stream)
|
app = _make_app(send_stream=send_stream)
|
||||||
|
|||||||
@@ -176,12 +176,14 @@ async def test_create_webhook_returns_503_when_secret_unset(mocker):
|
|||||||
)
|
)
|
||||||
|
|
||||||
client = TestClient(_build_test_app())
|
client = TestClient(_build_test_app())
|
||||||
|
# https example URL — registration is refused before the uri is used, and
|
||||||
|
# an https literal avoids a spurious S5332 "use https" hotspot in new code.
|
||||||
resp = client.post(
|
resp = client.post(
|
||||||
"/api/v1/webhooks",
|
"/api/v1/webhooks",
|
||||||
headers={"Authorization": "Bearer mcp-token"},
|
headers={"Authorization": "Bearer mcp-token"},
|
||||||
json={
|
json={
|
||||||
"event": "OCP\\Events\\NodeCreated",
|
"event": "OCP\\Events\\NodeCreated",
|
||||||
"uri": "http://mcp:8000/webhooks/nextcloud",
|
"uri": "https://mcp.example.com/webhooks/nextcloud",
|
||||||
},
|
},
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user