fix(webhooks): escape webhook_uri, lazy logging, document 401 header omission
Address round-5 reviewer feedback on PR #747: - Escape `webhook_uri` in the admin pane HTML template so an operator- controlled env value (`WEBHOOK_INTERNAL_URL`, `NEXTCLOUD_MCP_SERVER_URL`) can't inject markup. The sibling `preset_id` and exception messages were already escaped — this one was the odd one out. - Convert the eight remaining f-string `logger.warning`/`logger.error` calls in `api/webhooks.py` to lazy `%s` formatting, matching the style already adopted by `webhook_receiver.py` and `webhook_routes.py`. - Document why the 401 from `handle_nextcloud_webhook` deliberately omits `WWW-Authenticate`: NC's webhook delivery worker has no auth-flow state machine to negotiate against, the bearer is a static shared secret configured out-of-band via `WEBHOOK_SECRET`, and a challenge response wouldn't change client behaviour. The existing warning log already records the rejection. 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
4a3857aabb
commit
affe29f72e
@@ -37,7 +37,7 @@ async def get_installed_apps(request: Request) -> JSONResponse:
|
||||
# Validate OAuth token and extract user
|
||||
user_id, validated = await validate_token_and_get_user(request)
|
||||
except Exception as e:
|
||||
logger.warning(f"Unauthorized access to /api/v1/apps: {e}")
|
||||
logger.warning("Unauthorized access to /api/v1/apps: %s", e)
|
||||
return JSONResponse(
|
||||
{
|
||||
"error": "Unauthorized",
|
||||
@@ -86,7 +86,7 @@ async def get_installed_apps(request: Request) -> JSONResponse:
|
||||
return JSONResponse({"apps": apps})
|
||||
|
||||
except Exception as e:
|
||||
logger.error(f"Error getting installed apps for user {user_id}: {e}")
|
||||
logger.error("Error getting installed apps for user %s: %s", user_id, e)
|
||||
return JSONResponse(
|
||||
{
|
||||
"error": "Internal error",
|
||||
@@ -107,7 +107,7 @@ async def list_webhooks(request: Request) -> JSONResponse:
|
||||
# Validate OAuth token and extract user
|
||||
user_id, validated = await validate_token_and_get_user(request)
|
||||
except Exception as e:
|
||||
logger.warning(f"Unauthorized access to /api/v1/webhooks: {e}")
|
||||
logger.warning("Unauthorized access to /api/v1/webhooks: %s", e)
|
||||
return JSONResponse(
|
||||
{
|
||||
"error": "Unauthorized",
|
||||
@@ -142,7 +142,7 @@ async def list_webhooks(request: Request) -> JSONResponse:
|
||||
return JSONResponse({"webhooks": webhooks})
|
||||
|
||||
except Exception as e:
|
||||
logger.error(f"Error listing webhooks for user {user_id}: {e}")
|
||||
logger.error("Error listing webhooks for user %s: %s", user_id, e)
|
||||
return JSONResponse(
|
||||
{
|
||||
"error": "Internal error",
|
||||
@@ -170,7 +170,7 @@ async def create_webhook(request: Request) -> JSONResponse:
|
||||
# Validate OAuth token and extract user
|
||||
user_id, validated = await validate_token_and_get_user(request)
|
||||
except Exception as e:
|
||||
logger.warning(f"Unauthorized access to /api/v1/webhooks: {e}")
|
||||
logger.warning("Unauthorized access to /api/v1/webhooks: %s", e)
|
||||
return JSONResponse(
|
||||
{
|
||||
"error": "Unauthorized",
|
||||
@@ -229,7 +229,7 @@ async def create_webhook(request: Request) -> JSONResponse:
|
||||
return JSONResponse({"webhook": webhook_data})
|
||||
|
||||
except Exception as e:
|
||||
logger.error(f"Error creating webhook for user {user_id}: {e}")
|
||||
logger.error("Error creating webhook for user %s: %s", user_id, e)
|
||||
return JSONResponse(
|
||||
{
|
||||
"error": "Internal error",
|
||||
@@ -250,7 +250,7 @@ async def delete_webhook(request: Request) -> JSONResponse:
|
||||
# Validate OAuth token and extract user
|
||||
user_id, validated = await validate_token_and_get_user(request)
|
||||
except Exception as e:
|
||||
logger.warning(f"Unauthorized access to /api/v1/webhooks: {e}")
|
||||
logger.warning("Unauthorized access to /api/v1/webhooks: %s", e)
|
||||
return JSONResponse(
|
||||
{
|
||||
"error": "Unauthorized",
|
||||
@@ -301,7 +301,7 @@ async def delete_webhook(request: Request) -> JSONResponse:
|
||||
return JSONResponse({"success": True, "message": "Webhook deleted"})
|
||||
|
||||
except Exception as e:
|
||||
logger.error(f"Error deleting webhook for user {user_id}: {e}")
|
||||
logger.error("Error deleting webhook for user %s: %s", user_id, e)
|
||||
return JSONResponse(
|
||||
{
|
||||
"error": "Internal error",
|
||||
|
||||
@@ -394,7 +394,7 @@ async def webhook_management_pane(request: Request) -> HTMLResponse:
|
||||
<div class="info-message">
|
||||
<p><strong>About Webhooks</strong></p>
|
||||
<p>Webhooks enable real-time synchronization by notifying this server when content changes in Nextcloud.</p>
|
||||
<p><strong>Endpoint:</strong> <code>{webhook_uri}</code></p>
|
||||
<p><strong>Endpoint:</strong> <code>{html.escape(webhook_uri)}</code></p>
|
||||
</div>
|
||||
|
||||
<h3 style="margin-top: 30px;">Available Presets</h3>
|
||||
|
||||
@@ -60,6 +60,13 @@ async def handle_nextcloud_webhook(request: Request) -> JSONResponse:
|
||||
# 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):
|
||||
# Intentionally omit WWW-Authenticate. RFC 7235 §4.1 says a 401
|
||||
# SHOULD carry it, but Nextcloud's webhook delivery worker has no
|
||||
# auth-flow state machine to negotiate against — the bearer is a
|
||||
# static shared secret configured out-of-band via WEBHOOK_SECRET,
|
||||
# and a challenge response wouldn't change client behaviour.
|
||||
# Surfacing it would only mislead operators into expecting a
|
||||
# renegotiation that doesn't exist.
|
||||
logger.warning("Webhook rejected: missing or invalid Authorization header")
|
||||
return JSONResponse(
|
||||
{"status": "unauthorized"},
|
||||
|
||||
Reference in New Issue
Block a user