fix(updater): only install app updates from our own release assets - #528
Merged
Conversation
thcp
marked this pull request as ready for review
August 31, 2026 21:03
download_app_update took its URL straight from the WebView and verified the download against a SHA-256 supplied by the same caller, so the checksum proved the bytes arrived intact -- not that they came from us. apply_app_update then extracts that archive over StemDeck's own executable and backend/ and relaunches. Reachability is narrower than it first looks: in the normal flow the URL is appAsset.browser_download_url from the GitHub API, no XSS was found, and the CSP is script-src 'self'. But the page is served over http by the Python backend, which Tauri treats as a remote origin, and these app-defined commands are not ACL-gated by the capability config -- both facts the code already documents. A LAN attacker reaches this once network access is enabled. validate_release_url pins the host to github.com and objects.githubusercontent.com (GitHub redirects release assets to the latter) and requires https, so bytes cannot be swapped in flight on a network where the page itself is already plain http. Deliberately a new function rather than the existing validate_download_url: that one permits only 127.0.0.1/localhost, serves a different caller, and would reject every legitimate release URL. apply_app_update also re-verifies now. It trusted that whatever sat at the archive path was what download_app_update had approved, so anything able to write into data/downloads between the two calls was extracted unchecked. The verified digest is recorded next to the archive and re-checked before extraction. The test covers host lookalikes (github.com.evil.example, notgithub.com) as well as plain rejection, since a substring check would pass those. Refs #510
thcp
force-pushed
the
fix/510-updater-url-allowlist
branch
from
August 31, 2026 21:05
6a05da5 to
a404c7f
Compare
thcp
added a commit
that referenced
this pull request
Aug 31, 2026
#529) Fixes #516. **Targets `fix/510-updater-url-allowlist`** -- both touch `main.rs` and the plan sequences the Rust changes. Merge #528 first. ## The problem `child_output_with_timeout` read the pipes only after `try_wait()` reported an exit: ```rust loop { if let Some(status) = child.try_wait()? { // read_to_end on stdout/stderr -- only reachable once the child is gone } thread::sleep(Duration::from_millis(100)); } ``` A child that outruns the OS pipe buffer blocks in `write()` with nobody reading, so it never exits, so `try_wait()` never reports an exit. The call ends at the timeout with no output. `warmup_models` pipes both streams (`main.rs:1414-1415`) and its model downloads emit tqdm progress to stderr in proportion to how **long** they take, not how large they are. The failure therefore lands on slow connections -- exactly the users warmup exists to spare a mid-pipeline download. ## Change Each stream drains on its own thread. Both are required: draining one then the other reintroduces the deadlock on whichever is second. Readers are joined rather than detached on the timeout path, since killing the child closes its ends and dropping the handles would leak two threads per timeout. `command_output_with_timeout` had the identical shape and now delegates, so the two cannot drift apart again. ## Verification -- the deadlock is reproduced, not assumed New test `a_chatty_child_is_drained_rather_than_deadlocked` writes 512 KiB to each stream. | implementation | result | |---|---| | read-after-exit (old) | **FAILED** -- `"chatty child timed out after 20 seconds"`, took 20.09s | | concurrent draining (this PR) | passed in 0.11s | Worth recording: when I filed #516 I measured a real cold-cache warmup on a fast connection and found only **8,360 bytes** of stderr -- under the buffer. That is why this had not been hit in practice, not why it could not happen. The test settles it. ``` cargo fmt --check OK cargo clippy 0 errors cargo test 59 passed, 1 failed ``` The 1 failure is `tests::a_free_port_is_granted_as_asked`, the known parallel-execution flake, failing 3/3 on the base branch without this change. ## One thing to check in review While rewriting `command_output_with_timeout` I initially sliced out the `#[cfg(windows)]` attribute above `hide_console_window`, which `cargo check` caught immediately. Restored, and both `hide_console_window` definitions keep their cfg guards -- worth a glance since a Linux/macOS-only compile would not exercise the Windows one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #510.
The problem
download_app_updatetookplan.app_urlstraight from the WebView and verified againstplan.app_sha256from the same caller. A checksum supplied alongside the URL proves the bytes arrived intact, not that they came from us -- an attacker supplies both.apply_app_updatethen extracts that archive overStemDeck.exeandbackend/and relaunches.Reachability
Narrower than "remote code execution", but real:
appAsset.browser_download_urlfrom the GitHub APIscript-src 'self'with nounsafe-inlinecatalog.js:2386-2393documents that the page is served over http by the Python backend -- a remote origin to Tauri -- and that these app-defined commands are not ACL-gated by the capability config. A LAN attacker reaches this once "Make StemDeck available on your network" is on.So: missing defence-in-depth on a code-execution path.
Change
Both hosts are needed -- GitHub redirects release assets to
objects.githubusercontent.com.httpsis required, so bytes cannot be swapped in flight on a network where the page itself is already plain http.Deliberately a new function, not
validate_download_url. #510 originally suggested reusing that one; it permits only127.0.0.1/localhost, serves a different caller, and would reject every legitimate release URL.apply_app_updatere-verifies. It trusted that whatever sat at the archive path was whatdownload_app_updateapproved, so anything able to write intodata/downloadsbetween the two calls was extracted unchecked. The verified digest is recorded beside the archive and re-checked before extraction.Verification
New test
only_our_own_release_assets_are_downloadable_as_updates, covering host lookalikes a naive substring check would pass:The 1 failure is
tests::a_free_port_is_granted_as_asked. It fails 3/3 on0.16.1without this change too -- verified by checking out the base branch and running the full suite three times. It is the known parallel-execution flake: it probes port 21000 withstd::net::TcpListener(which setsSO_REUSEADDR) then assertsclaim_port(socket2, noSO_REUSEADDR) binds the same port. Currently failing 100% on this machine; unrelated to this PR and not filed.Left for a follow-up
fetch_text(main.rs:943-966) will still GET any URL the WebView passes asruntimeIdUrl/appShaUrl. Weaker than this -- the body is not returned on success, only reachability and parse outcome leak -- but it is the same class and would suit the same allowlist. Kept out of scope here.