ci: local test database so DB-gated suites run before push, not in CI - #767
Merged
Conversation
The DB-gated suites do not run without OPENWATCH_TEST_DSN, so `make ci-local` silently skipped them and a change could look clean locally and fail in CI. That happened three times in one day on the v0.7 branches: the report-schedule suites (#766), the RBAC role-assign and token tests (#765), and the go mod tidy drift (#762). Every one was a real defect that a local run would have caught. scripts/test-db.sh mirrors the CI service container exactly. Image and credentials match .github/workflows/go-ci.yml, because if they drift then "passes locally" stops meaning "passes in CI", which is the whole point. Two settings are load-bearing and are commented as such in the script: max_connections=400. The default 100 is not enough. The suite runs packages in parallel and internal/db/dbtest clones a template database per package, so a default container deadlocks: the first DB test times out at 120s and the rest cascade. Found this the honest way, by hitting it. It looks exactly like a real failure and is not one, which is worse than no local database at all. fsync, synchronous_commit and full_page_writes off. The database is disposable; durability buys nothing and costs most of the runtime. Port defaults to 5455 rather than 5432: on this workstation 5432 is the dev database and 5433 is hanalyx-postgres. Override with TEST_DB_PORT. The database name ends in _test so the internal/db guard accepts it without the ALLOW_NONTEST bypass, which we never want set. `make ci-local` now warns when OPENWATCH_TEST_DSN is unset and says exactly how to fix it. A silent skip is what let this through repeatedly; a loud skip is the minimum. Verified: full `go test ./internal/...` against the container is clean, and the three suites CI previously caught now run and pass locally.
The blanket `*test*.sh` in .gitignore swallowed scripts/test-db.sh. `git add` refused it, the ci-local change shipped referencing a script that was never committed, and nothing failed loudly: the PR just contained a Makefile target pointing at a file no one else would have. Same trap as the blanket `*.spec` rule that drops new packaging spec files. Negate deliberately and verify with `git check-ignore -v <path>` after adding anything to scripts/ whose name contains "test".
remyluslosius
added a commit
that referenced
this pull request
Jul 30, 2026
…G3) (#768) Four findings, each a different class, all reachable without credentials. enqueue-test-job wrote to the REAL job queue with no authentication (handlers.go, queue.Enqueue with no check above it). Anyone who could reach the port could enqueue without limit: a denial-of-service primitive and a source of unbounded table growth. Now requires system:config_write. /admin/license:verify answered anonymously on an /admin/ path. Submit any JWT and learn whether it verifies, its tier, its features, its expiry, and whether it was signed with the PREVIOUS key. That last one leaks key-rotation state. Now requires system:read. connectivity:check gated on host:read, but it is not a read: it makes the server open an SSH or ICMP connection to a managed host on demand. A dedicated host:connectivity_check permission already existed in the registry and was wired to nothing. Wired now. This is a deliberate behaviour change: viewer and auditor hold host:read but not host:connectivity_check, so they lose it; ops_lead, security_admin and admin keep it. GET /license disclosed customer_id, the paying organisation's identity, to any anonymous caller. Tier, status and features stay anonymous because a pre-login UI needs them; the identity does not qualify. While scoping this I measured the wider contract gap: 44 mutating routes carry no x-required-permission declaration. 30 of them DO enforce, so that is a contract accuracy problem rather than a hole, and 11 of the remaining 14 are correctly ungated (pre-session auth, or self-service on the caller's own identity). The genuinely exposed ones were the diagnostics routes above. Worth recording that the AC-18 coverage test cannot see any of this: it verifies declared-implies-enforced, and says nothing about routes that never declare. Closing that is a follow-up, and it needs the 30 declarations first. Also worth recording: my ad-hoc scan initially reported license:verify as enforcing, because it followed a helper and matched any EnforcePermission. AC-18 does not have that weakness; it requires the SPECIFIC constant the route declares, so following a helper that enforces something else does not match. Four existing tests asserted the old ungated behaviour and now pass a credentialled caller. Caught locally against the test database from #767 rather than in CI, which is the first time this session that has been true. Spec: system-rbac 1.2.0 -> 1.3.0, AC-21 (negative path). 116 specs, 116 passing. Full go test ./internal/... clean against the local test database.
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
1 participant
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.
Why
The DB-gated suites do not run without
OPENWATCH_TEST_DSN, somake ci-localsilently skipped them. A change could look clean locally and fail in CI.That happened three times in one day on the v0.7 branches:
go mod tidydriftattestationas a fixture, which I had just gatedEvery one was a real defect. A local run would have caught all three.
What it does
scripts/test-db.shmirrors the CI service container. Image and credentials match.github/workflows/go-ci.ymlexactly, because if they drift then "passes locally" stops meaning "passes in CI".Two settings that are load-bearing
max_connections=400. The default 100 is not enough. The suite runs packages in parallel andinternal/db/dbtestclones a template database per package, so a default container deadlocks: the first DB test times out at 120s and the rest cascade. I found this the honest way, by hitting it. It looks exactly like a real failure and is not one, which is worse than having no local database at all.fsync,synchronous_commit,full_page_writesoff. Disposable database; durability buys nothing and costs most of the runtime.Port
Defaults to 5455, not 5432: on this workstation 5432 is the dev database and 5433 is
hanalyx-postgres. Override withTEST_DB_PORT. The database name ends in_testso theinternal/dbguard accepts it without theALLOW_NONTESTbypass, which should never be set.The other half
make ci-localnow warns whenOPENWATCH_TEST_DSNis unset and prints the fix. A silent skip is what let this through repeatedly; a loud skip is the minimum.Verified
Full
go test ./internal/...against the container is clean, and the three suites CI previously caught now run and pass locally.