fix: harden what the security scan flagged (https-only downloads, no strtok, SHA-1 marked not for security) #1470

2026-11-10T04:05:54Z

NotRexed commented

2026-11-10 04:05:54 +00:00

(Migrated from github.com)

Follows up the security-scan triage (the real finding, the Dependabot cooldown, landed in #1464). The rest were false positives; where a small change makes the code actually safer, this makes it:

Downloads are https-only. builder/linux/linux_update.py sends every urlopen through _https(), which refuses anything else (the release API's asset link included). scripts/release_parts.py's get() does the same, so the CI token is never sent over plain http.
No strtok in setup. builder/setup/setup.c splits the setup list with next_line(): strtok's work without its hidden state. Checked on CRLF, blank lines, an empty list, and a list with no last newline.
SHA-1 cache keys are marked usedforsecurity=False. The digests are the same, so every build cache stays valid. It also lets the scripts run on FIPS-mode Pythons.
The AppRun chmod 0o755 now has a note on why: the launcher has to run.

The "generic API key" hits are 64-bit shader IDs and test constants, so nothing to change there.

Tested: setup.c compiles with llvm-mingw clang (no new warnings), and the Python files parse. test_linux_update/test_make_game_* give the same results as main on Windows; the Linux-only cases run in CI.

🤖 Generated with Claude Code

Follows up the security-scan triage (the real finding, the Dependabot cooldown, landed in #1464). The rest were false positives; where a small change makes the code actually safer, this makes it: - **Downloads are https-only.** `builder/linux/linux_update.py` sends every `urlopen` through `_https()`, which refuses anything else (the release API's asset link included). `scripts/release_parts.py`'s `get()` does the same, so the CI token is never sent over plain http. - **No `strtok` in setup.** `builder/setup/setup.c` splits the setup list with `next_line()`: strtok's work without its hidden state. Checked on CRLF, blank lines, an empty list, and a list with no last newline. - **SHA-1 cache keys are marked `usedforsecurity=False`.** The digests are the same, so every build cache stays valid. It also lets the scripts run on FIPS-mode Pythons. - **The AppRun `chmod 0o755`** now has a note on why: the launcher has to run. The "generic API key" hits are 64-bit shader IDs and test constants, so nothing to change there. Tested: setup.c compiles with llvm-mingw clang (no new warnings), and the Python files parse. `test_linux_update`/`test_make_game_*` give the same results as main on Windows; the Linux-only cases run in CI. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

hckrman101 (Migrated from github.com) reviewed

2026-11-10 04:18:43 +00:00

hckrman101 (Migrated from github.com) left a comment

Thanks for this. The SHA-1 usedforsecurity=False changes and the next_line() replacement for strtok look right to me (same digests, so caches stay valid, and next_line handles CRLF, blank lines and the no-trailing-newline case as the description says). One real bug though, in the new https check:

_request is broken by a stray argument (inline comment below). urllib.request.Request(_https(url), url, headers=...) passes url a second time as the positional data argument. That turns the request into a POST with a str body, and urlopen then raises TypeError: POST data should be bytes, an iterable of bytes, or a file object. It cannot be of type str. I reproduced it with Python 3.14: Request(u, u).get_method() is POST and urlopen raises. So every Linux update download through _request (the release-info fetch in _api_download and the plain-URL path) would fail. tests/test_linux_update.py doesn't exercise _request/_api_download (no urlopen or _request references), which is why nothing would catch this; a tiny test that mocks urllib.request.urlopen and checks that _request("https://...") succeeds and _request("http://...") raises ValueError would cover it (and the same for release_parts.get).

Smaller notes, none blocking:

The check validates only the URL you pass in. urllib still follows redirects, including an https to http one, so "https-only" holds for the first hop, not the whole chain. For GitHub release assets that's fine in practice (the CDN redirects stay on https), and I'd keep it simple, but a short HTTPRedirectHandler subclass that re-runs _https would make the claim exact. It's a nit if you think the description already says enough.
I checked that this doesn't break legitimate mirrors/redirects as far as I can see: the API asset URLs and https:// release URLs all pass, and nothing on Windows touches linux_update.py. release_parts.get is only called with https GitHub/llvm-mingw URLs in the workflows.
Nothing flagged is simply silenced as far as I can tell: the 0o755 chmod only gets a comment (fine, it really is the intended mode), and the other changes change behaviour.
CI is all still pending at the time of review, so I couldn't confirm the Linux test result; I haven't run the PR code.

Happy to re-check once the Request(...) call is fixed.

Thanks for this. The SHA-1 `usedforsecurity=False` changes and the `next_line()` replacement for `strtok` look right to me (same digests, so caches stay valid, and `next_line` handles CRLF, blank lines and the no-trailing-newline case as the description says). One real bug though, in the new https check: **`_request` is broken by a stray argument** (inline comment below). `urllib.request.Request(_https(url), url, headers=...)` passes `url` a second time as the positional `data` argument. That turns the request into a POST with a `str` body, and `urlopen` then raises `TypeError: POST data should be bytes, an iterable of bytes, or a file object. It cannot be of type str.` I reproduced it with Python 3.14: `Request(u, u).get_method()` is `POST` and `urlopen` raises. So every Linux update download through `_request` (the release-info fetch in `_api_download` and the plain-URL path) would fail. `tests/test_linux_update.py` doesn't exercise `_request`/`_api_download` (no `urlopen` or `_request` references), which is why nothing would catch this; a tiny test that mocks `urllib.request.urlopen` and checks that `_request("https://...")` succeeds and `_request("http://...")` raises `ValueError` would cover it (and the same for `release_parts.get`). Smaller notes, none blocking: - The check validates only the URL you pass in. urllib still follows redirects, including an https to http one, so "https-only" holds for the first hop, not the whole chain. For GitHub release assets that's fine in practice (the CDN redirects stay on https), and I'd keep it simple, but a short `HTTPRedirectHandler` subclass that re-runs `_https` would make the claim exact. It's a nit if you think the description already says enough. - I checked that this doesn't break legitimate mirrors/redirects as far as I can see: the API asset URLs and `https://` release URLs all pass, and nothing on Windows touches `linux_update.py`. `release_parts.get` is only called with https GitHub/llvm-mingw URLs in the workflows. - Nothing flagged is simply silenced as far as I can tell: the `0o755` chmod only gets a comment (fine, it really is the intended mode), and the other changes change behaviour. - CI is all still pending at the time of review, so I couldn't confirm the Linux test result; I haven't run the PR code. Happy to re-check once the `Request(...)` call is fixed.

builder/linux/linux_update.py

					
				@ -210,3 +217,3 @@

				def _request(url, timeout, limit=MAX_DOWNLOAD):

				    req = urllib.request.Request(url, headers={"User-Agent": "Hookshot", "Accept": "application/vnd.github+json"})

				    req = urllib.request.Request(_https(url), url, headers={"User-Agent": "Hookshot", "Accept": "application/vnd.github+json"})

				    with urllib.request.urlopen(req, timeout=timeout) as response:

This passes url twice: the second positional argument of Request is data, so this becomes a POST with a str body and urlopen raises TypeError. Should be urllib.request.Request(_https(url), headers={...}).

This passes `url` twice: the second positional argument of `Request` is `data`, so this becomes a POST with a str body and `urlopen` raises TypeError. Should be `urllib.request.Request(_https(url), headers={...})`.

Commenting is not possible because the repository is archived.

No reviewers