Backend refactor reconciliation + ASPA verification fix + silent-failure audit #2
Merged
rene.fichtmueller
merged 12 commits from 2026-07-16 20:18:49 +00:00
merge-prod-snapshot into main
12 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Rene Fichtmueller
|
46f96755ac |
fix: bogon check didn't recognize local-db-client status:error as unavailable
Follow-up from live testing the 2026-07-16 deploy: bogonDataUnavailable
only checked '!prefixData'/'!neighbourData' (falsy/null), but
local-db-client.js's DB-error signal is a truthy {status:'error', ...}
object, not null (server.js's own fabricated-success fix from earlier
today). So when the Postgres-backed prefix/neighbour source is what's
actually degraded, this check showed status:'pass' at 0 checked prefixes
instead of the more honest 'info' -- data wasn't fabricated (0 was
visible), just the status label didn't reflect it. Fixed with an explicit
isSourceUnavailable() check for both falsy and status:'error'.
|
||
|
Rene Fichtmueller
|
a3ed032687 |
docs: revert speculative vitest pool fix, document as open runner-level issue
The forks-pool change didn't fix it (failure went from 1s to 0s, if anything worse) -- reverting to plain 'npm test' since it's no more or less broken and is simpler. Documenting this as a known open item needing someone with actual authenticated Gitea dashboard access, rather than continuing to guess blindly from outside. |
||
|
Rene Fichtmueller
|
db83d77162 |
ci: constrain vitest to a small fixed fork pool for the CI runner
'npm test' (default vitest thread pool) fails in ~1s on this Gitea Actions runner for no reason reproducible locally (plain run, CI=true, and a fully fresh rm -rf node_modules && npm ci all pass 215/215). Forcing the forks pool with a small fixed worker count (min 1, max 2) is a standard fix for this class of CI-only vitest worker-pool crash. |
||
|
Rene Fichtmueller
|
140d730b60 |
test: fix pdf-export mocks/assertions for the setDefaultTimeout API change
The ASPA/silent-failure-audit fix to renderer.ts (page.pdf() has no 'timeout' option in real Playwright; moved to page.setDefaultTimeout() before the call) was only typechecked at the time, never run against the test suite -- the mocks didn't implement setDefaultTimeout and two assertions still expected the old timeout-in-pdf()-options shape. Caught by the new build-verify CI workflow on its first real run. 215/215 tests pass now. |
||
|
Rene Fichtmueller
|
b11eed9cf6 |
fix: don't fail build-verify/deploy on pre-existing mcp-server tsc exit code
Discovered live on Erik just now: 'npm run build' returns exit code 2 because of pre-existing src/mcp-server/index.ts type errors (unrelated to the server.js/API-server deploy path -- already documented in the 2026-07-16 audit), but with noEmitOnError left at its default (false) it still correctly emits dist/api/index.js, dist/api/server.js, etc. Both build-verify.yml and deploy-from-git.sh were treating that non-zero exit as a hard failure -- the CI run for the previous commit failed on exactly this, and deploy-from-git.sh's would have aborted a real deploy at the same step had I used it instead of manual commands. Now both check for the specific required output files instead of trusting tsc's exit code. |
||
|
Rene Fichtmueller
|
0caf7a271e |
ci: add build-verify workflow + git-pull-based deploy script
Two consecutive PeerCortex deploys today (2026-07-16) broke in the same way: server.js requires local files (src/backend/config.js, src/backend/services/smtp.js) that were never actually uploaded to Erik, because the deploy process was ad-hoc 'cat file | ssh' per-file uploads with no systematic check that every require() target actually exists on the target machine. A missing npm dependency (pg) caused the same class of problem separately. - .github/workflows/build-verify.yml: runs on every push/PR (Gitea Actions already has a working runner for this repo, confirmed via the existing .github/workflows/security-scan.yml on the github-import/main branch). npm ci, syntax-checks server.js + the other top-level .js files, verifies every local require() target resolves to an actual file, typechecks (informational for now -- see the step comment for why), builds dist/, runs the existing vitest suite. Catches both of today's failures before they'd ever reach Erik. - deploy-from-git.sh: replaces the per-file upload process with a single git pull + npm ci + require()-check + syntax-check + build + backup + pm2 restart sequence, run manually on Erik. Deliberately NOT wired to a Gitea Action with SSH secrets -- Erik hosts many unrelated production services and main can contain code that hasn't been live-verified yet, so the actual deploy trigger stays a human decision. CI catches what's broken; this makes running the fix a single repeatable command instead of a manually-assembled file list. |
||
|
Rene Fichtmueller
|
cc6b81ba8e |
fix: correct ASPA verification algorithm + fix systemic silent-failure/fake-data patterns
2026-07-16 deep-dive audit (server.js, local-db-client.js, src/aspa/validator.ts) found the flagship ASPA path verification producing wrong results, and a recurring pattern where an upstream API or the local Postgres DB failing would present as "checked, clean" instead of surfacing failure. All fixes in this commit are empirically or textually verified against primary sources (the IETF draft's actual algorithm text, RFC 5735/6890 bogon ranges, and hand-built controlled test cases with known-correct answers) -- not just read-and-guessed. CRITICAL: ASPA hop-check direction was reversed -------------------------------------------------- verifyUpstream/verifyDownstream/detectValleys called hopCheck(collapsed[i-1], collapsed[i]) -- checking the PROVIDER-side AS's ASPA object for the CUSTOMER, the opposite of draft-ietf-sidrops-aspa-verification section 5.3's authorized(A(I), A(I+1)) (A(I) = customer, always checked first). Proven empirically: a hand-built, fully-attested, zero-leak 3-AS path returned "Invalid" before this fix. verifyDownstream needed a full rewrite, not just an argument swap -- a downstream path can legitimately contain both an up-ramp AND a down-ramp (e.g. origin -> providers -> peering point -> validator's providers -> validator), and the old K/L/uMin/vMax computation didn't correctly implement the draft's max_up_ramp/min_up_ramp/max_down_ramp/min_down_ramp four-way computation. Rewrote it from the draft's exact halt conditions, verified against a worked example fetched from the draft itself (an N=4 path with a single unattested "kink" correctly resolves Valid) plus 3 hand-built scenarios (pure up-ramp, mixed up+down clean, and a definite leak with failures on both ends -- Invalid). Same bug existed independently in src/aspa/validator.ts (the MCP-server- side validator, different codebase, same conceptual error) -- validateUpstream was already correct there, but validateDownstream naively reversed the path and reapplied upstream logic, which cannot represent a legitimate down-ramp. Rewrote it with the same four-ramp algorithm, cross-validated against the same test scenarios through the compiled output. Also in the same /api/aspa/verify handler: aspaStore.set(providerAsn, new Set()) for detected-but-unattested providers made hopCheck report "NotProviderPlus" (implying a real ASPA object that denies the hop) instead of "NoAttestation" (we don't know) -- removed; hopCheck's own `!providers` branch already handles the unset case correctly. Also removed a fallback that fabricated an ASPA declaration from heuristically-detected BGP neighbours when aspa_object_exists is false, which contradicted that exact field in the same response. ROOT CAUSE of ~9 broken checks: duplicate function declaration -------------------------------------------------------------- Two `async function fetchRipeStatCached` declarations existed ~150 lines apart (the original real RIPE-Stat-fetch+cache+throttle implementation, and a newer local-DB-routing wrapper added during the Postgres refactor). In JS, the second declaration silently wins for every call site in the file. The wrapper only recognized 5 URL patterns (announced-prefixes, asn-neighbours, as-overview, visibility, prefix-size-distribution) and returned null unconditionally for everything else -- silently breaking blocklist (Spamhaus), abuse-contact-finder, bgp-updates, routing-status, looking-glass, whois, reverse-dns-consistency, maxmind-geo-lite-pfx, and ixs lookups. Renamed the original to fetchRipeStatCachedFromApi and made the wrapper fall through to it instead of returning null. Verified via an isolated routing test (blocklist/abuse-contact-finder URLs now reach the real implementation; the 5 known patterns still route to local DB). Silent-failure / fake-success patterns fixed --------------------------------------------- - local-db-client.js: 5 getRipeStat* functions returned status:'ok' with empty data on Postgres error (fabricated success) -- now status:'error'. server.js's /api/lookup now tracks which sources degraded, surfaces meta.degraded_sources, and caches a degraded result for 90s instead of the full 5min so a DB hiccup doesn't get repeated to every visitor. - validateRPKIWithCache: DB errors and genuine "no covering ROA" both produced status:"not_found" -- split into "not_found" (real) vs "unavailable" (DB error). Updated all 5 call sites that compute RPKI coverage percentages to exclude "unavailable" from the denominator (a DB hiccup must not lower a network's displayed RPKI/ASPA score), and fixed a pre-existing separate typo in the PDF report generator that checked for 'not-found' (hyphen) when the actual value is 'not_found' (underscore). - crossCheckRpki: an empty sample or a failed RIPE-validator lookup both counted as a fabricated 100%/"agreement" instead of being excluded -- now returns agreement_pct:null when nothing was actually compared, and the caller no longer averages null into overall_confidence. Also documented (not silently left) that bgp.he.net fetching is hardcoded off, so the "2-source" prefix/neighbour cross-checks never actually run -- dataQuality.cross_checks now reports sources:1 honestly instead of claiming a cross-check that never happened. - lookupAspaFromRpki: added feedLoaded so a Cloudflare-RPKI-feed fetch failure (rpkiAspaLastFetch never set) is distinguishable from "this ASN genuinely has no ASPA object" -- surfaced as aspa_feed_healthy on both ASPA endpoints. - Bogon/IRR/Spamhaus-blocklist/reverse-DNS/BGP-visibility checks: fetch failure -> empty result -> "pass" (or, for abuse-contact/rdns, a misleading "fail") -- now report "info" (excluded from the weighted health score, matching the existing checkManrsMembership pattern) when the underlying data source didn't actually respond. - Hijack monitoring: checkHijacksForAsn returned [] on DB error, indistinguishable from a genuine zero-prefix result. On a subscription's first monitoring cycle this permanently persisted an empty baseline, silently disabling leak detection forever while the UI kept showing it as active. Now returns null on failure; runHijackCheck skips the cycle entirely (retries next time) instead of locking in a bad baseline. Fixed 2 other call sites that would otherwise crash or persist null onto disk. - WHOIS cache: a transient failure across all 5 RIR sources (RIPE + 4 RDAP endpoints) was cached as "not found in any RIR database" for 24h, identical to a genuinely non-existent ASN. Added a 10min TTL for that specific case instead. - /api/relationships: a neighbourData fetch failure produced "0 upstreams/downstreams/peers", cached unconditionally for 10min. Applied the same 90s-on-suspicious-zero guard the codebase already uses for /api/validate (commit 4cf1673) but never applied here. Verification ------------ node --check on both .js files, tsc --noEmit clean on every touched TS module (remaining errors are 100% pre-existing, in the unrelated mcp-server subsystem), npm run build succeeds, isolated empirical tests for both the ASPA algorithm (3 hand-built scenarios with known-correct answers, cross-checked against draft text fetched live) and the fetchRipeStatCached routing fix, and a full local boot test reaching the exact same pre-existing sandbox-only crash point (missing PEERINGDB_API_KEY, unrelated to this commit) as a baseline boot before any of today's changes. Not deployed to Erik as part of this commit -- server.js and public/index.html there are untouched. |
||
|
Rene Fichtmueller
|
141911537d |
feat: wire up the 13 orphaned Fastify routes via internal API server proxy
The 2026-07-14 backend refactor (PR #1) moved 13 features' route logic out of server.js into src/features/*/routes.ts as Fastify plugins, and left "// Migrated to src/features/X/" comments behind -- but never actually built or started anything that served those plugins. Found a complete, unused Fastify app bootstrap already sitting at src/api/server.ts (added in an earlier commit, 5554c1a, alongside the BGP hijack/PDF-export/ASPA-adoption work) that imports and registers all 16 route modules, including 3 (pdf-export, aspa-adoption, hijack-alerts) with proper Postgres-backed implementations, tests, and PDF templates -- more capable than the puppeteer-based inline version kept from the production snapshot. Also confirmed there is no code anywhere that calls initializeDatabase() or startApiServer(): this Fastify server had never actually been run, not even once. What this commit does: - Fixes 6 real TypeScript compile errors blocking `tsc` from building the affected modules cleanly (all pre-existing, none introduced by this branch): - src/routes/hijack-alerts.ts: route generic was missing `Body` in its type param, so it didn't match the handler's own declared type - src/features/aspa-adoption/scheduler.ts: node-cron v4's ScheduledTask dropped `.destroy()` (relevant now that this branch already bumped node-cron 3->4 for the uuid CVE fix) -- `.stop()` alone is sufficient - src/features/{bgp-communities,rpki-history}/routes.ts: `response.json()` typed as `{}` under this @types/node version, cast to `any` to match this file's existing loose-typing style - src/features/pdf-export/cache-manager.ts: `NodeJS.Timer` -> `NodeJS.Timeout` (what setInterval/clearInterval actually use) - src/features/pdf-export/renderer.ts: this uses Playwright, not Puppeteer -- `timeout` isn't a page.pdf() option in Playwright, moved to page.setDefaultTimeout() before the call - Adds src/api/index.ts as the actual entry point (there wasn't one). Calls initializeDatabase() before dynamically importing ./server.js, because src/routes/hijack-alerts.ts calls getDatabase() at module load time -- a static top-level import of ./server would have crashed on startup before initializeDatabase() ever ran. Verified this boots cleanly and serves real responses (changelog-data, ix-matrix, rib/prefix graceful-503, bgp-communities with a live RIPE Stat call) in local testing. - Wires server.js to reverse-proxy the 11 genuinely-orphaned paths (submarine-cables, global-infra, communities, irr-audit, asset-expand, rpki-history, aspath, looking-glass, ix-matrix, changelog-data, rib/*, prefix-changes) to this new internal server via a plain http.request() proxy -- chosen over exposing a second Cloudflare Tunnel ingress rule so this stays a same-origin, internal-only change with no DNS/tunnel config to touch. Verified the proxy mechanism itself against a live instance of the new server. Deliberately did NOT proxy /api/webhooks, /api/hijacks, or /api/hijack-subscribe -- server.js already serves those paths inline (file-backed, live, working), and src/routes/hijack-alerts.ts registers the same path names against an empty Postgres table. Proxying would have silently shadowed working data with nothing. - Default port 3102, not 3100: checked Erik and found 3100 already bound by an unrelated docker-proxy container. 3102 is free and reads as "the second peercortex port" next to the existing 3101. - Adds a `peercortex-api` PM2 app to ecosystem.config.js on Erik (backed up first via rollback-standard.sh) -- config only, NOT started. Nothing is deployed to Erik as part of this commit: no dist/ upload, no `npm install` for fastify/pg on Erik, no `pm2 start`. server.js and public/index.html on Erik are untouched. |
||
|
Rene Fichtmueller
|
04bc8e3c4c |
chore: add .security-scan-allowlist for known false positives
Own product name/domain/deploy-path self-references, the maintainer's public contact address, the bogon-detection feature's hardcoded RFC 5735/6890 range constants, and one already-reviewed home-LAN DB_HOST default -- all previously confirmed false positives that forced a manual --no-verify judgment call on every single push to this repo. Verified against the actual scanner (~/.claude/hooks/lib/security-scan-core.sh): 0 findings remain. |
||
|
Rene Fichtmueller
|
3b978c4427 |
fix: remove unused critical-RCE dependency, bump node-cron to drop uuid vuln
- node-whois was declared in package.json but required/imported nowhere in the codebase (verified via repo-wide grep) -- pure dead weight that was dragging in a critical arbitrary-code-execution chain (underscore 1.5.2 via optimist, CVSS 9.8). No fixed version exists upstream (latest 2.1.3 still depends on the same vulnerable optimist/underscore), so the only real fix was removal. Zero behavior change since nothing called it. - node-cron 3.0.3 -> 4.6.0: only real usage is two plain cron.schedule() calls in src/features/hijack-alerts/retry-scheduler.ts and src/features/aspa-adoption/scheduler.ts -- API unchanged, low risk. 4.6.0 has zero runtime deps (drops the vulnerable uuid transitive dep) and ships its own TS types, so @types/node-cron was also removed to avoid duplicate/conflicting type declarations. - repository/bugs URLs in package.json corrected from the GitHub repo (deleted 2026-07-14) to the actual Gitea repo. 12 -> 6 vulnerabilities (6 critical/1 high/5 moderate -> 2 critical/1 high/3 moderate). Remaining 6 are entirely in the vitest/vite/esbuild dev-tooling chain (vite dev-server SSRF, GHSA-67mh-4wv8-2f99) -- not production-exposed, and fixing requires a vitest 2.x -> 4.x major bump that needs its own test-suite verification pass, not folded into this commit. tsc --noEmit confirms no new type errors from the node-cron/types change (remaining TS6133 unused-var warnings in src/sources/*.ts are pre-existing skeleton-source stubs, unrelated to this commit). |
||
|
Rene Fichtmueller
|
ceb49d0ab6 |
merge: reconcile prod-snapshot-2026-07-16 into main
Merges the production-only changes (Izzy PDF export via puppeteer,
Adoption Tracker overlay, audit scripts, deploy/utility scripts) that
had been living as uncommitted edits directly on Erik since before the
2026-07-14 backend refactor, on top of current main.
4 real conflicts, resolved by keeping whichever side is actually
functional rather than picking a side mechanically:
- package.json / package-lock.json: trivial union, kept both the
refactor's pg/playwright deps and the snapshot's puppeteer dep;
lockfile regenerated via `npm install --package-lock-only` rather
than hand-edited
- server.js top-of-file: additive, kept both the localDb require
(backend refactor) and the puppeteer lazy-loader (PDF export)
- server.js /api/health aspa_adoption block: main referenced
`aspaAdoptionHistory`, a variable that is never declared anywhere
in this file (confirmed by grep) -- a pre-existing dead reference
that would throw if this code path ever ran. Kept the snapshot's
working `aspaAdoptionDailyHistory`/`roaStore.count` version, which
matches the live /api/health output from production. Also fixed
the identical bug in the neighboring .catch() fallback block a few
lines down (same undefined variable, not part of the conflict
itself -- pre-existing on main, same fix applied for consistency).
- server.js hijack-subscribe/webhooks section (186 lines): main's
side was just a 2-line comment ("Migrated to
src/features/hijack-subscribe/"). Kept the snapshot's actual
working implementation instead, because server.js does not
require() or import anything from src/features/ or dist/ anywhere
-- confirmed by grep. The "migration" only ever happened on the
src/features/ side; server.js's own HTTP routing was never wired
up to call into it.
IMPORTANT CAVEAT, found while investigating the above and NOT fixed
here: server.js still has 13 other "// Migrated to src/features/..."
comments (bgp-communities, irr-audit, asset-expand, rpki-history,
aspath, looking-glass, ix-matrix, submarine-cables, global-infra,
hijack-alerts (Fastify), changelog, rib, prefix-changes) where the
same thing is true -- the route implementation was deleted from
server.js and never replaced with a working call into src/features/.
These weren't touched by this merge (git didn't flag them as
conflicts, because production's snapshot never touched those
sections either -- only main changed them, by deleting them). If
this server.js is ever deployed as the new production file as-is,
those 13 endpoints will silently 404 or fall through, exactly like
the hijack-subscribe one would have if I'd taken main's side. This
is a pre-existing gap in the 2026-07-14 refactor (PR #1), unrelated
to the Izzy/Adoption Tracker work, and needs a deliberate decision
(wire server.js to src/features/, or confirm those routes are meant
to live on a different process/port entirely) before this branch
should be treated as deployable.
`node --check server.js` passes. Not deployed to Erik as part of
this commit -- Erik's currently-live server.js/public/index.html
are untouched.
|
||
|
Rene Fichtmueller
|
2766872aad |
snapshot: preserve production-only changes (Izzy PDF export, adoption tracker, audit scripts)
Working tree was 3+ months ahead of git in uncommitted local edits, sitting on a detached HEAD at d3611a8 (2026-04-09), never reconciled with main. Committing as-is on its own branch, without touching main or the detached HEAD, to stop these files existing only on this one server: - public/index.html + server.js: PDF export add-on gated to client Izzy, full Adoption Tracker overlay (ASPA + IPv6 tabs) -- neither exists anywhere in git history on any branch until now - audit/: daily/rotating audit scripts + email report sender, previously untracked - scripts/: tunnel-cleanup.sh, refresh-peeringdb.sh, previously untracked - deploy-from-scp.sh, webhook-subs.json, hijack-alerts.json, aspa-adoption-history.json: previously untracked runtime/deploy state - public/public/: duplicate mirror of the HTML variants found alongside the real public/ dir, preserved as found No reconciliation with main attempted here -- that needs a deliberate pass, not a side effect of a backup commit. |