From 1e0e375d603bca05b06a3da7ef2744515c83aa10 Mon Sep 17 00:00:00 2001 From: Slawomir Koszewski Date: Fri, 30 Oct 2026 02:03:58 +0100 Subject: [PATCH] fix: an answer whose client went away ends quietly, and a shell whose client left before it opened is stopped. --- ADR.md | 25 +++++++++++++++ debian-package/debian/changelog | 12 +++++++ .../src/usr/lib/vpn-router/bootstrap | 18 +++++++---- debian-package/src/usr/lib/vpn-router/metrics | 22 +++++++------ .../lib/vpn-router/vpnrouter_web/handler.py | 22 +++++++------ .../usr/lib/vpn-router/vpnrouter_web/shell.py | 32 +++++++++++++------ .../usr/lib/vpn-router/vpnrouter_web/sites.py | 22 +++++++------ 7 files changed, 109 insertions(+), 44 deletions(-) diff --git a/ADR.md b/ADR.md index 0922686..f1e8eeb 100644 --- a/ADR.md +++ b/ADR.md @@ -1342,3 +1342,28 @@ method. Consequences: A change to one family of paths touches one module and the routing in `handler`. State shared between requests, such as the open shells, lives in the module that owns it; other modules change it and never rebind it. + +## ADR-48: Exceptions are handled; no traceback reaches the logs + +Status: Accepted + +Context: The services log to the journal, which administrators read through the Logs page and +`journalctl`. Python prints a traceback for an exception nothing handles: `socketserver` for a +failed request, `threading` for a failed thread and the interpreter for the process. A client +that closes its connection before the answer is written, as a page does when it is left or +reloaded, raises `BrokenPipeError` or `ConnectionResetError` in the handler. A traceback is many +lines of internals for one event and hides the lines around it. + +Decision: The code handles the exceptions it can expect where they occur, and answers or logs +them with their reason in one line. An exception left unhandled is a defect to be fixed. As the +last line of defence, every server replaces the default reports: + +- `vpnrouter_serve.serve`, which the web service and the metrics service run, sets the servers' + `handle_error`, `threading.excepthook` and `sys.excepthook`. +- The bootstrap server sets its server's `handle_error` and `sys.excepthook`. + +A `ConnectionError` of a client that went away is not logged; any other exception is logged in +one line with its type and message. + +Consequences: The logs hold one line per failure, without its stack. A failure that reaches the +last line of defence is found by its message and fixed where it occurs, with its own handling. diff --git a/debian-package/debian/changelog b/debian-package/debian/changelog index 35741b8..3a2526c 100644 --- a/debian-package/debian/changelog +++ b/debian-package/debian/changelog @@ -1,3 +1,15 @@ +vpn-router (0.37.3-1) unstable; urgency=medium + + fix: an answer whose client went away ends quietly, and a shell whose client left before it opened is stopped. + + * The web service, the metrics service and the bootstrap server stop + writing an answer when its client closes the connection, and a site + file whose download is left no longer gets a second answer. + * A shell whose client closed the connection before it opened is + stopped and its end is logged, instead of running on. + + -- Slawomir Koszewski Fri, 30 Oct 2026 02:03:26 +0100 + vpn-router (0.37.2-1) unstable; urgency=medium fix: the web service, the metrics service and the bootstrap server log a failure in one line instead of a traceback, and nothing when a client goes away. diff --git a/debian-package/src/usr/lib/vpn-router/bootstrap b/debian-package/src/usr/lib/vpn-router/bootstrap index 107bae9..caf3216 100755 --- a/debian-package/src/usr/lib/vpn-router/bootstrap +++ b/debian-package/src/usr/lib/vpn-router/bootstrap @@ -43,13 +43,17 @@ def report(): class Handler(BaseHTTPRequestHandler): def do_GET(self): body = report().encode() - self.send_response(200) - self.send_header('Content-Type', 'text/plain; charset=utf-8') - self.send_header('Content-Length', str(len(body))) - self.send_header('Cache-Control', 'no-store') - self.send_header('Refresh', '10') - self.end_headers() - self.wfile.write(body) + try: + self.send_response(200) + self.send_header('Content-Type', 'text/plain; charset=utf-8') + self.send_header('Content-Length', str(len(body))) + self.send_header('Cache-Control', 'no-store') + self.send_header('Refresh', '10') + self.end_headers() + self.wfile.write(body) + except ConnectionError: + # The client went away before the answer was written. + self.close_connection = True def failed(request, client_address): diff --git a/debian-package/src/usr/lib/vpn-router/metrics b/debian-package/src/usr/lib/vpn-router/metrics index ac54e69..4aad5ef 100755 --- a/debian-package/src/usr/lib/vpn-router/metrics +++ b/debian-package/src/usr/lib/vpn-router/metrics @@ -405,15 +405,19 @@ class Handler(BaseHTTPRequestHandler): registry = CollectorRegistry() registry.register(Sample(current)) body, kind, code = generate_latest(registry), CONTENT_TYPE_LATEST, 200 - self.send_response(code) - self.send_header('Content-Type', kind) - self.send_header('Content-Length', str(len(body))) - self.send_header('Cache-Control', 'no-store') - for name, value in extra: - self.send_header(name, value) - self.end_headers() - if send_body: - self.wfile.write(body) + try: + self.send_response(code) + self.send_header('Content-Type', kind) + self.send_header('Content-Length', str(len(body))) + self.send_header('Cache-Control', 'no-store') + for name, value in extra: + self.send_header(name, value) + self.end_headers() + if send_body: + self.wfile.write(body) + except ConnectionError: + # The client went away before the answer was written. + self.close_connection = True def do_GET(self): self.answer(True) diff --git a/debian-package/src/usr/lib/vpn-router/vpnrouter_web/handler.py b/debian-package/src/usr/lib/vpn-router/vpnrouter_web/handler.py index 98b0910..57c82c9 100644 --- a/debian-package/src/usr/lib/vpn-router/vpnrouter_web/handler.py +++ b/debian-package/src/usr/lib/vpn-router/vpnrouter_web/handler.py @@ -188,15 +188,19 @@ class Handler(BaseHTTPRequestHandler): code = 200 else: body, kind, code = b'Not found\n', 'text/plain; charset=utf-8', 404 - self.send_response(code) - self.send_header('Content-Type', kind) - self.send_header('Content-Length', str(len(body))) - self.send_header('Cache-Control', 'no-store') - for name, value in extra: - self.send_header(name, value) - self.end_headers() - if send_body: - self.wfile.write(body) + try: + self.send_response(code) + self.send_header('Content-Type', kind) + self.send_header('Content-Length', str(len(body))) + self.send_header('Cache-Control', 'no-store') + for name, value in extra: + self.send_header(name, value) + self.end_headers() + if send_body: + self.wfile.write(body) + except ConnectionError: + # The client went away before the answer was written. + self.close_connection = True def raw_body(self): """The request body's bytes, b'' without one or over BODY_LIMIT.""" diff --git a/debian-package/src/usr/lib/vpn-router/vpnrouter_web/shell.py b/debian-package/src/usr/lib/vpn-router/vpnrouter_web/shell.py index 426e099..32d1f74 100644 --- a/debian-package/src/usr/lib/vpn-router/vpnrouter_web/shell.py +++ b/debian-package/src/usr/lib/vpn-router/vpnrouter_web/shell.py @@ -367,11 +367,14 @@ def shell(handler, name): def refuse(code, message): log(f'shell of {name} from {handler.client()} refused: {message}') body = json.dumps({'error': message}).encode() + b'\n' - handler.send_response(code) - handler.send_header('Content-Type', 'application/json') - handler.send_header('Content-Length', str(len(body))) - handler.end_headers() - handler.wfile.write(body) + try: + handler.send_response(code) + handler.send_header('Content-Type', 'application/json') + handler.send_header('Content-Length', str(len(body))) + handler.end_headers() + handler.wfile.write(body) + except ConnectionError: + handler.close_connection = True key = handler.headers.get('Sec-WebSocket-Key', '').strip() if handler.headers.get('Upgrade', '').lower() != 'websocket' or 'upgrade' not in handler.headers.get('Connection', '').lower() \ @@ -408,12 +411,21 @@ def shell(handler, name): finally: os.close(terminal) accept = base64.b64encode(hashlib.sha1((key + WEBSOCKET_GUID).encode()).digest()).decode() - handler.send_response(101, 'Switching Protocols') - handler.send_header('Upgrade', 'websocket') - handler.send_header('Connection', 'Upgrade') - handler.send_header('Sec-WebSocket-Accept', accept) - handler.end_headers() handler.close_connection = True + try: + handler.send_response(101, 'Switching Protocols') + handler.send_header('Upgrade', 'websocket') + handler.send_header('Connection', 'Upgrade') + handler.send_header('Sec-WebSocket-Accept', accept) + handler.end_headers() + except ConnectionError: + # The client went away before the shell opened; the shell started for it is stopped. + run('systemctl', 'stop', unit) + os.close(master) + process.kill() + process.wait() + log(f'shell of {name} from {handler.client()} ended before it opened: the client closed the connection') + return client, started, idle = handler.client(), time.monotonic(), options['shell_idle_minutes'] * 60 log(f'shell of {name} from {client} opened as {account.pw_name} in {unit}') diff --git a/debian-package/src/usr/lib/vpn-router/vpnrouter_web/sites.py b/debian-package/src/usr/lib/vpn-router/vpnrouter_web/sites.py index 1181578..6994f5c 100644 --- a/debian-package/src/usr/lib/vpn-router/vpnrouter_web/sites.py +++ b/debian-package/src/usr/lib/vpn-router/vpnrouter_web/sites.py @@ -112,15 +112,19 @@ def site_files(handler, name, relative, query, payload, send_body): return 404, {'error': f'{relative} does not exist'} with open(target, 'rb') as handle: size = os.fstat(handle.fileno()).st_size - handler.send_response(200) - handler.send_header('Content-Type', mimetypes.guess_type(target.name)[0] or 'application/octet-stream') - handler.send_header('Content-Length', str(size)) - handler.send_header('Content-Disposition', - f"attachment; filename*=UTF-8''{urllib.parse.quote(target.name)}") - handler.send_header('Cache-Control', 'no-store') - handler.end_headers() - if send_body: - shutil.copyfileobj(handle, handler.wfile, vpnrouter_sites.CHUNK) + try: + handler.send_response(200) + handler.send_header('Content-Type', mimetypes.guess_type(target.name)[0] or 'application/octet-stream') + handler.send_header('Content-Length', str(size)) + handler.send_header('Content-Disposition', + f"attachment; filename*=UTF-8''{urllib.parse.quote(target.name)}") + handler.send_header('Cache-Control', 'no-store') + handler.end_headers() + if send_body: + shutil.copyfileobj(handle, handler.wfile, vpnrouter_sites.CHUNK) + except ConnectionError: + # The client went away before the file was sent. + handler.close_connection = True return None if handler.command == 'PUT': length = handler.headers.get('Content-Length')