Postulo/postulo

Watch

Fork 0

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

New issue

Closed

opened 2026-09-16 13:48:39 +00:00 by tiagoagueda · 2 comments

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.

tiagoagueda added this to the 0.5.0 milestone

2026-09-29 18:38:03 +00:00

tiagoagueda referenced this issue

2026-09-29 19:45:59 +00:00

Fetching a page or a logo: a capture address can forge a log line, NAT64 addresses pass the public-only check, and the size caps apply after the whole body is read #321

tiagoagueda commented

2026-09-29 19:46:00 +00:00

Author

Owner

Re-checked on 2026-09-29 against main at f097f1591. Nothing in this issue is a security problem worth fixing, so it is closed.

How it was checked

Two independent reviews. One traced where each value comes from. The other tried to exploit each dismissed finding, with probe scripts run against today's code.
A fresh CodeQL run. Same CLI 2.27.0, pack codeql/python-queries@1.8.10, suite python-security-and-quality, over the 701 tracked Python files. It gave 276 results: 200 are py/cyclic-import (#248) and 18 are the Protocol stubs.

The table above

Every row still does not hold. Three of the reasons are wrong as written, although the conclusion survives on another guard:

Row	Why it still does not hold
`plugins/registry.py` log-injection ×3	CodeQL now traces `kind`, which is logged with `%s`, not `%r`. It is safe because the only value that comes from a request is refused unless it is in `CONNECTED_KINDS` (`plugins/views.py:44`), and `plugins()` raises unless it is in `GROUPS` (`registry.py:276`).
`core/server_views.py:957`	Now `:1148-1154`. It logs usernames with `%s`, and the pattern's `$` accepts a trailing newline. Every web path strips and validates the username, and `User.save()` strips its ends. The one path without the validator, the importer, belongs to the operator (point 4 of #321).
The two 429 bodies	Still a fixed rate sentence, not a trace. `/metrics` is open, not token-guarded, when no token is set; that is by design, and it leaks nothing.

Eight new security results, all false positives

Result	Why it is not a problem
`resume/views.py:136`	A third `str(TooOften)` in a 429 body (#266).
`accounts/settings_views.py:85`, `:98`	Redirects to `request.path` (#299). The resolver pins the path, and the moved key only reaches the fragment.
`resume/views.py:241`	Point 2 of the list above. Tested with `//evil.example`, `\evil.example` and CR/LF: `Location` stays on the same path, with the value in the query string, percent-encoded.
`documents/docx.py:67`	An XML-character regex. CodeQL misreads its astral-plane escapes; the ranges are disjoint and tested.
Three in tests	`test_merging.py:508`, and `test_page_script.py:181` twice: fixture values, never served.

Found beside it, filed as #321

Attacking the same code turned up four problems that CodeQL does not see:

A capture address sent through the API can forge a console log line (low severity; the exploit was shown).
The public-only check lets NAT64 and IPv4-compatible IPv6 addresses through.
The size caps on page and logo fetches run only after the whole body is in memory.
The importer does not validate usernames.

Not done, and closed with this issue

The "What is worth doing" list is all quality work. Both reviews found that none of it has a security angle, and all of it is still open at f097f1591:

Assertions that do the work they check: the two named above, and two new ones at test_candidate_file.py:151 and test_document_formats.py:312.
Unencoded language code: resume/views.py:241.
Overridable method called from __init__: jobs/forms.py:83. All 13 subclasses were checked, and a future mistake would fail closed.
Unread response: in test_email_settings.py.
The three except: pass: accounts/deletion.py:141-150 only removes empty directories. Separately, :124-125 reports a file that exists but could not be deleted as "missing", which makes the erasure report wrong but is not an exploit.
The trivia.

Reopen this issue, or file a new one, if any of it is wanted.

Re-checked on 2026-09-29 against `main` at `f097f1591`. **Nothing in this issue is a security problem worth fixing**, so it is closed. ## How it was checked - **Two independent reviews.** One traced where each value comes from. The other tried to exploit each dismissed finding, with probe scripts run against today's code. - **A fresh CodeQL run.** Same CLI 2.27.0, pack `codeql/python-queries@1.8.10`, suite `python-security-and-quality`, over the 701 tracked Python files. It gave 276 results: 200 are `py/cyclic-import` (#248) and 18 are the `Protocol` stubs. ## The table above Every row still does not hold. Three of the reasons are wrong as written, although the conclusion survives on another guard: | Row | Why it still does not hold | | --- | --- | | `plugins/registry.py` log-injection ×3 | CodeQL now traces `kind`, which is logged with `%s`, not `%r`. It is safe because the only value that comes from a request is refused unless it is in `CONNECTED_KINDS` (`plugins/views.py:44`), and `plugins()` raises unless it is in `GROUPS` (`registry.py:276`). | | `core/server_views.py:957` | Now `:1148-1154`. It logs usernames with `%s`, and the pattern's `$` accepts a trailing newline. Every web path strips and validates the username, and `User.save()` strips its ends. The one path without the validator, the importer, belongs to the operator (point 4 of #321). | | The two 429 bodies | Still a fixed rate sentence, not a trace. `/metrics` is open, not token-guarded, when no token is set; that is by design, and it leaks nothing. | ## Eight new security results, all false positives | Result | Why it is not a problem | | --- | --- | | `resume/views.py:136` | A third `str(TooOften)` in a 429 body (#266). | | `accounts/settings_views.py:85`, `:98` | Redirects to `request.path` (#299). The resolver pins the path, and the moved key only reaches the fragment. | | `resume/views.py:241` | Point 2 of the list above. Tested with `//evil.example`, `\evil.example` and CR/LF: `Location` stays on the same path, with the value in the query string, percent-encoded. | | `documents/docx.py:67` | An XML-character regex. CodeQL misreads its astral-plane escapes; the ranges are disjoint and tested. | | Three in tests | `test_merging.py:508`, and `test_page_script.py:181` twice: fixture values, never served. | ## Found beside it, filed as #321 Attacking the same code turned up four problems that CodeQL does not see: 1. A capture address sent through the API can forge a console log line (low severity; the exploit was shown). 2. The public-only check lets NAT64 and IPv4-compatible IPv6 addresses through. 3. The size caps on page and logo fetches run only after the whole body is in memory. 4. The importer does not validate usernames. ## Not done, and closed with this issue The "What is worth doing" list is all quality work. Both reviews found that none of it has a security angle, and all of it is still open at `f097f1591`: - **Assertions that do the work they check:** the two named above, and two new ones at `test_candidate_file.py:151` and `test_document_formats.py:312`. - **Unencoded language code:** `resume/views.py:241`. - **Overridable method called from `__init__`:** `jobs/forms.py:83`. All 13 subclasses were checked, and a future mistake would fail closed. - **Unread response:** in `test_email_settings.py`. - **The three `except: pass`:** `accounts/deletion.py:141-150` only removes empty directories. Separately, `:124-125` reports a file that exists but could not be deleted as "missing", which makes the erasure report wrong but is not an exploit. - **The trivia.** Reopen this issue, or file a new one, if any of it is wanted.

tiagoagueda closed this issue

2026-09-29 19:46:01 +00:00

tiagoagueda referenced this issue from a commit

2026-10-01 13:47:40 +00:00

Harden fetching a page or a logo, and the username an archive brings