Repository navigation
fix: harden admin auth, upstream ingest, estimates swap and geocoding - #197
Conversation
- /admin/* requires an operator token from PC2NUTS_ADMIN_TOKENS; rate-limit bypass tokens get 403. Non-ASCII bearer tokens get 401, not 500. - Malformed upstream ZIPs (over-long CSV field, encrypted/unsupported member) are skipped and uncached instead of aborting startup; unexpected loader errors still reach the stale-cache fallback. - An incomplete rebuild keeps the previous snapshot's missing countries, is served as stale and retried after PC2NUTS_DB_PARTIAL_RETRY_HOURS instead of replacing the cache for the full TTL. Per-writer temp file for the DB save. - Runtime estimates refresh publishes the new table with a single rebind, so concurrent /lookup and /resolve readers never see it mid-mutation. - NUTS polygon archive: ZIP-validated, written atomically, uncompressed size capped at 256 MiB, corrupt cache replaced. - Photon geocoding: total per-call budget and a per-worker concurrency cap. - CI: packages:write only on the publish job; no persisted checkout token in the gate jobs.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd2de11970
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except _UPSTREAM_PARSE_ERRORS as exc: | ||
| # Drop the cached copy so a restart re-downloads instead of failing on | ||
| # the same file for the rest of the cache TTL. | ||
| logger.warning("Unusable ZIP from %s (%s); discarding cached copy", url, exc) | ||
| cached.unlink(missing_ok=True) | ||
| return 0 |
There was a problem hiding this comment.
If a CSV contains valid rows before an overlong field, _parse_csv_content() has already inserted those rows into _lookup when this handler returns zero. _carry_forward_missing_countries() then sees the country as present and does not restore its remaining rows from the previous snapshot. The rebuild can consequently overwrite a complete cache with a truncated country, potentially marked fresh for the full TTL. Parse into a temporary table or roll back the rejected file's changes before continuing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 43b6c01, with a regression test in tests/test_loader_resilience.py (TestCodexReviewRegressions).
| for cc, pc, nuts3 in rows: | ||
| if cc not in have: | ||
| _lookup[(cc, pc)] = nuts3 | ||
| missing.add(cc) |
There was a problem hiding this comment.
When an operator disables NSPL by clearing PC2NUTS_NSPL_URL, _db_is_valid() correctly invalidates the old cache because its configuration hash changed, but this unconditional carry-forward restores the old UK rows after the otherwise successful rebuild. The same happens when removing an extra source that supplied an entire country. Those mappings remain available indefinitely, and every retry restores them again and marks the service stale. Distinguish failed sources from intentionally removed sources before retaining their countries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 43b6c01, with a regression test in tests/test_loader_resilience.py (TestCodexReviewRegressions).
…onfigured sources A file rejected half-way no longer leaves rows that hide its country from the incomplete-rebuild carry-forward. Countries whose source was removed from the configuration (NSPL unset, extra source dropped, country delisted) are not resurrected from the previous snapshot.
Fixes from a security review of the service. There are no critical, high or medium issues; this addresses four confirmed low-severity defects, plus the leads that can be fixed in source.
/admin/*now requires a token from the newPC2NUTS_ADMIN_TOKENS(at least 32 characters each). Rate-limit bypass tokens, from the env var or the token DB, get 403 there. SetPC2NUTS_ADMIN_TOKENSon the production deployment before rolling out if anything calls/admin/refresh-estimatesor/admin/memory.Fixes
PC2NUTS_DB_PARTIAL_RETRY_HOURS(default 24). It is not retried on every restart, so a file that is permanently broken upstream does not slow every boot./resolvegeocoding: one time budget covers all fallback queries (PC2NUTS_PHOTON_TOTAL_TIMEOUT_SECONDS, default 10). Each worker runs at mostPC2NUTS_PHOTON_MAX_CONCURRENCYgeocodes at once (default 16); past that,/resolveanswers postal-only instead of tying up the threadpool/lookupneeds.packages: writeis granted only topublish, and the gate jobs check out withpersist-credentials: false.Not in this PR
PC2NUTS_FORWARDED_ALLOW_IPS) and needs no code change.Tests
570 pass locally on Python 3.12. ruff, ruff format and bandit are clean. Each new regression test fails on
mainand passes on this branch:tests/test_loader_resilience.py