feat(audit): optional best-effort webhook for clipboard and file events#256
feat(audit): optional best-effort webhook for clipboard and file events#256DL6ER wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds an opt-in audit webhook channel that emits best-effort JSON metadata for clipboard and file-upload events.
Changes:
- Introduces
audit.pywith an async, fire-and-forgetAuditClientand module-levelconfigure/emithelpers. - Emits audit events on clipboard send/receive and on file-upload completion/error paths.
- Adds new settings and configures the audit client at startup.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/selkies/settings.py | Adds settings for audit webhook URL/token/timeout. |
| src/selkies/selkies.py | Emits audit events for clipboard send and file-upload end/error. |
| src/selkies/input_handler.py | Emits audit events when clipboard content is received (single- and multi-part). |
| src/selkies/audit.py | New module implementing the audit webhook emitter (aiohttp, fire-and-forget). |
| src/selkies/main.py | Configures the audit client from settings at startup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def enabled(self) -> bool: | ||
| """True if a webhook URL is configured and the client will POST.""" | ||
| return bool(self.url) |
| def configure( | ||
| url: str = "", | ||
| token: str = "", | ||
| timeout_seconds: float = 2.0, | ||
| ) -> None: | ||
| """(Re)initialize the module-level :class:`AuditClient`. | ||
|
|
||
| Safe to call multiple times: previous instance is dropped without | ||
| explicit close. Call sites that hold a reference must use | ||
| :func:`emit` instead of caching the client. | ||
| """ | ||
| global _default | ||
| _default = AuditClient( | ||
| url=url, token=token, timeout_seconds=timeout_seconds | ||
| ) |
| # Configure the optional audit webhook (opt-in; empty URL is a no-op). | ||
| # str-type settings are stored as plain strings, so they are read | ||
| # directly (not via the [0] tuple accessor used for bool/range types). | ||
| try: | ||
| _audit_timeout = float(getattr(settings, "audit_webhook_timeout", "2.0") or "2.0") | ||
| except (TypeError, ValueError): | ||
| _audit_timeout = 2.0 | ||
| _audit.configure( | ||
| url=getattr(settings, "audit_webhook_url", "") or "", | ||
| token=getattr(settings, "audit_webhook_token", "") or "", | ||
| timeout_seconds=_audit_timeout, | ||
| ) |
| { | ||
| "name": "audit_webhook_timeout", | ||
| "type": "str", | ||
| "default": "2.0", | ||
| "help": "Per-request timeout in seconds (float) for audit webhook POSTs. Audit is fire-and-forget; on timeout the event is dropped with a warning log line. Default 2.0.", | ||
| }, |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a best-effort, non-blocking audit-event emitter to log metadata for clipboard and file-transfer events via an optional webhook. Key feedback focuses on securing the sensitive webhook token by marking it as sensitive in settings, maintaining strong references to background asyncio tasks to prevent premature garbage collection, safely reading response bodies to avoid memory exhaustion, and offloading the blocking os.path.getsize call to a separate thread to prevent event loop lag.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| "name": "audit_webhook_token", | ||
| "type": "str", | ||
| "default": "", | ||
| "help": "Optional Bearer token sent as Authorization header on audit webhook POSTs. Use this to authenticate Selkies against the audit collector. Empty omits the Authorization header.", |
There was a problem hiding this comment.
The audit_webhook_token is a sensitive credential used to authenticate against the audit collector. Currently, it is not marked as sensitive in its definition. As a result, it will be serialized and broadcast to all connected clients (including view-only or unauthorized clients) in the server_settings payload.
Adding "sensitive": True directly to the setting definition dictionary ensures it is excluded from the settings broadcast.
"name": "audit_webhook_token",
"type": "str",
"default": "",
"sensitive": True,
"help": "Optional Bearer token sent as Authorization header on audit webhook POSTs. Use this to authenticate Selkies against the audit collector. Empty omits the Authorization header.",| def __init__( | ||
| self, | ||
| url: str = "", | ||
| token: str = "", | ||
| timeout_seconds: float = 2.0, | ||
| ): | ||
| self.url = url.strip() | ||
| self.token = token.strip() | ||
| # Lower bound 100 ms: avoids accidental zero-timeout misconfiguration | ||
| # which aiohttp would interpret as "never time out". | ||
| self.timeout = aiohttp.ClientTimeout(total=max(0.1, float(timeout_seconds))) | ||
| self._session: Optional[aiohttp.ClientSession] = None |
There was a problem hiding this comment.
Initialize a set to keep strong references to active background tasks to prevent them from being garbage-collected mid-execution.
| def __init__( | |
| self, | |
| url: str = "", | |
| token: str = "", | |
| timeout_seconds: float = 2.0, | |
| ): | |
| self.url = url.strip() | |
| self.token = token.strip() | |
| # Lower bound 100 ms: avoids accidental zero-timeout misconfiguration | |
| # which aiohttp would interpret as "never time out". | |
| self.timeout = aiohttp.ClientTimeout(total=max(0.1, float(timeout_seconds))) | |
| self._session: Optional[aiohttp.ClientSession] = None | |
| def __init__( | |
| self, | |
| url: str = "", | |
| token: str = "", | |
| timeout_seconds: float = 2.0, | |
| ): | |
| self.url = url.strip() | |
| self.token = token.strip() | |
| # Lower bound 100 ms: avoids accidental zero-timeout misconfiguration | |
| # which aiohttp would interpret as "never time out". | |
| self.timeout = aiohttp.ClientTimeout(total=max(0.1, float(timeout_seconds))) | |
| self._session: Optional[aiohttp.ClientSession] = None | |
| self._background_tasks: set[asyncio.Task[None]] = set() |
| try: | ||
| loop = asyncio.get_running_loop() | ||
| except RuntimeError: | ||
| # No event loop yet - happens if a call site fires during | ||
| # synchronous import-time code. Drop the event quietly. | ||
| logger.debug("no running event loop, dropping audit event %s", event) | ||
| return | ||
| loop.create_task(self._post(payload)) |
There was a problem hiding this comment.
In Python's asyncio, tasks created via loop.create_task that are not strongly referenced elsewhere can be garbage-collected mid-execution. This can lead to silently dropped audit events.
To prevent this, maintain a set of active background tasks in the AuditClient instance, add the task to the set, and discard it via a done callback when finished.
| try: | |
| loop = asyncio.get_running_loop() | |
| except RuntimeError: | |
| # No event loop yet - happens if a call site fires during | |
| # synchronous import-time code. Drop the event quietly. | |
| logger.debug("no running event loop, dropping audit event %s", event) | |
| return | |
| loop.create_task(self._post(payload)) | |
| try: | |
| loop = asyncio.get_running_loop() | |
| except RuntimeError: | |
| # No event loop yet - happens if a call site fires during | |
| # synchronous import-time code. Drop the event quietly. | |
| logger.debug("no running event loop, dropping audit event %s", event) | |
| return | |
| task = loop.create_task(self._post(payload)) | |
| self._background_tasks.add(task) | |
| task.add_done_callback(self._background_tasks.discard) |
| async def close(self) -> None: | ||
| """Close the underlying HTTP session if one was opened.""" | ||
| if self._session is not None and not self._session.closed: | ||
| await self._session.close() | ||
| self._session = None |
There was a problem hiding this comment.
Ensure that any pending background audit tasks are fully completed before closing the underlying aiohttp.ClientSession during shutdown.
| async def close(self) -> None: | |
| """Close the underlying HTTP session if one was opened.""" | |
| if self._session is not None and not self._session.closed: | |
| await self._session.close() | |
| self._session = None | |
| async def close(self) -> None: | |
| """Close the underlying HTTP session if one was opened.""" | |
| if self._background_tasks: | |
| await asyncio.gather(*self._background_tasks, return_exceptions=True) | |
| if self._session is not None and not self._session.closed: | |
| await self._session.close() | |
| self._session = None |
| session = await self._ensure_session() | ||
| async with session.post(self.url, json=payload) as resp: | ||
| if resp.status >= 400: | ||
| body = (await resp.text())[:200] | ||
| logger.warning( | ||
| "audit webhook returned %s for event=%s: %s", | ||
| resp.status, | ||
| payload.get("event"), | ||
| body, | ||
| ) |
There was a problem hiding this comment.
await resp.text() reads the entire response body into memory. If the webhook target returns a very large error page or payload, this can consume excessive memory.
Reading only the first 200 bytes using await resp.content.read(200) and decoding it is much safer and more memory-efficient.
| session = await self._ensure_session() | |
| async with session.post(self.url, json=payload) as resp: | |
| if resp.status >= 400: | |
| body = (await resp.text())[:200] | |
| logger.warning( | |
| "audit webhook returned %s for event=%s: %s", | |
| resp.status, | |
| payload.get("event"), | |
| body, | |
| ) | |
| session = await self._ensure_session() | |
| async with session.post(self.url, json=payload) as resp: | |
| if resp.status >= 400: | |
| body_bytes = await resp.content.read(200) | |
| body = body_bytes.decode("utf-8", errors="replace") | |
| logger.warning( | |
| "audit webhook returned %s for event=%s: %s", | |
| resp.status, | |
| payload.get("event"), | |
| body, | |
| ) |
| try: | ||
| _au_size = os.path.getsize(active_upload_target_path_conn) | ||
| except OSError: | ||
| _au_size = -1 |
There was a problem hiding this comment.
os.path.getsize is a blocking synchronous I/O operation. Calling it directly on the main event loop can block the loop, causing frame drops, audio stutter, or latency spikes in the streaming session.
Since Python 3.9+ is supported and asyncio.to_thread is already used elsewhere in the codebase, we should run this blocking call in a separate thread using await asyncio.to_thread(...). We catch broad Exception here because a broad exception handler is already present or preferred to ensure all exceptions are caught without redundant handlers.
| try: | |
| _au_size = os.path.getsize(active_upload_target_path_conn) | |
| except OSError: | |
| _au_size = -1 | |
| try: | |
| _au_size = await asyncio.to_thread(os.path.getsize, active_upload_target_path_conn) | |
| except Exception: | |
| _au_size = -1 |
References
- When reviewing exception handling, verify if a broad except Exception block is present at the end of the try block before suggesting specific exception handlers (like except OSError), as except Exception already catches them.
Add an opt-in audit channel that POSTs small JSON metadata objects to a configurable webhook on clipboard send/receive and file upload events. Only metadata is sent (event type, byte size, mime type, timestamp); payload contents are never logged. The emitter is non-blocking and best-effort, so a slow or unreachable collector cannot stall the streaming pipeline. Disabled by default - an empty URL short-circuits emit() to a no-op. New settings: audit_webhook_url, audit_webhook_token, audit_webhook_timeout. Signed-off-by: DL6ER <dl6er@dl6er.de>
|
Thanks for the thorough review. Addressed:
Left Smoke-tested on the EL/XFCE (py3.9) image: default no-op, |
|
Heads-up for reviewers: the failing |
|
I think the decision is yet to be taken on this. |
File download is served by the nginx fancyindex module, outside the Selkies Python package, so it cannot be observed from here. The emitter already only emits clipboard and file-upload events; drop the docstring claim that it also covers downloads. Signed-off-by: DL6ER <dl6er@dl6er.de>
|
Agreed, thanks for the pointer. File download is the nginx Happy to hold for the decision on whether to take this at all. |
|
Note that #254 removes the explicit NGINX dependency (it can still be sped up by NGINX). |
Reference the issue numbers and reviewers
Closes #249.
Explain relevant issues and how this pull request solves them
#249 asks for a way to observe the data-transfer channels Selkies handles - an audit trail for regulated environments, without forking Selkies. Inside the Selkies Python package that means the bidirectional clipboard and file upload; file download is served by the nginx
libnginx-mod-http-fancyindexmodule and is out of scope here.This pull request implements the webhook design proposed in #249: a single optional
audit_webhook_urlsetting. When unset (the default) Selkies behaves exactly as before. When set, Selkies POSTs a small JSON object to that URL on each clipboard / file-upload event, carrying metadata only - never the payload contents.Describe the changes in code and its dependencies and justify that they work as intended after testing
selkies/audit.py: a fire-and-forget JSON-over-HTTP sink. Owns one lazily-createdaiohttp.ClientSession, exposes module-levelconfigure()/emit()/is_enabled()/close().emit()schedules the POST on the running loop and returns immediately; all failures (DNS, refused, timeout, HTTP >= 400, anythingaiohttpraises) are caught and logged, so a slow or unreachable collector can never stall the streaming pipeline. When no URL is configured,emit()is a no-op.settings.py: three newstrsettings -audit_webhook_url,audit_webhook_token,audit_webhook_timeout(defaults keep the channel disabled).__main__.py:audit.configure(...)is called once at startup from the parsed settings.emit()call sites:clipboard.send(insend_ws_clipboard_data),clipboard.receive(text / binary / multipart paths in the input handler),file.upload.endandfile.upload.error. Each carries event id, RFC 3339 timestamp, byte size, mime type and/or filename as applicable.aiohttp, already a Selkies dependency.Testing: the module ships in our EL/XFCE (Python 3.9) build and is imported and configured on every session start; the default (no URL) no-op path is exercised by the end-to-end suite, which is green, confirming the additions do not affect existing behavior when the channel is disabled. The enabled path was exercised against a local collector:
configure()with a URL + Bearer token, thenemit()for clipboard and file events, produces one POST per event with the expected JSON metadata andAuthorization: Bearerheader; a down/refusing collector only logs a warning and does not interrupt the session.Describe alternatives you've considered
Additional context
The Bearer token is sent as
Authorization: Bearer ...specifically so the POST can be terminated at any standard reverse proxy with mature auth middleware, as outlined in #249.