Conversation
Unauthenticated callers on a LAN could inject export history records
or read local filesystem paths exposed in export history. Added
`dependencies=[Depends(require_loopback)]` to both endpoints, matching
the pattern already used by DELETE /export/history/{id}.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016CV9A88tyfnofgQu4Dyj16
|
[High risk] Restricts two export endpoints to loopback-only access. The PR is not ready to merge because a supported remote-backend save can succeed without being recorded in export history, and the required guard tests are missing.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
|
|
||
| @router.post("/export/record") | ||
| @router.post("/export/record", dependencies=[Depends(require_loopback)]) |
There was a problem hiding this comment.
saveExport only logs the error, so the export never appears in history. Preserve an authorized way to record completed native saves without allowing unauthenticated remote writes.
|
|
||
|
|
||
| @router.post("/export/record") | ||
| @router.post("/export/record", dependencies=[Depends(require_loopback)]) |
There was a problem hiding this comment.
/export/record and /export/history have no tests checking that non-loopback callers are rejected and authorized loopback callers succeed. CLAUDE.md requires a fail-before/pass-after regression test for fixes; add that coverage for both routes before merging.
Context Used: Review as a panel of senior domain experts (ML inference, audio DSP, desktop systems). Comment ONLY on findings that would change what gets merged: a concrete bug, a violated house rule from CLAUDE.md, a real security/data risk. Per finding: at most ... (source)
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: debpalash/VoiceStudio/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe ChangesExport route access
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~5 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The export routes follow the existing server-mode access rules, with no identified issue blocking merge. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 6 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (6 passed)
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 |
Consolidate community engine, workflow, dictation, and setup fixes with the Electron composer, sidebar, and voice UI. Fix review findings in engine residency, remote exports, backup cleanup, bounded compressed-audio decoding, and reference preprocessing. Preserve contributor credits in CHANGELOG.md and leave the app version unchanged. Supersedes #2325, #2338, #2368, #2377, #2379, #2380, #2383, #2384, #2387, #2390, #2391, #2392, #2393, #2395, #2400, #2401, #2402, #2409, #2410, and #2412.
|
Implemented and merged through #2419. Contributor credit is preserved in CHANGELOG.md. |
sedatdagg
changed the title
3 participants
POST /export/record and GET /export/history were the only two routes on the
export router with no access dependency. On a desktop install both auth
middlewares are inert — NetworkAccessMiddleware needs a share PIN and
BearerKeyMiddleware needs OMNIVOICE_API_KEY, and a desktop install sets
neither — so nothing else stood in the way: any host on the same network could
forge rows into the user's export history and read it back. The rows carry
absolute destination paths, so the GET also leaked the shape of the user's
filesystem.
Found by auditing the router's guard matrix against its siblings; no issue filed.
This revision replaces the require_loopback first attempt with require_local
after @greptile-apps flagged (P1) that require_loopback would break remote
backends. Detail under Changes.
Changes
backend/api/routers/exports.py — dependencies=[Depends(require_local)] on
POST /export/record and GET /export/history.
tests/test_exports_api.py — regression coverage for all three deployment
shapes, plus a module-docstring entry describing it.
Why require_local and not require_loopback. require_loopback is too
strict here. Under OMNIVOICE_SERVER_MODE (Docker, or an Electron client
pointed at a remote backend) the bridge NAT rewrites client.host to the
gateway, so every caller looks non-loopback — POST is a non-safe method and
falls through to require_admin, which a share PIN cannot satisfy, and GET is
refused as soon as any credential is configured. The file would save and then
vanish from the user's history. require_local is the consumption-tier
companion api/dependencies.py documents for exactly this case.
Resulting behaviour, verified by executing the real require_local against each
shape:
Deployment
caller
before
after
Desktop (default)
loopback
allow
allow
Desktop (default)
LAN peer
allow — the bug
403
Desktop, OMNIVOICE_TRUSTED_NETWORKS set
listed LAN peer
allow
allow
Server mode (Docker / remote Electron)
bridge gateway
allow
allow
With OMNIVOICE_TRUSTED_NETWORKS unset — the desktop default — require_local
is identical to require_loopback, so the LAN exposure stays closed. In server
mode it is a no-op, so the remote deployment keeps working; exposure there stays
governed by the operator's port mapping plus the optional PIN or API key.
Export history is consumption data, not admin surface. The destructive
DELETE /export/history/{id} keeps its stricter require_loopback — untouched,
as is every other route on the router.
Type
[x] 🐛 Bug fix
[ ] ✨ New feature
[ ] ♻️ Refactor
[ ] 📝 Documentation
[ ] 🧪 Tests
[ ] 🔧 CI / Build
[ ] 🚀 Release prep
Testing
pytest could not be run in the authoring sandbox — it has no package index
access, so the backend dependencies cannot be installed. CI is the authority
on the suite. What was verified locally instead, by executing the real code
rather than a paraphrase of it:
Extracted require_local from backend/api/dependencies.py and
is_local_host / is_loopback / _trusted_networks from backend/core/auth.py
via ast.get_source_segment, executed them against stubbed collaborators, and
confirmed all eight scenarios in the table above (loopback v4/v6, three
non-loopback addresses, the Docker bridge gateway with and without server
mode, and an operator-trusted LAN range).
AST check of the router: the two routes now resolve to require_local,
POST /export, POST /export/reveal and DELETE /export/history/{id} are
unchanged, no route is left unguarded, and all three imported dependencies are
still referenced (no dead import).
Confirmed from backend/main.py that NetworkAccessMiddleware and
BearerKeyMiddleware are inert without a PIN / API key, so the new tests
exercise the dependency rather than a middleware rejection.
New tests (each parametrised over both routes):
test_history_routes_reject_non_local_callers — a 192.168.1.50 client gets
403. Fails against main, where the routes have no dependency at all.
test_history_routes_allow_loopback — the desktop app still gets 200.
test_history_routes_stay_reachable_in_server_mode — under
OMNIVOICE_SERVER_MODE=1 a non-loopback client gets 200. This is the
@greptile-apps P1 as an executable assertion: it fails against the
require_loopback revision of this branch.
Checklist
[x] I've tested this locally
[x] I've updated relevant documentation (if applicable) — module docstring of
tests/test_exports_api.py; no docs/** or README text describes these
routes' access tier, so nothing else is affected by the docs-sync rule
[x] No local machine paths, logs, or personal env details in this PR
[ ] Maintained version files are in sync (if an owner-requested bump) — n/a,
no version bump
[x] If this PR changes runtime behavior, the regression fixture at
tests/fixtures/omnivoice_data/ still loads green on the smoke-matrix
CI job (macOS + Windows + Linux) — no schema, data or path change; to be
confirmed by CI
Release cadence
VoiceStudio ships continuous-to-main — no release candidates, no soak windows.
Every merged PR is immediately part of rolling source (main) and Docker
:latest. Electron artifact rehearsals validate desktop packages without publishing.
Version bumps require owner approval; validated releases are tagged from main
and published explicitly under the release checklist.
Users who want stability install an Electron release or pin Docker :stable.