Repository navigation
Phase 1: security & correctness hardening - #21
Merged
Merged
Conversation
feat: prerequisite flags, personal access tokens, change-request appr…
docs: update roadmap
fix: docs and release
changed license to apache 2.0 from mit
docs: update
fix: update cve affected quinn-proto
docs: uodate
…imits The rate limiter believed the first X-Forwarded-For entry from any peer, so any client could pick its own rate-limit identity by sending the header, and one 60/min bucket covered every route. - New client_ip module: the TCP peer is the client unless it is listed in TRUSTED_PROXIES (IPs/CIDRs). From a trusted peer the X-Forwarded-For chain is walked from the right, skipping trusted hops; malformed or non-ASCII input falls back to the nearest trusted address, never to a client-chosen one. The walk is bounded. Invalid TRUSTED_PROXIES refuses startup. - Separate budgets for auth/setup, API, SDK snapshot, SSE connect and ingest (RATE_LIMIT_*_PER_MINUTE, 0 disables a class). /health is exempt. Only opening an SSE stream is counted, never events on it. - IPv6 clients are keyed by /64; idle entries are evicted every minute so limiter state stays bounded by recently active clients. - 429s carry Retry-After and a JSON body the dashboard login already reads. - Login lockout records and SDK health use the resolved client IP. Tests: resolver and limiter unit tests (spoofed headers, trusted, untrusted and multi-hop chains, malformed input, independent budgets, eviction, 429 shape, health exemption) and tests/rate_limit.rs against the real binary. The integration suite now marks 127.0.0.1 as a trusted proxy, which its X-Forwarded-For scenarios rely on.
… token The setup-complete check ran before the transaction that created the admin, so concurrent requests could all pass it, and the admin upsert (ON CONFLICT DO UPDATE) let a later racer overwrite the winner's password and role. Anyone who reached a fresh installation first could also claim it. - setup_complete takes pg_advisory_xact_lock and re-reads setup_complete under the lock, then creates the admin, workspace name, project name and completion flag in one transaction. Exactly one request wins, across any number of instances; the rest get 404. Any failure rolls everything back. - The admin upsert only claims a pre-password-auth row (password_hash IS NULL); an account that has a password is never overwritten (409). - /api/setup/key and /api/setup/complete require X-Checkgate-Setup-Token (header only). The token is SETUP_TOKEN (min. 16 chars, else startup fails) or, when unset, a random per-instance token written to SETUP_TOKEN_FILE (0600) with only its SHA-256 stored in settings, so every instance accepts it. The token is never logged. Completing setup revokes all tokens in the same transaction; stale token files are removed on the next start. - Dashboard wizard asks for the token on its first step and validates it before continuing; the token stays in memory only. - The image writes its token to /var/lib/checkgate/setup-token. Tests: tests/setup_ownership.rs (2 and 12 concurrent requests, concurrent requests on two instances, missing/wrong/query-string tokens, repeated setup, login after setup, injected failure rolls back and retry succeeds, generated tokens shared across instances and absent from logs, password accounts not overwritten, short token refused). The existing suites, the Node SDK e2e and the screenshot fixture pass the token.
GET /api/health/connections returned every SSE connection on the instance, with full client IPs, to any authenticated user or token. - Workspace admins keep installation-wide visibility and full addresses. - Everyone else (sessions and PATs of either scope, acting as their user) sees only connections of environments in projects they belong to, with client_ip reduced to its /24 (IPv4) or /48 (IPv6) network. - Connections without an environment (dashboard sessions, legacy SDK_KEY) are admin-only. SDK keys stay refused. - Connection records keep the resolved client IpAddr; the module docs, the API reference and the dashboard page state that tracking is per instance. Tests: tests/sdk_health.rs opens simultaneous streams in three environments across two projects plus an admin session, then checks the admin, a viewer of one project, its read-only PAT, an editor of the other project, a user with no membership, SDK keys and anonymous callers; also stream close and membership removal.
The server loaded every environment's flags into one checkgate_core FlagStore keyed only by flag key, so two environments defining the same key overwrote each other in it. Nothing on the server ever read that store: SSE bootstrap, /flags/snapshot and the REST API all query PostgreSQL per environment. It was only startup cost and a latent cross-environment hazard for any future reader. - Drop AppState::store, the startup flag/segment preload, the Redis subscriber's store updates and the handler-side upserts. The subscriber now forwards payloads unchanged, exactly as it did before; SSE handlers still filter them by environment. - The SDK-side FlagStore API in checkgate-core is unchanged; each SDK instance remains scoped to one environment. - Architecture and deployment docs no longer describe a server cache. Tests: tests/environment_isolation.rs defines `checkout` and `gate` in three environments (two projects) with different values and checks REST reads, poll snapshots, SSE bootstrap, update/delete/recreate in one environment, a server restart, and Redis-delivered updates on a second instance — evaluating each environment's SDK store with the core to cover targeting rules, weighted variants and prerequisites.
…loyment modes The entrypoint always started the bundled PostgreSQL and Redis and overwrote DATABASE_URL and REDIS_URL with localhost strings, so the documented "external services" deployment silently ran against a database inside the container. The release binary also fell back to localhost when either URL was missing. - One image, two modes, chosen by CHECKGATE_MODE (all-in-one | external). Unset: external when DATABASE_URL or REDIS_URL is given, else all-in-one. External mode starts no bundled service and never touches the URLs; a missing URL, a URL given with CHECKGATE_MODE=all-in-one, or an unknown mode stops the container with a message naming the problem. The target is logged with credentials redacted. - Release builds refuse to start without DATABASE_URL/REDIS_URL (debug builds keep the localhost defaults with a warning). - Bundled Redis now writes into /var/lib/redis, the volume the compose file and docs already mounted. Dropped the misleading SDK_KEY warning. - scripts/docker-smoke.sh exercises both modes against a built image: first boot, generated setup token (0600, never logged), setup and login, data surviving restart and container re-create, data landing in the external database, no bundled processes in external mode, and eight misconfigurations being refused. Runs in CI and gates the release. - Self-hosting guide restructured: modes, Compose all-in-one, external services, local development, production checklist, env reference, health checks, and upgrade notes (including moving data out of the bundled database for anyone who previously passed external URLs).
…adding 3,757 generated files were tracked: sdks/nodejs/node_modules (3,746 files), the Flutter SDK's .dart_tool/ and its build/ test caches (one 45 MB .dill). Nothing reads them from Git — every CI and release job runs npm install, bun install or (flutter|dart) pub get first, and the Node SDK unit tests mock their two runtime dependencies. Because pub packs from the working tree minus .gitignore, the Flutter caches could also have reached a published package. - Untracked with `git rm --cached`; local copies are left in place. Lockfiles, generated bindings and packaging scripts are kept. - .gitignore: node_modules/ everywhere, coverage/, .dart_tool/, Flutter plugin state and the Flutter SDK build directory. - scripts/check-tracked-artifacts.sh fails when generated or machine-specific paths are tracked; a new CI job runs it.
…ed in 4c393cb Commit 4c393cb ("satisfy clippy needless_range_loop") also picked up edits to NOTICE and README.md that were staged in the working tree at the time and are unrelated to this branch. Restore both files to main's content so this branch carries no attribution changes. (4c393cb also contains the entrypoint mode switch and connection_url check that belong with 4020782; they are kept.)
…ass) - Setup pins its transaction to READ COMMITTED before taking the advisory lock. The re-check of setup_complete relies on a fresh per-statement snapshot; under a REPEATABLE READ database default the snapshot was taken before the lock wait, so a losing request missed the winner's commit and failed with a serialization error (500) instead of 404. New test: concurrent setups against a database whose default isolation is repeatable read (fails without the fix, passes with it). - The entrypoint's logged connection target also drops the query string, since libpq-style URLs can carry ?password=; the smoke test checks it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 1 of the hardening roadmap: security and correctness fixes. No architecture change and no new infrastructure.
What changes
Trusted proxies and per-class rate limits (P0). The limiter believed the first
X-Forwarded-Forentry from any peer, so a client could choose its own rate-limit identity. Now the TCP peer is the client unless it is listed inTRUSTED_PROXIES. From a trusted peer, the forwarded chain is read from the right. Budgets are separate for auth/setup (20/min), API (300), SDK snapshot (600), SSE connect (120) and ingest (600), each set byRATE_LIMIT_*_PER_MINUTE./healthis exempt. Only opening an SSE stream counts. IPv6 is keyed by /64, and idle entries are evicted every minute. A 429 now carriesRetry-Afterand a JSON body. Login lockout and SDK health record the resolved client.Atomic, token-gated first-run setup (P0). The setup-complete check ran outside the transaction, and
ON CONFLICT DO UPDATElet a racing request overwrite the winner's password. Setup now runs in one READ COMMITTED transaction underpg_advisory_xact_lockand re-checks completion under the lock. Exactly one request wins, across instances. Existing password accounts are never overwritten (409)./api/setup/*requiresX-Checkgate-Setup-Token. It comes fromSETUP_TOKEN, or a per-instance generated token written to a 0600 file (hash insettings, never logged). The dashboard wizard asks for it.Project-scoped SDK health (P1).
/api/health/connectionsreturned every connection with full IPs to any user. Now workspace admins see everything. Others see only environments in their projects, with IPs masked to /24 or /48. Unscoped connections are admin-only. Tracking is documented as per instance.External DB deployment (P1). The entrypoint overwrote
DATABASE_URL/REDIS_URL, so the documented external deployment silently used the bundled database. The image now has two explicit modes (CHECKGATE_MODE=all-in-one|external, inferred from the URLs). Half-specified or conflicting configuration is refused. Release builds no longer fall back tolocalhost.scripts/docker-smoke.shcovers both modes and runs in CI and the release gate.Multi-environment flag cache (P1). The server's
FlagStorewas keyed by flag key only, and it turned out to be write-only: nothing read it. SSE bootstrap, snapshot and REST all query Postgres per environment. It has been removed. The SDKFlagStoreAPI is unchanged.Repository hygiene (P1). 3,757 generated files are untracked:
sdks/nodejs/node_modules, Flutter.dart_tool/andbuild/(including a 45 MB.dill)..gitignoreis updated, and a CI guard (scripts/check-tracked-artifacts.sh) stops them coming back.Upgrade notes
DATABASE_URL/REDIS_URL: your data is in the bundled database. Migrate it before upgrading (seedocs/self-hosting.md→ Upgrading).TRUSTED_PROXIES. Otherwise all clients share the proxy's budget.Commit note
4c393cb(titled as a clippy fix) also contains the entrypoint mode switch andconnection_urlcheck, which belong with4020782, plusNOTICE/READMEedits. Those files were staged in the working tree at the time.8a855cdrestored those two files.03cc307then applies the copyright attribution update toNOTICEandREADME.mdas its own commit.Tests
New:
server/tests/{rate_limit,setup_ownership,sdk_health,environment_isolation}.rs(shared harness, one database per test), 26 new unit tests, a dashboard setup-step test, and the Docker smoke script. Mutation checks: restoring the old XFF handling, the old setup race and the old isolation behaviour each makes the matching tests fail. All existing suites pass unchanged, apart from passing the setup token and marking the test client as a trusted proxy.