CodeQL: a small cluster of real quality findings, and the guards it cannot see #249

New issue

Open

opened 2026-09-16 13:48:39 +00:00 by tiagoagueda · 1 comment

tiagoagueda commented

2026-09-16 13:48:39 +00:00

Owner

Run on 2026-09-16 with the CodeQL CLI 2.27.0 (the toolchain the VS Code extension ships), pack codeql/python-queries@1.8.10, suite python-security-and-quality, over main at 23c742c7f. 131 results, 354 source files.

No security vulnerability survived review. All 12 error-severity and all 4
warning-severity findings are false positives. The reasons are recorded here so the next run
is not triaged from scratch:

Rule	Where	Why it does not hold
`py/full-ssrf` (9.1)	`jobs/logos.py:112`	The fetch goes through `http.public_only_client`, which checks every hop and connects to the address it checked (#215). CodeQL does not model that client.
`py/url-redirection` (6.1)	`core/arranging.py:121`	`mode_url()` always starts from `reverse("core:home")`, and `key` is refused unless it is in `widgets.REGISTRY`.
`py/log-injection` (6.1) x3	`plugins/registry.py:204,211,219`	Every interpolation is `%r`, and `repr()` escapes newlines.
`py/log-injection` (6.1)	`core/server_views.py:957`	Same, and a username must match `^[a-z0-9][a-z0-9._-]{1,30}[a-z0-9]$`, which admits no newline.
`py/stack-trace-exposure` (5.4) x2	`core/views_logs.py:93`, `core/views_metrics.py:54`	The value is `str(throttle.TooOften)` — a "try again in N seconds" line, not a trace. Both endpoints are token-guarded.
`py/incomplete-url-substring-sanitization` (7.8)	`tests/test_image_build.py:181`	`runtime` is Dockerfile text, not a URL.
`py/catch-base-exception`	`plugins/registry.py:197`	Deliberate and documented: a plugin's `sys.exit` at import time must not end the worker (#228), and `KeyboardInterrupt` is re-raised just above.
`py/ineffectual-statement` x18	`plugins/base.py`, `documents/pdf.py`, `core/channels.py`, `notifications/base.py`	All are `...` in `Protocol` method stubs.
`py/unreachable-statement`, `py/uninitialized-local-variable`	`tests/test_document_record.py:188`, `tests/test_documents.py:380`	CodeQL models neither `pytest.raises` nor `pytest.skip`.

What is worth doing

All small, and none of it urgent.

An assertion that does the work it is checking. tests/test_mail_destinations.py:299
and tests/test_mail_xoauth2.py:208 read assert backend.open() is False and
assert backend.open() is True. Under python -O the assert is stripped and the call
goes with it, so the test would pass without ever opening anything. Bind first, then
assert the name.
An unencoded value in a redirect. resume/views.py:187 builds
f"...?language={language}", and translating.normalise() only strips, lowercases and
swaps _ for - — it never checks the code against a known language. The host is fixed
by reverse(), so this is not an open redirect and is not a security finding; it is a
reflected value that should go through urlencode before it reaches a query string.
An overridable method called from __init__. jobs/forms.py:34:
OwnerScopedModelForm.__init__ calls self.scope_querysets(), which ApplicationForm,
ReminderForm and two others override. The override runs while the subclass's own
__init__ is still unfinished. It works today only because the one thing it needs,
self.user, is assigned before super().__init__() — which is a fact about the current
subclasses, not a promise the base class makes.
A response nobody looks at. tests/test_email_settings.py:362 assigns response from
a client.post and never reads it; the assertion below checks SiteSettings instead.
Either an assertion about the response is missing or the binding should go.
Three silent except: pass. core/csv_import.py:484 (falls through to a delimiter
heuristic), accounts/deletion.py:134 and :139 (a directory that will not remove).
All three are deliberate and all three want one line saying so. core/logs.py:235 is
flagged by the same rule but explains itself in its docstring already.
Trivia. ids=lambda value: str(value) is ids=str
(tests/test_slow_requests.py:62,67); a module imported twice in tests/test_brand.py:70,
tests/test_navigation.py:62 and tests/test_phones.py:220; import x beside
from x import y in accounts/migrations/0003_username_and_verified_addresses.py:12 and
tests/security/test_admin_exposure.py:36; and the adjacent byte literals at
seed_demo.py:234 build one PDF object deliberately but read exactly like a missing comma.

from .settings import * in config/settings/{dev,prod,test}.py is flagged as
py/polluting-import. That is how Django settings are layered — leave it.

The 79 import cycles are their own matter: #248.

Run on 2026-09-16 with the CodeQL CLI 2.27.0 (the toolchain the VS Code extension ships), pack `codeql/python-queries@1.8.10`, suite `python-security-and-quality`, over `main` at `23c742c7f`. 131 results, 354 source files. **No security vulnerability survived review.** All 12 error-severity and all 4 warning-severity findings are false positives. The reasons are recorded here so the next run is not triaged from scratch: | Rule | Where | Why it does not hold | | --- | --- | --- | | `py/full-ssrf` (9.1) | `jobs/logos.py:112` | The fetch goes through `http.public_only_client`, which checks every hop and connects to the address it checked (#215). CodeQL does not model that client. | | `py/url-redirection` (6.1) | `core/arranging.py:121` | `mode_url()` always starts from `reverse("core:home")`, and `key` is refused unless it is in `widgets.REGISTRY`. | | `py/log-injection` (6.1) x3 | `plugins/registry.py:204,211,219` | Every interpolation is `%r`, and `repr()` escapes newlines. | | `py/log-injection` (6.1) | `core/server_views.py:957` | Same, and a username must match `^[a-z0-9][a-z0-9._-]{1,30}[a-z0-9]$`, which admits no newline. | | `py/stack-trace-exposure` (5.4) x2 | `core/views_logs.py:93`, `core/views_metrics.py:54` | The value is `str(throttle.TooOften)` — a "try again in N seconds" line, not a trace. Both endpoints are token-guarded. | | `py/incomplete-url-substring-sanitization` (7.8) | `tests/test_image_build.py:181` | `runtime` is Dockerfile text, not a URL. | | `py/catch-base-exception` | `plugins/registry.py:197` | Deliberate and documented: a plugin's `sys.exit` at import time must not end the worker (#228), and `KeyboardInterrupt` is re-raised just above. | | `py/ineffectual-statement` x18 | `plugins/base.py`, `documents/pdf.py`, `core/channels.py`, `notifications/base.py` | All are `...` in `Protocol` method stubs. | | `py/unreachable-statement`, `py/uninitialized-local-variable` | `tests/test_document_record.py:188`, `tests/test_documents.py:380` | CodeQL models neither `pytest.raises` nor `pytest.skip`. | ### What is worth doing All small, and none of it urgent. 1. **An assertion that does the work it is checking.** `tests/test_mail_destinations.py:299` and `tests/test_mail_xoauth2.py:208` read `assert backend.open() is False` and `assert backend.open() is True`. Under `python -O` the assert is stripped and the call goes with it, so the test would pass without ever opening anything. Bind first, then assert the name. 2. **An unencoded value in a redirect.** `resume/views.py:187` builds `f"...?language={language}"`, and `translating.normalise()` only strips, lowercases and swaps `_` for `-` — it never checks the code against a known language. The host is fixed by `reverse()`, so this is not an open redirect and is not a security finding; it is a reflected value that should go through `urlencode` before it reaches a query string. 3. **An overridable method called from `__init__`.** `jobs/forms.py:34`: `OwnerScopedModelForm.__init__` calls `self.scope_querysets()`, which `ApplicationForm`, `ReminderForm` and two others override. The override runs while the subclass's own `__init__` is still unfinished. It works today only because the one thing it needs, `self.user`, is assigned before `super().__init__()` — which is a fact about the current subclasses, not a promise the base class makes. 4. **A response nobody looks at.** `tests/test_email_settings.py:362` assigns `response` from a `client.post` and never reads it; the assertion below checks `SiteSettings` instead. Either an assertion about the response is missing or the binding should go. 5. **Three silent `except: pass`.** `core/csv_import.py:484` (falls through to a delimiter heuristic), `accounts/deletion.py:134` and `:139` (a directory that will not remove). All three are deliberate and all three want one line saying so. `core/logs.py:235` is flagged by the same rule but explains itself in its docstring already. 6. **Trivia.** `ids=lambda value: str(value)` is `ids=str` (`tests/test_slow_requests.py:62,67`); a module imported twice in `tests/test_brand.py:70`, `tests/test_navigation.py:62` and `tests/test_phones.py:220`; `import x` beside `from x import y` in `accounts/migrations/0003_username_and_verified_addresses.py:12` and `tests/security/test_admin_exposure.py:36`; and the adjacent byte literals at `seed_demo.py:234` build one PDF object deliberately but read exactly like a missing comma. `from .settings import *` in `config/settings/{dev,prod,test}.py` is flagged as `py/polluting-import`. That is how Django settings are layered — leave it. The 79 import cycles are their own matter: #248.

tiagoagueda added the

tier

enhancement

labels

2026-09-16 13:48:39 +00:00

tiagoagueda referenced this issue

2026-09-16 13:48:58 +00:00

CI and operations tooling: 5,800 tests run serially three times, unpinned tools, releases not gated on CI, no config checks or request ids #233

tiagoagueda commented

2026-09-16 13:58:54 +00:00

Author

Owner

`python-security-experimental` as well, 2026-09-16 — nothing new to fix

Same database, same commit (23c742c7f), suite python-security-experimental (79 queries,
27 of them not in python-security-and-quality). 75 results, none of them real, so no
issue was opened for it. Recorded here so the next run is not triaged from scratch.

The 27 new queries all ran and all returned nothing. Confirmed by their presence in the
SARIF rule table with zero results, not by their absence: py/zipslip, py/tarslip,
py/tarslip-extended, py/unsafe-unpacking, py/csv-injection, py/decompression-bomb,
py/unicode-dos, py/unicode-bypass-validation, py/insecure-randomness,
py/jwt-empty-secret-or-algorithm, py/jwt-missing-verification,
py/cors-misconfiguration-with-credentials, py/prompt-injection, py/xslt-injection,
py/js2py-rce, py/improper-ldap-auth, py/insecure-ldap-auth, py/ldap-injection.

Two of those deserve a note:

The archive queries are the ones that mattered here, because the plugin installer
(#228, #246) and backup restore (#234, #242) both take an archive from an operator. They
are clean, and the code says why: backup.py::_safe_member_path refuses any member that
is absolute or contains .., refuses anything that is not a plain file, and checks every
member before writing any of them. installing.py::read_wheel and importer.py::_extract
only ever read() into memory — neither extracts to disk.
py/flask-constant-secret-key is Flask-only and cannot see a Django SECRET_KEY, so its
silence is not evidence about #234's key handling.

The other 65 results are two timing-attack queries, and all 65 are false positives.

53 are in tests/, comparing values inside assertions.
Four compare a checksum, not a secret: backup.py:453 and catalogue.py:313 verify an
archive against a digest the same party supplied, and idempotency.py:90 compares request
fingerprints. A timing leak tells an attacker something they already hold.
plugins/forms.py:152, server_forms.py:301, provenance.py:161 and keys.py:83 are
flagged for in and dict.get against field names and a placeholder list, not secrets.
server_views.py:1181 (token != pending.get("token")) is the closest to real and is
still not: the token it compares against lives in the caller's own session, so the only
token anybody can time is one they already know. The equality check also has to pass before
token reaches f"{token}.whl", which is what keeps that path safe.

The bearer-token endpoints that would have been the real finding already use
hmac.compare_digest — views_logs.py:63 and views_metrics.py:34.

Conclusion for #233's nightly: python-security-experimental costs a full extra run and
found nothing in two attempts. Worth repeating only when the archive, token or crypto paths
change; python-security-and-quality is the suite to schedule. If it is ever run in CI,
py/possible-timing-attack-sensitive-info and py/possible-timing-attack-against-hash need
excluding or they are 65 results of noise on their own.

### `python-security-experimental` as well, 2026-09-16 — nothing new to fix Same database, same commit (`23c742c7f`), suite `python-security-experimental` (79 queries, 27 of them not in `python-security-and-quality`). **75 results, none of them real**, so no issue was opened for it. Recorded here so the next run is not triaged from scratch. **The 27 new queries all ran and all returned nothing.** Confirmed by their presence in the SARIF rule table with zero results, not by their absence: `py/zipslip`, `py/tarslip`, `py/tarslip-extended`, `py/unsafe-unpacking`, `py/csv-injection`, `py/decompression-bomb`, `py/unicode-dos`, `py/unicode-bypass-validation`, `py/insecure-randomness`, `py/jwt-empty-secret-or-algorithm`, `py/jwt-missing-verification`, `py/cors-misconfiguration-with-credentials`, `py/prompt-injection`, `py/xslt-injection`, `py/js2py-rce`, `py/improper-ldap-auth`, `py/insecure-ldap-auth`, `py/ldap-injection`. Two of those deserve a note: - The **archive queries are the ones that mattered** here, because the plugin installer (#228, #246) and backup restore (#234, #242) both take an archive from an operator. They are clean, and the code says why: `backup.py::_safe_member_path` refuses any member that is absolute or contains `..`, refuses anything that is not a plain file, and checks every member before writing any of them. `installing.py::read_wheel` and `importer.py::_extract` only ever `read()` into memory — neither extracts to disk. - `py/flask-constant-secret-key` is Flask-only and cannot see a Django `SECRET_KEY`, so its silence is not evidence about #234's key handling. **The other 65 results are two timing-attack queries, and all 65 are false positives.** - 53 are in `tests/`, comparing values inside assertions. - Four compare a **checksum, not a secret**: `backup.py:453` and `catalogue.py:313` verify an archive against a digest the same party supplied, and `idempotency.py:90` compares request fingerprints. A timing leak tells an attacker something they already hold. - `plugins/forms.py:152`, `server_forms.py:301`, `provenance.py:161` and `keys.py:83` are flagged for `in` and `dict.get` against **field names and a placeholder list**, not secrets. - `server_views.py:1181` (`token != pending.get("token")`) is the closest to real and is still not: the token it compares against lives in the caller's **own session**, so the only token anybody can time is one they already know. The equality check also has to pass before `token` reaches `f"{token}.whl"`, which is what keeps that path safe. The bearer-token endpoints that would have been the real finding already use `hmac.compare_digest` — `views_logs.py:63` and `views_metrics.py:34`. **Conclusion for #233's nightly:** `python-security-experimental` costs a full extra run and found nothing in two attempts. Worth repeating only when the archive, token or crypto paths change; `python-security-and-quality` is the suite to schedule. If it is ever run in CI, `py/possible-timing-attack-sensitive-info` and `py/possible-timing-attack-against-hash` need excluding or they are 65 results of noise on their own.