fix(webhooks): escape HTML in error responses, compare bearer as bytes
Address the two Security findings from PR review: - webhook_receiver: encode Authorization header and expected bearer to utf-8 bytes before hmac.compare_digest. Conventional form; doesn't rely on Python's implicit ASCII encoding. - webhook_routes: html.escape user-influenced and exception-derived strings before interpolating into HTMLResponse content. Covers the preset_id path param echoed in the "Unknown preset" branch and the str(e) text rendered on handler exceptions. Adds regression tests verifying compare_digest is invoked on bytes and that <script> payloads (in preset_id and exception messages) are emitted as escaped entities, not active markup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
c1368b9a7f
commit
4a3857aabb
@@ -4,6 +4,7 @@ Provides browser-based endpoints for admin users to manage webhook configuration
|
||||
using preset templates. Only accessible to Nextcloud administrators.
|
||||
"""
|
||||
|
||||
import html
|
||||
import logging
|
||||
import os
|
||||
|
||||
@@ -411,7 +412,7 @@ async def webhook_management_pane(request: Request) -> HTMLResponse:
|
||||
content=f"""
|
||||
<div class="warning">
|
||||
<p><strong>Error Loading Webhooks</strong></p>
|
||||
<p>{str(e)}</p>
|
||||
<p>{html.escape(str(e))}</p>
|
||||
</div>
|
||||
""",
|
||||
status_code=500,
|
||||
@@ -447,7 +448,7 @@ async def enable_webhook_preset(request: Request) -> HTMLResponse:
|
||||
preset = get_preset(preset_id)
|
||||
if not preset:
|
||||
return HTMLResponse(
|
||||
content=f'<div class="warning">Unknown preset: {preset_id}</div>',
|
||||
content=f'<div class="warning">Unknown preset: {html.escape(preset_id)}</div>',
|
||||
status_code=404,
|
||||
)
|
||||
|
||||
@@ -500,7 +501,7 @@ async def enable_webhook_preset(request: Request) -> HTMLResponse:
|
||||
except Exception as e:
|
||||
logger.error("Failed to enable preset %s: %s", preset_id, e, exc_info=True)
|
||||
return HTMLResponse(
|
||||
content=f'<div class="warning">Failed to enable preset: {str(e)}</div>',
|
||||
content=f'<div class="warning">Failed to enable preset: {html.escape(str(e))}</div>',
|
||||
status_code=500,
|
||||
)
|
||||
|
||||
@@ -534,7 +535,7 @@ async def disable_webhook_preset(request: Request) -> HTMLResponse:
|
||||
preset = get_preset(preset_id)
|
||||
if not preset:
|
||||
return HTMLResponse(
|
||||
content=f'<div class="warning">Unknown preset: {preset_id}</div>',
|
||||
content=f'<div class="warning">Unknown preset: {html.escape(preset_id)}</div>',
|
||||
status_code=404,
|
||||
)
|
||||
|
||||
@@ -592,6 +593,6 @@ async def disable_webhook_preset(request: Request) -> HTMLResponse:
|
||||
except Exception as e:
|
||||
logger.error("Failed to disable preset %s: %s", preset_id, e, exc_info=True)
|
||||
return HTMLResponse(
|
||||
content=f'<div class="warning">Failed to disable preset: {str(e)}</div>',
|
||||
content=f'<div class="warning">Failed to disable preset: {html.escape(str(e))}</div>',
|
||||
status_code=500,
|
||||
)
|
||||
|
||||
@@ -52,12 +52,13 @@ async def handle_nextcloud_webhook(request: Request) -> JSONResponse:
|
||||
"""
|
||||
secret = get_settings().webhook_secret
|
||||
if secret:
|
||||
provided = request.headers.get("authorization", "")
|
||||
expected = f"Bearer {secret}"
|
||||
provided = request.headers.get("authorization", "").encode("utf-8")
|
||||
expected = f"Bearer {secret}".encode("utf-8")
|
||||
# Use compare_digest to avoid the character-by-character short-circuit
|
||||
# of `==`. compare_digest still returns False for differing lengths
|
||||
# but isn't fully constant-time across them; that's fine here — a
|
||||
# secret length leak is not a sensitive signal.
|
||||
# of `==`. Comparing as bytes is the conventional form and avoids any
|
||||
# surprise with non-ASCII input. compare_digest still returns False
|
||||
# for differing lengths but isn't fully constant-time across them;
|
||||
# that's fine here — a secret length leak is not a sensitive signal.
|
||||
if not hmac.compare_digest(provided, expected):
|
||||
logger.warning("Webhook rejected: missing or invalid Authorization header")
|
||||
return JSONResponse(
|
||||
|
||||
Reference in New Issue
Block a user