diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a4750d9..78a8b20 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -199,18 +199,29 @@ jobs: - name: Cache Rust build artefacts uses: Swatinem/rust-cache@v2 - - name: Apply database schema + # Apply the real migration chain (0001..latest) — the same artifact CI's + # `rust` job and production apply. This validates the migrations as an + # ordered, idempotent chain and can never silently depend on a stale + # database/schema.sql mirror (which had drifted from migrations). + - name: Apply database migrations env: PGPASSWORD: trident run: | - psql -h localhost -U postgres -d trident_test \ - -f database/schema.sql + set -euo pipefail + for f in database/migrations/*.sql; do + echo "Applying $f" + psql -v ON_ERROR_STOP=1 -h localhost -U postgres -d trident_test -f "$f" + done - name: Run integration tests env: DATABASE_URL: postgres://postgres:trident@localhost:5432/trident_test TEST_DATABASE_URL: postgres://postgres:trident@localhost:5432/trident_test TEST_REDIS_URL: redis://localhost:6379 + # Turns a missing TEST_DATABASE_URL/TEST_REDIS_URL into a hard test + # failure instead of a silent skip, so a misconfigured integration + # job cannot report green without running the DB/Redis tests. + REQUIRE_TEST_SERVICES: "1" CI: "true" run: cargo test --all -- --include-ignored --test-threads=1 diff --git a/REPORT.md b/REPORT.md new file mode 100644 index 0000000..47d7531 --- /dev/null +++ b/REPORT.md @@ -0,0 +1,129 @@ +# Trident Hardening Sweep — Findings Report + +Branch: `hardening-sweep` (cut from `pr-189-fix` @ `b93316a`, which is 78 commits ahead of +`origin/main` and is the "green under fire" work; `main` itself is behind and has red history). + +## Baseline (Phase 0) + +- HEAD `b93316a` = **all 8 CI jobs green** (Rust, Rust integration tests, Go, TypeScript, + OpenAPI, Docker build, E2E smoke, Helm). Verified via check-runs API. +- Repo map: `crates/{api,indexer,backfill,common}` (Rust), `services/api` (Go REST + gRPC + client), `sdk/{typescript,rust,go,python,react}`, `database/{schema.sql,migrations/0001-0009}`, + `docker/*compose*`, `helm/trident`, `.github/workflows/{ci,release}.yml`. + +## How "green" hides gaps (Phase 1 — proven from CI logs) + +- The env-gated Rust tests use `require_services!` which does `eprintln!("SKIP…"); return;` on + missing `TEST_DATABASE_URL`/`TEST_REDIS_URL`. cargo **hides passing-test stderr**, so + "no SKIP lines in the log" proves nothing. The real evidence is timing: + the same 61-test suite ran in **0.20s in the `rust` job** (env absent → early-returned/skipped) + vs **9.42s in `rust-integration`** (env present → real DB/Redis work). So the DB/Redis + integration tests genuinely execute **only** in `rust-integration`, and that job is + `if`-gated to push-to-main/dev or PR-with-base-main/dev. + +--- + +## Findings (ranked by severity) + +### HIGH + +**H1 — `database/schema.sql` is not a canonical mirror of the migration chain; a DB +bootstrapped from it is materially broken.** +`database/schema.sql:1` claims "Canonical definition. Migrations mirror this file." It has drifted hard: +- Missing table **`audit_log`** (migration `0006`). → Go audit middleware + (`services/api/middleware/audit.go:165`), admin analytics (`services/api/handlers/admin.go:151`), + and the 90-day cleanup (`services/api/main.go:290`) all hit `relation "audit_log" does not exist`. +- Missing table **`parse_errors`** (migration `0008`). → indexer parse-error isolation write + (`crates/indexer/src/db/mod.rs:246`) and the Go `/status` count + (`services/api/handlers/status.go:110`) fail at runtime. +- Missing `system_state` columns **`last_poll_at`, `last_alert_at`** (+`last_ledger_indexed`, + `events_indexed_total`, `events_in_last_poll`, `poll_duration_ms`, `alert_fired`) from + migrations `0002`/`0003`. → indexer health write `UPDATE system_state SET last_poll_at…` + (`crates/indexer/src/db/mod.rs:159`) and alert-state read/write + (`crates/indexer/src/db/mod.rs:201`) fail. +- **soroban_events indexes fully diverged.** schema.sql defines 9 legacy indexes + (`idx_soroban_events_contract_id`, `…_topic_0`, `…_topic_1`, `…_topics_gin`, + `…_contract_network`, `…_ledger_sequence`, `…_contract_topic_0`) that **no migration creates**, + and is missing the migration `0009` high-cardinality perf indexes actually deployed to + production (`idx_soroban_events_contract_ledger`, `…_id_desc`, `…_contract_topic0` partial, + `…_ledger_timestamp`) plus `0004`'s `…_network_contract`. +- **Extra constraint** `uq_soroban_events_tx_index UNIQUE(transaction_hash,event_index)` + (`database/schema.sql:42`) exists in no migration. Insert path uses `ON CONFLICT (id)` + (`crates/indexer/src/db/mod.rs:66`), so it is redundant divergence. + +*Failure scenario:* the **`rust-integration` CI job builds `trident_test` from schema.sql** +(`.github/workflows/ci.yml:206`), so every code path touching `audit_log`, `parse_errors`, or the +`system_state` health/alert columns is **not integration-covered** — tests are green only because +they never exercise those paths against the DB. Anyone who runs `psql -f schema.sql` for local/dev +gets a schema on which audit logging, parse-error isolation, health/alert tracking, and the +`/stats` + `/status` endpoints crash on first use. + +### MEDIUM + +**M1 — Integration tests can silently skip even inside the integration job.** +`require_services!` (`crates/indexer/src/streamer/mod.rs:400`, +`crates/api/src/services/events.rs:367`, `crates/indexer/src/db/mod.rs:304`) `return`s instead of +failing when the env is absent. GitHub Actions sets `CI=true` in *every* job, so `CI` can't +distinguish the two jobs. *Failure scenario:* if `TEST_DATABASE_URL` is ever misconfigured in the +`rust-integration` job (typo, service rename, port change), all DB/Redis tests skip and the job +still reports **green** — a false pass with zero signal. + +**M2 — Indexer resume passes a ledger sequence where the RPC expects a paging-token cursor.** +`crates/indexer/src/streamer/mod.rs:186-190`: on restart (`*cursor != 0`) it calls +`get_events(start_ledger=None, cursor=Some(cursor.to_string()))`, but the persisted cursor is a +**ledger sequence** (`db::set_cursor` stores `last.ledger` at `streamer/mod.rs:317`), while +in-loop pagination correctly uses the RPC `paging_token` (`streamer/mod.rs:223,349`). The Soroban +`getEvents` `cursor` field (`crates/indexer/src/rpc/mod.rs:93`) is a paging token, not a ledger +number. *Failure scenario:* after any indexer restart the first poll sends +`cursor:"12345"`; the RPC rejects it as an invalid cursor and the poll loop errors/retries, +stalling ingestion. **Flagged for review — changes runtime behavior of the resume feature; not +auto-fixed.** Likely correct fix: resume via `start_ledger = Some(cursor + 1)` instead of the +ledger-as-cursor. + +**M3 — Auth Redis cache is never invalidated on key revocation.** +`services/api/middleware/auth.go:99-123` caches a successful lookup for 5 min +(`authCacheTTL`) keyed by `sha256(key)` and does not delete/expire it when a key is revoked. +*Failure scenario:* a revoked/compromised API key keeps authenticating for up to 5 minutes after +revocation. **Flagged for review** (acceptable if documented; fix = delete cache key on revoke). + +**M4 — Floating Docker base image breaks build reproducibility.** +`crates/api/Dockerfile:4` and `crates/indexer/Dockerfile:4` use `FROM rust:1-slim` (floating +major). *Failure scenario:* a new upstream `rust:1` publish silently changes the toolchain, +producing non-reproducible builds and potential clippy/edition breakage unrelated to any commit. +Pin to an explicit patch (e.g. `rust:1.83-slim`). Go/alpine bases are already pinned. + +### LOW + +**L1 — `rust-integration` job is entirely skipped on feature-branch pushes.** +`.github/workflows/ci.yml:168-170` `if:` gate means integration tests only run on push to +main/dev or PR based on main/dev. Green on a feature branch does not mean the DB tests ran. +Acceptable by design; documented here so it isn't mistaken for coverage. + +**L2 — Obsolete compose `version:` keys.** `docker/docker-compose.yml:1` (`version: "3.9"`) and +`docker/docker-compose.dev.yml:1` — Compose v2 ignores and warns on these. + +**L3 — `pgbouncer` service still defined in `docker/docker-compose.yml` / +`docker-compose.ci.yml`** though a prior fix removed it from E2E `depends_on`. Dead scaffolding in +the CI stack. + +**L4 — Generated gRPC "not implemented" stubs** in `services/api/gen/trident_grpc.pb.go:95-103` +are normal protoc `Unimplemented*Server` output (the Go service is a gRPC *client*), not a real +gap. Noted to pre-empt false alarms. No other real-code TODO/FIXME/unimplemented in the tree. + +--- + +## Fix plan (Phase 7) + +1. **H1** — rewrite `database/schema.sql` to faithfully mirror migrations `0001–0009` + (add `audit_log`, `parse_errors`, the `system_state` health/alert columns; replace the + soroban_events index set with the `0004`+`0009` canonical indexes; drop the non-migration + `uq_soroban_events_tx_index`). +2. **H1/M1 (durable)** — switch the `rust-integration` job to build `trident_test` from the + **migration chain** instead of `schema.sql`, so CI validates the real deploy artifact and can + never again silently depend on a stale mirror; and make `require_services!` **panic** (fail + loud) when a dedicated `REQUIRE_TEST_SERVICES` flag (set only in that job) is present but the + URLs are missing. +3. **M4 / L2 / L3** — pin the Rust Docker base, drop obsolete `version:` keys and the dead + pgbouncer service (verified via the Docker build + E2E CI jobs). +4. **M2 / M3** — reported only; flagged for maintainer review because they change the runtime + behavior of existing features. diff --git a/crates/api/src/services/events.rs b/crates/api/src/services/events.rs index abfaef4..2dae9eb 100644 --- a/crates/api/src/services/events.rs +++ b/crates/api/src/services/events.rs @@ -364,23 +364,25 @@ mod tests { use super::*; + // When REQUIRE_TEST_SERVICES is set (the rust-integration CI job sets it), + // a missing URL is a hard failure instead of a silent skip — otherwise a + // misconfigured integration job would go green without running anything. macro_rules! require_services { () => {{ - let db = match std::env::var("TEST_DATABASE_URL") { - Ok(url) => url, - Err(_) => { - eprintln!("SKIP: TEST_DATABASE_URL not set"); + let required = std::env::var("REQUIRE_TEST_SERVICES").is_ok(); + match ( + std::env::var("TEST_DATABASE_URL"), + std::env::var("TEST_REDIS_URL"), + ) { + (Ok(db), Ok(rd)) => (db, rd), + _ if required => panic!( + "TEST_DATABASE_URL and TEST_REDIS_URL must be set when REQUIRE_TEST_SERVICES is set" + ), + _ => { + eprintln!("SKIP: TEST_DATABASE_URL / TEST_REDIS_URL not set"); return; } - }; - let rd = match std::env::var("TEST_REDIS_URL") { - Ok(url) => url, - Err(_) => { - eprintln!("SKIP: TEST_REDIS_URL not set"); - return; - } - }; - (db, rd) + } }}; } diff --git a/crates/indexer/src/db/mod.rs b/crates/indexer/src/db/mod.rs index ebc2c03..743d304 100644 --- a/crates/indexer/src/db/mod.rs +++ b/crates/indexer/src/db/mod.rs @@ -303,6 +303,11 @@ mod tests { async fn insert_event_is_idempotent() { let db_url = match std::env::var("TEST_DATABASE_URL") { Ok(url) => url, + // Hard-fail under the rust-integration CI job (REQUIRE_TEST_SERVICES) + // so a misconfigured DB URL cannot silently skip and go green. + Err(_) if std::env::var("REQUIRE_TEST_SERVICES").is_ok() => { + panic!("TEST_DATABASE_URL must be set when REQUIRE_TEST_SERVICES is set"); + } Err(_) => { eprintln!("SKIP: TEST_DATABASE_URL not set"); return; diff --git a/crates/indexer/src/streamer/mod.rs b/crates/indexer/src/streamer/mod.rs index 822af32..ca346d4 100644 --- a/crates/indexer/src/streamer/mod.rs +++ b/crates/indexer/src/streamer/mod.rs @@ -396,24 +396,26 @@ mod tests { use wiremock::matchers::{body_partial_json, method, path}; use wiremock::{Mock, MockServer, ResponseTemplate}; - // Skip the test and return early if required env vars are missing. + // Return the (db, redis) URLs, or skip the test when they are absent. + // When REQUIRE_TEST_SERVICES is set (the rust-integration CI job sets it), + // a missing URL is a hard failure instead of a silent skip — otherwise a + // misconfigured integration job would go green without running anything. macro_rules! require_services { () => {{ - let db = match std::env::var("TEST_DATABASE_URL") { - Ok(v) => v, - Err(_) => { - eprintln!("SKIP: TEST_DATABASE_URL not set"); + let required = std::env::var("REQUIRE_TEST_SERVICES").is_ok(); + match ( + std::env::var("TEST_DATABASE_URL"), + std::env::var("TEST_REDIS_URL"), + ) { + (Ok(db), Ok(rd)) => (db, rd), + _ if required => panic!( + "TEST_DATABASE_URL and TEST_REDIS_URL must be set when REQUIRE_TEST_SERVICES is set" + ), + _ => { + eprintln!("SKIP: TEST_DATABASE_URL / TEST_REDIS_URL not set"); return; } - }; - let rd = match std::env::var("TEST_REDIS_URL") { - Ok(v) => v, - Err(_) => { - eprintln!("SKIP: TEST_REDIS_URL not set"); - return; - } - }; - (db, rd) + } }}; } diff --git a/database/schema.sql b/database/schema.sql index f9e6550..ad7c52e 100644 --- a/database/schema.sql +++ b/database/schema.sql @@ -1,5 +1,8 @@ -- Trident PostgreSQL Schema --- Canonical definition. Migrations in ./migrations/ mirror this file incrementally. +-- Convenience full-schema snapshot for local/dev bootstrap and documentation. +-- The migration chain in ./migrations/ (0001-0009) is the source of truth and is +-- what CI and production apply; this file must mirror the end state of that chain. +-- Keep in sync whenever a migration is added. CREATE EXTENSION IF NOT EXISTS "pgcrypto"; @@ -23,25 +26,16 @@ CREATE TABLE IF NOT EXISTS soroban_events ( created_at TIMESTAMPTZ NOT NULL DEFAULT NOW() ); --- Single-column indexes -CREATE INDEX IF NOT EXISTS idx_soroban_events_contract_id ON soroban_events (contract_id); -CREATE INDEX IF NOT EXISTS idx_soroban_events_ledger_sequence ON soroban_events (ledger_sequence); -CREATE INDEX IF NOT EXISTS idx_soroban_events_ledger_timestamp ON soroban_events (ledger_timestamp); -CREATE INDEX IF NOT EXISTS idx_soroban_events_network ON soroban_events (network); -CREATE INDEX IF NOT EXISTS idx_soroban_events_topic_0 ON soroban_events (topic_0); -CREATE INDEX IF NOT EXISTS idx_soroban_events_topic_1 ON soroban_events (topic_1); - --- Composite indexes -CREATE INDEX IF NOT EXISTS idx_soroban_events_contract_topic_0 ON soroban_events (contract_id, topic_0); -CREATE INDEX IF NOT EXISTS idx_soroban_events_contract_network ON soroban_events (contract_id, network); - --- GIN index for arbitrary topic containment queries -CREATE INDEX IF NOT EXISTS idx_soroban_events_topics_gin ON soroban_events USING GIN (topics); - --- Unique constraint: a given event position within a transaction is immutable -ALTER TABLE soroban_events - ADD CONSTRAINT uq_soroban_events_tx_index - UNIQUE (transaction_hash, event_index); +-- Indexes — canonical set produced by migrations 0004 and 0009. +-- network isolation (0004) +CREATE INDEX IF NOT EXISTS idx_soroban_events_network ON soroban_events (network); +CREATE INDEX IF NOT EXISTS idx_soroban_events_network_contract ON soroban_events (network, contract_id); +-- high-cardinality query patterns (0009) +CREATE INDEX IF NOT EXISTS idx_soroban_events_contract_ledger ON soroban_events (contract_id, ledger_sequence DESC); +CREATE INDEX IF NOT EXISTS idx_soroban_events_contract_topic0 ON soroban_events (contract_id, topic_0) + WHERE topic_0 IS NOT NULL; +CREATE INDEX IF NOT EXISTS idx_soroban_events_id_desc ON soroban_events (id DESC); +CREATE INDEX IF NOT EXISTS idx_soroban_events_ledger_timestamp ON soroban_events (ledger_timestamp DESC); -- --------------------------------------------------------------------------- -- system_state @@ -49,9 +43,18 @@ ALTER TABLE soroban_events -- re-scanning from genesis. -- --------------------------------------------------------------------------- CREATE TABLE IF NOT EXISTS system_state ( - key TEXT PRIMARY KEY, - value TEXT NOT NULL, - updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW() + key TEXT PRIMARY KEY, + value TEXT NOT NULL, + updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + -- indexer health columns (migration 0002) + last_poll_at TIMESTAMPTZ, + last_ledger_indexed BIGINT, + events_indexed_total BIGINT NOT NULL DEFAULT 0, + events_in_last_poll INT NOT NULL DEFAULT 0, + poll_duration_ms INT NOT NULL DEFAULT 0, + -- alerting state columns (migration 0003) + last_alert_at TIMESTAMPTZ, + alert_fired BOOLEAN NOT NULL DEFAULT FALSE ); -- Seed the cursor row so the indexer can always do an UPDATE rather than @@ -114,6 +117,43 @@ CREATE INDEX IF NOT EXISTS idx_api_keys_key_hash ON api_keys (key_hash); CREATE INDEX IF NOT EXISTS idx_api_keys_active ON api_keys (key_hash) WHERE revoked_at IS NULL; +-- --------------------------------------------------------------------------- +-- audit_log +-- Per-request audit trail for API key usage (migration 0006). +-- --------------------------------------------------------------------------- +CREATE TABLE IF NOT EXISTS audit_log ( + id BIGSERIAL PRIMARY KEY, + api_key_id UUID REFERENCES api_keys(id) ON DELETE SET NULL, + endpoint TEXT NOT NULL, + method TEXT NOT NULL, + ip INET, + user_agent TEXT, + status_code INT NOT NULL, + duration_ms INT NOT NULL, + result_count INT, + request_id TEXT NOT NULL, + network TEXT, + ts TIMESTAMPTZ NOT NULL DEFAULT NOW() +); + +CREATE INDEX IF NOT EXISTS idx_audit_log_key_ts ON audit_log (api_key_id, ts DESC); +CREATE INDEX IF NOT EXISTS idx_audit_log_ts ON audit_log (ts DESC); + +-- --------------------------------------------------------------------------- +-- parse_errors +-- Audit trail for events that failed XDR decoding (migration 0008). +-- --------------------------------------------------------------------------- +CREATE TABLE IF NOT EXISTS parse_errors ( + id UUID PRIMARY KEY DEFAULT gen_random_uuid(), + ledger_sequence BIGINT NOT NULL, + event_index INT NOT NULL, + raw_payload TEXT NOT NULL, + error_message TEXT NOT NULL, + occurred_at TIMESTAMPTZ NOT NULL DEFAULT NOW() +); + +CREATE INDEX IF NOT EXISTS idx_parse_errors_occurred_at ON parse_errors (occurred_at DESC); + -- --------------------------------------------------------------------------- -- webhook_subscriptions -- ---------------------------------------------------------------------------