From affe29f72e9ecdce888b59a41ac2384109d08afa Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 30 Apr 2026 14:37:34 +0200 Subject: [PATCH] fix(webhooks): escape webhook_uri, lazy logging, document 401 header omission MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- nextcloud_mcp_server/api/webhooks.py | 16 ++++++++-------- nextcloud_mcp_server/auth/webhook_routes.py | 2 +- nextcloud_mcp_server/vector/webhook_receiver.py | 7 +++++++ 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/nextcloud_mcp_server/api/webhooks.py b/nextcloud_mcp_server/api/webhooks.py index e042164b..3f9a3a28 100644 --- a/nextcloud_mcp_server/api/webhooks.py +++ b/nextcloud_mcp_server/api/webhooks.py @@ -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", diff --git a/nextcloud_mcp_server/auth/webhook_routes.py b/nextcloud_mcp_server/auth/webhook_routes.py index 360a101d..6e04031a 100644 --- a/nextcloud_mcp_server/auth/webhook_routes.py +++ b/nextcloud_mcp_server/auth/webhook_routes.py @@ -394,7 +394,7 @@ async def webhook_management_pane(request: Request) -> HTMLResponse:

About Webhooks

Webhooks enable real-time synchronization by notifying this server when content changes in Nextcloud.

-

Endpoint: {webhook_uri}

+

Endpoint: {html.escape(webhook_uri)}

Available Presets

diff --git a/nextcloud_mcp_server/vector/webhook_receiver.py b/nextcloud_mcp_server/vector/webhook_receiver.py index 1c25c19b..d55d666c 100644 --- a/nextcloud_mcp_server/vector/webhook_receiver.py +++ b/nextcloud_mcp_server/vector/webhook_receiver.py @@ -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"},