Repository navigation
fix(geoip): separate IPv6 size limit, accept unknown codes, gate live verification before publish - #118
Conversation
… verification before publish Fixes #116, closes #117 - Per-file GeoIP size limits: geoip 16 MiB, geoip6 32 MiB; legacy TORPROXY_GEOIP_MAX_BYTES still caps both unless TORPROXY_GEOIP6_MAX_BYTES overrides the IPv6 limit. - Accept upstream '??' unknown-origin country codes in the GeoIP payload validator. - Record auto-update outcome to geoip-status.json and expose it additively as geoip in /status via GeoIpStatus port + file adapter, so silent fallbacks are visible. - Shift live Tor verification left: reusable live-image-verification workflow now gates PRs, publish and release against a locally built image; daily canary keeps alerting against the published image as drift detector only. Includes upstream Content-Length drift guard from shipped Settings defaults. - Branch protection on main requires the live gate.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Relative reusable-workflow paths fail on pull_request events (startup_failure, 0 jobs): GitHub resolves them against the simulated merge commit. Referencing the reusable workflow by repo@head_ref tests the PR's own version of the workflow.
…le workflow The reusable-workflow form failed with startup_failure (0 jobs) on pull_request events: GitHub resolves relative reusable paths against the simulated merge commit. A composite action under .github/actions/live-verification is resolved from the checkout and works on every event (PR, push, tag, schedule). Same steps, same inputs; callers now check out first and invoke the action.
- shell: bash on every run step (required by composite actions) - Docker Hub token passed as input (secrets context is not available inside composite actions) - drop timeout-minutes (unsupported in composite steps; the bootstrap loop is self-bounded at 36x5s)
…tion The gateway step polled the published host port through the kill switch, which never returns connected (ISSUE-19 pattern): PR #118 burned 8m50s in this step although the container's Tor bootstrapped in 8s. Poll over loopback inside the container with a 180s wall-clock cap, mirroring the PR gate fix.
MustermenschAlex
left a comment
MustermenschAlex
left a comment
There was a problem hiding this comment.
Thanks for this — a clean fix on both levels. The per-file GeoIP limits keep the legacy single-var behavior intact (good for the compatibility contract), the new /status geoip field is purely additive, and the hermetic tests pin the real upstream sizes plus the ?? codes. Shifting the live check into a pre-publish gate via the composite action is the right structural answer to the 'detected only after release' problem. All required checks green, including the new live gate. Approving.
kuenstlichedummheit-sys
left a comment
kuenstlichedummheit-sys
left a comment
There was a problem hiding this comment.
Nice catch on the double root cause — the ?? codes would have kept this red even after the size fix, classic whack-a-mole otherwise. Also glad the gateway poll got the in-container treatment; 8m50s of kill-switch ping-pong was never going to flip to connected. Live gate at 1m35s now, that is the way it should feel. Ship it.
majorungustl
left a comment
majorungustl
left a comment
There was a problem hiding this comment.
Honestly the best part is how boring this got: one shared action, clear inputs, no reusable-workflow merge-commit weirdness, and /status finally tells you when geoip silently fell back instead of making you grep container logs at 4am. The drift guard reading limits from Settings instead of copy-pasted numbers is a nice touch too. LGTM, merge away.
Addresses the CodeQL empty-except alert on PR #118: the except clause intentionally ignores a missing/corrupt previous status file; document that instead of a bare pass.
5a3d9d9
MustermenschAlex
left a comment
MustermenschAlex
left a comment
There was a problem hiding this comment.
Re-reviewed after the CodeQL follow-up: the explanatory comment on the empty except resolves the alert properly, and all checks are green again on the new head. The compatibility story (additive /status field, legacy env var still capping both files) remains solid. Approving.
kuenstlichedummheit-sys
left a comment
kuenstlichedummheit-sys
left a comment
There was a problem hiding this comment.
Second look after the tiny CodeQL comment fix: still all green, live gate still fast (1m28s), sanity restored. Nothing else to poke at from the edge-case department. Good to go.
majorungustl
left a comment
majorungustl
left a comment
There was a problem hiding this comment.
Re-checked post-comment-fix: no new complexity snuck in, just a two-line comment, CI all green. Still the same boring-in-a-good-way change. Merge it.
Closes #116
Closes #117
Problem
Daily Live Image Verification failed since PR #103/#114 because upstream geoip6 is ~16 MB but TORPROXY_GEOIP_MAX_BYTES defaulted to 10 MiB, so the IPv6 GeoIP download was always rejected. The live check only ran after publish, so the breakage shipped undetected. A second, latent breakage was found during local live verification: current upstream geoip files contain literal
??unknown-origin country codes, which the validator rejected.Changes
??unknown-origin codes (TDD, hermetic unit tests with real upstream lines).Verification (local)