polish(network): outermost PIN gate + non-buffering ASGI middleware + listener test#160
Conversation
Move the <RemoteAuthGate> wrap from App.jsx's main-studio return up to main-app.jsx, inside QueryClientProvider and wrapping the entire app tree (both the dictation widget and <App />). Previously the gate only wrapped the studio return, so a remote device opening a bare URL (no ?pin=) during first-run states — the /setup/status check, SetupWizard, or BootstrapSplash early returns — would 401 with no gate rendered to collect the PIN. The QR path was fine (PIN captured pre-fetch in client.ts); only bare-URL was broken. Remove the App.jsx wrap to avoid double-gating (two PIN dialogs). No behavior change for loopback or QR users. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rewrite NetworkAccessMiddleware from a starlette BaseHTTPMiddleware into a pure ASGI middleware (class with __init__(app) and __call__(scope, receive, send)). BaseHTTPMiddleware buffers StreamingResponse/SSE bodies before forwarding them, so PIN'd LAN clients on streaming endpoints (dictation SSE, tts streaming, /system/logs/stream) got buffered/laggy responses. Loopback was unaffected (bypasses early), but remote-share streaming was degraded. The ASGI form forwards send untouched on every pass-through path, and only wraps send to inject Set-Cookie on the http.response.start message for the first valid-PIN request — the body keeps streaming chunk-by-chunk. request.app resolves in ASGI scope (Starlette sets scope["app"]), so the inert/loopback/ shell/PIN logic is identical to before. Registered after CORS (unchanged) so CORS stays outermost. All 5 existing behavior tests pass unchanged. Adds three tests: a guard that the middleware is not a BaseHTTPMiddleware subclass, a StreamingResponse pass-through (401 without PIN, full chunked stream with PIN, no buffered Content-Length), and a Set-Cookie-via-ASGI assertion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add tests/test_network_share_lifecycle.py exercising the real second uvicorn listener: await network_share.enable(app) on a minimal FastAPI app, assert get_state().enabled is True with a share_port set and a live TCP listener on that port (real socket connect), then await disable(app) and assert the state resets and the port stops accepting connections. Uses the returned share_port (never a hardcoded port) and tolerates teardown timing by polling for socket close. Wrapped in asyncio.run inside a sync test so it does not depend on a pytest-asyncio event-loop mode; skips gracefully if binding 0.0.0.0 is not permitted in the sandbox. Defensive cleanup resets the module-level state on any failure path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughBackend middleware refactored from buffering ChangesPIN Authentication & Network Access Control
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| # Probe whether binding 0.0.0.0 is permitted in this sandbox. | ||
| with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as probe: | ||
| try: | ||
| probe.bind(("0.0.0.0", 0)) |
|
| Filename | Overview |
|---|---|
| backend/main.py | NetworkAccessMiddleware rewritten from BaseHTTPMiddleware to pure ASGI; all 5 security behaviors preserved; minor style issue with in-body import on hot path |
| frontend/src/App.jsx | RemoteAuthGate wrapper removed from App.jsx with clear comment explaining it now lives in main-app.jsx; no logic changes |
| frontend/src/main-app.jsx | RemoteAuthGate moved to outermost position inside QueryClientProvider, correctly covering both widget and full-app render paths |
| tests/test_network_middleware.py | Three new tests added: ASGI class-check, streaming pass-through with valid PIN, and cookie injection via ASGI send-wrapper; content-length absence check is a reasonable proxy but may be environment-sensitive |
| tests/test_network_share_lifecycle.py | New integration test exercises the real listener lifecycle (enable/disable) with actual TCP socket probing; includes sandbox skip guard and defensive finally cleanup |
Sequence Diagram
sequenceDiagram
participant Client as LAN Client
participant CORS as CORSMiddleware
participant NAM as NetworkAccessMiddleware (ASGI)
participant App as FastAPI App
Client->>CORS: HTTP Request
CORS->>NAM: scope, receive, send
NAM->>NAM: "scope["type"] != "http"?"
alt WebSocket / lifespan
NAM->>App: pass-through (untouched)
else No PIN configured
NAM->>App: pass-through
else Loopback client
NAM->>App: pass-through
else SPA shell path ("/", "/assets/…")
NAM->>App: pass-through
else PIN missing / wrong
NAM-->>Client: 401 JSON (via JSONResponse ASGI call)
else Valid PIN, cookie not set
NAM->>App: app(scope, receive, send_with_cookie)
App-->>NAM: http.response.start (headers)
NAM->>NAM: "MutableHeaders.append("set-cookie", "ov_pin=…")"
NAM-->>Client: response + Set-Cookie header (streamed chunk-by-chunk)
else Valid PIN, cookie already present
NAM->>App: app(scope, receive, send)
App-->>Client: response (streamed chunk-by-chunk)
end
Reviews (1): Last reviewed commit: "test(network-share): integration test fo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| async def __call__(self, scope, receive, send): | ||
| if scope["type"] != "http": | ||
| return await self.app(scope, receive, send) | ||
| from starlette.requests import Request | ||
|
|
||
| request = Request(scope, receive=receive) |
There was a problem hiding this comment.
from starlette.requests import Request import is placed inside the __call__ hot path, so it runs a sys.modules dict lookup on every HTTP request. Move it to the module-level import block alongside the other Starlette imports already there (e.g., MutableHeaders).
| async def __call__(self, scope, receive, send): | |
| if scope["type"] != "http": | |
| return await self.app(scope, receive, send) | |
| from starlette.requests import Request | |
| request = Request(scope, receive=receive) | |
| async def __call__(self, scope, receive, send): | |
| if scope["type"] != "http": | |
| return await self.app(scope, receive, send) | |
| request = Request(scope, receive=receive) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| from fastapi.staticfiles import StaticFiles | ||
| from fastapi.middleware.cors import CORSMiddleware | ||
| from starlette.middleware.base import BaseHTTPMiddleware | ||
| from starlette.datastructures import MutableHeaders |
There was a problem hiding this comment.
Request should be added alongside the existing Starlette imports so the in-body import can be removed.
| from starlette.datastructures import MutableHeaders | |
| from starlette.datastructures import MutableHeaders | |
| from starlette.requests import Request |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| assert f"chunk-{i}" in body | ||
| # Streaming responses carry no precomputed Content-Length — a buffering | ||
| # middleware would re-materialise the body and set one. | ||
| assert "content-length" not in {k.lower() for k in r.headers} |
There was a problem hiding this comment.
The assertion uses Content-Length absence to confirm the middleware isn't buffering the body, with the comment noting that a buffering middleware would re-materialise the body and set one. In practice, modern Starlette BaseHTTPMiddleware no longer unconditionally adds Content-Length after the buffering fix in 0.20+, so this check could silently pass even if the code regresses to BaseHTTPMiddleware. The test_middleware_is_plain_asgi_not_buffering test (line 46) is a more reliable guard for that regression. Consider strengthening this test by also verifying the body is delivered incrementally (e.g., via a chunked iter_lines() approach with the async client), or document that the Content-Length check is a best-effort signal only.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_network_middleware.py (1)
53-54: 💤 Low valueSimplify the callable check.
The Ruff B004 warning appears to be a false positive since this code uses
callable()correctly. However, the check can be simplified — instances of a class are callable if the class defines__call__, and this is more directly tested by checking an instance:- assert not issubclass(NetworkAccessMiddleware, BaseHTTPMiddleware) - assert callable(getattr(NetworkAccessMiddleware, "__call__", None)) + assert not issubclass(NetworkAccessMiddleware, BaseHTTPMiddleware) + # Class defines __call__ → instances are callable ASGI apps + assert callable(NetworkAccessMiddleware(lambda s, r, se: None))Alternatively, keep the current form since it's functionally correct; the static analysis hint is a false positive here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_network_middleware.py` around lines 53 - 54, The test currently checks callable(getattr(NetworkAccessMiddleware, "__call__", None)) which triggers Ruff B004; replace this with a direct instance-level callable check by creating an instance of NetworkAccessMiddleware (e.g., middleware = NetworkAccessMiddleware(...)) and asserting callable(middleware), while keeping the issubclass check against BaseHTTPMiddleware unchanged; reference the NetworkAccessMiddleware class and its __call__ behavior when making the change.frontend/src/main-app.jsx (1)
56-92: 💤 Low valueHardcoded string should use i18n.
Line 83 contains hardcoded English text
"Loading dictation…"which should go through the translation layer.♻️ Suggested fix
Add import at top of file:
import { useTranslation } from 'react-i18next';Then update the Suspense fallback to use a small wrapper component (since hooks can't be called directly in JSX expressions):
+const WidgetFallback = () => { + const { t } = useTranslation(); + return ( + <div + style={{ + position: 'fixed', + inset: 0, + display: 'flex', + alignItems: 'center', + justifyContent: 'center', + background: 'rgba(18, 18, 22, 0.88)', + backdropFilter: 'blur(24px) saturate(180%)', + WebkitBackdropFilter: 'blur(24px) saturate(180%)', + border: '1px solid rgba(255, 255, 255, 0.08)', + borderRadius: '100px', + color: 'rgba(255, 255, 255, 0.9)', + fontFamily: '"Inter Variable", "Inter", -apple-system, sans-serif', + fontSize: 13, + userSelect: 'none', + }} + > + {t('widget.loading_dictation')} + </div> + ); +};And use
<WidgetFallback />as the Suspense fallback.As per coding guidelines: "All user-facing text in the UI must go through the i18n translation layer using t('...') keys in locales/*.json files, never hardcode non-English text"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/main-app.jsx` around lines 56 - 92, The Suspense fallback contains a hardcoded user-facing string "Loading dictation…" which must use the i18n layer; import useTranslation from 'react-i18next', create a small wrapper component (e.g. WidgetFallback) that calls const { t } = useTranslation() and returns the same styled div but with t('widget.loading_dictation') (add the key to locales/*.json), and then replace the inline fallback JSX with <WidgetFallback /> inside RemoteAuthGate/Suspense (refer to CaptureWidget and Suspense fallback).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_network_share_lifecycle.py`:
- Around line 76-94: Replace the dynamic __import__("fastapi").FastAPI() calls
in test_share_listener_lifecycle() with a module-level "from fastapi import
FastAPI" and reuse FastAPI() instances for both defensive cleanup calls; update
test_share_listener_lifecycle() to call ns.disable(FastAPI()) in the two places
where __import__ is used, and remove the now-unnecessary local "from fastapi
import FastAPI" import inside _exercise_lifecycle(); keep all calls to
ns.disable(app) as-is since it only sets app.state.network_share.
---
Nitpick comments:
In `@frontend/src/main-app.jsx`:
- Around line 56-92: The Suspense fallback contains a hardcoded user-facing
string "Loading dictation…" which must use the i18n layer; import useTranslation
from 'react-i18next', create a small wrapper component (e.g. WidgetFallback)
that calls const { t } = useTranslation() and returns the same styled div but
with t('widget.loading_dictation') (add the key to locales/*.json), and then
replace the inline fallback JSX with <WidgetFallback /> inside
RemoteAuthGate/Suspense (refer to CaptureWidget and Suspense fallback).
In `@tests/test_network_middleware.py`:
- Around line 53-54: The test currently checks
callable(getattr(NetworkAccessMiddleware, "__call__", None)) which triggers Ruff
B004; replace this with a direct instance-level callable check by creating an
instance of NetworkAccessMiddleware (e.g., middleware =
NetworkAccessMiddleware(...)) and asserting callable(middleware), while keeping
the issubclass check against BaseHTTPMiddleware unchanged; reference the
NetworkAccessMiddleware class and its __call__ behavior when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 560cafb1-644a-441a-8467-cb484157d88d
📒 Files selected for processing (5)
backend/main.pyfrontend/src/App.jsxfrontend/src/main-app.jsxtests/test_network_middleware.pytests/test_network_share_lifecycle.py
| def test_share_listener_lifecycle(): | ||
| # Ensure a clean starting state regardless of test ordering. | ||
| if ns.get_state().enabled: | ||
| asyncio.run(ns.disable(__import__("fastapi").FastAPI())) | ||
|
|
||
| try: | ||
| # Probe whether binding 0.0.0.0 is permitted in this sandbox. | ||
| with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as probe: | ||
| try: | ||
| probe.bind(("0.0.0.0", 0)) | ||
| except OSError as e: | ||
| pytest.skip(f"binding 0.0.0.0 not permitted in this sandbox: {e}") | ||
|
|
||
| asyncio.run(_exercise_lifecycle()) | ||
| finally: | ||
| # Defensive cleanup so a failure mid-test never leaves a stray listener | ||
| # or a dirty module-level _state for the next test. | ||
| if ns.get_state().enabled: | ||
| asyncio.run(ns.disable(__import__("fastapi").FastAPI())) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether disable() reads app.state.network_share (would fail on a fresh app).
fd -t f 'network_share.py' --exec rg -nP -C8 '\b(async\s+)?def\s+disable\b' {}Repository: debpalash/OmniVoice-Studio
Length of output: 640
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the service module
fd -t f -i 'network_share.py' .
# Print the full disable() function body and any app.state usage around it
fd -t f -i 'network_share.py' --exec rg -nP -S '^(async\s+def\s+disable\s*\(|def\s+disable\s*\()' {} \; --max-count 20
# Show the disable() body + any app.state.network_share references
fd -t f -i 'network_share.py' --exec rg -nP -S -C3 'network_share\.|app\.state\.' {} \;
# Finally, dump a wider window around disable()
fd -t f -i 'network_share.py' --exec rg -nP -S -C25 'def\s+disable\s*\(' {} \;Repository: debpalash/OmniVoice-Studio
Length of output: 452
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="./backend/services/network_share.py"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE" >&2
exit 1
fi
echo "== file line count =="
wc -l "$FILE"
echo "== disable() definition lines =="
rg -nP 'async\s+def\s+disable\s*\(' "$FILE" || true
echo "== disable() body (approx) =="
# Print a window around disable()
LINE=$(rg -nP 'async\s+def\s+disable\s*\(' "$FILE" | head -n1 | cut -d: -f1)
START=$((LINE-5))
END=$((LINE+120))
sed -n "${START},${END}p" "$FILE" | cat -n
echo "== occurrences of app.state.network_share =="
rg -nP 'app\.state\.network_share' "$FILE" || true
echo "== occurrences of app.state. within network_share.py =="
rg -nP 'app\.state\.' "$FILE" || true
echo "== where FastAPI is imported/used in this module =="
rg -nP '\bFastAPI\b|from\s+fastapi|import\s+fastapi' "$FILE" || trueRepository: debpalash/OmniVoice-Studio
Length of output: 1274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== network_share.py =="
nl -ba backend/services/network_share.py | sed -n '1,160p'
echo "== locate lifecycle test file =="
fd -t f -i 'test_network_share_lifecycle\.py' . || true
echo "== show lifecycle test (if found) =="
TESTFILE=$(fd -t f -i 'test_network_share_lifecycle\.py' . | head -n1 || true)
if [ -n "${TESTFILE}" ] && [ -f "${TESTFILE}" ]; then
echo "Using: $TESTFILE"
nl -ba "$TESTFILE" | sed -n '1,200p'
fi
echo "== also locate any use of ns.disable(FastAPI|__import__) in tests =="
rg -n --hidden -S 'ns\.disable\(|network_share\.disable\(|disable\(__import__|FastAPI\(\)\)' . -g'*test*.py' || trueRepository: debpalash/OmniVoice-Studio
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== network_share.py =="
cat -n backend/services/network_share.py
echo "== locate lifecycle test file =="
fd -t f -i 'test_network_share_lifecycle\.py' . || true
echo "== locate any test file with network_share lifecycle wording =="
fd -t f -i '*network_share*lifecycle*test*.py' . || true
echo "== find uses of ns.disable/disable in tests =="
rg -n --hidden -S 'ns\.disable\(|network_share\.disable\(|disable\(__import__\(|FastAPI\(\)' . -g'*test*.py' || trueRepository: debpalash/OmniVoice-Studio
Length of output: 5488
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n tests/test_network_share_lifecycle.py | sed -n '1,140p'Repository: debpalash/OmniVoice-Studio
Length of output: 4205
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "__import__\\(\"fastapi\"\\)" -S tests backend || trueRepository: debpalash/OmniVoice-Studio
Length of output: 286
Tidy up FastAPI usage in test_share_listener_lifecycle()
ns.disable(app)only assignsapp.state.network_share(it doesn’t read it), so passing a freshFastAPI()for the defensive cleanup at lines 79/94 won’t trigger anAttributeError.- Replace
__import__("fastapi").FastAPI()with a module-scopefrom fastapi import FastAPIand reuseFastAPI()for the two cleanup calls (then the localfrom fastapi import FastAPIinside_exercise_lifecyclecan be removed).
♻️ Hoist the FastAPI import
import asyncio
import socket
import pytest
+from fastapi import FastAPI
from services import network_share as ns if ns.get_state().enabled:
- asyncio.run(ns.disable(__import__("fastapi").FastAPI()))
+ asyncio.run(ns.disable(FastAPI())) if ns.get_state().enabled:
- asyncio.run(ns.disable(__import__("fastapi").FastAPI()))
+ asyncio.run(ns.disable(FastAPI()))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_share_listener_lifecycle(): | |
| # Ensure a clean starting state regardless of test ordering. | |
| if ns.get_state().enabled: | |
| asyncio.run(ns.disable(__import__("fastapi").FastAPI())) | |
| try: | |
| # Probe whether binding 0.0.0.0 is permitted in this sandbox. | |
| with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as probe: | |
| try: | |
| probe.bind(("0.0.0.0", 0)) | |
| except OSError as e: | |
| pytest.skip(f"binding 0.0.0.0 not permitted in this sandbox: {e}") | |
| asyncio.run(_exercise_lifecycle()) | |
| finally: | |
| # Defensive cleanup so a failure mid-test never leaves a stray listener | |
| # or a dirty module-level _state for the next test. | |
| if ns.get_state().enabled: | |
| asyncio.run(ns.disable(__import__("fastapi").FastAPI())) | |
| def test_share_listener_lifecycle(): | |
| # Ensure a clean starting state regardless of test ordering. | |
| if ns.get_state().enabled: | |
| asyncio.run(ns.disable(FastAPI())) | |
| try: | |
| # Probe whether binding 0.0.0.0 is permitted in this sandbox. | |
| with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as probe: | |
| try: | |
| probe.bind(("0.0.0.0", 0)) | |
| except OSError as e: | |
| pytest.skip(f"binding 0.0.0.0 not permitted in this sandbox: {e}") | |
| asyncio.run(_exercise_lifecycle()) | |
| finally: | |
| # Defensive cleanup so a failure mid-test never leaves a stray listener | |
| # or a dirty module-level _state for the next test. | |
| if ns.get_state().enabled: | |
| asyncio.run(ns.disable(FastAPI())) |
🧰 Tools
🪛 Ruff (0.15.14)
[error] 85-85: Possible binding to all interfaces
(S104)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_network_share_lifecycle.py` around lines 76 - 94, Replace the
dynamic __import__("fastapi").FastAPI() calls in test_share_listener_lifecycle()
with a module-level "from fastapi import FastAPI" and reuse FastAPI() instances
for both defensive cleanup calls; update test_share_listener_lifecycle() to call
ns.disable(FastAPI()) in the two places where __import__ is used, and remove the
now-unnecessary local "from fastapi import FastAPI" import inside
_exercise_lifecycle(); keep all calls to ns.disable(app) as-is since it only
sets app.state.network_share.
2 participants
Three quality follow-ups to the network-sharing feature (#159), each from the final review.
App.jsx(which wrapped only the main studio view) tomain-app.jsxinsideQueryClientProvider, so a remote device loading a bare URL (no?pin=) during first-run states (setup-status / SetupWizard / BootstrapSplash) now gets the gate instead of a silent 401. QR path and loopback are unchanged (gate only renders onov:pin-required). Removed the old wrap to avoid double-gating.NetworkAccessMiddlewarefromBaseHTTPMiddleware(which buffersStreamingResponse/SSE bodies) to a pure ASGI middleware, so PIN'd LAN clients on streaming endpoints (dictation SSE, tts streaming,/system/logs/stream) stream chunk-by-chunk. All 5 original security behaviors preserved (inert-when-no-PIN, loopback bypass, SPA-shell allowlist, constant-time PIN compare, 401) + 3 new tests (streaming pass-through, plain-ASGI guard).0.0.0.0:<P+1>listener viaenable(), connects to confirm it's live, thendisable()and confirms the port closes.Verification (full CI-equivalent suite, all green locally)
test_network_middlewarenow 8,test_network_share,test_network_share_lifecycle, CJK guard).typecheck:ci✓,test:legacy36/0 ✓,vitest90/90 (17 files) ✓,build✓.🤖 Generated with Claude Code
Summary by CodeRabbit
Performance Improvements
Security Enhancements
Infrastructure