* ci: enforce strict mypy across the codebase
Add a Python mypy job that runs on every PR (`.github/workflows/python-mypy.yaml`) and
the supporting type annotations to make `strict = true` pass cleanly across all 88
source files.
Configuration choices:
* `strict = true` with no global relaxations (no `disallow_subclassing_any = false`,
no `disallow_untyped_decorators = false`, no `disable_error_code`,
no `ignore_missing_imports`).
* `follow_imports = "normal"`.
* django-stubs + djangorestframework-stubs plugins; django_stubs_ext.monkeypatch()
in settings so generic Django classes are subscriptable at runtime.
* Local stubs in `stubs/` for libraries that ship incomplete type info
(drf_spectacular's view methods, redis-py's sync API).
* A scoped `[[tool.mypy.overrides]]` block lists 7 third-party libs without any
type info (cec, geventwebsocket, hurry.filesize, pydbus, sh, splinter, vlc) so
future stub releases will be noticed instead of silently ignored.
The two `# type: ignore` escape hatches that previously existed are gone:
`lib/utils.py` now imports `mplayer` from `sh` properly, and `tests/test_settings.py`
patches `os.getenv` via `mock.patch.object` instead of direct assignment.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ci: fix mypy CI runtime deps and unused HttpResponse import
* `lib/auth.py`: drop the local `HttpResponse` import; only the string-form
annotation needs it and the runtime `isinstance` check only uses `HttpRequest`
(caught by ruff's F401).
* Add a `mypy` dependency group (extends `dev-host` with the runtime deps that
`anthias_django.settings` touches) so the django-stubs plugin can introspect
the app registry on a fresh CI runner. Skips the heavy native-extension deps
(cec, netifaces, etc.) that aren't needed for type checking.
* python-mypy workflow: install via `uv pip install --group mypy` (consistent
with other workflows) and create a dummy `~/.screenly/screenly.conf` so the
settings module's first-import side effect succeeds.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(security): migrate password hashing to PBKDF2 (CodeQL py/weak-sensitive-data-hashing)
CodeQL flagged the SHA256-based password hashing in lib/auth.py and api/views/v2.py
as a weak password hash (SHA256 is a fast hash, unsuitable for password storage).
Switch new passwords to Django's `make_password` (PBKDF2-SHA256, 600k iterations
by default in Django 4.2). Add `hash_password` / `verify_password` helpers in
lib/auth.py that:
* hash with PBKDF2 for any new password write,
* verify both Django-format hashes and legacy bare-SHA256 hex digests so existing
config files still authenticate,
* opportunistically re-hash legacy SHA256 entries to PBKDF2 on a successful
login, phasing out the weak format over time.
`settings.py` auto-migration now uses `hash_password` for first-load plaintext
passwords and recognises both legacy SHA256 and Django algorithm-prefixed
formats so it doesn't double-hash an already-stored hash.
Also fixes the stale ruff format check (5 files reformatted by `ruff format`)
that was breaking the python-lint job.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: drop legacy SHA256 password verify and add docker-image-builder to mypy group
CodeQL kept flagging the SHA256 fallback path in `lib/auth.py:verify_password`
as `py/weak-sensitive-data-hashing`, even though it was scoped to a one-time
migration. Rather than suppress the alert, drop the legacy verify entirely so
no password material ever flows into hashlib.sha256.
Migration of existing installs now happens at settings load time:
* `settings.py` detects a 64-char hex (legacy SHA256) password hash on read
and clears both the `password` field and `auth_backend`. The device stays
reachable (no auth required) and the operator must re-set credentials via
the web UI. A clear warning is logged so the change is visible.
Also fix the python-mypy CI job: include the `docker-image-builder` group in
the `mypy` group so `pygit2` and `python_on_whales` (imported by
`tools/image_builder/__main__.py`) resolve.
BREAKING: any Anthias install with a SHA256-format password in
`screenly.conf` will have basic auth disabled on first start of this version.
The operator must log in (no password required) and set a new password via
the UI to re-enable basic auth.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: address review feedback (Copilot + manual review)
Critical:
* settings.py — `_get` previously called `hash_password()` at module-import
time when it found a plaintext password. That imports Django's password
hashers, which raises `ImproperlyConfigured` if reached before
`django.setup()` (e.g. when `viewer/__init__.py` imports `settings`).
Drop the auto-hash entirely and treat plaintext the same as legacy
SHA256: clear it, disable basic auth, log an `error`-level warning, and
persist the cleaned state via `_needs_save_after_load` so the warning
doesn't repeat on every load.
* lib/backup_helper.py — `recover()` called `tar.extractall()` on an
uploaded archive. A crafted backup with `../` paths or symlinks could
overwrite arbitrary files under HOME. Add `_safe_extract()` that
validates each member's resolved path stays inside the destination and
rejects unsafe symlinks/hardlinks.
* api/helpers.py — `custom_exception_handler()` discarded DRF's default
response and always returned 500, breaking 4xx propagation for
`ValidationError`/`NotFound`/etc. Return DRF's response when present;
fall back to 500 only when it's None.
* tests/test_settings.py — `broken_settings_should_raise_value_error`
was missing the `test_` prefix and never actually ran. Renamed.
API correctness:
* api/views/mixins.py — replace bare `raise Exception(...)` for missing
uploads / wrong file extensions / missing asset URI with DRF's
`ValidationError` / `NotFound`, so callers get proper 4xx with a
structured error body instead of a 500.
* anthias_app/helpers.py, api/serializers/{mixins,v1_1}.py — replace
`assert video_duration is not None` with explicit raises. `assert` is
stripped under `python -O`, which would silently turn the next call
into an `AttributeError` on None.
Typing/stubs:
* host_agent.py — pass `decode_responses=True` to both `redis.Redis()`
constructions and switch the channel name + command map keys from
bytes to str. The local redis stub assumes decoded responses, and
host_agent was the only caller violating that invariant.
* lib/auth.py — document the `R | HttpResponse` return-type collapse
for DRF Response (mirrors Django's @login_required).
* api/serializers/__init__.py — comment why `UpdateAssetSerializer`'s
fields are widened to `Field[Any, Any, Any, Any]` (so v2's overrides
with different field types don't trip [assignment]).
Tests/CI:
* api/tests/test_v2_endpoints.py — also assert the stored hash starts
with `pbkdf2_sha256$` so a regression to a weaker hasher is caught
even if `verify_password()` itself were broken.
* .github/workflows/python-mypy.yaml — write a minimal valid
`screenly.conf` (with section headers) instead of an empty file. The
empty-file path only worked by accident of `_get`'s defensive
try/except.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(test): force re-import of `settings` in fake_settings()
The `fake_settings()` test helper only deleted `sys.modules['settings']`
*after* the yield (i.e. on clean exit). Once a prior test imported
`settings` cleanly, subsequent `import settings` calls returned the cache
without re-instantiating `AnthiasSettings`, so the fixture's config file
was silently ignored.
This was hidden because `test_broken_settings_should_raise_value_error`
was missing the `test_` prefix and never ran. After renaming it in the
previous commit it now runs and exposes the bug.
Pop the module before import (and again in `finally`) so each test gets
a fresh `AnthiasSettings()` instance bound to the fixture file.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: address Copilot review feedback
- backup_helper: use dedicated BackupRecoverError instead of bare
Exception so callers can map archive failures to 4xx responses.
- mixins: catch BackupRecoverError / TarError in RecoverViewMixin and
re-raise as DRF ValidationError so bad uploads return 400, not 500.
- utils: drop top-level `from sh import ffprobe, mplayer`; resolve
binaries lazily with sh.Command at call sites so a missing tool
doesn't break module import.
- host_agent: import redis ConnectionError explicitly (mypy strict).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(ci): unblock mypy and ruff format; sanitize recover error
- stubs/redis-stubs: export ConnectionError so host_agent's
retry_if_exception_type(redis.ConnectionError) typechecks under the
isolated mypy CI env (no real redis package installed).
- host_agent.py: switch to redis.ConnectionError (matches new stub).
- lib/utils.py: ruff format fix for the lazy sh.Command(...) call.
- api/views/mixins.py: don't echo backup-recover exception text into
the API response (CodeQL py/stack-trace-exposure). Log server-side
and return a generic 'Invalid backup archive.' message instead.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(security): generate server-side name for backup recovery upload
Don't pass the client-supplied `file_upload.name` into `path.join('static', ...)`.
A crafted name (path separators, leading `../`, or an absolute path) could
write outside the static directory before the tarball is parsed. Use a
UUID-named `.tar.gz` instead — the original filename is never needed
again after the content-type check.
Addresses suppressed Copilot review comment on api/views/mixins.py.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: address Copilot review round 2
- lib/utils.py: catch sh.CommandNotFound in get_video_duration (return
None) and url_fails (skip streaming probe) so missing ffprobe/mplayer
don't surface as 500s.
- lib/auth.py: decode basic-auth credentials with partition(':') so
passwords containing ':' are accepted (RFC 7617).
- lib/backup_helper.py: tighten _safe_extract — reject non-regular tar
members (symlinks, hardlinks, device nodes, FIFOs) and extract members
individually after validation instead of calling extractall().
- tests/test_backup_helper.py: add coverage for path traversal,
absolute-path, symlink, and FIFO rejection in _safe_extract.
- settings.py: fail fast in AnthiasSettings.__init__ when HOME is unset
instead of silently rooting all config/state paths at the cwd.
- api/serializers/v1_1.py: raise ValidationError when 'duration' is
missing/invalid for non-video assets instead of an unhandled KeyError.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(test): mark tar fixture/extract calls as NOSONAR
The new SafeExtractTest builds and opens single-member tar archives
specifically to test the safe-extract guard. SonarCloud flags any
tarfile.open(...) call (rule python:S5042). Annotate the two test-
fixture call sites with NOSONAR — same pattern used elsewhere in this
repo (e.g. host_agent.py).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* style(ci): use 6-space step indentation in python-mypy.yaml
Match the majority style used by the other workflows in this repo.
The previous 4-space indent under `steps:` was valid YAML and ran
fine in CI, but consistency with build-webview.yaml,
build-balena-disk-image.yaml, deploy-website.yaml, etc. makes the
file easier to read alongside the rest.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: address Copilot review round 3
- api/serializers/v1_1.py: raise DRF ValidationError instead of
ValueError when video duration can't be determined, so clients get
a 4xx with a field-level error rather than a 500.
- api/views/mixins.py: ensure the uploaded backup tarball is removed
in all paths (success and failure). recover() only deletes on
success; without explicit cleanup, rejected archives — including
attacker-controlled ones — accumulate under static/.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* style: ruff format tests/test_backup_helper.py
Single line that fit within 79 chars but I had broken across three
lines during the merge resolution.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: address Copilot review round 4
* Add partial PEP 561 stubs in `stubs/channels-stubs/` covering the
`channels` API surface we actually use: `AsyncWebsocketConsumer`
(with `as_asgi`), `ProtocolTypeRouter`, `URLRouter`,
`AllowedHostsOriginValidator`, and `get_channel_layer`. Drop the
`# type: ignore[misc]` from `AssetConsumer` and remove `channels.*`
from the mypy `ignore_missing_imports` overrides — the override
block is back to listing only libs that genuinely ship no type info.
* Delete `_safe_extract` and `_is_within_directory` from
`lib/backup_helper.py`, plus the corresponding `SafeExtractTest` and
`_build_archive_with` from `tests/test_backup_helper.py`. After the
master merge, `recover()` validates each member through
`_safe_tar_member` (skip-on-fail) and extracts inside the loop, so
the older raise-on-fail helper became dead code with no caller.
`RecoverLegacyTarballTest::test_recover_skips_path_traversal_member`
already exercises the path-traversal guard end-to-end.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: address Copilot review round 5
* `celery_tasks.py:cleanup()` — bail out with a logged error when HOME
is unset instead of `path.join('', 'anthias_assets')` resolving to a
relative path. The `find ... -delete` was never going to run on a
Pi without HOME, but the silent fallback would have chewed through
the celery worker's cwd if it ever did.
* `api/serializers/v1_1.py:CreateAssetSerializerV1_1.prepare_asset()` —
fix the video duration handling so non-zero and omitted durations
are no longer dropped on the floor. Previously only the magic
`duration == 0` case set `asset['duration']`; a client posting
`duration=30` for a video produced a serialized dict with no
duration key, then the view did `Asset.objects.create(**data)` and
the row got the field's default. Now: missing or 0 means "infer
from the file via get_video_duration()"; any other integer is
persisted as-is. Non-int values raise ValidationError up front.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This change improves the error handling when interacting with the github
API by enforcing a 5 minute backoff on API call failures as well as
caches the remote branch presence/absence for 24 hours.
This prevents potential crashes when Internet connectivity is
unavailable, as well as from having one's IP address rate limited by Github
if the remote branch on the pi references a branch name that does not exist
under https://github.com/Screenly/screenly-ose/tree/
Prior to this change, this scenario causes the remote branch status to be
checked during every UI request.
Signed-off-by: Nash Kaminski <nashkaminski@kaminski.io>
