fix(audio): pin download redirects and bound ElevenLabs bodies - #1423
Conversation
Whisper and Piper downloads no longer follow off-allowlist redirects. ElevenLabs TTS reads are capped at 32 MiB. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: blocked before merge. Reviewed September 23, 2026, 2:43 PM ET / 18:43 UTC (Revision 6). ClawSweeper reviewWhat this changesThe branch checks each Whisper and Piper asset redirect against approved HTTPS hosts and caps ElevenLabs speech responses at 32 MiB. Merge readiness⛔ Blocked before merge - 4 items remain Keep open. Current main and v2026.9.4 lack this hardening, and the focused patch has credible current-head transport proof. The landing owner still needs to settle CDN compatibility and the collaborator’s interactive Voice Settings proof request. Priority: P2 Review scores
Verification
How this fits togetherVoice Settings obtains catalogued models and voices for local speech features. The speech service sends text to ElevenLabs and passes the returned audio to playback. flowchart LR
A[Voice Settings] --> B[Asset catalog URL]
B --> C[Redirect host check]
C --> D[Verified local model]
E[Speech text] --> F[ElevenLabs response]
F --> G[Body size check]
G --> H[Audio playback]
Decision needed
Why: The collaborator expressly allows an explicit proof disposition, but the PR body alone does not establish who approved its scope or accepted future CDN compatibility failures. The prior rank-up move remains open through this choice. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep per-hop HTTPS host enforcement and bounded response reading, establish ownership of future CDN allowlist updates, and settle the Voice Settings proof scope before release. Do we have a high-confidence way to reproduce the issue? Yes. Current-main source clearly permits automatic asset redirects and buffers the full ElevenLabs body; this read-only review did not execute a failing current-main run. Is this the best way to solve the issue? Yes. Validating each redirect before network I/O and bounding declared and streamed response bodies directly address the reported paths; the remaining question is the intended compatibility and proof policy. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against a2c4647d4289. LabelsLabel changes: No label changes. Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (5 earlier review cycles)
|
Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
|
Global repo triage reviewed exact head Decision: NEEDS_HUMAN_TEST. Take confidence: 68%. Recommendation confidence: 98%. Risk: High compatibility. The implementation is focused and exact-head CI is green, but the compatibility and streaming boundaries are not yet proven. Redirect tests call only Add the required sections, run current-head Voice Settings download/re-download and normal synthesis proof under |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 41d6c280-f27d-481a-a9ff-a5cc8c5d2d78
|
Global triage: NEEDS_HUMAN_TEST. Take confidence 86%; recommendation confidence 96%; effort small; risk medium-high compatibility. Reviewed exact head Remaining gate is the declared |
Maintainer landing decisionExplicitly authorized for head The landing owner accepts the documented transport-only proof at 96% confidence and the The representative live catalog coverage is explicitly limited to two of ten assets. The intentional fail-closed policy for future off-list CDN redirects is accepted: a legitimate new CDN requires an application allowlist update after trusted provenance review. Do not bypass the restriction, weaken redirect security, or disable TLS verification. This explicitly dispositions the earlier interactive Voice Settings proof request as outside this transport-only diff's required scope, not as a passed UI test. Visible Voice Settings behavior, audible playback, valid-audio decoding, and actual ElevenLabs provider availability remain unverified. At this head, full local build passed; Shared 4,093 passed / 35 skipped, Tray 3,065 passed, and focused suites 22 passed. The production transport and official catalog proof was repeated after integration. Fresh Build and Test / CI Gate passed. The latest advisory review identifies no actionable code/security finding; its remaining compatibility and proof-scope decisions are explicitly settled above. Proceed with normal, non-admin, exact-head-guarded squash merge. No force push, check bypass, replacement PR, or branch deletion. |
What Problem This Solves
Voice Settings Whisper and Piper downloads used
new HttpClient()with default redirect following. A compromised Hugging Face Location can send the desktop to an off-allowlist host. SHA-256 still rejects a swapped blob, but the request still leaves the allowlist. ElevenLabs TTS also buffered the entire HTTP body with no size cap. MiniMax already caps at 32 MiB.Evidence
Terminal output from the patched tree:
AllowedAssetDownload.ValidateDownloadUriacceptshuggingface.coand*.huggingface.coon redirects, and rejectshttps://evil.example/model.bin. ElevenLabs rejects a body larger than 32 MiB.Required proof pools
none: Transport-only audio download redirect validation and ElevenLabs HTTP body-size admission. No WinUI controls, UI event wiring, accessibility, permissions, native dependencies, packaging, Gateway, or hardware-specific paths change. Current-head production-manager live catalog installs/re-downloads, controlled production redirect traversal, real localhost HTTP normal/boundary/overflow responses, and full build/tests directly prove the changed contract. Maintainer-approved scope assessment follows.github/proof-pools.jsonselection rules; this does not count an unrun interactive pool as passed.Visual Voice Settings behavior, audible playback, valid-audio decoding, and actual ElevenLabs provider availability remain not verified, outside this diff's proof scope.
Validation
Maintainer validation on 2026-09-23 at integrated head
8631bcf4b728f30043166295671491a26eac16e4, native Windows ARM64, private .NET SDK 10.0.400. Current maina2c4647dwas merged into the original contributor branch without conflicts, rebase, force-push, or replacement PR. All six audio patch file blobs are unchanged from author heada5e730d2; the net PR remains 170 additions / 7 deletions. Existing patch review is reused because integration did not change its implementation.OPENCLAW_REPO_ROOTpointed to the isolated worktree; unique tray data/appdata directories were used. Global-state/local-appdata overrides and E2E flags were unset. No provider credentials, paid synthesis, desktop launch, gateway, or WSL activity in local proof.Fresh CI also passed Core/CLI, Tray/setup/integration, UI/functional/accessibility, setup/connect E2E, revocation recovery E2E, and network recovery E2E. These hosted results are not claimed as local interactive audio proof. Live final disposition:
CLEAN,MERGEABLE, no formal reviews or inline review threads. ClawSweeper's latest review reports no actionable findings and sufficient transport proof; final compatibility-policy/merge approval remains with the landing maintainer.Skipped cases are not counted as runtime proof. Prior-head results remain in separate historical artifacts and are not substituted for integrated-head validation.
Real behavior proof
Integrated-head maintainer runtime evidence (2026-09-23)
The artifact-only harness referenced the actual built
OpenClaw.Shared.dll, invoked its internalAllowedAssetDownload.GetAsyncthrough reflection, and compiled the unmodified productionElevenLabsTextToSpeechClient.csdirectly against that assembly. The Shared assembly informational version contains exact head8631bcf4b728f30043166295671491a26eac16e4. This is execution of production code, not a reimplementation.Commands, with
$proofidentifying the retained session-artifact harness directory:Official live catalog compatibility, repeated after integration: production
WhisperModelManager.DownloadModelAsyncandPiperVoiceManager.DownloadVoiceAsynceach downloaded, deleted from an owned temporary directory, and downloaded again. Normal TLS verification, pinned catalog hashes, redirect validation, and extraction remained enabled. No user cache was accessed. Whispertinyis the smallest Whisper catalog entry; official release metadata identifiedfr_FR-siwis-lowas the smallest supported Piper archive (26,522,254 bytes).Compatibility boundary: live downloads cover two representative assets out of ten, not every catalog asset or future upstream redirect. A future off-allowlist CDN destination intentionally fails closed instead of being requested; accommodating a legitimate new destination would require a reviewed application allowlist update, not disabling redirect checks or TLS verification.
Controlled production redirect traversal: 301/302/303/307/308 each followed a relative hop and permitted CDN hop to success. Off-allowlist, lookalike-domain, HTTP, credential-bearing, and fragment-bearing destinations were rejected after the initial request; the rejected target was never requested. A loop stopped after six requests (initial plus five redirects), missing Location was rejected, and redirect responses were disposed. The production factory uses
SocketsHttpHandler.AllowAutoRedirect=false. This part uses controlled in-memory transport, not localhost HTTPS or an attacker-host request; the official downloads separately exercise real internet transport.ElevenLabs real localhost TCP HTTP: a synthetic POST passed through the production client. No real key or user text was used.
Early rejection / allocation bound: a generated 1 GiB logical stream avoids allocating the offered body. Known-length overflow read zero body bytes. Unknown-length overflow consumed only 33,619,968 bytes (32 MiB plus one 64 KiB read), cumulatively allocated 67,117,832 bytes, and disposed the body. The harness requires allocations below 80 MiB, catching destination growth to 64 MiB or whole-body buffering. Cumulative allocation is not peak working set.
Retained evidence: session
41d6c280-f27d-481a-a9ff-a5cc8c5d2d78,files\integrated-8631bcf4:validation.log,shared.trx,tray.trx,audio-owner.trx,audio-tray.trx,fixture.log,live-catalog.log,proof-identity.json,ci-watch.log, andfinal-review-state.json. Reusable harness source is in siblingfiles\audio-proof; exact validation environment is infiles\validate-audio.ps1. These are local maintainer artifacts, not public attachment links. All owned catalog assets and isolated unit settings were removed after proof.Not verified (outside changed transport scope): visible Voice Settings progress/ready/error presentation, audible local playback, valid-audio decoding, and actual ElevenLabs provider availability or paid synthesis. No passing claim is made for these. Gateway proof is not applicable. The earlier interactive-pool request was reassessed against the actual unchanged UI implementation, not silently treated as satisfied.
Original contributor proof (preserved)
AllowAutoRedirect=falseand only follow HTTPS hosts on the Hugging Face / GitHub allowlist. ElevenLabs reads through a 32 MiB bounded copy, same shape as MiniMax.Summary
Pin Whisper/Piper download redirects and cap ElevenLabs TTS bodies.