From 2ab2ffa65a05bcd1b81800be16dd416beefcc188 Mon Sep 17 00:00:00 2001 From: saphid <4596216+saphid@users.noreply.github.com> Date: Sat, 26 Sep 2026 20:49:21 +1000 Subject: [PATCH] Fix the final review's findings on the combined features - Links to localhost need FRAME_CONTROL_LOCAL_LINKS=1: otherwise any website's link could make the app fetch from services on this computer. - Title staging folders (unzipped titles) carry the server's PID and are swept on the next start like download folders, so quitting mid-install doesn't leave gigabytes behind. - An install from a link refreshes Sideloaded titles. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/web-install.md | 12 ++++++++---- tests/test_webinstall.py | 31 +++++++++++++++++++++---------- ui/frame_titles.py | 7 ++++--- ui/frame_webinstall.py | 19 +++++++++++-------- ui/index.html | 2 +- ui/server.py | 35 ++++++++++++++++++++--------------- 6 files changed, 65 insertions(+), 41 deletions(-) diff --git a/docs/web-install.md b/docs/web-install.md index 98c0365..f078a74 100644 --- a/docs/web-install.md +++ b/docs/web-install.md @@ -60,9 +60,11 @@ What gets installed depends on the file name's extension: Frame Control refuses a link, and downloads nothing, unless: - Every URL (the manifest's, the file's and each redirect) is `https://`. - `http://` works only for `localhost` or `127.0.0.1`, for testing, and only - when the link itself points there: a public manifest can't send Frame - Control to your own computer. + `http://` works only for `localhost` or `127.0.0.1`, for testing: only when + Frame Control runs with `FRAME_CONTROL_LOCAL_LINKS=1`, and only when the + link itself points there. It's off by default so a website's link can't make + the app fetch from services on your computer, and a public manifest can + never send it there. - No URL has a user name or password in it (`https://user:pw@…`). - No host is, or resolves to, a private, loopback, link-local, CGNAT (100.64.0.0/10), multicast or otherwise non-public address. Every address @@ -116,7 +118,9 @@ Control" link. It isn't published anywhere yet; host a copy to use it. ## Testing locally -Serve the manifest and file from your own computer: +Start Frame Control with `FRAME_CONTROL_LOCAL_LINKS=1` in its environment (for +example `FRAME_CONTROL_LOCAL_LINKS=1 npm start` in `app/`), then serve the +manifest and file from your own computer: ```sh cd mygame && python3 -m http.server 8000 diff --git a/tests/test_webinstall.py b/tests/test_webinstall.py index 0199e17..53917b7 100644 --- a/tests/test_webinstall.py +++ b/tests/test_webinstall.py @@ -181,10 +181,20 @@ class Downloads(unittest.TestCase): def setUp(self): self.tmp = tempfile.mkdtemp() + env = mock.patch.dict(os.environ, {wi.LOCAL_LINKS_ENV: "1"}) + env.start() + self.addCleanup(env.stop) def tearDown(self): shutil.rmtree(self.tmp, ignore_errors=True) + def test_localhost_links_need_the_developer_switch(self): + # Without it, a website's link can't make the app fetch from local services. + with mock.patch.dict(os.environ, {wi.LOCAL_LINKS_ENV: ""}): + for kw in ({"manifest": f"{self.base}/manifest.json"}, {"url": f"{self.base}/game.apk"}): + with self.assertRaisesRegex(wi.WebInstallError, wi.LOCAL_LINKS_ENV): + wi.plan(**kw) + def test_manifest_round_trip(self): p = wi.plan(manifest=f"{self.base}/manifest.json") self.assertEqual((p["name"], p["file"], p["kind"], p["host"], p["size"]), @@ -371,16 +381,17 @@ class ServerJobs(unittest.TestCase): def test_dead_servers_leftovers_swept(self): dead = subprocess.Popen([sys.executable, "-c", "pass"]) dead.wait() - prefix = self.server.WEB_TMP_PREFIX - gone = tempfile.mkdtemp(prefix=f"{prefix}{dead.pid}-") - live = tempfile.mkdtemp(prefix=f"{prefix}{os.getpid()}-") - try: - self.server.sweep_webinstall_tmp() - self.assertFalse(os.path.exists(gone)) - self.assertTrue(os.path.exists(live)) - finally: - shutil.rmtree(gone, ignore_errors=True) - shutil.rmtree(live, ignore_errors=True) + # Downloads and title staging (unzipped titles) are both swept. + for prefix in (self.server.WEB_TMP_PREFIX, self.server.frame_titles.TMP_PREFIX): + gone = tempfile.mkdtemp(prefix=f"{prefix}{dead.pid}-") + live = tempfile.mkdtemp(prefix=f"{prefix}{os.getpid()}-") + try: + self.server.sweep_tmp() + self.assertFalse(os.path.exists(gone), prefix) + self.assertTrue(os.path.exists(live), prefix) + finally: + shutil.rmtree(gone, ignore_errors=True) + shutil.rmtree(live, ignore_errors=True) def test_temp_dir_failure_ends_the_job(self): job, dispatch = self.run_job(mkdtemp_error=OSError("disk full")) diff --git a/ui/frame_titles.py b/ui/frame_titles.py index 0ab0192..a1d63d8 100644 --- a/ui/frame_titles.py +++ b/ui/frame_titles.py @@ -39,6 +39,7 @@ DIR_RE = re.compile(r'^/[A-Za-z0-9_./-]+$') # Zip limits: well above any real game, well below a zip bomb. MAX_UNPACKED = 64 * 1024**3 MAX_ENTRIES = 200000 +TMP_PREFIX = 'frame-title-' # then the server's PID, so server.sweep_tmp can clear a killed run's MAX_RATIO = 200 # uncompressed / compressed, once past 1 GB # The Steam compat tool aliases Valve's client uses (devkit_client RUNTIME_ALIASES). @@ -423,15 +424,15 @@ def inspect(path, name=None): if _has_links(root): # scp -r follows links, so a link out of the folder could upload # anything; copy the folder with its links made safe first. - work = tempfile.mkdtemp(prefix='frame-title-') + work = tempfile.mkdtemp(prefix=f'{TMP_PREFIX}{os.getpid()}-') root = _stage_folder(root, os.path.join(work, os.path.basename(root))) elif path.lower().endswith('.zip'): - work = tempfile.mkdtemp(prefix='frame-title-') + work = tempfile.mkdtemp(prefix=f'{TMP_PREFIX}{os.getpid()}-') extract_zip(path, work) root = _unwrap(work) elif classify(path): # A single executable is uploaded on its own; don't copy a whole Downloads folder. - work = tempfile.mkdtemp(prefix='frame-title-') + work = tempfile.mkdtemp(prefix=f'{TMP_PREFIX}{os.getpid()}-') shutil.copy2(path, os.path.join(work, os.path.basename(path))) root = work else: diff --git a/ui/frame_webinstall.py b/ui/frame_webinstall.py index bae552b..2f2e006 100644 --- a/ui/frame_webinstall.py +++ b/ui/frame_webinstall.py @@ -10,10 +10,10 @@ A manifest is the same JSON FrameDrop uses, so one works for both tools: "frame-control.install/v1" is accepted with the same shape. Rules: HTTPS only, except http(s)://localhost or 127.0.0.1 for testing, and then -only when the link itself points there. No credentials in URLs, no private, -loopback, link-local or CGNAT addresses (checked on every redirect, and the -connection goes to the address that was checked, so DNS can't change it in -between). The file URL must end in a file name. +only with FRAME_CONTROL_LOCAL_LINKS=1 set and when the link itself points there. +No credentials in URLs, no private, loopback, link-local or CGNAT addresses +(checked on every redirect, and the connection goes to the address that was +checked, so DNS can't change it in between). The file URL must end in a file name. Python stdlib only, 3.9 compatible. """ @@ -39,6 +39,8 @@ MAX_REDIRECTS = 5 TIMEOUT = 30 # seconds per socket operation CHUNK = 1 << 20 LOCAL_HOSTS = ("localhost", "127.0.0.1") +# Off by default: otherwise any website could make the app fetch from local services. +LOCAL_LINKS_ENV = "FRAME_CONTROL_LOCAL_LINKS" USER_AGENT = "FrameControl (+https://github.com/saphid/steam-frame)" # What dispatch() can install, by file extension. KINDS = {".apk": "apk", ".zip": "title", ".exe": "title"} @@ -101,7 +103,7 @@ def check_url(url, allow_local=False): raise WebInstallError("the URL has no host") local = host in LOCAL_HOSTS if local and not allow_local: - raise WebInstallError("localhost is only allowed when the link itself points there (for testing)") + raise WebInstallError(f"localhost links are for testing: set {LOCAL_LINKS_ENV}=1, and the link itself must point there") if scheme == "http" and not local: raise WebInstallError("only https:// is allowed (http:// only for localhost while testing)") if not local: @@ -342,9 +344,10 @@ def plan(manifest=None, url=None): if (manifest is None) == (url is None): raise WebInstallError("give either manifest or url") link = manifest if manifest is not None else url - # localhost is for testing a link on your own computer, so only a link that - # starts there may reach it: a public manifest can't point at localhost. - allow_local = check_url(link, allow_local=True)[3] + # localhost is for testing a link on your own computer: only with the developer + # switch on, and only for a link that starts there (a public manifest can't + # point at localhost). + allow_local = check_url(link, allow_local=os.environ.get(LOCAL_LINKS_ENV) == "1")[3] if manifest is not None: m = fetch_manifest(manifest, allow_local) name, f = m["name"], m["file"] diff --git a/ui/index.html b/ui/index.html index 81cb7d8..c907b73 100644 --- a/ui/index.html +++ b/ui/index.html @@ -1880,7 +1880,7 @@ async function wiPoll(gen) { if (j.phase === "done") { log(j.message, "ok"); toast(j.message); $("wiDlg").close(); - loadAndroid(); refresh(); + loadAndroid(); loadTitles(); refresh(); } else { $("wiMsg").textContent = j.error; log(`Install from link failed: ${j.error}`, "e"); toast(`Install failed: ${j.error}`, true); diff --git a/ui/server.py b/ui/server.py index 181b9e2..05661e3 100755 --- a/ui/server.py +++ b/ui/server.py @@ -890,7 +890,7 @@ _web_jobs = {} # id -> progress of the confirmed install (only the latest is _web_workers = set() # threads running an install, joined on shutdown _web_closing = False # set on shutdown; no new installs after that MAX_WEB_PLANS = 8 -WEB_TMP_PREFIX = "frame-webinstall-" # then the server's PID, for sweep_webinstall_tmp +WEB_TMP_PREFIX = "frame-webinstall-" # then the server's PID, for sweep_tmp def webinstall_check(body): @@ -1003,7 +1003,7 @@ def webinstall_cancel(body): def webinstall_shutdown(): """Stop downloads and give workers a moment to delete their temporary files. - An install already copying to the Frame may outlive this; sweep_webinstall_tmp + An install already copying to the Frame may outlive this; sweep_tmp removes what it leaves on a later start. """ global _web_closing @@ -1044,21 +1044,26 @@ def _pid_alive(pid): return True -def sweep_webinstall_tmp(): - """Delete download folders left by a server that was killed mid-install. +def sweep_tmp(): + """Delete download and title staging folders left by a server killed mid-install. Folders carry the server's PID, so only a dead server's are taken. """ - for d in Path(tempfile.gettempdir()).glob(f"{WEB_TMP_PREFIX}*"): - m = re.fullmatch(re.escape(WEB_TMP_PREFIX) + r"(\d+)-.*", d.name) - if not m: - continue - pid = int(m[1]) - try: - if pid != os.getpid() and not _pid_alive(pid) and d.is_dir(): - shutil.rmtree(d, ignore_errors=True) - except OSError: - pass + for prefix in (WEB_TMP_PREFIX, frame_titles.TMP_PREFIX): + for d in Path(tempfile.gettempdir()).glob(f"{prefix}*"): + _sweep_one(prefix, d) + + +def _sweep_one(prefix, d): + m = re.fullmatch(re.escape(prefix) + r"(\d+)-.*", d.name) + if not m: + return + pid = int(m[1]) + try: + if pid != os.getpid() and not _pid_alive(pid) and d.is_dir(): + shutil.rmtree(d, ignore_errors=True) + except OSError: + pass POST = {"/api/android/display": android_display, "/api/android": android, "/api/titles": titles, "/api/launch": launch, "/api/steam": steam, "/api/volume": set_volume, "/api/clipboard": clipboard, @@ -1350,7 +1355,7 @@ def main(): "Windows has no SIGTERM to catch)") args = ap.parse_args() httpd = ThreadingHTTPServer(("127.0.0.1", args.port), Handler) - sweep_webinstall_tmp() + sweep_tmp() if not frame_host.WINDOWS: signal.signal(signal.SIGTERM, lambda *_: (_ for _ in ()).throw(KeyboardInterrupt)) if args.exit_on_eof: